Bug 95397 – Need to merge didFirstVisuallyNonEmptyLayout and didNewFirstVisuallyNonEmptyLayout

• Component: WebKit API
• Status: RESOLVED FIXED

[Beth Dakin]

The name didNewFirstVisuallyNonEmptyLayout is an abomination. We should merge didFirstLayout, didFirstVisuallyNonEmptyLayout, and didNewFirstVisuallyNonEmptyLayout. They will all be part of the same callback api, but there will be an enum representing the three stages of these states. The callback will fire three times -- once for each state. 

<rdar://problem/10791680>

[Beth Dakin]

Created attachment 161368 [details]
Patch

The API that I added has a sort of silly name: didUnlockLayoutAchievement. I do have some less-silly ideas for alternatives, such as:

didReachLayoutMilestone
didCompleteLayoutPhase
didAchieveLayoutState

I thought we could have a discussion now during review about the best name.

I plan to remove the old API in a follow-up patch.

[Adam Barth]

didUnlockLayoutAchievement is cute.  Perhaps didChangeLayoutState to be analogous with document.readyState ?

[Early Warning System Bot]

Comment on attachment 161368 [details]
Patch

Attachment 161368 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/13680484

[Beth Dakin]

Created attachment 161375 [details]
Patch, trying to fix QT build

[Beth Dakin]

(In reply to comment #2)
> didUnlockLayoutAchievement is cute.  Perhaps didChangeLayoutState to be analogous with document.readyState ?

That's a good option too!

[Gyuyoung Kim]

Comment on attachment 161375 [details]
Patch, trying to fix QT build

Attachment 161375 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/13686470

[Beth Dakin]

Created attachment 161527 [details]
Patch, with more build fixing

[Gyuyoung Kim]

Comment on attachment 161527 [details]
Patch, with more build fixing

Attachment 161527 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/13682834

[Beth Dakin]

Created attachment 161565 [details]
Patch for EFL

[Beth Dakin]

Created attachment 162159 [details]
Patch with different design

I talked more with Dan and Sam, and we decided to go with an API with a slightly different design. In this design, didFirstLayout, didFirstVisuallyNonEmptyLayout, and didNewFirstVisuallyNonEmptyLayout are even more merged than before, and clients need to opt into which layout milestones they are interested in hearing about.

[WebKit Review Bot]

Attachment 162159 [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/WebKit2/WebProcess/WebPage/WebPage.cpp:411:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
Source/WebKit/mac/WebView/WebViewInternal.h:69:  The parameter name "options" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebKit/mac/WebView/WebViewInternal.h:70:  The parameter name "options" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebKit2/UIProcess/WebPageProxy.cpp:296:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
Source/WebKit2/WebProcess/InjectedBundle/InjectedBundlePageLoaderClient.h:32:  Alphabetical sorting problem.  [build/include_order] [4]
Source/WebCore/loader/FrameLoaderClient.h:33:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 6 in 87 files


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

[WebKit Review Bot]

Comment on attachment 162159 [details]
Patch with different design

Attachment 162159 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/13757461

[Early Warning System Bot]

Comment on attachment 162159 [details]
Patch with different design

Attachment 162159 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/13761396

[Early Warning System Bot]

Comment on attachment 162159 [details]
Patch with different design

Attachment 162159 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/13753658

[Build Bot]

Comment on attachment 162159 [details]
Patch with different design

Attachment 162159 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/13761398

[Peter Beverloo (cr-android ews)]

Comment on attachment 162159 [details]
Patch with different design

Attachment 162159 [details] did not pass cr-android-ews (chromium-android):
Output: http://queues.webkit.org/results/13743768

[Sam Weinig]

Comment on attachment 162159 [details]
Patch with different design

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

Lovin' it!

> Source/WebCore/page/LayoutMilestoneOptions.h:37
> +typedef unsigned LayoutMilestoneOptions;

I am not sure the word Options adds anything here.

> Source/WebKit2/UIProcess/API/C/WKPage.h:122
>      WKPageDidNewFirstVisuallyNonEmptyLayoutCallback                     didNewFirstVisuallyNonEmptyLayout;
> +    WKPageDidLayoutCallback                                             didLayout;

This new function needs to be added at the bottom of the struct.

> Source/WebKit2/WebProcess/InjectedBundle/InjectedBundlePageLoaderClient.cpp:259
> +void InjectedBundlePageLoaderClient::didLayout(WebPage* page, LayoutMilestoneOptions options, WTF::RefPtr<APIObject>& userData)

The WTF:: should not be necessary here.

> Source/WebKit2/WebProcess/InjectedBundle/InjectedBundlePageLoaderClient.h:86
> +    void didLayout(WebPage*, WebCore::LayoutMilestoneOptions, WTF::RefPtr<APIObject>& userData);

The WTF:: should not be needed here.

> Source/WebKit2/WebProcess/InjectedBundle/API/c/WKBundlePage.h:143
> +    WKBundlePageDidLayoutCallback                                           didLayout;

Function needs to be at the bottom of the struct.

> Source/WebKit2/WebProcess/InjectedBundle/API/c/WKBundlePagePrivate.h:-62
> -// FIXME: This is temporary. Ultimately WebKit should choose the threshold itself.
> -WK_EXPORT void WKBundlePageSetPaintedObjectsCounterThreshold(WKBundlePageRef page, uint64_t threshold);

Were sure this has no callers?

[Gyuyoung Kim]

Comment on attachment 162159 [details]
Patch with different design

Attachment 162159 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/13754680

[Gyuyoung Kim]

Comment on attachment 162159 [details]
Patch with different design

Attachment 162159 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/13742785

[Beth Dakin]

Created attachment 162307 [details]
Patch

Thanks, Sam! Here is a patch that addresses all of your comments and attempts to get the other ports building.

[WebKit Review Bot]

Attachment 162307 [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:319:  The parameter name "milestones" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 1 in 87 files


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

[Build Bot]

Comment on attachment 162307 [details]
Patch

Attachment 162307 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/13770043

[Beth Dakin]

Created attachment 162319 [details]
Patch

More build fixing.

[Beth Dakin]

Thanks, Sam! Committed http://trac.webkit.org/changeset/129545

[Beth Dakin]

This change caused a layout test to fail. I filed https://bugs.webkit.org/show_bug.cgi?id=97615 and committed a fix: http://trac.webkit.org/changeset/129569

[Adam Roben (:aroben)]

This seems to have caused bug 102970.