WebFonts: reset m_loadLimitState on memory cache revalidation

WebFonts: reset m_loadLimitState on memory cache revalidation

On reload, RevalidationPolicy Revalidate is applied.
This policy reuses the same Resource instance for the reloading
Resource, and FontResource::startLoadLimitTimersIfNeeded() is
called again.

FontResource does not expect that startLoadLimitTimersIfNeeded()
is called twice, and needs a place to reset an internal state
to manage font loading timeouts.

This patch changes Resource::setRevalidatingRequest to virtual
and reset m_loadLimitState at FontResource::setRevalidatingRequest.

BUG=603392
Committed: https://crrev.com/27d8b39e91bfc8ffd53c27cfb8dee3029b122d24
Cr-Commit-Position: refs/heads/master@{#389359}

[Takashi Toyoshima, 2016-04-18 05:19:35]

Description was changed from

==========
WebFotns: reset m_loadLimitState on each load

Resource::load() could be called again on reload, and we need to
reset m_loadLimitState to make it work with ASSERT in each
LimitCallbacks.

BUG=603392
==========

to

==========
WebFotns: reset m_loadLimitState on each load

Resource::load() could be called again on reload, and we need to
reset m_loadLimitState to make it work with ASSERT in each
LimitCallbacks.

BUG=603392
==========

[Takashi Toyoshima, 2016-04-18 05:19:35]

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

[Takashi Toyoshima, 2016-04-18 05:57:36]

Can you take a look?

[Takashi Toyoshima, 2016-04-18 11:13:15]

Description was changed from

==========
WebFotns: reset m_loadLimitState on each load

Resource::load() could be called again on reload, and we need to
reset m_loadLimitState to make it work with ASSERT in each
LimitCallbacks.

BUG=603392
==========

to

==========
WebFotns: reset m_loadLimitState on memory cache revalidation

On reload, RevalidationPolicy Revalidate is applied.
This policy reuses the same Resource instance for the reloading
Resource, and FontResource::startLoadLimitTimersIfNeeded() is
called again.

FontResource does not expect that startLoadLimitTimersIfNeeded()
is called twice, and needs a place to reset an internal state
to manage font loading timeouts.

This patch adds Resource::PrepareToRevalidate() callback and
calls it on starting memory cache revalidation.

BUG=603392
==========

[Takashi Toyoshima, 2016-04-18 11:13:15]

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

[tyoshino (SeeGerritForStatus), 2016-04-18 12:05:59]

Description/subject: WebFotns -> WebFonts

[Takashi Toyoshima, 2016-04-18 12:09:26]

Description was changed from

==========
WebFotns: reset m_loadLimitState on memory cache revalidation

On reload, RevalidationPolicy Revalidate is applied.
This policy reuses the same Resource instance for the reloading
Resource, and FontResource::startLoadLimitTimersIfNeeded() is
called again.

FontResource does not expect that startLoadLimitTimersIfNeeded()
is called twice, and needs a place to reset an internal state
to manage font loading timeouts.

This patch adds Resource::PrepareToRevalidate() callback and
calls it on starting memory cache revalidation.

BUG=603392
==========

to

==========
WebFonts: reset m_loadLimitState on memory cache revalidation

On reload, RevalidationPolicy Revalidate is applied.
This policy reuses the same Resource instance for the reloading
Resource, and FontResource::startLoadLimitTimersIfNeeded() is
called again.

FontResource does not expect that startLoadLimitTimersIfNeeded()
is called twice, and needs a place to reset an internal state
to manage font loading timeouts.

This patch adds Resource::PrepareToRevalidate() callback and
calls it on starting memory cache revalidation.

BUG=603392
==========

[Takashi Toyoshima, 2016-04-18 12:13:04]

Done for #6.
After #3, I had a offline chat with ksakamoto and hiroshige, and revised the CL
as PS2, but still I need to look the failed test.

[Takashi Toyoshima, 2016-04-18 15:43:33]

hiroshige: what do you think about this Resource related code change?

[Takashi Toyoshima, 2016-04-19 07:38:20]

ksakamoto: I added a layout test we talked in Patch Set 4. PTAL.

[hiroshige, 2016-04-19 07:51:50]

Basically looks good.

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

https://codereview.chromium.org/1893233002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/fetch/ResourceFetcher.cpp:545:
resource->prepareToRevalidate();
How about calling prepareToRevalidate() in setRevalidatingRequest()?
By doing so, Resource::setRevalidatingRequest() will be the single point of
modification to Resource for revalidation start.

[Kunihiko Sakamoto, 2016-04-19 07:55:36]

FontResource change and test lgtm

[Takashi Toyoshima, 2016-04-19 08:39:01]

PTAL

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

https://codereview.chromium.org/1893233002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/fetch/ResourceFetcher.cpp:545:
resource->prepareToRevalidate();
Thanks, that sounds smart, and I can just make the method virtual to initialize
font specific field.

[hiroshige, 2016-04-21 07:39:11]

https://codereview.chromium.org/1893233002/diff/80001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/http/tests/webfont/font-load-revalidation.html
(right):

https://codereview.chromium.org/1893233002/diff/80001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/http/tests/webfont/font-load-revalidation.html:9:
testRunner.waitUntilDone();
We can write |jsTestIsAsync = true;| instead of lines 8 and 9.

https://codereview.chromium.org/1893233002/diff/80001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/http/tests/webfont/font-load-revalidation.html:21:
testRunner.notifyDone();
We can write |finishJSTest();| instead of lines 20--21.

https://codereview.chromium.org/1893233002/diff/80001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/http/tests/webfont/resources/font-load-revalidation-frame.html
(right):

https://codereview.chromium.org/1893233002/diff/80001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/http/tests/webfont/resources/font-load-revalidation-frame.html:15:
parent.postMessage('pass:font.load()', '*');
We don't have lines |PASS font.load()| in -expected.txt. Is it OK?

[Takashi Toyoshima, 2016-04-21 08:31:25]

Thanks, I also update one more test to use jsTestIsAsync in the same directory.
Others in the directory still does not use it because they also do not use
js-test.js.

The latest patch set should work, but let me run a try.

https://codereview.chromium.org/1893233002/diff/80001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/http/tests/webfont/font-load-revalidation.html
(right):

https://codereview.chromium.org/1893233002/diff/80001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/http/tests/webfont/font-load-revalidation.html:9:
testRunner.waitUntilDone();
On 2016/04/21 07:39:10, hiroshige wrote:
> We can write |jsTestIsAsync = true;| instead of lines 8 and 9.

Done.

https://codereview.chromium.org/1893233002/diff/80001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/http/tests/webfont/font-load-revalidation.html:21:
testRunner.notifyDone();
On 2016/04/21 07:39:10, hiroshige wrote:
> We can write |finishJSTest();| instead of lines 20--21.

Done.

https://codereview.chromium.org/1893233002/diff/80001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/http/tests/webfont/resources/font-load-revalidation-frame.html
(right):

https://codereview.chromium.org/1893233002/diff/80001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/http/tests/webfont/resources/font-load-revalidation-frame.html:15:
parent.postMessage('pass:font.load()', '*');
Oops, good point. I forget to update the expectation after adding this line.

[hiroshige, 2016-04-21 08:39:43]

non-owner lgtm if bots are happy.

[hiroshige, 2016-04-21 08:41:08]

Please update the CL description: Resource::PrepareToRevalidate() no longer
exists in the latest PatchSet.

[Takashi Toyoshima, 2016-04-22 06:04:07]

Description was changed from

==========
WebFonts: reset m_loadLimitState on memory cache revalidation

On reload, RevalidationPolicy Revalidate is applied.
This policy reuses the same Resource instance for the reloading
Resource, and FontResource::startLoadLimitTimersIfNeeded() is
called again.

FontResource does not expect that startLoadLimitTimersIfNeeded()
is called twice, and needs a place to reset an internal state
to manage font loading timeouts.

This patch adds Resource::PrepareToRevalidate() callback and
calls it on starting memory cache revalidation.

BUG=603392
==========

to

==========
WebFonts: reset m_loadLimitState on memory cache revalidation

On reload, RevalidationPolicy Revalidate is applied.
This policy reuses the same Resource instance for the reloading
Resource, and FontResource::startLoadLimitTimersIfNeeded() is
called again.

FontResource does not expect that startLoadLimitTimersIfNeeded()
is called twice, and needs a place to reset an internal state
to manage font loading timeouts.

This patch changes Resource::setRevalidatingRequest to virtual
and reset m_loadLimitState at FontResource::setRevalidatingRequest.

BUG=603392
==========

[Takashi Toyoshima, 2016-04-22 06:04:07]

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

[Takashi Toyoshima, 2016-04-22 06:05:09]

description was updated to describe the latest patch correctly.
+Nate for fetch/loader owner review.

[Nate Chapin, 2016-04-22 20:56:22]

Unfortunate to have to virtual-ize yet another function on Resource, but this
makes sense. LGTM

[Takashi Toyoshima, 2016-04-23 00:59:43]

The CQ bit was checked by toyoshim@chromium.org

[Takashi Toyoshima, 2016-04-23 00:59:43]

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

[commit-bot: I haz the power, 2016-04-23 01:00:16]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1893233002/100001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1893233002/100001

[commit-bot: I haz the power, 2016-04-23 03:59:30]

Description was changed from

==========
WebFonts: reset m_loadLimitState on memory cache revalidation

On reload, RevalidationPolicy Revalidate is applied.
This policy reuses the same Resource instance for the reloading
Resource, and FontResource::startLoadLimitTimersIfNeeded() is
called again.

FontResource does not expect that startLoadLimitTimersIfNeeded()
is called twice, and needs a place to reset an internal state
to manage font loading timeouts.

This patch changes Resource::setRevalidatingRequest to virtual
and reset m_loadLimitState at FontResource::setRevalidatingRequest.

BUG=603392
==========

to

==========
WebFonts: reset m_loadLimitState on memory cache revalidation

On reload, RevalidationPolicy Revalidate is applied.
This policy reuses the same Resource instance for the reloading
Resource, and FontResource::startLoadLimitTimersIfNeeded() is
called again.

FontResource does not expect that startLoadLimitTimersIfNeeded()
is called twice, and needs a place to reset an internal state
to manage font loading timeouts.

This patch changes Resource::setRevalidatingRequest to virtual
and reset m_loadLimitState at FontResource::setRevalidatingRequest.

BUG=603392
==========

[commit-bot: I haz the power, 2016-04-23 03:59:31]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2016-04-23 04:00:48]

Description was changed from

==========
WebFonts: reset m_loadLimitState on memory cache revalidation

On reload, RevalidationPolicy Revalidate is applied.
This policy reuses the same Resource instance for the reloading
Resource, and FontResource::startLoadLimitTimersIfNeeded() is
called again.

FontResource does not expect that startLoadLimitTimersIfNeeded()
is called twice, and needs a place to reset an internal state
to manage font loading timeouts.

This patch changes Resource::setRevalidatingRequest to virtual
and reset m_loadLimitState at FontResource::setRevalidatingRequest.

BUG=603392
==========

to

==========
WebFonts: reset m_loadLimitState on memory cache revalidation

On reload, RevalidationPolicy Revalidate is applied.
This policy reuses the same Resource instance for the reloading
Resource, and FontResource::startLoadLimitTimersIfNeeded() is
called again.

FontResource does not expect that startLoadLimitTimersIfNeeded()
is called twice, and needs a place to reset an internal state
to manage font loading timeouts.

This patch changes Resource::setRevalidatingRequest to virtual
and reset m_loadLimitState at FontResource::setRevalidatingRequest.

BUG=603392

Committed: https://crrev.com/27d8b39e91bfc8ffd53c27cfb8dee3029b122d24
Cr-Commit-Position: refs/heads/master@{#389359}
==========

[commit-bot: I haz the power, 2016-04-23 04:00:49]

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