[webkit-reviews] review denied: [Bug 48077] HTMLParserScheduler should be suspended when page loading is deferred : [Attachment 71896] Updated

[bugzilla-daemon at webkit.org]

Adam Barth <abarth at webkit.org> has denied 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 71896: Updated
https://bugs.webkit.org/attachment.cgi?id=71896&action=review

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

Something about this change doesn't seem very elegant.	I think the problem is
we're plumbing this even though the DocumentParser interface even though it's
only relevant for an HTMLDocumentParser.  Maybe Eric has some thoughts.  R- for
nits below.

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

Please don't put implementations of virtual methods in the header.  That leads
to binary bloat.  What about all the other implementations of this interface? 
Do they not care about these events?

> WebCore/html/parser/HTMLParserScheduler.h:95
> +    void suspend()
> +    {
> +	   ASSERT(!m_isSuspendedWithActiveTimer);
> +	   if (!m_continueNextChunkTimer.isActive())
> +	       return;
> +	   m_isSuspendedWithActiveTimer = true;
> +	   m_continueNextChunkTimer.stop();
> +    }
> +
> +    void resume()
> +    {
> +	   ASSERT(!m_continueNextChunkTimer.isActive());
> +	   if (!m_isSuspendedWithActiveTimer)
> +	       return;
> +	   m_isSuspendedWithActiveTimer = false;
> +	   m_continueNextChunkTimer.startOneShot(0);
> +    }

These should go in the cpp file.

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

These operations feel grouped together.  Is there some higher-level concept
they're expressing?  Maybe the document should know how to do the details of
that operation.