Make internals.isPreloaded() to remain the same before/after clearPreloads()

Make internals.isPreloaded() to remain the same before/after clearPreloads()

Previously, internals.isPreloaded() depends on |ResourceFetcher::m_preloads|,
which is cleared by ResourceFetcher::clearPreloads().
This caused internals.isPreloaded() to turn false after around document's
load event.

This CL makes ResourceFetcher to keep a set of preloaded URLs (separately
from |m_preloads|) if a blink::Internals object is created, and use the set
to calculate internals.isPreloaded().

BUG=643621
Committed: https://crrev.com/7978814bba57818814b7a10aa3366e16c6d33784
Cr-Commit-Position: refs/heads/master@{#418821}

[hiroshige, 2016-09-14 08:21:17]

Description was changed from

==========
Make Internals.isPreloaded() to return true even after clearPreloads()

BUG=643621
==========

to

==========
Make internals.isPreloaded() to remain the same before/after clearPreloads()

Previously, internals.isPreloaded() depends on |ResourceFetcher::m_preloads|,
which is cleared by ResourceFetcher::clearPreloads().
This caused internals.isPreloaded() to turn false after around document's
load event.

This CL makes ResourceFetcher to keep a set of preloaded URLs (separately
from |m_preloads|) if a blink::Internals object is created, and use the set
to calculate internals.isPreloaded().

BUG=643621
==========

[hiroshige, 2016-09-15 06:29:02]

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

[commit-bot: I haz the power, 2016-09-15 06:29:14]

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

[hiroshige, 2016-09-15 06:34:07]

hiroshige@chromium.org changed reviewers:
+ yoav@yoav.ws

[hiroshige, 2016-09-15 06:34:08]

yoav@, could you take a look?

[Yoav Weiss, 2016-09-15 06:51:25]

On 2016/09/15 06:34:08, hiroshige wrote:
> yoav@, could you take a look?

Looks great!

Can you add a test that makes sure Link header based preloads are also accounted
for?

[Yoav Weiss, 2016-09-15 06:57:03]

https://codereview.chromium.org/2332333003/diff/40001/third_party/WebKit/Layo...
File third_party/WebKit/LayoutTests/fast/preloader/is-preloaded-after-load.html
(right):

https://codereview.chromium.org/2332333003/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/fast/preloader/is-preloaded-after-load.html:27:
<script src=resources/non-existant.js></script>
Can you also test that non-existant.js isPreloaded()?

[commit-bot: I haz the power, 2016-09-15 07:36:10]

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

[commit-bot: I haz the power, 2016-09-15 07:36:11]

Dry run: This issue passed the CQ dry run.

[hiroshige, 2016-09-15 08:04:40]

> Can you add a test that makes sure Link header based preloads are also
accounted for?
Modified http/tests/linkHeader/link-preload-in-iframe.html so that it also tests
isPreloaded() on and after the load event.
(When preloaded by Link header, isPreloaded() returns true after the load event
even before this CL, because clearPreloads() doesn't clear linkPreloaded
Resources? though)

https://codereview.chromium.org/2332333003/diff/40001/third_party/WebKit/Layo...
File third_party/WebKit/LayoutTests/fast/preloader/is-preloaded-after-load.html
(right):

https://codereview.chromium.org/2332333003/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/fast/preloader/is-preloaded-after-load.html:27:
<script src=resources/non-existant.js></script>
On 2016/09/15 06:57:03, Yoav Weiss wrote:
> Can you also test that non-existant.js isPreloaded()?

Done.

[Yoav Weiss, 2016-09-15 08:34:32]

LGTM! :)

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

https://codereview.chromium.org/2332333003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/fetch/ResourceFetcher.cpp:888: for (auto resource
: *m_preloads)
nit: const auto&

(I changed all other m_preloads iterations to that in
https://codereview.chromium.org/2343623002/ )

[Yoav Weiss, 2016-09-15 08:35:57]

On 2016/09/15 08:04:40, hiroshige wrote:
> > Can you add a test that makes sure Link header based preloads are also
> accounted for?
> Modified http/tests/linkHeader/link-preload-in-iframe.html so that it also
tests
> isPreloaded() on and after the load event.
> (When preloaded by Link header, isPreloaded() returns true after the load
event
> even before this CL, because clearPreloads() doesn't clear linkPreloaded
> Resources? though)
> 

That should be fixed by changes in https://codereview.chromium.org/2343623002/

[hiroshige, 2016-09-15 08:39:12]

Thanks!

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

https://codereview.chromium.org/2332333003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/fetch/ResourceFetcher.cpp:888: for (auto resource
: *m_preloads)
On 2016/09/15 08:34:31, Yoav Weiss wrote:
> nit: const auto&
> 
> (I changed all other m_preloads iterations to that in
> https://codereview.chromium.org/2343623002/ )

Done.

[hiroshige, 2016-09-15 10:00:21]

The CQ bit was checked by hiroshige@chromium.org

[hiroshige, 2016-09-15 10:00:21]

The patchset sent to the CQ was uploaded after l-g-t-m from yoav@yoav.ws
Link to the patchset: https://codereview.chromium.org/2332333003/#ps80001
(title: "const auto&")

[commit-bot: I haz the power, 2016-09-15 10:00:48]

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-15 10:05:41]

Description was changed from

==========
Make internals.isPreloaded() to remain the same before/after clearPreloads()

Previously, internals.isPreloaded() depends on |ResourceFetcher::m_preloads|,
which is cleared by ResourceFetcher::clearPreloads().
This caused internals.isPreloaded() to turn false after around document's
load event.

This CL makes ResourceFetcher to keep a set of preloaded URLs (separately
from |m_preloads|) if a blink::Internals object is created, and use the set
to calculate internals.isPreloaded().

BUG=643621
==========

to

==========
Make internals.isPreloaded() to remain the same before/after clearPreloads()

Previously, internals.isPreloaded() depends on |ResourceFetcher::m_preloads|,
which is cleared by ResourceFetcher::clearPreloads().
This caused internals.isPreloaded() to turn false after around document's
load event.

This CL makes ResourceFetcher to keep a set of preloaded URLs (separately
from |m_preloads|) if a blink::Internals object is created, and use the set
to calculate internals.isPreloaded().

BUG=643621
==========

[commit-bot: I haz the power, 2016-09-15 10:05:43]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2016-09-15 10:07:41]

Description was changed from

==========
Make internals.isPreloaded() to remain the same before/after clearPreloads()

Previously, internals.isPreloaded() depends on |ResourceFetcher::m_preloads|,
which is cleared by ResourceFetcher::clearPreloads().
This caused internals.isPreloaded() to turn false after around document's
load event.

This CL makes ResourceFetcher to keep a set of preloaded URLs (separately
from |m_preloads|) if a blink::Internals object is created, and use the set
to calculate internals.isPreloaded().

BUG=643621
==========

to

==========
Make internals.isPreloaded() to remain the same before/after clearPreloads()

Previously, internals.isPreloaded() depends on |ResourceFetcher::m_preloads|,
which is cleared by ResourceFetcher::clearPreloads().
This caused internals.isPreloaded() to turn false after around document's
load event.

This CL makes ResourceFetcher to keep a set of preloaded URLs (separately
from |m_preloads|) if a blink::Internals object is created, and use the set
to calculate internals.isPreloaded().

BUG=643621

Committed: https://crrev.com/7978814bba57818814b7a10aa3366e16c6d33784
Cr-Commit-Position: refs/heads/master@{#418821}
==========

[commit-bot: I haz the power, 2016-09-15 10:07:42]

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

[kouhei (in TOK), 2016-09-15 12:20:50]

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

[kouhei (in TOK), 2016-09-15 12:20:51]

lgtm Thanks for the work!