FontLoadHistograms: classify as memory cache hit if FontResource loading not triggered

FontLoadHistograms: classify as memory cache hit if FontResource loading not triggered

In cases where FontResource loading is already started by another
RemoteFontFaceSource instance, |m_loadStartTime| is not set and result in
invalid load time records. Since we are collecting load times from the beginning
of the request, we classify this case as memory cache hit and don't record load
time.

Committed: https://crrev.com/63b083a06f0b83985eb39d1bf7d9543d942c82c7
Cr-Commit-Position: refs/heads/master@{#422006}

[Shao-Chuan Lee, 2016-09-21 08:43:50]

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

[Shao-Chuan Lee, 2016-09-21 08:43:50]

PTAL

[Kunihiko Sakamoto, 2016-09-21 09:23:51]

lgtm

[Shao-Chuan Lee, 2016-09-21 09:25:42]

shaochuan@chromium.org changed reviewers:
+ kouhei@chromium.org

[Shao-Chuan Lee, 2016-09-21 09:25:42]

kouhei@: PTAL as OWNERS, thanks

[Shao-Chuan Lee, 2016-09-23 01:12:30]

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

[commit-bot: I haz the power, 2016-09-23 01:12:59]

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

[kouhei (in TOK), 2016-09-23 01:34:00]

lgtm % below

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

https://codereview.chromium.org/2359493004/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/css/RemoteFontFaceSource.cpp:282:
DCHECK(m_loadStartTime);
using DCHECK on double scares me a bit.
DCHECK_LT(m_loadStartTime, 0.0); perhaps?

[Shao-Chuan Lee, 2016-09-23 01:47:19]

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

https://codereview.chromium.org/2359493004/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/css/RemoteFontFaceSource.cpp:282:
DCHECK(m_loadStartTime);
On 2016/09/23 01:33:59, kouhei wrote:
> using DCHECK on double scares me a bit.
> DCHECK_LT(m_loadStartTime, 0.0); perhaps?

Checking if non-zero should suffice here, maybe I should use
DCHECK_NE(m_loadStartTime, 0) to be clear?

[Shao-Chuan Lee, 2016-09-23 01:50:19]

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

[commit-bot: I haz the power, 2016-09-23 01:50:53]

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-09-23 02:50:01]

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

[commit-bot: I haz the power, 2016-09-23 02:50:01]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[Shao-Chuan Lee, 2016-09-23 06:45:57]

Previous fix only deal with cases when |m_font| is pending while
beginLoadIfNeeded() is called from CSSFontFace::load(). If |m_font| loading is
complete, beginLoadIfNeeded() will not be called.
I think the simplest solution is to check if |m_loadStartTime| is set at
recordRemoteFont().

[Kunihiko Sakamoto, 2016-09-23 07:39:19]

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

https://codereview.chromium.org/2359493004/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/RemoteFontFaceSource.cpp:287: m_dataSource =
FromMemoryCache;
Before this patch, we only had transitions from FromUnknown to others. This
introduces a path from From{DiskCache,Network} to FromMemoryCache - I think this
makes it hard to reason about this field.

Instead, can we do this in the maySetDataSource() call at line 110?

[Shao-Chuan Lee, 2016-09-23 08:41:06]

I noticed that m_dataSource may also be set in
FontLoadHistograms::longLimitExceeded(), which will cause problems if we
determine the case in notifyFinished().
I guess it's OK to move all recordInterventionResult() calls to fontLoaded()
which is called afterwards in notifyFinished()? However this will change the
results of WebFont.InterventionResult.MissedCache.

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

https://codereview.chromium.org/2359493004/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/RemoteFontFaceSource.cpp:287: m_dataSource =
FromMemoryCache;
On 2016/09/23 07:39:18, Kunihiko Sakamoto wrote:
> Before this patch, we only had transitions from FromUnknown to others. This
> introduces a path from From{DiskCache,Network} to FromMemoryCache - I think
this
> makes it hard to reason about this field.
> 
> Instead, can we do this in the maySetDataSource() call at line 110?

Done.

[Kunihiko Sakamoto, 2016-09-23 09:19:05]

On 2016/09/23 08:41:06, Shao-Chuan Lee wrote:
> I noticed that m_dataSource may also be set in
> FontLoadHistograms::longLimitExceeded(), which will cause problems if we
> determine the case in notifyFinished().
> I guess it's OK to move all recordInterventionResult() calls to fontLoaded()
> which is called afterwards in notifyFinished()? However this will change the
> results of WebFont.InterventionResult.MissedCache.

If a font takes too long to load and user left the page before completion,
fontLoaded() may not be called. Is that OK?

[Shao-Chuan Lee, 2016-09-29 08:48:18]

Description was changed from

==========
FontLoadHistograms: classify as memory cache hit if FontResource already loading

In cases where FontResource loading is already started by another
RemoteFontFaceSource instance, |m_loadStartTime| is not set and result in an
invalid load time record. Since we are collecting load times from the beginning
of the request, we classify this case as memory cache hit and don't record load
time.
==========

to

==========
FontLoadHistograms: classify as memory cache hit if FontResource loading not
triggered

In cases where FontResource loading is already started by another
RemoteFontFaceSource instance, |m_loadStartTime| is not set and result in
invalid load time records. Since we are collecting load times from the beginning
of the request, we classify this case as memory cache hit and don't record load
time.
==========

[Shao-Chuan Lee, 2016-09-29 09:01:36]

It looks okay to determine |m_dataSource| in longLimitExceeded() before
recordInterventionResult() instead of moving it away.

[Kunihiko Sakamoto, 2016-09-29 09:19:26]

lgtm % comment

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

https://codereview.chromium.org/2359493004/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/RemoteFontFaceSource.cpp:257:
maySetDataSource(isLoadStartedCalled() ? FromNetwork : FromMemoryCache);
Can you put this logic (if m_loadStartTime == 0 then FromMemoryCache) into
maySetDataSource()?

[Shao-Chuan Lee, 2016-09-29 09:31:43]

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

https://codereview.chromium.org/2359493004/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/RemoteFontFaceSource.cpp:257:
maySetDataSource(isLoadStartedCalled() ? FromNetwork : FromMemoryCache);
On 2016/09/29 09:19:26, Kunihiko Sakamoto wrote:
> Can you put this logic (if m_loadStartTime == 0 then FromMemoryCache) into
> maySetDataSource()?

Done.

[Shao-Chuan Lee, 2016-09-29 09:33:10]

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

[commit-bot: I haz the power, 2016-09-29 09:33:29]

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-09-29 10:55:38]

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

[commit-bot: I haz the power, 2016-09-29 10:55:39]

Dry run: This issue passed the CQ dry run.

[Shao-Chuan Lee, 2016-09-30 01:13:21]

The CQ bit was checked by shaochuan@chromium.org

[Shao-Chuan Lee, 2016-09-30 01:13:21]

The patchset sent to the CQ was uploaded after l-g-t-m from kouhei@chromium.org,
ksakamoto@chromium.org
Link to the patchset: https://codereview.chromium.org/2359493004/#ps100001
(title: "#20")

[commit-bot: I haz the power, 2016-09-30 01:13:49]

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

[kouhei (in TOK), 2016-09-30 01:18:35]

lgtm % comment

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

https://codereview.chromium.org/2359493004/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/css/RemoteFontFaceSource.cpp:282:
DCHECK(m_loadStartTime);
On 2016/09/23 01:47:19, Shao-Chuan Lee wrote:
> On 2016/09/23 01:33:59, kouhei wrote:
> > using DCHECK on double scares me a bit.
> > DCHECK_LT(m_loadStartTime, 0.0); perhaps?
> 
> Checking if non-zero should suffice here, maybe I should use
> DCHECK_NE(m_loadStartTime, 0) to be clear?
DCHECK_NE(m_loadStartTime, 0.0) is also OK.

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

https://codereview.chromium.org/2359493004/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/css/RemoteFontFaceSource.h:80: void
maySetDataSource(DataSource dataSource) { m_dataSource = (m_dataSource !=
FromUnknown) ? m_dataSource : (m_loadStartTime == 0) ? FromMemoryCache :
dataSource; }
Please implement this in .cpp file, not nesting ?: op.

[commit-bot: I haz the power, 2016-09-30 01:19:30]

Description was changed from

==========
FontLoadHistograms: classify as memory cache hit if FontResource loading not
triggered

In cases where FontResource loading is already started by another
RemoteFontFaceSource instance, |m_loadStartTime| is not set and result in
invalid load time records. Since we are collecting load times from the beginning
of the request, we classify this case as memory cache hit and don't record load
time.
==========

to

==========
FontLoadHistograms: classify as memory cache hit if FontResource loading not
triggered

In cases where FontResource loading is already started by another
RemoteFontFaceSource instance, |m_loadStartTime| is not set and result in
invalid load time records. Since we are collecting load times from the beginning
of the request, we classify this case as memory cache hit and don't record load
time.
==========

[commit-bot: I haz the power, 2016-09-30 01:19:31]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2016-09-30 01:23:04]

Description was changed from

==========
FontLoadHistograms: classify as memory cache hit if FontResource loading not
triggered

In cases where FontResource loading is already started by another
RemoteFontFaceSource instance, |m_loadStartTime| is not set and result in
invalid load time records. Since we are collecting load times from the beginning
of the request, we classify this case as memory cache hit and don't record load
time.
==========

to

==========
FontLoadHistograms: classify as memory cache hit if FontResource loading not
triggered

In cases where FontResource loading is already started by another
RemoteFontFaceSource instance, |m_loadStartTime| is not set and result in
invalid load time records. Since we are collecting load times from the beginning
of the request, we classify this case as memory cache hit and don't record load
time.

Committed: https://crrev.com/63b083a06f0b83985eb39d1bf7d9543d942c82c7
Cr-Commit-Position: refs/heads/master@{#422006}
==========

[commit-bot: I haz the power, 2016-09-30 01:23:05]

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