Schedule image resource downloads by decreasing visual impact - Blink Side

Schedule image resource downloads by decreasing visual impact

This CL adds an intra-priority level sorting value for finer grained
control of resource loading. The blink side of this patch sorts images by
estimated on screen real-estate, preferring to download images in order of
decreasing visual impact. Ie. On a page with a large banner, and dozens of
small icons, the large banner will most likely be scheduled ahead of the
others.

In the future, we can also use this to experiment with other signals, ie.
prioritize out of viewport images by their distance to viewport.

Will be followed up by https://codereview.chromium.org/146333004/.

In our tests, this was a nice win on speed-index for desktop, with a gain of 
about ~2-3%. 

This is first of 3 sided patch. Following pattern suggested on chromium-dev:

"
1) Add the new WebKit interface, preserving the old.
2) Change Chrome to use the new interface.
3) Modify WebKit to remove the old interface.
"

Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=170072

[shatch, 2014-02-12 21:31:20]

On 2014/02/10 19:54:15, shatch wrote:

ping!

[esprehn, 2014-02-12 21:42:17]

Is there a reason to make this a generic "int intraPriorityValue" instead of
making it "screenArea"? Do you plan to use the int for a different meaning
elsewhere?

https://codereview.chromium.org/153983004/diff/150001/Source/core/loader/Fram...
File Source/core/loader/FrameLoaderClient.h (right):

https://codereview.chromium.org/153983004/diff/150001/Source/core/loader/Fram...
Source/core/loader/FrameLoaderClient.h:220: virtual void
dispatchDidChangeResourcePriority(unsigned long /*identifier*/,
ResourceLoadPriority, int /*intraPriorityValue*/) { }
I don't think you need to comment out the names here. I'd just put the vars in.

https://codereview.chromium.org/153983004/diff/150001/Source/web/FrameLoaderC...
File Source/web/FrameLoaderClientImpl.cpp (right):

https://codereview.chromium.org/153983004/diff/150001/Source/web/FrameLoaderC...
Source/web/FrameLoaderClientImpl.cpp:291: unsigned long identifier,
ResourceLoadPriority priority, int intraPriorityValue)
I wouldn't wrap this at all, you don't wrap the code below anyway.

[shatch, 2014-02-13 18:13:49]

New snapshot uploaded, ptal.

For the "intraPriorityValue" name, screen area gives us a nice boost right now
but we do plan on trying out some different signals in the future to see what we
can get. Plumbing through the intra priority value from the
ResourceLoadPriorityOptimizer through to the resource scheduler is a first step.
There's no guarantee that it'll just be screen area in the future.

https://codereview.chromium.org/153983004/diff/150001/Source/core/loader/Fram...
File Source/core/loader/FrameLoaderClient.h (right):

https://codereview.chromium.org/153983004/diff/150001/Source/core/loader/Fram...
Source/core/loader/FrameLoaderClient.h:220: virtual void
dispatchDidChangeResourcePriority(unsigned long /*identifier*/,
ResourceLoadPriority, int /*intraPriorityValue*/) { }
On 2014/02/12 21:42:18, esprehn wrote:
> I don't think you need to comment out the names here. I'd just put the vars
in.

Done.

https://codereview.chromium.org/153983004/diff/150001/Source/web/FrameLoaderC...
File Source/web/FrameLoaderClientImpl.cpp (right):

https://codereview.chromium.org/153983004/diff/150001/Source/web/FrameLoaderC...
Source/web/FrameLoaderClientImpl.cpp:291: unsigned long identifier,
ResourceLoadPriority priority, int intraPriorityValue)
On 2014/02/12 21:42:18, esprehn wrote:
> I wouldn't wrap this at all, you don't wrap the code below anyway.

Done.

[shatch, 2014-02-19 00:33:01]

ping!

On 2014/02/13 18:13:49, shatch wrote:
> New snapshot uploaded, ptal.
> 
> For the "intraPriorityValue" name, screen area gives us a nice boost right now
> but we do plan on trying out some different signals in the future to see what
we
> can get. Plumbing through the intra priority value from the
> ResourceLoadPriorityOptimizer through to the resource scheduler is a first
step.
> There's no guarantee that it'll just be screen area in the future.
> 
>
https://codereview.chromium.org/153983004/diff/150001/Source/core/loader/Fram...
> File Source/core/loader/FrameLoaderClient.h (right):
> 
>
https://codereview.chromium.org/153983004/diff/150001/Source/core/loader/Fram...
> Source/core/loader/FrameLoaderClient.h:220: virtual void
> dispatchDidChangeResourcePriority(unsigned long /*identifier*/,
> ResourceLoadPriority, int /*intraPriorityValue*/) { }
> On 2014/02/12 21:42:18, esprehn wrote:
> > I don't think you need to comment out the names here. I'd just put the vars
> in.
> 
> Done.
> 
>
https://codereview.chromium.org/153983004/diff/150001/Source/web/FrameLoaderC...
> File Source/web/FrameLoaderClientImpl.cpp (right):
> 
>
https://codereview.chromium.org/153983004/diff/150001/Source/web/FrameLoaderC...
> Source/web/FrameLoaderClientImpl.cpp:291: unsigned long identifier,
> ResourceLoadPriority priority, int intraPriorityValue)
> On 2014/02/12 21:42:18, esprehn wrote:
> > I wouldn't wrap this at all, you don't wrap the code below anyway.
> 
> Done.

[shatch, 2014-02-20 22:38:44]

ping!

On 2014/02/19 00:33:01, shatch wrote:
> ping!
> 
> On 2014/02/13 18:13:49, shatch wrote:
> > New snapshot uploaded, ptal.
> > 
> > For the "intraPriorityValue" name, screen area gives us a nice boost right
now
> > but we do plan on trying out some different signals in the future to see
what
> we
> > can get. Plumbing through the intra priority value from the
> > ResourceLoadPriorityOptimizer through to the resource scheduler is a first
> step.
> > There's no guarantee that it'll just be screen area in the future.
> > 
> >
>
https://codereview.chromium.org/153983004/diff/150001/Source/core/loader/Fram...
> > File Source/core/loader/FrameLoaderClient.h (right):
> > 
> >
>
https://codereview.chromium.org/153983004/diff/150001/Source/core/loader/Fram...
> > Source/core/loader/FrameLoaderClient.h:220: virtual void
> > dispatchDidChangeResourcePriority(unsigned long /*identifier*/,
> > ResourceLoadPriority, int /*intraPriorityValue*/) { }
> > On 2014/02/12 21:42:18, esprehn wrote:
> > > I don't think you need to comment out the names here. I'd just put the
vars
> > in.
> > 
> > Done.
> > 
> >
>
https://codereview.chromium.org/153983004/diff/150001/Source/web/FrameLoaderC...
> > File Source/web/FrameLoaderClientImpl.cpp (right):
> > 
> >
>
https://codereview.chromium.org/153983004/diff/150001/Source/web/FrameLoaderC...
> > Source/web/FrameLoaderClientImpl.cpp:291: unsigned long identifier,
> > ResourceLoadPriority priority, int intraPriorityValue)
> > On 2014/02/12 21:42:18, esprehn wrote:
> > > I wouldn't wrap this at all, you don't wrap the code below anyway.
> > 
> > Done.

[esprehn, 2014-02-20 23:05:14]

lgtm, seems reasonable. Btw I fixed up your change description. It should start
with a summary line and then a new line followed by a description. You should
also wrap your lines somewhere reasonable.

https://codereview.chromium.org/153983004/diff/220001/Source/core/fetch/Resou...
File Source/core/fetch/ResourceLoadPriorityOptimizer.cpp (right):

https://codereview.chromium.org/153983004/diff/220001/Source/core/fetch/Resou...
Source/core/fetch/ResourceLoadPriorityOptimizer.cpp:48: , screenArea(s)
We don't usually abbreviate like this.

[shatch, 2014-03-24 23:48:10]

The CQ bit was checked by simonhatch@chromium.org

[commit-bot: I haz the power, 2014-03-24 23:48:11]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/simonhatch@chromium.org/153983004/340001

[commit-bot: I haz the power, 2014-03-24 23:51:58]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-03-24 23:51:59]

Try jobs failed on following builders:
  tryserver.blink on blink_presubmit

[shatch, 2014-03-25 18:58:08]

The CQ bit was checked by simonhatch@chromium.org

[commit-bot: I haz the power, 2014-03-25 18:58:15]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/simonhatch@chromium.org/153983004/340001

[commit-bot: I haz the power, 2014-03-25 19:10:02]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-03-25 19:10:02]

Try jobs failed on following builders:
  tryserver.blink on blink_presubmit

[esprehn, 2014-03-25 21:24:37]

The CQ bit was checked by esprehn@chromium.org

[commit-bot: I haz the power, 2014-03-25 21:24:38]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/simonhatch@chromium.org/153983004/340001

[commit-bot: I haz the power, 2014-03-25 21:33:38]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-03-25 21:33:38]

Try jobs failed on following builders:
  tryserver.blink on blink_presubmit

[abarth-chromium, 2014-03-25 21:56:17]

public/ LGTM

[shatch, 2014-03-26 17:21:09]

On 2014/03/25 21:56:17, abarth wrote:
> public/ LGTM

Doh! Thought I had reviewers covered, thanks!

[shatch, 2014-03-26 17:48:31]

The CQ bit was checked by simonhatch@chromium.org

[commit-bot: I haz the power, 2014-03-26 17:48:35]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/simonhatch@chromium.org/153983004/340001

[commit-bot: I haz the power, 2014-03-26 17:53:22]

Change committed as 170072