Turn off isLinkPreload() when used

Turn off isLinkPreload() when used

Make sure that isLinkPreload flag is turned off when the resource is used,
making sure that it can be cleared earlier.

BUG=635483
Committed: https://crrev.com/41924d42a6d0ebdd5e2aebc4a5dcca2d5bc11eb6
Cr-Commit-Position: refs/heads/master@{#419283}

[Yoav Weiss, 2016-09-07 08:03:45]

The CQ bit was checked by yoav@yoav.ws to run a CQ dry run

[commit-bot: I haz the power, 2016-09-07 08:03:56]

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

[Yoav Weiss, 2016-09-07 08:09:54]

Description was changed from

==========
Make Resource::isPreloaded() boolean based

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

to

==========
Make Resource::isPreloaded() boolean based

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

[commit-bot: I haz the power, 2016-09-07 08:17:31]

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

[commit-bot: I haz the power, 2016-09-07 08:17:32]

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

[Yoav Weiss, 2016-09-07 08:22:49]

The CQ bit was checked by yoav@yoav.ws to run a CQ dry run

[commit-bot: I haz the power, 2016-09-07 08:23:01]

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-07 10:32:53]

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

[commit-bot: I haz the power, 2016-09-07 10:32:54]

Dry run: This issue passed the CQ dry run.

[Yoav Weiss, 2016-09-07 11:16:51]

Description was changed from

==========
Make Resource::isPreloaded() boolean based

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

to

==========
Make Resource::isPreloaded() boolean based

As currently implemented, isPreloaded() is based on a counter, which increases
when preloadStarted runs and decreases when clearPreloads is run. 
However, for link preload based resources, I suspect that counter may never get
turned off, as they get preloaded several times (preloadScanner for link
preload, then link preload), so the counter gets incremented multiple times, but
gets decremented once.

I suspect that causes link preload based resources to not be removed in
pruneDeadResources(), so this issue might be related to memory issues we're
seeing in https://crbug.com/635483

BUG=635483
==========

[Yoav Weiss, 2016-09-07 11:16:51]

yoav@yoav.ws changed reviewers:
+ csharrison@chromium.org, japhet@chromium.org

[Yoav Weiss, 2016-09-07 11:18:42]

Hey Nate and Charlie,

Can you take a look and tell me if I'm horribly wrong about isPreloaded() and it
should stay a boolean? :)

[Yoav Weiss, 2016-09-07 11:25:07]

On 2016/09/07 11:18:42, Yoav Weiss wrote:
> Hey Nate and Charlie,
> 
> Can you take a look and tell me if I'm horribly wrong about isPreloaded() and
it
> should stay a boolean? :)

that is, should stay a counter...

[Charlie Harrison, 2016-09-07 15:08:20]

This l-g-t-m but I'll defer to Nate for final word.

If this actually solves the memory issue you reference it would be nice to add a
test if possible :)

[Yoav Weiss, 2016-09-07 15:13:48]

On 2016/09/07 15:08:20, Charlie Harrison wrote:
> This l-g-t-m but I'll defer to Nate for final word.
> 
> If this actually solves the memory issue you reference it would be nice to add
a
> test if possible :)

I thought that stopping the regression on the mem benchmarks might be test
enough, but I can try to add a unit test inspecting isPreloaded()

[Charlie Harrison, 2016-09-07 15:17:58]

Thanks. Perf dashboard is good but not as good as unit tests which can prevent a
regression from landing in the first place :)

[Nate Chapin, 2016-09-07 15:56:42]

On 2016/09/07 15:13:48, Yoav Weiss wrote:
> On 2016/09/07 15:08:20, Charlie Harrison wrote:
> > This l-g-t-m but I'll defer to Nate for final word.
> > 
> > If this actually solves the memory issue you reference it would be nice to
add
> a
> > test if possible :)
> 
> I thought that stopping the regression on the mem benchmarks might be test
> enough, but I can try to add a unit test inspecting isPreloaded()

Would you mind testing how this works if multiple ResourceFetchers try to
preload the same resource? That's the one place I can think of where this might
cause isPreloaded() might now go false too soon.

[Yoav Weiss, 2016-09-08 07:52:27]

The CQ bit was checked by yoav@yoav.ws to run a CQ dry run

[commit-bot: I haz the power, 2016-09-08 07:52:46]

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

[Yoav Weiss, 2016-09-08 07:55:59]

Description was changed from

==========
Make Resource::isPreloaded() boolean based

As currently implemented, isPreloaded() is based on a counter, which increases
when preloadStarted runs and decreases when clearPreloads is run. 
However, for link preload based resources, I suspect that counter may never get
turned off, as they get preloaded several times (preloadScanner for link
preload, then link preload), so the counter gets incremented multiple times, but
gets decremented once.

I suspect that causes link preload based resources to not be removed in
pruneDeadResources(), so this issue might be related to memory issues we're
seeing in https://crbug.com/635483

BUG=635483
==========

to

==========
Turn off isLinkPreload() when used and make isPreloaded() boolean

Make sure that isLinkPreload flag is turned off when the resource is used,
making sure that it can be cleared earlier.
Also, as currently implemented, isPreloaded() is based on a counter, which
increases when preloadStarted runs and decreases when clearPreloads is run. 
However, for link preload based resources, I suspect that counter may never get
turned off, as they get preloaded several times (preloadScanner for link
preload, then link preload), so the counter gets incremented multiple times, but
gets decremented once.

I suspect that causes link preload based resources to not be removed in
pruneDeadResources().

BUG=635483
==========

[Yoav Weiss, 2016-09-08 07:58:52]

On 2016/09/07 15:56:42, Nate Chapin wrote:
> On 2016/09/07 15:13:48, Yoav Weiss wrote:
> > On 2016/09/07 15:08:20, Charlie Harrison wrote:
> > > This l-g-t-m but I'll defer to Nate for final word.
> > > 
> > > If this actually solves the memory issue you reference it would be nice to
> add
> > a
> > > test if possible :)
> > 
> > I thought that stopping the regression on the mem benchmarks might be test
> > enough, but I can try to add a unit test inspecting isPreloaded()
> 
> Would you mind testing how this works if multiple ResourceFetchers try to
> preload the same resource? That's the one place I can think of where this
might
> cause isPreloaded() might now go false too soon.

Added tests and fixed another issue found along the way, where the
isLinkPreloaded flag wasn't turned off even after the discovery of the element
that was using it. (which meant that the preload couldn't be cleared, unlike
speculative preloads)

[commit-bot: I haz the power, 2016-09-08 08:40:44]

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

[commit-bot: I haz the power, 2016-09-08 08:40:45]

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

[Nate Chapin, 2016-09-08 17:26:47]

Thanks for adding for tests, but you didn't test the case I was concerned about:
the only situation in which counter vs bool differs is when multiple different
ResourceFetchers try to preload the same Resource. How should we handle that
situation? I could see the argument that a Resource should cease to be marked
preloaded as soon as it gets Used, but marking it as not preloaded when one of
many Documents no longer cares about it doesn't seem right.

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

https://codereview.chromium.org/2319483002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/fetch/ResourceFetcher.cpp:477:
resource->setLinkPreload(request.isLinkPreload());
Doesn't this have the potential to mark something as a preload that wasn't
previously?

[Yoav Weiss, 2016-09-12 07:33:30]

The CQ bit was checked by yoav@yoav.ws to run a CQ dry run

[commit-bot: I haz the power, 2016-09-12 07:33:40]

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

[Yoav Weiss, 2016-09-12 07:56:41]

> Thanks for adding for tests, but you didn't test the case I was concerned
about:
> the only situation in which counter vs bool differs is when multiple different
> ResourceFetchers try to preload the same Resource. How should we handle that
> situation? I could see the argument that a Resource should cease to be marked
> preloaded as soon as it gets Used, but marking it as not preloaded when one of
> many Documents no longer cares about it doesn't seem right.

Looking at isPreloaded() uses, the only case that seems problematic on that
front is when it's used for early bailout on detemineRevalidationPolicy() - here
there can be a scenario where we turn off isPreloaded after one document DCLed,
and the second ResourceFetcher will end up redownloading the resource.

Otherwise uses seem safe on that front:
* In MemoryCache it's used in order to figure out that resources that finished
loading but don't yet have an observer should not be pruned. If one fetcher is
using that resource, it would be considered "alive", right?
* In Resource.cpp it's used to remove only non-preloaded resources that errored
from the MemoryCache (unless the error was a cancellation). I don't get why this
is necessary.
* in canRequest it's used to suppress reporting on preload requests.

I'll see if it's possible to keep the counter while avoiding the current
situation where the counter can be incremented multiple times for a single
fetcher.

> 
>
https://codereview.chromium.org/2319483002/diff/40001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/core/fetch/ResourceFetcher.cpp (right):
> 
>
https://codereview.chromium.org/2319483002/diff/40001/third_party/WebKit/Sour...
> third_party/WebKit/Source/core/fetch/ResourceFetcher.cpp:477:
> resource->setLinkPreload(request.isLinkPreload());
> Doesn't this have the potential to mark something as a preload that wasn't
> previously?

Modified to avoid that

[Yoav Weiss, 2016-09-12 08:07:04]

The CQ bit was checked by yoav@yoav.ws to run a CQ dry run

[commit-bot: I haz the power, 2016-09-12 08:07:23]

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

[Yoav Weiss, 2016-09-12 08:10:19]

The CQ bit was checked by yoav@yoav.ws to run a CQ dry run

[commit-bot: I haz the power, 2016-09-12 08:10:25]

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

[Yoav Weiss, 2016-09-12 08:47:11]

On 2016/09/12 07:56:41, Yoav Weiss wrote:
> > Thanks for adding for tests, but you didn't test the case I was concerned
> about:
> > the only situation in which counter vs bool differs is when multiple
different
> > ResourceFetchers try to preload the same Resource. How should we handle that
> > situation? I could see the argument that a Resource should cease to be
marked
> > preloaded as soon as it gets Used, but marking it as not preloaded when one
of
> > many Documents no longer cares about it doesn't seem right.
> 
> Looking at isPreloaded() uses, the only case that seems problematic on that
> front is when it's used for early bailout on detemineRevalidationPolicy() -
here
> there can be a scenario where we turn off isPreloaded after one document
DCLed,
> and the second ResourceFetcher will end up redownloading the resource.
> 
> Otherwise uses seem safe on that front:
> * In MemoryCache it's used in order to figure out that resources that finished
> loading but don't yet have an observer should not be pruned. If one fetcher is
> using that resource, it would be considered "alive", right?
> * In Resource.cpp it's used to remove only non-preloaded resources that
errored
> from the MemoryCache (unless the error was a cancellation). I don't get why
this
> is necessary.
> * in canRequest it's used to suppress reporting on preload requests.
> 
> I'll see if it's possible to keep the counter while avoiding the current
> situation where the counter can be incremented multiple times for a single
> fetcher.
> 

Changed to a new approach that keeps the counter, but only increments it when
ResourceFetcher's m_preloads doesn't contain the resource already. Let me know
what you think.

[commit-bot: I haz the power, 2016-09-12 09:24:37]

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

[commit-bot: I haz the power, 2016-09-12 09:24:38]

Dry run: This issue passed the CQ dry run.

[Nate Chapin, 2016-09-12 20:44:51]

https://codereview.chromium.org/2319483002/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/fetch/ResourceFetcher.cpp (right):

https://codereview.chromium.org/2319483002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/fetch/ResourceFetcher.cpp:876: if
(m_preloads->add(resource).isNewEntry)
Why doesn't the m_preloads->contains(resource) above catch the case where
isNewEntry would be false?

[Yoav Weiss, 2016-09-12 21:01:17]

https://codereview.chromium.org/2319483002/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/fetch/ResourceFetcher.cpp (right):

https://codereview.chromium.org/2319483002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/fetch/ResourceFetcher.cpp:876: if
(m_preloads->add(resource).isNewEntry)
On 2016/09/12 20:44:51, Nate Chapin wrote:
> Why doesn't the m_preloads->contains(resource) above catch the case where
> isNewEntry would be false?

You're right. It does.

[Yoav Weiss, 2016-09-12 21:02:34]

Description was changed from

==========
Turn off isLinkPreload() when used and make isPreloaded() boolean

Make sure that isLinkPreload flag is turned off when the resource is used,
making sure that it can be cleared earlier.
Also, as currently implemented, isPreloaded() is based on a counter, which
increases when preloadStarted runs and decreases when clearPreloads is run. 
However, for link preload based resources, I suspect that counter may never get
turned off, as they get preloaded several times (preloadScanner for link
preload, then link preload), so the counter gets incremented multiple times, but
gets decremented once.

I suspect that causes link preload based resources to not be removed in
pruneDeadResources().

BUG=635483
==========

to

==========
Turn off isLinkPreload() when used

Make sure that isLinkPreload flag is turned off when the resource is used,
making sure that it can be cleared earlier.

BUG=635483
==========

[Yoav Weiss, 2016-09-12 21:04:40]

The CQ bit was checked by yoav@yoav.ws to run a CQ dry run

[commit-bot: I haz the power, 2016-09-12 21:05:11]

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

[Charlie Harrison, 2016-09-12 21:32:31]

A few points:
- Can you update the title description (and wrap to 80 cols)?
- Can you document these methods a little bit so the next person who wants to
remove the counter will know why its there?

https://codereview.chromium.org/2319483002/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/fetch/ResourceFetcherTest.cpp (right):

https://codereview.chromium.org/2319483002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/fetch/ResourceFetcherTest.cpp:583:
EXPECT_FALSE(memoryCache()->contains(resource));
Why is the resource evicted when it was used by a non-preload?

[Nate Chapin, 2016-09-12 21:44:52]

On 2016/09/12 07:56:41, Yoav Weiss wrote:
> * In Resource.cpp it's used to remove only non-preloaded resources that
errored
> from the MemoryCache (unless the error was a cancellation). I don't get why
this
> is necessary.

I didn't write that code, but I think the rationale is that most of the time we
don't want to leave failed loads in the cache, so for non-preloads we just evict
on failures. However, we also don't want to do 2 failed loads in the
preloaded-then-real-load case, so preload errors get to live on in the cache.
The exception is when the preload gets cancelled, since that should be a strong
signal that we don't care about the result of the preload and are never going to
use it.

[Yoav Weiss, 2016-09-12 21:48:13]

On 2016/09/12 21:44:52, Nate Chapin wrote:
> On 2016/09/12 07:56:41, Yoav Weiss wrote:
> > * In Resource.cpp it's used to remove only non-preloaded resources that
> errored
> > from the MemoryCache (unless the error was a cancellation). I don't get why
> this
> > is necessary.
> 
> I didn't write that code, but I think the rationale is that most of the time
we
> don't want to leave failed loads in the cache, so for non-preloads we just
evict
> on failures. However, we also don't want to do 2 failed loads in the
> preloaded-then-real-load case, so preload errors get to live on in the cache.
> The exception is when the preload gets cancelled, since that should be a
strong
> signal that we don't care about the result of the preload and are never going
to
> use it.

makes sense. Thanks!

[Yoav Weiss, 2016-09-12 21:48:27]

Description was changed from

==========
Turn off isLinkPreload() when used

Make sure that isLinkPreload flag is turned off when the resource is used,
making sure that it can be cleared earlier.

BUG=635483
==========

to

==========
Turn off isLinkPreload() when used

Make sure that isLinkPreload flag is turned off when the resource is used,
making sure that it can be cleared earlier.

BUG=635483
==========

[Yoav Weiss, 2016-09-12 21:51:09]

https://codereview.chromium.org/2319483002/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/fetch/ResourceFetcherTest.cpp (right):

https://codereview.chromium.org/2319483002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/fetch/ResourceFetcherTest.cpp:583:
EXPECT_FALSE(memoryCache()->contains(resource));
On 2016/09/12 21:32:31, Charlie Harrison wrote:
> Why is the resource evicted when it was used by a non-preload?

Good catch! The resource is evicted since the resource never changed its
PreloadResult from PreloadNotReferenced, since it never had a client/observer
added to it.

I should change the test to add one and make sure it is not removed.

[commit-bot: I haz the power, 2016-09-12 22:24:57]

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

[commit-bot: I haz the power, 2016-09-12 22:24:58]

Dry run: This issue passed the CQ dry run.

[Yoav Weiss, 2016-09-13 09:27:39]

The CQ bit was checked by yoav@yoav.ws to run a CQ dry run

[commit-bot: I haz the power, 2016-09-13 09:27:56]

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

[Yoav Weiss, 2016-09-13 09:33:18]

Fixed both tests. PTAL

https://codereview.chromium.org/2319483002/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/fetch/Resource.h (right):

https://codereview.chromium.org/2319483002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/fetch/Resource.h:259: void
testOnlyWillAddClientOrObserver(PreloadReferencePolicy policy) {
willAddClientOrObserver(policy); }
Adding this test only method, since adding an actual observer (even a Mock one)
resulted in weird crashes around the m_observers HashCountedSet, which I suspect
is the result of it not being thread safe.

[commit-bot: I haz the power, 2016-09-13 10:50:35]

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

[commit-bot: I haz the power, 2016-09-13 10:50:36]

Dry run: This issue passed the CQ dry run.

[Nate Chapin, 2016-09-13 22:34:18]

https://codereview.chromium.org/2319483002/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/fetch/Resource.h (right):

https://codereview.chromium.org/2319483002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/fetch/Resource.h:259: void
testOnlyWillAddClientOrObserver(PreloadReferencePolicy policy) {
willAddClientOrObserver(policy); }
On 2016/09/13 09:33:18, Yoav Weiss wrote:
> Adding this test only method, since adding an actual observer (even a Mock
one)
> resulted in weird crashes around the m_observers HashCountedSet, which I
suspect
> is the result of it not being thread safe.

...this is all single threaded, isn't it? This explanation sounds suspect to me.
Are you sure the observer was getting both added and removed?

[Yoav Weiss, 2016-09-14 05:58:20]

On 2016/09/13 22:34:18, Nate Chapin wrote:
>
https://codereview.chromium.org/2319483002/diff/140001/third_party/WebKit/Sou...
> File third_party/WebKit/Source/core/fetch/Resource.h (right):
> 
>
https://codereview.chromium.org/2319483002/diff/140001/third_party/WebKit/Sou...
> third_party/WebKit/Source/core/fetch/Resource.h:259: void
> testOnlyWillAddClientOrObserver(PreloadReferencePolicy policy) {
> willAddClientOrObserver(policy); }
> On 2016/09/13 09:33:18, Yoav Weiss wrote:
> > Adding this test only method, since adding an actual observer (even a Mock
> one)
> > resulted in weird crashes around the m_observers HashCountedSet, which I
> suspect
> > is the result of it not being thread safe.
> 
> ...this is all single threaded, isn't it? This explanation sounds suspect to
me.
> Are you sure the observer was getting both added and removed?

Both Mock and real observers crashed upon addition. I can investigate further.

[Yoav Weiss, 2016-09-16 09:27:59]

The CQ bit was checked by yoav@yoav.ws to run a CQ dry run

[commit-bot: I haz the power, 2016-09-16 09:28:15]

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

[Yoav Weiss, 2016-09-16 10:27:30]

On 2016/09/14 05:58:20, Yoav Weiss wrote:
> On 2016/09/13 22:34:18, Nate Chapin wrote:
> >
>
https://codereview.chromium.org/2319483002/diff/140001/third_party/WebKit/Sou...
> > File third_party/WebKit/Source/core/fetch/Resource.h (right):
> > 
> >
>
https://codereview.chromium.org/2319483002/diff/140001/third_party/WebKit/Sou...
> > third_party/WebKit/Source/core/fetch/Resource.h:259: void
> > testOnlyWillAddClientOrObserver(PreloadReferencePolicy policy) {
> > willAddClientOrObserver(policy); }
> > On 2016/09/13 09:33:18, Yoav Weiss wrote:
> > > Adding this test only method, since adding an actual observer (even a Mock
> > one)
> > > resulted in weird crashes around the m_observers HashCountedSet, which I
> > suspect
> > > is the result of it not being thread safe.
> > 
> > ...this is all single threaded, isn't it? This explanation sounds suspect to
> me.
> > Are you sure the observer was getting both added and removed?
> 
> Both Mock and real observers crashed upon addition. I can investigate further.

Put back the mock observer and no longer see those crashes. Must've been Using
It Wrong™

PTAL?

[commit-bot: I haz the power, 2016-09-16 10:59:05]

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

[commit-bot: I haz the power, 2016-09-16 10:59:06]

Dry run: This issue passed the CQ dry run.

[Charlie Harrison, 2016-09-16 12:18:47]

non owner lgtm

https://codereview.chromium.org/2319483002/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/fetch/Resource.h (right):

https://codereview.chromium.org/2319483002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/fetch/Resource.h:259: void
testOnlyWillAddClientOrObserver(PreloadReferencePolicy policy) {
willAddClientOrObserver(policy); }
On 2016/09/13 09:33:18, Yoav Weiss wrote:
> Adding this test only method, since adding an actual observer (even a Mock
one)
> resulted in weird crashes around the m_observers HashCountedSet, which I
suspect
> is the result of it not being thread safe.

I think a "ForTesting" suffix is more common and I think (?) it gets checked by
a presubmit that the method is only used for tests.

[Charlie Harrison, 2016-09-16 12:31:38]

https://codereview.chromium.org/2319483002/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/fetch/Resource.h (right):

https://codereview.chromium.org/2319483002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/fetch/Resource.h:259: void
testOnlyWillAddClientOrObserver(PreloadReferencePolicy policy) {
willAddClientOrObserver(policy); }
On 2016/09/16 12:18:47, Charlie Harrison wrote:
> On 2016/09/13 09:33:18, Yoav Weiss wrote:
> > Adding this test only method, since adding an actual observer (even a Mock
> one)
> > resulted in weird crashes around the m_observers HashCountedSet, which I
> suspect
> > is the result of it not being thread safe.
> 
> I think a "ForTesting" suffix is more common and I think (?) it gets checked
by
> a presubmit that the method is only used for tests.

Apologies this was a very out of date draft comment :)

[Nate Chapin, 2016-09-16 21:31:33]

lgtm

[Yoav Weiss, 2016-09-16 21:34:53]

The CQ bit was checked by yoav@yoav.ws

[commit-bot: I haz the power, 2016-09-16 21:35:21]

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-16 21:42:39]

Description was changed from

==========
Turn off isLinkPreload() when used

Make sure that isLinkPreload flag is turned off when the resource is used,
making sure that it can be cleared earlier.

BUG=635483
==========

to

==========
Turn off isLinkPreload() when used

Make sure that isLinkPreload flag is turned off when the resource is used,
making sure that it can be cleared earlier.

BUG=635483
==========

[commit-bot: I haz the power, 2016-09-16 21:42:40]

Committed patchset #9 (id:160001)

[commit-bot: I haz the power, 2016-09-16 21:43:58]

Description was changed from

==========
Turn off isLinkPreload() when used

Make sure that isLinkPreload flag is turned off when the resource is used,
making sure that it can be cleared earlier.

BUG=635483
==========

to

==========
Turn off isLinkPreload() when used

Make sure that isLinkPreload flag is turned off when the resource is used,
making sure that it can be cleared earlier.

BUG=635483

Committed: https://crrev.com/41924d42a6d0ebdd5e2aebc4a5dcca2d5bc11eb6
Cr-Commit-Position: refs/heads/master@{#419283}
==========

[commit-bot: I haz the power, 2016-09-16 21:44:00]

Patchset 9 (id:??) landed as
https://crrev.com/41924d42a6d0ebdd5e2aebc4a5dcca2d5bc11eb6
Cr-Commit-Position: refs/heads/master@{#419283}