Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2873123004:
[subresource_filter] Duplicate settings notifications should not be logged (Closed)
|
Created:
3 years, 7 months ago by Charlie Harrison Modified:
3 years, 7 months ago CC:
chromium-reviews, subresource-filter-reviews_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
Description[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}
Committed: https://chromium.googlesource.com/chromium/src/+/5ce9a842e75528da201d21f2571eb7dba991d044
Patch Set 1 #
Total comments: 2
Patch Set 2 : fix test #
Total comments: 4
Patch Set 3 : shivanisha review #
Messages
Total messages: 32 (16 generated)
csharrison@chromium.org changed reviewers: + msramek@chromium.org
msramek: WDYT about this patch? As mentioned in the linked bug I saw that flipping the toggle on the site settings page notified the observer three times. Additionally, clearing all settings from the settings page notified this observer two times. I am not sure if that is WAI but for our purposes we should just consider that a no-op.
On 2017/05/11 18:23:37, Charlie Harrison wrote: > msramek: WDYT about this patch? As mentioned in the linked bug I saw that > flipping the toggle on the site settings page notified the observer three times. > > Additionally, clearing all settings from the settings page notified this > observer two times. > > I am not sure if that is WAI but for our purposes we should just consider that a > no-op. Oh, and I did some testing of whether this problem occurs for non-global settings and I couldn't ever trigger it.
On 2017/05/11 18:24:46, Charlie Harrison wrote: > On 2017/05/11 18:23:37, Charlie Harrison wrote: > > msramek: WDYT about this patch? As mentioned in the linked bug I saw that > > flipping the toggle on the site settings page notified the observer three > times. > > > > Additionally, clearing all settings from the settings page notified this > > observer two times. > > > > I am not sure if that is WAI but for our purposes we should just consider that > a > > no-op. What specifically do you mean? chrome://settings/content doesn't have a "clear all" button. > > Oh, and I did some testing of whether this problem occurs for non-global > settings and I couldn't ever trigger it. The observer wasn't called?
LGTM. This makes sense, but I have some questions :) https://codereview.chromium.org/2873123004/diff/1/chrome/browser/subresource_... File chrome/browser/subresource_filter/subresource_filter_content_settings_manager_unittest.cc (right): https://codereview.chromium.org/2873123004/diff/1/chrome/browser/subresource_... chrome/browser/subresource_filter/subresource_filter_content_settings_manager_unittest.cc:255: IgnoreDuplicateGlobalSettings) { Does this test really fail if you remove your new code? (i.e. is the duplication really caused by HCSM or is it something in the UI?)
On 2017/05/12 11:36:26, msramek wrote: > On 2017/05/11 18:24:46, Charlie Harrison wrote: > > On 2017/05/11 18:23:37, Charlie Harrison wrote: > > > msramek: WDYT about this patch? As mentioned in the linked bug I saw that > > > flipping the toggle on the site settings page notified the observer three > > times. > > > > > > Additionally, clearing all settings from the settings page notified this > > > observer two times. > > > > > > I am not sure if that is WAI but for our purposes we should just consider > that > > a > > > no-op. > > What specifically do you mean? chrome://settings/content doesn't have a "clear > all" button. Oops sorry I meant the "Restore settings to their original defaults" on the standard chrome://settings page. > > > > > Oh, and I did some testing of whether this problem occurs for non-global > > settings and I couldn't ever trigger it. > > The observer wasn't called? The observer was only called once. i.e. I couldn't reproduce this bug using UI surfaces targeting origin-scoped settings, just global ones.
https://codereview.chromium.org/2873123004/diff/1/chrome/browser/subresource_... File chrome/browser/subresource_filter/subresource_filter_content_settings_manager_unittest.cc (right): https://codereview.chromium.org/2873123004/diff/1/chrome/browser/subresource_... chrome/browser/subresource_filter/subresource_filter_content_settings_manager_unittest.cc:255: IgnoreDuplicateGlobalSettings) { On 2017/05/12 11:39:56, msramek wrote: > Does this test really fail if you remove your new code? (i.e. is the duplication > really caused by HCSM or is it something in the UI?) The test does indeed fail without the new code, but the problem as far as I can tell is still in the UI which calls into HandleSetDefaultValueForContentType three times. Maybe HCSM should filter out these duplicates, but I would place the blame on the bad user of the API than on HCSM not detecting/filtering erroneous API usage.
The CQ bit was checked by csharrison@chromium.org
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by csharrison@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Had to fix a test trivially so that we always set away from what the value current is at. Feel free to take another look if you want.
csharrison@chromium.org changed reviewers: + shivanisha@chromium.org
shivanisha: Would you take a quick look at this too?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Still LGTM.
https://codereview.chromium.org/2873123004/diff/20001/chrome/browser/subresou... File chrome/browser/subresource_filter/subresource_filter_content_settings_manager.cc (right): https://codereview.chromium.org/2873123004/diff/20001/chrome/browser/subresou... chrome/browser/subresource_filter/subresource_filter_content_settings_manager.cc:151: // Ignore changes which retain the status quo. This also avoid logging typo: avoid -> avoids https://codereview.chromium.org/2873123004/diff/20001/chrome/browser/subresou... File chrome/browser/subresource_filter/subresource_filter_content_settings_manager_unittest.cc (right): https://codereview.chromium.org/2873123004/diff/20001/chrome/browser/subresou... chrome/browser/subresource_filter/subresource_filter_content_settings_manager_unittest.cc:118: Why this re-ordering?
The CQ bit was checked by csharrison@chromium.org to run a CQ dry run
https://codereview.chromium.org/2873123004/diff/20001/chrome/browser/subresou... File chrome/browser/subresource_filter/subresource_filter_content_settings_manager.cc (right): https://codereview.chromium.org/2873123004/diff/20001/chrome/browser/subresou... chrome/browser/subresource_filter/subresource_filter_content_settings_manager.cc:151: // Ignore changes which retain the status quo. This also avoid logging On 2017/05/12 16:53:21, shivanisha wrote: > typo: avoid -> avoids Done. https://codereview.chromium.org/2873123004/diff/20001/chrome/browser/subresou... File chrome/browser/subresource_filter/subresource_filter_content_settings_manager_unittest.cc (right): https://codereview.chromium.org/2873123004/diff/20001/chrome/browser/subresou... chrome/browser/subresource_filter/subresource_filter_content_settings_manager_unittest.cc:118: On 2017/05/12 16:53:21, shivanisha wrote: > Why this re-ordering? See the new comment, the setting is in ALLOW state by default so we need to move it to BLOCK to trigger a setting change. I'll update the test to be a bit clearer.
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2017/05/12 at 17:19:11, csharrison wrote: > https://codereview.chromium.org/2873123004/diff/20001/chrome/browser/subresou... > File chrome/browser/subresource_filter/subresource_filter_content_settings_manager.cc (right): > > https://codereview.chromium.org/2873123004/diff/20001/chrome/browser/subresou... > chrome/browser/subresource_filter/subresource_filter_content_settings_manager.cc:151: // Ignore changes which retain the status quo. This also avoid logging > On 2017/05/12 16:53:21, shivanisha wrote: > > typo: avoid -> avoids > > Done. > > https://codereview.chromium.org/2873123004/diff/20001/chrome/browser/subresou... > File chrome/browser/subresource_filter/subresource_filter_content_settings_manager_unittest.cc (right): > > https://codereview.chromium.org/2873123004/diff/20001/chrome/browser/subresou... > chrome/browser/subresource_filter/subresource_filter_content_settings_manager_unittest.cc:118: > On 2017/05/12 16:53:21, shivanisha wrote: > > Why this re-ordering? > > See the new comment, the setting is in ALLOW state by default so we need to move it to BLOCK to trigger a setting change. I'll update the test to be a bit clearer. lgtm
The CQ bit was checked by csharrison@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from msramek@chromium.org Link to the patchset: https://codereview.chromium.org/2873123004/#ps40001 (title: "shivanisha review")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 40001, "attempt_start_ts": 1494616133215730,
"parent_rev": "5418e9a45cc7e18e81cf6e17aa93d8c340eda329", "commit_rev":
"5ce9a842e75528da201d21f2571eb7dba991d044"}
Message was sent while issue was closed.
Description was changed from ========== [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 ========== to ========== [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} Committed: https://chromium.googlesource.com/chromium/src/+/5ce9a842e75528da201d21f2571e... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001) as https://chromium.googlesource.com/chromium/src/+/5ce9a842e75528da201d21f2571e... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
