[SVG 2/5] Do not reload FontResource created by resourceForStaticData()

Do not reload FontResource created by resourceForStaticData()

There are cases where FontResource::checkNotify() is called without calling
FontResource::load(), e.g. when FontResource is created by
ResourceFetcher::resourceForStaticData().
In such cases, loading is no longer needed, but FontResource::stillNeedsLoad()
remains true after checkNotify() and thus FontResource is reloaded.

This CL sets |FontResource::m_state| to LoadInitiated in checkNotify() and
thus prevents subsequent FontResource::load() calls.

This CL is [2/5] to load FontResource of data: URLs synchronously so that
all subresource loading of SVG image completes before loading of the SVG file:
[1/5] Make fonts with data: URLs to be served by resourceForStaticData().
      https://codereview.chromium.org/1700233003/
[2/5] Do not reload FontResource created by resourceForStaticData().
      https://codereview.chromium.org/1704693002/ (This CL)
[3/5] Make invalid data: URLs to be served by resourceForStaticData().
      https://codereview.chromium.org/1707013002/
[4/5] Add layout tests.
      https://codereview.chromium.org/1699323002/
[5/5] Check SVG's subresource load completion in ImageLoader::notifyFinished().
      https://codereview.chromium.org/1515963005/

BUG=382170

[hiroshige, 2016-02-16 22:12:57]

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

[commit-bot: I haz the power, 2016-02-16 22:16:34]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1704693002/1
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1704693002/1

[commit-bot: I haz the power, 2016-02-16 23:28:43]

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

[commit-bot: I haz the power, 2016-02-16 23:28:47]

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

[hiroshige, 2016-02-17 00:06:48]

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

[commit-bot: I haz the power, 2016-02-17 00:09:28]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1704693002/20001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1704693002/20001

[commit-bot: I haz the power, 2016-02-17 01:31:48]

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

[commit-bot: I haz the power, 2016-02-17 01:31:49]

Dry run: This issue passed the CQ dry run.

[hiroshige, 2016-02-17 18:43:15]

Description was changed from

==========
Do not load FontResource when checkNotify() is already called

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

to

==========
Do not call Resource::load() for precached FontResource

This CL is [1/2] to load FontResource of data: URLs synchronously so that
all subresource loading of SVG image completes before loading of the SVG file.

There are cases where FontResource::checkNotify() is called without calling
FontResource::load(), e.g. when FontResource is created by
ResourceFetcher::preCacheData().
In such cases, loading is no longer needed, but FontResource::stillNeedsLoad()
remains true after checkNotify().
This causes FontResource of a data: URL to be always loaded by WebURLLoader.

This CL sets |FontResource::m_state| in checkNotify() and prevents subsequent
FontResource::load() calls.

[1/2] Make data: URLs with font/ MIME types to be precached:
      https://codereview.chromium.org/1700233003/
[2/2] Do not call Resource::load() for precached FontResource: This CL.

BUG=382170
==========

[hiroshige, 2016-02-17 18:43:15]

hiroshige@chromium.org changed reviewers:
+ japhet@chromium.org, kouhei@chromium.org

[hiroshige, 2016-02-17 18:44:02]

Description was changed from

==========
Do not call Resource::load() for precached FontResource

This CL is [1/2] to load FontResource of data: URLs synchronously so that
all subresource loading of SVG image completes before loading of the SVG file.

There are cases where FontResource::checkNotify() is called without calling
FontResource::load(), e.g. when FontResource is created by
ResourceFetcher::preCacheData().
In such cases, loading is no longer needed, but FontResource::stillNeedsLoad()
remains true after checkNotify().
This causes FontResource of a data: URL to be always loaded by WebURLLoader.

This CL sets |FontResource::m_state| in checkNotify() and prevents subsequent
FontResource::load() calls.

[1/2] Make data: URLs with font/ MIME types to be precached:
      https://codereview.chromium.org/1700233003/
[2/2] Do not call Resource::load() for precached FontResource: This CL.

BUG=382170
==========

to

==========
Do not call Resource::load() for precached FontResource

This CL is [2/2] to load FontResource of data: URLs synchronously so that
all subresource loading of SVG image completes before loading of the SVG file.

There are cases where FontResource::checkNotify() is called without calling
FontResource::load(), e.g. when FontResource is created by
ResourceFetcher::preCacheData().
In such cases, loading is no longer needed, but FontResource::stillNeedsLoad()
remains true after checkNotify().
This causes FontResource of a data: URL to be always loaded by WebURLLoader.

This CL sets |FontResource::m_state| in checkNotify() and prevents subsequent
FontResource::load() calls.

[1/2] Make data: URLs with font/ MIME types to be precached:
      https://codereview.chromium.org/1700233003/
[2/2] Do not call Resource::load() for precached FontResource: This CL.

BUG=382170
==========

[hiroshige, 2016-02-17 19:17:23]

Description was changed from

==========
Do not call Resource::load() for precached FontResource

This CL is [2/2] to load FontResource of data: URLs synchronously so that
all subresource loading of SVG image completes before loading of the SVG file.

There are cases where FontResource::checkNotify() is called without calling
FontResource::load(), e.g. when FontResource is created by
ResourceFetcher::preCacheData().
In such cases, loading is no longer needed, but FontResource::stillNeedsLoad()
remains true after checkNotify().
This causes FontResource of a data: URL to be always loaded by WebURLLoader.

This CL sets |FontResource::m_state| in checkNotify() and prevents subsequent
FontResource::load() calls.

[1/2] Make data: URLs with font/ MIME types to be precached:
      https://codereview.chromium.org/1700233003/
[2/2] Do not call Resource::load() for precached FontResource: This CL.

BUG=382170
==========

to

==========
Do not call Resource::load() for precached FontResource

This CL is [2/2] to load FontResource of data: URLs synchronously so that
all subresource loading of SVG image completes before loading of the SVG file.

There are cases where FontResource::checkNotify() is called without calling
FontResource::load(), e.g. when FontResource is created by
ResourceFetcher::preCacheData().
In such cases, loading is no longer needed, but FontResource::stillNeedsLoad()
remains true after checkNotify().
This causes FontResource of a data: URL to be always loaded by WebURLLoader.

This CL sets |FontResource::m_state| in checkNotify() and prevents subsequent
FontResource::load() calls.

[1/2] Make data: URLs with font/ MIME types to be precached:
      https://codereview.chromium.org/1700233003/
[2/2] Do not call Resource::load() for precached FontResource: This CL.

BUG=382170

(R=kouhei, japhet)
==========

[hiroshige, 2016-02-17 19:17:23]

hiroshige@chromium.org changed reviewers:
- japhet@chromium.org, kouhei@chromium.org

[hiroshige, 2016-02-29 19:46:25]

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

[commit-bot: I haz the power, 2016-02-29 19:46:40]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1704693002/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1704693002/40001

[commit-bot: I haz the power, 2016-02-29 20:23:40]

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

[commit-bot: I haz the power, 2016-02-29 20:23:41]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[hiroshige, 2016-02-29 21:53:24]

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

[commit-bot: I haz the power, 2016-02-29 21:53:52]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1704693002/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1704693002/60001

[commit-bot: I haz the power, 2016-02-29 23:59:24]

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

[commit-bot: I haz the power, 2016-02-29 23:59:24]

Dry run: This issue passed the CQ dry run.

[hiroshige, 2016-03-01 18:24:10]

Description was changed from

==========
Do not call Resource::load() for precached FontResource

This CL is [2/2] to load FontResource of data: URLs synchronously so that
all subresource loading of SVG image completes before loading of the SVG file.

There are cases where FontResource::checkNotify() is called without calling
FontResource::load(), e.g. when FontResource is created by
ResourceFetcher::preCacheData().
In such cases, loading is no longer needed, but FontResource::stillNeedsLoad()
remains true after checkNotify().
This causes FontResource of a data: URL to be always loaded by WebURLLoader.

This CL sets |FontResource::m_state| in checkNotify() and prevents subsequent
FontResource::load() calls.

[1/2] Make data: URLs with font/ MIME types to be precached:
      https://codereview.chromium.org/1700233003/
[2/2] Do not call Resource::load() for precached FontResource: This CL.

BUG=382170

(R=kouhei, japhet)
==========

to

==========
Do not reload FontResource created by resourceForStaticData()

There are cases where FontResource::checkNotify() is called without calling
FontResource::load(), e.g. when FontResource is created by
ResourceFetcher::resourceForStaticData().
In such cases, loading is no longer needed, but FontResource::stillNeedsLoad()
remains true after checkNotify() and thus FontResource is reloaded.

This CL sets |FontResource::m_state| to LoadInitiated in checkNotify() and
thus prevents subsequent FontResource::load() calls.

This CL is [2/5] to load FontResource of data: URLs synchronously so that
all subresource loading of SVG image completes before loading of the SVG file:
[1/5] Make fonts with data: URLs to be served by resourceForStaticData().
      https://codereview.chromium.org/1700233003/
[2/5] Do not reload FontResource created by resourceForStaticData().
      https://codereview.chromium.org/1704693002/ (This CL)
[3/5] Make invalid data: URLs to be served by resourceForStaticData().
      https://codereview.chromium.org/1707013002/
[4/5] Add layout tests.
      https://codereview.chromium.org/1699323002/
[5/5] Check SVG's subresource load completion in ImageLoader::notifyFinished().
      https://codereview.chromium.org/1515963005/

BUG=382170
==========

[hiroshige, 2016-03-01 18:46:20]

hiroshige@chromium.org changed reviewers:
+ japhet@chromium.org

[hiroshige, 2016-03-01 18:46:21]

PTAL.

[Nate Chapin, 2016-03-01 19:17:29]

lgtm

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

https://codereview.chromium.org/1704693002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/fetch/FontResource.cpp:198: // example when the
FontResource is created by
"for example" doesn't seem right here, as this should be the only case where
this is true.

https://codereview.chromium.org/1704693002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/fetch/FontResource.cpp:199: //
ResourceFetcher::preCacheData(), we no longer need loading, so we set
Update preCacheData() to the new name.

https://codereview.chromium.org/1704693002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/fetch/FontResource.cpp:202: m_state =
LoadInitiated;
Doing this work here is kind of hacky. Is there anywhere else we can do it?
Should we at least note that this is kind of hacky? :)

[hiroshige, 2016-03-01 20:24:29]

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

[commit-bot: I haz the power, 2016-03-01 20:24:52]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1704693002/80001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1704693002/80001

[commit-bot: I haz the power, 2016-03-01 21:34:08]

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

[commit-bot: I haz the power, 2016-03-01 21:34:09]

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

[hiroshige, 2016-03-01 22:11:51]

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

[hiroshige, 2016-03-01 22:11:52]

+ksakamoto@, could you take another look at this change to state management in
FontResource?

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

https://codereview.chromium.org/1704693002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/fetch/FontResource.cpp:198: // example when the
FontResource is created by
On 2016/03/01 19:17:28, Nate Chapin wrote:
> "for example" doesn't seem right here, as this should be the only case where
> this is true.

Done.

https://codereview.chromium.org/1704693002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/fetch/FontResource.cpp:199: //
ResourceFetcher::preCacheData(), we no longer need loading, so we set
On 2016/03/01 19:17:28, Nate Chapin wrote:
> Update preCacheData() to the new name.

Done.

https://codereview.chromium.org/1704693002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/fetch/FontResource.cpp:202: m_state =
LoadInitiated;
On 2016/03/01 19:17:28, Nate Chapin wrote:
> Doing this work here is kind of hacky. Is there anywhere else we can do it?
> Should we at least note that this is kind of hacky? :)

Not sure > anywhere else.
ksakamoto@, do you have any thoughts?

[Kunihiko Sakamoto, 2016-03-02 03:17:49]

Hmm, so FontResource::checkNotify() can be called even if beginLoadIfNeeded()
isn't called...
Wouldn't this break the laziness of WebFont loading?

<!doctype html>
<style>
@font-face {
   font-family: Unused;
   src:url("data:font/ttf,");
}
</style>
// The data URL shouldn't be fetched

[hiroshige, 2016-03-02 06:24:39]

On 2016/03/02 03:17:49, Kunihiko Sakamoto wrote:
> Hmm, so FontResource::checkNotify() can be called even if beginLoadIfNeeded()
> isn't called...
> Wouldn't this break the laziness of WebFont loading?
> 
> <!doctype html>
> <style>
> @font-face {
>    font-family: Unused;
>    src:url("data:font/ttf,");
> }
> </style>
> // The data URL shouldn't be fetched

Current implementation:
  data URL is parsed and converted to WebData
  when ResourceFetcher::requestResource() is called (i.e. around when
FontResource::load() is called).

After [1/5] https://codereview.chromium.org/1700233003/:
  data URL is parsed and converted to WebData, and Resource::finish() etc. are
called
  when ResourceFetcher::requestResource() is called.
  (See ResourceFetcher::resourceForStaticData() around
  
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...
  )

Should we also delay some (or all) of these to the timing when
beginLoadIfNeeded()/Resource::load() is actually called?

[Kunihiko Sakamoto, 2016-03-02 09:54:37]

On 2016/03/02 06:24:39, hiroshige wrote:
> On 2016/03/02 03:17:49, Kunihiko Sakamoto wrote:
> > Hmm, so FontResource::checkNotify() can be called even if
beginLoadIfNeeded()
> > isn't called...
> > Wouldn't this break the laziness of WebFont loading?
> > 
> > <!doctype html>
> > <style>
> > @font-face {
> >    font-family: Unused;
> >    src:url("data:font/ttf,");
> > }
> > </style>
> > // The data URL shouldn't be fetched
> 
> Current implementation:
>   data URL is parsed and converted to WebData
>   when ResourceFetcher::requestResource() is called (i.e. around when
> FontResource::load() is called).
> 
> After [1/5] https://codereview.chromium.org/1700233003/:
>   data URL is parsed and converted to WebData, and Resource::finish() etc. are
> called
>   when ResourceFetcher::requestResource() is called.
>   (See ResourceFetcher::resourceForStaticData() around
>   
>
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...
>   )
> 
> Should we also delay some (or all) of these to the timing when
> beginLoadIfNeeded()/Resource::load() is actually called?

FontResource::checkNotify() calls RemoteFontFaceSource::fontLoaded(), which
starts decoding the font. At least we should delay that until the font is
actually requested (i.e. beginLoadIfNeeded() called).

Also, I see unused webfonts with data URL appear on devtools network tab,
locally applying [1/5] and [2/5]. I'm not sure what's triggering this.

[hiroshige, 2016-03-03 01:57:59]

On 2016/03/02 09:54:37, Kunihiko Sakamoto wrote:
> On 2016/03/02 06:24:39, hiroshige wrote:
> > On 2016/03/02 03:17:49, Kunihiko Sakamoto wrote:
> > > Hmm, so FontResource::checkNotify() can be called even if
> beginLoadIfNeeded()
> > > isn't called...
> > > Wouldn't this break the laziness of WebFont loading?
> > > 
> > > <!doctype html>
> > > <style>
> > > @font-face {
> > >    font-family: Unused;
> > >    src:url("data:font/ttf,");
> > > }
> > > </style>
> > > // The data URL shouldn't be fetched
> > 
> > Current implementation:
> >   data URL is parsed and converted to WebData
> >   when ResourceFetcher::requestResource() is called (i.e. around when
> > FontResource::load() is called).
> > 
> > After [1/5] https://codereview.chromium.org/1700233003/:
> >   data URL is parsed and converted to WebData, and Resource::finish() etc.
are
> > called
> >   when ResourceFetcher::requestResource() is called.
> >   (See ResourceFetcher::resourceForStaticData() around
> >   
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...
> >   )
> > 
> > Should we also delay some (or all) of these to the timing when
> > beginLoadIfNeeded()/Resource::load() is actually called?
> 
> FontResource::checkNotify() calls RemoteFontFaceSource::fontLoaded(), which
> starts decoding the font. At least we should delay that until the font is
> actually requested (i.e. beginLoadIfNeeded() called).
> 
> Also, I see unused webfonts with data URL appear on devtools network tab,
> locally applying [1/5] and [2/5]. I'm not sure what's triggering this.

Thanks. I'm rewriting CLs to defer FontResource::checkNotify() calls.
I'll restart reviews when ready. Probably this CL will be unnecessary and
closed.

[Nate Chapin, 2016-03-23 18:04:26]

On 2016/03/03 01:57:59, hiroshige wrote:
> On 2016/03/02 09:54:37, Kunihiko Sakamoto wrote:
> > On 2016/03/02 06:24:39, hiroshige wrote:
> > > On 2016/03/02 03:17:49, Kunihiko Sakamoto wrote:
> > > > Hmm, so FontResource::checkNotify() can be called even if
> > beginLoadIfNeeded()
> > > > isn't called...
> > > > Wouldn't this break the laziness of WebFont loading?
> > > > 
> > > > <!doctype html>
> > > > <style>
> > > > @font-face {
> > > >    font-family: Unused;
> > > >    src:url("data:font/ttf,");
> > > > }
> > > > </style>
> > > > // The data URL shouldn't be fetched
> > > 
> > > Current implementation:
> > >   data URL is parsed and converted to WebData
> > >   when ResourceFetcher::requestResource() is called (i.e. around when
> > > FontResource::load() is called).
> > > 
> > > After [1/5] https://codereview.chromium.org/1700233003/:
> > >   data URL is parsed and converted to WebData, and Resource::finish() etc.
> are
> > > called
> > >   when ResourceFetcher::requestResource() is called.
> > >   (See ResourceFetcher::resourceForStaticData() around
> > >   
> > >
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...
> > >   )
> > > 
> > > Should we also delay some (or all) of these to the timing when
> > > beginLoadIfNeeded()/Resource::load() is actually called?
> > 
> > FontResource::checkNotify() calls RemoteFontFaceSource::fontLoaded(), which
> > starts decoding the font. At least we should delay that until the font is
> > actually requested (i.e. beginLoadIfNeeded() called).
> > 
> > Also, I see unused webfonts with data URL appear on devtools network tab,
> > locally applying [1/5] and [2/5]. I'm not sure what's triggering this.
> 
> Thanks. I'm rewriting CLs to defer FontResource::checkNotify() calls.
> I'll restart reviews when ready. Probably this CL will be unnecessary and
> closed.

BTW, I think this will be fixed by https://codereview.chromium.org/1802123002/,
which unifies FontResource's definition of needing load with Resource's
definition.