[webkit-reviews] review granted: [Bug 130101] Ensure that layout milestones complete in all cases : [Attachment 226451] patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Antti Koivisto <koivisto at iki.fi>'s
request for review:
Bug 130101: Ensure that layout milestones complete in all cases
https://bugs.webkit.org/show_bug.cgi?id=130101

Attachment 226451: patch
https://bugs.webkit.org/attachment.cgi?id=226451&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=226451&action=review


Some risk in changing the order of things here, but seems obviously a step int
he right direction.

> Source/WebCore/dom/Document.cpp:2490
> +    if (!m_bParsing && view() && !view()->needsLayout())
> +	   view()->fireLayoutRelatedMilestonesIfNeeded();

It’s hard to understand the relationships here. I think that future me would
understand this better if the function the document called on FrameView was
named something more like “document emptiness status may have changed”, and
that function would then turn around and do this:

    if (!needsLayout) fireLayoutRelatedMilestonesIfNeeded

within the FrameView class.

> Source/WebCore/page/FrameView.cpp:4129
> +    Page* page = frame().page();
> +    if (page)

I’d put the variable inside the if statement.