[webkit-reviews] review denied: [Bug 77383] Add a different didFirstVisuallNonEmptyLayout heuristic to experiment with : [Attachment 124865] New patch that implements didNewFirstVisuallyNonEmptyLayout

[bugzilla-daemon at webkit.org]

Sam Weinig <sam at webkit.org> has denied Beth Dakin <bdakin at apple.com>'s request
for review:
Bug 77383: Add a different didFirstVisuallNonEmptyLayout heuristic to
experiment with
https://bugs.webkit.org/show_bug.cgi?id=77383

Attachment 124865: New patch that implements didNewFirstVisuallyNonEmptyLayout
https://bugs.webkit.org/attachment.cgi?id=124865&action=review

------- Additional Comments from Sam Weinig <sam at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=124865&action=review


This obviously needs to have tests.

> Source/WebCore/ChangeLog:11
> +	   This patch adds a new heuristic called
didNewFirstVisuallNonEmptyLayout and 

Typo: Visuall -> Visually

> Source/WebCore/ChangeLog:27
> +	   Start counting relevant painted RenderObjects on the page once the
first 
> +	   layout is complete.

Why do we only start counting once the first layout is complete?

> Source/WebCore/page/Page.cpp:1102
> +    if (m_relevantPaintedRenderObjects.size() ==
(int)gPaintedObjectCounterThreshold) {

This should use a static_cast.

> Source/WebCore/page/Page.h:479
> +	   HashSet<RenderObject*> m_relevantPaintedRenderObjects;
> +	   bool m_isCountingRepaintedObjects;

Since the other layout heuristic stuff is on FrameView, FrameView seems like a
better place for this than Page.

> Source/WebCore/rendering/InlineBox.cpp:224
> +    if (RenderView* view = renderer()->view()) {
> +	   if (paintInfo.rect.intersects(view->viewRect())) {
> +	       if (Frame* frame = renderer()->frame()) {
> +		   if (Page* page = frame->page())
> +		       page->addRelevantRepaintedObject(renderer());
> +	       }
> +	   }
> +    }

This should have a comment explaining what it is doing.  I though you only
wanted to do this when until the threshold is hit, but this looks like it is
always happening. Even if you are early returning in the Page call, this set of
operations seems unnecessary to do a lot of the time.  Since this logic is
repeated so much, it should probably be refactored to be in one place.

> Source/WebCore/rendering/InlineTextBox.cpp:507
> +    if (RenderView* view = renderer()->view()) {
> +	   if (paintInfo.rect.intersects(view->viewRect())) {
> +	       if (Frame* frame = renderer()->frame()) {
> +		   if (Page* page = frame->page())
> +		       page->addRelevantRepaintedObject(renderer());
> +	       }
> +	   }
> +    }

Same comment as above.

> Source/WebCore/rendering/RenderEmbeddedObject.cpp:178
> +    if (RenderView* view = this->view()) {
> +	   if (paintInfo.rect.intersects(view->viewRect())) {
> +	       if (Frame* frame = this->frame()) {
> +		   if (Page* page = frame->page())
> +		       page->addRelevantRepaintedObject(this);
> +	       }
> +	   }
> +    }

Same comment...

> Source/WebKit2/UIProcess/API/C/WKPage.h:64
>  typedef void
(*WKPageDidFirstVisuallyNonEmptyLayoutForFrameCallback)(WKPageRef page,
WKFrameRef frame, WKTypeRef userData, const void *clientInfo);
> +typedef void (*WKPageDidNewFirstVisuallyNonEmptyLayoutCallback)(WKPageRef
page, WKTypeRef userData, const void *clientInfo);

It seems oddly inconsistent not have this take a frame.

> Source/WebKit2/UIProcess/API/C/WKPage.h:89
> +    WKPageDidNewFirstVisuallyNonEmptyLayoutCallback			  
didNewFirstVisuallyNonEmptyLayout;

This can't go in the middle, as that will break nightlies.  You need to put it
at the end and make sure the version number is bumped from the last shipping
release.