[webkit-reviews] review granted: [Bug 48077] HTMLParserScheduler should be suspended when page loading is deferred : [Attachment 73422] Try again

[bugzilla-daemon at webkit.org]

Adam Barth <abarth at webkit.org> has granted Yong Li <yong.li.webkit at gmail.com>'s
request for review:
Bug 48077: HTMLParserScheduler should be suspended when page loading is
deferred
https://bugs.webkit.org/show_bug.cgi?id=48077

Attachment 73422: Try again
https://bugs.webkit.org/attachment.cgi?id=73422&action=review

------- Additional Comments from Adam Barth <abarth at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=73422&action=review

> WebCore/dom/DocumentParser.h:95
> +    virtual void willDeferLoading();
> +    virtual void didResumeLoading();

These really want to be named suspend and resume, respectively.  The only
problem is that calling suspend doesn't actually suspend the parser.  It just
stops the parser from running its scheduled callback.

I think the code content of this patch is fine, I'm just struggling to find the
right names for these functions.  I don't want to hold your patch hostage any
longer, but we might want to come back and think of better names here.	Maybe
add a FIXME comment?

> WebCore/page/PageGroupLoadDeferrer.cpp:54
>		      
frame->document()->suspendActiveDOMObjects(ActiveDOMObject::WillShowDialog);
>		       frame->document()->asyncScriptRunner()->suspend();
> +		       if (DocumentParser* parser =
frame->document()->parser())
> +			   parser->willDeferLoading();

I understand that you don't want to add a FIXME for grouping these operations,
but can you add a FIXME about moving this work to Document at some point?

> WebCore/page/PageGroupLoadDeferrer.cpp:76
>		   frame->document()->resumeActiveDOMObjects();
>		   frame->document()->asyncScriptRunner()->resume();
> +		   if (DocumentParser* parser = frame->document()->parser())
> +		       parser->didResumeLoading();

Same here.