Summary: | Ensure that layout milestones complete in all cases | ||||||
---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Antti Koivisto <koivisto> | ||||
Component: | Page Loading | Assignee: | Nobody <webkit-unassigned> | ||||
Status: | RESOLVED FIXED | ||||||
Severity: | Normal | CC: | esprehn+autocc, glenn, kangil.han, kondapallykalyan, psolanki, simon.fraser, webkit-ews | ||||
Priority: | P2 | Keywords: | InRadar | ||||
Version: | 528+ (Nightly build) | ||||||
Hardware: | Unspecified | ||||||
OS: | Unspecified | ||||||
Attachments: |
|
Description
Antti Koivisto
2014-03-11 16:34:24 PDT
Created attachment 226451 [details]
patch
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. > 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. 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. https://trac.webkit.org/r165534 https://trac.webkit.org/r165543 (iOS build fix) |