Bug 130101 – Ensure that layout milestones complete in all cases

• Status: RESOLVED
• Resolution: FIXED
• Component: Page Loading

[Antti Koivisto]

In some testing scenarios milestones fail to complete.

[Antti Koivisto]

Created attachment 226451 [details]
patch

[Pratik Solanki]

<rdar://problem/14651144>

[Darin Adler]

Comment on attachment 226451 [details]
patch

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.

[Antti Koivisto]

https://trac.webkit.org/r165534

[Antti Koivisto]

> 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.

Didn't do this. That would essentially be another finishedParsing function which we already have in multiple places. Some sort of bigger cleanup is needed.

> I’d put the variable inside the if statement.

It is used below too.

[Simon Fraser (smfr)]

Comment on attachment 226451 [details]
patch

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

Should we make a way to layout-test milestones?

> Source/WebCore/page/FrameView.cpp:2669
> +    if (auto* page = frame().page()) {
> +        if (auto* scrollingCoordinator = page->scrollingCoordinator())

Why auto here...

>> Source/WebCore/page/FrameView.cpp:4129
>> +    Page* page = frame().page();
>> +    if (page)
> 
> I’d put the variable inside the if statement.

but no auto here? I'd prefer less auto, not more.

[Antti Koivisto]

https://trac.webkit.org/r165534
https://trac.webkit.org/r165543 (iOS build fix)