Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(80)

Issue 2243863002: Send activation signal to subframes. (Closed)

Created:
4 years, 4 months ago by melandory
Modified:
4 years, 3 months ago
Reviewers:
engedy
CC:
chromium-reviews, jam, darin-cc_chromium.org
Base URL:
https://chromium.googlesource.com/chromium/src.git@master
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

Send activation signal to subframes. BUG=609747, 641035 Committed: https://crrev.com/73880b6945b9cc635396db030426a21c825669f5 Cr-Commit-Position: refs/heads/master@{#415179}

Patch Set 1 #

Patch Set 2 : cl format #

Patch Set 3 : fix unittests #

Total comments: 3

Patch Set 4 : rebased #

Patch Set 5 : . #

Patch Set 6 : .. #

Total comments: 4

Patch Set 7 : special case test #

Total comments: 9

Patch Set 8 : embedded tests server #

Patch Set 9 : fix test after rebase #

Patch Set 10 : . #

Patch Set 11 : cleanup #

Unified diffs Side-by-side diffs Delta from patch set Stats (+167 lines, -51 lines) Patch
M chrome/browser/subresource_filter/subresource_filter_browsertest.cc View 1 2 3 4 5 6 7 8 6 chunks +14 lines, -12 lines 0 comments Download
M components/subresource_filter/content/browser/content_subresource_filter_driver_factory.h View 1 2 3 4 5 6 7 1 chunk +8 lines, -0 lines 0 comments Download
M components/subresource_filter/content/browser/content_subresource_filter_driver_factory.cc View 1 2 3 4 5 6 7 3 chunks +30 lines, -14 lines 0 comments Download
M components/subresource_filter/content/browser/content_subresource_filter_driver_factory_unittest.cc View 1 2 3 4 5 6 7 8 9 10 6 chunks +115 lines, -25 lines 0 comments Download

Messages

Total messages: 64 (46 generated)
melandory
PTAL, thanks!
4 years, 4 months ago (2016-08-15 21:27:51 UTC) #14
engedy
Please see some initial comments. https://codereview.chromium.org/2243863002/diff/40001/components/subresource_filter/content/browser/content_subresource_filter_driver_factory.cc File components/subresource_filter/content/browser/content_subresource_filter_driver_factory.cc (right): https://codereview.chromium.org/2243863002/diff/40001/components/subresource_filter/content/browser/content_subresource_filter_driver_factory.cc#newcode159 components/subresource_filter/content/browser/content_subresource_filter_driver_factory.cc:159: driver->ActivateForProvisionalLoad(GetMaximumActivationState()); Do we need ...
4 years, 4 months ago (2016-08-15 23:13:02 UTC) #15
melandory
I should be able to finish this before branch point, currently have some problems with ...
4 years, 4 months ago (2016-08-24 07:31:16 UTC) #16
engedy
Sounds good, let me know if you want to discuss any issues!
4 years, 4 months ago (2016-08-24 08:01:05 UTC) #17
melandory
On 2016/08/24 08:01:05, engedy wrote: > Sounds good, let me know if you want to ...
4 years, 3 months ago (2016-08-25 06:32:07 UTC) #27
engedy
https://codereview.chromium.org/2243863002/diff/100001/components/subresource_filter/content/browser/content_subresource_filter_driver_factory.cc File components/subresource_filter/content/browser/content_subresource_filter_driver_factory.cc (right): https://codereview.chromium.org/2243863002/diff/100001/components/subresource_filter/content/browser/content_subresource_filter_driver_factory.cc#newcode125 components/subresource_filter/content/browser/content_subresource_filter_driver_factory.cc:125: void ContentSubresourceFilterDriverFactory::ActivateForFrameHostIfNeeded( To make it easier to argue about ...
4 years, 3 months ago (2016-08-25 12:21:29 UTC) #30
melandory
On 2016/08/25 12:21:29, engedy wrote: > https://codereview.chromium.org/2243863002/diff/100001/components/subresource_filter/content/browser/content_subresource_filter_driver_factory.cc > File > components/subresource_filter/content/browser/content_subresource_filter_driver_factory.cc > (right): > > ...
4 years, 3 months ago (2016-08-25 17:32:46 UTC) #32
melandory
https://codereview.chromium.org/2243863002/diff/100001/components/subresource_filter/content/browser/content_subresource_filter_driver_factory.cc File components/subresource_filter/content/browser/content_subresource_filter_driver_factory.cc (right): https://codereview.chromium.org/2243863002/diff/100001/components/subresource_filter/content/browser/content_subresource_filter_driver_factory.cc#newcode186 components/subresource_filter/content/browser/content_subresource_filter_driver_factory.cc:186: ActivateForFrameHostIfNeeded(render_frame_host, validated_url); On 2016/08/25 12:21:29, engedy wrote: > Let's ...
4 years, 3 months ago (2016-08-25 19:41:30 UTC) #37
engedy
LGTM % comments. https://codereview.chromium.org/2243863002/diff/120001/components/subresource_filter/content/browser/content_subresource_filter_driver_factory.cc File components/subresource_filter/content/browser/content_subresource_filter_driver_factory.cc (right): https://codereview.chromium.org/2243863002/diff/120001/components/subresource_filter/content/browser/content_subresource_filter_driver_factory.cc#newcode121 components/subresource_filter/content/browser/content_subresource_filter_driver_factory.cc:121: if (GetCurrentActivationScope() == ActivationScope::ALL_SITES) nit: GetCurrentActivationScope() ...
4 years, 3 months ago (2016-08-25 20:54:50 UTC) #39
melandory
Still not sure what to do with failing tests. So I probably can try to ...
4 years, 3 months ago (2016-08-27 07:27:03 UTC) #40
melandory
On 2016/08/27 07:27:03, melandory wrote: > Still not sure what to do with failing tests. ...
4 years, 3 months ago (2016-08-27 07:39:19 UTC) #41
melandory
On 2016/08/27 07:27:03, melandory wrote: > Still not sure what to do with failing tests. ...
4 years, 3 months ago (2016-08-27 07:39:22 UTC) #42
engedy
Looks like we could quite easily use EmbeddedTestServer in the browsertest. An instance is already ...
4 years, 3 months ago (2016-08-27 19:30:06 UTC) #43
engedy
And for the unit test, something like this should work: content::WebContentsTester::For(web_contents())->StartNavigation(url); factory()->ReadyToCommitMainFrameNavigation(main_rfh(), url); content::RenderFrameHostTester::For(main_rfh())->SimulateNavigationCommit(url); content::RenderFrameHostTester::For(main_rfh())->SimulateNavigationCommit(url);
4 years, 3 months ago (2016-08-28 16:58:36 UTC) #44
melandory
On 2016/08/28 16:58:36, engedy wrote: > And for the unit test, something like this should ...
4 years, 3 months ago (2016-08-29 18:18:01 UTC) #51
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2243863002/200001
4 years, 3 months ago (2016-08-30 03:45:35 UTC) #60
commit-bot: I haz the power
Committed patchset #11 (id:200001)
4 years, 3 months ago (2016-08-30 05:48:30 UTC) #62
commit-bot: I haz the power
4 years, 3 months ago (2016-08-30 05:49:56 UTC) #64
Message was sent while issue was closed.
Patchset 11 (id:??) landed as
https://crrev.com/73880b6945b9cc635396db030426a21c825669f5
Cr-Commit-Position: refs/heads/master@{#415179}

Powered by Google App Engine
This is Rietveld 408576698