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

Issue 1295053002: Move dispatchDidFirstVisuallyNonEmptyLayout into WebViewImpl. (Closed)

Created:
5 years, 4 months ago by esprehn
Modified:
5 years, 4 months ago
Reviewers:
dglazkov, ojan
CC:
blink-reviews
Target Ref:
refs/heads/master
Project:
blink
Visibility:
Public.

Description

Reland: Move dispatchDidFirstVisuallyNonEmptyLayout into WebViewImpl. This removes a user of didLayoutWithPendingStylesheets() and instead moves the call to dispatchDidFirstVisuallyNonEmptyLayout into WebViewImpl so the notification to the embedder is tied to the pumping of frames. This changes the behavior of this notification to only dispatch for the top level frame, but Chromium already has a check for that: https://chromium.googlesource.com/chromium/src/+/976d4d9a2735bdc11e5a641c84b6382566d48f1d/content/renderer/render_frame_impl.cc#3472 RenderFrameImpl::didFirstVisuallyNonEmptyLayout's first check is if (frame->parent()) return; so we already ignore all frames that are not the main frame. Future patches will clean up this interface so it's obvious this really only happens for the main frame. This was originally comitted as r200664, but got reverted due to making a test flaky in Chromium that was depending on very specific timing of this callback. That test was fixed by: https://crrev.com/0ea17fce0d5218c909985e2216051eab79419463 BUG=521692 R=dglazkov@chromium.org Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=200934

Patch Set 1 #

Patch Set 2 : simpler. #

Unified diffs Side-by-side diffs Delta from patch set Stats (+19 lines, -15 lines) Patch
M Source/core/frame/FrameView.h View 2 chunks +1 line, -1 line 0 comments Download
M Source/core/frame/FrameView.cpp View 3 chunks +5 lines, -14 lines 0 comments Download
M Source/web/WebViewImpl.h View 1 1 chunk +1 line, -0 lines 0 comments Download
M Source/web/WebViewImpl.cpp View 1 4 chunks +12 lines, -0 lines 0 comments Download

Messages

Total messages: 19 (7 generated)
commit-bot: I haz the power
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1295053002/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1295053002/20001
5 years, 4 months ago (2015-08-16 21:37:37 UTC) #2
commit-bot: I haz the power
Dry run: Try jobs failed on following builders: linux_chromium_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_rel_ng/builds/98820)
5 years, 4 months ago (2015-08-16 22:32:40 UTC) #4
esprehn
5 years, 4 months ago (2015-08-17 06:35:08 UTC) #6
dglazkov
On 2015/08/17 at 06:35:08, esprehn wrote: > LGTM. Would love to have a meta bug ...
5 years, 4 months ago (2015-08-17 15:04:31 UTC) #7
esprehn
On 2015/08/17 at 15:04:31, dglazkov wrote: > On 2015/08/17 at 06:35:08, esprehn wrote: > > ...
5 years, 4 months ago (2015-08-17 17:59:41 UTC) #9
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1295053002/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1295053002/20001
5 years, 4 months ago (2015-08-17 17:59:51 UTC) #10
esprehn
Committed patchset #2 (id:20001) manually as 200664 (presubmit successful).
5 years, 4 months ago (2015-08-17 20:14:11 UTC) #11
esprehn
A revert of this CL (patchset #2 id:20001) has been created in https://codereview.chromium.org/1294683004/ by esprehn@chromium.org. ...
5 years, 4 months ago (2015-08-17 21:22:26 UTC) #12
commit-bot: I haz the power
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1295053002/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1295053002/20001
5 years, 4 months ago (2015-08-20 18:28:01 UTC) #14
commit-bot: I haz the power
Dry run: This issue passed the CQ dry run.
5 years, 4 months ago (2015-08-20 20:32:20 UTC) #16
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1295053002/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1295053002/20001
5 years, 4 months ago (2015-08-20 21:01:08 UTC) #18
commit-bot: I haz the power
5 years, 4 months ago (2015-08-20 21:09:11 UTC) #19
Message was sent while issue was closed.
Committed patchset #2 (id:20001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=200934

Powered by Google App Engine
This is Rietveld 408576698