[Webkit-unassigned] [Bug 48077] HTMLParserScheduler should be suspended when page loading is deferred

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=48077


Adam Barth <abarth at webkit.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #71896|review?                     |review-
               Flag|                            |




--- Comment #14 from Adam Barth <abarth at webkit.org>  2010-10-31 18:26:00 PST ---
(From update of attachment 71896)
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.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.