Bug 89258 – [Chromium] Ensure layout has happened before calling into hasNonEmptyBoundingBox()

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Ilya Sherman]

[Chromium] Ensure layout has happened before calling into hasNonEmptyBoundingBox()

[Ilya Sherman]

Created attachment 147929 [details]
Patch

[James Robinson]

Comment on attachment 147929 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=147929&action=review

> Source/WebKit/chromium/src/WebNode.cpp:218
> +    m_private->document()->updateLayout();

most code in WebCore of this nature calls updateLayoutIgnorePendingStylesheets() - it deals with a m_hasNodesWithPlaceholderStyle case that seems important

[Ilya Sherman]

Created attachment 147939 [details]
Patch

[Ilya Sherman]

(In reply to comment #2)
> (From update of attachment 147929 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=147929&action=review
> 
> > Source/WebKit/chromium/src/WebNode.cpp:218
> > +    m_private->document()->updateLayout();
> 
> most code in WebCore of this nature calls updateLayoutIgnorePendingStylesheets() - it deals with a m_hasNodesWithPlaceholderStyle case that seems important

Ok, switched to using that method.

[WebKit Review Bot]

Comment on attachment 147939 [details]
Patch

Clearing flags on attachment: 147939

Committed r120637: <http://trac.webkit.org/changeset/120637>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Eric Seidel (no email)]

What are the side-effects of this change?  Also I'm surprised by the lack of testing?  is testing impossible/impractical?

[Ilya Sherman]

(In reply to comment #7)
> What are the side-effects of this change?

Side-effects include correctness improvements for callers of the two affected methods, and presumably some minimal performance degradations to compensate.  (The impact on performance appears not to be causing any issues for existing clients.)

> Also I'm surprised by the lack of testing?  is testing impossible/impractical?

AFAIK, the Chromium WebKit API is generally not tested within the WebKit project.  However, the correctness issue was caught by a Chromium test, so there is some test coverage.