Chromium Code Reviews
Help | Chromium Project | Gerrit Changes | Sign in
(17)

Issue 2155393002: Fix ScopedPageLoadDeferrer to work with pages that have remote main frames. (Closed)

Created:
3 years, 1 month ago by alexmos
Modified:
3 years, 1 month ago
Reviewers:
Mike West, dcheng
Base URL:
https://chromium.googlesource.com/chromium/src.git@master
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

Fix ScopedPageLoadDeferrer to work with pages that have remote main frames. BUG=628942 Committed: https://crrev.com/07ff366089e56cb17712457e3f5e8469f034631b Cr-Commit-Position: refs/heads/master@{#406632}

Patch Set 1 #

Patch Set 2 : Add a test #

Total comments: 2

Patch Set 3 : dcheng's nit #

Unified diffs Side-by-side diffs Delta from patch set Stats (+49 lines, -12 lines) Patch
M third_party/WebKit/Source/core/page/ScopedPageLoadDeferrer.h View 1 chunk +1 line, -1 line 0 comments Download
M third_party/WebKit/Source/core/page/ScopedPageLoadDeferrer.cpp View 2 chunks +7 lines, -11 lines 0 comments Download
M third_party/WebKit/Source/web/tests/WebFrameTest.cpp View 1 2 2 chunks +41 lines, -0 lines 0 comments Download

Messages

Total messages: 22 (12 generated)
dcheng
Code change LGTM I wonder if we should have a test for this checked in?
3 years, 1 month ago (2016-07-19 02:00:39 UTC) #6
Mike West
On 2016/07/19 at 02:00:39, dcheng wrote: > Code change LGTM > > I wonder if ...
3 years, 1 month ago (2016-07-19 08:56:42 UTC) #7
alexmos
Thanks, I added a test - can you please take a look at it? Note ...
3 years, 1 month ago (2016-07-19 23:11:21 UTC) #9
dcheng
lgtm with a nit https://codereview.chromium.org/2155393002/diff/20001/third_party/WebKit/Source/web/tests/WebFrameTest.cpp File third_party/WebKit/Source/web/tests/WebFrameTest.cpp (right): https://codereview.chromium.org/2155393002/diff/20001/third_party/WebKit/Source/web/tests/WebFrameTest.cpp#newcode8191 third_party/WebKit/Source/web/tests/WebFrameTest.cpp:8191: EXPECT_FALSE(remoteRoot->toImplBase()->frame()->page()->defersLoading()); Nit: page->defersLoading()?
3 years, 1 month ago (2016-07-20 02:08:57 UTC) #10
Mike West
LGTM. Thanks!
3 years, 1 month ago (2016-07-20 07:26:12 UTC) #11
alexmos
Thanks! https://codereview.chromium.org/2155393002/diff/20001/third_party/WebKit/Source/web/tests/WebFrameTest.cpp File third_party/WebKit/Source/web/tests/WebFrameTest.cpp (right): https://codereview.chromium.org/2155393002/diff/20001/third_party/WebKit/Source/web/tests/WebFrameTest.cpp#newcode8191 third_party/WebKit/Source/web/tests/WebFrameTest.cpp:8191: EXPECT_FALSE(remoteRoot->toImplBase()->frame()->page()->defersLoading()); On 2016/07/20 02:08:57, dcheng wrote: > Nit: ...
3 years, 1 month ago (2016-07-20 16:08:32 UTC) #12
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/2155393002/40001
3 years, 1 month ago (2016-07-20 16:25:53 UTC) #16
commit-bot: I haz the power
Failed to apply the patch.
3 years, 1 month ago (2016-07-20 19:04:56 UTC) #18
commit-bot: I haz the power
CQ bit was unchecked.
3 years, 1 month ago (2016-07-20 19:05:09 UTC) #20
commit-bot: I haz the power
3 years, 1 month ago (2016-07-20 19:05:55 UTC) #22
Message was sent while issue was closed.
Patchset 3 (id:??) landed as
https://crrev.com/07ff366089e56cb17712457e3f5e8469f034631b
Cr-Commit-Position: refs/heads/master@{#406632}

Powered by Google App Engine
This is Rietveld 408576698