Bug 32875 – [Android] Android requires ability to increase layout delay

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Steve Block]

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.

[Steve Block]

Created attachment 45656 [details]
Patch 1 for Bug 32875

[WebKit Review Bot]

style-queue ran check-webkit-style on attachment 45656 [details] without any errors.

[Eric Seidel (no email)]

Why is this desirable?

[Steve Block]

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

[Eric Seidel (no email)]

I don't know if this is a good idea, but Hyatt, Beth or Mitz might have opinions.

[David Kilzer (:ddkilzer)]

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

[David Kilzer (:ddkilzer)]

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

[Grace Kloba]

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?

[David Kilzer (:ddkilzer)]

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

[David Kilzer (:ddkilzer)]

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

[Steve Block]

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.

[Darin Adler]

Comment on attachment 45656 [details]
Patch 1 for Bug 32875

> +    m_extraLayoutDelay = 0;

Should use member initialization syntax instead of assignment.

Seems OK.

[Darin Adler]

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.

[Eric Seidel (no email)]

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.

[Steve Block]

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

[WebKit Review Bot]

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'

[Eric Seidel (no email)]

Looks like either the style-queue needs a reboot, or CJ's patch broke check-webkit-style. :(

[Darin Adler]

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

[Darin Adler]

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

[Chris Jerdonek]

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

[Chris Jerdonek]

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

[Steve Block]

Comment on attachment 45970 [details]
Patch 2 for Bug 32875

Will land manually

[Steve Block]

> 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

[Yong Li]

Will making Timer prioritied help?

-Yong