Bug 32875 - [Android] Android requires ability to increase layout delay
Summary: [Android] Android requires ability to increase layout delay
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: Android Android
: P2 Normal
Assignee: Steve Block
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2009-12-22 12:09 PST by Steve Block
Modified: 2010-01-07 07:05 PST (History)
13 users (show)

See Also:


Attachments
Patch 1 for Bug 32875 (2.68 KB, patch)
2009-12-30 05:31 PST, Steve Block
darin: review+
Details | Formatted Diff | Diff
Patch 2 for Bug 32875 (3.05 KB, patch)
2010-01-06 11:18 PST, Steve Block
darin: review+
steveblock: commit-queue-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Steve Block 2009-12-22 12:09:16 PST
Android requires the ability to increase the delay used when scheduling layout. See FrameView::scheduleRelayout().

Android increases the delay used after the first layout has completed.
Comment 1 Steve Block 2009-12-30 05:31:16 PST
Created attachment 45656 [details]
Patch 1 for Bug 32875
Comment 2 WebKit Review Bot 2009-12-30 05:31:40 PST
style-queue ran check-webkit-style on attachment 45656 [details] without any errors.
Comment 3 Eric Seidel 2009-12-30 09:40:46 PST
Why is this desirable?
Comment 4 Steve Block 2010-01-04 10:43:24 PST
(In reply to comment #3)
> Why is this desirable?
The idea is to reduce the number of re-layouts performed, in order to reduce the total page load time. This is particularly relevant for mobile devices like Android.

On Android, once the first layout is complete, we call setExtraLayoutDelay() to reduce the frequency of future re-layouts.

CC'ing Grace, who initially added this to Android.
Comment 5 Eric Seidel 2010-01-04 13:19:13 PST
I don't know if this is a good idea, but Hyatt, Beth or Mitz might have opinions.
Comment 6 David Kilzer (:ddkilzer) 2010-01-04 18:07:17 PST
Comment on attachment 45656 [details]
Patch 1 for Bug 32875

> Index: WebCore/dom/Document.cpp
> ===================================================================
> --- WebCore/dom/Document.cpp	(revision 52652)
> +++ WebCore/dom/Document.cpp	(working copy)
> @@ -407,6 +407,7 @@ Document::Document(Frame* frame, bool is
>      m_processingLoadEvent = false;
>      m_startTime = currentTime();
>      m_overMinimumLayoutThreshold = false;
> +    m_extraLayoutDelay = 0;
>      
>      initSecurityContext();
>      initDNSPrefetch();

I think you should be modifying cLayoutScheduleThreshold in Document.cpp instead of adding another member variable to Document.

Another approach (which the iPhone WebKit port uses) is to add a 'layoutInterval' item to Settings (so the value can be injected), then change uses of cLayoutScheduleThreshold to call settings()->layoutInterval() instead.  (The Settings::setLayoutInterval() method doesn't allow values less than cLayoutScheduleThreshold to be set when it's called, but that requires exporting cLayoutScheduleThreshold and removing the static keyword.)
Comment 7 David Kilzer (:ddkilzer) 2010-01-04 18:09:08 PST
(In reply to comment #6)
> Another approach (which the iPhone WebKit port uses) is to add a
> 'layoutInterval' item to Settings (so the value can be injected), then change
> uses of cLayoutScheduleThreshold to call settings()->layoutInterval() instead. 
> (The Settings::setLayoutInterval() method doesn't allow values less than
> cLayoutScheduleThreshold to be set when it's called, but that requires
> exporting cLayoutScheduleThreshold and removing the static keyword.)

And (obviously) the default value for layoutInterval is cLayoutScheduleThreshold as set in the WebKit port files.
Comment 8 Grace Kloba 2010-01-04 22:22:39 PST
It will be nice if you can share the iPhone WebKit port.

In Android, we only add an extra delay during loading. After a page is completed, the extra layout delay is removed. So if there is any JavaScript modifying the DOM, the layout won't be delayed.

Do you still think it is a good idea to use setting?
Comment 9 David Kilzer (:ddkilzer) 2010-01-04 23:13:41 PST
(In reply to comment #8)
> It will be nice if you can share the iPhone WebKit port.

We want to merge the iPhone port back to trunk; it's simply a matter of time and resources.  The source code for various releases is available on <http://www.opensource.apple.com/>.

> In Android, we only add an extra delay during loading. After a page is
> completed, the extra layout delay is removed. So if there is any JavaScript
> modifying the DOM, the layout won't be delayed.
> 
> Do you still think it is a good idea to use setting?

It probably doesn't make *as much* sense to use a setting here if you're going to vary it within the lifetime of the Document object, although you still have to export Document::setExtraLayoutDelay() to call it from your WebKit port.  The nice thing about using Settings is that you don't need exports since the getter/setter methods are all inline in Settings.h.

I would still favor using the Settings object, even though it's a bit unorthodox (unless someone else objects to it).
Comment 10 David Kilzer (:ddkilzer) 2010-01-04 23:33:37 PST
(In reply to comment #9)
> I would still favor using the Settings object, even though it's a bit
> unorthodox (unless someone else objects to it).

Hmm...the Settings object is per Page, not per Document.  The more I think about this, the less it feels like a setting.  And Document::setExtraLayoutDelay() doesn't have to be exported since it's also defined in the header.

I'd go ahead and put the review? flag back on the patch to see what others say.  Sorry for the review flag churn.
Comment 11 Steve Block 2010-01-05 04:47:43 PST
Comment on attachment 45656 [details]
Patch 1 for Bug 32875

Re-applying review? flag following result of discussion ...

Modifying cLayoutScheduleThreshold or using a setting is not appropriate, as the delay needs to be modified within the lifetime of the Document object, and is specific to a Document object, not to a Page.
Comment 12 Darin Adler 2010-01-05 13:08:31 PST
Comment on attachment 45656 [details]
Patch 1 for Bug 32875

> +    m_extraLayoutDelay = 0;

Should use member initialization syntax instead of assignment.

Seems OK.
Comment 13 Darin Adler 2010-01-05 13:09:01 PST
Comment on attachment 45656 [details]
Patch 1 for Bug 32875

In the .cpp file you probably need a comment mentioning this is used by Android. Otherwise, someone may remove this unused function.
Comment 14 Eric Seidel 2010-01-05 13:51:19 PST
I don't understand why minimumLayoutDelay returns an int.  It should be unsigned I would think.

I also think m_extraLayoutDelay should have a comment enxt to it in Document.h explaining what it's used for.

Otherwise this looks fine to me.
Comment 15 Steve Block 2010-01-06 11:18:14 PST
Created attachment 45970 [details]
Patch 2 for Bug 32875

> > +    m_extraLayoutDelay = 0;
> Should use member initialization syntax instead of assignment.
I used assignment to keep this variable with the other variables used in minimumLayoutDelay(), which are
initialized in this way. Do you want me to move the entire group to the initializer list?
 
> In the .cpp file you probably need a comment mentioning this is used by
> Android. Otherwise, someone may remove this unused function.
Fixed.

> I don't understand why minimumLayoutDelay returns an int.  It should be
> unsigned I would think.
It looks like there are plenty of cases (in this file and elsewhere) where int is used for values which will always be non-negative and I don't think this is the place to fix them all.

> I also think m_extraLayoutDelay should have a comment enxt to it in Document.h
> explaining what it's used for.
Fixed
Comment 16 WebKit Review Bot 2010-01-06 11:23:51 PST
Attachment 45970 [details] did not pass style-queue:

Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
Traceback (most recent call last):
  File "WebKitTools/Scripts/check-webkit-style", line 98, in <module>
    main()
  File "WebKitTools/Scripts/check-webkit-style", line 62, in main
    defaults = style.ArgumentDefaults(style.DEFAULT_OUTPUT_FORMAT,
AttributeError: 'module' object has no attribute 'ArgumentDefaults'
Comment 17 Eric Seidel 2010-01-06 11:25:24 PST
Looks like either the style-queue needs a reboot, or CJ's patch broke check-webkit-style. :(
Comment 18 Darin Adler 2010-01-06 12:13:28 PST
(In reply to comment #15)
> Created an attachment (id=45970) [details]
> > > +    m_extraLayoutDelay = 0;
> > Should use member initialization syntax instead of assignment.
> I used assignment to keep this variable with the other variables used in
> minimumLayoutDelay(), which are
> initialized in this way. Do you want me to move the entire group to the
> initializer list?

Sure, although normally I'd prefer to do that in a separate patch either before or after this one.

> > I don't understand why minimumLayoutDelay returns an int.  It should be
> > unsigned I would think.
> It looks like there are plenty of cases (in this file and elsewhere) where int
> is used for values which will always be non-negative and I don't think this is
> the place to fix them all.

I think this is a weak argument. Was Eric proposing fixing them all?
Comment 19 Darin Adler 2010-01-06 12:15:06 PST
Comment on attachment 45970 [details]
Patch 2 for Bug 32875

Repeating the same comment for both setExtraLayoutDelay and m_extraLayoutDelay seems like overkill to me.

Because of adding the comment, I suggest putting setExtraLayoutDelay into a separate paragraph with its comment with a blank line before and after.

r=me as is though
Comment 20 Chris Jerdonek 2010-01-06 13:44:01 PST
(In reply to comment #16)
> Attachment 45970 [details] did not pass style-queue:
> 
> Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
> Traceback (most recent call last):
>   File "WebKitTools/Scripts/check-webkit-style", line 98, in <module>
>     main()
>   File "WebKitTools/Scripts/check-webkit-style", line 62, in main
>     defaults = style.ArgumentDefaults(style.DEFAULT_OUTPUT_FORMAT,
> AttributeError: 'module' object has no attribute 'ArgumentDefaults'

That's unusual. That line gets executed every time check-webkit-style runs.  Has check-webkit-style failed any more times since last night's patch?

https://bugs.webkit.org/show_bug.cgi?id=32966#c13

I know it has succeeded at least once this morning.  See here for instance:

https://bugs.webkit.org/show_bug.cgi?id=32971#c4

By the way, this morning's check-webkit-style patch does not seem to have landed yet, so it doesn't seem to be the culprit either:

https://bugs.webkit.org/show_bug.cgi?id=32971

I can also see that it hasn't landed yet from the error message.
Comment 21 Chris Jerdonek 2010-01-06 13:52:44 PST
(In reply to comment #17)
> Looks like either the style-queue needs a reboot, or CJ's patch broke
> check-webkit-style. :(

Seems to be running okay as of a few minutes ago (2010-01-06 13:48:05 PST):

https://bugs.webkit.org/show_bug.cgi?id=32689#c7

So perhaps it was an isolated incident.
Comment 22 Steve Block 2010-01-07 03:38:12 PST
Comment on attachment 45970 [details]
Patch 2 for Bug 32875

Will land manually
Comment 23 Steve Block 2010-01-07 04:18:00 PST
> Sure, although normally I'd prefer to do that in a separate patch either before
> or after this one.
I've opened Bug 33316 to do this.

> I think this is a weak argument. Was Eric proposing fixing them all?
I don't think he was proposing to fix them all, but if we change the return type of the method, and the type of the variables and helper functions it uses, the original change might be lost in the churn. I've opened Bug 33317 to address this.

> Because of adding the comment, I suggest putting setExtraLayoutDelay into a
> separate paragraph with its comment with a blank line before and after.
Fixed

Submitted as http://trac.webkit.org/changeset/52919
Comment 24 Yong Li 2010-01-07 07:05:52 PST
Will making Timer prioritied help?

-Yong