Add a Setting to make iframes load at VeryLow priority, disabled by default.

Add a Setting to make iframes load at VeryLow priority, disabled by default.

This will allow us to experiment with deferring iframes to see if it improves
loading behavior in bandwith-constrained environments.

BUG=

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

[Nate Chapin, 2015-06-19 19:14:31]

japhet@chromium.org changed reviewers:
+ bmcquade@chromium.org, ojan@chromium.org

[ojan, 2015-06-19 20:08:53]

ojan@chromium.org changed reviewers:
+ esprehn@chromium.org

[ojan, 2015-06-19 20:08:53]

The CQ bit was checked by ojan@chromium.org

[ojan, 2015-06-19 20:08:53]

lgtm

[commit-bot: I haz the power, 2015-06-19 20:09:23]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1195023002/1

[Bryan McQuade, 2015-06-19 20:27:10]

Thanks for putting this together! I'm wondering if there's some way to avoid
passing a bool isLowPriorityIFrame to ResourceFetcher::loadPriority(). It seems
like that method shouldn't have to take such a specific boolean as an argument.
Some ideas:
* we already know whether or not we're dealing with a frame based on the
Resource::Type type parameter
* I think we can figure out if we're dealing with the main or a child frame from
the FetchRequest parameter: request.resourceRequest().frameType() (If I
understand that attribute correctly)
* so the only bit of missing information is the general policy of whether to
deprioritize frames, as specified in the Settings object

Is there a shared object similar to Settings where we can store this policy
boolean information? Maybe ResourceFetcher itself should be told whether iframes
are low priority when it's constructed (maybe the class responsible for
constructing ResourceFetcher can look at Settings::lowPriorityIFrames and pass
that information into ResourceFetcher via a setter method?), and then
ResourceFetcher uses that internal state accordingly when loadPriority() gets
called?

[ojan, 2015-06-19 20:28:05]

The CQ bit was unchecked by ojan@chromium.org

[ojan, 2015-06-19 20:28:31]

I'll take it out of the CQ since Bryan has questions.

[Bryan McQuade, 2015-06-19 20:35:34]

Sorry for the late comment on the review. I'm also fine going forward with this
change as-is given ojan's lgtm.

https://codereview.chromium.org/1195023002/diff/1/Source/web/WebSettingsImpl.h
File Source/web/WebSettingsImpl.h (right):

https://codereview.chromium.org/1195023002/diff/1/Source/web/WebSettingsImpl....
Source/web/WebSettingsImpl.h:191: void setLowPriorityIframes(bool) override;
alternatively to this, we can use --blink-settings flag, which goes through
WebSettings::setFromStrings to set the value in the Settings instance. That
means you don't need to expose setters on WbSettings/WebSettingsImpl. For
running a field trial, I've found that it's easy to use the blink-settings flag
(see
https://chromium.googlesource.com/chromium/src/+/5d2d9cf3248dbefa5eab41dfc7c2...
for an example of another field trial that uses this flag), so unless you need
this for other purposes, you could consider removing it from this patch.

[Nate Chapin, 2015-06-19 20:37:55]

On 2015/06/19 20:27:10, Bryan McQuade wrote:
> Thanks for putting this together! I'm wondering if there's some way to avoid
> passing a bool isLowPriorityIFrame to ResourceFetcher::loadPriority(). It
seems
> like that method shouldn't have to take such a specific boolean as an
argument.
> Some ideas:
> * we already know whether or not we're dealing with a frame based on the
> Resource::Type type parameter
> * I think we can figure out if we're dealing with the main or a child frame
from
> the FetchRequest parameter: request.resourceRequest().frameType() (If I
> understand that attribute correctly)
> * so the only bit of missing information is the general policy of whether to
> deprioritize frames, as specified in the Settings object
> 
> Is there a shared object similar to Settings where we can store this policy
> boolean information? Maybe ResourceFetcher itself should be told whether
iframes
> are low priority when it's constructed (maybe the class responsible for
> constructing ResourceFetcher can look at Settings::lowPriorityIFrames and pass
> that information into ResourceFetcher via a setter method?), and then
> ResourceFetcher uses that internal state accordingly when loadPriority() gets
> called?

I keep thinking loadPriority() is a static file-scope function (it used to be,
but isn't anymore). It seems a little out of place for ResourceFetcher to stash
this state, since it has a canonical way to query more global state (via
FetchContext), and I think we typically haven't duplicated things like Settings
unless it needs to move off the main thread. I guess I'd lean toward dropping
the argument on loadPriority() and have loadPriority() call
FetchContext::isLowPriorityIframe() directly. Is that a reasonable balance?

[Bryan McQuade, 2015-06-19 20:41:57]

On 2015/06/19 20:27:10, Bryan McQuade wrote:
> Thanks for putting this together! I'm wondering if there's some way to avoid
> passing a bool isLowPriorityIFrame to ResourceFetcher::loadPriority(). It
seems
> like that method shouldn't have to take such a specific boolean as an
argument.
> Some ideas:
> * we already know whether or not we're dealing with a frame based on the
> Resource::Type type parameter
> * I think we can figure out if we're dealing with the main or a child frame
from
> the FetchRequest parameter: request.resourceRequest().frameType() (If I
> understand that attribute correctly)
> * so the only bit of missing information is the general policy of whether to
> deprioritize frames, as specified in the Settings object
> 
> Is there a shared object similar to Settings where we can store this policy
> boolean information? Maybe ResourceFetcher itself should be told whether
iframes
> are low priority when it's constructed (maybe the class responsible for
> constructing ResourceFetcher can look at Settings::lowPriorityIFrames and pass
> that information into ResourceFetcher via a setter method?), and then
> ResourceFetcher uses that internal state accordingly when loadPriority() gets
> called?

Looking at the code a bit more, I don't think a boolean 'lowPriorityIFrames'
belongs on ResoruceFetcher either. If there's a natural place to store this
that's accessible to ResourceFetcher::loadPriority() then we can consider moving
it there. Otherwise, I think it's fine to proceed with this change as you have
it.

[Bryan McQuade, 2015-06-19 20:43:56]

On 2015/06/19 20:37:55, Nate Chapin wrote:
> On 2015/06/19 20:27:10, Bryan McQuade wrote:
> > Thanks for putting this together! I'm wondering if there's some way to avoid
> > passing a bool isLowPriorityIFrame to ResourceFetcher::loadPriority(). It
> seems
> > like that method shouldn't have to take such a specific boolean as an
> argument.
> > Some ideas:
> > * we already know whether or not we're dealing with a frame based on the
> > Resource::Type type parameter
> > * I think we can figure out if we're dealing with the main or a child frame
> from
> > the FetchRequest parameter: request.resourceRequest().frameType() (If I
> > understand that attribute correctly)
> > * so the only bit of missing information is the general policy of whether to
> > deprioritize frames, as specified in the Settings object
> > 
> > Is there a shared object similar to Settings where we can store this policy
> > boolean information? Maybe ResourceFetcher itself should be told whether
> iframes
> > are low priority when it's constructed (maybe the class responsible for
> > constructing ResourceFetcher can look at Settings::lowPriorityIFrames and
pass
> > that information into ResourceFetcher via a setter method?), and then
> > ResourceFetcher uses that internal state accordingly when loadPriority()
gets
> > called?
> 
> I keep thinking loadPriority() is a static file-scope function (it used to be,
> but isn't anymore). It seems a little out of place for ResourceFetcher to
stash
> this state, since it has a canonical way to query more global state (via
> FetchContext), and I think we typically haven't duplicated things like
Settings
> unless it needs to move off the main thread. I guess I'd lean toward dropping
> the argument on loadPriority() and have loadPriority() call
> FetchContext::isLowPriorityIframe() directly. Is that a reasonable balance?

+1 to using FetchContext::isLowPriorityIframe - that does seem better.

[Nate Chapin, 2015-06-19 21:17:14]

On 2015/06/19 20:43:56, Bryan McQuade wrote:
> On 2015/06/19 20:37:55, Nate Chapin wrote:
> > On 2015/06/19 20:27:10, Bryan McQuade wrote:
> > > Thanks for putting this together! I'm wondering if there's some way to
avoid
> > > passing a bool isLowPriorityIFrame to ResourceFetcher::loadPriority(). It
> > seems
> > > like that method shouldn't have to take such a specific boolean as an
> > argument.
> > > Some ideas:
> > > * we already know whether or not we're dealing with a frame based on the
> > > Resource::Type type parameter
> > > * I think we can figure out if we're dealing with the main or a child
frame
> > from
> > > the FetchRequest parameter: request.resourceRequest().frameType() (If I
> > > understand that attribute correctly)
> > > * so the only bit of missing information is the general policy of whether
to
> > > deprioritize frames, as specified in the Settings object
> > > 
> > > Is there a shared object similar to Settings where we can store this
policy
> > > boolean information? Maybe ResourceFetcher itself should be told whether
> > iframes
> > > are low priority when it's constructed (maybe the class responsible for
> > > constructing ResourceFetcher can look at Settings::lowPriorityIFrames and
> pass
> > > that information into ResourceFetcher via a setter method?), and then
> > > ResourceFetcher uses that internal state accordingly when loadPriority()
> gets
> > > called?
> > 
> > I keep thinking loadPriority() is a static file-scope function (it used to
be,
> > but isn't anymore). It seems a little out of place for ResourceFetcher to
> stash
> > this state, since it has a canonical way to query more global state (via
> > FetchContext), and I think we typically haven't duplicated things like
> Settings
> > unless it needs to move off the main thread. I guess I'd lean toward
dropping
> > the argument on loadPriority() and have loadPriority() call
> > FetchContext::isLowPriorityIframe() directly. Is that a reasonable balance?
> 
> +1 to using FetchContext::isLowPriorityIframe - that does seem better.

Done

[Nate Chapin, 2015-06-19 21:30:05]

The CQ bit was checked by japhet@chromium.org

[Nate Chapin, 2015-06-19 21:30:05]

The patchset sent to the CQ was uploaded after l-g-t-m from
bmcquade@chromium.org, ojan@chromium.org
Link to the patchset: https://codereview.chromium.org/1195023002/#ps20001
(title: "Assume ApplyFromString(), call isLowPriorityIframe() from
loadPriority()")

[commit-bot: I haz the power, 2015-06-19 21:30:19]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1195023002/20001

[commit-bot: I haz the power, 2015-06-19 22:42:31]

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

[commit-bot: I haz the power, 2015-06-19 22:42:33]

Try jobs failed on following builders:
  mac_blink_rel on tryserver.blink (JOB_FAILED,
http://build.chromium.org/p/tryserver.blink/builders/mac_blink_rel/builds/59861)

[Nate Chapin, 2015-06-19 22:44:24]

The CQ bit was checked by japhet@chromium.org

[commit-bot: I haz the power, 2015-06-19 22:45:03]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1195023002/20001

[commit-bot: I haz the power, 2015-06-19 22:52:53]

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

[commit-bot: I haz the power, 2015-06-19 22:52:54]

Try jobs failed on following builders:
  mac_blink_rel on tryserver.blink (JOB_FAILED,
http://build.chromium.org/p/tryserver.blink/builders/mac_blink_rel/builds/59878)

[Nate Chapin, 2015-06-19 23:17:43]

The CQ bit was checked by japhet@chromium.org

[commit-bot: I haz the power, 2015-06-19 23:18:06]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1195023002/20001

[commit-bot: I haz the power, 2015-06-19 23:55:34]

Committed patchset #2 (id:20001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=197510