Bug 77383 – Add a different didFirstVisuallNonEmptyLayout heuristic to experiment with

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit API

[Beth Dakin]

This bug tracks adding WebKit2 api to count relevant RenderObjects. Patch forthcoming.

<rdar://problem/10709560>

[Beth Dakin]

Created attachment 124631 [details]
Patch

[Beth Dakin]

Created attachment 124779 [details]
Patch again

Don't know why the bots didn't like that last patch. Here's a new one on an updated tree.

[WebKit Review Bot]

Attachment 124779 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/update-webkit']" exit_code: 9

Updating OpenSource
From git://git.webkit.org/WebKit
   516488a..35bf539  master     -> origin/master
Partial-rebuilding .git/svn/refs/remotes/origin/master/.rev_map.268f45cc-cd09-0410-ab3c-d52691b4dbfc ...
Currently at 106369 = 516488acc916a40a28a51c73fcf5ad5640b39f3d
r106370 = a323c82f735dd9f8795f6bcd8849a4085284c855
r106371 = f2b568786583ff10720556051ed600931eb5c982
r106372 = 35bf53917ffea9b99274893b132819d2a8add71f
Done rebuilding .git/svn/refs/remotes/origin/master/.rev_map.268f45cc-cd09-0410-ab3c-d52691b4dbfc
First, rewinding head to replay your work on top of it...
Applying: Fix compilation errors on build-webkit --debug --no-workers on mac.
Using index info to reconstruct a base tree...
Falling back to patching base and 3-way merge...
Auto-merging LayoutTests/ChangeLog
CONFLICT (content): Merge conflict in LayoutTests/ChangeLog
Auto-merging LayoutTests/platform/qt/Skipped
CONFLICT (content): Merge conflict in LayoutTests/platform/qt/Skipped
Auto-merging Source/WebCore/ChangeLog
CONFLICT (content): Merge conflict in Source/WebCore/ChangeLog
Failed to merge in the changes.
Patch failed at 0001 Fix compilation errors on build-webkit --debug --no-workers on mac.

When you have resolved this problem run "git rebase --continue".
If you would prefer to skip this patch, instead run "git rebase --skip".
To restore the original branch and stop rebasing run "git rebase --abort".

rebase refs/remotes/origin/master: command returned error: 1

Died at Tools/Scripts/update-webkit line 164.


If any of these errors are false positives, please file a bug against check-webkit-style.

[Beth Dakin]

Created attachment 124785 [details]
Another try for the bots

[WebKit Review Bot]

Attachment 124785 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/update-webkit']" exit_code: 9

Updating OpenSource
First, rewinding head to replay your work on top of it...
Applying: Fix compilation errors on build-webkit --debug --no-workers on mac.
Using index info to reconstruct a base tree...
Falling back to patching base and 3-way merge...
Auto-merging LayoutTests/ChangeLog
CONFLICT (content): Merge conflict in LayoutTests/ChangeLog
Auto-merging LayoutTests/platform/qt/Skipped
CONFLICT (content): Merge conflict in LayoutTests/platform/qt/Skipped
Auto-merging Source/WebCore/ChangeLog
CONFLICT (content): Merge conflict in Source/WebCore/ChangeLog
Auto-merging Source/WebCore/WebCore.exp.in
Auto-merging Source/WebKit/mac/ChangeLog
CONFLICT (content): Merge conflict in Source/WebKit/mac/ChangeLog
Failed to merge in the changes.
Patch failed at 0001 Fix compilation errors on build-webkit --debug --no-workers on mac.

When you have resolved this problem run "git rebase --continue".
If you would prefer to skip this patch, instead run "git rebase --skip".
To restore the original branch and stop rebasing run "git rebase --abort".

rebase refs/remotes/origin/master: command returned error: 1

Died at Tools/Scripts/update-webkit line 164.


If any of these errors are false positives, please file a bug against check-webkit-style.

[Beth Dakin]

Created attachment 124865 [details]
New patch that implements didNewFirstVisuallyNonEmptyLayout

[WebKit Review Bot]

Attachment 124865 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1

Source/WebCore/page/Page.h:357:  The parameter name "threshold" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 1 in 34 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Philippe Normand]

Comment on attachment 124865 [details]
New patch that implements didNewFirstVisuallyNonEmptyLayout

Attachment 124865 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/11387265

[Sam Weinig]

Comment on attachment 124865 [details]
New patch that implements didNewFirstVisuallyNonEmptyLayout

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.

[Beth Dakin]

(In reply to comment #9)
> (From update of attachment 124865 [details])
> 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
> 

Will fix.

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

This can obviously be changed to be somewhere else. I was assuming that nothing meaningful would be painted until after the first layout, but if you disagree, then I am more than happy to find a new place.

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

Will change.

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

I disagree. I don't think this heuristic makes sense per-frame. I think it makes sense only for the page as a whole, and since we don't have a class that represents the main frame or main FrameView, I think Page makes the most sense.

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

I considered adding this logic to the addRelevantRepaintedObject(), and I probably should have. I will change that.

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

There are existing members of the client that do not take frames, and as I explained above, I really do not think that this heuristic makes sense per-frame. It's a Page-level concept.

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

Will fix.

[Beth Dakin]

Created attachment 124973 [details]
New patch that addresses most of Sam's comments

This patch addresses most of Sam's comments. I will work on tests this afternoon, and I kept the stuff on Page rather than moving it to FrameView since I think it's a Page-level concept.

[WebKit Review Bot]

Attachment 124973 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1

Source/WebCore/page/Page.h:357:  The parameter name "threshold" adds no information, so it should be removed.  [readability/parameter_name] [5]
Tools/WebKitTestRunner/TestController.cpp:392:  One space before end of line comments  [whitespace/comments] [5]
Tools/WebKitTestRunner/TestController.cpp:393:  One space before end of line comments  [whitespace/comments] [5]
Total errors found: 3 in 34 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Sam Weinig]

Comment on attachment 124973 [details]
New patch that addresses most of Sam's comments

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

This still needs tests, and I would like a follow up that rationalizes that we don't want to track layouts (first, firstVisuallyNonEmpty, firstNewFirstVisuallyNonEmpty) at the frame level anymore, and therefore should not have a frame in their API.

> Source/WebCore/page/Page.h:358
> +        void setPaintedObjectsCounterThreshold(uint64_t threshold);
> +        void startCountingRepaintedObjects();

Should these mention the word "relevant"?

> Source/WebCore/page/Page.h:479
> +        bool m_isCountingRepaintedObjects;

Should this mention the word "relevant"?

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

Please put a comment here about merging this with FirstVisuallyNonEmptyLayout.

[Beth Dakin]

I addressed Sam's remaining comments and checked in the patch: http://trac.webkit.org/changeset/106492

[Adam Barth]

This patch landed.