Log web fonts intervention result more accurately

Log web fonts intervention result more accurately

1) Log result only when there was no CORS error.
2) Log result only when font display was set to auto since
intervention is run only when the font display is auto.
3) Do not log result if the font failed to load.

BUG=656836
Committed: https://crrev.com/6f885eafc877e4170ac854794a7a33d72e1b0269
Cr-Commit-Position: refs/heads/master@{#426891}

[tbansal1, 2016-10-18 00:38:51]

Description was changed from

==========
Log web fonts intervention result

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

to

==========
Log web fonts intervention result

1) Log result only when there was no CORS error
2) Log result only when font display was set to auto
3) if the font failed to load, consider that as a 
longLimitExceeded event.

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

[tbansal1, 2016-10-18 00:39:07]

Description was changed from

==========
Log web fonts intervention result

1) Log result only when there was no CORS error
2) Log result only when font display was set to auto
3) if the font failed to load, consider that as a 
longLimitExceeded event.

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

to

==========
Log web fonts intervention result more accurately

1) Log result only when there was no CORS error
2) Log result only when font display was set to auto
3) if the font failed to load, consider that as a 
longLimitExceeded event.

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

[tbansal1, 2016-10-18 00:44:38]

Description was changed from

==========
Log web fonts intervention result more accurately

1) Log result only when there was no CORS error
2) Log result only when font display was set to auto
3) if the font failed to load, consider that as a 
longLimitExceeded event.

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

to

==========
Log web fonts intervention result more accurately

1) Log result only when there was no CORS error.
2) Log result only when font display was set to auto since
intervention is run only when the font display is auto.
3) If the font failed to load, consider that as a 
longLimitExceeded event.

BUG=656836
==========

[tbansal1, 2016-10-18 00:44:52]

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

[tbansal1, 2016-10-18 00:44:53]

toyoshim: ptal. thanks.

[tbansal1, 2016-10-18 00:45:00]

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

[commit-bot: I haz the power, 2016-10-18 00:45:33]

Dry run: 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-10-18 02:13:59]

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

[commit-bot: I haz the power, 2016-10-18 02:14:00]

Dry run: This issue passed the CQ dry run.

[tbansal1, 2016-10-18 05:49:30]

Description was changed from

==========
Log web fonts intervention result more accurately

1) Log result only when there was no CORS error.
2) Log result only when font display was set to auto since
intervention is run only when the font display is auto.
3) If the font failed to load, consider that as a 
longLimitExceeded event.

BUG=656836
==========

to

==========
Log web fonts intervention result more accurately

1) Log result only when there was no CORS error.
2) Log result only when font display was set to auto since
intervention is run only when the font display is auto.
3) Do not log result if the font failed to load.

BUG=656836
==========

[tbansal1, 2016-10-18 06:12:48]

toyoshim: ptal. I slightly updated the logic in ps#2.

[Takashi Toyoshima, 2016-10-19 06:36:53]

+ksakamoto

https://codereview.chromium.org/2431503002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/css/RemoteFontFaceSource.cpp (right):

https://codereview.chromium.org/2431503002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/RemoteFontFaceSource.cpp:412: 
As you changed, this method is called from two places.
- fontLoaded()
- longLimitExceeded()

For the latter case, font->X() won't work correctly because loading is still
under working. So, can you have this error state check in caller side? We may
want to have this check only in fontLoaded() case, in notifyFinished().

That would omit to record WebFont.InterventionResult too at line 411, but I
think it's fine.

https://codereview.chromium.org/2431503002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/RemoteFontFaceSource.cpp:417: display ==
FontDisplayAuto && font->getStatus() != Resource::LoadError) {
I slightly prefer to pass the |display| at m_histograms' constructor and hold it
inside the instance rather than passing it on demand.

Note: |display| should be always FontDisplayAuto unless users enable
chrome://flags/#enable-experimental-web-platform-features. So I thinks this
check won't affect results so much.

[tbansal1, 2016-10-19 08:27:56]

https://codereview.chromium.org/2431503002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/css/RemoteFontFaceSource.cpp (right):

https://codereview.chromium.org/2431503002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/RemoteFontFaceSource.cpp:412: 
On 2016/10/19 06:36:52, toyoshim wrote:
> As you changed, this method is called from two places.
> - fontLoaded()
> - longLimitExceeded()
> 
> For the latter case, font->X() won't work correctly because loading is still
> under working. So, can you have this error state check in caller side? We may
> want to have this check only in fontLoaded() case, in notifyFinished().
> 
> That would omit to record WebFont.InterventionResult too at line 411, but I
> think it's fine.

I need to check this to make sure, but when this method is called in case of
longLimitExceeded, then should not resource status be pending? I am skipping the
UMA only if the status is LoadError. So, the UMA should still be recorded in the
case of longLimitExceeded. Right?

[Takashi Toyoshima, 2016-10-19 10:16:28]

https://codereview.chromium.org/2431503002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/css/RemoteFontFaceSource.cpp (right):

https://codereview.chromium.org/2431503002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/RemoteFontFaceSource.cpp:412: 
That's true for the current implementation. But we do not want to have
unnecessary assumptions here against Resource internal design. It could be
changed frequently, and would be a root of regression bugs. E.g., cache-aware
loading would have a retry logic, and it isn't trivial if the internal state
continues to be always pending there in the future.

Also passing |font| instance to m_histograms results in exposing too much
information from RemoteFontFaceSource to FontLoadHistograms. That's my another
concern.

[tbansal1, 2016-10-19 23:51:06]

Patchset #3 (id:40001) has been deleted

[tbansal1, 2016-10-20 00:09:11]

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

[commit-bot: I haz the power, 2016-10-20 00:10: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-10-20 00:10:58]

toyoshim: ptal. thanks.

https://codereview.chromium.org/2431503002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/css/RemoteFontFaceSource.cpp (right):

https://codereview.chromium.org/2431503002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/RemoteFontFaceSource.cpp:412: 
On 2016/10/19 10:16:28, toyoshim wrote:
> That's true for the current implementation. But we do not want to have
> unnecessary assumptions here against Resource internal design. It could be
> changed frequently, and would be a root of regression bugs. E.g., cache-aware
> loading would have a retry logic, and it isn't trivial if the internal state
> continues to be always pending there in the future.
Agreed. That;s why I am not checking for pending state, but instead for error
state which seems more reliable (since it is a clear signal that browser was not
able to fetch the resource). 

This would help in reducing false positives when the linked web font points to a
URL that does not exist. Right now, all those cases are incorrectly recorded as
false positives.

> 
> Also passing |font| instance to m_histograms results in exposing too much
> information from RemoteFontFaceSource to FontLoadHistograms. That's my another
> concern.
Done

https://codereview.chromium.org/2431503002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/RemoteFontFaceSource.cpp:417: display ==
FontDisplayAuto && font->getStatus() != Resource::LoadError) {
On 2016/10/19 06:36:52, toyoshim wrote:
> I slightly prefer to pass the |display| at m_histograms' constructor and hold
it
> inside the instance rather than passing it on demand.
> 
> Note: |display| should be always FontDisplayAuto unless users enable
> chrome://flags/#enable-experimental-web-platform-features. So I thinks this
> check won't affect results so much.

Done.

[commit-bot: I haz the power, 2016-10-20 02:23:34]

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

[commit-bot: I haz the power, 2016-10-20 02:23:34]

Dry run: Try jobs failed on following builders:
  linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Takashi Toyoshima, 2016-10-20 10:12:44]

https://codereview.chromium.org/2431503002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/css/RemoteFontFaceSource.cpp (right):

https://codereview.chromium.org/2431503002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/RemoteFontFaceSource.cpp:417: display ==
FontDisplayAuto && font->getStatus() != Resource::LoadError) {
Thanks!

https://codereview.chromium.org/2431503002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/css/RemoteFontFaceSource.cpp (right):

https://codereview.chromium.org/2431503002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/RemoteFontFaceSource.cpp:267: bool
isCorsFailed,
Probably we can just have one argument to indicate any error here.

https://codereview.chromium.org/2431503002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/RemoteFontFaceSource.cpp:270: if
(!m_isLongLimitExceeded) {
Can we check error here?

if (!m_isLongLimitExceeded  & !error)
  recordInterventionResult(isInterventionTriggered);

https://codereview.chromium.org/2431503002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/RemoteFontFaceSource.cpp:276: bool
isCorsFailed,
These two arguments look curious to me, because this method is called when long
timeout is fired and loading is still in progress without any error.

If we want to exclude cases that long limit is exceeded and some network errors
happen, we need to stop recording intervention result here and report it only
when the load is finished. But it causes another issue that we fail to report
metrics in cases user takes another navigation while loading the font.

https://codereview.chromium.org/2431503002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/RemoteFontFaceSource.cpp:418: 
Do you have any reason why you omit only missedCacheInterventionHistogram here?

IMO, as names implies, it would be better not to have other differences between
WebFont.InterventionResult and WebFont.InterventionResult.MissedCache than cache
condition.

[tbansal1, 2016-10-21 06:56:13]

Patchset #4 (id:80001) has been deleted

[tbansal1, 2016-10-21 06:58:13]

toyoshim: ptal. thanks.

https://codereview.chromium.org/2431503002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/css/RemoteFontFaceSource.cpp (right):

https://codereview.chromium.org/2431503002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/RemoteFontFaceSource.cpp:267: bool
isCorsFailed,
On 2016/10/20 10:12:44, toyoshim wrote:
> Probably we can just have one argument to indicate any error here.

I am still using two different errors. It seems more readable this way, but I
can change if you feel strongly about this.

https://codereview.chromium.org/2431503002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/RemoteFontFaceSource.cpp:270: if
(!m_isLongLimitExceeded) {
On 2016/10/20 10:12:44, toyoshim wrote:
> Can we check error here?
> 
> if (!m_isLongLimitExceeded  & !error)
>   recordInterventionResult(isInterventionTriggered);

Done.

https://codereview.chromium.org/2431503002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/RemoteFontFaceSource.cpp:276: bool
isCorsFailed,
On 2016/10/20 10:12:44, toyoshim wrote:
> These two arguments look curious to me, because this method is called when
long
> timeout is fired and loading is still in progress without any error.
> 
> If we want to exclude cases that long limit is exceeded and some network
errors
> happen, we need to stop recording intervention result here and report it only
> when the load is finished. But it causes another issue that we fail to report
> metrics in cases user takes another navigation while loading the font.

Done.

https://codereview.chromium.org/2431503002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/RemoteFontFaceSource.cpp:418: 
On 2016/10/20 10:12:44, toyoshim wrote:
> Do you have any reason why you omit only missedCacheInterventionHistogram
here?
> 
> IMO, as names implies, it would be better not to have other differences
between
> WebFont.InterventionResult and WebFont.InterventionResult.MissedCache than
cache
> condition.

Done.

[Takashi Toyoshima, 2016-10-21 10:02:16]

Thanks. PS4 LGTM.

[tbansal1, 2016-10-21 16:08:51]

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

[tbansal1, 2016-10-21 16:08:52]

kinuko: ptal at * for OWNERS approval. Thanks.

[kinuko, 2016-10-21 17:07:11]

lgtm

https://codereview.chromium.org/2431503002/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/css/RemoteFontFaceSource.cpp (right):

https://codereview.chromium.org/2431503002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/css/RemoteFontFaceSource.cpp:117:
m_font->getStatus() == Resource::LoadError,
nit: if these two are only used in &&'ed bool check, shall we change
fontLoaded() to just take two params, say, loadError and
isInterventionTriggered, and just pass "m_font->isCORSFailed() ||
m_font->getStatus() == Resource::LoadError" here?

(either works for me though)

[tbansal1, 2016-10-21 17:14:32]

https://codereview.chromium.org/2431503002/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/css/RemoteFontFaceSource.cpp (right):

https://codereview.chromium.org/2431503002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/css/RemoteFontFaceSource.cpp:117:
m_font->getStatus() == Resource::LoadError,
On 2016/10/21 17:07:11, kinuko wrote:
> nit: if these two are only used in &&'ed bool check, shall we change
> fontLoaded() to just take two params, say, loadError and
> isInterventionTriggered, and just pass "m_font->isCORSFailed() ||
> m_font->getStatus() == Resource::LoadError" here?
> 
> (either works for me though)

I thought about this, but concluded that it might be a bit confusing. If we use
|loadError| as the variable name, then |loadError| would now mean either a CORS
failed error, or an actual resource load error. This would be confusing.

If we pass in a single variable, then other options for the variable name are
|corsOrLoadError| or |error|. The former is clearer but mouthful. The latter is
a bit vague.

So, I decided to just use two different variables to be clearer.

[tbansal1, 2016-10-21 17:14:38]

The CQ bit was checked by tbansal@chromium.org

[commit-bot: I haz the power, 2016-10-21 17:15:06]

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-10-21 19:08:15]

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

[commit-bot: I haz the power, 2016-10-21 19:08:17]

Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[tbansal1, 2016-10-21 21:07:41]

The CQ bit was checked by tbansal@chromium.org

[commit-bot: I haz the power, 2016-10-21 21:08:20]

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-10-21 21:34:37]

Description was changed from

==========
Log web fonts intervention result more accurately

1) Log result only when there was no CORS error.
2) Log result only when font display was set to auto since
intervention is run only when the font display is auto.
3) Do not log result if the font failed to load.

BUG=656836
==========

to

==========
Log web fonts intervention result more accurately

1) Log result only when there was no CORS error.
2) Log result only when font display was set to auto since
intervention is run only when the font display is auto.
3) Do not log result if the font failed to load.

BUG=656836

Committed: https://crrev.com/6f885eafc877e4170ac854794a7a33d72e1b0269
Cr-Commit-Position: refs/heads/master@{#426891}
==========

[commit-bot: I haz the power, 2016-10-21 21:34:39]

Patchset 4 (id:??) landed as
https://crrev.com/6f885eafc877e4170ac854794a7a33d72e1b0269
Cr-Commit-Position: refs/heads/master@{#426891}

[commit-bot: I haz the power, 2016-10-21 21:35:15]

Failed to apply the patch.
On branch working_branch
Your branch is up-to-date with 'origin/refs/pending/heads/master'.
nothing to commit, working tree clean