Make ResourceFetcher::m_documentResources use WeakPtr

Make ResourceFetcher::m_documentResources use WeakPtr

This removes the need for a special GC process.

BUG=

Committed: https://crrev.com/bf76e786d1c965ca8cf93e3abbc326f208b7c188
Cr-Commit-Position: refs/heads/master@{#365483}

[Nate Chapin, 2015-12-14 23:12:15]

japhet@chromium.org changed reviewers:
+ mkwst@chromium.org, sigbjorn@opera.com

[Nate Chapin, 2015-12-14 23:12:16]

mkwst: PTAL
sigbjorn: Can I get your blessing for the Oilpan-flavored bits in
Resource/ResourceFetcher?

I'm not sure how worried to be about the fact that this may leave null entries
in m_documentResources.

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

https://codereview.chromium.org/1428383002/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/fetch/ResourceFetcher.cpp:197: const
WeakPtrWillBeWeakMember<Resource>& resource = m_documentResources.get(url);
WeakMember is smarter than WeakPtr about HashMap return values and actually
returns a Resource* from get(url). I wasn't able to find a single-line return
statement that compiled both with and without Oilpan.

[kinuko, 2015-12-15 03:36:56]

Description was changed from

==========
Make ResourceFetcher::m_documentResources use WeakPtr

This removes the need for a special GC process.

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

to

==========
Make ResourceFetcher::m_documentResources use WeakPtr

This removes the need for a special GC process.

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

[Mike West, 2015-12-15 09:01:38]

Conceptually, I think this is the right direction, but the transitional types
still scare me. If sigbjorn@, et al are happy, then I'm happy. LGTM.

[sof, 2015-12-15 09:26:54]

sigbjornf@opera.com changed reviewers:
+ oilpan-reviews@chromium.org, sigbjornf@opera.com
- sigbjorn@opera.com

[sof, 2015-12-15 09:26:55]

+oilpan-reviews

[haraken, 2015-12-15 09:34:25]

ResourceFetcher::m_documentResources has been unnecessarily keeping Resources
alive longer than needed, so this CL makes a lot of sense to me.

[haraken, 2015-12-15 09:36:17]

On 2015/12/15 09:34:25, haraken wrote:
> ResourceFetcher::m_documentResources has been unnecessarily keeping Resources
> alive longer than needed, so this CL makes a lot of sense to me.

LGTM in terms of the transition types, but I want to have sigbjorn take another
look.

[sof, 2015-12-15 10:08:01]

Probably ok, but let me first test locally how this behaves with the leak
detector and Oilpan.

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

https://codereview.chromium.org/1428383002/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/fetch/ResourceFetcher.cpp:197: const
WeakPtrWillBeWeakMember<Resource>& resource = m_documentResources.get(url);
On 2015/12/14 23:12:16, Nate Chapin wrote:
> WeakMember is smarter than WeakPtr about HashMap return values and actually
> returns a Resource* from get(url). I wasn't able to find a single-line return
> statement that compiled both with and without Oilpan.

Traits are in place to unwrap the WeakMember on get(); not worth #if'ing for,
the transition type will be removed soon enough.

https://codereview.chromium.org/1428383002/diff/220001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/inspector/InspectorPageAgent.cpp (right):

https://codereview.chromium.org/1428383002/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/inspector/InspectorPageAgent.cpp:433: if
(!cachedResource)
Null resources won't be observable with Oilpan; weak processing during GC
sweeping will remove the dead weak references from the map.

And that's the main (and only) worry here -- that GC timing might turn out to be
observable.

[sof, 2015-12-15 12:29:45]

no observable differences encountered with leak detector nor LSan - lgtm.

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

https://codereview.chromium.org/1428383002/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/fetch/Resource.h:268:
WeakPtrWillBeRawPtr<Resource> asWeakPtr();
Add a TODO about removing this once Oilpan ships?

https://codereview.chromium.org/1428383002/diff/220001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/fetch/ResourceFetcher.cpp (left):

https://codereview.chromium.org/1428383002/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/fetch/ResourceFetcher.cpp:773: for (const auto&
documentResource : m_documentResources) {
This can be simplified some once we can assume Oilpan; add a TODO here also?

[Nate Chapin, 2015-12-15 20:30:59]

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

https://codereview.chromium.org/1428383002/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/fetch/Resource.h:268:
WeakPtrWillBeRawPtr<Resource> asWeakPtr();
On 2015/12/15 12:29:43, sof wrote:
> Add a TODO about removing this once Oilpan ships?

Done.

https://codereview.chromium.org/1428383002/diff/220001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/fetch/ResourceFetcher.cpp (left):

https://codereview.chromium.org/1428383002/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/fetch/ResourceFetcher.cpp:773: for (const auto&
documentResource : m_documentResources) {
On 2015/12/15 12:29:45, sof wrote:
> This can be simplified some once we can assume Oilpan; add a TODO here also?

Done.

[Nate Chapin, 2015-12-15 20:31:06]

The CQ bit was checked by japhet@chromium.org

[Nate Chapin, 2015-12-15 20:31:06]

The patchset sent to the CQ was uploaded after l-g-t-m from
haraken@chromium.org, sigbjornf@opera.com, mkwst@chromium.org
Link to the patchset: https://codereview.chromium.org/1428383002/#ps240001
(title: "Add TODOs")

[commit-bot: I haz the power, 2015-12-15 20:34:17]

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

[commit-bot: I haz the power, 2015-12-15 23:10:15]

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

[commit-bot: I haz the power, 2015-12-15 23:10:16]

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_...)

[Nate Chapin, 2015-12-15 23:14:45]

The CQ bit was checked by japhet@chromium.org

[commit-bot: I haz the power, 2015-12-15 23:16:44]

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

[commit-bot: I haz the power, 2015-12-16 03:42:34]

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

[commit-bot: I haz the power, 2015-12-16 03:42:35]

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, 2015-12-16 04:56:53]

The CQ bit was checked by hiroshige@chromium.org

[commit-bot: I haz the power, 2015-12-16 04:59:08]

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

[commit-bot: I haz the power, 2015-12-16 05:58:39]

Description was changed from

==========
Make ResourceFetcher::m_documentResources use WeakPtr

This removes the need for a special GC process.

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

to

==========
Make ResourceFetcher::m_documentResources use WeakPtr

This removes the need for a special GC process.

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

[commit-bot: I haz the power, 2015-12-16 05:58:40]

Committed patchset #13 (id:240001)

[commit-bot: I haz the power, 2015-12-16 05:59:32]

Description was changed from

==========
Make ResourceFetcher::m_documentResources use WeakPtr

This removes the need for a special GC process.

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

to

==========
Make ResourceFetcher::m_documentResources use WeakPtr

This removes the need for a special GC process.

BUG=

Committed: https://crrev.com/bf76e786d1c965ca8cf93e3abbc326f208b7c188
Cr-Commit-Position: refs/heads/master@{#365483}
==========

[commit-bot: I haz the power, 2015-12-16 05:59:35]

Patchset 13 (id:??) landed as
https://crrev.com/bf76e786d1c965ca8cf93e3abbc326f208b7c188
Cr-Commit-Position: refs/heads/master@{#365483}