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>
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   
Description Flags
Patch none

Description James Simonsen 2013-02-01 17:01:44 PST
[Chromium] Add a signal for when the body is inserted in the document
Comment 1 James Simonsen 2013-02-01 18:21:04 PST
Created attachment 186198 [details]
Comment 2 WebKit Review Bot 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.
Comment 3 James Robinson 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?
Comment 5 James Simonsen 2013-02-01 18:31:27 PST
Err, the comment is a few lines up.
Comment 6 James Robinson 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 :/
Comment 7 James Simonsen 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.
Comment 8 James Robinson 2013-02-01 18:54:08 PST
Comment on attachment 186198 [details]

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?
Comment 9 Adam Barth 2013-02-02 00:24:21 PST
Comment on attachment 186198 [details]

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
Comment 10 Adam Barth 2013-02-02 00:24:54 PST
(More specifically, when the task queued by that function executes.)
Comment 11 James Simonsen 2013-02-04 15:17:20 PST
Created attachment 186474 [details]
Comment 12 James Simonsen 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).
Comment 13 Adam Barth 2013-02-04 16:06:35 PST
Comment on attachment 186474 [details]

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.
Comment 14 Adam Barth 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.
Comment 15 James Simonsen 2013-02-04 18:14:51 PST
Created attachment 186517 [details]
Comment 16 James Simonsen 2013-02-04 18:15:09 PST
Comments updated and test added.
Comment 17 Adam Barth 2013-02-04 18:42:36 PST
Comment on attachment 186517 [details]

Comment 18 WebKit Review Bot 2013-02-04 20:24:52 PST
Comment on attachment 186517 [details]

Clearing flags on attachment: 186517

Committed r141850: <http://trac.webkit.org/changeset/141850>
Comment 19 WebKit Review Bot 2013-02-04 20:24:57 PST
All reviewed patches have been landed.  Closing bug.