From 35d2d9dc567cfd52bd2377eae037df5418e505ff Mon Sep 17 00:00:00 2001 From: "sangwoo.ko" Date: Thu, 11 Jul 2024 10:08:51 +0900 Subject: [PATCH 1/9] Show site origin in file select dialog For security reason, we should show the site origin in the file select dialog --- app/brave_generated_resources.grd | 26 +++ browser/ui/BUILD.gn | 8 +- browser/ui/brave_browser.cc | 30 ++++ browser/ui/brave_browser.h | 4 + browser/ui/brave_file_select_helper.cc | 148 ++++++++++++++++++ browser/ui/brave_file_select_helper.h | 50 ++++++ .../ui/brave_file_select_helper_unittest.cc | 148 ++++++++++++++++++ .../browser/download/download_file_picker.cc | 42 +++++ 8 files changed, 455 insertions(+), 1 deletion(-) create mode 100644 browser/ui/brave_file_select_helper.cc create mode 100644 browser/ui/brave_file_select_helper.h create mode 100644 browser/ui/brave_file_select_helper_unittest.cc create mode 100644 chromium_src/chrome/browser/download/download_file_picker.cc diff --git a/app/brave_generated_resources.grd b/app/brave_generated_resources.grd index 95aa52ce6f6..a6d59441ad7 100644 --- a/app/brave_generated_resources.grd +++ b/app/brave_generated_resources.grd @@ -1299,6 +1299,32 @@ Or change later at $2brave://settings/ext Brave Player + + + + $1http://foo.com wants to open + + + An embedded page at $1http://foo.com wants to open + + + This page wants to open + + + An embedded page on this page wants to open + + + $1http://foo.com wants to save + + + An embedded page at $1http://foo.com wants to save + + + This page wants to save + + + An embedded page on this page wants to open + diff --git a/browser/ui/BUILD.gn b/browser/ui/BUILD.gn index 0d27f7b8cbc..4e1b8321a01 100644 --- a/browser/ui/BUILD.gn +++ b/browser/ui/BUILD.gn @@ -150,6 +150,8 @@ source_set("ui") { "brave_browser_content_setting_bubble_model_delegate.h", "brave_browser_window.cc", "brave_browser_window.h", + "brave_file_select_helper.cc", + "brave_file_select_helper.h", "brave_icon_with_badge_image_source.cc", "brave_icon_with_badge_image_source.h", "brave_layout_constants.cc", @@ -1336,9 +1338,13 @@ source_set("ui") { source_set("unit_tests") { if (!is_android) { testonly = true - sources = [ "brave_layout_constants_unittest.cc" ] + sources = [ + "brave_file_select_helper_unittest.cc", + "brave_layout_constants_unittest.cc", + ] deps = [ "//chrome/browser/ui", + "//components/javascript_dialogs", "//testing/gtest", "//ui/base:test_support", "//ui/gfx", diff --git a/browser/ui/brave_browser.cc b/browser/ui/brave_browser.cc index c1b6007495b..914a0f193e3 100644 --- a/browser/ui/brave_browser.cc +++ b/browser/ui/brave_browser.cc @@ -14,6 +14,7 @@ #include "base/functional/callback_helpers.h" #include "brave/browser/brave_browser_features.h" #include "brave/browser/ui/brave_browser_window.h" +#include "brave/browser/ui/brave_file_select_helper.h" #include "brave/browser/ui/tabs/brave_tab_prefs.h" #include "brave/browser/ui/tabs/features.h" #include "brave/components/constants/pref_names.h" @@ -27,7 +28,9 @@ #include "chrome/browser/ui/tabs/tab_strip_model_observer.h" #include "chrome/common/webui_url_constants.h" #include "components/prefs/pref_service.h" +#include "content/public/browser/file_select_listener.h" #include "content/public/common/url_constants.h" +#include "third_party/blink/public/mojom/choosers/file_chooser.mojom.h" #include "url/gurl.h" #if defined(TOOLKIT_VIEWS) @@ -128,6 +131,33 @@ void BraveBrowser::TabStripEmpty() { true, std::nullopt)); } +void BraveBrowser::RunFileChooser( + content::RenderFrameHost* render_frame_host, + scoped_refptr listener, + const blink::mojom::FileChooserParams& params) { +#if BUILDFLAG(IS_ANDROID) + Browser::RunFileChooser(render_frame_host, listener, params); +#else + auto new_params = params.Clone(); + if (new_params->title.empty()) { + // Fill title of file chooser with origin of the frame. + + // Note that save mode param is for PPAPI. 'Save As...' or downloading + // something doesn't reach here. They shows select file dialog from + // DownloadFilePicker::DownloadFilePicker directly. + // https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/public/mojom/choosers/file_chooser.mojom;l=27;drc=047c7dc4ee1ce908d7fea38ca063fa2f80f92c77 + + new_params->title = brave::GetFileSelectTitle( + content::WebContents::FromRenderFrameHost(render_frame_host), + render_frame_host->GetLastCommittedOrigin(), + params.mode == blink::mojom::FileChooserParams::Mode::kSave + ? brave::GetFileSelectResourceIDsForSave() + : brave::GetFileSelectResourceIDsForOpen()); + } + Browser::RunFileChooser(render_frame_host, listener, *new_params); +#endif +} + bool BraveBrowser::ShouldDisplayFavicon( content::WebContents* web_contents) const { // Override to not show favicon for NTP in tab. diff --git a/browser/ui/brave_browser.h b/browser/ui/brave_browser.h index 6ea5acb4cd4..f4e02d2b1e3 100644 --- a/browser/ui/brave_browser.h +++ b/browser/ui/brave_browser.h @@ -54,6 +54,10 @@ class BraveBrowser : public Browser { void OnTabClosing(content::WebContents* contents) override; void TabStripEmpty() override; + void RunFileChooser(content::RenderFrameHost* render_frame_host, + scoped_refptr listener, + const blink::mojom::FileChooserParams& params) override; + // Returns true when we should ask browser closing to users before handling // any warning/onbeforeunload handlers. bool ShouldAskForBrowserClosingBeforeHandlers(); diff --git a/browser/ui/brave_file_select_helper.cc b/browser/ui/brave_file_select_helper.cc new file mode 100644 index 00000000000..ff761be896f --- /dev/null +++ b/browser/ui/brave_file_select_helper.cc @@ -0,0 +1,148 @@ +/* Copyright (c) 2024 The Brave Authors. All rights reserved. + * This Source Code Form is subject to the terms of the Mozilla Public + * License, v. 2.0. If a copy of the MPL was not distributed with this file, + * You can obtain one at https://mozilla.org/MPL/2.0/. */ + +#include "brave/browser/ui/brave_file_select_helper.h" + +#include "base/i18n/rtl.h" +#include "base/no_destructor.h" +#include "brave/grit/brave_generated_resources.h" +#include "chrome/browser/profiles/profile.h" +#include "chrome/browser/ui/url_identity.h" +#include "components/url_formatter/elide_url.h" +#include "content/public/browser/web_contents.h" +#include "ui/base/l10n/l10n_util.h" +#include "url/gurl.h" +#include "url/origin.h" + +namespace brave { + +namespace { + +// If an origin is opaque but has a precursor, then returns the precursor +// origin. If the origin is not opaque, returns it unchanged. Unwrapping origins +// allows the dialog code to provide the user with a clearer picture of which +// page is actually showing the dialog. +url::Origin UnwrapOriginIfOpaque(const url::Origin& origin) { + if (!origin.opaque()) { + return origin; + } + + const url::SchemeHostPort& precursor = + origin.GetTupleOrPrecursorTupleIfOpaque(); + if (!precursor.IsValid()) { + return origin; + } + + return url::Origin::CreateFromNormalizedTuple( + precursor.scheme(), precursor.host(), precursor.port()); +} + +} // namespace + +std::u16string GetFileSelectTitle(content::WebContents* web_contents, + const url::Origin& alerting_frame_origin, + const SiteTitleResourceIDMap& resource_ids) { + // This implementation partially mirrors + // ChromeAppModalDialogManagerDelegate::GetTitle(). + // TODO(sko) It's hard to test this behavior is in sync at this moment. Even + // upstream tests aren't covering this. Need to figure out how we can test + // extension and isolated web app case. + Profile* profile = + Profile::FromBrowserContext(web_contents->GetBrowserContext()); + + UrlIdentity url_identity = UrlIdentity::CreateFromUrl( + profile, alerting_frame_origin.GetURL(), + /*allowed_types*/ + {UrlIdentity::Type::kDefault, UrlIdentity::Type::kFile, + UrlIdentity::Type::kIsolatedWebApp, UrlIdentity::Type::kChromeExtension}, + /*default_options*/ {.default_options = {}}); + + if (url_identity.type == UrlIdentity::Type::kChromeExtension) { + return url_identity.name; + } + + if (url_identity.type == UrlIdentity::Type::kIsolatedWebApp) { + return url_identity.name; + } + + const auto main_frame_origin = + web_contents->GetPrimaryMainFrame()->GetLastCommittedOrigin(); + return GetSiteFrameTitleForFileSelect( + GetSiteFrameTitleType(main_frame_origin, alerting_frame_origin), + alerting_frame_origin, resource_ids); +} + +std::u16string GetSiteFrameTitleForFileSelect( + SiteFrameTitleType type, + const url::Origin& alerting_frame_origin, + const SiteTitleResourceIDMap& resource_ids) { + if (type == SiteFrameTitleType::kStandardSameOrigin || + type == SiteFrameTitleType::kStandardDifferentOrigin) { + std::u16string origin_string = + url_formatter::FormatOriginForSecurityDisplay( + UnwrapOriginIfOpaque(alerting_frame_origin), + url_formatter::SchemeDisplay::OMIT_HTTP_AND_HTTPS); + return l10n_util::GetStringFUTF16( + resource_ids.at(type), + base::i18n::GetDisplayStringInLTRDirectionality(origin_string)); + } + + return l10n_util::GetStringUTF16(resource_ids.at(type)); +} + +SiteFrameTitleType GetSiteFrameTitleType( + const url::Origin& main_frame_origin, + const url::Origin& alerting_frame_origin) { + // This implementation mirrors `AppModalDialogManager::GetSiteFrameTitle()`. + // We have a test to check if the two implementations are in sync. + // - BraveFileSelectHelperUnitTest.GetSiteFrameTitleType_InSyncWithUpstream. + const url::Origin unwrapped_main_frame_origin = + UnwrapOriginIfOpaque(main_frame_origin); + const url::Origin unwrapped_alerting_frame_origin = + UnwrapOriginIfOpaque(alerting_frame_origin); + + const bool is_same_origin_as_main_frame = + unwrapped_alerting_frame_origin.IsSameOriginWith( + unwrapped_main_frame_origin); + if (unwrapped_alerting_frame_origin.GetURL().IsStandard() && + !unwrapped_alerting_frame_origin.GetURL().SchemeIsFile()) { + return is_same_origin_as_main_frame + ? SiteFrameTitleType::kStandardSameOrigin + : SiteFrameTitleType::kStandardDifferentOrigin; + } + return is_same_origin_as_main_frame + ? SiteFrameTitleType::kNonStandardSameOrigin + : SiteFrameTitleType::kNonStandardDifferentOrigin; +} + +const SiteTitleResourceIDMap& GetFileSelectResourceIDsForOpen() { + static const base::NoDestructor + kResourcesForOpen( + {{brave::SiteFrameTitleType::kStandardSameOrigin, + IDS_FILE_SELECT_OPEN_TITLE}, + {brave::SiteFrameTitleType::kStandardDifferentOrigin, + IDS_FILE_SELECT_OPEN_TITLE_IFRAME}, + {brave::SiteFrameTitleType::kNonStandardSameOrigin, + IDS_FILE_SELECT_OPEN_TITLE_NONSTANDARD_URL}, + {brave::SiteFrameTitleType::kNonStandardDifferentOrigin, + IDS_FILE_SELECT_OPEN_TITLE_NONSTANDARD_URL_IFRAME}}); + return *kResourcesForOpen; +} + +const SiteTitleResourceIDMap& GetFileSelectResourceIDsForSave() { + static const base::NoDestructor + kResourcesForSave( + {{brave::SiteFrameTitleType::kStandardSameOrigin, + IDS_FILE_SELECT_SAVE_TITLE}, + {brave::SiteFrameTitleType::kStandardDifferentOrigin, + IDS_FILE_SELECT_SAVE_TITLE_IFRAME}, + {brave::SiteFrameTitleType::kNonStandardSameOrigin, + IDS_FILE_SELECT_SAVE_TITLE_NONSTANDARD_URL}, + {brave::SiteFrameTitleType::kNonStandardDifferentOrigin, + IDS_FILE_SELECT_SAVE_TITLE_NONSTANDARD_URL_IFRAME}}); + return *kResourcesForSave; +} + +} // namespace brave diff --git a/browser/ui/brave_file_select_helper.h b/browser/ui/brave_file_select_helper.h new file mode 100644 index 00000000000..d29eb24604f --- /dev/null +++ b/browser/ui/brave_file_select_helper.h @@ -0,0 +1,50 @@ +/* Copyright (c) 2024 The Brave Authors. All rights reserved. + * This Source Code Form is subject to the terms of the Mozilla Public + * License, v. 2.0. If a copy of the MPL was not distributed with this file, + * You can obtain one at https://mozilla.org/MPL/2.0/. */ + +#ifndef BRAVE_BROWSER_UI_BRAVE_FILE_SELECT_HELPER_H_ +#define BRAVE_BROWSER_UI_BRAVE_FILE_SELECT_HELPER_H_ + +#include + +#include "base/containers/flat_map.h" + +namespace content { +class WebContents; +} // namespace content + +namespace url { +class Origin; +} // namespace url + +namespace brave { + +enum class SiteFrameTitleType { + kStandardSameOrigin, + kStandardDifferentOrigin, + kNonStandardSameOrigin, + kNonStandardDifferentOrigin +}; + +using SiteTitleResourceIDMap = base::flat_map; + +std::u16string GetFileSelectTitle(content::WebContents* contents, + const url::Origin& alerting_frame_origin, + const SiteTitleResourceIDMap& resource_ids); + +std::u16string GetSiteFrameTitleForFileSelect( + SiteFrameTitleType type, + const url::Origin& alerting_frame_origin, + const SiteTitleResourceIDMap& resource_ids); + +SiteFrameTitleType GetSiteFrameTitleType( + const url::Origin& main_frame_origin, + const url::Origin& alerting_frame_origin); + +const SiteTitleResourceIDMap& GetFileSelectResourceIDsForOpen(); +const SiteTitleResourceIDMap& GetFileSelectResourceIDsForSave(); + +} // namespace brave + +#endif // BRAVE_BROWSER_UI_BRAVE_FILE_SELECT_HELPER_H_ diff --git a/browser/ui/brave_file_select_helper_unittest.cc b/browser/ui/brave_file_select_helper_unittest.cc new file mode 100644 index 00000000000..0b3556c0994 --- /dev/null +++ b/browser/ui/brave_file_select_helper_unittest.cc @@ -0,0 +1,148 @@ +/* Copyright (c) 2024 The Brave Authors. All rights reserved. + * This Source Code Form is subject to the terms of the Mozilla Public + * License, v. 2.0. If a copy of the MPL was not distributed with this file, + * You can obtain one at https://mozilla.org/MPL/2.0/. */ + +#include "brave/browser/ui/brave_file_select_helper.h" + +#include "base/strings/utf_string_conversions.h" +#include "components/javascript_dialogs/app_modal_dialog_manager.h" +#include "components/strings/grit/components_strings.h" +#include "testing/gtest/include/gtest/gtest.h" +#include "url/origin.h" + +namespace { +constexpr struct Case { + // The name of the test case. + const char* case_name; + + // The URL of the main frame of the page. + const char* main_frame_url; + + // Whether the main frame is alerting. + bool is_main_frame; + + // If `is_main_frame` is false, the URL of the alerting frame of the page. + const char* alerting_frame_url; + + // The expected title for the alert. + const char* expected; +} kCases[] = { + // Standard main frame alert. + {"standard", "http://foo.com/", true, "", "foo.com wants to open"}, + + // Subframe alert from the same origin. + {"subframe same origin", "http://foo.com/1", false, "http://foo.com/2", + "foo.com wants to open"}, + // Subframe alert from a different origin. + {"subframe different origin", "http://foo.com/", false, "http://bar.com/", + "An embedded page at bar.com wants to open"}, + + // file: + // - main frame: + {"file main frame", "file:///path/to/page.html", true, "", + "This page wants to open"}, + // - subframe: + {"file subframe", "http://foo.com/", false, "file:///path/to/page.html", + "An embedded page on this page wants to open"}, + + // data: + // /!\ NOTE that this is for data URLs entered directly in the omnibox. + // For pages that generate frames with data URLs, see the browsertest. + // - main frame: + {"data main frame", "data:blahblah", true, "", "This page wants to open"}, + // - subframe: + {"data subframe", "http://foo.com/", false, "data:blahblah", + "An embedded page on this page wants to open"}, + + // javascript: + // /!\ NOTE that this is for javascript URLs entered directly in the + // omnibox. For pages that generate frames with javascript URLs, see the + // browsertest. + // - main frame: + {"javascript main frame", "javascript:abc", true, "", + "This page wants to open"}, + // - subframe: + {"javascript subframe", "http://foo.com/", false, "javascript:abc", + "An embedded page on this page wants to open"}, + + // about: + // /!\ NOTE that this is for about:blank URLs entered directly in the + // omnibox. For pages that generate frames with about:blank URLs, see the + // browsertest. + // - main frame: + {"about main frame", "about:blank", true, "", "This page wants to open"}, + // - subframe: + {"about subframe", "http://foo.com/", false, "about:blank", + "An embedded page on this page wants to open"}, + + // blob: + // - main frame: + {"blob main frame", + "blob:http://foo.com/66666666-6666-6666-6666-666666666666", true, "", + "foo.com wants to open"}, + // - subframe: + {"blob subframe", "http://bar.com/", false, + "blob:http://foo.com/66666666-6666-6666-6666-666666666666", + "An embedded page at foo.com wants to open"}, + + // filesystem: + // - main frame: + {"filesystem main frame", "filesystem:http://foo.com/bar.html", true, "", + "foo.com wants to open"}, + // - subframe: + {"filesystem subframe", "http://bar.com/", false, + "filesystem:http://foo.com/bar.html", + "An embedded page at foo.com wants to open"}, +}; + +} // namespace + +TEST(BraveFileSelectHelperUnitTest, GetSiteFrameTitle_InSyncWithUpstream) { + // Checks if our implementation is in sync with upstream. + const brave::SiteTitleResourceIDMap resource_ids({ + {brave::SiteFrameTitleType::kStandardSameOrigin, + IDS_JAVASCRIPT_MESSAGEBOX_TITLE}, + {brave::SiteFrameTitleType::kStandardDifferentOrigin, + IDS_JAVASCRIPT_MESSAGEBOX_TITLE_IFRAME}, + {brave::SiteFrameTitleType::kNonStandardSameOrigin, + IDS_JAVASCRIPT_MESSAGEBOX_TITLE_NONSTANDARD_URL}, + {brave::SiteFrameTitleType::kNonStandardDifferentOrigin, + IDS_JAVASCRIPT_MESSAGEBOX_TITLE_NONSTANDARD_URL_IFRAME}, + }); + + for (const auto& test_case : kCases) { + SCOPED_TRACE(test_case.case_name); + + url::Origin main_frame_origin = + url::Origin::Create(GURL(test_case.main_frame_url)); + url::Origin alerting_frame_origin = + test_case.is_main_frame + ? main_frame_origin + : url::Origin::Create(GURL(test_case.alerting_frame_url)); + EXPECT_EQ(javascript_dialogs::AppModalDialogManager::GetSiteFrameTitle( + main_frame_origin, alerting_frame_origin), + brave::GetSiteFrameTitleForFileSelect( + brave::GetSiteFrameTitleType(main_frame_origin, + alerting_frame_origin), + alerting_frame_origin, resource_ids)); + } +} + +TEST(BraveFileSelectHelperUnitTest, GetSiteFrameTitleForFileSelect) { + for (const auto& test_case : kCases) { + SCOPED_TRACE(test_case.case_name); + url::Origin main_frame_origin = + url::Origin::Create(GURL(test_case.main_frame_url)); + url::Origin alerting_frame_origin = + test_case.is_main_frame + ? main_frame_origin + : url::Origin::Create(GURL(test_case.alerting_frame_url)); + EXPECT_EQ( + base::UTF8ToUTF16(test_case.expected), + brave::GetSiteFrameTitleForFileSelect( + brave::GetSiteFrameTitleType(main_frame_origin, + alerting_frame_origin), + alerting_frame_origin, brave::GetFileSelectResourceIDsForOpen())); + } +} diff --git a/chromium_src/chrome/browser/download/download_file_picker.cc b/chromium_src/chrome/browser/download/download_file_picker.cc new file mode 100644 index 00000000000..3b948b30ce2 --- /dev/null +++ b/chromium_src/chrome/browser/download/download_file_picker.cc @@ -0,0 +1,42 @@ +/* Copyright (c) 2024 The Brave Authors. All rights reserved. + * This Source Code Form is subject to the terms of the Mozilla Public + * License, v. 2.0. If a copy of the MPL was not distributed with this file, + * You can obtain one at https://mozilla.org/MPL/2.0/. */ + +#include + +#include "content/public/browser/render_frame_host.h" +#include "content/public/browser/web_contents.h" +#include "third_party/blink/public/mojom/choosers/file_chooser.mojom.h" +#include "ui/shell_dialogs/select_file_dialog.h" + +#if !BUILDFLAG(IS_ANDROID) +#include "brave/browser/ui/brave_file_select_helper.h" +#endif + +namespace { + +std::u16string GetTitle(content::RenderFrameHost* render_frame_host, + const std::u16string& original_title) { +#if BUILDFLAG(IS_ANDROID) + return original_title; +#else + return brave::GetFileSelectTitle( + content::WebContents::FromRenderFrameHost(render_frame_host), + render_frame_host->GetLastCommittedOrigin(), + brave::GetFileSelectResourceIDsForSave()); +#endif +} + +} // namespace + +// Override title of the file select dialog for downloads. +#define SelectFile(type, title, default_path, file_types, file_type_index, \ + default_extension, owning_window, params, caller) \ + SelectFile(type, GetTitle(render_frame_host, title), default_path, \ + file_types, file_type_index, default_extension, owning_window, \ + params, caller) + +#include "src/chrome/browser/download/download_file_picker.cc" + +#undef SelectFile From 0097c66e77a94f0ff3ff8e73a81e0a0994a3def3 Mon Sep 17 00:00:00 2001 From: sangwoo Date: Tue, 16 Jul 2024 09:02:56 +0900 Subject: [PATCH 2/9] Pass title params to file select dialog on Windows --- .../shell_dialogs/execute_select_file_win.cc | 22 ++++++++++ ...l_dialogs-execute_select_file_win.cc.patch | 44 +++++++++++++++++++ 2 files changed, 66 insertions(+) create mode 100644 chromium_src/ui/shell_dialogs/execute_select_file_win.cc create mode 100644 patches/ui-shell_dialogs-execute_select_file_win.cc.patch diff --git a/chromium_src/ui/shell_dialogs/execute_select_file_win.cc b/chromium_src/ui/shell_dialogs/execute_select_file_win.cc new file mode 100644 index 00000000000..65ede1721a0 --- /dev/null +++ b/chromium_src/ui/shell_dialogs/execute_select_file_win.cc @@ -0,0 +1,22 @@ +/* Copyright (c) 2024 The Brave Authors. All rights reserved. + * This Source Code Form is subject to the terms of the Mozilla Public + * License, v. 2.0. If a copy of the MPL was not distributed with this file, + * You can obtain one at https://mozilla.org/MPL/2.0/. */ + +#define BRAVE_EXECUTE_SELECT_SINGLE_FILE \ + return RunOpenFileDialog(owner, title, std::u16string(), default_path, \ + filter, 0, filter_index, paths); + +#define BRAVE_EXECUTE_SELECT_MULTIPLE_FILE \ + return RunOpenFileDialog(owner, title, std::u16string(), default_path, \ + filter, dialog_options, filter_index, paths); + +#define BRAVE_EXECUTE_SAVE_FILE \ + return RunSaveFileDialog(owner, title, default_path, filter, dialog_options, \ + def_ext, filter_index, path); + +#include "src/ui/shell_dialogs/execute_select_file_win.cc" + +#undef BRAVE_EXECUTE_SAVE_FILE +#undef BRAVE_EXECUTE_SELECT_MULTIPLE_FILE +#undef BRAVE_EXECUTE_SELECT_SINGLE_FILE diff --git a/patches/ui-shell_dialogs-execute_select_file_win.cc.patch b/patches/ui-shell_dialogs-execute_select_file_win.cc.patch new file mode 100644 index 00000000000..f0f1946f9ed --- /dev/null +++ b/patches/ui-shell_dialogs-execute_select_file_win.cc.patch @@ -0,0 +1,44 @@ +diff --git a/ui/shell_dialogs/execute_select_file_win.cc b/ui/shell_dialogs/execute_select_file_win.cc +index 5ef377bcbd69f2f2160aeed6ec4aa9fe3f20275c..ef56ddbe38b42410d7d6571796e025f3059c9c1a 100644 +--- a/ui/shell_dialogs/execute_select_file_win.cc ++++ b/ui/shell_dialogs/execute_select_file_win.cc +@@ -362,6 +362,7 @@ bool ExecuteSelectSingleFile(HWND owner, + std::vector* paths) { + // Note: The title is not passed down for historical reasons. + // TODO(pmonette): Figure out if it's a worthwhile improvement. ++ BRAVE_EXECUTE_SELECT_SINGLE_FILE + return RunOpenFileDialog(owner, std::u16string(), std::u16string(), + default_path, filter, 0, filter_index, paths); + } +@@ -376,12 +377,14 @@ bool ExecuteSelectMultipleFile(HWND owner, + + // Note: The title is not passed down for historical reasons. + // TODO(pmonette): Figure out if it's a worthwhile improvement. ++ BRAVE_EXECUTE_SELECT_MULTIPLE_FILE + return RunOpenFileDialog(owner, std::u16string(), std::u16string(), + default_path, filter, dialog_options, filter_index, + paths); + } + + bool ExecuteSaveFile(HWND owner, ++ const std::u16string& title, + const base::FilePath& default_path, + const std::vector& filter, + const std::wstring& def_ext, +@@ -396,6 +399,7 @@ bool ExecuteSaveFile(HWND owner, + + // Note: The title is not passed down for historical reasons. + // TODO(pmonette): Figure out if it's a worthwhile improvement. ++ BRAVE_EXECUTE_SAVE_FILE + return RunSaveFileDialog(owner, std::u16string(), default_path, filter, + dialog_options, def_ext, filter_index, path); + } +@@ -421,7 +425,7 @@ void ExecuteSelectFile( + break; + case SelectFileDialog::SELECT_SAVEAS_FILE: { + base::FilePath path; +- if (ExecuteSaveFile(owner, default_path, filter, default_extension, ++ if (ExecuteSaveFile(owner, title, default_path, filter, default_extension, + &file_type_index, &path)) { + paths.push_back(std::move(path)); + } From 5f875d3f8f2f012d80766e1a545cf2f7db6f4971 Mon Sep 17 00:00:00 2001 From: "sangwoo.ko" Date: Tue, 16 Jul 2024 09:14:12 +0900 Subject: [PATCH 3/9] Move test cases into actual tests --- .../ui/brave_file_select_helper_unittest.cc | 239 +++++++++++------- 1 file changed, 152 insertions(+), 87 deletions(-) diff --git a/browser/ui/brave_file_select_helper_unittest.cc b/browser/ui/brave_file_select_helper_unittest.cc index 0b3556c0994..a4428a2bf9d 100644 --- a/browser/ui/brave_file_select_helper_unittest.cc +++ b/browser/ui/brave_file_select_helper_unittest.cc @@ -11,94 +11,76 @@ #include "testing/gtest/include/gtest/gtest.h" #include "url/origin.h" -namespace { -constexpr struct Case { - // The name of the test case. - const char* case_name; - - // The URL of the main frame of the page. - const char* main_frame_url; - - // Whether the main frame is alerting. - bool is_main_frame; - - // If `is_main_frame` is false, the URL of the alerting frame of the page. - const char* alerting_frame_url; - - // The expected title for the alert. - const char* expected; -} kCases[] = { - // Standard main frame alert. - {"standard", "http://foo.com/", true, "", "foo.com wants to open"}, - - // Subframe alert from the same origin. - {"subframe same origin", "http://foo.com/1", false, "http://foo.com/2", - "foo.com wants to open"}, - // Subframe alert from a different origin. - {"subframe different origin", "http://foo.com/", false, "http://bar.com/", - "An embedded page at bar.com wants to open"}, - - // file: - // - main frame: - {"file main frame", "file:///path/to/page.html", true, "", - "This page wants to open"}, - // - subframe: - {"file subframe", "http://foo.com/", false, "file:///path/to/page.html", - "An embedded page on this page wants to open"}, - - // data: - // /!\ NOTE that this is for data URLs entered directly in the omnibox. - // For pages that generate frames with data URLs, see the browsertest. - // - main frame: - {"data main frame", "data:blahblah", true, "", "This page wants to open"}, - // - subframe: - {"data subframe", "http://foo.com/", false, "data:blahblah", - "An embedded page on this page wants to open"}, - - // javascript: - // /!\ NOTE that this is for javascript URLs entered directly in the - // omnibox. For pages that generate frames with javascript URLs, see the - // browsertest. - // - main frame: - {"javascript main frame", "javascript:abc", true, "", - "This page wants to open"}, - // - subframe: - {"javascript subframe", "http://foo.com/", false, "javascript:abc", - "An embedded page on this page wants to open"}, - - // about: - // /!\ NOTE that this is for about:blank URLs entered directly in the - // omnibox. For pages that generate frames with about:blank URLs, see the - // browsertest. - // - main frame: - {"about main frame", "about:blank", true, "", "This page wants to open"}, - // - subframe: - {"about subframe", "http://foo.com/", false, "about:blank", - "An embedded page on this page wants to open"}, - - // blob: - // - main frame: - {"blob main frame", - "blob:http://foo.com/66666666-6666-6666-6666-666666666666", true, "", - "foo.com wants to open"}, - // - subframe: - {"blob subframe", "http://bar.com/", false, - "blob:http://foo.com/66666666-6666-6666-6666-666666666666", - "An embedded page at foo.com wants to open"}, - - // filesystem: - // - main frame: - {"filesystem main frame", "filesystem:http://foo.com/bar.html", true, "", - "foo.com wants to open"}, - // - subframe: - {"filesystem subframe", "http://bar.com/", false, - "filesystem:http://foo.com/bar.html", - "An embedded page at foo.com wants to open"}, -}; - -} // namespace - TEST(BraveFileSelectHelperUnitTest, GetSiteFrameTitle_InSyncWithUpstream) { + constexpr struct Case { + // The name of the test case. + const char* case_name; + + // The URL of the main frame of the page. + const char* main_frame_url; + + // Whether the main frame is alerting. + bool is_main_frame; + + // If `is_main_frame` is false, the URL of the alerting frame of the page. + const char* alerting_frame_url; + } kCases[] = { + // Standard main frame alert. + {"standard", "http://foo.com/", true, ""}, + + // Subframe alert from the same origin. + {"subframe same origin", "http://foo.com/1", false, "http://foo.com/2"}, + // Subframe alert from a different origin. + {"subframe different origin", "http://foo.com/", false, + "http://bar.com/"}, + + // file: + // - main frame: + {"file main frame", "file:///path/to/page.html", true, ""}, + // - subframe: + {"file subframe", "http://foo.com/", false, "file:///path/to/page.html"}, + + // data: + // /!\ NOTE that this is for data URLs entered directly in the omnibox. + // For pages that generate frames with data URLs, see the browsertest. + // - main frame: + {"data main frame", "data:blahblah", true, ""}, + // - subframe: + {"data subframe", "http://foo.com/", false, "data:blahblah"}, + + // javascript: + // /!\ NOTE that this is for javascript URLs entered directly in the + // omnibox. For pages that generate frames with javascript URLs, see the + // browsertest. + // - main frame: + {"javascript main frame", "javascript:abc", true, ""}, + // - subframe: + {"javascript subframe", "http://foo.com/", false, "javascript:abc"}, + + // about: + // /!\ NOTE that this is for about:blank URLs entered directly in the + // omnibox. For pages that generate frames with about:blank URLs, see the + // browsertest. + // - main frame: + {"about main frame", "about:blank", true, ""}, + // - subframe: + {"about subframe", "http://foo.com/", false, "about:blank"}, + + // blob: + // - main frame: + {"blob main frame", + "blob:http://foo.com/66666666-6666-6666-6666-666666666666", true, ""}, + // - subframe: + {"blob subframe", "http://bar.com/", false, + "blob:http://foo.com/66666666-6666-6666-6666-666666666666"}, + + // filesystem: + // - main frame: + {"filesystem main frame", "filesystem:http://foo.com/bar.html", true, ""}, + // - subframe: + {"filesystem subframe", "http://bar.com/", false, + "filesystem:http://foo.com/bar.html"}, + }; // Checks if our implementation is in sync with upstream. const brave::SiteTitleResourceIDMap resource_ids({ {brave::SiteFrameTitleType::kStandardSameOrigin, @@ -130,6 +112,89 @@ TEST(BraveFileSelectHelperUnitTest, GetSiteFrameTitle_InSyncWithUpstream) { } TEST(BraveFileSelectHelperUnitTest, GetSiteFrameTitleForFileSelect) { + constexpr struct Case { + // The name of the test case. + const char* case_name; + + // The URL of the main frame of the page. + const char* main_frame_url; + + // Whether the main frame is alerting. + bool is_main_frame; + + // If `is_main_frame` is false, the URL of the alerting frame of the page. + const char* alerting_frame_url; + + // The expected title for the alert. + const char* expected; + } kCases[] = { + // Standard main frame alert. + {"standard", "http://foo.com/", true, "", "foo.com wants to open"}, + + // Subframe alert from the same origin. + {"subframe same origin", "http://foo.com/1", false, "http://foo.com/2", + "foo.com wants to open"}, + // Subframe alert from a different origin. + {"subframe different origin", "http://foo.com/", false, "http://bar.com/", + "An embedded page at bar.com wants to open"}, + + // file: + // - main frame: + {"file main frame", "file:///path/to/page.html", true, "", + "This page wants to open"}, + // - subframe: + {"file subframe", "http://foo.com/", false, "file:///path/to/page.html", + "An embedded page on this page wants to open"}, + + // data: + // /!\ NOTE that this is for data URLs entered directly in the omnibox. + // For pages that generate frames with data URLs, see the browsertest. + // - main frame: + {"data main frame", "data:blahblah", true, "", "This page wants to open"}, + // - subframe: + {"data subframe", "http://foo.com/", false, "data:blahblah", + "An embedded page on this page wants to open"}, + + // javascript: + // /!\ NOTE that this is for javascript URLs entered directly in the + // omnibox. For pages that generate frames with javascript URLs, see the + // browsertest. + // - main frame: + {"javascript main frame", "javascript:abc", true, "", + "This page wants to open"}, + // - subframe: + {"javascript subframe", "http://foo.com/", false, "javascript:abc", + "An embedded page on this page wants to open"}, + + // about: + // /!\ NOTE that this is for about:blank URLs entered directly in the + // omnibox. For pages that generate frames with about:blank URLs, see the + // browsertest. + // - main frame: + {"about main frame", "about:blank", true, "", "This page wants to open"}, + // - subframe: + {"about subframe", "http://foo.com/", false, "about:blank", + "An embedded page on this page wants to open"}, + + // blob: + // - main frame: + {"blob main frame", + "blob:http://foo.com/66666666-6666-6666-6666-666666666666", true, "", + "foo.com wants to open"}, + // - subframe: + {"blob subframe", "http://bar.com/", false, + "blob:http://foo.com/66666666-6666-6666-6666-666666666666", + "An embedded page at foo.com wants to open"}, + + // filesystem: + // - main frame: + {"filesystem main frame", "filesystem:http://foo.com/bar.html", true, "", + "foo.com wants to open"}, + // - subframe: + {"filesystem subframe", "http://bar.com/", false, + "filesystem:http://foo.com/bar.html", + "An embedded page at foo.com wants to open"}, + }; for (const auto& test_case : kCases) { SCOPED_TRACE(test_case.case_name); url::Origin main_frame_origin = From b94d973f96df355fab4bb5194cb6ba27dd7dd17d Mon Sep 17 00:00:00 2001 From: "sangwoo.ko" Date: Wed, 17 Jul 2024 08:58:22 +0900 Subject: [PATCH 4/9] Update strings --- app/brave_generated_resources.grd | 6 +++--- browser/ui/brave_file_select_helper_unittest.cc | 6 +++--- 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/app/brave_generated_resources.grd b/app/brave_generated_resources.grd index a6d59441ad7..933b1a3dd03 100644 --- a/app/brave_generated_resources.grd +++ b/app/brave_generated_resources.grd @@ -1305,7 +1305,7 @@ Or change later at $2brave://settings/ext $1http://foo.com wants to open - An embedded page at $1http://foo.com wants to open + An embedded page on $1http://foo.com wants to open This page wants to open @@ -1317,13 +1317,13 @@ Or change later at $2brave://settings/ext $1http://foo.com wants to save - An embedded page at $1http://foo.com wants to save + An embedded page on $1http://foo.com wants to save This page wants to save - An embedded page on this page wants to open + An embedded page on this page wants to save diff --git a/browser/ui/brave_file_select_helper_unittest.cc b/browser/ui/brave_file_select_helper_unittest.cc index a4428a2bf9d..abbdf4844c0 100644 --- a/browser/ui/brave_file_select_helper_unittest.cc +++ b/browser/ui/brave_file_select_helper_unittest.cc @@ -136,7 +136,7 @@ TEST(BraveFileSelectHelperUnitTest, GetSiteFrameTitleForFileSelect) { "foo.com wants to open"}, // Subframe alert from a different origin. {"subframe different origin", "http://foo.com/", false, "http://bar.com/", - "An embedded page at bar.com wants to open"}, + "An embedded page on bar.com wants to open"}, // file: // - main frame: @@ -184,7 +184,7 @@ TEST(BraveFileSelectHelperUnitTest, GetSiteFrameTitleForFileSelect) { // - subframe: {"blob subframe", "http://bar.com/", false, "blob:http://foo.com/66666666-6666-6666-6666-666666666666", - "An embedded page at foo.com wants to open"}, + "An embedded page on foo.com wants to open"}, // filesystem: // - main frame: @@ -193,7 +193,7 @@ TEST(BraveFileSelectHelperUnitTest, GetSiteFrameTitleForFileSelect) { // - subframe: {"filesystem subframe", "http://bar.com/", false, "filesystem:http://foo.com/bar.html", - "An embedded page at foo.com wants to open"}, + "An embedded page on foo.com wants to open"}, }; for (const auto& test_case : kCases) { SCOPED_TRACE(test_case.case_name); From 234b448acfdcbe264ca3c1f58037e3aa8380ff14 Mon Sep 17 00:00:00 2001 From: "sangwoo.ko" Date: Thu, 18 Jul 2024 12:52:48 +0900 Subject: [PATCH 5/9] Update comment --- browser/ui/brave_browser.cc | 2 +- chromium_src/ui/shell_dialogs/execute_select_file_win.cc | 9 +++++++++ 2 files changed, 10 insertions(+), 1 deletion(-) diff --git a/browser/ui/brave_browser.cc b/browser/ui/brave_browser.cc index 914a0f193e3..0881cc28f2d 100644 --- a/browser/ui/brave_browser.cc +++ b/browser/ui/brave_browser.cc @@ -143,7 +143,7 @@ void BraveBrowser::RunFileChooser( // Fill title of file chooser with origin of the frame. // Note that save mode param is for PPAPI. 'Save As...' or downloading - // something doesn't reach here. They shows select file dialog from + // something doesn't reach here. They show 'select file dialog' from // DownloadFilePicker::DownloadFilePicker directly. // https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/public/mojom/choosers/file_chooser.mojom;l=27;drc=047c7dc4ee1ce908d7fea38ca063fa2f80f92c77 diff --git a/chromium_src/ui/shell_dialogs/execute_select_file_win.cc b/chromium_src/ui/shell_dialogs/execute_select_file_win.cc index 65ede1721a0..b398d40d4f1 100644 --- a/chromium_src/ui/shell_dialogs/execute_select_file_win.cc +++ b/chromium_src/ui/shell_dialogs/execute_select_file_win.cc @@ -3,14 +3,23 @@ * License, v. 2.0. If a copy of the MPL was not distributed with this file, * You can obtain one at https://mozilla.org/MPL/2.0/. */ +// Can be removed once +// https://chromium-review.googlesource.com/c/chromium/src/+/5711152 arrives in +// Brave. #define BRAVE_EXECUTE_SELECT_SINGLE_FILE \ return RunOpenFileDialog(owner, title, std::u16string(), default_path, \ filter, 0, filter_index, paths); +// Can be removed once +// https://chromium-review.googlesource.com/c/chromium/src/+/5711152 arrives in +// Brave. #define BRAVE_EXECUTE_SELECT_MULTIPLE_FILE \ return RunOpenFileDialog(owner, title, std::u16string(), default_path, \ filter, dialog_options, filter_index, paths); +// Can be removed once +// https://chromium-review.googlesource.com/c/chromium/src/+/5711152 arrives in +// Brave. #define BRAVE_EXECUTE_SAVE_FILE \ return RunSaveFileDialog(owner, title, default_path, filter, dialog_options, \ def_ext, filter_index, path); From dbc9407e842d79add95f3e00ed0c41c34680838d Mon Sep 17 00:00:00 2001 From: "sangwoo.ko" Date: Fri, 19 Jul 2024 09:13:18 +0900 Subject: [PATCH 6/9] brave_file_select_helper => brave_file_select_utils --- browser/ui/BUILD.gn | 6 +++--- browser/ui/brave_browser.cc | 2 +- ...ve_file_select_helper.cc => brave_file_select_utils.cc} | 2 +- ...rave_file_select_helper.h => brave_file_select_utils.h} | 6 +++--- ...per_unittest.cc => brave_file_select_utils_unittest.cc} | 7 +++---- .../chrome/browser/download/download_file_picker.cc | 2 +- 6 files changed, 12 insertions(+), 13 deletions(-) rename browser/ui/{brave_file_select_helper.cc => brave_file_select_utils.cc} (99%) rename browser/ui/{brave_file_select_helper.h => brave_file_select_utils.h} (89%) rename browser/ui/{brave_file_select_helper_unittest.cc => brave_file_select_utils_unittest.cc} (97%) diff --git a/browser/ui/BUILD.gn b/browser/ui/BUILD.gn index 4e1b8321a01..042621f9c1c 100644 --- a/browser/ui/BUILD.gn +++ b/browser/ui/BUILD.gn @@ -150,8 +150,8 @@ source_set("ui") { "brave_browser_content_setting_bubble_model_delegate.h", "brave_browser_window.cc", "brave_browser_window.h", - "brave_file_select_helper.cc", - "brave_file_select_helper.h", + "brave_file_select_utils.cc", + "brave_file_select_utils.h", "brave_icon_with_badge_image_source.cc", "brave_icon_with_badge_image_source.h", "brave_layout_constants.cc", @@ -1339,7 +1339,7 @@ source_set("unit_tests") { if (!is_android) { testonly = true sources = [ - "brave_file_select_helper_unittest.cc", + "brave_file_select_utils_unittest.cc", "brave_layout_constants_unittest.cc", ] deps = [ diff --git a/browser/ui/brave_browser.cc b/browser/ui/brave_browser.cc index 0881cc28f2d..7d5b2ee6b88 100644 --- a/browser/ui/brave_browser.cc +++ b/browser/ui/brave_browser.cc @@ -14,7 +14,7 @@ #include "base/functional/callback_helpers.h" #include "brave/browser/brave_browser_features.h" #include "brave/browser/ui/brave_browser_window.h" -#include "brave/browser/ui/brave_file_select_helper.h" +#include "brave/browser/ui/brave_file_select_utils.h" #include "brave/browser/ui/tabs/brave_tab_prefs.h" #include "brave/browser/ui/tabs/features.h" #include "brave/components/constants/pref_names.h" diff --git a/browser/ui/brave_file_select_helper.cc b/browser/ui/brave_file_select_utils.cc similarity index 99% rename from browser/ui/brave_file_select_helper.cc rename to browser/ui/brave_file_select_utils.cc index ff761be896f..6f9ac4a925d 100644 --- a/browser/ui/brave_file_select_helper.cc +++ b/browser/ui/brave_file_select_utils.cc @@ -3,7 +3,7 @@ * License, v. 2.0. If a copy of the MPL was not distributed with this file, * You can obtain one at https://mozilla.org/MPL/2.0/. */ -#include "brave/browser/ui/brave_file_select_helper.h" +#include "brave/browser/ui/brave_file_select_utils.h" #include "base/i18n/rtl.h" #include "base/no_destructor.h" diff --git a/browser/ui/brave_file_select_helper.h b/browser/ui/brave_file_select_utils.h similarity index 89% rename from browser/ui/brave_file_select_helper.h rename to browser/ui/brave_file_select_utils.h index d29eb24604f..dabae5f244a 100644 --- a/browser/ui/brave_file_select_helper.h +++ b/browser/ui/brave_file_select_utils.h @@ -3,8 +3,8 @@ * License, v. 2.0. If a copy of the MPL was not distributed with this file, * You can obtain one at https://mozilla.org/MPL/2.0/. */ -#ifndef BRAVE_BROWSER_UI_BRAVE_FILE_SELECT_HELPER_H_ -#define BRAVE_BROWSER_UI_BRAVE_FILE_SELECT_HELPER_H_ +#ifndef BRAVE_BROWSER_UI_BRAVE_FILE_SELECT_UTILS_H_ +#define BRAVE_BROWSER_UI_BRAVE_FILE_SELECT_UTILS_H_ #include @@ -47,4 +47,4 @@ const SiteTitleResourceIDMap& GetFileSelectResourceIDsForSave(); } // namespace brave -#endif // BRAVE_BROWSER_UI_BRAVE_FILE_SELECT_HELPER_H_ +#endif // BRAVE_BROWSER_UI_BRAVE_FILE_SELECT_UTILS_H_ diff --git a/browser/ui/brave_file_select_helper_unittest.cc b/browser/ui/brave_file_select_utils_unittest.cc similarity index 97% rename from browser/ui/brave_file_select_helper_unittest.cc rename to browser/ui/brave_file_select_utils_unittest.cc index abbdf4844c0..3effa709408 100644 --- a/browser/ui/brave_file_select_helper_unittest.cc +++ b/browser/ui/brave_file_select_utils_unittest.cc @@ -3,15 +3,14 @@ * License, v. 2.0. If a copy of the MPL was not distributed with this file, * You can obtain one at https://mozilla.org/MPL/2.0/. */ -#include "brave/browser/ui/brave_file_select_helper.h" - #include "base/strings/utf_string_conversions.h" +#include "brave/browser/ui/brave_file_select_utils.h" #include "components/javascript_dialogs/app_modal_dialog_manager.h" #include "components/strings/grit/components_strings.h" #include "testing/gtest/include/gtest/gtest.h" #include "url/origin.h" -TEST(BraveFileSelectHelperUnitTest, GetSiteFrameTitle_InSyncWithUpstream) { +TEST(BraveFileSelectUtilsUnitTest, GetSiteFrameTitle_InSyncWithUpstream) { constexpr struct Case { // The name of the test case. const char* case_name; @@ -111,7 +110,7 @@ TEST(BraveFileSelectHelperUnitTest, GetSiteFrameTitle_InSyncWithUpstream) { } } -TEST(BraveFileSelectHelperUnitTest, GetSiteFrameTitleForFileSelect) { +TEST(BraveFileSelectUtilsUnitTest, GetSiteFrameTitleForFileSelect) { constexpr struct Case { // The name of the test case. const char* case_name; diff --git a/chromium_src/chrome/browser/download/download_file_picker.cc b/chromium_src/chrome/browser/download/download_file_picker.cc index 3b948b30ce2..e6343350f6d 100644 --- a/chromium_src/chrome/browser/download/download_file_picker.cc +++ b/chromium_src/chrome/browser/download/download_file_picker.cc @@ -11,7 +11,7 @@ #include "ui/shell_dialogs/select_file_dialog.h" #if !BUILDFLAG(IS_ANDROID) -#include "brave/browser/ui/brave_file_select_helper.h" +#include "brave/browser/ui/brave_file_select_utils.h" #endif namespace { From 13732de9f8c57d5af56b191fe584fa4cc8dc4d19 Mon Sep 17 00:00:00 2001 From: "sangwoo.ko" Date: Fri, 19 Jul 2024 09:31:14 +0900 Subject: [PATCH 7/9] Add comment to enum --- browser/ui/brave_file_select_utils.h | 17 +++++++++++++---- browser/ui/brave_file_select_utils_unittest.cc | 3 ++- 2 files changed, 15 insertions(+), 5 deletions(-) diff --git a/browser/ui/brave_file_select_utils.h b/browser/ui/brave_file_select_utils.h index dabae5f244a..1aa0e5cdb48 100644 --- a/browser/ui/brave_file_select_utils.h +++ b/browser/ui/brave_file_select_utils.h @@ -20,11 +20,20 @@ class Origin; namespace brave { +// This enum values are used to determine the title of the file select dialog. +// Basically it mirrors std::u16string +// AppModalDialogManager::GetSiteFrameTitle() implementation. enum class SiteFrameTitleType { - kStandardSameOrigin, - kStandardDifferentOrigin, - kNonStandardSameOrigin, - kNonStandardDifferentOrigin + kStandardSameOrigin, // alerting frame has http(s) scheme and has the same + // origin with main frame + kStandardDifferentOrigin, // alerting frame http(s) scheme and has a + // different origin with main frame + kNonStandardSameOrigin, // alerting frame has other schemes (e.g. file, + // data, javascript) and has the same origin with + // main frame + kNonStandardDifferentOrigin // alerting frame has other schemes (e.g. file, + // data, javascript) and has a different origin + // with main frame }; using SiteTitleResourceIDMap = base::flat_map; diff --git a/browser/ui/brave_file_select_utils_unittest.cc b/browser/ui/brave_file_select_utils_unittest.cc index 3effa709408..fbce4a85c74 100644 --- a/browser/ui/brave_file_select_utils_unittest.cc +++ b/browser/ui/brave_file_select_utils_unittest.cc @@ -3,8 +3,9 @@ * License, v. 2.0. If a copy of the MPL was not distributed with this file, * You can obtain one at https://mozilla.org/MPL/2.0/. */ -#include "base/strings/utf_string_conversions.h" #include "brave/browser/ui/brave_file_select_utils.h" + +#include "base/strings/utf_string_conversions.h" #include "components/javascript_dialogs/app_modal_dialog_manager.h" #include "components/strings/grit/components_strings.h" #include "testing/gtest/include/gtest/gtest.h" From 505740dc702596f0a6acd346d65245cbf32e0e91 Mon Sep 17 00:00:00 2001 From: "sangwoo.ko" Date: Fri, 19 Jul 2024 09:35:41 +0900 Subject: [PATCH 8/9] Add tests for save and set default locale --- browser/ui/BUILD.gn | 1 + .../ui/brave_file_select_utils_unittest.cc | 110 +++++++++++++++++- 2 files changed, 110 insertions(+), 1 deletion(-) diff --git a/browser/ui/BUILD.gn b/browser/ui/BUILD.gn index 042621f9c1c..d38389f7c70 100644 --- a/browser/ui/BUILD.gn +++ b/browser/ui/BUILD.gn @@ -1343,6 +1343,7 @@ source_set("unit_tests") { "brave_layout_constants_unittest.cc", ] deps = [ + "//brave/components/l10n/common:test_support", "//chrome/browser/ui", "//components/javascript_dialogs", "//testing/gtest", diff --git a/browser/ui/brave_file_select_utils_unittest.cc b/browser/ui/brave_file_select_utils_unittest.cc index fbce4a85c74..0b478b3406a 100644 --- a/browser/ui/brave_file_select_utils_unittest.cc +++ b/browser/ui/brave_file_select_utils_unittest.cc @@ -6,6 +6,7 @@ #include "brave/browser/ui/brave_file_select_utils.h" #include "base/strings/utf_string_conversions.h" +#include "brave/components/l10n/common/test/scoped_default_locale.h" #include "components/javascript_dialogs/app_modal_dialog_manager.h" #include "components/strings/grit/components_strings.h" #include "testing/gtest/include/gtest/gtest.h" @@ -111,7 +112,7 @@ TEST(BraveFileSelectUtilsUnitTest, GetSiteFrameTitle_InSyncWithUpstream) { } } -TEST(BraveFileSelectUtilsUnitTest, GetSiteFrameTitleForFileSelect) { +TEST(BraveFileSelectUtilsUnitTest, GetSiteFrameTitleForFileSelect_Open) { constexpr struct Case { // The name of the test case. const char* case_name; @@ -195,6 +196,9 @@ TEST(BraveFileSelectUtilsUnitTest, GetSiteFrameTitleForFileSelect) { "filesystem:http://foo.com/bar.html", "An embedded page on foo.com wants to open"}, }; + + brave_l10n::test::ScopedDefaultLocale scoped_locale("en-US"); + for (const auto& test_case : kCases) { SCOPED_TRACE(test_case.case_name); url::Origin main_frame_origin = @@ -211,3 +215,107 @@ TEST(BraveFileSelectUtilsUnitTest, GetSiteFrameTitleForFileSelect) { alerting_frame_origin, brave::GetFileSelectResourceIDsForOpen())); } } + +TEST(BraveFileSelectHelperUnitTest, GetSiteFrameTitleForFileSelect_Save) { + constexpr struct Case { + // The name of the test case. + const char* case_name; + + // The URL of the main frame of the page. + const char* main_frame_url; + + // Whether the main frame is alerting. + bool is_main_frame; + + // If `is_main_frame` is false, the URL of the alerting frame of the page. + const char* alerting_frame_url; + + // The expected title for the alert. + const char* expected; + } kCases[] = { + // Standard main frame alert. + {"standard", "http://foo.com/", true, "", "foo.com wants to save"}, + + // Subframe alert from the same origin. + {"subframe same origin", "http://foo.com/1", false, "http://foo.com/2", + "foo.com wants to save"}, + // Subframe alert from a different origin. + {"subframe different origin", "http://foo.com/", false, "http://bar.com/", + "An embedded page on bar.com wants to save"}, + + // file: + // - main frame: + {"file main frame", "file:///path/to/page.html", true, "", + "This page wants to save"}, + // - subframe: + {"file subframe", "http://foo.com/", false, "file:///path/to/page.html", + "An embedded page on this page wants to save"}, + + // data: + // /!\ NOTE that this is for data URLs entered directly in the omnibox. + // For pages that generate frames with data URLs, see the browsertest. + // - main frame: + {"data main frame", "data:blahblah", true, "", "This page wants to save"}, + // - subframe: + {"data subframe", "http://foo.com/", false, "data:blahblah", + "An embedded page on this page wants to save"}, + + // javascript: + // /!\ NOTE that this is for javascript URLs entered directly in the + // omnibox. For pages that generate frames with javascript URLs, see the + // browsertest. + // - main frame: + {"javascript main frame", "javascript:abc", true, "", + "This page wants to save"}, + // - subframe: + {"javascript subframe", "http://foo.com/", false, "javascript:abc", + "An embedded page on this page wants to save"}, + + // about: + // /!\ NOTE that this is for about:blank URLs entered directly in the + // omnibox. For pages that generate frames with about:blank URLs, see the + // browsertest. + // - main frame: + {"about main frame", "about:blank", true, "", "This page wants to save"}, + // - subframe: + {"about subframe", "http://foo.com/", false, "about:blank", + "An embedded page on this page wants to save"}, + + // blob: + // - main frame: + {"blob main frame", + "blob:http://foo.com/66666666-6666-6666-6666-666666666666", true, "", + "foo.com wants to save"}, + // - subframe: + {"blob subframe", "http://bar.com/", false, + "blob:http://foo.com/66666666-6666-6666-6666-666666666666", + "An embedded page on foo.com wants to save"}, + + // filesystem: + // - main frame: + {"filesystem main frame", "filesystem:http://foo.com/bar.html", true, "", + "foo.com wants to save"}, + // - subframe: + {"filesystem subframe", "http://bar.com/", false, + "filesystem:http://foo.com/bar.html", + "An embedded page on foo.com wants to save"}, + }; + + brave_l10n::test::ScopedDefaultLocale scoped_locale("en-US"); + + for (const auto& test_case : kCases) { + SCOPED_TRACE(test_case.case_name); + url::Origin main_frame_origin = + url::Origin::Create(GURL(test_case.main_frame_url)); + url::Origin alerting_frame_origin = + test_case.is_main_frame + ? main_frame_origin + : url::Origin::Create(GURL(test_case.alerting_frame_url)); + EXPECT_EQ( + base::UTF8ToUTF16(test_case.expected), + brave::GetSiteFrameTitleForFileSelect( + brave::GetSiteFrameTitleType(main_frame_origin, + alerting_frame_origin), + alerting_frame_origin, brave::GetFileSelectResourceIDsForSave())); + } +} From a6649dc61d2cea5fa92c541334c8385f4d765e8a Mon Sep 17 00:00:00 2001 From: "sangwoo.ko" Date: Fri, 19 Jul 2024 17:11:50 +0900 Subject: [PATCH 9/9] Address code review --- app/brave_generated_resources.grd | 16 ++-- browser/ui/brave_browser.cc | 4 +- browser/ui/brave_file_select_utils.cc | 75 ++++++++++--------- browser/ui/brave_file_select_utils.h | 34 +++++---- .../ui/brave_file_select_utils_unittest.cc | 38 ++++------ .../browser/download/download_file_picker.cc | 2 +- 6 files changed, 82 insertions(+), 87 deletions(-) diff --git a/app/brave_generated_resources.grd b/app/brave_generated_resources.grd index 933b1a3dd03..b8c2eba7ece 100644 --- a/app/brave_generated_resources.grd +++ b/app/brave_generated_resources.grd @@ -1301,28 +1301,28 @@ Or change later at $2brave://settings/ext - + $1http://foo.com wants to open - + An embedded page on $1http://foo.com wants to open - + This page wants to open - + An embedded page on this page wants to open - + $1http://foo.com wants to save - + An embedded page on $1http://foo.com wants to save - + This page wants to save - + An embedded page on this page wants to save diff --git a/browser/ui/brave_browser.cc b/browser/ui/brave_browser.cc index 7d5b2ee6b88..e9e51b6203c 100644 --- a/browser/ui/brave_browser.cc +++ b/browser/ui/brave_browser.cc @@ -151,8 +151,8 @@ void BraveBrowser::RunFileChooser( content::WebContents::FromRenderFrameHost(render_frame_host), render_frame_host->GetLastCommittedOrigin(), params.mode == blink::mojom::FileChooserParams::Mode::kSave - ? brave::GetFileSelectResourceIDsForSave() - : brave::GetFileSelectResourceIDsForOpen()); + ? brave::FileSelectTitleType::kSave + : brave::FileSelectTitleType::kOpen); } Browser::RunFileChooser(render_frame_host, listener, *new_params); #endif diff --git a/browser/ui/brave_file_select_utils.cc b/browser/ui/brave_file_select_utils.cc index 6f9ac4a925d..1bbb60cefed 100644 --- a/browser/ui/brave_file_select_utils.cc +++ b/browser/ui/brave_file_select_utils.cc @@ -5,11 +5,14 @@ #include "brave/browser/ui/brave_file_select_utils.h" +#include + #include "base/i18n/rtl.h" #include "base/no_destructor.h" #include "brave/grit/brave_generated_resources.h" #include "chrome/browser/profiles/profile.h" #include "chrome/browser/ui/url_identity.h" +#include "components/strings/grit/components_strings.h" #include "components/url_formatter/elide_url.h" #include "content/public/browser/web_contents.h" #include "ui/base/l10n/l10n_util.h" @@ -43,7 +46,7 @@ url::Origin UnwrapOriginIfOpaque(const url::Origin& origin) { std::u16string GetFileSelectTitle(content::WebContents* web_contents, const url::Origin& alerting_frame_origin, - const SiteTitleResourceIDMap& resource_ids) { + FileSelectTitleType file_select_type) { // This implementation partially mirrors // ChromeAppModalDialogManagerDelegate::GetTitle(). // TODO(sko) It's hard to test this behavior is in sync at this moment. Even @@ -71,25 +74,53 @@ std::u16string GetFileSelectTitle(content::WebContents* web_contents, web_contents->GetPrimaryMainFrame()->GetLastCommittedOrigin(); return GetSiteFrameTitleForFileSelect( GetSiteFrameTitleType(main_frame_origin, alerting_frame_origin), - alerting_frame_origin, resource_ids); + alerting_frame_origin, file_select_type); } std::u16string GetSiteFrameTitleForFileSelect( - SiteFrameTitleType type, + SiteFrameTitleType frame_type, const url::Origin& alerting_frame_origin, - const SiteTitleResourceIDMap& resource_ids) { - if (type == SiteFrameTitleType::kStandardSameOrigin || - type == SiteFrameTitleType::kStandardDifferentOrigin) { + FileSelectTitleType file_select_type) { + constexpr std::array< + std::array(SiteFrameTitleType::kSize)>, + static_cast(FileSelectTitleType::kSize)> + kResourceIDs = { + {/*FileSelectTitleType::kOpen,*/ + { + IDS_BRAVE_FILE_SELECT_OPEN_TITLE, // brave::SiteFrameTitleType::kStandardSameOrigin + IDS_BRAVE_FILE_SELECT_OPEN_TITLE_IFRAME, // brave::SiteFrameTitleType::kStandardDifferentOrigin + IDS_BRAVE_FILE_SELECT_OPEN_TITLE_NONSTANDARD_URL, // brave::SiteFrameTitleType::kNonStandardSameOrigin + IDS_BRAVE_FILE_SELECT_OPEN_TITLE_NONSTANDARD_URL_IFRAME // brave::SiteFrameTitleType::kNonStandardDifferentOrigin + }, + /*FileSelectTitleType::kSave,*/ + { + IDS_BRAVE_FILE_SELECT_SAVE_TITLE, // brave::SiteFrameTitleType::kStandardSameOrigin + IDS_BRAVE_FILE_SELECT_SAVE_TITLE_IFRAME, // brave::SiteFrameTitleType::kStandardDifferentOrigin + IDS_BRAVE_FILE_SELECT_SAVE_TITLE_NONSTANDARD_URL, // brave::SiteFrameTitleType::kNonStandardSameOrigin + IDS_BRAVE_FILE_SELECT_SAVE_TITLE_NONSTANDARD_URL_IFRAME // brave::SiteFrameTitleType::kNonStandardDifferentOrigin + }, + /*FileSelectTitleType::kChromiumDefault*/ + { + IDS_JAVASCRIPT_MESSAGEBOX_TITLE, // brave::SiteFrameTitleType::kStandardSameOrigin + IDS_JAVASCRIPT_MESSAGEBOX_TITLE_IFRAME, // brave::SiteFrameTitleType::kStandardDifferentOrigin, + IDS_JAVASCRIPT_MESSAGEBOX_TITLE_NONSTANDARD_URL, // brave::SiteFrameTitleType::kNonStandardSameOrigin + IDS_JAVASCRIPT_MESSAGEBOX_TITLE_NONSTANDARD_URL_IFRAME, // brave::SiteFrameTitleType::kNonStandardDifferentOrigin, + }}}; + + if (frame_type == SiteFrameTitleType::kStandardSameOrigin || + frame_type == SiteFrameTitleType::kStandardDifferentOrigin) { std::u16string origin_string = url_formatter::FormatOriginForSecurityDisplay( UnwrapOriginIfOpaque(alerting_frame_origin), url_formatter::SchemeDisplay::OMIT_HTTP_AND_HTTPS); return l10n_util::GetStringFUTF16( - resource_ids.at(type), + kResourceIDs[static_cast(file_select_type)] + [static_cast(frame_type)], base::i18n::GetDisplayStringInLTRDirectionality(origin_string)); } - return l10n_util::GetStringUTF16(resource_ids.at(type)); + return l10n_util::GetStringUTF16(kResourceIDs[static_cast( + file_select_type)][static_cast(frame_type)]); } SiteFrameTitleType GetSiteFrameTitleType( @@ -117,32 +148,4 @@ SiteFrameTitleType GetSiteFrameTitleType( : SiteFrameTitleType::kNonStandardDifferentOrigin; } -const SiteTitleResourceIDMap& GetFileSelectResourceIDsForOpen() { - static const base::NoDestructor - kResourcesForOpen( - {{brave::SiteFrameTitleType::kStandardSameOrigin, - IDS_FILE_SELECT_OPEN_TITLE}, - {brave::SiteFrameTitleType::kStandardDifferentOrigin, - IDS_FILE_SELECT_OPEN_TITLE_IFRAME}, - {brave::SiteFrameTitleType::kNonStandardSameOrigin, - IDS_FILE_SELECT_OPEN_TITLE_NONSTANDARD_URL}, - {brave::SiteFrameTitleType::kNonStandardDifferentOrigin, - IDS_FILE_SELECT_OPEN_TITLE_NONSTANDARD_URL_IFRAME}}); - return *kResourcesForOpen; -} - -const SiteTitleResourceIDMap& GetFileSelectResourceIDsForSave() { - static const base::NoDestructor - kResourcesForSave( - {{brave::SiteFrameTitleType::kStandardSameOrigin, - IDS_FILE_SELECT_SAVE_TITLE}, - {brave::SiteFrameTitleType::kStandardDifferentOrigin, - IDS_FILE_SELECT_SAVE_TITLE_IFRAME}, - {brave::SiteFrameTitleType::kNonStandardSameOrigin, - IDS_FILE_SELECT_SAVE_TITLE_NONSTANDARD_URL}, - {brave::SiteFrameTitleType::kNonStandardDifferentOrigin, - IDS_FILE_SELECT_SAVE_TITLE_NONSTANDARD_URL_IFRAME}}); - return *kResourcesForSave; -} - } // namespace brave diff --git a/browser/ui/brave_file_select_utils.h b/browser/ui/brave_file_select_utils.h index 1aa0e5cdb48..9147c836f12 100644 --- a/browser/ui/brave_file_select_utils.h +++ b/browser/ui/brave_file_select_utils.h @@ -26,34 +26,38 @@ namespace brave { enum class SiteFrameTitleType { kStandardSameOrigin, // alerting frame has http(s) scheme and has the same // origin with main frame - kStandardDifferentOrigin, // alerting frame http(s) scheme and has a - // different origin with main frame - kNonStandardSameOrigin, // alerting frame has other schemes (e.g. file, - // data, javascript) and has the same origin with - // main frame - kNonStandardDifferentOrigin // alerting frame has other schemes (e.g. file, - // data, javascript) and has a different origin - // with main frame + kStandardDifferentOrigin, // alerting frame http(s) scheme and has a + // different origin with main frame + kNonStandardSameOrigin, // alerting frame has other schemes (e.g. file, + // data, javascript) and has the same origin with + // main frame + kNonStandardDifferentOrigin, // alerting frame has other schemes (e.g. file, + // data, javascript) and has a different origin + // with main frame + kSize }; -using SiteTitleResourceIDMap = base::flat_map; +enum class FileSelectTitleType { + kOpen, + kSave, + kChromiumDefault, // used for comparing with the default title of the file + // select dialog in Chromium + kSize +}; std::u16string GetFileSelectTitle(content::WebContents* contents, const url::Origin& alerting_frame_origin, - const SiteTitleResourceIDMap& resource_ids); + FileSelectTitleType file_select_type); std::u16string GetSiteFrameTitleForFileSelect( - SiteFrameTitleType type, + SiteFrameTitleType frame_type, const url::Origin& alerting_frame_origin, - const SiteTitleResourceIDMap& resource_ids); + FileSelectTitleType select_type); SiteFrameTitleType GetSiteFrameTitleType( const url::Origin& main_frame_origin, const url::Origin& alerting_frame_origin); -const SiteTitleResourceIDMap& GetFileSelectResourceIDsForOpen(); -const SiteTitleResourceIDMap& GetFileSelectResourceIDsForSave(); - } // namespace brave #endif // BRAVE_BROWSER_UI_BRAVE_FILE_SELECT_UTILS_H_ diff --git a/browser/ui/brave_file_select_utils_unittest.cc b/browser/ui/brave_file_select_utils_unittest.cc index 0b478b3406a..5738960b4e2 100644 --- a/browser/ui/brave_file_select_utils_unittest.cc +++ b/browser/ui/brave_file_select_utils_unittest.cc @@ -8,7 +8,6 @@ #include "base/strings/utf_string_conversions.h" #include "brave/components/l10n/common/test/scoped_default_locale.h" #include "components/javascript_dialogs/app_modal_dialog_manager.h" -#include "components/strings/grit/components_strings.h" #include "testing/gtest/include/gtest/gtest.h" #include "url/origin.h" @@ -83,16 +82,6 @@ TEST(BraveFileSelectUtilsUnitTest, GetSiteFrameTitle_InSyncWithUpstream) { "filesystem:http://foo.com/bar.html"}, }; // Checks if our implementation is in sync with upstream. - const brave::SiteTitleResourceIDMap resource_ids({ - {brave::SiteFrameTitleType::kStandardSameOrigin, - IDS_JAVASCRIPT_MESSAGEBOX_TITLE}, - {brave::SiteFrameTitleType::kStandardDifferentOrigin, - IDS_JAVASCRIPT_MESSAGEBOX_TITLE_IFRAME}, - {brave::SiteFrameTitleType::kNonStandardSameOrigin, - IDS_JAVASCRIPT_MESSAGEBOX_TITLE_NONSTANDARD_URL}, - {brave::SiteFrameTitleType::kNonStandardDifferentOrigin, - IDS_JAVASCRIPT_MESSAGEBOX_TITLE_NONSTANDARD_URL_IFRAME}, - }); for (const auto& test_case : kCases) { SCOPED_TRACE(test_case.case_name); @@ -108,7 +97,8 @@ TEST(BraveFileSelectUtilsUnitTest, GetSiteFrameTitle_InSyncWithUpstream) { brave::GetSiteFrameTitleForFileSelect( brave::GetSiteFrameTitleType(main_frame_origin, alerting_frame_origin), - alerting_frame_origin, resource_ids)); + alerting_frame_origin, + brave::FileSelectTitleType::kChromiumDefault)); } } @@ -207,16 +197,15 @@ TEST(BraveFileSelectUtilsUnitTest, GetSiteFrameTitleForFileSelect_Open) { test_case.is_main_frame ? main_frame_origin : url::Origin::Create(GURL(test_case.alerting_frame_url)); - EXPECT_EQ( - base::UTF8ToUTF16(test_case.expected), - brave::GetSiteFrameTitleForFileSelect( - brave::GetSiteFrameTitleType(main_frame_origin, - alerting_frame_origin), - alerting_frame_origin, brave::GetFileSelectResourceIDsForOpen())); + EXPECT_EQ(base::UTF8ToUTF16(test_case.expected), + brave::GetSiteFrameTitleForFileSelect( + brave::GetSiteFrameTitleType(main_frame_origin, + alerting_frame_origin), + alerting_frame_origin, brave::FileSelectTitleType::kOpen)); } } -TEST(BraveFileSelectHelperUnitTest, GetSiteFrameTitleForFileSelect_Save) { +TEST(BraveFileSelectUtilsUnitTest, GetSiteFrameTitleForFileSelect_Save) { constexpr struct Case { // The name of the test case. const char* case_name; @@ -311,11 +300,10 @@ TEST(BraveFileSelectHelperUnitTest, GetSiteFrameTitleForFileSelect_Save) { test_case.is_main_frame ? main_frame_origin : url::Origin::Create(GURL(test_case.alerting_frame_url)); - EXPECT_EQ( - base::UTF8ToUTF16(test_case.expected), - brave::GetSiteFrameTitleForFileSelect( - brave::GetSiteFrameTitleType(main_frame_origin, - alerting_frame_origin), - alerting_frame_origin, brave::GetFileSelectResourceIDsForSave())); + EXPECT_EQ(base::UTF8ToUTF16(test_case.expected), + brave::GetSiteFrameTitleForFileSelect( + brave::GetSiteFrameTitleType(main_frame_origin, + alerting_frame_origin), + alerting_frame_origin, brave::FileSelectTitleType::kSave)); } } diff --git a/chromium_src/chrome/browser/download/download_file_picker.cc b/chromium_src/chrome/browser/download/download_file_picker.cc index e6343350f6d..9b0c7529335 100644 --- a/chromium_src/chrome/browser/download/download_file_picker.cc +++ b/chromium_src/chrome/browser/download/download_file_picker.cc @@ -24,7 +24,7 @@ std::u16string GetTitle(content::RenderFrameHost* render_frame_host, return brave::GetFileSelectTitle( content::WebContents::FromRenderFrameHost(render_frame_host), render_frame_host->GetLastCommittedOrigin(), - brave::GetFileSelectResourceIDsForSave()); + brave::FileSelectTitleType::kSave); #endif }