Subresource filter global default settings metrics are broken |
||||
Issue descriptionWe seem to set the global setting even when we navigate to e.g. chrome://settings/content/subresourceFilter, without doing any user action It looks like hooks in the web UI cause the page to always set the default setting even if the default is already set? https://cs.chromium.org/chromium/src/chrome/browser/resources/settings/site_settings/category_default_setting.js?rcl=4e0c9590a53bdd1969d3d6ae2c7cfd36c3b30096&l=74 In any case this really messes up our metrics, which should only be changing for user initiated actions. There may be other places where this is set automatically which should be fixed as our UMA metrics show that this is the only content setting metric being reported.
,
May 11 2017
More investigation that the metrics are broken: it looks like flipping the global toggle ends up calling SiteSettingsHandler::HandleSetDefaultValueForContentType three times on my dev build?
,
May 11 2017
,
May 11 2017
,
May 12 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/5ce9a842e75528da201d21f2571eb7dba991d044 commit 5ce9a842e75528da201d21f2571eb7dba991d044 Author: csharrison <csharrison@chromium.org> Date: Fri May 12 19:16:16 2017 [subresource_filter] Duplicate settings notifications should not be logged There are places in the code where global content setting changes somehow notify our observer multiple times in a row. To guard against this in the code, we cache the global setting in the settings observer and only log metrics if a notification is received which ended up changing the cached value. Places which trigger these duplicate notifications should probably be fixed too. BUG= 721396 Review-Url: https://codereview.chromium.org/2873123004 Cr-Commit-Position: refs/heads/master@{#471399} [modify] https://crrev.com/5ce9a842e75528da201d21f2571eb7dba991d044/chrome/browser/subresource_filter/subresource_filter_content_settings_manager.cc [modify] https://crrev.com/5ce9a842e75528da201d21f2571eb7dba991d044/chrome/browser/subresource_filter/subresource_filter_content_settings_manager.h [modify] https://crrev.com/5ce9a842e75528da201d21f2571eb7dba991d044/chrome/browser/subresource_filter/subresource_filter_content_settings_manager_unittest.cc
,
Jun 6 2017
I think this is all that needs to be done for this bug on the subresource filter side. |
||||
►
Sign in to add a comment |
||||
Comment 1 by csharrison@chromium.org
, May 11 2017