Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2525303002:
Don't duplicate the Subresource Filter prompt after it was shown already. (Closed)
DescriptionDon't duplicate the Subresource Filter prompt after it was shown already.
BUG=667751
Committed: https://crrev.com/f5b227bac298b089fe110e3ec686e5711a572448
Cr-Commit-Position: refs/heads/master@{#434948}
Patch Set 1 #Patch Set 2 : . #
Total comments: 6
Patch Set 3 : adressed comments #Patch Set 4 : rebased #
Total comments: 10
Patch Set 5 : adressed comments #Patch Set 6 : fix test #Patch Set 7 : . #
Messages
Total messages: 52 (44 generated)
The CQ bit was checked by melandory@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: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...) win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
Description was changed from ========== debug BUG=667751 ========== to ========== Don't duplicate the Subresource Filter prompt after it was shown already. BUG=667751 ==========
melandory@chromium.org changed reviewers: + engedy@chromium.org
PTAL, thanks!
https://codereview.chromium.org/2525303002/diff/20001/chrome/browser/subresou... File chrome/browser/subresource_filter/subresource_filter_browsertest.cc (right): https://codereview.chromium.org/2525303002/diff/20001/chrome/browser/subresou... chrome/browser/subresource_filter/subresource_filter_browsertest.cc:33: } nit: blank line before, `// namespace` after. https://codereview.chromium.org/2525303002/diff/20001/chrome/browser/subresou... chrome/browser/subresource_filter/subresource_filter_browsertest.cc:167: IN_PROC_BROWSER_TEST_F(SubresourceFilterBrowserTest, Can you please add another test that verifies that for two subsequent navigations, the bubble is shown twice? https://codereview.chromium.org/2525303002/diff/20001/chrome/test/data/subres... File chrome/test/data/subresource_filter/frame_with_button.html (right): https://codereview.chromium.org/2525303002/diff/20001/chrome/test/data/subres... chrome/test/data/subresource_filter/frame_with_button.html:3: function clicky() { Do we actually need a button here? Have you considered having a similar function live that would dynamically inject an explicitly non-async script, and call that function from the browsertest and see if it executed?
The CQ bit was checked by melandory@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_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by melandory@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_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by melandory@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...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by melandory@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...
Patchset #4 (id:60001) has been deleted
https://codereview.chromium.org/2525303002/diff/20001/chrome/browser/subresou... File chrome/browser/subresource_filter/subresource_filter_browsertest.cc (right): https://codereview.chromium.org/2525303002/diff/20001/chrome/browser/subresou... chrome/browser/subresource_filter/subresource_filter_browsertest.cc:33: } On 2016/11/24 15:05:11, engedy wrote: > nit: blank line before, `// namespace` after. Done. https://codereview.chromium.org/2525303002/diff/20001/chrome/browser/subresou... chrome/browser/subresource_filter/subresource_filter_browsertest.cc:167: IN_PROC_BROWSER_TEST_F(SubresourceFilterBrowserTest, On 2016/11/24 15:05:11, engedy wrote: > Can you please add another test that verifies that for two subsequent > navigations, the bubble is shown twice? Done. https://codereview.chromium.org/2525303002/diff/20001/chrome/test/data/subres... File chrome/test/data/subresource_filter/frame_with_button.html (right): https://codereview.chromium.org/2525303002/diff/20001/chrome/test/data/subres... chrome/test/data/subresource_filter/frame_with_button.html:3: function clicky() { On 2016/11/24 15:05:11, engedy wrote: > Do we actually need a button here? Have you considered having a similar function > live that would dynamically inject an explicitly non-async script, and call that > function from the browsertest and see if it executed? Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
LGTM % comments. Now that the image is no longer needed, could you please remove it from this patch? https://codereview.chromium.org/2525303002/diff/80001/chrome/browser/subresou... File chrome/browser/subresource_filter/subresource_filter_browsertest.cc (right): https://codereview.chromium.org/2525303002/diff/80001/chrome/browser/subresou... chrome/browser/subresource_filter/subresource_filter_browsertest.cc:26: #include "testing/gmock/include/gmock/gmock-generated-matchers.h" nit: Include gmock.h instead, which is the public API. https://codereview.chromium.org/2525303002/diff/80001/chrome/browser/subresou... chrome/browser/subresource_filter/subresource_filter_browsertest.cc:169: MultipleSubresourcesBlocked) { nit: Please update test name. Suggestion: `PromptShownAgainOnNextNavigation`. https://codereview.chromium.org/2525303002/diff/80001/chrome/browser/subresou... chrome/browser/subresource_filter/subresource_filter_browsertest.cc:170: ASSERT_NO_FATAL_FAILURE( nit: The next line overrides this, could this be deleted? https://codereview.chromium.org/2525303002/diff/80001/chrome/test/data/subres... File chrome/test/data/subresource_filter/frame_image.html (right): https://codereview.chromium.org/2525303002/diff/80001/chrome/test/data/subres... chrome/test/data/subresource_filter/frame_image.html:4: <iframe name="two" src="frame_with_sync_script.html"></iframe> To keep the number of test HTML pages low, could you please just add this as frame "three" into frame_set.html? https://codereview.chromium.org/2525303002/diff/80001/chrome/test/data/subres... File chrome/test/data/subresource_filter/frame_with_sync_script.html (right): https://codereview.chromium.org/2525303002/diff/80001/chrome/test/data/subres... chrome/test/data/subresource_filter/frame_with_sync_script.html:1: <html> nit: To better reflect that this is performing the subresource load in a delayed manner, can we call this file frame_with_delayed_script or frame_with_dynamic[ally_loaded]_script? We could point out why `async = false` is important (i.e. to make sure the test would non-flakily fail if the subresource is allowed to load) in a comment below.
The CQ bit was checked by melandory@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_chromeos_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 melandory@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_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by melandory@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_...)
The CQ bit was checked by melandory@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.
https://codereview.chromium.org/2525303002/diff/80001/chrome/browser/subresou... File chrome/browser/subresource_filter/subresource_filter_browsertest.cc (right): https://codereview.chromium.org/2525303002/diff/80001/chrome/browser/subresou... chrome/browser/subresource_filter/subresource_filter_browsertest.cc:26: #include "testing/gmock/include/gmock/gmock-generated-matchers.h" On 2016/11/25 14:46:34, engedy wrote: > nit: Include gmock.h instead, which is the public API. Done. https://codereview.chromium.org/2525303002/diff/80001/chrome/browser/subresou... chrome/browser/subresource_filter/subresource_filter_browsertest.cc:169: MultipleSubresourcesBlocked) { On 2016/11/25 14:46:34, engedy wrote: > nit: Please update test name. Suggestion: `PromptShownAgainOnNextNavigation`. Done. https://codereview.chromium.org/2525303002/diff/80001/chrome/browser/subresou... chrome/browser/subresource_filter/subresource_filter_browsertest.cc:170: ASSERT_NO_FATAL_FAILURE( On 2016/11/25 14:46:34, engedy wrote: > nit: The next line overrides this, could this be deleted? Done. https://codereview.chromium.org/2525303002/diff/80001/chrome/test/data/subres... File chrome/test/data/subresource_filter/frame_image.html (right): https://codereview.chromium.org/2525303002/diff/80001/chrome/test/data/subres... chrome/test/data/subresource_filter/frame_image.html:4: <iframe name="two" src="frame_with_sync_script.html"></iframe> On 2016/11/25 14:46:34, engedy wrote: > To keep the number of test HTML pages low, could you please just add this as > frame "three" into frame_set.html? Done. https://codereview.chromium.org/2525303002/diff/80001/chrome/test/data/subres... File chrome/test/data/subresource_filter/frame_with_sync_script.html (right): https://codereview.chromium.org/2525303002/diff/80001/chrome/test/data/subres... chrome/test/data/subresource_filter/frame_with_sync_script.html:1: <html> On 2016/11/25 14:46:34, engedy wrote: > nit: To better reflect that this is performing the subresource load in a delayed > manner, can we call this file frame_with_delayed_script or > frame_with_dynamic[ally_loaded]_script? We could point out why `async = false` > is important (i.e. to make sure the test would non-flakily fail if the > subresource is allowed to load) in a comment below. Done.
The CQ bit was checked by melandory@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/2525303002/#ps140001 (title: ".")
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": 140001, "attempt_start_ts": 1480415101170850,
"parent_rev": "014ac951ce3f8245c478c3d72ad0571326de281a", "commit_rev":
"46f98a8d9d98d0f0bb2a80f54f0193726a26c0d6"}
Message was sent while issue was closed.
Description was changed from ========== Don't duplicate the Subresource Filter prompt after it was shown already. BUG=667751 ========== to ========== Don't duplicate the Subresource Filter prompt after it was shown already. BUG=667751 ==========
Message was sent while issue was closed.
Committed patchset #7 (id:140001)
Message was sent while issue was closed.
Description was changed from ========== Don't duplicate the Subresource Filter prompt after it was shown already. BUG=667751 ========== to ========== Don't duplicate the Subresource Filter prompt after it was shown already. BUG=667751 Committed: https://crrev.com/f5b227bac298b089fe110e3ec686e5711a572448 Cr-Commit-Position: refs/heads/master@{#434948} ==========
Message was sent while issue was closed.
Patchset 7 (id:??) landed as https://crrev.com/f5b227bac298b089fe110e3ec686e5711a572448 Cr-Commit-Position: refs/heads/master@{#434948} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
