[ImageResource 4] Split ImageResource into Resource and Image parts

Split ImageResource into Resource and Image parts

Currently, ImageResource is
- A part of Image, as a kind of a wrapper of blink::Image and
  the entry point of ImageResourceObserver, and
- A part of Resource, because it is a subclass of Resource and highly
  involved into internals of Resource such as state control of Resource.
This complicates the ImageResource, causes dependencies and
layering violation between loading- and image-related things, and
complicates reloading logic.

This CL splits ImageResource into two parts:
ImageResource, the Resource-part:
- A subclass of Resource.
ImageResourceContent, the Image-part:
- Manages blink::Image and ImageResourceObserver, and inherits ImageObserver.
- Has limited access to Resource-part (if any) via ImageResourceInfo.

Most of current users of ImageResource, especially those in core/style, are
migrated to use ImageResourceContent and/or
to inherit ImageResourceObserver instead of ResourceClient,
as they want to access Image, not necessarily resource loading.

Design doc:
https://docs.google.com/document/d/1O-fB83mrE0B_V8gzXNqHgmRLCvstTB4MMi3RnVLr8bE/edit?usp=sharing

In this CL, the Resource-part and the Image-part has 1:1 correspondence that
doesn't change during their lifetime and therefore ImageResource::m_content
is always non-null.
Subsequent CLs will make them detachable.

Changes in most of the files are replacing ImageResource with
ImageResourceContent.
Notable changes are:

Main part of CL: split ImageResource into two parts:
- core/fetch/ImageResource.h
- core/fetch/ImageResource.cpp
- core/fetch/ImageResourceContent.h
- core/fetch/ImageResourceContent.cpp

emulateLoadStartedForInspector() hack is now called via ImageResourceContent:
- core/css/CSSImageValue.cpp

ImageLoader is mostly migrated to ImageResourceContent, except for
imageResourceForImageDocument() that returns ImageResource only for
ImageDocument.
- core/html/HTMLImageElement.h
- core/html/ImageDocument.cpp
- core/loader/ImageLoader.h
- core/loader/ImageLoader.cpp

A reference to Resource from FrameSerializer is replaced with |mimeType|
String and |hasCacheControlNoStoreHeader| boolean in order to migrate to
ImageResourceContent.
- web/WebFrameSerializer.cpp
- core/frame/FrameSerializer.h
- core/frame/FrameSerializer.cpp

BUG=667641
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2

Committed: https://crrev.com/7b80eac87bd4a9f5d0fad898dd73e239ee36fbb3
Cr-Commit-Position: refs/heads/master@{#437867}

[hiroshige, 2016-11-01 19:05:07]

Description was changed from

==========
temp


temp

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

to

==========
temp


temp

BUG=
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

[hiroshige, 2016-11-04 10:44:03]

Description was changed from

==========
temp


temp

BUG=
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

to

==========
temp


temp

BUG=
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

[hiroshige, 2016-11-09 07:28:30]

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

[commit-bot: I haz the power, 2016-11-09 07:28:41]

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

[hiroshige, 2016-11-09 07:54:47]

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

[commit-bot: I haz the power, 2016-11-09 07:55:02]

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-11-09 08:22:07]

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

[commit-bot: I haz the power, 2016-11-09 08:22:08]

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

[hiroshige, 2016-11-09 08:23:36]

Description was changed from

==========
temp


temp

BUG=
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

to

==========
Currently, ImageResource is
- A part of Resource, because it is a subclass of Resource and highly involved
into internals of Resource such as state control of Resource, and
- A part of Image, as a kind of a wrapper of blink::Image and the entry point of
ImageResourceObserver.
This complicates the ImageResource and causes layering violation.

Also, ImageResource manages timings of Resource- and Image-related callbacks,
which leads to timing dependencies between them, e.g. between
ResourceLoader::didFinishLoading() and <img>'s onload.

This CL splits ImageResource into
- ImageResourceInternal, the Resource-only class, and
- ImageResourceContent, the Image-only class which manages blink::Image and
ImageResourceObserver.

Most of users of ImageResource, especially those in core/style, are migrated
into ImageResourceContent, because they need image and shouldn't touch resource
loading directly.

BUG=

CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

[hiroshige, 2016-11-09 11:43:51]

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

[commit-bot: I haz the power, 2016-11-09 11:44:02]

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

[hiroshige, 2016-11-09 11:44:32]

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

[commit-bot: I haz the power, 2016-11-09 11:44:42]

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

[hiroshige, 2016-11-09 11:53:43]

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

[commit-bot: I haz the power, 2016-11-09 11:53:56]

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-11-09 12:22:16]

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

[commit-bot: I haz the power, 2016-11-09 12:22:17]

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

[hiroshige, 2016-11-22 05:46:41]

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

[hiroshige, 2016-11-22 12:59:59]

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

[commit-bot: I haz the power, 2016-11-22 13:00:20]

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

[hiroshige, 2016-11-22 13:10:02]

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

[commit-bot: I haz the power, 2016-11-22 13:10:12]

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-11-22 15:25:18]

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

[commit-bot: I haz the power, 2016-11-22 15:25:19]

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

[hiroshige, 2016-11-23 16:21:05]

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

[commit-bot: I haz the power, 2016-11-23 16:21:30]

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-11-23 17:46:14]

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

[commit-bot: I haz the power, 2016-11-23 17:46:15]

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

[hiroshige, 2016-11-24 05:11:18]

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

[hiroshige, 2016-11-24 05:12:05]

Description was changed from

==========
Currently, ImageResource is
- A part of Resource, because it is a subclass of Resource and highly involved
into internals of Resource such as state control of Resource, and
- A part of Image, as a kind of a wrapper of blink::Image and the entry point of
ImageResourceObserver.
This complicates the ImageResource and causes layering violation.

Also, ImageResource manages timings of Resource- and Image-related callbacks,
which leads to timing dependencies between them, e.g. between
ResourceLoader::didFinishLoading() and <img>'s onload.

This CL splits ImageResource into
- ImageResourceInternal, the Resource-only class, and
- ImageResourceContent, the Image-only class which manages blink::Image and
ImageResourceObserver.

Most of users of ImageResource, especially those in core/style, are migrated
into ImageResourceContent, because they need image and shouldn't touch resource
loading directly.

BUG=

CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

to

==========
Currently, ImageResource is
- A part of Resource, because it is a subclass of Resource and highly involved
into internals of Resource such as state control of Resource, and
- A part of Image, as a kind of a wrapper of blink::Image and the entry point of
ImageResourceObserver.
This complicates the ImageResource and causes layering violation.

Also, ImageResource manages timings of Resource- and Image-related callbacks,
which leads to timing dependencies between them, e.g. between
ResourceLoader::didFinishLoading() and <img>'s onload.

This CL splits ImageResource into
- ImageResourceInternal, the Resource-only class, and
- ImageResourceContent, the Image-only class which manages blink::Image and
ImageResourceObserver.

Most of users of ImageResource, especially those in core/style, are migrated
into ImageResourceContent, because they need image and shouldn't touch resource
loading directly.

https://docs.google.com/document/d/1O-fB83mrE0B_V8gzXNqHgmRLCvstTB4MMi3RnVLr8...

BUG=

CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

[hiroshige, 2016-11-24 05:15:28]

Description was changed from

==========
Currently, ImageResource is
- A part of Resource, because it is a subclass of Resource and highly involved
into internals of Resource such as state control of Resource, and
- A part of Image, as a kind of a wrapper of blink::Image and the entry point of
ImageResourceObserver.
This complicates the ImageResource and causes layering violation.

Also, ImageResource manages timings of Resource- and Image-related callbacks,
which leads to timing dependencies between them, e.g. between
ResourceLoader::didFinishLoading() and <img>'s onload.

This CL splits ImageResource into
- ImageResourceInternal, the Resource-only class, and
- ImageResourceContent, the Image-only class which manages blink::Image and
ImageResourceObserver.

Most of users of ImageResource, especially those in core/style, are migrated
into ImageResourceContent, because they need image and shouldn't touch resource
loading directly.

https://docs.google.com/document/d/1O-fB83mrE0B_V8gzXNqHgmRLCvstTB4MMi3RnVLr8...

BUG=

CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

to

==========
Currently, ImageResource is
- A part of Resource, because it is a subclass of Resource and highly involved
into internals of Resource such as state control of Resource, and
- A part of Image, as a kind of a wrapper of blink::Image and the entry point of
ImageResourceObserver.
This complicates the ImageResource and causes layering violation.

Also, ImageResource manages timings of Resource- and Image-related callbacks,
which leads to timing dependencies between them, e.g. between
ResourceLoader::didFinishLoading() and <img>'s onload.

This CL splits ImageResource into
- ImageResourceInternal, the Resource-only class, and
- ImageResourceContent, the Image-only class which manages blink::Image and
ImageResourceObserver.

Most of users of ImageResource, especially those in core/style, are migrated
into ImageResourceContent, because they need image and shouldn't touch resource
loading directly.

https://docs.google.com/document/d/1O-fB83mrE0B_V8gzXNqHgmRLCvstTB4MMi3RnVLr8...

(Note that currently Image-part and Resource-part have names "ImageResource" and
"ImageResourceInternal" to make the CL smaller for easier rebase. If the design
and the CL looks good I'll rename them and split them into separate files.)

BUG=667641

CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

[hiroshige, 2016-11-25 10:47:50]

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

[commit-bot: I haz the power, 2016-11-25 10:48:03]

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-11-25 12:43:11]

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

[commit-bot: I haz the power, 2016-11-25 12:43:12]

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

[hiroshige, 2016-11-28 06:31:15]

Description was changed from

==========
Currently, ImageResource is
- A part of Resource, because it is a subclass of Resource and highly involved
into internals of Resource such as state control of Resource, and
- A part of Image, as a kind of a wrapper of blink::Image and the entry point of
ImageResourceObserver.
This complicates the ImageResource and causes layering violation.

Also, ImageResource manages timings of Resource- and Image-related callbacks,
which leads to timing dependencies between them, e.g. between
ResourceLoader::didFinishLoading() and <img>'s onload.

This CL splits ImageResource into
- ImageResourceInternal, the Resource-only class, and
- ImageResourceContent, the Image-only class which manages blink::Image and
ImageResourceObserver.

Most of users of ImageResource, especially those in core/style, are migrated
into ImageResourceContent, because they need image and shouldn't touch resource
loading directly.

https://docs.google.com/document/d/1O-fB83mrE0B_V8gzXNqHgmRLCvstTB4MMi3RnVLr8...

(Note that currently Image-part and Resource-part have names "ImageResource" and
"ImageResourceInternal" to make the CL smaller for easier rebase. If the design
and the CL looks good I'll rename them and split them into separate files.)

BUG=667641

CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

to

==========
Currently, ImageResource is
- A part of Image, as a kind of a wrapper of blink::Image and
  the entry point of ImageResourceObserver, and
- A part of Resource, because it is a subclass of Resource and highly
  involved into internals of Resource such as state control of Resource.
This complicates the ImageResource, causes dependencies and
layering violation between loading- and image-related things, and
complicates reloading logic.

This CL splits ImageResource into two parts:
ImageResource, the Resource-part:
- A subclass of Resource.
ImageResourceContent, the Image-part:
- Manages blink::Image and ImageResourceObserver, and inherits ImageObserver.
- Has limited access to Resource-part (if any) via ImageResourceInfo.

Most of current users of ImageResource, especially those in core/style, are
migrated to use ImageResourceContent and/or
to inherit ImageResourceObserver instead of ResourceClient,
as they want to access Image, not necessarily resource loading.

Design doc:
https://docs.google.com/document/d/1O-fB83mrE0B_V8gzXNqHgmRLCvstTB4MMi3RnVLr8...

BUG=667641

CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2

Note that currently Image-part and Resource-part have names
"ImageResource" and "ImageResourceInternal" to make the CL smaller for easier
rebase.
If the design and the CL looks good I'll rename them and split them into
separate files.
==========

[hiroshige, 2016-11-28 06:33:40]

Description was changed from

==========
Currently, ImageResource is
- A part of Image, as a kind of a wrapper of blink::Image and
  the entry point of ImageResourceObserver, and
- A part of Resource, because it is a subclass of Resource and highly
  involved into internals of Resource such as state control of Resource.
This complicates the ImageResource, causes dependencies and
layering violation between loading- and image-related things, and
complicates reloading logic.

This CL splits ImageResource into two parts:
ImageResource, the Resource-part:
- A subclass of Resource.
ImageResourceContent, the Image-part:
- Manages blink::Image and ImageResourceObserver, and inherits ImageObserver.
- Has limited access to Resource-part (if any) via ImageResourceInfo.

Most of current users of ImageResource, especially those in core/style, are
migrated to use ImageResourceContent and/or
to inherit ImageResourceObserver instead of ResourceClient,
as they want to access Image, not necessarily resource loading.

Design doc:
https://docs.google.com/document/d/1O-fB83mrE0B_V8gzXNqHgmRLCvstTB4MMi3RnVLr8...

BUG=667641

CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2

Note that currently Image-part and Resource-part have names
"ImageResource" and "ImageResourceInternal" to make the CL smaller for easier
rebase.
If the design and the CL looks good I'll rename them and split them into
separate files.
==========

to

==========
Currently, ImageResource is
- A part of Image, as a kind of a wrapper of blink::Image and
  the entry point of ImageResourceObserver, and
- A part of Resource, because it is a subclass of Resource and highly
  involved into internals of Resource such as state control of Resource.
This complicates the ImageResource, causes dependencies and
layering violation between loading- and image-related things, and
complicates reloading logic.

This CL splits ImageResource into two parts:
ImageResource, the Resource-part:
- A subclass of Resource.
ImageResourceContent, the Image-part:
- Manages blink::Image and ImageResourceObserver, and inherits ImageObserver.
- Has limited access to Resource-part (if any) via ImageResourceInfo.

Most of current users of ImageResource, especially those in core/style, are
migrated to use ImageResourceContent and/or
to inherit ImageResourceObserver instead of ResourceClient,
as they want to access Image, not necessarily resource loading.

Design doc:
https://docs.google.com/document/d/1O-fB83mrE0B_V8gzXNqHgmRLCvstTB4MMi3RnVLr8...

In this CL, the Resource-part and the Image-part has 1:1 correspondence that
doesn't change during their lifetime and therefore ImageResource::m_content
is always non-null.
Subsequent CLs will make them detachable.

BUG=667641

CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2

Note that currently Image-part and Resource-part have names
"ImageResource" and "ImageResourceInternal" to make the CL smaller for easier
rebase.
If the design and the CL looks good I'll rename them and split them into
separate files.
==========

[hiroshige, 2016-11-29 07:53:18]

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

[commit-bot: I haz the power, 2016-11-29 07:53:46]

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

[hiroshige, 2016-11-29 08:55:44]

Description was changed from

==========
Currently, ImageResource is
- A part of Image, as a kind of a wrapper of blink::Image and
  the entry point of ImageResourceObserver, and
- A part of Resource, because it is a subclass of Resource and highly
  involved into internals of Resource such as state control of Resource.
This complicates the ImageResource, causes dependencies and
layering violation between loading- and image-related things, and
complicates reloading logic.

This CL splits ImageResource into two parts:
ImageResource, the Resource-part:
- A subclass of Resource.
ImageResourceContent, the Image-part:
- Manages blink::Image and ImageResourceObserver, and inherits ImageObserver.
- Has limited access to Resource-part (if any) via ImageResourceInfo.

Most of current users of ImageResource, especially those in core/style, are
migrated to use ImageResourceContent and/or
to inherit ImageResourceObserver instead of ResourceClient,
as they want to access Image, not necessarily resource loading.

Design doc:
https://docs.google.com/document/d/1O-fB83mrE0B_V8gzXNqHgmRLCvstTB4MMi3RnVLr8...

In this CL, the Resource-part and the Image-part has 1:1 correspondence that
doesn't change during their lifetime and therefore ImageResource::m_content
is always non-null.
Subsequent CLs will make them detachable.

BUG=667641

CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2

Note that currently Image-part and Resource-part have names
"ImageResource" and "ImageResourceInternal" to make the CL smaller for easier
rebase.
If the design and the CL looks good I'll rename them and split them into
separate files.
==========

to

==========
Currently, ImageResource is
- A part of Image, as a kind of a wrapper of blink::Image and
  the entry point of ImageResourceObserver, and
- A part of Resource, because it is a subclass of Resource and highly
  involved into internals of Resource such as state control of Resource.
This complicates the ImageResource, causes dependencies and
layering violation between loading- and image-related things, and
complicates reloading logic.

This CL splits ImageResource into two parts:
ImageResource, the Resource-part:
- A subclass of Resource.
ImageResourceContent, the Image-part:
- Manages blink::Image and ImageResourceObserver, and inherits ImageObserver.
- Has limited access to Resource-part (if any) via ImageResourceInfo.

Most of current users of ImageResource, especially those in core/style, are
migrated to use ImageResourceContent and/or
to inherit ImageResourceObserver instead of ResourceClient,
as they want to access Image, not necessarily resource loading.

Design doc:
https://docs.google.com/document/d/1O-fB83mrE0B_V8gzXNqHgmRLCvstTB4MMi3RnVLr8...

In this CL, the Resource-part and the Image-part has 1:1 correspondence that
doesn't change during their lifetime and therefore ImageResource::m_content
is always non-null.
Subsequent CLs will make them detachable.

Changes in most of the files are replacing ImageResource with
ImageResourceContent.
Notable changes are:

Main part of CL: split ImageResource into two parts:
- core/fetch/ImageResource.h
- core/fetch/ImageResource.cpp
- core/fetch/ImageResourceContent.h
- core/fetch/ImageResourceContent.cpp

StyleImage is mostly migrated to ImageResourceContent, except for the
workaround in CSSImageValue::restoreCachedResourceIfNeeded() (Issue 666214).
StyleImage::cachedImageResourceDeprecated() returns ImageResource only for
that.
- core/style/StyleImage.h
- core/style/StyleFetchedImage.h
- core/style/StyleFetchedImage.cpp
- core/style/StyleFetchedImageSet.h
- core/style/StyleFetchedImageSet.cpp

ImageLoader is mostly migrated to ImageResourceContent, except for
imageResourceForImageDocument() that returns ImageResource only for
ImageDocument.
- core/html/HTMLImageElement.h
- core/html/ImageDocument.cpp
- core/loader/ImageLoader.h
- core/loader/ImageLoader.cpp

A reference to Resource from FrameSerializer is replaced with |mimeType|
String and |hasCacheControlNoStoreHeader| boolean in order to migrate to
ImageResourceContent.
- web/WebFrameSerializer.cpp
- core/frame/FrameSerializer.h
- core/frame/FrameSerializer.cpp

BUG=667641

CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

[hiroshige, 2016-11-29 09:26:16]

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

[commit-bot: I haz the power, 2016-11-29 09:26:36]

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

[hiroshige, 2016-11-29 09:47:29]

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

[commit-bot: I haz the power, 2016-11-29 09:47:51]

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-11-29 11:17:07]

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

[commit-bot: I haz the power, 2016-11-29 11:17:08]

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

[hiroshige, 2016-11-29 12:08:45]

Description was changed from

==========
Currently, ImageResource is
- A part of Image, as a kind of a wrapper of blink::Image and
  the entry point of ImageResourceObserver, and
- A part of Resource, because it is a subclass of Resource and highly
  involved into internals of Resource such as state control of Resource.
This complicates the ImageResource, causes dependencies and
layering violation between loading- and image-related things, and
complicates reloading logic.

This CL splits ImageResource into two parts:
ImageResource, the Resource-part:
- A subclass of Resource.
ImageResourceContent, the Image-part:
- Manages blink::Image and ImageResourceObserver, and inherits ImageObserver.
- Has limited access to Resource-part (if any) via ImageResourceInfo.

Most of current users of ImageResource, especially those in core/style, are
migrated to use ImageResourceContent and/or
to inherit ImageResourceObserver instead of ResourceClient,
as they want to access Image, not necessarily resource loading.

Design doc:
https://docs.google.com/document/d/1O-fB83mrE0B_V8gzXNqHgmRLCvstTB4MMi3RnVLr8...

In this CL, the Resource-part and the Image-part has 1:1 correspondence that
doesn't change during their lifetime and therefore ImageResource::m_content
is always non-null.
Subsequent CLs will make them detachable.

Changes in most of the files are replacing ImageResource with
ImageResourceContent.
Notable changes are:

Main part of CL: split ImageResource into two parts:
- core/fetch/ImageResource.h
- core/fetch/ImageResource.cpp
- core/fetch/ImageResourceContent.h
- core/fetch/ImageResourceContent.cpp

StyleImage is mostly migrated to ImageResourceContent, except for the
workaround in CSSImageValue::restoreCachedResourceIfNeeded() (Issue 666214).
StyleImage::cachedImageResourceDeprecated() returns ImageResource only for
that.
- core/style/StyleImage.h
- core/style/StyleFetchedImage.h
- core/style/StyleFetchedImage.cpp
- core/style/StyleFetchedImageSet.h
- core/style/StyleFetchedImageSet.cpp

ImageLoader is mostly migrated to ImageResourceContent, except for
imageResourceForImageDocument() that returns ImageResource only for
ImageDocument.
- core/html/HTMLImageElement.h
- core/html/ImageDocument.cpp
- core/loader/ImageLoader.h
- core/loader/ImageLoader.cpp

A reference to Resource from FrameSerializer is replaced with |mimeType|
String and |hasCacheControlNoStoreHeader| boolean in order to migrate to
ImageResourceContent.
- web/WebFrameSerializer.cpp
- core/frame/FrameSerializer.h
- core/frame/FrameSerializer.cpp

BUG=667641

CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

to

==========
Currently, ImageResource is
- A part of Image, as a kind of a wrapper of blink::Image and
  the entry point of ImageResourceObserver, and
- A part of Resource, because it is a subclass of Resource and highly
  involved into internals of Resource such as state control of Resource.
This complicates the ImageResource, causes dependencies and
layering violation between loading- and image-related things, and
complicates reloading logic.

This CL splits ImageResource into two parts:
ImageResource, the Resource-part:
- A subclass of Resource.
ImageResourceContent, the Image-part:
- Manages blink::Image and ImageResourceObserver, and inherits ImageObserver.
- Has limited access to Resource-part (if any) via ImageResourceInfo.

Most of current users of ImageResource, especially those in core/style, are
migrated to use ImageResourceContent and/or
to inherit ImageResourceObserver instead of ResourceClient,
as they want to access Image, not necessarily resource loading.

Design doc:
https://docs.google.com/document/d/1O-fB83mrE0B_V8gzXNqHgmRLCvstTB4MMi3RnVLr8...

In this CL, the Resource-part and the Image-part has 1:1 correspondence that
doesn't change during their lifetime and therefore ImageResource::m_content
is always non-null.
Subsequent CLs will make them detachable.

Changes in most of the files are replacing ImageResource with
ImageResourceContent.
Notable changes are:

Main part of CL: split ImageResource into two parts:
- core/fetch/ImageResource.h
- core/fetch/ImageResource.cpp
- core/fetch/ImageResourceContent.h
- core/fetch/ImageResourceContent.cpp

emulateLoadStartedForInspector() hack is now called via ImageResourceContent:
- core/css/CSSImageValue.cpp

ImageLoader is mostly migrated to ImageResourceContent, except for
imageResourceForImageDocument() that returns ImageResource only for
ImageDocument.
- core/html/HTMLImageElement.h
- core/html/ImageDocument.cpp
- core/loader/ImageLoader.h
- core/loader/ImageLoader.cpp

A reference to Resource from FrameSerializer is replaced with |mimeType|
String and |hasCacheControlNoStoreHeader| boolean in order to migrate to
ImageResourceContent.
- web/WebFrameSerializer.cpp
- core/frame/FrameSerializer.h
- core/frame/FrameSerializer.cpp

BUG=667641

CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

[hiroshige, 2016-11-29 12:09:07]

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

[commit-bot: I haz the power, 2016-11-29 12:09:18]

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

[hiroshige, 2016-11-29 12:34:38]

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

[commit-bot: I haz the power, 2016-11-29 12:34:55]

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-11-29 14:57:17]

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

[commit-bot: I haz the power, 2016-11-29 14:57:19]

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

[hiroshige, 2016-11-30 05:12:24]

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

[commit-bot: I haz the power, 2016-11-30 05:13:04]

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-11-30 05:17:17]

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

[commit-bot: I haz the power, 2016-11-30 05:17:18]

Dry run: Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[hiroshige, 2016-11-30 06:12:23]

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

[commit-bot: I haz the power, 2016-11-30 06:13:01]

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

[hiroshige, 2016-11-30 06:46:25]

PTAL.

Suggestion for better class names are welcome.

https://codereview.chromium.org/2469873002/diff/460001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/fetch/ImageResourceContent.cpp (right):

https://codereview.chromium.org/2469873002/diff/460001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/fetch/ImageResourceContent.cpp:155:
m_image->destroyDecodedData();
The UMA is collected in the caller of this method in ImageResource.

https://codereview.chromium.org/2469873002/diff/460001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/fetch/ImageResourceContentInterface.h
(right):

https://codereview.chromium.org/2469873002/diff/460001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/fetch/ImageResourceContentInterface.h:17: class
CORE_EXPORT ImageResourceContentInterface : public GarbageCollectedMixin {
I introduced ImageResourceContentInterface in the hope to
1. Clarify what methods of ImageResourceContent are called from ImageResource
(to help further separation of them), and
2. Remove dependency from ImageResource to ImageResourceContent, and possibly
move ImageResource and ImageResourceContentInterface to platform and
ImageResourceContent to core/loader or somewhere.

However, we anyway need to obtain ImageResourceContent from ImageResource when
we get ImageResource from MemoryCache, so ImageResource need to have references
to ImageResourceContent, therefore the Goal 2. is not directly achieved.
For code simplicity, it might better to merge ImageResourceContentInterface and
ImageResourceContent again to reduce the number of classes.

toyoshim@, do you have any thoughts in terms of platform loading?

https://codereview.chromium.org/2469873002/diff/460001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/loader/ImageLoader.cpp (left):

https://codereview.chromium.org/2469873002/diff/460001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/loader/ImageLoader.cpp:496: if
(resource->resourceError().isAccessCheck()) {
TODO: I'll revert this change in next major rebase of this CL.

https://codereview.chromium.org/2469873002/diff/460001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/loader/ImageLoader.cpp:513: if
(resource->wasCanceled()) {
TODO: This change will be done in https://codereview.chromium.org/2471133007/.

[hiroshige, 2016-12-01 07:12:56]

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

[commit-bot: I haz the power, 2016-12-01 07:13:19]

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-12-01 09:34:07]

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

[commit-bot: I haz the power, 2016-12-01 09:34:08]

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

[kinuko, 2016-12-01 16:40:56]

A few random drive-by comments (not meant to be useful review comments).  Fwiw I
like the general direction.

https://codereview.chromium.org/2469873002/diff/480001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/fetch/ImageResource.h (right):

https://codereview.chromium.org/2469873002/diff/480001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/fetch/ImageResource.h:68: ImageResourceContent*
getRealContent() const;
Having both methods here feels a bit confusing... and I'm a bit puzzled why we
need the Interface class.  Can we get rid of one of them?

https://codereview.chromium.org/2469873002/diff/480001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/fetch/ImageResource.h:68: ImageResourceContent*
getRealContent() const;
...ok I just read the comment about why you introduced it.  I'd +1 to merge them
into a single class at least for now / in this patch. (I'm deferring the final
decision to toyoshim / other reviewers though)

https://codereview.chromium.org/2469873002/diff/480001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/fetch/ImageResourceInfo.h (right):

https://codereview.chromium.org/2469873002/diff/480001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/fetch/ImageResourceInfo.h:23: class CORE_EXPORT
ImageResourceInfo : public GarbageCollectedMixin {
ImageResourceView... or ImageResourceAccessor or something?  I don't have a
great idea but this name feels more like something that is a simple struct held
by ImageResource.

[hiroshige, 2016-12-02 08:51:18]

https://codereview.chromium.org/2469873002/diff/480001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/fetch/ImageResource.h (right):

https://codereview.chromium.org/2469873002/diff/480001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/fetch/ImageResource.h:68: ImageResourceContent*
getRealContent() const;
On 2016/12/01 16:40:55, kinuko wrote:
> ...ok I just read the comment about why you introduced it.  I'd +1 to merge
them
> into a single class at least for now / in this patch. (I'm deferring the final
> decision to toyoshim / other reviewers though)

Discussed with toyoshim@ offline, and we conclude to remove
ImageResourceContentInterface because currently we don't have to clear the
dependencies around ImageResourceContent strictly (as we can move ImageResource
to core/loader/resource/).
I'll merge ImageResourceContentInterface and ImageResourceContent in the next
PatchSet.

[hiroshige, 2016-12-05 06:29:56]

On 2016/12/02 08:51:18, hiroshige wrote:
>
https://codereview.chromium.org/2469873002/diff/480001/third_party/WebKit/Sou...
> File third_party/WebKit/Source/core/fetch/ImageResource.h (right):
> 
>
https://codereview.chromium.org/2469873002/diff/480001/third_party/WebKit/Sou...
> third_party/WebKit/Source/core/fetch/ImageResource.h:68: ImageResourceContent*
> getRealContent() const;
> On 2016/12/01 16:40:55, kinuko wrote:
> > ...ok I just read the comment about why you introduced it.  I'd +1 to merge
> them
> > into a single class at least for now / in this patch. (I'm deferring the
final
> > decision to toyoshim / other reviewers though)
> 
> Discussed with toyoshim@ offline, and we conclude to remove
> ImageResourceContentInterface because currently we don't have to clear the
> dependencies around ImageResourceContent strictly (as we can move
ImageResource
> to core/loader/resource/).
> I'll merge ImageResourceContentInterface and ImageResourceContent in the next
> PatchSet.

Merged ImageResourceContentInterface back to ImageResourceContent.
PTAL.

[hiroshige, 2016-12-05 06:31:42]

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

[commit-bot: I haz the power, 2016-12-05 06:31:53]

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

[kouhei (in TOK), 2016-12-05 07:13:06]

https://codereview.chromium.org/2469873002/diff/520001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/fetch/ImageResource.h (right):

https://codereview.chromium.org/2469873002/diff/520001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/fetch/ImageResource.h:39: // ImageResource is the
resource-related part of image loading.
ImageResource implements blink::Resource interface for loading images.

https://codereview.chromium.org/2469873002/diff/520001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/fetch/ImageResource.h:56: // Use
ImageResourceContent::fetch() unless ImageResource is required.
Can we make this private && friend ImageResourceContent?

https://codereview.chromium.org/2469873002/diff/520001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/fetch/ImageResourceContent.h (right):

https://codereview.chromium.org/2469873002/diff/520001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/fetch/ImageResourceContent.h:31: //
ImageResourceContent is the image-related part of image loading that
ImageResourceContent is a container that holds fetch result of a ImageResource
in a decoded form.
Clients which use the fetched images should hold onto this class instead of
ImageResource.

[hiroshige, 2016-12-05 08:44:12]

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

[commit-bot: I haz the power, 2016-12-05 08:44:27]

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

[yhirano, 2016-12-05 09:02:04]

https://codereview.chromium.org/2469873002/diff/520001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/css/CSSImageValue.cpp (right):

https://codereview.chromium.org/2469873002/diff/520001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/css/CSSImageValue.cpp:99: if
(ImageResourceContent* cachedResource = m_cachedImage->cachedImage())
cachedResourceContent? I'm not sure if you want to change local names in this
CL, but you are renaming one in the above function and being consistent (at
least in a file) is good...

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

https://codereview.chromium.org/2469873002/diff/520001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/fetch/ImageResource.cpp:28: #include
"core/fetch/ImageResourceObserver.h"
Is this needed?

https://codereview.chromium.org/2469873002/diff/520001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/fetch/ImageResource.cpp:40: #include
"wtf/HashCountedSet.h"
Not needed?

https://codereview.chromium.org/2469873002/diff/520001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/fetch/ImageResource.cpp:42: #include
"wtf/Vector.h"
Not needed?

https://codereview.chromium.org/2469873002/diff/520001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/fetch/ImageResource.cpp:127:
Member<ImageResource> m_resource;
[optional] const Member<ImageResource> m_resource;

https://codereview.chromium.org/2469873002/diff/520001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/fetch/ImageResource.cpp:374: clearData();
+setEncodedSize(0)?

https://codereview.chromium.org/2469873002/diff/520001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/fetch/ImageResource.cpp:374: clearData();
Resource::error calls clearData().

https://codereview.chromium.org/2469873002/diff/520001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/fetch/ImageResource.h (right):

https://codereview.chromium.org/2469873002/diff/520001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/fetch/ImageResource.h:28: #include
"core/fetch/Resource.h"
+platform/Timer.h
+platform/heap/Handle.h

https://codereview.chromium.org/2469873002/diff/520001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/fetch/ImageResource.h:37: class
ImageResourceContent;
alphabetical order?

https://codereview.chromium.org/2469873002/diff/520001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/fetch/ImageResource.h:110: // Only for
ImageResourceInfoImpl.
Can you move these functions declarations below the constructor?

https://codereview.chromium.org/2469873002/diff/520001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/fetch/ImageResource.h:142:
Member<ImageResourceContent> m_content;
[optional] const Member<ImageResourceContent> m_content;

https://codereview.chromium.org/2469873002/diff/520001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/fetch/ImageResource.h:144: float
m_devicePixelRatioHeaderValue;
Is it desirable to move this member variable to ImageResourceContent and update
the value in ImageResource::responseReceived?

https://codereview.chromium.org/2469873002/diff/520001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/fetch/ImageResource.h:149: bool
m_hasDevicePixelRatioHeaderValue;
ditto

https://codereview.chromium.org/2469873002/diff/520001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/fetch/ImageResourceContent.cpp (right):

https://codereview.chromium.org/2469873002/diff/520001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/fetch/ImageResourceContent.cpp:18: #include
"wtf/HashCountedSet.h"
This should be moved to the header file.

https://codereview.chromium.org/2469873002/diff/520001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/fetch/ImageResourceContent.cpp:63: class
ResourceFetcher;
Not needed?

https://codereview.chromium.org/2469873002/diff/520001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/fetch/ImageResourceContent.h (right):

https://codereview.chromium.org/2469873002/diff/520001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/fetch/ImageResourceContent.h:17: #include
"platform/weborigin/KURL.h"
+wtf/HashCountedSet.h

https://codereview.chromium.org/2469873002/diff/520001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/fetch/ImageResourceInfo.h (right):

https://codereview.chromium.org/2469873002/diff/520001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/fetch/ImageResourceInfo.h:11: #include
"platform/weborigin/KURL.h"
+wtf/Forward.h

https://codereview.chromium.org/2469873002/diff/520001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/fetch/ImageResourceInfo.h:36: virtual bool
isAccessAllowed(
Please use enum rather than boolean for a public method parameter.

https://codereview.chromium.org/2469873002/diff/520001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/fetch/ImageResourceInfo.h:42:
DEFINE_INLINE_VIRTUAL_TRACE() {}
[optional] can you place this function at the bottom?

https://codereview.chromium.org/2469873002/diff/520001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/fetch/ImageResourceInfo.h:44: virtual void
decodeError(bool allDataReceived) {}
Can you tell me why these functions have the default implementations unlike
above ones?

https://codereview.chromium.org/2469873002/diff/520001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/fetch/ImageResourceInfo.h:44: virtual void
decodeError(bool allDataReceived) {}
It might be good to have comments for non-trivial public methods.

https://codereview.chromium.org/2469873002/diff/520001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/frame/FrameSerializer.h (right):

https://codereview.chromium.org/2469873002/diff/520001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/frame/FrameSerializer.h:90: virtual bool
shouldSkipResource(bool hasCacheControlNoStoreHeader) {
Can you use enum rather than a boolean parameter?

[hiroshige, 2016-12-05 09:11:19]

https://codereview.chromium.org/2469873002/diff/520001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/fetch/ImageResource.h (right):

https://codereview.chromium.org/2469873002/diff/520001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/fetch/ImageResource.h:39: // ImageResource is the
resource-related part of image loading.
On 2016/12/05 07:13:06, kouhei wrote:
> ImageResource implements blink::Resource interface for loading images.

Done.

https://codereview.chromium.org/2469873002/diff/520001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/fetch/ImageResource.h:56: // Use
ImageResourceContent::fetch() unless ImageResource is required.
On 2016/12/05 07:13:06, kouhei wrote:
> Can we make this private && friend ImageResourceContent?

Currently No, because this is used from e.g. DocumentLoader.
I'd like to clean up related code so that this can be private in the near
future, but not in this CL.

https://codereview.chromium.org/2469873002/diff/520001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/fetch/ImageResourceContent.h (right):

https://codereview.chromium.org/2469873002/diff/520001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/fetch/ImageResourceContent.h:31: //
ImageResourceContent is the image-related part of image loading that
On 2016/12/05 07:13:06, kouhei wrote:
> ImageResourceContent is a container that holds fetch result of a ImageResource
> in a decoded form.
> Clients which use the fetched images should hold onto this class instead of
> ImageResource.

Done.

[kouhei (in TOK), 2016-12-05 09:22:05]

https://codereview.chromium.org/2469873002/diff/520001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/css/CSSImageValue.cpp (right):

https://codereview.chromium.org/2469873002/diff/520001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/css/CSSImageValue.cpp:99: if
(ImageResourceContent* cachedResource = m_cachedImage->cachedImage())
On 2016/12/05 09:02:03, yhirano wrote:
> cachedResourceContent? I'm not sure if you want to change local names in this
> CL, but you are renaming one in the above function and being consistent (at
> least in a file) is good...
FYI: cachedResource is an obsolete name from the ancient age where Resource was
called CachedResource.

I also think "content" is appropriate here.

https://codereview.chromium.org/2469873002/diff/520001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/fetch/ImageResource.h (right):

https://codereview.chromium.org/2469873002/diff/520001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/fetch/ImageResource.h:56: // Use
ImageResourceContent::fetch() unless ImageResource is required.
On 2016/12/05 09:11:19, hiroshige wrote:
> On 2016/12/05 07:13:06, kouhei wrote:
> > Can we make this private && friend ImageResourceContent?
> 
> Currently No, because this is used from e.g. DocumentLoader.
> I'd like to clean up related code so that this can be private in the near
> future, but not in this CL.

Optional: Please consider adding TODO(hiroshige) to make this private.

[hiroshige, 2016-12-05 10:17:34]

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

[commit-bot: I haz the power, 2016-12-05 10:17:47]

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

[kouhei (in TOK), 2016-12-05 11:32:55]

OWNER lgtm from myside % yhirano comments

[commit-bot: I haz the power, 2016-12-05 12:56:07]

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

[commit-bot: I haz the power, 2016-12-05 12:56:08]

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

[hiroshige, 2016-12-06 04:27:16]

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

[commit-bot: I haz the power, 2016-12-06 04:27:42]

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-12-06 05:16:21]

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

[commit-bot: I haz the power, 2016-12-06 05:16:23]

Dry run: Try jobs failed on following builders:
  cast_shell_android on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...)

[hiroshige, 2016-12-06 09:29:07]

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

[commit-bot: I haz the power, 2016-12-06 09:29:22]

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

[hiroshige, 2016-12-06 09:32:51]

Reflected comments.

Also, PatchSet 30 fixes a bug:
Before PatchSet 30, ImageResource::error() clears ImageResourceContent's image
by updateImage(), but it caused decoded error.
PatchSet 30 introduces clearImageAndNotifyObservers() and expose
ImageResourceContent::clearImage() and use them in ImageResource to keep the
current behavior.
(They are conceptually similar to updateImage() but I created separate classes
not to increase parameters of updateImage() at this point)

https://codereview.chromium.org/2469873002/diff/520001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/css/CSSImageValue.cpp (right):

https://codereview.chromium.org/2469873002/diff/520001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/css/CSSImageValue.cpp:99: if
(ImageResourceContent* cachedResource = m_cachedImage->cachedImage())
On 2016/12/05 09:22:04, kouhei wrote:
> On 2016/12/05 09:02:03, yhirano wrote:
> > cachedResourceContent? I'm not sure if you want to change local names in
this
> > CL, but you are renaming one in the above function and being consistent (at
> > least in a file) is good...
> FYI: cachedResource is an obsolete name from the ancient age where Resource
was
> called CachedResource.
> 
> I also think "content" is appropriate here.

I agree the names should be fixed.
I'll rename the names in a separate CL and in this CL I reverted the name change
above and added a TODO to rename local variable names.
(Currently we already have a mixture of names like |image|, |cachedImage|,
|resource|, |imageResource|, |cachedResource| etc. for Resource, ImageResource,
StyleImage, etc. etc. We might need a consistent rules for naming such things to
rename member functions like cachedImage())

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

https://codereview.chromium.org/2469873002/diff/520001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/fetch/ImageResource.cpp:28: #include
"core/fetch/ImageResourceObserver.h"
On 2016/12/05 09:02:03, yhirano wrote:
> Is this needed?

Done.

https://codereview.chromium.org/2469873002/diff/520001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/fetch/ImageResource.cpp:40: #include
"wtf/HashCountedSet.h"
On 2016/12/05 09:02:03, yhirano wrote:
> Not needed?

Done.

https://codereview.chromium.org/2469873002/diff/520001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/fetch/ImageResource.cpp:42: #include
"wtf/Vector.h"
On 2016/12/05 09:02:03, yhirano wrote:
> Not needed?

Done.

https://codereview.chromium.org/2469873002/diff/520001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/fetch/ImageResource.cpp:127:
Member<ImageResource> m_resource;
On 2016/12/05 09:02:03, yhirano wrote:
> [optional] const Member<ImageResource> m_resource;

Done.

https://codereview.chromium.org/2469873002/diff/520001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/fetch/ImageResource.cpp:374: clearData();
On 2016/12/05 09:02:03, yhirano wrote:
> Resource::error calls clearData().

Done.

https://codereview.chromium.org/2469873002/diff/520001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/fetch/ImageResource.cpp:374: clearData();
On 2016/12/05 09:02:03, yhirano wrote:
> +setEncodedSize(0)?

Done to keep the current behavior.
But I'm not so sure whether we should call setEncodedSize(0) here.
I added a TODO.

https://codereview.chromium.org/2469873002/diff/520001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/fetch/ImageResource.h (right):

https://codereview.chromium.org/2469873002/diff/520001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/fetch/ImageResource.h:28: #include
"core/fetch/Resource.h"
On 2016/12/05 09:02:03, yhirano wrote:
> +platform/Timer.h
> +platform/heap/Handle.h

Done.

https://codereview.chromium.org/2469873002/diff/520001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/fetch/ImageResource.h:37: class
ImageResourceContent;
On 2016/12/05 09:02:03, yhirano wrote:
> alphabetical order?

Done.

https://codereview.chromium.org/2469873002/diff/520001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/fetch/ImageResource.h:56: // Use
ImageResourceContent::fetch() unless ImageResource is required.
On 2016/12/05 09:22:05, kouhei wrote:
> On 2016/12/05 09:11:19, hiroshige wrote:
> > On 2016/12/05 07:13:06, kouhei wrote:
> > > Can we make this private && friend ImageResourceContent?
> > 
> > Currently No, because this is used from e.g. DocumentLoader.
> > I'd like to clean up related code so that this can be private in the near
> > future, but not in this CL.
> 
> Optional: Please consider adding TODO(hiroshige) to make this private.

Done.

https://codereview.chromium.org/2469873002/diff/520001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/fetch/ImageResource.h:110: // Only for
ImageResourceInfoImpl.
On 2016/12/05 09:02:03, yhirano wrote:
> Can you move these functions declarations below the constructor?

Done.

https://codereview.chromium.org/2469873002/diff/520001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/fetch/ImageResource.h:142:
Member<ImageResourceContent> m_content;
On 2016/12/05 09:02:03, yhirano wrote:
> [optional] const Member<ImageResourceContent> m_content;
I'll keep |m_content| non-const because it will be non-const soon after this CL.

https://codereview.chromium.org/2469873002/diff/520001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/fetch/ImageResource.h:144: float
m_devicePixelRatioHeaderValue;
On 2016/12/05 09:02:03, yhirano wrote:
> Is it desirable to move this member variable to ImageResourceContent and
update
> the value in ImageResource::responseReceived?

Moving |m_devicePixelRatioHeaderValue| to ImageResourceContent and make
ImageResource to update it is desirable, because it is more natural (the members
looks image-related) and removes the corresponding interfaces in
ImageResourceInfo.

However, I'd like to update it in updateImage(), not in a new
ImageResourceConent method called in responseReceived(), because I'd like to
make updateImage() the single point of state change of ImageResourceContent
caused by ImageResource (we don't have updateImage() call in
responseReceived()), to simplify timing constraints between ImageResource and
its content.
Older Patch Set (e.g. Patch Set 7) does this: updateImage() has
|devicePixelRatioHeaderValue| argument, and ImageResourceContent updates its
|m_devicePixelRatioHeaderValue| in updateImage().
I felt it is over-complex at this time, but I'll re-apply that change if it
looks good. (Added a TODO anyway in the latest PatchSet).

https://codereview.chromium.org/2469873002/diff/520001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/fetch/ImageResource.h:149: bool
m_hasDevicePixelRatioHeaderValue;
On 2016/12/05 09:02:03, yhirano wrote:
> ditto

ditto.

https://codereview.chromium.org/2469873002/diff/520001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/fetch/ImageResourceContent.cpp (right):

https://codereview.chromium.org/2469873002/diff/520001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/fetch/ImageResourceContent.cpp:18: #include
"wtf/HashCountedSet.h"
On 2016/12/05 09:02:03, yhirano wrote:
> This should be moved to the header file.

Done.

https://codereview.chromium.org/2469873002/diff/520001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/fetch/ImageResourceContent.cpp:63: class
ResourceFetcher;
On 2016/12/05 09:02:03, yhirano wrote:
> Not needed?

Done.

https://codereview.chromium.org/2469873002/diff/520001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/fetch/ImageResourceContent.h (right):

https://codereview.chromium.org/2469873002/diff/520001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/fetch/ImageResourceContent.h:17: #include
"platform/weborigin/KURL.h"
On 2016/12/05 09:02:03, yhirano wrote:
> +wtf/HashCountedSet.h

Done.

https://codereview.chromium.org/2469873002/diff/520001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/fetch/ImageResourceInfo.h (right):

https://codereview.chromium.org/2469873002/diff/520001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/fetch/ImageResourceInfo.h:11: #include
"platform/weborigin/KURL.h"
On 2016/12/05 09:02:04, yhirano wrote:
> +wtf/Forward.h

Done.

https://codereview.chromium.org/2469873002/diff/520001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/fetch/ImageResourceInfo.h:36: virtual bool
isAccessAllowed(
On 2016/12/05 09:02:03, yhirano wrote:
> Please use enum rather than boolean for a public method parameter.

Done.

https://codereview.chromium.org/2469873002/diff/520001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/fetch/ImageResourceInfo.h:42:
DEFINE_INLINE_VIRTUAL_TRACE() {}
On 2016/12/05 09:02:03, yhirano wrote:
> [optional] can you place this function at the bottom?

Done.

https://codereview.chromium.org/2469873002/diff/520001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/fetch/ImageResourceInfo.h:44: virtual void
decodeError(bool allDataReceived) {}
On 2016/12/05 09:02:03, yhirano wrote:
> Can you tell me why these functions have the default implementations unlike
> above ones?

I made e.g. url() abstract because we need a dummy KURL object if there is no
corresponding Resource (which is in NullImageResourceInfo).
I don't have any reason to have default implementation here, so I made them
abstract in the latest PatchSet.

https://codereview.chromium.org/2469873002/diff/520001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/fetch/ImageResourceInfo.h:44: virtual void
decodeError(bool allDataReceived) {}
On 2016/12/05 09:02:03, yhirano wrote:
> It might be good to have comments for non-trivial public methods.

The most thing I want to say for these methods is: Don't use, remove them.
So added such comments.

Although they are public, the members inherit the tight dependencies to and bugs
of the internals of ImageResource, and all of the non-const methods need care
when used.

https://codereview.chromium.org/2469873002/diff/520001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/frame/FrameSerializer.h (right):

https://codereview.chromium.org/2469873002/diff/520001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/frame/FrameSerializer.h:90: virtual bool
shouldSkipResource(bool hasCacheControlNoStoreHeader) {
On 2016/12/05 09:02:04, yhirano wrote:
> Can you use enum rather than a boolean parameter?

Done, but better name might be needed.

[commit-bot: I haz the power, 2016-12-06 10:05:50]

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

[commit-bot: I haz the power, 2016-12-06 10:05:52]

Dry run: Try jobs failed on following builders:
  cast_shell_android on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...)

[hiroshige, 2016-12-07 07:10:45]

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

[commit-bot: I haz the power, 2016-12-07 07:11: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-12-07 07:50:18]

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

[commit-bot: I haz the power, 2016-12-07 07:50:22]

Dry run: Try jobs failed on following builders:
  cast_shell_android on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...)

[Nate Chapin, 2016-12-07 20:27:28]

I only looked at the fetch/ changes, but I'm liking this. Definitely still some
untangling to do, but a great start! LGTM w/nits

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

https://codereview.chromium.org/2469873002/diff/620001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/fetch/ImageResource.cpp:287: if
(!getContent()->isSizeAvailable() ||
Should this if() block be a helper ImageResourceContent instead?

https://codereview.chromium.org/2469873002/diff/620001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/fetch/ImageResourceContent.cpp (right):

https://codereview.chromium.org/2469873002/diff/620001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/fetch/ImageResourceContent.cpp:80: ImageResource*
resource = ImageResource::fetch(request, fetcher);
This feels like a layering violation, and is (I think) the only ImageResource
reference in ImageResourceContet. Add a TODO to get rid of it?

https://codereview.chromium.org/2469873002/diff/620001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/fetch/ImageResourceContent.h (right):

https://codereview.chromium.org/2469873002/diff/620001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/fetch/ImageResourceContent.h:113: void
setImageResourceInfo(ImageResourceInfo*);
Any reason this is a separate method instead of taking an ImageResourceInfo* in
the constructor?

[yhirano, 2016-12-08 04:17:58]

lgtm

https://codereview.chromium.org/2469873002/diff/620001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/fetch/ImageResourceContent.cpp (right):

https://codereview.chromium.org/2469873002/diff/620001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/fetch/ImageResourceContent.cpp:266: inline
PassRefPtr<Image> ImageResourceContent::createImage() {
Is this inline needed? Ditto below.

[hiroshige, 2016-12-09 07:00:52]

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

[commit-bot: I haz the power, 2016-12-09 07:01:13]

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-12-09 08:51:12]

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

[commit-bot: I haz the power, 2016-12-09 08:51:14]

Dry run: This issue passed the CQ dry run.

[kinuko, 2016-12-09 10:42:22]

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

[kinuko, 2016-12-09 10:42:24]

rs-ish lgtm % nits

https://codereview.chromium.org/2469873002/diff/660001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/fetch/ImageResource.h (right):

https://codereview.chromium.org/2469873002/diff/660001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/fetch/ImageResource.h:69: ImageResourceContent*
getContent() const;
nit: avoid returning non-const pointer from const method

https://codereview.chromium.org/2469873002/diff/660001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/fetch/ImageResourceContent.h (right):

https://codereview.chromium.org/2469873002/diff/660001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/fetch/ImageResourceContent.h:55: blink::Image*
getImage() const;
ditto, avoid returning non-const ptr from const method

https://codereview.chromium.org/2469873002/diff/660001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/fetch/ImageResourceContent.h:135:
ImageResourceContent(PassRefPtr<blink::Image> = nullptr);
explicit

[hiroshige, 2016-12-09 22:50:39]

Addressed comments from yhirano@ and japhet@.

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

https://codereview.chromium.org/2469873002/diff/620001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/fetch/ImageResource.cpp:287: if
(!getContent()->isSizeAvailable() ||
On 2016/12/07 20:27:28, Nate Chapin (slow until Jan 3) wrote:
> Should this if() block be a helper ImageResourceContent instead?

I'd like to call updateImage() directly here, rather than moving this whole
block into ImageResourceContent, in order to
- Make updateImage() a single point of modification of ImageResourceContent
(directed by ImageResource) as much as possible, and
- Keep the mental model that "ImageResource just throttles the call of
updateImage() in Lines 283--322" and thus keep the control logic in
ImageResource.

Alternatively, I also thought if we can move the whole of throttling mechanism
into ImageResourceContent (i.e. Move the most of Lines 283--322 as well as the
timer). However, it requires references to Resource::m_data from
ImageResourceContent if we delay updateImage() until flushImageIfNeeded(), which
is not clean.

I expect we might be able to move the throttling logic to ImageResource when we
further decouples ImageResource and ImageResourceContent in the future (probably
it will be after we remove EventSenders in ImageLoader).

https://codereview.chromium.org/2469873002/diff/620001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/fetch/ImageResourceContent.cpp (right):

https://codereview.chromium.org/2469873002/diff/620001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/fetch/ImageResourceContent.cpp:80: ImageResource*
resource = ImageResource::fetch(request, fetcher);
On 2016/12/07 20:27:28, Nate Chapin (slow until Jan 3) wrote:
> This feels like a layering violation, and is (I think) the only ImageResource
> reference in ImageResourceContet. Add a TODO to get rid of it?

Done.

https://codereview.chromium.org/2469873002/diff/620001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/fetch/ImageResourceContent.cpp:266: inline
PassRefPtr<Image> ImageResourceContent::createImage() {
On 2016/12/08 04:17:58, yhirano wrote:
> Is this inline needed? Ditto below.

Removed.

https://codereview.chromium.org/2469873002/diff/620001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/fetch/ImageResourceContent.h (right):

https://codereview.chromium.org/2469873002/diff/620001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/fetch/ImageResourceContent.h:113: void
setImageResourceInfo(ImageResourceInfo*);
On 2016/12/07 20:27:28, Nate Chapin (slow until Jan 3) wrote:
> Any reason this is a separate method instead of taking an ImageResourceInfo*
in
> the constructor?

If we have ImageResourceInfo* in the constructor, we first create ImageResource
and ImageResourceInfo, and then create ImageResourceContent.

However, I think this should be in a reverse order:
User of loading code first creates an ImageResourceContent that it wants to
obtain, and then asks core/fetch to start loading, i.e. to create a
corresponding ImageResource.
Therefore this CL first creates ImageResourceContent, then creates
ImageResource, and then call setImageResourceInfo().

(Also in https://codereview.chromium.org/2527353002/ |m_info| is updated in LoFi
reloading, and thus setImageResourceInfo() is called for second time)

[hiroshige, 2016-12-11 17:57:17]

Patchset #35 (id:680001) has been deleted

[hiroshige, 2016-12-11 19:11:38]

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

[commit-bot: I haz the power, 2016-12-11 19:11:55]

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

[hiroshige, 2016-12-11 19:12:29]

Patchset #35 (id:700001) has been deleted

[hiroshige, 2016-12-11 19:17:02]

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

https://codereview.chromium.org/2469873002/diff/620001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/fetch/ImageResource.cpp:287: if
(!getContent()->isSizeAvailable() ||
On 2016/12/09 22:50:39, hiroshige wrote:
> On 2016/12/07 20:27:28, Nate Chapin (slow until Jan 3) wrote:
> > Should this if() block be a helper ImageResourceContent instead?
> 
> I'd like to call updateImage() directly here, rather than moving this whole
> block into ImageResourceContent, in order to
> - Make updateImage() a single point of modification of ImageResourceContent
> (directed by ImageResource) as much as possible, and
> - Keep the mental model that "ImageResource just throttles the call of
> updateImage() in Lines 283--322" and thus keep the control logic in
> ImageResource.
> 
> Alternatively, I also thought if we can move the whole of throttling mechanism
> into ImageResourceContent (i.e. Move the most of Lines 283--322 as well as the
> timer). However, it requires references to Resource::m_data from
> ImageResourceContent if we delay updateImage() until flushImageIfNeeded(),
which
> is not clean.
> 
> I expect we might be able to move the throttling logic to ImageResource when
we
> further decouples ImageResource and ImageResourceContent in the future
(probably
> it will be after we remove EventSenders in ImageLoader).

I moved only the |if| conditional expression to ImageResourceContent,
- to partially handle this japhet@'s comment, and
- to remove getImage() call from ImageResource that was (and should be)
|m_image|.

https://codereview.chromium.org/2469873002/diff/660001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/fetch/ImageResource.h (right):

https://codereview.chromium.org/2469873002/diff/660001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/fetch/ImageResource.h:69: ImageResourceContent*
getContent() const;
On 2016/12/09 10:42:24, kinuko wrote:
> nit: avoid returning non-const pointer from const method

Done.

https://codereview.chromium.org/2469873002/diff/660001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/fetch/ImageResourceContent.h (right):

https://codereview.chromium.org/2469873002/diff/660001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/fetch/ImageResourceContent.h:55: blink::Image*
getImage() const;
On 2016/12/09 10:42:24, kinuko wrote:
> ditto, avoid returning non-const ptr from const method

I found ImageResource already have potentials for this kind of const
inconsistency.
For example, in ImageResource::resourceBuffer() const,
|m_image->data()| is called, where data() is non-const method.
Replacing |m_image| with getImage() made this inconsistency explicit.

BTW, I also found that I shouldn't have replaced |m_image| with |getImage()|,
because ImageResourceContent::getImage() is NOT a simple getter of |m_image|.
Two callsites of getImage() are affected (ImageResource::resourceBuffer() and
ImageResource::appendData()).

In PatchSet 35,
- I made getImage() non-const method.
- I moved the blocks containing the affected getImage() callsites to
ImageResourceContent, and reverted them back to |m_image|.
- I made ImageResourceContent::isAccessAllowed() non-const, because it calls
getImage(). (ImageResource::isAccessAllowed() was non-const before this CL)

This doesn't make things worse, just makes the problem implicit again though.

https://codereview.chromium.org/2469873002/diff/660001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/fetch/ImageResourceContent.h:135:
ImageResourceContent(PassRefPtr<blink::Image> = nullptr);
On 2016/12/09 10:42:24, kinuko wrote:
> explicit

Done.

[commit-bot: I haz the power, 2016-12-11 19:19:47]

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

[commit-bot: I haz the power, 2016-12-11 19:19:48]

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

[hiroshige, 2016-12-11 19:30:58]

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

[commit-bot: I haz the power, 2016-12-11 19:31:06]

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-12-11 21:13:02]

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

[commit-bot: I haz the power, 2016-12-11 21:13:03]

Dry run: This issue passed the CQ dry run.

[hiroshige, 2016-12-12 09:27:26]

The CQ bit was checked by hiroshige@chromium.org

[hiroshige, 2016-12-12 09:27:27]

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

[commit-bot: I haz the power, 2016-12-12 09:27:52]

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-12-12 09:35:45]

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

[commit-bot: I haz the power, 2016-12-12 09:35:48]

Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[hiroshige, 2016-12-12 09:44:27]

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

[hiroshige, 2016-12-12 09:44:29]

+mkwst@, could you take a look at, especially as a modules/ OWNER?

[Mike West, 2016-12-12 13:57:26]

modules LGTM.

I only skimmed the rest; it looks pretty reasonable. If the bots are still
happy, then landing this to get us further down the refactoring path sgtm.

[hiroshige, 2016-12-12 14:19:31]

The CQ bit was checked by hiroshige@chromium.org

[commit-bot: I haz the power, 2016-12-12 14:19:52]

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-12-12 14:25:58]

CQ is committing da patch.

Bot data: {"patchset_id": 740001, "attempt_start_ts": 1481552371354380,
"parent_rev": "8cfd31343366b0b905a1965fe07a1ef50413ebf3", "commit_rev":
"f67316984947e6c049de69c8490341aa2654d243"}

[commit-bot: I haz the power, 2016-12-12 14:26:29]

Description was changed from

==========
Currently, ImageResource is
- A part of Image, as a kind of a wrapper of blink::Image and
  the entry point of ImageResourceObserver, and
- A part of Resource, because it is a subclass of Resource and highly
  involved into internals of Resource such as state control of Resource.
This complicates the ImageResource, causes dependencies and
layering violation between loading- and image-related things, and
complicates reloading logic.

This CL splits ImageResource into two parts:
ImageResource, the Resource-part:
- A subclass of Resource.
ImageResourceContent, the Image-part:
- Manages blink::Image and ImageResourceObserver, and inherits ImageObserver.
- Has limited access to Resource-part (if any) via ImageResourceInfo.

Most of current users of ImageResource, especially those in core/style, are
migrated to use ImageResourceContent and/or
to inherit ImageResourceObserver instead of ResourceClient,
as they want to access Image, not necessarily resource loading.

Design doc:
https://docs.google.com/document/d/1O-fB83mrE0B_V8gzXNqHgmRLCvstTB4MMi3RnVLr8...

In this CL, the Resource-part and the Image-part has 1:1 correspondence that
doesn't change during their lifetime and therefore ImageResource::m_content
is always non-null.
Subsequent CLs will make them detachable.

Changes in most of the files are replacing ImageResource with
ImageResourceContent.
Notable changes are:

Main part of CL: split ImageResource into two parts:
- core/fetch/ImageResource.h
- core/fetch/ImageResource.cpp
- core/fetch/ImageResourceContent.h
- core/fetch/ImageResourceContent.cpp

emulateLoadStartedForInspector() hack is now called via ImageResourceContent:
- core/css/CSSImageValue.cpp

ImageLoader is mostly migrated to ImageResourceContent, except for
imageResourceForImageDocument() that returns ImageResource only for
ImageDocument.
- core/html/HTMLImageElement.h
- core/html/ImageDocument.cpp
- core/loader/ImageLoader.h
- core/loader/ImageLoader.cpp

A reference to Resource from FrameSerializer is replaced with |mimeType|
String and |hasCacheControlNoStoreHeader| boolean in order to migrate to
ImageResourceContent.
- web/WebFrameSerializer.cpp
- core/frame/FrameSerializer.h
- core/frame/FrameSerializer.cpp

BUG=667641

CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

to

==========
Currently, ImageResource is
- A part of Image, as a kind of a wrapper of blink::Image and
  the entry point of ImageResourceObserver, and
- A part of Resource, because it is a subclass of Resource and highly
  involved into internals of Resource such as state control of Resource.
This complicates the ImageResource, causes dependencies and
layering violation between loading- and image-related things, and
complicates reloading logic.

This CL splits ImageResource into two parts:
ImageResource, the Resource-part:
- A subclass of Resource.
ImageResourceContent, the Image-part:
- Manages blink::Image and ImageResourceObserver, and inherits ImageObserver.
- Has limited access to Resource-part (if any) via ImageResourceInfo.

Most of current users of ImageResource, especially those in core/style, are
migrated to use ImageResourceContent and/or
to inherit ImageResourceObserver instead of ResourceClient,
as they want to access Image, not necessarily resource loading.

Design doc:
https://docs.google.com/document/d/1O-fB83mrE0B_V8gzXNqHgmRLCvstTB4MMi3RnVLr8...

In this CL, the Resource-part and the Image-part has 1:1 correspondence that
doesn't change during their lifetime and therefore ImageResource::m_content
is always non-null.
Subsequent CLs will make them detachable.

Changes in most of the files are replacing ImageResource with
ImageResourceContent.
Notable changes are:

Main part of CL: split ImageResource into two parts:
- core/fetch/ImageResource.h
- core/fetch/ImageResource.cpp
- core/fetch/ImageResourceContent.h
- core/fetch/ImageResourceContent.cpp

emulateLoadStartedForInspector() hack is now called via ImageResourceContent:
- core/css/CSSImageValue.cpp

ImageLoader is mostly migrated to ImageResourceContent, except for
imageResourceForImageDocument() that returns ImageResource only for
ImageDocument.
- core/html/HTMLImageElement.h
- core/html/ImageDocument.cpp
- core/loader/ImageLoader.h
- core/loader/ImageLoader.cpp

A reference to Resource from FrameSerializer is replaced with |mimeType|
String and |hasCacheControlNoStoreHeader| boolean in order to migrate to
ImageResourceContent.
- web/WebFrameSerializer.cpp
- core/frame/FrameSerializer.h
- core/frame/FrameSerializer.cpp

BUG=667641

CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2

Review-Url: https://codereview.chromium.org/2469873002
==========

[commit-bot: I haz the power, 2016-12-12 14:26:32]

Committed patchset #36 (id:740001)

[commit-bot: I haz the power, 2016-12-12 15:12:06]

Description was changed from

==========
Currently, ImageResource is
- A part of Image, as a kind of a wrapper of blink::Image and
  the entry point of ImageResourceObserver, and
- A part of Resource, because it is a subclass of Resource and highly
  involved into internals of Resource such as state control of Resource.
This complicates the ImageResource, causes dependencies and
layering violation between loading- and image-related things, and
complicates reloading logic.

This CL splits ImageResource into two parts:
ImageResource, the Resource-part:
- A subclass of Resource.
ImageResourceContent, the Image-part:
- Manages blink::Image and ImageResourceObserver, and inherits ImageObserver.
- Has limited access to Resource-part (if any) via ImageResourceInfo.

Most of current users of ImageResource, especially those in core/style, are
migrated to use ImageResourceContent and/or
to inherit ImageResourceObserver instead of ResourceClient,
as they want to access Image, not necessarily resource loading.

Design doc:
https://docs.google.com/document/d/1O-fB83mrE0B_V8gzXNqHgmRLCvstTB4MMi3RnVLr8...

In this CL, the Resource-part and the Image-part has 1:1 correspondence that
doesn't change during their lifetime and therefore ImageResource::m_content
is always non-null.
Subsequent CLs will make them detachable.

Changes in most of the files are replacing ImageResource with
ImageResourceContent.
Notable changes are:

Main part of CL: split ImageResource into two parts:
- core/fetch/ImageResource.h
- core/fetch/ImageResource.cpp
- core/fetch/ImageResourceContent.h
- core/fetch/ImageResourceContent.cpp

emulateLoadStartedForInspector() hack is now called via ImageResourceContent:
- core/css/CSSImageValue.cpp

ImageLoader is mostly migrated to ImageResourceContent, except for
imageResourceForImageDocument() that returns ImageResource only for
ImageDocument.
- core/html/HTMLImageElement.h
- core/html/ImageDocument.cpp
- core/loader/ImageLoader.h
- core/loader/ImageLoader.cpp

A reference to Resource from FrameSerializer is replaced with |mimeType|
String and |hasCacheControlNoStoreHeader| boolean in order to migrate to
ImageResourceContent.
- web/WebFrameSerializer.cpp
- core/frame/FrameSerializer.h
- core/frame/FrameSerializer.cpp

BUG=667641

CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2

Review-Url: https://codereview.chromium.org/2469873002
==========

to

==========
Currently, ImageResource is
- A part of Image, as a kind of a wrapper of blink::Image and
  the entry point of ImageResourceObserver, and
- A part of Resource, because it is a subclass of Resource and highly
  involved into internals of Resource such as state control of Resource.
This complicates the ImageResource, causes dependencies and
layering violation between loading- and image-related things, and
complicates reloading logic.

This CL splits ImageResource into two parts:
ImageResource, the Resource-part:
- A subclass of Resource.
ImageResourceContent, the Image-part:
- Manages blink::Image and ImageResourceObserver, and inherits ImageObserver.
- Has limited access to Resource-part (if any) via ImageResourceInfo.

Most of current users of ImageResource, especially those in core/style, are
migrated to use ImageResourceContent and/or
to inherit ImageResourceObserver instead of ResourceClient,
as they want to access Image, not necessarily resource loading.

Design doc:
https://docs.google.com/document/d/1O-fB83mrE0B_V8gzXNqHgmRLCvstTB4MMi3RnVLr8...

In this CL, the Resource-part and the Image-part has 1:1 correspondence that
doesn't change during their lifetime and therefore ImageResource::m_content
is always non-null.
Subsequent CLs will make them detachable.

Changes in most of the files are replacing ImageResource with
ImageResourceContent.
Notable changes are:

Main part of CL: split ImageResource into two parts:
- core/fetch/ImageResource.h
- core/fetch/ImageResource.cpp
- core/fetch/ImageResourceContent.h
- core/fetch/ImageResourceContent.cpp

emulateLoadStartedForInspector() hack is now called via ImageResourceContent:
- core/css/CSSImageValue.cpp

ImageLoader is mostly migrated to ImageResourceContent, except for
imageResourceForImageDocument() that returns ImageResource only for
ImageDocument.
- core/html/HTMLImageElement.h
- core/html/ImageDocument.cpp
- core/loader/ImageLoader.h
- core/loader/ImageLoader.cpp

A reference to Resource from FrameSerializer is replaced with |mimeType|
String and |hasCacheControlNoStoreHeader| boolean in order to migrate to
ImageResourceContent.
- web/WebFrameSerializer.cpp
- core/frame/FrameSerializer.h
- core/frame/FrameSerializer.cpp

BUG=667641

CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2

Committed: https://crrev.com/7b80eac87bd4a9f5d0fad898dd73e239ee36fbb3
Cr-Commit-Position: refs/heads/master@{#437867}
==========

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

Patchset 36 (id:??) landed as
https://crrev.com/7b80eac87bd4a9f5d0fad898dd73e239ee36fbb3
Cr-Commit-Position: refs/heads/master@{#437867}

[hiroshige, 2016-12-12 16:23:40]

Description was changed from

==========
Currently, ImageResource is
- A part of Image, as a kind of a wrapper of blink::Image and
  the entry point of ImageResourceObserver, and
- A part of Resource, because it is a subclass of Resource and highly
  involved into internals of Resource such as state control of Resource.
This complicates the ImageResource, causes dependencies and
layering violation between loading- and image-related things, and
complicates reloading logic.

This CL splits ImageResource into two parts:
ImageResource, the Resource-part:
- A subclass of Resource.
ImageResourceContent, the Image-part:
- Manages blink::Image and ImageResourceObserver, and inherits ImageObserver.
- Has limited access to Resource-part (if any) via ImageResourceInfo.

Most of current users of ImageResource, especially those in core/style, are
migrated to use ImageResourceContent and/or
to inherit ImageResourceObserver instead of ResourceClient,
as they want to access Image, not necessarily resource loading.

Design doc:
https://docs.google.com/document/d/1O-fB83mrE0B_V8gzXNqHgmRLCvstTB4MMi3RnVLr8...

In this CL, the Resource-part and the Image-part has 1:1 correspondence that
doesn't change during their lifetime and therefore ImageResource::m_content
is always non-null.
Subsequent CLs will make them detachable.

Changes in most of the files are replacing ImageResource with
ImageResourceContent.
Notable changes are:

Main part of CL: split ImageResource into two parts:
- core/fetch/ImageResource.h
- core/fetch/ImageResource.cpp
- core/fetch/ImageResourceContent.h
- core/fetch/ImageResourceContent.cpp

emulateLoadStartedForInspector() hack is now called via ImageResourceContent:
- core/css/CSSImageValue.cpp

ImageLoader is mostly migrated to ImageResourceContent, except for
imageResourceForImageDocument() that returns ImageResource only for
ImageDocument.
- core/html/HTMLImageElement.h
- core/html/ImageDocument.cpp
- core/loader/ImageLoader.h
- core/loader/ImageLoader.cpp

A reference to Resource from FrameSerializer is replaced with |mimeType|
String and |hasCacheControlNoStoreHeader| boolean in order to migrate to
ImageResourceContent.
- web/WebFrameSerializer.cpp
- core/frame/FrameSerializer.h
- core/frame/FrameSerializer.cpp

BUG=667641

CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2

Committed: https://crrev.com/7b80eac87bd4a9f5d0fad898dd73e239ee36fbb3
Cr-Commit-Position: refs/heads/master@{#437867}
==========

to

==========
Split ImageResource into Resource and Image parts

Currently, ImageResource is
- A part of Image, as a kind of a wrapper of blink::Image and
  the entry point of ImageResourceObserver, and
- A part of Resource, because it is a subclass of Resource and highly
  involved into internals of Resource such as state control of Resource.
This complicates the ImageResource, causes dependencies and
layering violation between loading- and image-related things, and
complicates reloading logic.

This CL splits ImageResource into two parts:
ImageResource, the Resource-part:
- A subclass of Resource.
ImageResourceContent, the Image-part:
- Manages blink::Image and ImageResourceObserver, and inherits ImageObserver.
- Has limited access to Resource-part (if any) via ImageResourceInfo.

Most of current users of ImageResource, especially those in core/style, are
migrated to use ImageResourceContent and/or
to inherit ImageResourceObserver instead of ResourceClient,
as they want to access Image, not necessarily resource loading.

Design doc:
https://docs.google.com/document/d/1O-fB83mrE0B_V8gzXNqHgmRLCvstTB4MMi3RnVLr8...

In this CL, the Resource-part and the Image-part has 1:1 correspondence that
doesn't change during their lifetime and therefore ImageResource::m_content
is always non-null.
Subsequent CLs will make them detachable.

Changes in most of the files are replacing ImageResource with
ImageResourceContent.
Notable changes are:

Main part of CL: split ImageResource into two parts:
- core/fetch/ImageResource.h
- core/fetch/ImageResource.cpp
- core/fetch/ImageResourceContent.h
- core/fetch/ImageResourceContent.cpp

emulateLoadStartedForInspector() hack is now called via ImageResourceContent:
- core/css/CSSImageValue.cpp

ImageLoader is mostly migrated to ImageResourceContent, except for
imageResourceForImageDocument() that returns ImageResource only for
ImageDocument.
- core/html/HTMLImageElement.h
- core/html/ImageDocument.cpp
- core/loader/ImageLoader.h
- core/loader/ImageLoader.cpp

A reference to Resource from FrameSerializer is replaced with |mimeType|
String and |hasCacheControlNoStoreHeader| boolean in order to migrate to
ImageResourceContent.
- web/WebFrameSerializer.cpp
- core/frame/FrameSerializer.h
- core/frame/FrameSerializer.cpp

BUG=667641

CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2

Committed: https://crrev.com/7b80eac87bd4a9f5d0fad898dd73e239ee36fbb3
Cr-Commit-Position: refs/heads/master@{#437867}
==========

[hiroshige, 2016-12-12 16:24:25]

On 2016/12/12 15:12:09, commit-bot: I haz the power wrote:
> Patchset 36 (id:??) landed as
> https://crrev.com/7b80eac87bd4a9f5d0fad898dd73e239ee36fbb3
> Cr-Commit-Position: refs/heads/master@{#437867}

Oh, the CL title was not included in the CL description