From cdc18bbb848ea6024652d60e5bf19285781c1559 Mon Sep 17 00:00:00 2001 From: Lei Zhang Date: Wed, 15 Dec 2021 03:26:58 +0000 Subject: [PATCH 01/26] Move base::BrokenClampThatShouldNotBeUsed() to the file that uses it. Move it out of base/numerics/ranges.h and remove some unnecessary base/numerics/ranges.h includes / DEPS entries. Bug: 1231569 Change-Id: I3690d6fecf8462cefea2d4aa160baeceef77ee4b Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3331667 Reviewed-by: Avi Drissman Commit-Queue: Lei Zhang Cr-Commit-Position: refs/heads/main@{#951801} --- base/allocator/partition_allocator/DEPS | 1 - base/numerics/ranges.h | 19 +------------------ .../gaussian_trainer.cc | 1 - .../browser/ui/window_sizer/window_sizer.cc | 19 ++++++++++++------- 4 files changed, 13 insertions(+), 27 deletions(-) diff --git a/base/allocator/partition_allocator/DEPS b/base/allocator/partition_allocator/DEPS index 9d183d68b174b2..1a171ab5d2a719 100644 --- a/base/allocator/partition_allocator/DEPS +++ b/base/allocator/partition_allocator/DEPS @@ -33,7 +33,6 @@ include_rules = [ "+base/memory/scoped_refptr.h", "+base/memory/tagging.h", "+base/no_destructor.h", - "+base/numerics/ranges.h", "+base/posix/eintr_wrapper.h", "+base/process/memory.h", "+base/rand_util.h", diff --git a/base/numerics/ranges.h b/base/numerics/ranges.h index 58deb6feecbde8..ebe1302b3d5fca 100644 --- a/base/numerics/ranges.h +++ b/base/numerics/ranges.h @@ -5,28 +5,11 @@ #ifndef BASE_NUMERICS_RANGES_H_ #define BASE_NUMERICS_RANGES_H_ -#include #include +#include namespace base { -// DO NOT USE THIS FUNCTION. IT IS DUE TO BE REMOVED. https://crbug.com/1231569 -// Please use base::clamp() from base/cxx17_backports.h instead. -// -// This function, unlike base::clamp(), does not check if `min` is greater than -// `max`, and returns a bogus answer if it is. Please migrate all code that -// calls this function to use base::clamp() instead. -// -// If, for some reason the broken behavior is required, please re-create this -// min/max nesting inline in the host code and explain with a comment why it -// is needed. -template -constexpr const T& BrokenClampThatShouldNotBeUsed(const T& value, - const T& min, - const T& max) { - return std::min(std::max(value, min), max); -} - template constexpr bool IsApproximatelyEqual(T lhs, T rhs, T tolerance) { static_assert(std::is_arithmetic::value, "Argument must be arithmetic"); diff --git a/chrome/browser/ash/power/auto_screen_brightness/gaussian_trainer.cc b/chrome/browser/ash/power/auto_screen_brightness/gaussian_trainer.cc index f7ed155ebd0e2e..44ee6c4c924b6a 100644 --- a/chrome/browser/ash/power/auto_screen_brightness/gaussian_trainer.cc +++ b/chrome/browser/ash/power/auto_screen_brightness/gaussian_trainer.cc @@ -13,7 +13,6 @@ #include "base/metrics/field_trial_params.h" #include "base/metrics/histogram_functions.h" #include "base/metrics/histogram_macros.h" -#include "base/numerics/ranges.h" #include "chrome/browser/ash/power/auto_screen_brightness/utils.h" namespace ash { diff --git a/chrome/browser/ui/window_sizer/window_sizer.cc b/chrome/browser/ui/window_sizer/window_sizer.cc index 875aa4c62db095..97895b39a4e31a 100644 --- a/chrome/browser/ui/window_sizer/window_sizer.cc +++ b/chrome/browser/ui/window_sizer/window_sizer.cc @@ -4,11 +4,11 @@ #include "chrome/browser/ui/window_sizer/window_sizer.h" +#include #include #include "base/command_line.h" #include "base/memory/raw_ptr.h" -#include "base/numerics/ranges.h" #include "build/build_config.h" #include "build/chromeos_buildflags.h" #include "chrome/browser/browser_process.h" @@ -147,6 +147,13 @@ class DefaultStateProvider : public WindowSizer::StateProvider { raw_ptr browser_; }; +// This function, unlike base::clamp(), does not check if `min` is greater than +// `max`, and returns a bogus answer if it is. TODO(crbug.com/1235666) migrate +// all code that calls this function to use base::clamp() instead. +constexpr int BrokenClampThatShouldNotBeUsed(int value, int min, int max) { + return std::min(std::max(value, min), max); +} + } // namespace WindowSizer::WindowSizer(std::unique_ptr state_provider, @@ -324,9 +331,9 @@ void WindowSizer::AdjustBoundsToBeVisibleOnDisplay( bounds->set_height(std::min(bounds->height(), work_area.height())); // TODO(crbug.com/1235666): Make sure these use correct ranges (lo <= hi) // and migrate to base::clamp(). - bounds->set_x(base::BrokenClampThatShouldNotBeUsed( + bounds->set_x(BrokenClampThatShouldNotBeUsed( bounds->x(), work_area.x(), work_area.right() - bounds->width())); - bounds->set_y(base::BrokenClampThatShouldNotBeUsed( + bounds->set_y(BrokenClampThatShouldNotBeUsed( bounds->y(), work_area.y(), work_area.bottom() - bounds->height())); } @@ -357,10 +364,8 @@ void WindowSizer::AdjustBoundsToBeVisibleOnDisplay( const int max_x = work_area.right() - kMinVisibleWidth; // TODO(crbug.com/1235666): Make sure these use correct ranges (lo <= hi) // and migrate to base::clamp(). - bounds->set_y( - base::BrokenClampThatShouldNotBeUsed(bounds->y(), min_y, max_y)); - bounds->set_x( - base::BrokenClampThatShouldNotBeUsed(bounds->x(), min_x, max_x)); + bounds->set_y(BrokenClampThatShouldNotBeUsed(bounds->y(), min_y, max_y)); + bounds->set_x(BrokenClampThatShouldNotBeUsed(bounds->x(), min_x, max_x)); #endif // defined(OS_MAC) } From a5590715f74dc92538a67acd015afb0e37ca8ac0 Mon Sep 17 00:00:00 2001 From: chromium-autoroll Date: Wed, 15 Dec 2021 03:37:31 +0000 Subject: [PATCH 02/26] Roll Chrome Win32 PGO Profile Roll Chrome Win32 PGO profile from chrome-win32-main-1639515544-3861e0300d09429621942ad80a47946e7c2f9a70.profdata to chrome-win32-main-1639526239-fba5218e9f7efbe8971262d0927a477c0a0708f7.profdata If this roll has caused a breakage, revert this CL and stop the roller using the controls here: https://autoroll.skia.org/r/pgo-win32-chromium Please CC pgo-profile-sheriffs@google.com on the revert to ensure that a human is aware of the problem. To file a bug in Chromium main branch: https://bugs.chromium.org/p/chromium/issues/entry To report a problem with the AutoRoller itself, please file a bug: https://bugs.chromium.org/p/skia/issues/entry?template=Autoroller+Bug Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md Cq-Include-Trybots: luci.chrome.try:win-chrome Tbr: pgo-profile-sheriffs@google.com Change-Id: I3564b61f0aa763829dadcf1d0db43fdb3da17a50 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3339166 Commit-Queue: chromium-autoroll Bot-Commit: chromium-autoroll Cr-Commit-Position: refs/heads/main@{#951802} --- chrome/build/win32.pgo.txt | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/chrome/build/win32.pgo.txt b/chrome/build/win32.pgo.txt index 4500bf1e165218..f51359e5c07371 100644 --- a/chrome/build/win32.pgo.txt +++ b/chrome/build/win32.pgo.txt @@ -1 +1 @@ -chrome-win32-main-1639515544-3861e0300d09429621942ad80a47946e7c2f9a70.profdata +chrome-win32-main-1639526239-fba5218e9f7efbe8971262d0927a477c0a0708f7.profdata From 718f73492051bccb52cdb936d1b8541ed5f78806 Mon Sep 17 00:00:00 2001 From: Claudio DeSouza Date: Wed, 15 Dec 2021 03:38:29 +0000 Subject: [PATCH 03/26] [MPArch] Fix FederatedAuthNavigationThrottle handling of fenced frames This CL corrects the factory function for FederatedAuthNavigationThrottle to make sure we handle fenced frames in the same way we are handling iframes. Bug: 1276449 Change-Id: I302d46ad06a0744cd533468a2d716d9797cf5c8b Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3318296 Reviewed-by: Ken Buchanan Reviewed-by: Alex Moshchuk Reviewed-by: Adithya Srinivasan Commit-Queue: Claudio DeSouza Cr-Commit-Position: refs/heads/main@{#951803} --- .../federated_auth_navigation_throttle.cc | 2 +- ...rated_auth_navigation_throttle_unittest.cc | 44 +++++++++++++++++++ 2 files changed, 45 insertions(+), 1 deletion(-) diff --git a/content/browser/webid/federated_auth_navigation_throttle.cc b/content/browser/webid/federated_auth_navigation_throttle.cc index 0518b058b06e58..25c49c18074c93 100644 --- a/content/browser/webid/federated_auth_navigation_throttle.cc +++ b/content/browser/webid/federated_auth_navigation_throttle.cc @@ -54,7 +54,7 @@ bool AreCookieIsolatedPrincipals(url::Origin src_origin, std::unique_ptr FederatedAuthNavigationThrottle::MaybeCreateThrottleFor( NavigationHandle* handle) { - if (!IsFedCmInterceptionEnabled() || !handle->IsInMainFrame()) + if (!IsFedCmInterceptionEnabled() || handle->GetParentFrameOrOuterDocument()) return nullptr; return std::make_unique(handle); diff --git a/content/browser/webid/federated_auth_navigation_throttle_unittest.cc b/content/browser/webid/federated_auth_navigation_throttle_unittest.cc index 7eaa56affcca39..fc30b8b6b7f064 100644 --- a/content/browser/webid/federated_auth_navigation_throttle_unittest.cc +++ b/content/browser/webid/federated_auth_navigation_throttle_unittest.cc @@ -21,6 +21,7 @@ #include "content/public/test/navigation_simulator.h" #include "content/public/test/test_renderer_host.h" #include "third_party/abseil-cpp/absl/types/optional.h" +#include "third_party/blink/public/common/features.h" namespace content { @@ -187,4 +188,47 @@ TEST_P(CrossSiteFederatedAuthNavigationThrottleTest, SameSiteAuthRequest) { EXPECT_EQ(test_case.expected_action, throttle->WillStartRequest().action()); } +class ContentSubresourceFilterThrottleManagerFencedFramesTest + : public FederatedAuthNavigationThrottleWithFlagEnabledTest { + public: + ContentSubresourceFilterThrottleManagerFencedFramesTest() { + scoped_feature_list_.InitAndEnableFeatureWithParameters( + blink::features::kFencedFrames, {{"implementation_type", "mparch"}}); + } + ~ContentSubresourceFilterThrottleManagerFencedFramesTest() override = default; + + content::RenderFrameHost* CreateFencedFrame( + content::RenderFrameHost* parent) { + content::RenderFrameHost* fenced_frame = + content::RenderFrameHostTester::For(parent)->AppendFencedFrame(); + return fenced_frame; + } + + private: + base::test::ScopedFeatureList scoped_feature_list_; +}; + +TEST_F(ContentSubresourceFilterThrottleManagerFencedFramesTest, + BlockThrottleCreationForFencedFrames) { + const GURL kUrl("https://idp.example"); + GURL kFencedFrameUrl("https://child.example"); + + content::RenderFrameHostTester::For(main_rfh()) + ->InitializeRenderFrameIfNeeded(); + RenderFrameHost* fenced_frame_rfh = CreateFencedFrame(main_rfh()); + + MockNavigationHandle top_frame_handle(kUrl, main_rfh()); + MockNavigationHandle fenced_frame_handle(kFencedFrameUrl, fenced_frame_rfh); + + // Should be able to create throttle for the main frame. + auto throttle = FederatedAuthNavigationThrottle::MaybeCreateThrottleFor( + &top_frame_handle); + ASSERT_TRUE(throttle); + + // A throttle should not be allowed for a fenced frame. + throttle = FederatedAuthNavigationThrottle::MaybeCreateThrottleFor( + &fenced_frame_handle); + ASSERT_FALSE(throttle); +} + } // namespace content From c44d568ff2c8bf46a41d3321558d94bacb4a0a50 Mon Sep 17 00:00:00 2001 From: chromium-autoroll Date: Wed, 15 Dec 2021 03:51:38 +0000 Subject: [PATCH 04/26] Roll Catapult from 563885e399ba to b759738dea9a (3 revisions) https://chromium.googlesource.com/catapult.git/+log/563885e399ba..b759738dea9a 2021-12-14 fdegans@google.com [code-health] Prepare dashboard/dashboard/api for pylint update 2021-12-14 hypan@google.com Revert "Add the function to install privileged apps to devices" 2021-12-14 fdegans@google.com [code-health] Prepare dashboard/dashboard/models for pylint update If this roll has caused a breakage, revert this CL and stop the roller using the controls here: https://autoroll.skia.org/r/catapult-autoroll Please CC skyostil@google.com on the revert to ensure that a human is aware of the problem. To file a bug in Chromium: https://bugs.chromium.org/p/chromium/issues/entry To report a problem with the AutoRoller itself, please file a bug: https://bugs.chromium.org/p/skia/issues/entry?template=Autoroller+Bug Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md Cq-Include-Trybots: luci.chromium.try:android_optional_gpu_tests_rel;luci.chromium.try:chromeos-kevin-rel;luci.chromium.try:linux_optional_gpu_tests_rel;luci.chromium.try:mac_optional_gpu_tests_rel;luci.chromium.try:win_optional_gpu_tests_rel Bug: chromium:1262292,chromium:1278096,chromium:1279818 Tbr: skyostil@google.com Change-Id: I71cdc21c508508be48d5ed3d5b309cb27f4066a4 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3339738 Commit-Queue: chromium-autoroll Bot-Commit: chromium-autoroll Cr-Commit-Position: refs/heads/main@{#951804} --- DEPS | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/DEPS b/DEPS index b4721d9f22a27c..a666897eaa5072 100644 --- a/DEPS +++ b/DEPS @@ -306,7 +306,7 @@ vars = { # Three lines of non-changing comments so that # the commit queue can handle CLs rolling catapult # and whatever else without interference from each other. - 'catapult_revision': '563885e399ba18672cd7d009a0c0f0a996d71f6b', + 'catapult_revision': 'b759738dea9abafcbd0dc69553fd049a86e7b56b', # Three lines of non-changing comments so that # the commit queue can handle CLs rolling libFuzzer # and whatever else without interference from each other. From 3e1d447ace62672ecba2a14614bb82e6ee82a635 Mon Sep 17 00:00:00 2001 From: "Chrome Release Bot (LUCI)" Date: Wed, 15 Dec 2021 04:02:15 +0000 Subject: [PATCH 05/26] Updating trunk VERSION from 4767.0 to 4768.0 * This is an automated release commit. * Do not revert without consulting chrome-pmo@google.com. NOAUTOREVERT=true Change-Id: Iecff6b34486c6e608c04f9b577441110d3ac2958 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3340021 Bot-Commit: Chrome Release Bot (LUCI) Cr-Commit-Position: refs/heads/main@{#951805} --- chrome/VERSION | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/chrome/VERSION b/chrome/VERSION index aaa50afa9765e2..da106233dfea60 100644 --- a/chrome/VERSION +++ b/chrome/VERSION @@ -1,4 +1,4 @@ MAJOR=99 MINOR=0 -BUILD=4767 +BUILD=4768 PATCH=0 From e905c8b5d79c2eb3ca70c131d75c6ea749a1baa6 Mon Sep 17 00:00:00 2001 From: Minoru Chikamune Date: Wed, 15 Dec 2021 04:05:28 +0000 Subject: [PATCH 06/26] [Code Health] Modernize Value API in metrics_reporting_pref_helper.cc Bug: 1187061 Change-Id: I95b3631ad22d36d8ba9cc445010c3366825ef323 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3334212 Reviewed-by: Jesse Doherty Commit-Queue: Minoru Chikamune Cr-Commit-Position: refs/heads/main@{#951806} --- .../testing/metrics_reporting_pref_helper.cc | 15 +++++++-------- 1 file changed, 7 insertions(+), 8 deletions(-) diff --git a/chrome/browser/metrics/testing/metrics_reporting_pref_helper.cc b/chrome/browser/metrics/testing/metrics_reporting_pref_helper.cc index 23888fa45aa7d7..4b28b65f779c7e 100644 --- a/chrome/browser/metrics/testing/metrics_reporting_pref_helper.cc +++ b/chrome/browser/metrics/testing/metrics_reporting_pref_helper.cc @@ -22,9 +22,8 @@ #if BUILDFLAG(IS_CHROMEOS_ASH) namespace { -void SetMetricsReportingEnabledChromeOS( - bool is_enabled, - base::DictionaryValue* local_state_dict) { +void SetMetricsReportingEnabledChromeOS(bool is_enabled, + base::Value& local_state_dict) { namespace em = enterprise_management; em::ChromeDeviceSettingsProto device_settings_proto; device_settings_proto.mutable_metrics_enabled()->set_metrics_enabled( @@ -32,7 +31,7 @@ void SetMetricsReportingEnabledChromeOS( em::PolicyData policy_data; policy_data.set_policy_type("google/chromeos/device"); policy_data.set_policy_value(device_settings_proto.SerializeAsString()); - local_state_dict->SetString( + local_state_dict.SetStringKey( prefs::kDeviceSettingsCache, ash::device_settings_cache::PolicyDataToString(policy_data)); } @@ -43,9 +42,9 @@ void SetMetricsReportingEnabledChromeOS( namespace metrics { base::FilePath SetUpUserDataDirectoryForTesting(bool is_enabled) { - base::DictionaryValue local_state_dict; - local_state_dict.SetBoolean(metrics::prefs::kMetricsReportingEnabled, - is_enabled); + base::Value local_state_dict(base::Value::Type::DICTIONARY); + local_state_dict.SetBoolPath(metrics::prefs::kMetricsReportingEnabled, + is_enabled); base::FilePath user_data_dir; if (!base::PathService::Get(chrome::DIR_USER_DATA, &user_data_dir)) @@ -53,7 +52,7 @@ base::FilePath SetUpUserDataDirectoryForTesting(bool is_enabled) { #if BUILDFLAG(IS_CHROMEOS_ASH) // ChromeOS checks a separate place for reporting enabled. - SetMetricsReportingEnabledChromeOS(is_enabled, &local_state_dict); + SetMetricsReportingEnabledChromeOS(is_enabled, local_state_dict); #endif base::FilePath local_state_path = From 0ecaa65819ded7b0f0ba4cf58b4150e982bcd3b1 Mon Sep 17 00:00:00 2001 From: Alex Kalugin Date: Wed, 15 Dec 2021 04:06:53 +0000 Subject: [PATCH 07/26] Use StringPiece in GetKey and GetValue in QueryIterator Bug: 1279769 Change-Id: I268fa4b1d9ef8195375edb1da27d62302fcbd423 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3317350 Reviewed-by: Mikel Astiz Reviewed-by: Mihai Sardarescu Reviewed-by: Takumi Fujimoto Reviewed-by: Josh Karlin Reviewed-by: Andrey Kosyakov Reviewed-by: John Lee Reviewed-by: Glenn Hartmann Reviewed-by: Varun Khaneja Reviewed-by: Kelvin Jiang Reviewed-by: Alan Cutter Commit-Queue: Kalugin Alexander Cr-Commit-Position: refs/heads/main@{#951807} --- chrome/browser/devtools/devtools_ui_bindings.cc | 7 ++++--- .../router/providers/dial/dial_activity_manager.cc | 6 +++--- .../safe_browsing/local_two_phase_testserver.cc | 2 +- .../ui/views/sync/inline_login_ui_browsertest.cc | 2 +- chrome/browser/ui/webui/fileicon_source.cc | 5 +++-- .../browser/web_applications/web_app_install_task.cc | 2 +- components/favicon_base/favicon_url_parser.cc | 2 +- .../common/providers/cast/cast_media_source.cc | 2 +- .../browser/android/app_banner_manager_android.cc | 2 +- .../declarative_net_request/ruleset_matcher_base.cc | 2 +- net/base/url_util.cc | 12 ++++++------ net/base/url_util.h | 8 ++++++-- 12 files changed, 29 insertions(+), 23 deletions(-) diff --git a/chrome/browser/devtools/devtools_ui_bindings.cc b/chrome/browser/devtools/devtools_ui_bindings.cc index 47d8c96b5878a4..8042c954515557 100644 --- a/chrome/browser/devtools/devtools_ui_bindings.cc +++ b/chrome/browser/devtools/devtools_ui_bindings.cc @@ -372,11 +372,12 @@ GURL SanitizeFrontendURL(const GURL& url, std::string fragment; if (allow_query_and_fragment) { for (net::QueryIterator it(url); !it.IsAtEnd(); it.Advance()) { - std::string value = SanitizeFrontendQueryParam(it.GetKey(), - it.GetValue()); + const std::string key = std::string(it.GetKey()); + std::string value = + SanitizeFrontendQueryParam(key, std::string(it.GetValue())); if (!value.empty()) { query_parts.push_back( - base::StringPrintf("%s=%s", it.GetKey().c_str(), value.c_str())); + base::StringPrintf("%s=%s", key.c_str(), value.c_str())); } } if (url.has_ref() && url.ref_piece().find('\'') == base::StringPiece::npos) diff --git a/chrome/browser/media/router/providers/dial/dial_activity_manager.cc b/chrome/browser/media/router/providers/dial/dial_activity_manager.cc index 146f482f302c16..0b5295e4e04af2 100644 --- a/chrome/browser/media/router/providers/dial/dial_activity_manager.cc +++ b/chrome/browser/media/router/providers/dial/dial_activity_manager.cc @@ -88,11 +88,11 @@ std::unique_ptr DialActivity::From( // temporary object. for (net::QueryIterator query_it(url); !query_it.IsAtEnd(); query_it.Advance()) { - std::string key = query_it.GetKey(); + const base::StringPiece key = query_it.GetKey(); if (key == "clientId") { - client_id = query_it.GetValue(); + client_id = std::string(query_it.GetValue()); } else if (key == "dialPostData") { - post_data = query_it.GetValue(); + post_data = std::string(query_it.GetValue()); } } if (client_id.empty()) diff --git a/chrome/browser/safe_browsing/local_two_phase_testserver.cc b/chrome/browser/safe_browsing/local_two_phase_testserver.cc index 110632705dfed2..3d563ca06a1d00 100644 --- a/chrome/browser/safe_browsing/local_two_phase_testserver.cc +++ b/chrome/browser/safe_browsing/local_two_phase_testserver.cc @@ -75,7 +75,7 @@ std::unique_ptr HandleTwoPhaseRequest( if (it.GetKey() == "p2close") p2close = "1"; if (it.GetKey() == "p2code") - p2code = it.GetValue(); + p2code = std::string(it.GetValue()); } std::string put_url = base::StringPrintf( diff --git a/chrome/browser/ui/views/sync/inline_login_ui_browsertest.cc b/chrome/browser/ui/views/sync/inline_login_ui_browsertest.cc index 4baa415d6d8c81..74f0bc77e1640c 100644 --- a/chrome/browser/ui/views/sync/inline_login_ui_browsertest.cc +++ b/chrome/browser/ui/views/sync/inline_login_ui_browsertest.cc @@ -844,7 +844,7 @@ class HtmlRequestTracker { net::QueryIterator it(request_url); for (; !it.IsAtEnd(); it.Advance()) { query_params.push_back( - std::make_pair(it.GetKey(), it.GetUnescapedValue())); + std::make_pair(std::string(it.GetKey()), it.GetUnescapedValue())); } requested_urls_[GURL(request.GetURL().GetWithEmptyPath().Resolve( request.GetURL().path()))] diff --git a/chrome/browser/ui/webui/fileicon_source.cc b/chrome/browser/ui/webui/fileicon_source.cc index 628d7f7ff6451c..c489d37b7c4fc2 100644 --- a/chrome/browser/ui/webui/fileicon_source.cc +++ b/chrome/browser/ui/webui/fileicon_source.cc @@ -37,7 +37,8 @@ const char kPathParameter[] = "path"; // URL parameter specifying scale factor. const char kScaleFactorParameter[] = "scale"; -IconLoader::IconSize SizeStringToIconSize(const std::string& size_string) { +IconLoader::IconSize SizeStringToIconSize( + const base::StringPiece& size_string) { if (size_string == "small") return IconLoader::SMALL; if (size_string == "large") return IconLoader::LARGE; // We default to NORMAL if we don't recognize the size_string. Including @@ -51,7 +52,7 @@ void ParseQueryParams(const std::string& path, IconLoader::IconSize* icon_size) { GURL request = GURL(chrome::kChromeUIFileiconURL).Resolve(path); for (net::QueryIterator it(request); !it.IsAtEnd(); it.Advance()) { - std::string key = it.GetKey(); + const base::StringPiece key = it.GetKey(); if (key == kPathParameter) { *file_path = base::FilePath::FromUTF8Unsafe(it.GetUnescapedValue()) .NormalizePathSeparators(); diff --git a/chrome/browser/web_applications/web_app_install_task.cc b/chrome/browser/web_applications/web_app_install_task.cc index ee24bb1debb76d..61b86af57478a7 100644 --- a/chrome/browser/web_applications/web_app_install_task.cc +++ b/chrome/browser/web_applications/web_app_install_task.cc @@ -61,7 +61,7 @@ constexpr bool kAddAppsToQuickLaunchBarByDefault = false; std::string ExtractQueryValueForName(const GURL& url, const std::string& name) { for (net::QueryIterator it(url); !it.IsAtEnd(); it.Advance()) { if (it.GetKey() == name) - return it.GetValue(); + return std::string(it.GetValue()); } return std::string(); } diff --git a/components/favicon_base/favicon_url_parser.cc b/components/favicon_base/favicon_url_parser.cc index 83a7b5f6ec2b6e..a8c23a8a8d6dbd 100644 --- a/components/favicon_base/favicon_url_parser.cc +++ b/components/favicon_base/favicon_url_parser.cc @@ -94,7 +94,7 @@ bool ParseFaviconPathWithFavicon2Format(const std::string& path, *parsed = chrome::ParsedFaviconPath(); for (net::QueryIterator it(query_url); !it.IsAtEnd(); it.Advance()) { - const std::string key = it.GetKey(); + const base::StringPiece key = it.GetKey(); // Note: each of these keys can be used in chrome://favicon2 path. See file // "favicon_url_parser.h" for a description of what each one does. if (key == "allow_google_server_fallback") { diff --git a/components/media_router/common/providers/cast/cast_media_source.cc b/components/media_router/common/providers/cast/cast_media_source.cc index fb656b8cc8cbd7..c78144f5b458a6 100644 --- a/components/media_router/common/providers/cast/cast_media_source.cc +++ b/components/media_router/common/providers/cast/cast_media_source.cc @@ -152,7 +152,7 @@ base::flat_map MakeQueryMap(const GURL& url) { base::flat_map result; for (net::QueryIterator query_it(url); !query_it.IsAtEnd(); query_it.Advance()) { - result[query_it.GetKey()] = query_it.GetUnescapedValue(); + result[std::string(query_it.GetKey())] = query_it.GetUnescapedValue(); } return result; } diff --git a/components/webapps/browser/android/app_banner_manager_android.cc b/components/webapps/browser/android/app_banner_manager_android.cc index dd855fc23ea466..40b3b709c8e9ed 100644 --- a/components/webapps/browser/android/app_banner_manager_android.cc +++ b/components/webapps/browser/android/app_banner_manager_android.cc @@ -364,7 +364,7 @@ std::string AppBannerManagerAndroid::ExtractQueryValueForName( const std::string& name) { for (net::QueryIterator it(url); !it.IsAtEnd(); it.Advance()) { if (it.GetKey() == name) - return it.GetValue(); + return std::string(it.GetValue()); } return std::string(); } diff --git a/extensions/browser/api/declarative_net_request/ruleset_matcher_base.cc b/extensions/browser/api/declarative_net_request/ruleset_matcher_base.cc index c48992538599fb..5f0f0a41b83abb 100644 --- a/extensions/browser/api/declarative_net_request/ruleset_matcher_base.cc +++ b/extensions/browser/api/declarative_net_request/ruleset_matcher_base.cc @@ -110,7 +110,7 @@ bool GetModifiedQuery(const GURL& url, bool query_changed = false; for (net::QueryIterator it(url); !it.IsAtEnd(); it.Advance()) { - std::string key = it.GetKey(); + const base::StringPiece key = it.GetKey(); // Remove query param. if (std::binary_search(remove_query_params.begin(), remove_query_params.end(), key)) { diff --git a/net/base/url_util.cc b/net/base/url_util.cc index 339822020f22c8..7880bb4aca7490 100644 --- a/net/base/url_util.cc +++ b/net/base/url_util.cc @@ -125,18 +125,18 @@ QueryIterator::QueryIterator(const GURL& url) QueryIterator::~QueryIterator() = default; -std::string QueryIterator::GetKey() const { +base::StringPiece QueryIterator::GetKey() const { DCHECK(!at_end_); if (key_.is_nonempty()) - return url_.spec().substr(key_.begin, key_.len); - return std::string(); + return base::StringPiece(&url_.spec()[key_.begin], key_.len); + return base::StringPiece(); } -std::string QueryIterator::GetValue() const { +base::StringPiece QueryIterator::GetValue() const { DCHECK(!at_end_); if (value_.is_nonempty()) - return url_.spec().substr(value_.begin, value_.len); - return std::string(); + return base::StringPiece(&url_.spec()[value_.begin], value_.len); + return base::StringPiece(); } const std::string& QueryIterator::GetUnescapedValue() { diff --git a/net/base/url_util.h b/net/base/url_util.h index c941957acb8e04..962777da990750 100644 --- a/net/base/url_util.h +++ b/net/base/url_util.h @@ -56,6 +56,10 @@ NET_EXPORT GURL AppendOrReplaceQueryParameter(const GURL& url, const std::string& value); // Iterates over the key-value pairs in the query portion of |url|. +// NOTE: QueryIterator stores reference to |url| and creates base::StringPiece +// instances which refer to the data inside |url| query. Therefore |url| must +// outlive QueryIterator and all base::StringPiece objects returned from GetKey +// and GetValue methods. class NET_EXPORT QueryIterator { public: explicit QueryIterator(const GURL& url); @@ -63,8 +67,8 @@ class NET_EXPORT QueryIterator { QueryIterator& operator=(const QueryIterator&) = delete; ~QueryIterator(); - std::string GetKey() const; - std::string GetValue() const; + base::StringPiece GetKey() const; + base::StringPiece GetValue() const; const std::string& GetUnescapedValue(); bool IsAtEnd() const; From cf016ce1ce2080317de5ed35aa18d7308fbb718e Mon Sep 17 00:00:00 2001 From: chromium-autoroll Date: Wed, 15 Dec 2021 04:10:10 +0000 Subject: [PATCH 08/26] Roll Skia from d337cf94a077 to 71f7880bb635 (1 revision) https://skia.googlesource.com/skia.git/+log/d337cf94a077..71f7880bb635 2021-12-15 johnstiles@google.com Emit trace_scope ops from SkVM code generation. If this roll has caused a breakage, revert this CL and stop the roller using the controls here: https://autoroll.skia.org/r/skia-autoroll Please CC bungeman@google.com on the revert to ensure that a human is aware of the problem. To file a bug in Skia: https://bugs.chromium.org/p/skia/issues/entry To file a bug in Chromium: https://bugs.chromium.org/p/chromium/issues/entry To report a problem with the AutoRoller itself, please file a bug: https://bugs.chromium.org/p/skia/issues/entry?template=Autoroller+Bug Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md Cq-Include-Trybots: luci.chromium.try:android_optional_gpu_tests_rel;luci.chromium.try:linux-blink-rel;luci.chromium.try:linux-chromeos-compile-dbg;luci.chromium.try:linux_optional_gpu_tests_rel;luci.chromium.try:mac_optional_gpu_tests_rel;luci.chromium.try:win_optional_gpu_tests_rel Cq-Do-Not-Cancel-Tryjobs: true Bug: None Tbr: bungeman@google.com Change-Id: I7591ada65c461072700506eb61248fc60ec7043a Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3340512 Commit-Queue: chromium-autoroll Bot-Commit: chromium-autoroll Cr-Commit-Position: refs/heads/main@{#951808} --- DEPS | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/DEPS b/DEPS index a666897eaa5072..47af91bdba7990 100644 --- a/DEPS +++ b/DEPS @@ -239,7 +239,7 @@ vars = { # Three lines of non-changing comments so that # the commit queue can handle CLs rolling Skia # and whatever else without interference from each other. - 'skia_revision': 'd337cf94a077c9ddec8f1c7d99c04e2ed14cd553', + 'skia_revision': '71f7880bb635810ca81e6cebaaaa25018499f2d7', # Three lines of non-changing comments so that # the commit queue can handle CLs rolling V8 # and whatever else without interference from each other. From 8d6308183e71b6312eb7448ac24f3b62ab23a3d3 Mon Sep 17 00:00:00 2001 From: Stefan Zager Date: Wed, 15 Dec 2021 04:10:39 +0000 Subject: [PATCH 09/26] Add NonBlockingCommit feature flag and implementation When enabled, the main thread will no longer block while commit runs on the impl thread. cc_unittests, blink_platform_unittests, and blink_unittests all pass with the feature enabled, but it is not yet safe to enable generally. Subsequent CL's will add calls to LTH::WaitForCommitCompletion where necessary to guarantee thread safety. Bug: chromium:1255972 Change-Id: I1c30c421da7060b7469e79dd7226661f34f96c1c Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3212131 Reviewed-by: Vladimir Levin Reviewed-by: Khushal Sagar Commit-Queue: Stefan Zager Cr-Commit-Position: refs/heads/main@{#951809} --- cc/base/features.cc | 4 +- cc/base/features.h | 8 ++- cc/layers/video_layer_impl.cc | 7 +- cc/trees/layer_tree_host.cc | 2 + cc/trees/layer_tree_host_client.h | 9 +++ cc/trees/layer_tree_host_unittest.cc | 13 +++- cc/trees/proxy_impl.cc | 66 +++++++++++++++---- cc/trees/proxy_impl.h | 8 +-- cc/trees/proxy_main.cc | 56 ++++++++++------ cc/trees/proxy_main.h | 9 +++ cc/trees/single_thread_proxy.cc | 2 +- .../core/frame/web_frame_widget_impl.cc | 11 ++-- .../widget/compositing/layer_tree_view.cc | 11 +++- 13 files changed, 153 insertions(+), 53 deletions(-) diff --git a/cc/base/features.cc b/cc/base/features.cc index d6834560836dcb..6027d1a41afa56 100644 --- a/cc/base/features.cc +++ b/cc/base/features.cc @@ -62,7 +62,9 @@ const base::Feature kDurationEstimatesInCompositorTimingHistory{ "DurationEstimatesInCompositorTimingHistory", base::FEATURE_DISABLED_BY_DEFAULT}; +const base::Feature kNonBlockingCommit{"NonBlockingCommit", + base::FEATURE_DISABLED_BY_DEFAULT}; + const base::Feature kSlidingWindowForDroppedFrameCounter{ "SlidingWindowForDroppedFrameCounter", base::FEATURE_DISABLED_BY_DEFAULT}; - } // namespace features diff --git a/cc/base/features.h b/cc/base/features.h index 6df0037ae3a42a..fa165f2c1b1260 100644 --- a/cc/base/features.h +++ b/cc/base/features.h @@ -53,10 +53,16 @@ CC_BASE_EXPORT extern const base::Feature CC_BASE_EXPORT extern const base::Feature kDurationEstimatesInCompositorTimingHistory; +// When enabled, the main thread does not block while commit is running on the +// impl thread. +// WARNING: This feature is not yet safe to enable. Work is needed to ensure +// that main thread cc data structures are not modified on the main thread while +// commit is running concurrently on the impl thread. +CC_BASE_EXPORT extern const base::Feature kNonBlockingCommit; + // When enabled, DroppedFrameCounter will use an adjusted sliding window // interval specified by field trial params. CC_BASE_EXPORT extern const base::Feature kSlidingWindowForDroppedFrameCounter; - } // namespace features #endif // CC_BASE_FEATURES_H_ diff --git a/cc/layers/video_layer_impl.cc b/cc/layers/video_layer_impl.cc index 0c6087a43896cf..ebaa4916271cf1 100644 --- a/cc/layers/video_layer_impl.cc +++ b/cc/layers/video_layer_impl.cc @@ -12,6 +12,7 @@ #include "base/bind.h" #include "base/check.h" #include "base/memory/ptr_util.h" +#include "cc/base/features.h" #include "cc/layers/video_frame_provider_client_impl.h" #include "cc/trees/layer_tree_frame_sink.h" #include "cc/trees/layer_tree_impl.h" @@ -33,7 +34,8 @@ std::unique_ptr VideoLayerImpl::Create( int id, VideoFrameProvider* provider, media::VideoTransformation video_transform) { - DCHECK(tree_impl->task_runner_provider()->IsMainThreadBlocked()); + DCHECK(tree_impl->task_runner_provider()->IsMainThreadBlocked() || + base::FeatureList::IsEnabled(features::kNonBlockingCommit)); DCHECK(tree_impl->task_runner_provider()->IsImplThread()); scoped_refptr provider_client_impl = @@ -62,8 +64,9 @@ VideoLayerImpl::~VideoLayerImpl() { // on the VideoFrameProviderClientImpl, but we stop when the first // LayerImpl (the one on the pending tree) is destroyed since we know // the main thread is blocked for this commit. + DCHECK(layer_tree_impl()->task_runner_provider()->IsMainThreadBlocked() || + base::FeatureList::IsEnabled(features::kNonBlockingCommit)); DCHECK(layer_tree_impl()->task_runner_provider()->IsImplThread()); - DCHECK(layer_tree_impl()->task_runner_provider()->IsMainThreadBlocked()); provider_client_impl_->Stop(); } } diff --git a/cc/trees/layer_tree_host.cc b/cc/trees/layer_tree_host.cc index 07460bbf423703..e60b735718bfcf 100644 --- a/cc/trees/layer_tree_host.cc +++ b/cc/trees/layer_tree_host.cc @@ -425,6 +425,7 @@ std::unique_ptr LayerTreeHost::ActivateCommitState() { void LayerTreeHost::WaitForCommitCompletion() { DCHECK(IsMainThread()); if (commit_completion_event_) { + TRACE_EVENT0("cc", "LayerTreeHost::WaitForCommitCompletion"); commit_completion_event_->Wait(); commit_completion_event_ = nullptr; } @@ -442,6 +443,7 @@ bool LayerTreeHost::IsUsingLayerLists() const { } void LayerTreeHost::CommitComplete(const CommitTimestamps& commit_timestamps) { + DCHECK(IsMainThread()); // This DCHECK ensures that WaitForCommitCompletion() will not block. DCHECK(IsMainThread()); DCHECK(!in_commit()); diff --git a/cc/trees/layer_tree_host_client.h b/cc/trees/layer_tree_host_client.h index 44aad4b8eb486e..0e775a23e531ff 100644 --- a/cc/trees/layer_tree_host_client.h +++ b/cc/trees/layer_tree_host_client.h @@ -108,6 +108,10 @@ class LayerTreeHostClient { virtual void BeginMainFrameNotExpectedSoon() = 0; virtual void BeginMainFrameNotExpectedUntil(base::TimeTicks time) = 0; + // This is called immediately after notifying the impl thread that it should + // do a commit, possibly before the commit has finished (depending on whether + // features::kNonBlockingCommit is enabled). It is meant for work that must + // happen prior to returning control to the main thread event loop. virtual void DidBeginMainFrame() = 0; virtual void WillUpdateLayers() = 0; virtual void DidUpdateLayers() = 0; @@ -168,6 +172,11 @@ class LayerTreeHostClient { // Mark the frame start and end time for UMA and UKM metrics that require // the time from the start of BeginMainFrame to the Commit, or early out. virtual void RecordStartOfFrameMetrics() = 0; + // This is called immediately after notifying the impl thread that it should + // do a commit, possibly before the commit has finished (depending on whether + // features::kNonBlockingCommit is enabled). It is meant to record the time + // when the main thread is finished with its part of a main frame, and will + // return control to the main thread event loop. virtual void RecordEndOfFrameMetrics( base::TimeTicks frame_begin_time, ActiveFrameSequenceTrackers trackers) = 0; diff --git a/cc/trees/layer_tree_host_unittest.cc b/cc/trees/layer_tree_host_unittest.cc index b926a44c978614..b4f6f798434119 100644 --- a/cc/trees/layer_tree_host_unittest.cc +++ b/cc/trees/layer_tree_host_unittest.cc @@ -7981,7 +7981,18 @@ class LayerTreeHostTestNoTasksBetweenWillAndDidCommit public: LayerTreeHostTestNoTasksBetweenWillAndDidCommit() : did_commit_(false) {} - void BeginTest() override { PostSetNeedsCommitToMainThread(); } + void BeginTest() override { + // The entire purpose of Non-Blocking Commit is to allow the main thread to + // continue doing work while commit is running on the impl thread, making + // this test obsolete. + if (base::FeatureList::IsEnabled(features::kNonBlockingCommit) && + layer_tree_host()->IsThreaded()) { + DidCommit(); + EndTest(); + } else { + PostSetNeedsCommitToMainThread(); + } + } void WillCommit(const CommitState&) override { MainThreadTaskRunner()->PostTask( diff --git a/cc/trees/proxy_impl.cc b/cc/trees/proxy_impl.cc index 04e23e8896e47e..2079ef6fb5575c 100644 --- a/cc/trees/proxy_impl.cc +++ b/cc/trees/proxy_impl.cc @@ -49,17 +49,37 @@ constexpr auto kSmoothnessTakesPriorityExpirationDelay = } // namespace -// Ensures that a CompletionEvent is always signaled. -class ScopedCompletionEvent { +// Ensures that a CompletionEvent for commit is always signaled. +class ScopedCommitCompletionEvent { public: - explicit ScopedCompletionEvent(CompletionEvent* event) : event_(event) {} - ScopedCompletionEvent(const ScopedCompletionEvent&) = delete; - ~ScopedCompletionEvent() { event_->Signal(); } + ScopedCommitCompletionEvent( + CompletionEvent* event, + base::TimeTicks start_time, + base::SingleThreadTaskRunner* main_thread_task_runner, + base::WeakPtr proxy_main_weak_ptr) + : event_(event), + commit_timestamps_({start_time, base::TimeTicks()}), + main_thread_task_runner_(main_thread_task_runner), + proxy_main_weak_ptr_(proxy_main_weak_ptr) {} + ScopedCommitCompletionEvent(const ScopedCommitCompletionEvent&) = delete; + ~ScopedCommitCompletionEvent() { + event_->Signal(); + main_thread_task_runner_->PostTask( + FROM_HERE, base::BindOnce(&ProxyMain::DidCompleteCommit, + proxy_main_weak_ptr_, commit_timestamps_)); + } + ScopedCommitCompletionEvent& operator=(const ScopedCommitCompletionEvent&) = + delete; - ScopedCompletionEvent& operator=(const ScopedCompletionEvent&) = delete; + void SetFinishTime(base::TimeTicks finish_time) { + commit_timestamps_.finish = finish_time; + } private: - const raw_ptr event_; + CompletionEvent* const event_; + CommitTimestamps commit_timestamps_; + base::SingleThreadTaskRunner* main_thread_task_runner_; + base::WeakPtr proxy_main_weak_ptr_; }; ProxyImpl::ProxyImpl( @@ -275,13 +295,24 @@ void ProxyImpl::NotifyReadyToCommitOnImpl( CommitTimestamps* commit_timestamps) { TRACE_EVENT0("cc", "ProxyImpl::NotifyReadyToCommitOnImpl"); DCHECK(!data_for_commit_.get()); - DCHECK(IsImplThread() && IsMainThreadBlocked()); + DCHECK(IsImplThread()); + DCHECK(base::FeatureList::IsEnabled(features::kNonBlockingCommit) || + IsMainThreadBlocked()); DCHECK(scheduler_); DCHECK(scheduler_->CommitPending()); // Inform the layer tree host that the commit has started, so that metrics // can determine how long we waited for thread synchronization. - commit_timestamps->start = base::TimeTicks::Now(); + // + // If NonBlockingCommit is disabled, then commit_timestamps points to a + // variable on the call stack of the main thread. If NonBlockingCommit is + // enabled, then the commit timestamps are transmitted back to the main thread + // by ScopedCommitCompletionEvent. + DCHECK_NE((bool)commit_timestamps, + base::FeatureList::IsEnabled(features::kNonBlockingCommit)); + base::TimeTicks start_time = base::TimeTicks::Now(); + if (commit_timestamps) + commit_timestamps->start = start_time; if (!host_impl_) { TRACE_EVENT_INSTANT0("cc", "EarlyOut_NoLayerTree", @@ -298,7 +329,9 @@ void ProxyImpl::NotifyReadyToCommitOnImpl( host_impl_->ReadyToCommit(commit_args, begin_main_frame_metrics.get()); data_for_commit_ = std::make_unique( - std::make_unique(completion_event), + std::make_unique( + completion_event, start_time, MainThreadTaskRunner(), + proxy_main_weak_ptr_), std::move(commit_state), unsafe_state, commit_timestamps); // Extract metrics data from the layer tree host and send them to the @@ -666,7 +699,8 @@ DrawResult ProxyImpl::ScheduledActionDrawForced() { void ProxyImpl::ScheduledActionCommit() { TRACE_EVENT0("cc", "ProxyImpl::ScheduledActionCommit"); DCHECK(IsImplThread()); - DCHECK(IsMainThreadBlocked()); + DCHECK(base::FeatureList::IsEnabled(features::kNonBlockingCommit) || + IsMainThreadBlocked()); DCHECK(data_for_commit_.get()); DCHECK(data_for_commit_->IsValid()); @@ -680,7 +714,10 @@ void ProxyImpl::ScheduledActionCommit() { auto* unsafe_state = data_for_commit_->unsafe_state; host_impl_->BeginCommit(commit_state->source_frame_number); host_impl_->FinishCommit(*commit_state, *unsafe_state); - data_for_commit_->commit_timestamps->finish = base::TimeTicks::Now(); + base::TimeTicks finish_time = base::TimeTicks::Now(); + if (data_for_commit_->commit_timestamps) + data_for_commit_->commit_timestamps->finish = finish_time; + data_for_commit_->commit_completion_event->SetFinishTime(finish_time); if (commit_state->commit_waits_for_activation) { // For some layer types in impl-side painting, the commit is held until the @@ -859,7 +896,7 @@ void ProxyImpl::SetEnableFrameRateThrottling( } ProxyImpl::DataForCommit::DataForCommit( - std::unique_ptr commit_completion_event, + std::unique_ptr commit_completion_event, std::unique_ptr commit_state, ThreadUnsafeCommitState* unsafe_state, CommitTimestamps* commit_timestamps) @@ -872,7 +909,8 @@ ProxyImpl::DataForCommit::~DataForCommit() = default; bool ProxyImpl::DataForCommit::IsValid() const { return commit_completion_event.get() && commit_state.get() && unsafe_state && - commit_timestamps; + (base::FeatureList::IsEnabled(features::kNonBlockingCommit) || + commit_timestamps); } } // namespace cc diff --git a/cc/trees/proxy_impl.h b/cc/trees/proxy_impl.h index 8ed500797f1ecd..0eddea8496226e 100644 --- a/cc/trees/proxy_impl.h +++ b/cc/trees/proxy_impl.h @@ -28,7 +28,7 @@ class ProxyMain; class RenderFrameMetadataObserver; class JankInjector; -class ScopedCompletionEvent; +class ScopedCommitCompletionEvent; // This class aggregates all the interactions that the main side of the // compositor needs to have with the impl side. @@ -169,7 +169,7 @@ class CC_EXPORT ProxyImpl : public LayerTreeHostImplClient, struct DataForCommit { DataForCommit( - std::unique_ptr commit_completion_event, + std::unique_ptr commit_completion_event, std::unique_ptr commit_state, ThreadUnsafeCommitState* unsafe_state, CommitTimestamps* commit_timestamps); @@ -179,7 +179,7 @@ class CC_EXPORT ProxyImpl : public LayerTreeHostImplClient, bool IsValid() const; // Set when the main thread is waiting on a commit to complete. - std::unique_ptr commit_completion_event; + std::unique_ptr commit_completion_event; std::unique_ptr commit_state; ThreadUnsafeCommitState* unsafe_state; // This is passed from the main thread so the impl thread can record @@ -190,7 +190,7 @@ class CC_EXPORT ProxyImpl : public LayerTreeHostImplClient, std::unique_ptr data_for_commit_; // Set when the main thread is waiting for activation to complete. - std::unique_ptr activation_completion_event_; + std::unique_ptr activation_completion_event_; // Set when the next draw should post DidCommitAndDrawFrame to the main // thread. diff --git a/cc/trees/proxy_main.cc b/cc/trees/proxy_main.cc index 41982d05676a87..a3ba577b6c6996 100644 --- a/cc/trees/proxy_main.cc +++ b/cc/trees/proxy_main.cc @@ -15,6 +15,7 @@ #include "base/trace_event/traced_value.h" #include "cc/base/completion_event.h" #include "cc/base/devtools_instrumentation.h" +#include "cc/base/features.h" #include "cc/benchmarks/benchmark_instrumentation.h" #include "cc/paint/paint_worklet_layer_painter.h" #include "cc/resources/ui_resource_manager.h" @@ -138,12 +139,6 @@ void ProxyMain::BeginMainFrame( if (layer_tree_host_->scheduling_client()) layer_tree_host_->scheduling_client()->DidRunBeginMainFrame(); - // If the commit finishes, LayerTreeHost will transfer its swap promises to - // LayerTreeImpl. The destructor of ScopedSwapPromiseChecker aborts the - // remaining swap promises. - ScopedAbortRemainingSwapPromises swap_promise_checker( - layer_tree_host_->GetSwapPromiseManager()); - // We need to issue image decode callbacks whether or not we will abort this // update and commit, since the request ids are only stored in // |begin_main_frame_state|. @@ -171,6 +166,8 @@ void ProxyMain::BeginMainFrame( begin_main_frame_start_time, std::move(empty_swap_promises), false /* scroll_and_viewport_changes_synced */)); + layer_tree_host_->GetSwapPromiseManager()->BreakSwapPromises( + SwapPromise::COMMIT_FAILS); return; } @@ -197,6 +194,8 @@ void ProxyMain::BeginMainFrame( // previously requested pipeline stages. deferred_final_pipeline_stage_ = std::max(final_pipeline_stage_, deferred_final_pipeline_stage_); + layer_tree_host_->GetSwapPromiseManager()->BreakSwapPromises( + SwapPromise::COMMIT_FAILS); return; } @@ -292,6 +291,8 @@ void ProxyMain::BeginMainFrame( // When we stop deferring commits, we should resume any previously requested // pipeline stages. deferred_final_pipeline_stage_ = final_pipeline_stage_; + layer_tree_host_->GetSwapPromiseManager()->BreakSwapPromises( + SwapPromise::COMMIT_FAILS); return; } @@ -317,7 +318,7 @@ void ProxyMain::BeginMainFrame( if (updated) final_pipeline_stage_ = COMMIT_PIPELINE_STAGE; - devtools_instrumentation::ScopedCommitTrace commit_task( + commit_trace_ = std::make_unique( layer_tree_host_->GetId(), begin_main_frame_state->begin_frame_args.frame_id.sequence_number); @@ -364,6 +365,7 @@ void ProxyMain::BeginMainFrame( layer_tree_host_->RecordEndOfFrameMetrics( begin_main_frame_start_time, begin_main_frame_state->active_sequence_trackers); + commit_trace_.reset(); return; } @@ -374,10 +376,13 @@ void ProxyMain::BeginMainFrame( // point of view, but asynchronously performed on the impl thread, // coordinated by the Scheduler. CommitTimestamps commit_timestamps; + bool blocking = !base::FeatureList::IsEnabled(features::kNonBlockingCommit); { TRACE_EVENT0("cc,raf_investigation", "ProxyMain::BeginMainFrame::commit"); - DebugScopedSetMainThreadBlocked main_thread_blocked(task_runner_provider_); + absl::optional main_thread_blocked; + if (blocking) + main_thread_blocked.emplace(task_runner_provider_); ImplThreadTaskRunner()->PostTask( FROM_HERE, base::BindOnce(&ProxyImpl::NotifyReadyToCommitOnImpl, @@ -385,8 +390,9 @@ void ProxyMain::BeginMainFrame( completion_event, std::move(commit_state), &unsafe_state, begin_main_frame_start_time, begin_main_frame_state->begin_frame_args, - &commit_timestamps)); - layer_tree_host_->WaitForCommitCompletion(); + blocking ? &commit_timestamps : nullptr)); + if (blocking) + layer_tree_host_->WaitForCommitCompletion(); } // For Blink implementations, this updates frame throttling and @@ -394,12 +400,21 @@ void ProxyMain::BeginMainFrame( // but *not* script-created IntersectionObserver. See // blink::LocalFrameView::RunPostLifecycleSteps. layer_tree_host_->DidBeginMainFrame(); - layer_tree_host_->CommitComplete(commit_timestamps); + if (blocking) + layer_tree_host_->CommitComplete(commit_timestamps); layer_tree_host_->RecordEndOfFrameMetrics( begin_main_frame_start_time, begin_main_frame_state->active_sequence_trackers); } +void ProxyMain::DidCompleteCommit(CommitTimestamps commit_timestamps) { + if (!base::FeatureList::IsEnabled(features::kNonBlockingCommit)) + return; + if (layer_tree_host_) + layer_tree_host_->CommitComplete(commit_timestamps); + commit_trace_.reset(); +} + void ProxyMain::DidPresentCompositorFrame( uint32_t frame_token, std::vector callbacks, @@ -643,16 +658,15 @@ void ProxyMain::SetPaintWorkletLayerPainter( bool ProxyMain::MainFrameWillHappenForTesting() { DCHECK(IsMainThread()); bool main_frame_will_happen = false; - { - DebugScopedSetMainThreadBlocked main_thread_blocked(task_runner_provider_); - CompletionEvent completion; - ImplThreadTaskRunner()->PostTask( - FROM_HERE, - base::BindOnce(&ProxyImpl::MainFrameWillHappenOnImplForTesting, - base::Unretained(proxy_impl_.get()), &completion, - &main_frame_will_happen)); - completion.Wait(); - } + if (layer_tree_host_) + layer_tree_host_->WaitForCommitCompletion(); + DebugScopedSetMainThreadBlocked main_thread_blocked(task_runner_provider_); + CompletionEvent completion; + ImplThreadTaskRunner()->PostTask( + FROM_HERE, base::BindOnce(&ProxyImpl::MainFrameWillHappenOnImplForTesting, + base::Unretained(proxy_impl_.get()), + &completion, &main_frame_will_happen)); + completion.Wait(); return main_frame_will_happen; } diff --git a/cc/trees/proxy_main.h b/cc/trees/proxy_main.h index 3d0daf4a9898ae..31f6c4c004d7d0 100644 --- a/cc/trees/proxy_main.h +++ b/cc/trees/proxy_main.h @@ -26,6 +26,10 @@ class PaintWorkletLayerPainter; class ProxyImpl; class RenderFrameMetadataObserver; +namespace devtools_instrumentation { +struct ScopedCommitTrace; +} + // This class aggregates all interactions that the impl side of the compositor // needs to have with the main side. // The class is created and lives on the main thread. @@ -58,6 +62,7 @@ class CC_EXPORT ProxyMain : public Proxy { void DidCompletePageScaleAnimation(); void BeginMainFrame( std::unique_ptr begin_main_frame_state); + void DidCompleteCommit(CommitTimestamps); void DidPresentCompositorFrame( uint32_t frame_token, std::vector callbacks, @@ -160,6 +165,10 @@ class CC_EXPORT ProxyMain : public Proxy { // Only used when defer_commits_ is active and must be set in such cases. base::TimeTicks commits_restart_time_; + // TODO(paint-dev): it's not clear how devtools will handle interlacing of + // main thread tasks with commit tracing (crbug.com/1277952). + std::unique_ptr commit_trace_; + // ProxyImpl is created and destroyed on the impl thread, and should only be // accessed on the impl thread. // It is safe to use base::Unretained to post tasks to ProxyImpl on the impl diff --git a/cc/trees/single_thread_proxy.cc b/cc/trees/single_thread_proxy.cc index a58b2c17d3e44f..c3cd9e77f344d5 100644 --- a/cc/trees/single_thread_proxy.cc +++ b/cc/trees/single_thread_proxy.cc @@ -214,7 +214,7 @@ void SingleThreadProxy::DoCommit(const viz::BeginFrameArgs& commit_args) { // Strictly speaking, it's not necessary to pass a CompletionEvent to // WillCommit, since we can't have thread contention issues. The benefit to // creating one here is that it simplifies LayerTreeHost::in_commit(), which - // useful in DCHECKs sprinkled throughout the code. + // is useful in DCHECKs sprinkled throughout the code. auto completion_event_ptr = std::make_unique( base::WaitableEvent::ResetPolicy::MANUAL); auto* completion_event = completion_event_ptr.get(); diff --git a/third_party/blink/renderer/core/frame/web_frame_widget_impl.cc b/third_party/blink/renderer/core/frame/web_frame_widget_impl.cc index beb040e74d819b..a531eb8ef6b2f0 100644 --- a/third_party/blink/renderer/core/frame/web_frame_widget_impl.cc +++ b/third_party/blink/renderer/core/frame/web_frame_widget_impl.cc @@ -2061,12 +2061,6 @@ void WebFrameWidgetImpl::BeginMainFrame(base::TimeTicks last_frame_time) { void WebFrameWidgetImpl::BeginCommitCompositorFrame() { commit_compositor_frame_start_time_.emplace(base::TimeTicks::Now()); probe::LayerTreePainted(LocalRootImpl()->GetFrame()); -} - -void WebFrameWidgetImpl::EndCommitCompositorFrame( - base::TimeTicks commit_start_time, - base::TimeTicks commit_finish_time) { - DCHECK(commit_compositor_frame_start_time_.has_value()); if (ForTopMostMainFrame()) { Document* doc = local_root_->GetFrame()->GetDocument(); if (doc->GetSettings()->GetViewportMetaEnabled() && @@ -2087,7 +2081,12 @@ void WebFrameWidgetImpl::EndCommitCompositorFrame( base::debug::DumpWithoutCrashing(); } } +} +void WebFrameWidgetImpl::EndCommitCompositorFrame( + base::TimeTicks commit_start_time, + base::TimeTicks commit_finish_time) { + DCHECK(commit_compositor_frame_start_time_.has_value()); LocalRootImpl() ->GetFrame() ->View() diff --git a/third_party/blink/renderer/platform/widget/compositing/layer_tree_view.cc b/third_party/blink/renderer/platform/widget/compositing/layer_tree_view.cc index ed4af1277ae477..ff315412ab3ea7 100644 --- a/third_party/blink/renderer/platform/widget/compositing/layer_tree_view.cc +++ b/third_party/blink/renderer/platform/widget/compositing/layer_tree_view.cc @@ -24,6 +24,7 @@ #include "base/values.h" #include "cc/animation/animation_host.h" #include "cc/animation/animation_timeline.h" +#include "cc/base/features.h" #include "cc/base/region.h" #include "cc/benchmarks/micro_benchmark.h" #include "cc/debug/layer_tree_debug_state.h" @@ -279,6 +280,10 @@ void LayerTreeView::WillCommit(const cc::CommitState&) { if (!delegate_) return; delegate_->WillCommitCompositorFrame(); + if (base::FeatureList::IsEnabled(features::kNonBlockingCommit)) { + if (web_main_thread_scheduler_) + web_main_thread_scheduler_->DidCommitFrameToCompositor(); + } } void LayerTreeView::DidCommit(base::TimeTicks commit_start_time, @@ -286,8 +291,10 @@ void LayerTreeView::DidCommit(base::TimeTicks commit_start_time, if (!delegate_) return; delegate_->DidCommitCompositorFrame(commit_start_time, commit_finish_time); - if (web_main_thread_scheduler_) - web_main_thread_scheduler_->DidCommitFrameToCompositor(); + if (!base::FeatureList::IsEnabled(features::kNonBlockingCommit)) { + if (web_main_thread_scheduler_) + web_main_thread_scheduler_->DidCommitFrameToCompositor(); + } } void LayerTreeView::DidCommitAndDrawFrame() { From 83844683f6b903d966baa37e6d49073a4e74d278 Mon Sep 17 00:00:00 2001 From: Pi-Hsun Shih Date: Wed, 15 Dec 2021 04:11:19 +0000 Subject: [PATCH 10/26] CCA: Migrate waitable_event.js to TypeScript Bug: b:172340451 Test: tsc compiles Change-Id: I363952cfcc94962315fbfcdc2bfa11213c6a2cbe Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3300070 Reviewed-by: Shik Chen Commit-Queue: Pi-Hsun Shih Cr-Commit-Position: refs/heads/main@{#951810} --- .../camera_app_ui/resources/js/app_window.ts | 4 +- .../resources/js/device/stream_manager.js | 21 ++++-- ash/webui/camera_app_ui/resources/js/js.gni | 2 +- .../resources/js/models/file_system.js | 1 + .../resources/js/mojo/device_operator.js | 3 + ash/webui/camera_app_ui/resources/js/sound.js | 1 + .../camera_app_ui/resources/js/thumbnailer.js | 1 + .../resources/js/views/camera/preview.js | 1 + .../resources/js/waitable_event.js | 74 ------------------- .../resources/js/waitable_event.ts | 58 +++++++++++++++ 10 files changed, 81 insertions(+), 85 deletions(-) delete mode 100644 ash/webui/camera_app_ui/resources/js/waitable_event.js create mode 100644 ash/webui/camera_app_ui/resources/js/waitable_event.ts diff --git a/ash/webui/camera_app_ui/resources/js/app_window.ts b/ash/webui/camera_app_ui/resources/js/app_window.ts index 1755a819931d94..57157d0605277a 100644 --- a/ash/webui/camera_app_ui/resources/js/app_window.ts +++ b/ash/webui/camera_app_ui/resources/js/app_window.ts @@ -47,8 +47,8 @@ export class AppWindow { * launched. */ private readonly readyOnCCASide = new WaitableEvent(); - private readonly readyOnTastSide = new WaitableEvent(); - private readonly onClosed = new WaitableEvent(); + private readonly readyOnTastSide = new WaitableEvent(); + private readonly onClosed = new WaitableEvent(); private inClosingItself = false; private readonly errors: ErrorInfo[] = []; private readonly perfs: PerfEntry[] = []; diff --git a/ash/webui/camera_app_ui/resources/js/device/stream_manager.js b/ash/webui/camera_app_ui/resources/js/device/stream_manager.js index c925acf0f93aaa..7cc9576c474e2f 100644 --- a/ash/webui/camera_app_ui/resources/js/device/stream_manager.js +++ b/ash/webui/camera_app_ui/resources/js/device/stream_manager.js @@ -310,20 +310,25 @@ export class StreamManager { */ async setMultipleStreamsEnabled(deviceId, enabled) { assert(await DeviceOperator.isSupported()); - const waitEvent = new WaitableEvent(); - if (enabled) { - this.waitVirtual_ = waitEvent; - } else { - this.waitVirtualRemoved_ = waitEvent; - } const deviceOperator = await DeviceOperator.getInstance(); - await deviceOperator.setMultipleStreamsEnabled(deviceId, enabled); - await this.deviceUpdate(); if (enabled) { + /** @type {WaitableEvent} */ + const waitEvent = new WaitableEvent(); + this.waitVirtual_ = waitEvent; + + await deviceOperator.setMultipleStreamsEnabled(deviceId, enabled); + await this.deviceUpdate(); + const virtualId = await waitEvent.timedWait(3000); this.virtualMap_ = {realId: deviceId, virtualId}; } else { + const waitEvent = new WaitableEvent(); + this.waitVirtualRemoved_ = waitEvent; + + await deviceOperator.setMultipleStreamsEnabled(deviceId, enabled); + await this.deviceUpdate(); + await waitEvent.timedWait(3000); this.virtualMap_ = null; } diff --git a/ash/webui/camera_app_ui/resources/js/js.gni b/ash/webui/camera_app_ui/resources/js/js.gni index 0ace6fa22a7660..c8971943ae1859 100644 --- a/ash/webui/camera_app_ui/resources/js/js.gni +++ b/ash/webui/camera_app_ui/resources/js/js.gni @@ -96,7 +96,7 @@ compile_js_files = [ "views/settings.js", "views/view.js", "views/warning.js", - "waitable_event.js", + "waitable_event.ts", "window_controller.js", ] diff --git a/ash/webui/camera_app_ui/resources/js/models/file_system.js b/ash/webui/camera_app_ui/resources/js/models/file_system.js index e2a40ed1ee9b46..de02268351d9a8 100644 --- a/ash/webui/camera_app_ui/resources/js/models/file_system.js +++ b/ash/webui/camera_app_ui/resources/js/models/file_system.js @@ -82,6 +82,7 @@ async function initInternalTempDir() { * @return {!Promise} Promise for the directory result. */ async function initCameraDirectory() { + /** @type {WaitableEvent} */ const handle = new WaitableEvent(); // We use the sessionStorage to decide if we should use the handle in the diff --git a/ash/webui/camera_app_ui/resources/js/mojo/device_operator.js b/ash/webui/camera_app_ui/resources/js/mojo/device_operator.js index 91a4578c7f92a8..274a9bff4a62d8 100644 --- a/ash/webui/camera_app_ui/resources/js/mojo/device_operator.js +++ b/ash/webui/camera_app_ui/resources/js/mojo/device_operator.js @@ -561,6 +561,9 @@ export class DeviceOperator { const reprocessEvents = new Map; const callbacks = []; for (const effect of effects) { + // TODO(pihsun): This should be WaitableEvent, since we + // sometimes call event.signal with Error. + /** @type {WaitableEvent} */ const event = new WaitableEvent(); reprocessEvents.set(effect, event); callbacks.push(event.wait()); diff --git a/ash/webui/camera_app_ui/resources/js/sound.js b/ash/webui/camera_app_ui/resources/js/sound.js index 3978bcb06c4a73..ad914cdd95f5e2 100644 --- a/ash/webui/camera_app_ui/resources/js/sound.js +++ b/ash/webui/camera_app_ui/resources/js/sound.js @@ -35,6 +35,7 @@ export async function play(el) { await el.play(); elementsStatus.set(el, Status.PLAYING); + /** @type {WaitableEvent} */ const audioStopped = new WaitableEvent(); const events = ['ended', 'pause']; const onAudioStopped = () => { diff --git a/ash/webui/camera_app_ui/resources/js/thumbnailer.js b/ash/webui/camera_app_ui/resources/js/thumbnailer.js index 1b221f042a9dc0..3fc791a26a97e7 100644 --- a/ash/webui/camera_app_ui/resources/js/thumbnailer.js +++ b/ash/webui/camera_app_ui/resources/js/thumbnailer.js @@ -47,6 +47,7 @@ async function elementToJpegBlob(element, width, height) { async function loadVideoBlob(blob) { const el = document.createElement('video'); try { + /** @type {WaitableEvent} */ const hasLoaded = new WaitableEvent(); el.addEventListener('error', () => { hasLoaded.signal(false); diff --git a/ash/webui/camera_app_ui/resources/js/views/camera/preview.js b/ash/webui/camera_app_ui/resources/js/views/camera/preview.js index 9a2ed9c9214bfe..a20f480c56e7f4 100644 --- a/ash/webui/camera_app_ui/resources/js/views/camera/preview.js +++ b/ash/webui/camera_app_ui/resources/js/views/camera/preview.js @@ -433,6 +433,7 @@ export class Preview { // from the preview and checked video muted state before taking photo. const track = this.getVideoTrack_(); const waitFrame = async () => { + /** @type {WaitableEvent} */ const onReady = new WaitableEvent(); const callbackId = this.video_.requestVideoFrameCallback((now) => { onReady.signal(true); diff --git a/ash/webui/camera_app_ui/resources/js/waitable_event.js b/ash/webui/camera_app_ui/resources/js/waitable_event.js deleted file mode 100644 index f57d35d984eb0c..00000000000000 --- a/ash/webui/camera_app_ui/resources/js/waitable_event.js +++ /dev/null @@ -1,74 +0,0 @@ -// Copyright 2020 The Chromium Authors. All rights reserved. -// Use of this source code is governed by a BSD-style license that can be -// found in the LICENSE file. - -/** - * A waitable event for synchronization between asynchronous jobs. - * @template T - */ -export class WaitableEvent { - /** - * @public - */ - constructor() { - /** - * @type {boolean} - * @private - */ - this.isSignaled_ = false; - - /** - * @type {function(T): void} - * @private - */ - this.resolve_; - - /** - * @type {!Promise} - * @private - */ - this.promise_ = new Promise((resolve) => { - this.resolve_ = resolve; - }); - } - - /** - * @return {boolean} Whether the event is signaled - */ - isSignaled() { - return this.isSignaled_; - } - - /** - * Signals the event. - * @param {T=} value - */ - signal(value) { - if (this.isSignaled_) { - return; - } - this.isSignaled_ = true; - this.resolve_(value); - } - - /** - * @return {!Promise} Resolved when the event is signaled. - */ - wait() { - return this.promise_; - } - - /** - * @param {number} timeout Timeout in ms. - * @return {!Promise} Resolved when the event is signaled, or rejected when - * timed out. - */ - timedWait(timeout) { - const timeoutPromise = new Promise((_resolve, reject) => { - setTimeout(() => { - reject(new Error(`Timed out after ${timeout}ms`)); - }, timeout); - }); - return Promise.race([this.promise_, timeoutPromise]); - } -} diff --git a/ash/webui/camera_app_ui/resources/js/waitable_event.ts b/ash/webui/camera_app_ui/resources/js/waitable_event.ts new file mode 100644 index 00000000000000..8ac823f11e585a --- /dev/null +++ b/ash/webui/camera_app_ui/resources/js/waitable_event.ts @@ -0,0 +1,58 @@ +// Copyright 2020 The Chromium Authors. All rights reserved. +// Use of this source code is governed by a BSD-style license that can be +// found in the LICENSE file. + +/** + * A waitable event for synchronization between asynchronous jobs. + */ +export class WaitableEvent { + private isSignaledInternal = false; + private resolve: (val: T) => void; + private promise: Promise; + /** + * @public + */ + constructor() { + this.promise = new Promise((resolve) => { + this.resolve = resolve; + }); + } + + /** + * @return Whether the event is signaled + */ + isSignaled(): boolean { + return this.isSignaledInternal; + } + + /** + * Signals the event. + */ + signal(value: T|undefined): void { + if (this.isSignaledInternal) { + return; + } + this.isSignaledInternal = true; + this.resolve(value); + } + + /** + * @return Resolved when the event is signaled. + */ + wait(): Promise { + return this.promise; + } + + /** + * @param timeout Timeout in ms. + * @return Resolved when the event is signaled, or rejected when timed out. + */ + timedWait(timeout: number): Promise { + const timeoutPromise = new Promise((_resolve, reject) => { + setTimeout(() => { + reject(new Error(`Timed out after ${timeout}ms`)); + }, timeout); + }); + return Promise.race([this.promise, timeoutPromise]); + } +} From 71a3d8e7a37c620907122f5e77a78da3b542d770 Mon Sep 17 00:00:00 2001 From: Gordon Seto Date: Wed, 15 Dec 2021 04:19:59 +0000 Subject: [PATCH 11/26] Revert "[CrOS Bluetooth] A11y in bluetooth pairing device list" This reverts commit d2591b8f10f081e119e11a6f4ea4933191e00587. Reason for revert: Causing console errors. Bug: b/210724942 Change-Id: I2967fe43cff9ea738a8c1759dc1c992b0e6d6abe Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3340074 Reviewed-by: Kyle Horimoto Commit-Queue: Gordon Seto Auto-Submit: Gordon Seto Cr-Commit-Position: refs/heads/main@{#951811} --- chrome/app/chromeos_strings.grdp | 33 --------- ...S_DEVICE_ITEM_A11Y_LABEL_COMPUTER.png.sha1 | 1 - ...E_ITEM_A11Y_LABEL_GAME_CONTROLLER.png.sha1 | 1 - ...GS_DEVICE_ITEM_A11Y_LABEL_HEADSET.png.sha1 | 1 - ...S_DEVICE_ITEM_A11Y_LABEL_KEYBOARD.png.sha1 | 1 - ...INGS_DEVICE_ITEM_A11Y_LABEL_MOUSE.png.sha1 | 1 - ...INGS_DEVICE_ITEM_A11Y_LABEL_PHONE.png.sha1 | 1 - ...NGS_DEVICE_ITEM_A11Y_LABEL_TABLET.png.sha1 | 1 - ...GS_DEVICE_ITEM_A11Y_LABEL_UNKNOWN.png.sha1 | 1 - ...VICE_ITEM_A11Y_LABEL_VIDEO_CAMERA.png.sha1 | 1 - ...E_ITEM_SECONDARY_ERROR_A11Y_LABEL.png.sha1 | 1 - ...ITEM_SECONDARY_PAIRING_A11Y_LABEL.png.sha1 | 1 - ...luetooth_shared_load_time_data_provider.cc | 22 ------ .../bluetooth_pairing_device_item_test.js | 29 +------- .../bluetooth_pairing_device_item.html | 33 +++------ .../bluetooth_pairing_device_item.js | 74 +------------------ ...uetooth_pairing_device_selection_page.html | 15 +--- ...bluetooth_pairing_device_selection_page.js | 14 +--- 18 files changed, 18 insertions(+), 213 deletions(-) delete mode 100644 chrome/app/chromeos_strings_grdp/IDS_BLUETOOTH_PAIRINGS_DEVICE_ITEM_A11Y_LABEL_COMPUTER.png.sha1 delete mode 100644 chrome/app/chromeos_strings_grdp/IDS_BLUETOOTH_PAIRINGS_DEVICE_ITEM_A11Y_LABEL_GAME_CONTROLLER.png.sha1 delete mode 100644 chrome/app/chromeos_strings_grdp/IDS_BLUETOOTH_PAIRINGS_DEVICE_ITEM_A11Y_LABEL_HEADSET.png.sha1 delete mode 100644 chrome/app/chromeos_strings_grdp/IDS_BLUETOOTH_PAIRINGS_DEVICE_ITEM_A11Y_LABEL_KEYBOARD.png.sha1 delete mode 100644 chrome/app/chromeos_strings_grdp/IDS_BLUETOOTH_PAIRINGS_DEVICE_ITEM_A11Y_LABEL_MOUSE.png.sha1 delete mode 100644 chrome/app/chromeos_strings_grdp/IDS_BLUETOOTH_PAIRINGS_DEVICE_ITEM_A11Y_LABEL_PHONE.png.sha1 delete mode 100644 chrome/app/chromeos_strings_grdp/IDS_BLUETOOTH_PAIRINGS_DEVICE_ITEM_A11Y_LABEL_TABLET.png.sha1 delete mode 100644 chrome/app/chromeos_strings_grdp/IDS_BLUETOOTH_PAIRINGS_DEVICE_ITEM_A11Y_LABEL_UNKNOWN.png.sha1 delete mode 100644 chrome/app/chromeos_strings_grdp/IDS_BLUETOOTH_PAIRINGS_DEVICE_ITEM_A11Y_LABEL_VIDEO_CAMERA.png.sha1 delete mode 100644 chrome/app/chromeos_strings_grdp/IDS_BLUETOOTH_PAIRINGS_DEVICE_ITEM_SECONDARY_ERROR_A11Y_LABEL.png.sha1 delete mode 100644 chrome/app/chromeos_strings_grdp/IDS_BLUETOOTH_PAIRINGS_DEVICE_ITEM_SECONDARY_PAIRING_A11Y_LABEL.png.sha1 diff --git a/chrome/app/chromeos_strings.grdp b/chrome/app/chromeos_strings.grdp index c5ed57cdf4e05e..8297c1c2cb74e6 100644 --- a/chrome/app/chromeos_strings.grdp +++ b/chrome/app/chromeos_strings.grdp @@ -854,39 +854,6 @@ Enter these keys on $1Nexus S - - Device $11 of $215, Unknown device named $3Beats - - - Device $11 of $215, Computer named $3Beats - - - Device $11 of $215, Phone named $3Beats - - - Device $11 of $215, Audio device named $3Beats - - - Device $11 of $215, Video camera named $3Beats - - - Device $11 of $215, Game controller named $3Beats - - - Device $11 of $215, Keyboard named $3Beats - - - Device $11 of $215, Mouse named $3Beats - - - Device $11 of $215, Tablet named $3Beats - - - Couldn't pair to device $1Beats; select device to try again - - - Pairing to $1Beats - diff --git a/chrome/app/chromeos_strings_grdp/IDS_BLUETOOTH_PAIRINGS_DEVICE_ITEM_A11Y_LABEL_COMPUTER.png.sha1 b/chrome/app/chromeos_strings_grdp/IDS_BLUETOOTH_PAIRINGS_DEVICE_ITEM_A11Y_LABEL_COMPUTER.png.sha1 deleted file mode 100644 index 697c12cd4175fc..00000000000000 --- a/chrome/app/chromeos_strings_grdp/IDS_BLUETOOTH_PAIRINGS_DEVICE_ITEM_A11Y_LABEL_COMPUTER.png.sha1 +++ /dev/null @@ -1 +0,0 @@ -384318d0fe4e3716ffb32ae3c5ad279fc246acbb \ No newline at end of file diff --git a/chrome/app/chromeos_strings_grdp/IDS_BLUETOOTH_PAIRINGS_DEVICE_ITEM_A11Y_LABEL_GAME_CONTROLLER.png.sha1 b/chrome/app/chromeos_strings_grdp/IDS_BLUETOOTH_PAIRINGS_DEVICE_ITEM_A11Y_LABEL_GAME_CONTROLLER.png.sha1 deleted file mode 100644 index 697c12cd4175fc..00000000000000 --- a/chrome/app/chromeos_strings_grdp/IDS_BLUETOOTH_PAIRINGS_DEVICE_ITEM_A11Y_LABEL_GAME_CONTROLLER.png.sha1 +++ /dev/null @@ -1 +0,0 @@ -384318d0fe4e3716ffb32ae3c5ad279fc246acbb \ No newline at end of file diff --git a/chrome/app/chromeos_strings_grdp/IDS_BLUETOOTH_PAIRINGS_DEVICE_ITEM_A11Y_LABEL_HEADSET.png.sha1 b/chrome/app/chromeos_strings_grdp/IDS_BLUETOOTH_PAIRINGS_DEVICE_ITEM_A11Y_LABEL_HEADSET.png.sha1 deleted file mode 100644 index 697c12cd4175fc..00000000000000 --- a/chrome/app/chromeos_strings_grdp/IDS_BLUETOOTH_PAIRINGS_DEVICE_ITEM_A11Y_LABEL_HEADSET.png.sha1 +++ /dev/null @@ -1 +0,0 @@ -384318d0fe4e3716ffb32ae3c5ad279fc246acbb \ No newline at end of file diff --git a/chrome/app/chromeos_strings_grdp/IDS_BLUETOOTH_PAIRINGS_DEVICE_ITEM_A11Y_LABEL_KEYBOARD.png.sha1 b/chrome/app/chromeos_strings_grdp/IDS_BLUETOOTH_PAIRINGS_DEVICE_ITEM_A11Y_LABEL_KEYBOARD.png.sha1 deleted file mode 100644 index 697c12cd4175fc..00000000000000 --- a/chrome/app/chromeos_strings_grdp/IDS_BLUETOOTH_PAIRINGS_DEVICE_ITEM_A11Y_LABEL_KEYBOARD.png.sha1 +++ /dev/null @@ -1 +0,0 @@ -384318d0fe4e3716ffb32ae3c5ad279fc246acbb \ No newline at end of file diff --git a/chrome/app/chromeos_strings_grdp/IDS_BLUETOOTH_PAIRINGS_DEVICE_ITEM_A11Y_LABEL_MOUSE.png.sha1 b/chrome/app/chromeos_strings_grdp/IDS_BLUETOOTH_PAIRINGS_DEVICE_ITEM_A11Y_LABEL_MOUSE.png.sha1 deleted file mode 100644 index 697c12cd4175fc..00000000000000 --- a/chrome/app/chromeos_strings_grdp/IDS_BLUETOOTH_PAIRINGS_DEVICE_ITEM_A11Y_LABEL_MOUSE.png.sha1 +++ /dev/null @@ -1 +0,0 @@ -384318d0fe4e3716ffb32ae3c5ad279fc246acbb \ No newline at end of file diff --git a/chrome/app/chromeos_strings_grdp/IDS_BLUETOOTH_PAIRINGS_DEVICE_ITEM_A11Y_LABEL_PHONE.png.sha1 b/chrome/app/chromeos_strings_grdp/IDS_BLUETOOTH_PAIRINGS_DEVICE_ITEM_A11Y_LABEL_PHONE.png.sha1 deleted file mode 100644 index 697c12cd4175fc..00000000000000 --- a/chrome/app/chromeos_strings_grdp/IDS_BLUETOOTH_PAIRINGS_DEVICE_ITEM_A11Y_LABEL_PHONE.png.sha1 +++ /dev/null @@ -1 +0,0 @@ -384318d0fe4e3716ffb32ae3c5ad279fc246acbb \ No newline at end of file diff --git a/chrome/app/chromeos_strings_grdp/IDS_BLUETOOTH_PAIRINGS_DEVICE_ITEM_A11Y_LABEL_TABLET.png.sha1 b/chrome/app/chromeos_strings_grdp/IDS_BLUETOOTH_PAIRINGS_DEVICE_ITEM_A11Y_LABEL_TABLET.png.sha1 deleted file mode 100644 index 697c12cd4175fc..00000000000000 --- a/chrome/app/chromeos_strings_grdp/IDS_BLUETOOTH_PAIRINGS_DEVICE_ITEM_A11Y_LABEL_TABLET.png.sha1 +++ /dev/null @@ -1 +0,0 @@ -384318d0fe4e3716ffb32ae3c5ad279fc246acbb \ No newline at end of file diff --git a/chrome/app/chromeos_strings_grdp/IDS_BLUETOOTH_PAIRINGS_DEVICE_ITEM_A11Y_LABEL_UNKNOWN.png.sha1 b/chrome/app/chromeos_strings_grdp/IDS_BLUETOOTH_PAIRINGS_DEVICE_ITEM_A11Y_LABEL_UNKNOWN.png.sha1 deleted file mode 100644 index 697c12cd4175fc..00000000000000 --- a/chrome/app/chromeos_strings_grdp/IDS_BLUETOOTH_PAIRINGS_DEVICE_ITEM_A11Y_LABEL_UNKNOWN.png.sha1 +++ /dev/null @@ -1 +0,0 @@ -384318d0fe4e3716ffb32ae3c5ad279fc246acbb \ No newline at end of file diff --git a/chrome/app/chromeos_strings_grdp/IDS_BLUETOOTH_PAIRINGS_DEVICE_ITEM_A11Y_LABEL_VIDEO_CAMERA.png.sha1 b/chrome/app/chromeos_strings_grdp/IDS_BLUETOOTH_PAIRINGS_DEVICE_ITEM_A11Y_LABEL_VIDEO_CAMERA.png.sha1 deleted file mode 100644 index 697c12cd4175fc..00000000000000 --- a/chrome/app/chromeos_strings_grdp/IDS_BLUETOOTH_PAIRINGS_DEVICE_ITEM_A11Y_LABEL_VIDEO_CAMERA.png.sha1 +++ /dev/null @@ -1 +0,0 @@ -384318d0fe4e3716ffb32ae3c5ad279fc246acbb \ No newline at end of file diff --git a/chrome/app/chromeos_strings_grdp/IDS_BLUETOOTH_PAIRINGS_DEVICE_ITEM_SECONDARY_ERROR_A11Y_LABEL.png.sha1 b/chrome/app/chromeos_strings_grdp/IDS_BLUETOOTH_PAIRINGS_DEVICE_ITEM_SECONDARY_ERROR_A11Y_LABEL.png.sha1 deleted file mode 100644 index d135cad7083a03..00000000000000 --- a/chrome/app/chromeos_strings_grdp/IDS_BLUETOOTH_PAIRINGS_DEVICE_ITEM_SECONDARY_ERROR_A11Y_LABEL.png.sha1 +++ /dev/null @@ -1 +0,0 @@ -abd68cb9e244d8a82451b5f4567a847b2d4e3def \ No newline at end of file diff --git a/chrome/app/chromeos_strings_grdp/IDS_BLUETOOTH_PAIRINGS_DEVICE_ITEM_SECONDARY_PAIRING_A11Y_LABEL.png.sha1 b/chrome/app/chromeos_strings_grdp/IDS_BLUETOOTH_PAIRINGS_DEVICE_ITEM_SECONDARY_PAIRING_A11Y_LABEL.png.sha1 deleted file mode 100644 index ea08739888c7c1..00000000000000 --- a/chrome/app/chromeos_strings_grdp/IDS_BLUETOOTH_PAIRINGS_DEVICE_ITEM_SECONDARY_PAIRING_A11Y_LABEL.png.sha1 +++ /dev/null @@ -1 +0,0 @@ -18fe36a18e4e679249ef2ae6ea6490cb8bacf28d \ No newline at end of file diff --git a/chrome/browser/ui/webui/chromeos/bluetooth_shared_load_time_data_provider.cc b/chrome/browser/ui/webui/chromeos/bluetooth_shared_load_time_data_provider.cc index 7885bbd56e6275..8b77a80466a9e7 100644 --- a/chrome/browser/ui/webui/chromeos/bluetooth_shared_load_time_data_provider.cc +++ b/chrome/browser/ui/webui/chromeos/bluetooth_shared_load_time_data_provider.cc @@ -46,28 +46,6 @@ void AddLocalizedStrings(content::WebUIDataSource* html_source) { {"bluetoothConfirmCodeMessage", IDS_BLUETOOTH_PAIRING_CONFIRM_CODE_MESSAGE}, {"bluetoothPairingEnterKeys", IDS_BLUETOOTH_PAIRING_ENTER_KEYS}, - {"bluetoothPairingDeviceItemA11YLabelUnknown", - IDS_BLUETOOTH_PAIRINGS_DEVICE_ITEM_A11Y_LABEL_UNKNOWN}, - {"bluetoothPairingDeviceItemA11YLabelComputer", - IDS_BLUETOOTH_PAIRINGS_DEVICE_ITEM_A11Y_LABEL_COMPUTER}, - {"bluetoothPairingDeviceItemA11YLabelPhone", - IDS_BLUETOOTH_PAIRINGS_DEVICE_ITEM_A11Y_LABEL_PHONE}, - {"bluetoothPairingDeviceItemA11YLabelHeadset", - IDS_BLUETOOTH_PAIRINGS_DEVICE_ITEM_A11Y_LABEL_HEADSET}, - {"bluetoothPairingDeviceItemA11YLabelVideoCamera", - IDS_BLUETOOTH_PAIRINGS_DEVICE_ITEM_A11Y_LABEL_VIDEO_CAMERA}, - {"bluetoothPairingDeviceItemA11YLabelGameContoller", - IDS_BLUETOOTH_PAIRINGS_DEVICE_ITEM_A11Y_LABEL_GAME_CONTROLLER}, - {"bluetoothPairingDeviceItemA11YLabelKeyboard", - IDS_BLUETOOTH_PAIRINGS_DEVICE_ITEM_A11Y_LABEL_KEYBOARD}, - {"bluetoothPairingDeviceItemA11YLabelMouse", - IDS_BLUETOOTH_PAIRINGS_DEVICE_ITEM_A11Y_LABEL_MOUSE}, - {"bluetoothPairingDeviceItemA11YLabelTablet", - IDS_BLUETOOTH_PAIRINGS_DEVICE_ITEM_A11Y_LABEL_TABLET}, - {"bluetoothPairingDeviceItemSecondaryErrorA11YLabel", - IDS_BLUETOOTH_PAIRINGS_DEVICE_ITEM_SECONDARY_ERROR_A11Y_LABEL}, - {"bluetoothPairingDeviceItemSecondaryPairingA11YLabel", - IDS_BLUETOOTH_PAIRINGS_DEVICE_ITEM_SECONDARY_PAIRING_A11Y_LABEL}, // Device connecting and pairing. // These ids are generated in JS using 'bluetooth_' + a value from // bluetoothPrivate.PairingEventType (see bluetooth_private.idl). diff --git a/chrome/test/data/webui/cr_components/chromeos/bluetooth/bluetooth_pairing_device_item_test.js b/chrome/test/data/webui/cr_components/chromeos/bluetooth/bluetooth_pairing_device_item_test.js index 9d94042e2a3903..efb55f0f08c6da 100644 --- a/chrome/test/data/webui/cr_components/chromeos/bluetooth/bluetooth_pairing_device_item_test.js +++ b/chrome/test/data/webui/cr_components/chromeos/bluetooth/bluetooth_pairing_device_item_test.js @@ -86,9 +86,9 @@ suite('CrComponentsBluetoothPairingDeviceItemTest', function() { }); test('Pairing message is shown', async function() { - const deviceName = 'BeatsX'; const device = createDefaultBluetoothDevice( - /*id=*/ '12//345&6789', deviceName, + /*id=*/ '12//345&6789', + /*publicName=*/ 'BeatsX', /*connectionState=*/ chromeos.bluetoothConfig.mojom.DeviceConnectionState.kConnected, /*nickname=*/ 'device1', @@ -98,28 +98,11 @@ suite('CrComponentsBluetoothPairingDeviceItemTest', function() { bluetoothPairingDeviceItem.device = device.deviceProperties; await flushAsync(); - const itemIndex = 1; - const listSize = 10; - bluetoothPairingDeviceItem.itemIndex = itemIndex; - bluetoothPairingDeviceItem.listSize = listSize; - const getSecondaryLabel = () => bluetoothPairingDeviceItem.shadowRoot.querySelector('#secondaryLabel'); - const getItemSecondaryA11yLabel = () => - bluetoothPairingDeviceItem.shadowRoot.querySelector('.text-row') - .ariaLabel; - const getItemA11yLabel = () => - bluetoothPairingDeviceItem.shadowRoot.querySelector('#container') - .ariaLabel; assertTrue(!!getSecondaryLabel()); assertEquals('', getSecondaryLabel().textContent.trim()); - assertEquals( - getItemA11yLabel(), - bluetoothPairingDeviceItem.i18n( - 'bluetoothPairingDeviceItemA11YLabelMouse', itemIndex + 1, listSize, - deviceName)); - assertEquals(getItemSecondaryA11yLabel(), ''); bluetoothPairingDeviceItem.deviceItemState = DeviceItemState.PAIRING; await flushAsync(); @@ -127,10 +110,6 @@ suite('CrComponentsBluetoothPairingDeviceItemTest', function() { assertEquals( bluetoothPairingDeviceItem.i18n('bluetoothPairing'), getSecondaryLabel().textContent.trim()); - assertEquals( - getItemSecondaryA11yLabel(), - bluetoothPairingDeviceItem.i18n( - 'bluetoothPairingDeviceItemSecondaryPairingA11YLabel', deviceName)); bluetoothPairingDeviceItem.deviceItemState = DeviceItemState.FAILED; await flushAsync(); @@ -138,10 +117,6 @@ suite('CrComponentsBluetoothPairingDeviceItemTest', function() { assertEquals( bluetoothPairingDeviceItem.i18n('bluetoothPairingFailed'), getSecondaryLabel().textContent.trim()); - assertEquals( - getItemSecondaryA11yLabel(), - bluetoothPairingDeviceItem.i18n( - 'bluetoothPairingDeviceItemSecondaryErrorA11YLabel', deviceName)); bluetoothPairingDeviceItem.deviceItemState = DeviceItemState.DEFAULT; await flushAsync(); diff --git a/ui/webui/resources/cr_components/chromeos/bluetooth/bluetooth_pairing_device_item.html b/ui/webui/resources/cr_components/chromeos/bluetooth/bluetooth_pairing_device_item.html index eebb09c87da75f..32693121be0b58 100644 --- a/ui/webui/resources/cr_components/chromeos/bluetooth/bluetooth_pairing_device_item.html +++ b/ui/webui/resources/cr_components/chromeos/bluetooth/bluetooth_pairing_device_item.html @@ -30,30 +30,15 @@ } -
-
- -
- - +
+ +
+ [[getDeviceName_(device.*)]] + +
+ [[secondaryLabel_]]
diff --git a/ui/webui/resources/cr_components/chromeos/bluetooth/bluetooth_pairing_device_item.js b/ui/webui/resources/cr_components/chromeos/bluetooth/bluetooth_pairing_device_item.js index f8d84305c44130..21849587b9d7b9 100644 --- a/ui/webui/resources/cr_components/chromeos/bluetooth/bluetooth_pairing_device_item.js +++ b/ui/webui/resources/cr_components/chromeos/bluetooth/bluetooth_pairing_device_item.js @@ -12,8 +12,9 @@ import './bluetooth_icon.js'; import {I18nBehavior, I18nBehaviorInterface} from '//resources/js/i18n_behavior.m.js'; import {html, mixinBehaviors, PolymerElement} from '//resources/polymer/v3_0/polymer/polymer_bundled.min.js'; -import {FocusRowBehavior} from 'chrome://resources/js/cr/ui/focus_row_behavior.m.js'; + import {assertNotReached} from '../../../js/assert.m.js'; + import {DeviceItemState} from './bluetooth_types.js'; import {mojoString16ToString} from './bluetooth_utils.js'; @@ -23,7 +24,7 @@ import {mojoString16ToString} from './bluetooth_utils.js'; * @extends {PolymerElement} */ const SettingsBluetoothPairingDeviceItemElementBase = - mixinBehaviors([I18nBehavior, FocusRowBehavior], PolymerElement); + mixinBehaviors([I18nBehavior], PolymerElement); /** @polymer */ export class SettingsBluetoothPairingDeviceItemElement extends @@ -49,15 +50,6 @@ export class SettingsBluetoothPairingDeviceItemElement extends value: DeviceItemState.DEFAULT, }, - /** The index of this item in its parent list, used for its a11y label. */ - itemIndex: Number, - - /** - * The total number of elements in this item's parent list, used for its - * a11y label. - */ - listSize: Number, - /** @private {string} */ secondaryLabel_: { type: String, @@ -139,66 +131,6 @@ export class SettingsBluetoothPairingDeviceItemElement extends detail: {device: this.device}, })); } - - /** - * @return {string} - * @private - */ - getAriaLabel_() { - return this.i18n( - this.getA11yLabelMessageId_(), this.itemIndex + 1, this.listSize, - this.getDeviceName_()); - } - - /** - * @return {string} - * @private - */ - getA11yLabelMessageId_() { - const deviceType = chromeos.bluetoothConfig.mojom.DeviceType; - switch (this.device.deviceType) { - case deviceType.kUnknown: - return 'bluetoothPairingDeviceItemA11YLabelUnknown'; - case deviceType.kComputer: - return 'bluetoothPairingDeviceItemA11YLabelComputer'; - case deviceType.kPhone: - return 'bluetoothPairingDeviceItemA11YLabelPhone'; - case deviceType.kHeadset: - return 'bluetoothPairingDeviceItemA11YLabelHeadset'; - case deviceType.kVideoCamera: - return 'bluetoothPairingDeviceItemA11YLabelVideoCamera'; - case deviceType.kGameController: - return 'bluetoothPairingDeviceItemA11YLabelGameContoller'; - case deviceType.kKeyboard: - return 'bluetoothPairingDeviceItemA11YLabelKeyboard'; - case deviceType.kMouse: - return 'bluetoothPairingDeviceItemA11YLabelMouse'; - case deviceType.kTablet: - return 'bluetoothPairingDeviceItemA11YLabelTablet'; - default: - assertNotReached(); - } - } - - /** - * @return {string} - * @private - */ - getSecondaryAriaLabel_() { - const deviceName = this.getDeviceName_(); - switch (this.deviceItemState) { - case DeviceItemState.FAILED: - return this.i18n( - 'bluetoothPairingDeviceItemSecondaryErrorA11YLabel', deviceName); - case DeviceItemState.PAIRING: - return this.i18n( - 'bluetoothPairingDeviceItemSecondaryPairingA11YLabel', deviceName); - case DeviceItemState.DEFAULT: - return ''; - default: - assertNotReached(); - } - } } customElements.define( diff --git a/ui/webui/resources/cr_components/chromeos/bluetooth/bluetooth_pairing_device_selection_page.html b/ui/webui/resources/cr_components/chromeos/bluetooth/bluetooth_pairing_device_selection_page.html index 618dba864850b1..a2b53ce486d115 100644 --- a/ui/webui/resources/cr_components/chromeos/bluetooth/bluetooth_pairing_device_selection_page.html +++ b/ui/webui/resources/cr_components/chromeos/bluetooth/bluetooth_pairing_device_selection_page.html @@ -24,26 +24,17 @@
[[getDeviceListTitle_(devices.*, isBluetoothEnabled)]]
+