This bug tracks adding WebKit2 api to count relevant RenderObjects. Patch forthcoming. <rdar://problem/10709560>
Created attachment 124631 [details] Patch
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.
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.
Created attachment 124785 [details] Another try for the bots
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.
Created attachment 124865 [details] New patch that implements didNewFirstVisuallyNonEmptyLayout
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.
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
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.
(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.
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.
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.
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.
I addressed Sam's remaining comments and checked in the patch: http://trac.webkit.org/changeset/106492
This patch landed.