Set WebFont priority to very low if the network is detected to be slow

Set WebFont priority to very low if the network is detected to be slow

When the network is detected to be slow, Blink immediately changes to
using fallback font. This CL changes the Blink resource priority of the
WebFont resource to VeryLow since the font is no longer required for
text paint.

BUG=665504
Committed: https://crrev.com/5ea0c57befbcadba648b21447e83d5d4a280c885
Cr-Commit-Position: refs/heads/master@{#432254}

[tbansal1, 2016-11-12 02:18:40]

Description was changed from

==========
Set web font priority to low if the network is detected to be slow

BUG=
==========

to

==========
Set web font priority to very low if the network is detected to be slow

When the network is detected to be slow, Blink immediately changes to
using fallback font. This CL changes the Blink resource priority of the
web font to VeryLow since the font is no longer required for
text paint.

BUG=
==========

[tbansal1, 2016-11-12 02:19:00]

The CQ bit was checked by tbansal@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-12 02:19:20]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[tbansal1, 2016-11-12 02:20:18]

Description was changed from

==========
Set web font priority to very low if the network is detected to be slow

When the network is detected to be slow, Blink immediately changes to
using fallback font. This CL changes the Blink resource priority of the
web font to VeryLow since the font is no longer required for
text paint.

BUG=
==========

to

==========
Set web font priority to very low if the network is detected to be slow

When the network is detected to be slow, Blink immediately changes to
using fallback font. This CL changes the Blink resource priority of the
web font resource to VeryLow since the font is no longer required for
text paint.

BUG=578029
==========

[tbansal1, 2016-11-12 02:20:39]

Description was changed from

==========
Set web font priority to very low if the network is detected to be slow

When the network is detected to be slow, Blink immediately changes to
using fallback font. This CL changes the Blink resource priority of the
web font resource to VeryLow since the font is no longer required for
text paint.

BUG=578029
==========

to

==========
Set WebFont priority to very low if the network is detected to be slow

When the network is detected to be slow, Blink immediately changes to
using fallback font. This CL changes the Blink resource priority of the
WebFont resource to VeryLow since the font is no longer required for
text paint.

BUG=578029
==========

[tbansal1, 2016-11-12 02:21:13]

tbansal@chromium.org changed reviewers:
+ toyoshim@chromium.org

[tbansal1, 2016-11-12 02:21:13]

toyoshim: ptal.

[commit-bot: I haz the power, 2016-11-12 05:09:59]

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

[commit-bot: I haz the power, 2016-11-12 05:10:00]

Dry run: This issue passed the CQ dry run.

[Takashi Toyoshima, 2016-11-14 03:44:25]

toyoshim@chromium.org changed reviewers:
+ hiroshige@chromium.org, pmeenan@google.com

[Takashi Toyoshima, 2016-11-14 03:45:10]

+pmeenan for resource fetcher prority, +hiroshige for fetch/ caller usages.

[Takashi Toyoshima, 2016-11-14 03:53:22]

toyoshim@chromium.org changed reviewers:
+ ksakamoto@chromium.org

[Takashi Toyoshima, 2016-11-14 03:53:56]

Here are some comments in terms of WebFonts.
+ksakamoto just in case.

https://codereview.chromium.org/2494243003/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/css/RemoteFontFaceSource.cpp (right):

https://codereview.chromium.org/2494243003/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/css/RemoteFontFaceSource.cpp:77: if
(!font->url().protocolIsData() && !font->isLoaded() &&
|font| is shared by multiple RemoteFontFaceSource instances.
So we may need to lower the priority only when all clients agreed.

https://codereview.chromium.org/2494243003/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/css/RemoteFontFaceSource.cpp:243: void
RemoteFontFaceSource::beginLoadIfNeeded() {
This is called for each RemoteFontFaceSource instance, and if shared |m_font|
didn't start actual font loading, this instance kicks it.

This may be a possible place to change loading priority though still other
clients may need higher priorities later.

[Pat Meenan, 2016-11-14 14:54:48]

pmeenan@chromium.org changed reviewers:
+ pmeenan@chromium.org

[Pat Meenan, 2016-11-14 14:54:48]

The priority change itself LGTM but odds are that the request will already be
in-flight by the time you make the change (even if it requires a new DNS lookup
and connection it will already be past the resource scheduler).

It would be a lot better if the fallback state was known at the time of the
request before it is sent to the scheduler and when the priority is initially
set on the blink side.

[tbansal1, 2016-11-14 20:09:52]

Patchset #2 (id:20001) has been deleted

[tbansal1, 2016-11-14 20:11:28]

Patchset #2 (id:40001) has been deleted

[tbansal1, 2016-11-14 20:12:08]

toyoshim: ptal. thanks.

https://codereview.chromium.org/2494243003/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/css/RemoteFontFaceSource.cpp (right):

https://codereview.chromium.org/2494243003/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/css/RemoteFontFaceSource.cpp:77: if
(!font->url().protocolIsData() && !font->isLoaded() &&
On 2016/11/14 03:53:56, toyoshim wrote:
> |font| is shared by multiple RemoteFontFaceSource instances.
> So we may need to lower the priority only when all clients agreed.

Done.

https://codereview.chromium.org/2494243003/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/css/RemoteFontFaceSource.cpp:243: void
RemoteFontFaceSource::beginLoadIfNeeded() {
On 2016/11/14 03:53:56, toyoshim wrote:
> This is called for each RemoteFontFaceSource instance, and if shared |m_font|
> didn't start actual font loading, this instance kicks it.
> 
> This may be a possible place to change loading priority though still other
> clients may need higher priorities later.

Done.

[tbansal1, 2016-11-14 20:13:49]

On 2016/11/14 14:54:48, Pat Meenan wrote:
> The priority change itself LGTM but odds are that the request will already be
> in-flight by the time you make the change (even if it requires a new DNS
lookup
> and connection it will already be past the resource scheduler).
> 
> It would be a lot better if the fallback state was known at the time of the
> request before it is sent to the scheduler and when the priority is initially
> set on the blink side.

Can you explain a bit more. I am setting priority on the blink side before
it hits content/net. I would expect it to work. I also checked
using net::internals. When the connection is fast, priority of font
requests is set to HIGHEST. On slow connections,
net::internals shows priority as IDLE.

[Pat Meenan, 2016-11-14 20:22:14]

On 2016/11/14 20:13:49, tbansal1 wrote:
> On 2016/11/14 14:54:48, Pat Meenan wrote:
> > The priority change itself LGTM but odds are that the request will already
be
> > in-flight by the time you make the change (even if it requires a new DNS
> lookup
> > and connection it will already be past the resource scheduler).
> > 
> > It would be a lot better if the fallback state was known at the time of the
> > request before it is sent to the scheduler and when the priority is
initially
> > set on the blink side.
> 
> Can you explain a bit more. I am setting priority on the blink side before
> it hits content/net. I would expect it to work. I also checked
> using net::internals. When the connection is fast, priority of font
> requests is set to HIGHEST. On slow connections,
> net::internals shows priority as IDLE.

Sorry, my comments were on the first patchset and it wasn't clear that the
priority was being changed before the resource request was issued.  The updated
CL looks much clearer and LGTM

[Takashi Toyoshima, 2016-11-15 02:22:42]

Patch Set 2 looks very nice. WebFonts LGTM.

Pat Meenan:
Because WebFonts loading is deferred in Blink, even the first patch would work
in most cases, e.g. when the current RemoteFontFaceSource is only the
FontResourceClient, I think. That's exceptional behavior of FontResource. But if
it's a second client or something, loading could be started there.

[tbansal1, 2016-11-15 02:35:58]

tbansal@chromium.org changed reviewers:
+ kinuko@chromium.org

[tbansal1, 2016-11-15 02:35:59]

kinuko: ptal at * for OWNERS approval. Thanks.

[Takashi Toyoshima, 2016-11-15 03:42:40]

To anyone who knows, I have a question not about this CL, but for related code.
didChangePriority() sounds like a client callback to be notified priority
change, but actually it looks a right code to set the priority.
Why the method name isn't changePriority()?
I'm trying to have more comments in core/fetch and core/loading as a side work
of Yukari project, and want to clarify this.

[kinuko, 2016-11-15 05:43:48]

lgtm % nit

https://codereview.chromium.org/2494243003/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/fetch/FontResource.h (right):

https://codereview.chromium.org/2494243003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/fetch/FontResource.h:122: virtual bool
isLowPriorityLoadingAllowedForRemoteFont() const { return true; }
It probably doesn't change anything but is the default return value being true
intentional?  (I often see returning false in such cases)

[kinuko, 2016-11-15 05:45:59]

On 2016/11/15 03:42:40, toyoshim wrote:
> To anyone who knows, I have a question not about this CL, but for related
code.
> didChangePriority() sounds like a client callback to be notified priority
> change, but actually it looks a right code to set the priority.
> Why the method name isn't changePriority()?
> I'm trying to have more comments in core/fetch and core/loading as a side work
> of Yukari project, and want to clarify this.

Resource::didChangePriority didn't use to actually set priority but was just a
notification after changing the priority until this change:
https://codereview.chromium.org/1378743002

We could probably rename it & clean it up.

(CC-ing japhet@ though he's ooo right now)

[hiroshige, 2016-11-15 06:05:41]

https://codereview.chromium.org/2494243003/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/fetch/FontResource.cpp (right):

https://codereview.chromium.org/2494243003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/fetch/FontResource.cpp:180: assert(!isLoaded());
Please use DCHECK() instead of assert().

[tbansal1, 2016-11-15 08:17:04]

hiroshige, ksakamato: ptal. Thanks.

https://codereview.chromium.org/2494243003/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/fetch/FontResource.cpp (right):

https://codereview.chromium.org/2494243003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/fetch/FontResource.cpp:180: assert(!isLoaded());
On 2016/11/15 06:05:41, hiroshige wrote:
> Please use DCHECK() instead of assert().

Done. Thanks for catching this.

https://codereview.chromium.org/2494243003/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/fetch/FontResource.h (right):

https://codereview.chromium.org/2494243003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/fetch/FontResource.h:122: virtual bool
isLowPriorityLoadingAllowedForRemoteFont() const { return true; }
On 2016/11/15 05:43:47, kinuko wrote:
> It probably doesn't change anything but is the default return value being true
> intentional?  (I often see returning false in such cases)
Yes it is intentional.  I have added more comments based on offline discussion
with ksakamato and toyoshim. 

Basically, for every font resource FontResourceHelper::FontResourceHelper also
gets automatically registered as a FontResourceClient. If we set the default
return to false here, then calling this function on
FontResourceHelper::FontResourceHelper  client would return false. This will
incorrectly prevent the priority from getting lowered.

Also, since this function is only called by RemoteFontFaceSOurce, and is
pertinent only to remote fonts, it is okay for default return to be true (as
long as RemoteFontFaceSOurce correctly overrides the function).

[Kunihiko Sakamoto, 2016-11-15 09:34:53]

lgtm

https://codereview.chromium.org/2494243003/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/fetch/FontResource.h (right):

https://codereview.chromium.org/2494243003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/fetch/FontResource.h:122: virtual bool
isLowPriorityLoadingAllowedForRemoteFont() const { return true; }
On 2016/11/15 08:17:04, tbansal1 wrote:
> On 2016/11/15 05:43:47, kinuko wrote:
> > It probably doesn't change anything but is the default return value being
true
> > intentional?  (I often see returning false in such cases)
> Yes it is intentional.  I have added more comments based on offline discussion
> with ksakamato and toyoshim. 
> 
> Basically, for every font resource FontResourceHelper::FontResourceHelper also
> gets automatically registered as a FontResourceClient. If we set the default
> return to false here, then calling this function on
> FontResourceHelper::FontResourceHelper  client would return false. This will
> incorrectly prevent the priority from getting lowered.
> 
> Also, since this function is only called by RemoteFontFaceSOurce, and is
> pertinent only to remote fonts, it is okay for default return to be true (as
> long as RemoteFontFaceSOurce correctly overrides the function).
> 

A supplementary explanation:
All subclasses of ResourceOwner<FontResource> are FontResourceClient, because
ResourceOwner<R> is derived from R::ClientType. So FontResourceHelper is a
FontResourceClient and can observe FontResource, but it shouldn't participate in
the decision of load priority.

[hiroshige, 2016-11-15 09:58:08]

lgtm

[tbansal1, 2016-11-15 17:31:54]

The CQ bit was checked by tbansal@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-15 17:33:00]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[tbansal1, 2016-11-15 18:09:09]

Description was changed from

==========
Set WebFont priority to very low if the network is detected to be slow

When the network is detected to be slow, Blink immediately changes to
using fallback font. This CL changes the Blink resource priority of the
WebFont resource to VeryLow since the font is no longer required for
text paint.

BUG=578029
==========

to

==========
Set WebFont priority to very low if the network is detected to be slow

When the network is detected to be slow, Blink immediately changes to
using fallback font. This CL changes the Blink resource priority of the
WebFont resource to VeryLow since the font is no longer required for
text paint.

BUG=665504
==========

[commit-bot: I haz the power, 2016-11-15 19:41:59]

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

[commit-bot: I haz the power, 2016-11-15 19:42:00]

Dry run: This issue passed the CQ dry run.

[tbansal1, 2016-11-15 20:18:39]

The CQ bit was checked by tbansal@chromium.org

[tbansal1, 2016-11-15 20:18:39]

The patchset sent to the CQ was uploaded after l-g-t-m from
toyoshim@chromium.org, kinuko@chromium.org, pmeenan@chromium.org
Link to the patchset: https://codereview.chromium.org/2494243003/#ps40002
(title: "addressed comments")

[tbansal1, 2016-11-15 20:18:49]

Thanks everybody for the review.

[commit-bot: I haz the power, 2016-11-15 20:19:07]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-15 20:55:20]

Description was changed from

==========
Set WebFont priority to very low if the network is detected to be slow

When the network is detected to be slow, Blink immediately changes to
using fallback font. This CL changes the Blink resource priority of the
WebFont resource to VeryLow since the font is no longer required for
text paint.

BUG=665504
==========

to

==========
Set WebFont priority to very low if the network is detected to be slow

When the network is detected to be slow, Blink immediately changes to
using fallback font. This CL changes the Blink resource priority of the
WebFont resource to VeryLow since the font is no longer required for
text paint.

BUG=665504
==========

[commit-bot: I haz the power, 2016-11-15 20:55:22]

Committed patchset #3 (id:40002)

[commit-bot: I haz the power, 2016-11-15 21:00:41]

Description was changed from

==========
Set WebFont priority to very low if the network is detected to be slow

When the network is detected to be slow, Blink immediately changes to
using fallback font. This CL changes the Blink resource priority of the
WebFont resource to VeryLow since the font is no longer required for
text paint.

BUG=665504
==========

to

==========
Set WebFont priority to very low if the network is detected to be slow

When the network is detected to be slow, Blink immediately changes to
using fallback font. This CL changes the Blink resource priority of the
WebFont resource to VeryLow since the font is no longer required for
text paint.

BUG=665504

Committed: https://crrev.com/5ea0c57befbcadba648b21447e83d5d4a280c885
Cr-Commit-Position: refs/heads/master@{#432254}
==========

[commit-bot: I haz the power, 2016-11-15 21:00:42]

Patchset 3 (id:??) landed as
https://crrev.com/5ea0c57befbcadba648b21447e83d5d4a280c885
Cr-Commit-Position: refs/heads/master@{#432254}