Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2667303003:
Disable subresource filtering on page reloads. (Closed)
|
Created:
3 years, 10 months ago by Bryan McQuade Modified:
3 years, 10 months ago CC:
blundell+watchlist_chromium.org, chromium-reviews, darin-cc_chromium.org, droger+watchlist_chromium.org, jam, sdefresne+watchlist_chromium.org, subresource-filter-reviews_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionDisable subresource filtering on page reloads.
BUG=688058
Review-Url: https://codereview.chromium.org/2667303003
Cr-Commit-Position: refs/heads/master@{#449091}
Committed: https://chromium.googlesource.com/chromium/src/+/0766bcb584f9dc069573be95e06e019b37d280e0
Patch Set 1 #Patch Set 2 : fix comment #Patch Set 3 : improve reload detection #Patch Set 4 : cleanup #Patch Set 5 : remove whitespace #Patch Set 6 : add field trial support #Patch Set 7 : remove browser side nav check #Patch Set 8 : fix comment #Patch Set 9 : fix test #
Total comments: 4
Patch Set 10 : address comment #Patch Set 11 : add browser tests #Patch Set 12 : suppress activation for subsequent same origin navs in same tab #Patch Set 13 : rebase #
Total comments: 11
Patch Set 14 : address comments #Patch Set 15 : address comments #Patch Set 16 : add dep on ui/base #
Total comments: 1
Patch Set 17 : move ui/base to public_deps #Messages
Total messages: 85 (65 generated)
The CQ bit was checked by bmcquade@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...
The CQ bit was checked by bmcquade@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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
The CQ bit was checked by bmcquade@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...
Description was changed from ========== Disable subresource filtering on page reloads. BUG= ========== to ========== Disable subresource filtering on page reloads. BUG=688058 ==========
The CQ bit was checked by bmcquade@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...
The CQ bit was checked by bmcquade@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...
The CQ bit was checked by bmcquade@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...
bmcquade@chromium.org changed reviewers: + engedy@chromium.org
PTAL, thanks!
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: 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 bmcquade@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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: 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_...)
On 2017/02/02 at 19:35:20, Bryan McQuade wrote: > PTAL, thanks! Note that browser side navigation tests are broken - I need to figure out what's going on there. But the rest of the change is ready for review - happy to get early feedback from you in the meantime.
The CQ bit was checked by bmcquade@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...
The CQ bit was checked by bmcquade@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...
On 2017/02/03 at 01:22:27, Bryan McQuade wrote: > On 2017/02/02 at 19:35:20, Bryan McQuade wrote: > > PTAL, thanks! > > Note that browser side navigation tests are broken - I need to figure out what's going on there. But the rest of the change is ready for review - happy to get early feedback from you in the meantime. I've disabled the one affected test when browser side navigation is enabled, and filed crbug.com/688393 about the underlying issue. So this is now ready for review, thanks! Note that the browser side nav issue is specific to tests; we see the correct behavior in the browser when browser side nav is enabled.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Taking a look now -- in the meantime, could you please add a browser test for this in SubresourceFilterBrowserTest?
https://codereview.chromium.org/2667303003/diff/160001/components/subresource... File components/subresource_filter/content/browser/content_subresource_filter_driver_factory.cc (right): https://codereview.chromium.org/2667303003/diff/160001/components/subresource... components/subresource_filter/content/browser/content_subresource_filter_driver_factory.cc:195: client_->ToggleNotificationVisibility(false); Note that calling this is not idempotent because of metrics being recorded inside. Let's call this only on DidStartNavigation for now, and fix this sad state in a follow-up CL. https://codereview.chromium.org/2667303003/diff/160001/components/subresource... components/subresource_filter/content/browser/content_subresource_filter_driver_factory.cc:305: // Some pages 'reload' from JavaScript by navigating to themselves. Let's add two browser tests, one for each reload flavor.
The CQ bit was checked by bmcquade@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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by bmcquade@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...
Thanks! Added tests. https://codereview.chromium.org/2667303003/diff/160001/components/subresource... File components/subresource_filter/content/browser/content_subresource_filter_driver_factory.cc (right): https://codereview.chromium.org/2667303003/diff/160001/components/subresource... components/subresource_filter/content/browser/content_subresource_filter_driver_factory.cc:195: client_->ToggleNotificationVisibility(false); On 2017/02/03 at 17:41:52, engedy wrote: > Note that calling this is not idempotent because of metrics being recorded inside. Let's call this only on DidStartNavigation for now, and fix this sad state in a follow-up CL. Good point, it's non-intuitive that a Reset() method would have potential UI side effects, so I'll remove this. Thanks for catching it! https://codereview.chromium.org/2667303003/diff/160001/components/subresource... components/subresource_filter/content/browser/content_subresource_filter_driver_factory.cc:305: // Some pages 'reload' from JavaScript by navigating to themselves. On 2017/02/03 at 17:41:52, engedy wrote: > Let's add two browser tests, one for each reload flavor. Added unit tests and browser tests for both.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Description was changed from ========== Disable subresource filtering on page reloads. BUG=688058 ========== to ========== Disable subresource filtering on page reloads. BUG=688058 ==========
The CQ bit was checked by bmcquade@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...
I've updated this change to whitelist the origin of domains for reloaded URLs, using the existing whitelisting mechanism bound to the lifetime of a tab. PTAL, thanks!
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by bmcquade@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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
LGTM % comments, thanks! https://codereview.chromium.org/2667303003/diff/240001/components/subresource... File components/subresource_filter/content/browser/content_subresource_filter_driver_factory.cc (right): https://codereview.chromium.org/2667303003/diff/240001/components/subresource... components/subresource_filter/content/browser/content_subresource_filter_driver_factory.cc:196: void ContentSubresourceFilterDriverFactory::Reset() { nit: Trying to think about a more descriptive name for this, maybe ResetActivationState? https://codereview.chromium.org/2667303003/diff/240001/components/subresource... components/subresource_filter/content/browser/content_subresource_filter_driver_factory.cc:308: const bool is_reload = nit: Let's extract this to a function in the anonymous namespace at the top. This way, we could name the logic but avoid the awkward formatting and conditionals here: if (ShouldSuppressActivationOnReload() && NavigationIsPageReload(url, referrer, transitions)) { .. } Naming of the helper function above is for illustration, and up to you. https://codereview.chromium.org/2667303003/diff/240001/components/subresource... components/subresource_filter/content/browser/content_subresource_filter_driver_factory.cc:313: AddHostOfURLToWhitelistSet(url); nit: Adding |url| to the whitelist will immediately result in ShouldActivateForMainFrameURL() returning false below, we could just rely on that to simplify things. If you think it's useful to the reader, we could add a comment to clarify this, along the lines: // Whitelist this host for the current as well as subsequent navigations. https://codereview.chromium.org/2667303003/diff/240001/components/subresource... File components/subresource_filter/content/browser/content_subresource_filter_driver_factory_unittest.cc (right): https://codereview.chromium.org/2667303003/diff/240001/components/subresource... components/subresource_filter/content/browser/content_subresource_filter_driver_factory_unittest.cc:552: TEST_F(ContentSubresourceFilterDriverFactoryTest, Do I understand correctly that presently there is no way to simulate a navigation with a custom transition type and/or referer using RenderFrameHostTester and friends; so essentially we can only have this unittest for non-PlzNavigate world because we are (forced to) manually calling ReadyToCommitNavigationInternal? https://codereview.chromium.org/2667303003/diff/240001/components/subresource... File components/subresource_filter/core/browser/subresource_filter_features.cc (right): https://codereview.chromium.org/2667303003/diff/240001/components/subresource... components/subresource_filter/core/browser/subresource_filter_features.cc:44: "suppress_activation_on_reload"; nit: Let's update this (and related names around this file) to reflect new semantics. How about one of these (in decreasing order of my preference): whitelist_site_on_reload = true|false whitelist_host_on_reload = true|false whitelist_on_reload = nothing|page|site (if we ever plan to add support for `page` whitelisting)
The CQ bit was checked by bmcquade@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...
The CQ bit was checked by bmcquade@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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Addressed comments, thanks! https://codereview.chromium.org/2667303003/diff/240001/components/subresource... File components/subresource_filter/content/browser/content_subresource_filter_driver_factory.cc (right): https://codereview.chromium.org/2667303003/diff/240001/components/subresource... components/subresource_filter/content/browser/content_subresource_filter_driver_factory.cc:196: void ContentSubresourceFilterDriverFactory::Reset() { On 2017/02/08 at 10:48:31, engedy wrote: > nit: Trying to think about a more descriptive name for this, maybe ResetActivationState? Done https://codereview.chromium.org/2667303003/diff/240001/components/subresource... components/subresource_filter/content/browser/content_subresource_filter_driver_factory.cc:308: const bool is_reload = On 2017/02/08 at 10:48:32, engedy wrote: > nit: Let's extract this to a function in the anonymous namespace at the top. This way, we could name the logic but avoid the awkward formatting and conditionals here: > > if (ShouldSuppressActivationOnReload() && > NavigationIsPageReload(url, referrer, transitions)) { > .. > } > > Naming of the helper function above is for illustration, and up to you. Done https://codereview.chromium.org/2667303003/diff/240001/components/subresource... components/subresource_filter/content/browser/content_subresource_filter_driver_factory.cc:313: AddHostOfURLToWhitelistSet(url); On 2017/02/08 at 10:48:32, engedy wrote: > nit: Adding |url| to the whitelist will immediately result in ShouldActivateForMainFrameURL() returning false below, we could just rely on that to simplify things. > > If you think it's useful to the reader, we could add a comment to clarify this, along the lines: > // Whitelist this host for the current as well as subsequent navigations. Done https://codereview.chromium.org/2667303003/diff/240001/components/subresource... File components/subresource_filter/content/browser/content_subresource_filter_driver_factory_unittest.cc (right): https://codereview.chromium.org/2667303003/diff/240001/components/subresource... components/subresource_filter/content/browser/content_subresource_filter_driver_factory_unittest.cc:552: TEST_F(ContentSubresourceFilterDriverFactoryTest, On 2017/02/08 at 10:48:32, engedy wrote: > Do I understand correctly that presently there is no way to simulate a navigation with a custom transition type and/or referer using RenderFrameHostTester and friends; so essentially we can only have this unittest for non-PlzNavigate world because we are (forced to) manually calling ReadyToCommitNavigationInternal? It should be possible to make the tests work with plznav (the base class for the test fixture has a Reload() method, for instance). I spent some time trying to get the plznav flow working, but it added enough branching complexity that I concluded it best for now to defer that work until the plznav bug is fixed. https://codereview.chromium.org/2667303003/diff/240001/components/subresource... File components/subresource_filter/core/browser/subresource_filter_features.cc (right): https://codereview.chromium.org/2667303003/diff/240001/components/subresource... components/subresource_filter/core/browser/subresource_filter_features.cc:44: "suppress_activation_on_reload"; On 2017/02/08 at 10:48:32, engedy wrote: > nit: Let's update this (and related names around this file) to reflect new semantics. How about one of these (in decreasing order of my preference): > > whitelist_site_on_reload = true|false > whitelist_host_on_reload = true|false > whitelist_on_reload = nothing|page|site (if we ever plan to add support for `page` whitelisting) Good idea, done!
The CQ bit was checked by bmcquade@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from engedy@chromium.org Link to the patchset: https://codereview.chromium.org/2667303003/#ps280001 (title: "address comments")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2667303003/diff/240001/components/subresource... File components/subresource_filter/content/browser/content_subresource_filter_driver_factory_unittest.cc (right): https://codereview.chromium.org/2667303003/diff/240001/components/subresource... components/subresource_filter/content/browser/content_subresource_filter_driver_factory_unittest.cc:552: TEST_F(ContentSubresourceFilterDriverFactoryTest, On 2017/02/08 17:42:21, Bryan McQuade wrote: > On 2017/02/08 at 10:48:32, engedy wrote: > > Do I understand correctly that presently there is no way to simulate a > navigation with a custom transition type and/or referer using > RenderFrameHostTester and friends; so essentially we can only have this unittest > for non-PlzNavigate world because we are (forced to) manually calling > ReadyToCommitNavigationInternal? > > It should be possible to make the tests work with plznav (the base class for the > test fixture has a Reload() method, for instance). I spent some time trying to > get the plznav flow working, but it added enough branching complexity that I > concluded it best for now to defer that work until the plznav bug is fixed. Ack, thanks for investigating!
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
bmcquade@chromium.org changed reviewers: + thakis@chromium.org
thakis, PTAL for dependency on ui/base/page_transition_types.h
don't you need to edit some .gn file to add a dep on //ui/base?
The CQ bit was checked by bmcquade@chromium.org to run a CQ dry run
On 2017/02/08 at 18:25:47, thakis wrote: > don't you need to edit some .gn file to add a dep on //ui/base? done, thanks!
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
lgtm
https://codereview.chromium.org/2667303003/diff/300001/components/subresource... File components/subresource_filter/content/browser/content_subresource_filter_driver_factory.h (right): https://codereview.chromium.org/2667303003/diff/300001/components/subresource... components/subresource_filter/content/browser/content_subresource_filter_driver_factory.h:20: #include "ui/base/page_transition_types.h" (since you're including the header in a header, you might want public_deps instead of deps if this header here is used outside of the target it's in)
The CQ bit was checked by bmcquade@chromium.org to run a CQ dry run
On 2017/02/08 at 18:35:17, thakis wrote: > https://codereview.chromium.org/2667303003/diff/300001/components/subresource... > File components/subresource_filter/content/browser/content_subresource_filter_driver_factory.h (right): > > https://codereview.chromium.org/2667303003/diff/300001/components/subresource... > components/subresource_filter/content/browser/content_subresource_filter_driver_factory.h:20: #include "ui/base/page_transition_types.h" > (since you're including the header in a header, you might want public_deps instead of deps if this header here is used outside of the target it's in) ah, thank you! updated.
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.
The CQ bit was checked by bmcquade@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from engedy@chromium.org, thakis@chromium.org Link to the patchset: https://codereview.chromium.org/2667303003/#ps320001 (title: "move ui/base to public_deps")
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": 320001, "attempt_start_ts": 1486587947941600,
"parent_rev": "903b3929e9ea7f4403bea67d27ff954707549006", "commit_rev":
"0766bcb584f9dc069573be95e06e019b37d280e0"}
Message was sent while issue was closed.
Description was changed from ========== Disable subresource filtering on page reloads. BUG=688058 ========== to ========== Disable subresource filtering on page reloads. BUG=688058 Review-Url: https://codereview.chromium.org/2667303003 Cr-Commit-Position: refs/heads/master@{#449091} Committed: https://chromium.googlesource.com/chromium/src/+/0766bcb584f9dc069573be95e06e... ==========
Message was sent while issue was closed.
Committed patchset #17 (id:320001) as https://chromium.googlesource.com/chromium/src/+/0766bcb584f9dc069573be95e06e... |
