Split ImageResourceClient into ResourceClient and ImageResourceObserver [1/2]

Split ImageResourceClient into ResourceClient and ImageResourceObserver [1/2]

We are planning to move ResourceClient to on-heap, but LayoutObject (an
ImageResourceClient subclass) is hard to move to on-heap.

This series of CLs splits ImageResourceClient into
- ResourceClient (notified via Resource, to be moved to Oilpan heap) and
- ImageResourceObserver (notified via ImageObserver, to remain non-heap)
and makes LayoutObject to be ImageResourceObserver but not ResourceClient.

This is the [1/2] CL that
- Splits ImageResourceClient into ResourceClient and ImageResourceObserver,
- Makes LayoutObject and all of its subclasses (except for LayoutImage and its
  subclasses) to be ImageResourceObserver but not ResourceClient.

LayoutImage will be made non-ResourceClient by the [2/2] CL:
https://codereview.chromium.org/1728313003/.

Original CL: https://codereview.chromium.org/1697713002/ by Nate Chapin.

In addition to Nate's original CL (Patch Set 1), this CL:
- Fixes layout test (Patch Set 2).
- Reuses ResourceClientWalker to iterate ImageResourceObserver (Patch Set 3).
- Handles ResourceClient and ImageResourceObserver more similarly (Patch Set 4).
- Renames methods to reflect whether they are for both ResourceClient and
  ImageResourceObserver (Patch Set 6).

BUG=587663
Committed: https://crrev.com/a4581858f57e20724f9b8a3166ef9c864f79a758
Cr-Commit-Position: refs/heads/master@{#383425}

[hiroshige, 2016-02-18 01:49:59]

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

[hiroshige, 2016-02-18 01:50:09]

Patchset #1 (id:1) has been deleted

[commit-bot: I haz the power, 2016-02-18 01:50:23]

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

[hiroshige, 2016-02-18 01:51:30]

The CQ bit was unchecked by hiroshige@chromium.org

[hiroshige, 2016-02-18 01:55:32]

Patchset #1 (id:20001) has been deleted

[hiroshige, 2016-02-18 01:55:37]

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

[commit-bot: I haz the power, 2016-02-18 01:57:40]

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

[hiroshige, 2016-02-18 02:02:11]

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

[commit-bot: I haz the power, 2016-02-18 02:03:42]

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

[commit-bot: I haz the power, 2016-02-18 03:13:20]

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

[commit-bot: I haz the power, 2016-02-18 03:13:21]

Dry run: This issue passed the CQ dry run.

[hiroshige, 2016-02-18 18:32:52]

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

[commit-bot: I haz the power, 2016-02-18 18:39:54]

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

[hiroshige, 2016-02-18 19:21:44]

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

[commit-bot: I haz the power, 2016-02-18 19:22:32]

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

[hiroshige, 2016-02-18 19:39:44]

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

[commit-bot: I haz the power, 2016-02-18 19:40:15]

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

[commit-bot: I haz the power, 2016-02-18 21:10:31]

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

[commit-bot: I haz the power, 2016-02-18 21:10:32]

Dry run: This issue passed the CQ dry run.

[hiroshige, 2016-02-18 21:25:33]

Description was changed from

==========
[WIP2] ImageResourceClient->ImageResourceObserver
==========

to

==========
[WIP2] Split ImageResourceClient into ResourceClient and ImageResourceObserver

BUG=587663
==========

[hiroshige, 2016-02-19 01:28:38]

Description was changed from

==========
[WIP2] Split ImageResourceClient into ResourceClient and ImageResourceObserver

BUG=587663
==========

to

==========
[WIP2] Split ImageResourceClient into ResourceClient and ImageResourceObserver

Basically ready. TODO for simple method renaming.

BUG=587663
==========

[hiroshige, 2016-02-19 03:53:19]

Description was changed from

==========
[WIP2] Split ImageResourceClient into ResourceClient and ImageResourceObserver

Basically ready. TODO for simple method renaming.

BUG=587663
==========

to

==========
[WIP2] Split ImageResourceClient into ResourceClient and ImageResourceObserver

Basically ready. TODO for simple method renaming.

In addition to Nate's original CL (Patch Set 1), this CL addresses:
- Fix layout test (Patch Set 2)
- Reuse ResourceClientWalker to iterate over ImageResourceObserver (Patch Set 3)
- Handle ResourceClient and ImageResourceObserver more similarly (Patch Set 4,
5)
- Do the same thing in add/removeClient() and add/removeObserver() if
applicable.
- Make hasClients() to return true if there are observers.

BUG=587663
==========

[hiroshige, 2016-02-19 06:22:52]

Description was changed from

==========
[WIP2] Split ImageResourceClient into ResourceClient and ImageResourceObserver

Basically ready. TODO for simple method renaming.

In addition to Nate's original CL (Patch Set 1), this CL addresses:
- Fix layout test (Patch Set 2)
- Reuse ResourceClientWalker to iterate over ImageResourceObserver (Patch Set 3)
- Handle ResourceClient and ImageResourceObserver more similarly (Patch Set 4,
5)
- Do the same thing in add/removeClient() and add/removeObserver() if
applicable.
- Make hasClients() to return true if there are observers.

BUG=587663
==========

to

==========
[WIP2] Split ImageResourceClient into ResourceClient and ImageResourceObserver

Basically ready. TODO for simple method renaming.

In addition to Nate's original CL (Patch Set 1), this CL addresses:
- Fix layout test (Patch Set 2)
- Reuse ResourceClientWalker to iterate over ImageResourceObserver (Patch Set 3)
- Handle ResourceClient and ImageResourceObserver more similarly (Patch Set 4,
5)
-- Do the same thing in add/removeClient() and add/removeObserver() if
applicable.
-- Make hasClients() to return true if there are observers.

BUG=587663
==========

[hiroshige, 2016-02-20 01:53:13]

Description was changed from

==========
[WIP2] Split ImageResourceClient into ResourceClient and ImageResourceObserver

Basically ready. TODO for simple method renaming.

In addition to Nate's original CL (Patch Set 1), this CL addresses:
- Fix layout test (Patch Set 2)
- Reuse ResourceClientWalker to iterate over ImageResourceObserver (Patch Set 3)
- Handle ResourceClient and ImageResourceObserver more similarly (Patch Set 4,
5)
-- Do the same thing in add/removeClient() and add/removeObserver() if
applicable.
-- Make hasClients() to return true if there are observers.

BUG=587663
==========

to

==========
[WIP2] Split ImageResourceClient into ResourceClient and ImageResourceObserver

Basically ready. TODO for simple method renaming.

Original CL: https://codereview.chromium.org/1697713002/
by Nate Chapin.

In addition to Nate's original CL (Patch Set 1), this CL addresses:
- Fix layout test (Patch Set 2)
- Reuse ResourceClientWalker to iterate over ImageResourceObserver (Patch Set 3)
- Handle ResourceClient and ImageResourceObserver more similarly (Patch Set 4,
5)
-- Do the same thing in add/removeClient() and add/removeObserver() if
applicable.
-- Make hasClients() to return true if there are observers.

BUG=587663
==========

[hiroshige, 2016-02-20 01:53:13]

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

[hiroshige, 2016-02-20 01:57:49]

Description was changed from

==========
[WIP2] Split ImageResourceClient into ResourceClient and ImageResourceObserver

Basically ready. TODO for simple method renaming.

Original CL: https://codereview.chromium.org/1697713002/
by Nate Chapin.

In addition to Nate's original CL (Patch Set 1), this CL addresses:
- Fix layout test (Patch Set 2)
- Reuse ResourceClientWalker to iterate over ImageResourceObserver (Patch Set 3)
- Handle ResourceClient and ImageResourceObserver more similarly (Patch Set 4,
5)
-- Do the same thing in add/removeClient() and add/removeObserver() if
applicable.
-- Make hasClients() to return true if there are observers.

BUG=587663
==========

to

==========
Split ImageResourceClient into ResourceClient and ImageResourceObserver

Original CL: https://codereview.chromium.org/1697713002/
by Nate Chapin.

We are planning to move ResourceClient to on-heap, but LayoutObject (an
ImageResourceClient subclass) is hard to move to on-heap.
This CL splits ImageResourceClient into ResourceClient (notified via Resource)
and ImageResourceObserver (notified via ImageObserver).
This CL makes LayoutObject to be ImageResourceObserver but not ResourceClient,
and thus make it easier to move ResourceClient to on-heap.

In addition to Nate's original CL (Patch Set 1), this CL addresses:
- Fix layout test (Patch Set 2)
- Reuse ResourceClientWalker to iterate over ImageResourceObserver (Patch Set 3)
- Handle ResourceClient and ImageResourceObserver more similarly (Patch Set 4,
5)
-- Do the same thing in add/removeClient() and add/removeObserver() if
applicable.
-- Make hasClients() to return true if there are observers.

BUG=587663
==========

[hiroshige, 2016-02-20 02:00:19]

PTAL.

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

https://codereview.chromium.org/1706083002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/fetch/Resource.h:140: virtual bool hasClients()
const { return !m_clients.isEmpty() || !m_clientsAwaitingCallback.isEmpty() ||
!m_finishedClients.isEmpty(); }
In this CL, hasClients() counts not only ResourceClient but also
ImageResourceOberver in case of ImageResource.
Thus should we change the function name to hasClientsOrObservers() or so?
ditto below.

[hiroshige, 2016-02-26 01:24:46]

I'll make LayoutImage non-ResourceClient by removing
LayoutImage::notifyFinished() before this CL, in order to make all subclasses of
LayoutObject non-ResourceClient and non-Oilpan.
(Draft CL: https://codereview.chromium.org/1728313003/)
The draft CL contains dirty workarounds, and I'll wait for yhirano@'s multipart
cleanup CL that might enable me more cleaner implementation.

[hiroshige, 2016-02-29 22:43:26]

Patchset #6 (id:140001) has been deleted

[hiroshige, 2016-02-29 23:22: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 23:23:10]

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

[commit-bot: I haz the power, 2016-03-01 00:18:11]

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

[commit-bot: I haz the power, 2016-03-01 00:18:12]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[hiroshige, 2016-03-01 02:05:55]

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

[commit-bot: I haz the power, 2016-03-01 02:06:18]

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

[commit-bot: I haz the power, 2016-03-01 02:23:28]

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

[commit-bot: I haz the power, 2016-03-01 02:23:30]

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

[hiroshige, 2016-03-01 18:56:43]

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

[commit-bot: I haz the power, 2016-03-01 18:57:14]

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

[hiroshige, 2016-03-01 19:01:22]

Patchset #4 (id:100001) has been deleted

[hiroshige, 2016-03-01 19:06:01]

Description was changed from

==========
Split ImageResourceClient into ResourceClient and ImageResourceObserver

Original CL: https://codereview.chromium.org/1697713002/
by Nate Chapin.

We are planning to move ResourceClient to on-heap, but LayoutObject (an
ImageResourceClient subclass) is hard to move to on-heap.
This CL splits ImageResourceClient into ResourceClient (notified via Resource)
and ImageResourceObserver (notified via ImageObserver).
This CL makes LayoutObject to be ImageResourceObserver but not ResourceClient,
and thus make it easier to move ResourceClient to on-heap.

In addition to Nate's original CL (Patch Set 1), this CL addresses:
- Fix layout test (Patch Set 2)
- Reuse ResourceClientWalker to iterate over ImageResourceObserver (Patch Set 3)
- Handle ResourceClient and ImageResourceObserver more similarly (Patch Set 4,
5)
-- Do the same thing in add/removeClient() and add/removeObserver() if
applicable.
-- Make hasClients() to return true if there are observers.

BUG=587663
==========

to

==========
Split ImageResourceClient into ResourceClient and ImageResourceObserver [1/2]

Original CL: https://codereview.chromium.org/1697713002/
by Nate Chapin.

We are planning to move ResourceClient to on-heap, but LayoutObject (an
ImageResourceClient subclass) is hard to move to on-heap.
This CL splits ImageResourceClient into ResourceClient (notified via Resource)
and ImageResourceObserver (notified via ImageObserver).
This CL makes LayoutObject to be ImageResourceObserver but not ResourceClient,
and thus make it easier to move ResourceClient to on-heap.

In addition to Nate's original CL (Patch Set 1), this CL addresses:
- Fix layout test (Patch Set 2).
- Reuse ResourceClientWalker to iterate over ImageResourceObserver (Patch Set
3).
- Handle ResourceClient and ImageResourceObserver more similarly (Patch Set 4).
-- Do the same thing in add/removeClient() and add/removeObserver() if
applicable.
-- Make hasClients() to return true if there are observers.
- Rename methods to reflect whether they are for both ResourceClient and
  ImageResourceObserver (Patch Set 6).

This CL left the inheritance of ResourceClient by LayoutImage.
The inheritance will be removed by https://codereview.chromium.org/1728313003/.

BUG=587663
==========

[hiroshige, 2016-03-01 19:42:41]

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

[commit-bot: I haz the power, 2016-03-01 19:43:13]

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

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

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

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

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:24:34]

Description was changed from

==========
Split ImageResourceClient into ResourceClient and ImageResourceObserver [1/2]

Original CL: https://codereview.chromium.org/1697713002/
by Nate Chapin.

We are planning to move ResourceClient to on-heap, but LayoutObject (an
ImageResourceClient subclass) is hard to move to on-heap.
This CL splits ImageResourceClient into ResourceClient (notified via Resource)
and ImageResourceObserver (notified via ImageObserver).
This CL makes LayoutObject to be ImageResourceObserver but not ResourceClient,
and thus make it easier to move ResourceClient to on-heap.

In addition to Nate's original CL (Patch Set 1), this CL addresses:
- Fix layout test (Patch Set 2).
- Reuse ResourceClientWalker to iterate over ImageResourceObserver (Patch Set
3).
- Handle ResourceClient and ImageResourceObserver more similarly (Patch Set 4).
-- Do the same thing in add/removeClient() and add/removeObserver() if
applicable.
-- Make hasClients() to return true if there are observers.
- Rename methods to reflect whether they are for both ResourceClient and
  ImageResourceObserver (Patch Set 6).

This CL left the inheritance of ResourceClient by LayoutImage.
The inheritance will be removed by https://codereview.chromium.org/1728313003/.

BUG=587663
==========

to

==========
Split ImageResourceClient into ResourceClient and ImageResourceObserver [1/2]

We are planning to move ResourceClient to on-heap, but LayoutObject (an
ImageResourceClient subclass) is hard to move to on-heap.

This series of CLs splits ImageResourceClient into
- ResourceClient (notified via Resource, to be moved to Oilpan heap) and
- ImageResourceObserver (notified via ImageObserver, to remain non-heap)
and makes LayoutObject to be ImageResourceObserver but not ResourceClient.

This is the [1/2] CL that
- Splits ImageResourceClient into ResourceClient and ImageResourceObserver,
- Makes LayoutObject and all of its subclasses (except for LayoutImage and its
  subclasses) to be ImageResourceObserver but not ResourceClient.

LayoutImage will be made non-ResourceClient by the [2/2] CL:
https://codereview.chromium.org/1728313003/.

Original CL: https://codereview.chromium.org/1697713002/ by Nate Chapin.

In addition to Nate's original CL (Patch Set 1), this CL:
- Fixes layout test (Patch Set 2).
- Reuses ResourceClientWalker to iterate ImageResourceObserver (Patch Set 3).
- Handles ResourceClient and ImageResourceObserver more similarly (Patch Set 4).
- Renames methods to reflect whether they are for both ResourceClient and
  ImageResourceObserver (Patch Set 6).

BUG=587663
==========

[hiroshige, 2016-03-01 22:38:40]

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

[hiroshige, 2016-03-01 22:38:42]

esprehn@:
could you take a look at core/layout/ changes?

japhet@, yhirano@:
Created [2/2] https://codereview.chromium.org/1728313003/ to make LayoutImage
non-ResourceClient.
PTAL again?

[yhirano, 2016-03-02 01:20:52]

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

https://codereview.chromium.org/1706083002/diff/240001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/fetch/Resource.h:151: // |this| object may be
dead after didRemoveClient().
didRemoveClientOrObserver

https://codereview.chromium.org/1706083002/diff/240001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/fetch/ResourceClient.h (right):

https://codereview.chromium.org/1706083002/diff/240001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/fetch/ResourceClient.h:39: ImageType,
Is this needed?

https://codereview.chromium.org/1706083002/diff/240001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/fetch/ResourceClientWalker.h (right):

https://codereview.chromium.org/1706083002/diff/240001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/fetch/ResourceClientWalker.h:43:
ResourceClientOrObserverWalker(const HashCountedSet<ClientOrObserver*>& set)
+explicit

https://codereview.chromium.org/1706083002/diff/240001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/fetch/ResourceClientWalker.h:73:
ResourceClientWalker(const HashCountedSet<ResourceClient*>& set)
+explicit

https://codereview.chromium.org/1706083002/diff/240001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/layout/LayoutImage.h (right):

https://codereview.chromium.org/1706083002/diff/240001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/LayoutImage.h:81: String debugName() const
final { return LayoutObject::debugName(); }
Why are you using LayoutObject's name?

[hiroshige, 2016-03-11 22:55:13]

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

[hiroshige, 2016-03-11 22:55:38]

Patchset #10 (id:260001) has been deleted

[commit-bot: I haz the power, 2016-03-11 22:55:56]

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

[commit-bot: I haz the power, 2016-03-12 00:34:05]

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

[commit-bot: I haz the power, 2016-03-12 00:34:07]

Dry run: This issue passed the CQ dry run.

[hiroshige, 2016-03-12 01:03:36]

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

https://codereview.chromium.org/1706083002/diff/240001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/fetch/Resource.h:151: // |this| object may be
dead after didRemoveClient().
On 2016/03/02 01:20:52, yhirano wrote:
> didRemoveClientOrObserver

Done.

https://codereview.chromium.org/1706083002/diff/240001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/fetch/ResourceClient.h (right):

https://codereview.chromium.org/1706083002/diff/240001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/fetch/ResourceClient.h:39: ImageType,
On 2016/03/02 01:20:52, yhirano wrote:
> Is this needed?

No longer needed. Removed.

https://codereview.chromium.org/1706083002/diff/240001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/fetch/ResourceClientWalker.h (right):

https://codereview.chromium.org/1706083002/diff/240001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/fetch/ResourceClientWalker.h:43:
ResourceClientOrObserverWalker(const HashCountedSet<ClientOrObserver*>& set)
On 2016/03/02 01:20:52, yhirano wrote:
> +explicit

Done.

https://codereview.chromium.org/1706083002/diff/240001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/fetch/ResourceClientWalker.h:73:
ResourceClientWalker(const HashCountedSet<ResourceClient*>& set)
On 2016/03/02 01:20:52, yhirano wrote:
> +explicit

Done.

https://codereview.chromium.org/1706083002/diff/240001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/layout/LayoutImage.h (right):

https://codereview.chromium.org/1706083002/diff/240001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/LayoutImage.h:81: String debugName() const
final { return LayoutObject::debugName(); }
On 2016/03/02 01:20:52, yhirano wrote:
> Why are you using LayoutObject's name?

LayoutObject::debugName() is intended to be used for all subclasses of
LayoutObject and thus previously declared final.
So calls to debugName() for LayoutImage should use LayoutObject::debugName(),
but ResourceClient inheritance here in Line 47 requires LayoutImage::debugName()
to exist.

This debugName() is removed in [2/2] where LayoutImage is no longer
ResourceClient.

[yhirano, 2016-03-17 04:44:57]

lgtm

https://codereview.chromium.org/1706083002/diff/280001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/fetch/ImageResource.cpp (right):

https://codereview.chromium.org/1706083002/diff/280001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/fetch/ImageResource.cpp:44: namespace {
Is this unnamed namespace necessary? My (rough) understanding is that a typedef
name has generally no linkage.

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

https://codereview.chromium.org/1706083002/diff/280001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/fetch/Resource.h:144: virtual void
didAddClient(ResourceClient*);
I think placing these two functions and willAddClientsOrObserver close is good.

https://codereview.chromium.org/1706083002/diff/280001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/fetch/ResourceClientWalker.h (right):

https://codereview.chromium.org/1706083002/diff/280001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/fetch/ResourceClientWalker.h:40: class
ResourceClientOrObserverWalker {
Is it good to rename this file?

[hiroshige, 2016-03-22 16:45:48]

friendly ping; could you take a look?

[Nate Chapin, 2016-03-22 21:27:42]

LGTM

https://codereview.chromium.org/1706083002/diff/280001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/fetch/ImageResourceObserver.h (right):

https://codereview.chromium.org/1706083002/diff/280001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/fetch/ImageResourceObserver.h:58: static bool
isExpectedType(ImageResourceObserver*) { return true; }
Is this needed?

https://codereview.chromium.org/1706083002/diff/280001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/layout/LayoutImage.h (right):

https://codereview.chromium.org/1706083002/diff/280001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/LayoutImage.h:47: class CORE_EXPORT
LayoutImage : public LayoutReplaced, public ResourceClient {
We're going to need to figure out how to not have this be a ResourceClient
eventually, right?

[hiroshige, 2016-03-22 21:39:15]

(I'll address yhirano@'s comments soon)

https://codereview.chromium.org/1706083002/diff/280001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/fetch/ImageResourceObserver.h (right):

https://codereview.chromium.org/1706083002/diff/280001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/fetch/ImageResourceObserver.h:58: static bool
isExpectedType(ImageResourceObserver*) { return true; }
On 2016/03/22 21:27:42, Nate Chapin wrote:
> Is this needed?

Yes, referenced from |ASSERT(T::isExpectedType(next))| in
ResourceClientWalker.h.

https://codereview.chromium.org/1706083002/diff/280001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/layout/LayoutImage.h (right):

https://codereview.chromium.org/1706083002/diff/280001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/LayoutImage.h:47: class CORE_EXPORT
LayoutImage : public LayoutReplaced, public ResourceClient {
On 2016/03/22 21:27:42, Nate Chapin wrote:
> We're going to need to figure out how to not have this be a ResourceClient
> eventually, right?

This is done immediately by https://codereview.chromium.org/1728313003/, PTAL.

[hiroshige, 2016-03-23 16:40:57]

Patchset #11 (id:300001) has been deleted

[hiroshige, 2016-03-23 17:24:43]

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

[commit-bot: I haz the power, 2016-03-23 17:24:58]

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

[commit-bot: I haz the power, 2016-03-23 18:37:40]

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

[commit-bot: I haz the power, 2016-03-23 18:37:42]

Dry run: This issue passed the CQ dry run.

[hiroshige, 2016-03-23 18:47:25]

https://codereview.chromium.org/1706083002/diff/280001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/fetch/ImageResource.cpp (right):

https://codereview.chromium.org/1706083002/diff/280001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/fetch/ImageResource.cpp:44: namespace {
On 2016/03/17 04:44:57, yhirano wrote:
> Is this unnamed namespace necessary? My (rough) understanding is that a
typedef
> name has generally no linkage.

Done.

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

https://codereview.chromium.org/1706083002/diff/280001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/fetch/Resource.h:144: virtual void
didAddClient(ResourceClient*);
On 2016/03/17 04:44:57, yhirano wrote:
> I think placing these two functions and willAddClientsOrObserver close is
good.

Done. Also make them protected.

https://codereview.chromium.org/1706083002/diff/280001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/fetch/ResourceClientWalker.h (right):

https://codereview.chromium.org/1706083002/diff/280001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/fetch/ResourceClientWalker.h:40: class
ResourceClientOrObserverWalker {
On 2016/03/17 04:44:57, yhirano wrote:
> Is it good to rename this file?

Done.

[hiroshige, 2016-03-25 20:08:09]

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

[commit-bot: I haz the power, 2016-03-25 20:08:23]

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

[commit-bot: I haz the power, 2016-03-25 22:35:43]

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

[commit-bot: I haz the power, 2016-03-25 22:35:45]

Dry run: This issue passed the CQ dry run.

[hiroshige, 2016-03-26 00:43:13]

The CQ bit was checked by hiroshige@chromium.org

[hiroshige, 2016-03-26 00:43:14]

The patchset sent to the CQ was uploaded after l-g-t-m from
yhirano@chromium.org, japhet@chromium.org
Link to the patchset: https://codereview.chromium.org/1706083002/#ps380001
(title: "Rebase.")

[commit-bot: I haz the power, 2016-03-26 00:43:40]

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

[commit-bot: I haz the power, 2016-03-26 00:50:25]

Description was changed from

==========
Split ImageResourceClient into ResourceClient and ImageResourceObserver [1/2]

We are planning to move ResourceClient to on-heap, but LayoutObject (an
ImageResourceClient subclass) is hard to move to on-heap.

This series of CLs splits ImageResourceClient into
- ResourceClient (notified via Resource, to be moved to Oilpan heap) and
- ImageResourceObserver (notified via ImageObserver, to remain non-heap)
and makes LayoutObject to be ImageResourceObserver but not ResourceClient.

This is the [1/2] CL that
- Splits ImageResourceClient into ResourceClient and ImageResourceObserver,
- Makes LayoutObject and all of its subclasses (except for LayoutImage and its
  subclasses) to be ImageResourceObserver but not ResourceClient.

LayoutImage will be made non-ResourceClient by the [2/2] CL:
https://codereview.chromium.org/1728313003/.

Original CL: https://codereview.chromium.org/1697713002/ by Nate Chapin.

In addition to Nate's original CL (Patch Set 1), this CL:
- Fixes layout test (Patch Set 2).
- Reuses ResourceClientWalker to iterate ImageResourceObserver (Patch Set 3).
- Handles ResourceClient and ImageResourceObserver more similarly (Patch Set 4).
- Renames methods to reflect whether they are for both ResourceClient and
  ImageResourceObserver (Patch Set 6).

BUG=587663
==========

to

==========
Split ImageResourceClient into ResourceClient and ImageResourceObserver [1/2]

We are planning to move ResourceClient to on-heap, but LayoutObject (an
ImageResourceClient subclass) is hard to move to on-heap.

This series of CLs splits ImageResourceClient into
- ResourceClient (notified via Resource, to be moved to Oilpan heap) and
- ImageResourceObserver (notified via ImageObserver, to remain non-heap)
and makes LayoutObject to be ImageResourceObserver but not ResourceClient.

This is the [1/2] CL that
- Splits ImageResourceClient into ResourceClient and ImageResourceObserver,
- Makes LayoutObject and all of its subclasses (except for LayoutImage and its
  subclasses) to be ImageResourceObserver but not ResourceClient.

LayoutImage will be made non-ResourceClient by the [2/2] CL:
https://codereview.chromium.org/1728313003/.

Original CL: https://codereview.chromium.org/1697713002/ by Nate Chapin.

In addition to Nate's original CL (Patch Set 1), this CL:
- Fixes layout test (Patch Set 2).
- Reuses ResourceClientWalker to iterate ImageResourceObserver (Patch Set 3).
- Handles ResourceClient and ImageResourceObserver more similarly (Patch Set 4).
- Renames methods to reflect whether they are for both ResourceClient and
  ImageResourceObserver (Patch Set 6).

BUG=587663
==========

[commit-bot: I haz the power, 2016-03-26 00:50:27]

Committed patchset #14 (id:380001)

[commit-bot: I haz the power, 2016-03-26 00:52:09]

Description was changed from

==========
Split ImageResourceClient into ResourceClient and ImageResourceObserver [1/2]

We are planning to move ResourceClient to on-heap, but LayoutObject (an
ImageResourceClient subclass) is hard to move to on-heap.

This series of CLs splits ImageResourceClient into
- ResourceClient (notified via Resource, to be moved to Oilpan heap) and
- ImageResourceObserver (notified via ImageObserver, to remain non-heap)
and makes LayoutObject to be ImageResourceObserver but not ResourceClient.

This is the [1/2] CL that
- Splits ImageResourceClient into ResourceClient and ImageResourceObserver,
- Makes LayoutObject and all of its subclasses (except for LayoutImage and its
  subclasses) to be ImageResourceObserver but not ResourceClient.

LayoutImage will be made non-ResourceClient by the [2/2] CL:
https://codereview.chromium.org/1728313003/.

Original CL: https://codereview.chromium.org/1697713002/ by Nate Chapin.

In addition to Nate's original CL (Patch Set 1), this CL:
- Fixes layout test (Patch Set 2).
- Reuses ResourceClientWalker to iterate ImageResourceObserver (Patch Set 3).
- Handles ResourceClient and ImageResourceObserver more similarly (Patch Set 4).
- Renames methods to reflect whether they are for both ResourceClient and
  ImageResourceObserver (Patch Set 6).

BUG=587663
==========

to

==========
Split ImageResourceClient into ResourceClient and ImageResourceObserver [1/2]

We are planning to move ResourceClient to on-heap, but LayoutObject (an
ImageResourceClient subclass) is hard to move to on-heap.

This series of CLs splits ImageResourceClient into
- ResourceClient (notified via Resource, to be moved to Oilpan heap) and
- ImageResourceObserver (notified via ImageObserver, to remain non-heap)
and makes LayoutObject to be ImageResourceObserver but not ResourceClient.

This is the [1/2] CL that
- Splits ImageResourceClient into ResourceClient and ImageResourceObserver,
- Makes LayoutObject and all of its subclasses (except for LayoutImage and its
  subclasses) to be ImageResourceObserver but not ResourceClient.

LayoutImage will be made non-ResourceClient by the [2/2] CL:
https://codereview.chromium.org/1728313003/.

Original CL: https://codereview.chromium.org/1697713002/ by Nate Chapin.

In addition to Nate's original CL (Patch Set 1), this CL:
- Fixes layout test (Patch Set 2).
- Reuses ResourceClientWalker to iterate ImageResourceObserver (Patch Set 3).
- Handles ResourceClient and ImageResourceObserver more similarly (Patch Set 4).
- Renames methods to reflect whether they are for both ResourceClient and
  ImageResourceObserver (Patch Set 6).

BUG=587663

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

[commit-bot: I haz the power, 2016-03-26 00:52:11]

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