Bug 108725

Summary: [Chromium] Add a signal for when the body is inserted in the document
Product: WebKit Reporter: James Simonsen <simonjam>
Component: New BugsAssignee: James Simonsen <simonjam>
Status: RESOLVED FIXED    
Severity: Normal CC: abarth, dglazkov, fishd, jamesr, japhet, ojan.autocc, tkent+wkapi, webkit.review.bot, willchan
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch
none
Patch none

James Simonsen
Reported 2013-02-01 17:01:44 PST
[Chromium] Add a signal for when the body is inserted in the document
Attachments
Patch (5.14 KB, patch)
2013-02-01 18:21 PST, James Simonsen 		no flags
DetailsFormatted DiffDiff
Patch (5.24 KB, patch)
2013-02-04 15:17 PST, James Simonsen 		no flags
DetailsFormatted DiffDiff
Patch (7.28 KB, patch)
2013-02-04 18:14 PST, James Simonsen 		no flags
DetailsFormatted DiffDiff
Show Obsolete (2) View All Add attachment proposed patch, testcase, etc.
James Simonsen
Comment 1 2013-02-01 18:21:04 PST
Created attachment 186198 [details] Patch
WebKit Review Bot
Comment 2 2013-02-01 18:22:52 PST
Please wait for approval from abarth@webkit.org, dglazkov@chromium.org, fishd@chromium.org, jamesr@chromium.org or tkent@chromium.org before submitting, as this patch contains changes to the Chromium public API. See also https://trac.webkit.org/wiki/ChromiumWebKitAPI.
James Robinson
Comment 3 2013-02-01 18:25:33 PST
What does inserting the body have to do with knowing we have enough to paint? That just means we hit either the <body> or any non-<head> tag, but it doesn't mean we have reasonable content. Is this emperically determined to help?
James Simonsen
Comment 5 2013-02-01 18:31:27 PST
Err, the comment is a few lines up.
James Robinson
Comment 6 2013-02-01 18:34:06 PST
That heuristic is a little different since it's checking for the body element's renderer. That won't become non-null until we do the first style resolution (which is hopefully when doing the first layout). That's also a horrible layering violation for the loader to be checking for a render object :/
James Simonsen
Comment 7 2013-02-01 18:50:50 PST
Yeah, I know it's not exact. Basically, we want to know that parser has made it through all of the parse blocking resources in the head before we start loading lower priority resources like images. We don't actually want to wait for the layout to finish before starting in.
James Robinson
Comment 8 2013-02-01 18:54:08 PST
Comment on attachment 186198 [details] Patch OK. I know the page cyclers are extremely sensitive to the timing of when that first style resolution+layout ends up happening, but maybe that's not as true for this. LGTM for the WebKit API parts. I don't know enough about the /html/ and /loader/ interactions to set the review? flag. Maybe japhet could do the honors?
Adam Barth
Comment 9 2013-02-02 00:24:21 PST
Comment on attachment 186198 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=186198&action=review > Source/WebCore/html/HTMLBodyElement.cpp:190 > + if (Frame* frame = document()->frame()) > + frame->loader()->client()->dispatchDidInsertBody(); This is wrong. It will fire every time a body element is inserted anywhere, regardless of whether it's document.body. Instead what you want is a notification at the end of HTMLConstructionSite::insertHTMLBodyElement
Adam Barth
Comment 10 2013-02-02 00:24:54 PST
(More specifically, when the task queued by that function executes.)
James Simonsen
Comment 11 2013-02-04 15:17:20 PST
Created attachment 186474 [details] Patch
James Simonsen
Comment 12 2013-02-04 15:19:00 PST
Thanks for the tip. That's a better location for it. I actually don't mind dispatching this message before it's actually inserted in the document. If the parser has gotten this far, it means nothing external will block the body from being inserted, which is what I care about. I've updated the function name to match (did -> will).
Adam Barth
Comment 13 2013-02-04 16:06:35 PST
Comment on attachment 186474 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=186474&action=review > Source/WebCore/loader/FrameLoaderClient.h:375 > + virtual void dispatchWillInsertBody() { } I might add a comment about this only working for HTML document (e.g., not for SVG since they don't have a <body> element). > Source/WebKit/chromium/public/WebFrameClient.h:331 > + // The <body> tag is now present. I'd also mentioning that this makes sense only for HTML document here too.
Adam Barth
Comment 14 2013-02-04 16:07:15 PST
This patch seems fine to me, but it would be good to have a unit test showing that we call the callback.
James Simonsen
Comment 15 2013-02-04 18:14:51 PST
Created attachment 186517 [details] Patch
James Simonsen
Comment 16 2013-02-04 18:15:09 PST
Comments updated and test added.
Adam Barth
Comment 17 2013-02-04 18:42:36 PST
Comment on attachment 186517 [details] Patch Thanks!
WebKit Review Bot
Comment 18 2013-02-04 20:24:52 PST
Comment on attachment 186517 [details] Patch Clearing flags on attachment: 186517 Committed r141850: <http://trac.webkit.org/changeset/141850>
WebKit Review Bot
Comment 19 2013-02-04 20:24:57 PST
All reviewed patches have been landed. Closing bug.
Note You need to log in before you can comment on or make changes to this bug.