Summary: | Need to merge didFirstVisuallyNonEmptyLayout and didNewFirstVisuallyNonEmptyLayout | ||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Beth Dakin <bdakin> | ||||||||||||||||
Component: | WebKit API | Assignee: | Beth Dakin <bdakin> | ||||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||||
Severity: | Normal | CC: | abarth, abecsi, aroben, bdakin, cmarcelo, dglazkov, gyuyoung.kim, hausmann, japhet, jturcotte, kenneth, menard, mifenton, peter+ews, rakuco, sam, webkit.review.bot, zalan, zoltan | ||||||||||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||||||||
Hardware: | Unspecified | ||||||||||||||||||
OS: | Unspecified | ||||||||||||||||||
Attachments: |
|
Description
Beth Dakin
2012-08-29 16:24:56 PDT
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.
didUnlockLayoutAchievement is cute. Perhaps didChangeLayoutState to be analogous with document.readyState ? Comment on attachment 161368 [details] Patch Attachment 161368 [details] did not pass qt-wk2-ews (qt): Output: http://queues.webkit.org/results/13680484 Created attachment 161375 [details]
Patch, trying to fix QT build
(In reply to comment #2) > didUnlockLayoutAchievement is cute. Perhaps didChangeLayoutState to be analogous with document.readyState ? That's a good option too! 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 Created attachment 161527 [details]
Patch, with more build fixing
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 Created attachment 161565 [details]
Patch for EFL
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.
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.
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 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 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 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 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 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? 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 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 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.
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.
Comment on attachment 162307 [details] Patch Attachment 162307 [details] did not pass win-ews (win): Output: http://queues.webkit.org/results/13770043 Created attachment 162319 [details]
Patch
More build fixing.
Thanks, Sam! Committed http://trac.webkit.org/changeset/129545 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 This seems to have caused bug 102970. |