New issue
Advanced search Search tips

Issue 667641 link

Starred by 4 users

Issue metadata

Status: Started
Owner:
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: All
Pri: 2
Type: Bug

Blocked on:
issue 632580
issue 666214
issue 668598
issue 695808
issue 737392

Blocking:
issue 382170
issue 624697



Sign in to add a comment

Split ImageResource into Image-part and Resource-part

Project Member Reported by hirosh...@chromium.org, Nov 22 2016

Issue description

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 issue splits ImageResource into Image-part and Resource-part.

Design doc:
https://docs.google.com/document/d/1O-fB83mrE0B_V8gzXNqHgmRLCvstTB4MMi3RnVLr8bE/edit?usp=sharing
 
Blocking: 624697 382170
This blocks ImageLoader's EventSender removal and SVG image onload fix.
For them, we need to resolve timing dependencies around image's onload events, which I expect is easier after we split ImageResource.
Blockedon: 666214
Cc: hajimehoshi@chromium.org japhet@chromium.org yhirano@chromium.org toyoshim@chromium.org
Description: Show this description
Labels: -Pri-3 M-57 Pri-2
Blockedon: 668598
Project Member

Comment 7 by bugdroid1@chromium.org, Nov 30 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/d8fa1cd54a5df1a17fb9359a5b9cac00293517aa

commit d8fa1cd54a5df1a17fb9359a5b9cac00293517aa
Author: hiroshige <hiroshige@chromium.org>
Date: Wed Nov 30 12:58:10 2016

Decouple ImageResourceObserver::imageNotifyFinished() from notifyFinished()

Previously, imageNotifyFinished() is coupled with ResourceClient::notifyFinished().
In order to decouple ImageResorceObserver from resource loading and make it
purely image-related observer, this CL make imageNotifyFinished() to be
called after ImageResourceObserver::imageChanged(), not notifyFinished().

BUG=667641

Review-Url: https://codereview.chromium.org/2468883003
Cr-Commit-Position: refs/heads/master@{#435235}

[modify] https://crrev.com/d8fa1cd54a5df1a17fb9359a5b9cac00293517aa/third_party/WebKit/Source/core/fetch/ImageResource.cpp
[modify] https://crrev.com/d8fa1cd54a5df1a17fb9359a5b9cac00293517aa/third_party/WebKit/Source/core/fetch/ImageResource.h
[modify] https://crrev.com/d8fa1cd54a5df1a17fb9359a5b9cac00293517aa/third_party/WebKit/Source/core/fetch/ImageResourceObserver.h
[modify] https://crrev.com/d8fa1cd54a5df1a17fb9359a5b9cac00293517aa/third_party/WebKit/Source/core/fetch/ImageResourceTest.cpp
[modify] https://crrev.com/d8fa1cd54a5df1a17fb9359a5b9cac00293517aa/third_party/WebKit/Source/core/fetch/MockResourceClients.cpp
[modify] https://crrev.com/d8fa1cd54a5df1a17fb9359a5b9cac00293517aa/third_party/WebKit/Source/core/fetch/MockResourceClients.h

Project Member

Comment 8 by bugdroid1@chromium.org, Nov 30 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/37e0c8e928dfad9c455b5f2145a5e3e25e1350ca

commit 37e0c8e928dfad9c455b5f2145a5e3e25e1350ca
Author: hiroshige <hiroshige@chromium.org>
Date: Wed Nov 30 15:51:00 2016

Change StyleFetchedImage from ResourceClient to ImageResourceObserver

This CL is a part of moving ResourceClient subclasses used for ImageResource
to ImageResourceObserver, in order to split ImageResource into Image-part and
Resource-part.

BUG=667641

Review-Url: https://codereview.chromium.org/2486073004
Cr-Commit-Position: refs/heads/master@{#435269}

[modify] https://crrev.com/37e0c8e928dfad9c455b5f2145a5e3e25e1350ca/third_party/WebKit/Source/core/style/StyleFetchedImage.cpp
[modify] https://crrev.com/37e0c8e928dfad9c455b5f2145a5e3e25e1350ca/third_party/WebKit/Source/core/style/StyleFetchedImage.h

Project Member

Comment 9 by bugdroid1@chromium.org, Nov 30 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/a0a7ada7f1270cbacd19827816ff312c6fa9f685

commit a0a7ada7f1270cbacd19827816ff312c6fa9f685
Author: hiroshige <hiroshige@chromium.org>
Date: Wed Nov 30 16:03:06 2016

Change SVGFEImageElement from ResourceClient to ImageResourceObserver

This CL is a part of moving ResourceClient subclasses used for ImageResource
to ImageResourceObserver, in order to split ImageResource into Image-part and
Resource-part.

BUG=667641

Review-Url: https://codereview.chromium.org/2485873004
Cr-Commit-Position: refs/heads/master@{#435274}

[modify] https://crrev.com/a0a7ada7f1270cbacd19827816ff312c6fa9f685/third_party/WebKit/Source/core/svg/SVGFEImageElement.cpp
[modify] https://crrev.com/a0a7ada7f1270cbacd19827816ff312c6fa9f685/third_party/WebKit/Source/core/svg/SVGFEImageElement.h

Project Member

Comment 10 by bugdroid1@chromium.org, Nov 30 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/072105722f8a1095345faaa749c296e1bce19c15

commit 072105722f8a1095345faaa749c296e1bce19c15
Author: hiroshige <hiroshige@chromium.org>
Date: Wed Nov 30 18:20:34 2016

Change StyleFetchedImageSet from ResourceClient to ImageResourceObserver

This CL is a part of moving ResourceClient subclasses used for ImageResource
to ImageResourceObserver, in order to split ImageResource into Image-part and
Resource-part.

BUG=667641

Review-Url: https://codereview.chromium.org/2521823003
Cr-Commit-Position: refs/heads/master@{#435339}

[modify] https://crrev.com/072105722f8a1095345faaa749c296e1bce19c15/third_party/WebKit/Source/core/style/StyleFetchedImageSet.cpp
[modify] https://crrev.com/072105722f8a1095345faaa749c296e1bce19c15/third_party/WebKit/Source/core/style/StyleFetchedImageSet.h

Project Member

Comment 11 by bugdroid1@chromium.org, Nov 30 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/1e6d5389ff332f09924b180e4a114cbbec1435ce

commit 1e6d5389ff332f09924b180e4a114cbbec1435ce
Author: hiroshige <hiroshige@chromium.org>
Date: Wed Nov 30 19:08:47 2016

Remove wasCanceled() check in ImageLoader

This CL removes a code block that seems unreachable, because errorOccurred()
should be always true if wasCanceled() is true.

BUG=667641

Review-Url: https://codereview.chromium.org/2471133007
Cr-Commit-Position: refs/heads/master@{#435364}

[modify] https://crrev.com/1e6d5389ff332f09924b180e4a114cbbec1435ce/third_party/WebKit/Source/core/loader/ImageLoader.cpp

Project Member

Comment 12 by bugdroid1@chromium.org, Dec 7 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/c53087f6e68fe790901cedf96ae37e696d613848

commit c53087f6e68fe790901cedf96ae37e696d613848
Author: hiroshige <hiroshige@chromium.org>
Date: Wed Dec 07 06:13:30 2016

Move hacks for Inspector in restoreCachedResourceIfNeeded() to ResourceFetcher

This CL moves the hacks for Inspector in:
- CSSFontFaceSrcValue::restoreCachedResourceIfNeeded()
- CSSImageValue::restoreCachedResourceIfNeeded()
to ResourceFetcher, in order to
- centralize the workaround and
- prevent core/css from directly touching ImageResource objects
  (as preparation for Issue 667641).

This CL replaces MixedContentChecker::shouldBlockFetch() calls with
FetchContext::canRequest() (that calls shouldBlockFetch() inside it) to
prevent layering violation (MixedContentChecker cannot be called directly
from core/fetch).

BUG=666214, 667641

Review-Url: https://codereview.chromium.org/2533983003
Cr-Commit-Position: refs/heads/master@{#436877}

[modify] https://crrev.com/c53087f6e68fe790901cedf96ae37e696d613848/third_party/WebKit/Source/core/css/CSSFontFaceSrcValue.cpp
[modify] https://crrev.com/c53087f6e68fe790901cedf96ae37e696d613848/third_party/WebKit/Source/core/css/CSSImageValue.cpp
[modify] https://crrev.com/c53087f6e68fe790901cedf96ae37e696d613848/third_party/WebKit/Source/core/fetch/ResourceFetcher.cpp
[modify] https://crrev.com/c53087f6e68fe790901cedf96ae37e696d613848/third_party/WebKit/Source/core/fetch/ResourceFetcher.h

Project Member

Comment 14 by bugdroid1@chromium.org, Dec 12 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/7b80eac87bd4a9f5d0fad898dd73e239ee36fbb3

commit 7b80eac87bd4a9f5d0fad898dd73e239ee36fbb3
Author: hiroshige <hiroshige@chromium.org>
Date: Mon Dec 12 14:25:51 2016

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

Review-Url: https://codereview.chromium.org/2469873002
Cr-Commit-Position: refs/heads/master@{#437867}

[modify] https://crrev.com/7b80eac87bd4a9f5d0fad898dd73e239ee36fbb3/third_party/WebKit/Source/core/clipboard/DataTransfer.cpp
[modify] https://crrev.com/7b80eac87bd4a9f5d0fad898dd73e239ee36fbb3/third_party/WebKit/Source/core/clipboard/DataTransfer.h
[modify] https://crrev.com/7b80eac87bd4a9f5d0fad898dd73e239ee36fbb3/third_party/WebKit/Source/core/css/CSSCrossfadeValue.cpp
[modify] https://crrev.com/7b80eac87bd4a9f5d0fad898dd73e239ee36fbb3/third_party/WebKit/Source/core/css/CSSCrossfadeValue.h
[modify] https://crrev.com/7b80eac87bd4a9f5d0fad898dd73e239ee36fbb3/third_party/WebKit/Source/core/css/CSSCursorImageValue.cpp
[modify] https://crrev.com/7b80eac87bd4a9f5d0fad898dd73e239ee36fbb3/third_party/WebKit/Source/core/css/CSSImageSetValue.cpp
[modify] https://crrev.com/7b80eac87bd4a9f5d0fad898dd73e239ee36fbb3/third_party/WebKit/Source/core/css/CSSImageValue.cpp
[modify] https://crrev.com/7b80eac87bd4a9f5d0fad898dd73e239ee36fbb3/third_party/WebKit/Source/core/css/cssom/CSSStyleImageValue.cpp
[modify] https://crrev.com/7b80eac87bd4a9f5d0fad898dd73e239ee36fbb3/third_party/WebKit/Source/core/css/cssom/CSSStyleImageValue.h
[modify] https://crrev.com/7b80eac87bd4a9f5d0fad898dd73e239ee36fbb3/third_party/WebKit/Source/core/editing/Editor.cpp
[modify] https://crrev.com/7b80eac87bd4a9f5d0fad898dd73e239ee36fbb3/third_party/WebKit/Source/core/fetch/BUILD.gn
[modify] https://crrev.com/7b80eac87bd4a9f5d0fad898dd73e239ee36fbb3/third_party/WebKit/Source/core/fetch/ImageResource.cpp
[modify] https://crrev.com/7b80eac87bd4a9f5d0fad898dd73e239ee36fbb3/third_party/WebKit/Source/core/fetch/ImageResource.h
[add] https://crrev.com/7b80eac87bd4a9f5d0fad898dd73e239ee36fbb3/third_party/WebKit/Source/core/fetch/ImageResourceContent.cpp
[add] https://crrev.com/7b80eac87bd4a9f5d0fad898dd73e239ee36fbb3/third_party/WebKit/Source/core/fetch/ImageResourceContent.h
[add] https://crrev.com/7b80eac87bd4a9f5d0fad898dd73e239ee36fbb3/third_party/WebKit/Source/core/fetch/ImageResourceInfo.h
[modify] https://crrev.com/7b80eac87bd4a9f5d0fad898dd73e239ee36fbb3/third_party/WebKit/Source/core/fetch/ImageResourceObserver.h
[modify] https://crrev.com/7b80eac87bd4a9f5d0fad898dd73e239ee36fbb3/third_party/WebKit/Source/core/fetch/ImageResourceTest.cpp
[modify] https://crrev.com/7b80eac87bd4a9f5d0fad898dd73e239ee36fbb3/third_party/WebKit/Source/core/fetch/MockResourceClients.cpp
[modify] https://crrev.com/7b80eac87bd4a9f5d0fad898dd73e239ee36fbb3/third_party/WebKit/Source/core/fetch/MockResourceClients.h
[modify] https://crrev.com/7b80eac87bd4a9f5d0fad898dd73e239ee36fbb3/third_party/WebKit/Source/core/frame/FrameSerializer.cpp
[modify] https://crrev.com/7b80eac87bd4a9f5d0fad898dd73e239ee36fbb3/third_party/WebKit/Source/core/frame/FrameSerializer.h
[modify] https://crrev.com/7b80eac87bd4a9f5d0fad898dd73e239ee36fbb3/third_party/WebKit/Source/core/frame/ImageBitmapTest.cpp
[modify] https://crrev.com/7b80eac87bd4a9f5d0fad898dd73e239ee36fbb3/third_party/WebKit/Source/core/html/HTMLImageElement.cpp
[modify] https://crrev.com/7b80eac87bd4a9f5d0fad898dd73e239ee36fbb3/third_party/WebKit/Source/core/html/HTMLImageElement.h
[modify] https://crrev.com/7b80eac87bd4a9f5d0fad898dd73e239ee36fbb3/third_party/WebKit/Source/core/html/HTMLImageFallbackHelper.cpp
[modify] https://crrev.com/7b80eac87bd4a9f5d0fad898dd73e239ee36fbb3/third_party/WebKit/Source/core/html/HTMLImageLoader.cpp
[modify] https://crrev.com/7b80eac87bd4a9f5d0fad898dd73e239ee36fbb3/third_party/WebKit/Source/core/html/HTMLImageLoader.h
[modify] https://crrev.com/7b80eac87bd4a9f5d0fad898dd73e239ee36fbb3/third_party/WebKit/Source/core/html/HTMLObjectElement.cpp
[modify] https://crrev.com/7b80eac87bd4a9f5d0fad898dd73e239ee36fbb3/third_party/WebKit/Source/core/html/ImageDocument.cpp
[modify] https://crrev.com/7b80eac87bd4a9f5d0fad898dd73e239ee36fbb3/third_party/WebKit/Source/core/html/ImageDocument.h
[modify] https://crrev.com/7b80eac87bd4a9f5d0fad898dd73e239ee36fbb3/third_party/WebKit/Source/core/html/forms/ImageInputType.cpp
[modify] https://crrev.com/7b80eac87bd4a9f5d0fad898dd73e239ee36fbb3/third_party/WebKit/Source/core/input/EventHandler.cpp
[modify] https://crrev.com/7b80eac87bd4a9f5d0fad898dd73e239ee36fbb3/third_party/WebKit/Source/core/inspector/InspectorPageAgent.cpp
[modify] https://crrev.com/7b80eac87bd4a9f5d0fad898dd73e239ee36fbb3/third_party/WebKit/Source/core/inspector/InspectorTraceEvents.cpp
[modify] https://crrev.com/7b80eac87bd4a9f5d0fad898dd73e239ee36fbb3/third_party/WebKit/Source/core/inspector/InspectorTraceEvents.h
[modify] https://crrev.com/7b80eac87bd4a9f5d0fad898dd73e239ee36fbb3/third_party/WebKit/Source/core/layout/LayoutImage.cpp
[modify] https://crrev.com/7b80eac87bd4a9f5d0fad898dd73e239ee36fbb3/third_party/WebKit/Source/core/layout/LayoutImage.h
[modify] https://crrev.com/7b80eac87bd4a9f5d0fad898dd73e239ee36fbb3/third_party/WebKit/Source/core/layout/LayoutImageResource.cpp
[modify] https://crrev.com/7b80eac87bd4a9f5d0fad898dd73e239ee36fbb3/third_party/WebKit/Source/core/layout/LayoutImageResource.h
[modify] https://crrev.com/7b80eac87bd4a9f5d0fad898dd73e239ee36fbb3/third_party/WebKit/Source/core/layout/LayoutImageResourceStyleImage.cpp
[modify] https://crrev.com/7b80eac87bd4a9f5d0fad898dd73e239ee36fbb3/third_party/WebKit/Source/core/layout/LayoutListMarker.cpp
[modify] https://crrev.com/7b80eac87bd4a9f5d0fad898dd73e239ee36fbb3/third_party/WebKit/Source/core/layout/LayoutObject.cpp
[modify] https://crrev.com/7b80eac87bd4a9f5d0fad898dd73e239ee36fbb3/third_party/WebKit/Source/core/layout/LayoutObject.h
[modify] https://crrev.com/7b80eac87bd4a9f5d0fad898dd73e239ee36fbb3/third_party/WebKit/Source/core/layout/LayoutTableRow.cpp
[modify] https://crrev.com/7b80eac87bd4a9f5d0fad898dd73e239ee36fbb3/third_party/WebKit/Source/core/layout/compositing/CompositedLayerMapping.cpp
[modify] https://crrev.com/7b80eac87bd4a9f5d0fad898dd73e239ee36fbb3/third_party/WebKit/Source/core/layout/shapes/Shape.cpp
[modify] https://crrev.com/7b80eac87bd4a9f5d0fad898dd73e239ee36fbb3/third_party/WebKit/Source/core/layout/shapes/ShapeOutsideInfo.cpp
[modify] https://crrev.com/7b80eac87bd4a9f5d0fad898dd73e239ee36fbb3/third_party/WebKit/Source/core/layout/svg/LayoutSVGImage.cpp
[modify] https://crrev.com/7b80eac87bd4a9f5d0fad898dd73e239ee36fbb3/third_party/WebKit/Source/core/loader/ImageLoader.cpp
[modify] https://crrev.com/7b80eac87bd4a9f5d0fad898dd73e239ee36fbb3/third_party/WebKit/Source/core/loader/ImageLoader.h
[modify] https://crrev.com/7b80eac87bd4a9f5d0fad898dd73e239ee36fbb3/third_party/WebKit/Source/core/loader/ProgressTrackerTest.cpp
[modify] https://crrev.com/7b80eac87bd4a9f5d0fad898dd73e239ee36fbb3/third_party/WebKit/Source/core/loader/resource/CSSStyleSheetResourceTest.cpp
[modify] https://crrev.com/7b80eac87bd4a9f5d0fad898dd73e239ee36fbb3/third_party/WebKit/Source/core/page/DragController.cpp
[modify] https://crrev.com/7b80eac87bd4a9f5d0fad898dd73e239ee36fbb3/third_party/WebKit/Source/core/paint/SVGImagePainter.cpp
[modify] https://crrev.com/7b80eac87bd4a9f5d0fad898dd73e239ee36fbb3/third_party/WebKit/Source/core/style/ShapeValue.h
[modify] https://crrev.com/7b80eac87bd4a9f5d0fad898dd73e239ee36fbb3/third_party/WebKit/Source/core/style/StyleFetchedImage.cpp
[modify] https://crrev.com/7b80eac87bd4a9f5d0fad898dd73e239ee36fbb3/third_party/WebKit/Source/core/style/StyleFetchedImage.h
[modify] https://crrev.com/7b80eac87bd4a9f5d0fad898dd73e239ee36fbb3/third_party/WebKit/Source/core/style/StyleFetchedImageSet.cpp
[modify] https://crrev.com/7b80eac87bd4a9f5d0fad898dd73e239ee36fbb3/third_party/WebKit/Source/core/style/StyleFetchedImageSet.h
[modify] https://crrev.com/7b80eac87bd4a9f5d0fad898dd73e239ee36fbb3/third_party/WebKit/Source/core/style/StyleImage.h
[modify] https://crrev.com/7b80eac87bd4a9f5d0fad898dd73e239ee36fbb3/third_party/WebKit/Source/core/svg/SVGFEImageElement.cpp
[modify] https://crrev.com/7b80eac87bd4a9f5d0fad898dd73e239ee36fbb3/third_party/WebKit/Source/core/svg/SVGFEImageElement.h
[modify] https://crrev.com/7b80eac87bd4a9f5d0fad898dd73e239ee36fbb3/third_party/WebKit/Source/core/svg/SVGImageElement.h
[modify] https://crrev.com/7b80eac87bd4a9f5d0fad898dd73e239ee36fbb3/third_party/WebKit/Source/core/svg/SVGImageLoader.cpp
[modify] https://crrev.com/7b80eac87bd4a9f5d0fad898dd73e239ee36fbb3/third_party/WebKit/Source/core/svg/graphics/SVGImage.cpp
[modify] https://crrev.com/7b80eac87bd4a9f5d0fad898dd73e239ee36fbb3/third_party/WebKit/Source/core/svg/graphics/SVGImageChromeClient.cpp
[modify] https://crrev.com/7b80eac87bd4a9f5d0fad898dd73e239ee36fbb3/third_party/WebKit/Source/core/testing/Internals.cpp
[modify] https://crrev.com/7b80eac87bd4a9f5d0fad898dd73e239ee36fbb3/third_party/WebKit/Source/modules/shapedetection/ShapeDetector.cpp
[modify] https://crrev.com/7b80eac87bd4a9f5d0fad898dd73e239ee36fbb3/third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp
[modify] https://crrev.com/7b80eac87bd4a9f5d0fad898dd73e239ee36fbb3/third_party/WebKit/Source/platform/graphics/BitmapImage.cpp
[modify] https://crrev.com/7b80eac87bd4a9f5d0fad898dd73e239ee36fbb3/third_party/WebKit/Source/platform/graphics/Image.h
[modify] https://crrev.com/7b80eac87bd4a9f5d0fad898dd73e239ee36fbb3/third_party/WebKit/Source/web/WebFrameSerializer.cpp
[modify] https://crrev.com/7b80eac87bd4a9f5d0fad898dd73e239ee36fbb3/third_party/WebKit/Source/web/tests/WebFrameTest.cpp

Project Member

Comment 15 by bugdroid1@chromium.org, Jan 5 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/9758ade87950804041be8a2e1725aad2811f318a

commit 9758ade87950804041be8a2e1725aad2811f318a
Author: hiroshige <hiroshige@chromium.org>
Date: Thu Jan 05 19:41:54 2017

Unify ImageResource* local variable names in ImageResourceTest.cpp

BUG=667641

Review-Url: https://codereview.chromium.org/2609463002
Cr-Commit-Position: refs/heads/master@{#441737}

[modify] https://crrev.com/9758ade87950804041be8a2e1725aad2811f318a/third_party/WebKit/Source/core/loader/resource/ImageResourceTest.cpp

Project Member

Comment 16 by bugdroid1@chromium.org, Jan 9 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/5a597473adc7e1e3d0dd703d6d64dbe9364144ba

commit 5a597473adc7e1e3d0dd703d6d64dbe9364144ba
Author: hiroshige <hiroshige@chromium.org>
Date: Mon Jan 09 21:25:33 2017

Merge clearImage() and clearImageAndNotifyObservers() into updateImage()

In order to make ImageResourceContent::updateImage() the single control point
of image updates from ImageResource, this CL merges clearImage() and
clearImageAndNotifyObservers() calls from ImageResource into updateImage().

BUG=667641

Review-Url: https://codereview.chromium.org/2587503002
Cr-Commit-Position: refs/heads/master@{#442351}

[modify] https://crrev.com/5a597473adc7e1e3d0dd703d6d64dbe9364144ba/third_party/WebKit/Source/core/loader/resource/ImageResource.cpp
[modify] https://crrev.com/5a597473adc7e1e3d0dd703d6d64dbe9364144ba/third_party/WebKit/Source/core/loader/resource/ImageResourceContent.cpp
[modify] https://crrev.com/5a597473adc7e1e3d0dd703d6d64dbe9364144ba/third_party/WebKit/Source/core/loader/resource/ImageResourceContent.h

Project Member

Comment 17 by bugdroid1@chromium.org, Jan 23 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/30e0ef53363917a324acf0b89f62f64223d06f3f

commit 30e0ef53363917a324acf0b89f62f64223d06f3f
Author: hiroshige <hiroshige@chromium.org>
Date: Mon Jan 23 22:40:38 2017

Introduce ImageResource::updateImage()

This CL centralizes ImageResourceContent::updateImage() calls into a single
point of ImageResource::updateImage() to help further refactoring.
No behavior changes.

BUG=667641

Review-Url: https://codereview.chromium.org/2644733002
Cr-Commit-Position: refs/heads/master@{#445520}

[modify] https://crrev.com/30e0ef53363917a324acf0b89f62f64223d06f3f/third_party/WebKit/Source/core/loader/resource/ImageResource.cpp
[modify] https://crrev.com/30e0ef53363917a324acf0b89f62f64223d06f3f/third_party/WebKit/Source/core/loader/resource/ImageResource.h

Project Member

Comment 18 by bugdroid1@chromium.org, Jan 24 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/5488d0abc4a664aefb1e65f883bacea353523dba

commit 5488d0abc4a664aefb1e65f883bacea353523dba
Author: hiroshige <hiroshige@chromium.org>
Date: Tue Jan 24 05:36:23 2017

Replace MockImageResourceClient with MockImageResourceObserver

To distinguish ImageResource and ImageResourceContent in ImageResourceTest
more clearly and make tests more ImageResourceContent-centric, this CL
- Replaces MockImageResourceClient
  (that was both a ResourceClient and an ImageResourceObserver) with
  MockImageResourceObserver (that is not a ResourceClient),
- Replaces checks on notifyFinished() with checks on imageNotifyFinished(),
- Replaces checks on encodedSize() (that are used to check that expected
  image data is loaded) with checks on image width.

BUG=667641

Review-Url: https://codereview.chromium.org/2607023002
Cr-Commit-Position: refs/heads/master@{#445658}

[modify] https://crrev.com/5488d0abc4a664aefb1e65f883bacea353523dba/third_party/WebKit/Source/core/BUILD.gn
[modify] https://crrev.com/5488d0abc4a664aefb1e65f883bacea353523dba/third_party/WebKit/Source/core/loader/resource/ImageResourceTest.cpp
[delete] https://crrev.com/d280ab1a052245d5bcacf63a4478fc3c0f085899/third_party/WebKit/Source/core/loader/resource/MockImageResourceClient.cpp
[delete] https://crrev.com/d280ab1a052245d5bcacf63a4478fc3c0f085899/third_party/WebKit/Source/core/loader/resource/MockImageResourceClient.h
[add] https://crrev.com/5488d0abc4a664aefb1e65f883bacea353523dba/third_party/WebKit/Source/core/loader/resource/MockImageResourceObserver.cpp
[add] https://crrev.com/5488d0abc4a664aefb1e65f883bacea353523dba/third_party/WebKit/Source/core/loader/resource/MockImageResourceObserver.h

Project Member

Comment 19 by bugdroid1@chromium.org, Jan 24 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/999a78abf761b51b7d02ee13c1cc3a1bc75b6db7

commit 999a78abf761b51b7d02ee13c1cc3a1bc75b6db7
Author: hiroshige <hiroshige@chromium.org>
Date: Tue Jan 24 06:51:51 2017

Make the uses of Resource::m_resourceRequest/m_response const if applicable

This CL
- Marks some Resource methods as const, and
- Replace |m_resourceRequest| and |m_response| with with
  resourceRequest() and response(), respectively, to make it clear
  that these references are const.

BUG=667641

Review-Url: https://codereview.chromium.org/2656443003
Cr-Commit-Position: refs/heads/master@{#445668}

[modify] https://crrev.com/999a78abf761b51b7d02ee13c1cc3a1bc75b6db7/third_party/WebKit/Source/core/fetch/Resource.cpp
[modify] https://crrev.com/999a78abf761b51b7d02ee13c1cc3a1bc75b6db7/third_party/WebKit/Source/core/fetch/Resource.h

Project Member

Comment 20 by bugdroid1@chromium.org, Jan 24 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/347f7a59d1fe355675e1844d0c1e2caa993985df

commit 347f7a59d1fe355675e1844d0c1e2caa993985df
Author: hiroshige <hiroshige@chromium.org>
Date: Tue Jan 24 20:46:48 2017

Replace Resource::Status with ResourceStatus

Follow-up of https://codereview.chromium.org/2537063002.

BUG=667641

Review-Url: https://codereview.chromium.org/2642383003
Cr-Commit-Position: refs/heads/master@{#445800}

[modify] https://crrev.com/347f7a59d1fe355675e1844d0c1e2caa993985df/third_party/WebKit/Source/bindings/core/v8/ScriptStreamerTest.cpp
[modify] https://crrev.com/347f7a59d1fe355675e1844d0c1e2caa993985df/third_party/WebKit/Source/core/css/RemoteFontFaceSource.cpp
[modify] https://crrev.com/347f7a59d1fe355675e1844d0c1e2caa993985df/third_party/WebKit/Source/core/css/cssom/CSSResourceValue.h
[modify] https://crrev.com/347f7a59d1fe355675e1844d0c1e2caa993985df/third_party/WebKit/Source/core/css/cssom/CSSResourceValueTest.cpp
[modify] https://crrev.com/347f7a59d1fe355675e1844d0c1e2caa993985df/third_party/WebKit/Source/core/css/cssom/CSSStyleImageValue.h
[modify] https://crrev.com/347f7a59d1fe355675e1844d0c1e2caa993985df/third_party/WebKit/Source/core/html/HTMLObjectElement.cpp
[modify] https://crrev.com/347f7a59d1fe355675e1844d0c1e2caa993985df/third_party/WebKit/Source/core/html/ImageDocument.cpp
[modify] https://crrev.com/347f7a59d1fe355675e1844d0c1e2caa993985df/third_party/WebKit/Source/core/html/parser/CSSPreloadScannerTest.cpp
[modify] https://crrev.com/347f7a59d1fe355675e1844d0c1e2caa993985df/third_party/WebKit/Source/core/inspector/InspectorPageAgent.cpp
[modify] https://crrev.com/347f7a59d1fe355675e1844d0c1e2caa993985df/third_party/WebKit/Source/core/loader/ImageLoader.cpp
[modify] https://crrev.com/347f7a59d1fe355675e1844d0c1e2caa993985df/third_party/WebKit/Source/core/loader/resource/CSSStyleSheetResource.cpp
[modify] https://crrev.com/347f7a59d1fe355675e1844d0c1e2caa993985df/third_party/WebKit/Source/core/loader/resource/FontResource.cpp
[modify] https://crrev.com/347f7a59d1fe355675e1844d0c1e2caa993985df/third_party/WebKit/Source/core/loader/resource/ImageResource.cpp
[modify] https://crrev.com/347f7a59d1fe355675e1844d0c1e2caa993985df/third_party/WebKit/Source/core/loader/resource/ImageResourceTest.cpp
[modify] https://crrev.com/347f7a59d1fe355675e1844d0c1e2caa993985df/third_party/WebKit/Source/core/testing/Internals.cpp
[modify] https://crrev.com/347f7a59d1fe355675e1844d0c1e2caa993985df/third_party/WebKit/Source/platform/loader/fetch/MemoryCacheCorrectnessTest.cpp
[modify] https://crrev.com/347f7a59d1fe355675e1844d0c1e2caa993985df/third_party/WebKit/Source/platform/loader/fetch/Resource.cpp
[modify] https://crrev.com/347f7a59d1fe355675e1844d0c1e2caa993985df/third_party/WebKit/Source/platform/loader/fetch/Resource.h
[modify] https://crrev.com/347f7a59d1fe355675e1844d0c1e2caa993985df/third_party/WebKit/Source/platform/loader/fetch/ResourceFetcher.cpp
[modify] https://crrev.com/347f7a59d1fe355675e1844d0c1e2caa993985df/third_party/WebKit/Source/web/tests/WebFrameTest.cpp

Project Member

Comment 21 by bugdroid1@chromium.org, Jan 25 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/ab84059f6799efdab7036fe076f2a35d1593d1a6

commit ab84059f6799efdab7036fe076f2a35d1593d1a6
Author: hiroshige <hiroshige@chromium.org>
Date: Wed Jan 25 01:41:40 2017

Add more unit tests for ImageResource's DecodeError

This CL
- Adds a unit test where DecodeError is caused by the empty body, and
- Adds assertions that the state is DecodeError when
  imageNotifyFinished() is called.

BUG=667641, 677188

Review-Url: https://codereview.chromium.org/2645953005
Cr-Commit-Position: refs/heads/master@{#445904}

[modify] https://crrev.com/ab84059f6799efdab7036fe076f2a35d1593d1a6/third_party/WebKit/Source/core/loader/resource/ImageResourceTest.cpp
[modify] https://crrev.com/ab84059f6799efdab7036fe076f2a35d1593d1a6/third_party/WebKit/Source/core/loader/resource/MockImageResourceObserver.cpp
[modify] https://crrev.com/ab84059f6799efdab7036fe076f2a35d1593d1a6/third_party/WebKit/Source/core/loader/resource/MockImageResourceObserver.h

Project Member

Comment 22 by bugdroid1@chromium.org, Jan 25 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/25ff90033184e53f7fd47b44511397da29a1ce27

commit 25ff90033184e53f7fd47b44511397da29a1ce27
Author: hiroshige <hiroshige@chromium.org>
Date: Wed Jan 25 01:51:04 2017

Check imageChangedCount() in DecodeError-related unit tests

BUG=667641, 677188

Review-Url: https://codereview.chromium.org/2653493005
Cr-Commit-Position: refs/heads/master@{#445908}

[modify] https://crrev.com/25ff90033184e53f7fd47b44511397da29a1ce27/third_party/WebKit/Source/core/loader/resource/ImageResourceTest.cpp

Project Member

Comment 23 by bugdroid1@chromium.org, Feb 10 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/6d03fad48660f6a6a767c6cc70c621ff10f8fcfa

commit 6d03fad48660f6a6a767c6cc70c621ff10f8fcfa
Author: hiroshige <hiroshige@chromium.org>
Date: Fri Feb 10 23:08:59 2017

Add unit tests for placeholder reloading on didFinishLoading

ImageResourceTest.ReloadIfLoFiOrPlaceholderViaResourceFetcher
tests reloading in ResourceFetcher::reloadLoFiImages().

ImageResourceTest.PartialContentWithoutDimensions
tests that DecodeError with non-empty body in
ResourceLoader::didFinishLoading().

ImageResourceTest.FetchAllowPlaceholderPartialContentWithoutDimensions
tests the case where DecodeError with non-empty body in
ResourceLoader::didFinishLoading() causes reloading.

BUG=667641, 677188

Review-Url: https://codereview.chromium.org/2648073002
Cr-Commit-Position: refs/heads/master@{#449776}

[modify] https://crrev.com/6d03fad48660f6a6a767c6cc70c621ff10f8fcfa/third_party/WebKit/Source/core/loader/resource/ImageResourceTest.cpp

Project Member

Comment 25 by bugdroid1@chromium.org, Feb 17 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/d4601dd9252054edfcd8bb4e4c2a93f11c307d85

commit d4601dd9252054edfcd8bb4e4c2a93f11c307d85
Author: hiroshige <hiroshige@chromium.org>
Date: Fri Feb 17 20:22:08 2017

Refactor reloading tests in ImageResourceTest.cpp

This CL merges the things after LoFi/Placeholder reloading is started
into testThatReloadIsStartedThenServeReload().

BUG=667641

Review-Url: https://codereview.chromium.org/2650543003
Cr-Commit-Position: refs/heads/master@{#451369}

[modify] https://crrev.com/d4601dd9252054edfcd8bb4e4c2a93f11c307d85/third_party/WebKit/Source/core/loader/resource/ImageResourceTest.cpp

Project Member

Comment 26 by bugdroid1@chromium.org, Feb 22 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/bd8c80c437ceaa8427186bbbbd3040ae17d6d13d

commit bd8c80c437ceaa8427186bbbbd3040ae17d6d13d
Author: hiroshige <hiroshige@chromium.org>
Date: Wed Feb 22 11:22:03 2017

Phase II Step 1: Remove updateImage() reentrancy around decodeError()

Design doc:
https://docs.google.com/document/d/1O-fB83mrE0B_V8gzXNqHgmRLCvstTB4MMi3RnVLr8bE/edit#heading=h.1rxkos5samv7

Previously, ImageResourceContent::updateImage() is reentered when
DecodeError occurred in ImageResource.

This CL removes the reentrancy and ImageResourceInfo::decodeError() by
1. making ImageResourceContent::updateImage() to return
   ShouldDecodeError without calling notifyObservers()
   when an decode error occurs,
2. then making ImageResource::updateImage() to call
   ImageResource::decodeError(), and
3. then making ImageResource::decodeError() to call notifyObservers()
   (via ImageResourceContent::updateImage()) if necessary.

This CL also adds DCHECK() for prohibiting reentering to
ImageResourceContent::updateImage().

In the case of a decode error inside didReceiveData(),
this CL removes one imageChanged() call, which looks unnecessary.

BUG=667641, 677188

Review-Url: https://codereview.chromium.org/2642823005
Cr-Commit-Position: refs/heads/master@{#451996}

[modify] https://crrev.com/bd8c80c437ceaa8427186bbbbd3040ae17d6d13d/third_party/WebKit/Source/core/loader/resource/ImageResource.cpp
[modify] https://crrev.com/bd8c80c437ceaa8427186bbbbd3040ae17d6d13d/third_party/WebKit/Source/core/loader/resource/ImageResourceContent.cpp
[modify] https://crrev.com/bd8c80c437ceaa8427186bbbbd3040ae17d6d13d/third_party/WebKit/Source/core/loader/resource/ImageResourceContent.h
[modify] https://crrev.com/bd8c80c437ceaa8427186bbbbd3040ae17d6d13d/third_party/WebKit/Source/core/loader/resource/ImageResourceInfo.h
[modify] https://crrev.com/bd8c80c437ceaa8427186bbbbd3040ae17d6d13d/third_party/WebKit/Source/core/loader/resource/ImageResourceTest.cpp

Project Member

Comment 27 by bugdroid1@chromium.org, Mar 7 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/810f7ecd76b0aba74f0a215dbd00b8e8cc5705ba

commit 810f7ecd76b0aba74f0a215dbd00b8e8cc5705ba
Author: hiroshige <hiroshige@chromium.org>
Date: Tue Mar 07 23:23:14 2017

Phase II Step 2: Remove setIsPlaceholder() in updateImage()

Design doc:
https://docs.google.com/document/d/1O-fB83mrE0B_V8gzXNqHgmRLCvstTB4MMi3RnVLr8bE/edit#heading=h.1rxkos5samv7

Previsouly, |m_isPlaceholder| is cleared in updateImage(), if:
- isEntireResource() is true, AND
- (1) The HTTP status code is not 4XX or 5XX OR
  (2) the response has no DecodeError,
to prevent creating of placeholder and reloading.

However, this causes timing and code dependencies from
ImageResourceContent::updateImage()
to |ImageResource::m_isPlaceholder|
and ImageResource::reloadIfLoFiOrPlaceholderImage().

This CL removes the dependencies and thus
ImageResourceInfo::setIsPlaceholder(),
by making |ImageResource::m_isPlaceholder| a tri-state enum
(renamed as |ImageResource::placeholderOption|):
- DoNotReloadPlaceholder (previously m_isPlaceholder == false)
- ReloadPlaceholderOnDecodeError (new state)
- ShowAndReloadPlaceholderAlways (previously m_isPlaceholder == true)
and setting it when receiving ResourceResponse.

This CL preserves the behavior of a placeholder ImageResource:

If isEntireResource() && (1):
  Behavior: Placeholder is not created && !shouldReloadBrokenPlaceholder().
  Previously: because |m_isPlaceholder| is cleared in updateImage().
  Now: because |m_placeholderOption| is set to |DoNotReloadPlaceholder|.

If isEntireResource() && not (1) && (2):
  Behavior: Placeholder is not created && !shouldReloadBrokenPlaceholder().
  Previously: because |m_isPlaceholder| is cleared in updateImage().
  Now: because |m_placeholderOption| is set to |ReloadPlaceholderOnDecodeError|
       and DecodeError does NOT occur because (2) is true.

If isEntireResource() && not (1) && not (2):
  Behavior: Placeholder is not created && shouldReloadBrokenPlaceholder().
  Previously: because updateImage() creates placeholder only if (2) is true
              and |m_isPlaceholder| is NOT cleared in updateImage().
  Now: because |m_placeholderOption| is set to |ReloadPlaceholderOnDecodeError|
       and DecodeError does occur because (2) is false.

Otherwise:
  Not affected.

BUG=667641, 677188

Review-Url: https://codereview.chromium.org/2650113002
Cr-Commit-Position: refs/heads/master@{#455282}

[modify] https://crrev.com/810f7ecd76b0aba74f0a215dbd00b8e8cc5705ba/third_party/WebKit/Source/core/loader/resource/ImageResource.cpp
[modify] https://crrev.com/810f7ecd76b0aba74f0a215dbd00b8e8cc5705ba/third_party/WebKit/Source/core/loader/resource/ImageResource.h
[modify] https://crrev.com/810f7ecd76b0aba74f0a215dbd00b8e8cc5705ba/third_party/WebKit/Source/core/loader/resource/ImageResourceContent.cpp
[modify] https://crrev.com/810f7ecd76b0aba74f0a215dbd00b8e8cc5705ba/third_party/WebKit/Source/core/loader/resource/ImageResourceInfo.h
[modify] https://crrev.com/810f7ecd76b0aba74f0a215dbd00b8e8cc5705ba/third_party/WebKit/Source/core/loader/resource/ImageResourceTest.cpp

Project Member

Comment 28 by bugdroid1@chromium.org, Mar 8 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/e09d3b69602086399cccd6af5d4eb7f8d0f171b3

commit e09d3b69602086399cccd6af5d4eb7f8d0f171b3
Author: hiroshige <hiroshige@chromium.org>
Date: Wed Mar 08 01:17:54 2017

ImageResourceTest: merge successful placeholder/non-placeholder loading

- Introduces testThatIsPlaceholderRequestAndServeResponse() and
  testThatIsNotPlaceholderRequestAndServeResponse(), for testing
  successful placeholder and non-placeholder loading, and
- Replaces some magic numbers with
  |kJpegImageWidth| or |kJpegImageHeight|.

BUG=667641

Review-Url: https://codereview.chromium.org/2728643004
Cr-Commit-Position: refs/heads/master@{#455320}

[modify] https://crrev.com/e09d3b69602086399cccd6af5d4eb7f8d0f171b3/third_party/WebKit/Source/core/loader/resource/ImageResourceTest.cpp

Project Member

Comment 29 by bugdroid1@chromium.org, Mar 8 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/79c699b99525ab50568821fce704976a9278e3fa

commit 79c699b99525ab50568821fce704976a9278e3fa
Author: hiroshige <hiroshige@chromium.org>
Date: Wed Mar 08 06:11:31 2017

Do not reload in ImageResource::fetch()

When there exists an placeholder ImageResource on MemoryCache and
a new non-placeholder request is created,
this CL starts a new non-placeholder loading with a new ImageResource,
instead of reusing the existing placeholder ImageResource and
triggering (non-placeholder) reloading on that.

This simplifies ImageResource::fetch() and removes one enum value for
controlling reload.

This leaves the existing placeholder ImageResource not to be reloaded
in such cases, and thus the unit tests are modified accordingly.

BUG=667641
TEST=ImageResourceTest.FetchAllowPlaceholderThenDisallowPlaceholder
     ImageResourceTest.FetchAllowPlaceholderThenDisallowPlaceholderAfterLoaded

Review-Url: https://codereview.chromium.org/2602793003
Cr-Commit-Position: refs/heads/master@{#455390}

[modify] https://crrev.com/79c699b99525ab50568821fce704976a9278e3fa/third_party/WebKit/Source/core/loader/resource/ImageResource.cpp
[modify] https://crrev.com/79c699b99525ab50568821fce704976a9278e3fa/third_party/WebKit/Source/core/loader/resource/ImageResource.h
[modify] https://crrev.com/79c699b99525ab50568821fce704976a9278e3fa/third_party/WebKit/Source/core/loader/resource/ImageResourceTest.cpp
[modify] https://crrev.com/79c699b99525ab50568821fce704976a9278e3fa/third_party/WebKit/Source/platform/loader/fetch/RawResource.cpp
[modify] https://crrev.com/79c699b99525ab50568821fce704976a9278e3fa/third_party/WebKit/Source/platform/loader/fetch/RawResource.h
[modify] https://crrev.com/79c699b99525ab50568821fce704976a9278e3fa/third_party/WebKit/Source/platform/loader/fetch/RawResourceTest.cpp
[modify] https://crrev.com/79c699b99525ab50568821fce704976a9278e3fa/third_party/WebKit/Source/platform/loader/fetch/Resource.h
[modify] https://crrev.com/79c699b99525ab50568821fce704976a9278e3fa/third_party/WebKit/Source/platform/loader/fetch/ResourceFetcher.cpp

Blockedon: 695808
Blockedon: 632580
Project Member

Comment 32 by bugdroid1@chromium.org, May 8 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/ebd2fa73e3046f3d6397545285a41fa1d78c4be8

commit ebd2fa73e3046f3d6397545285a41fa1d78c4be8
Author: hiroshige <hiroshige@chromium.org>
Date: Mon May 08 20:23:29 2017

Make ImageResourceContent manage its own ResourceStatus

Design doc:
https://docs.google.com/document/d/1O-fB83mrE0B_V8gzXNqHgmRLCvstTB4MMi3RnVLr8bE/edit#

1. This CL
- Adds ImageResourceContent::content_status_,
- Renames ImageResourceContent::GetStatus() to GetContentStatus(), and
- Makes them updated independently from Resource::status_/GetStatus().

This is to decouple GetStatus() timing dependencies
between ImageResource and ImageResourceContent, particularly to allow
ImageResourceContent::GetContentStatus() to be changed to kCached
(in https://codereview.chromium.org/2613853002/) when SVG image loading
is really finished, not necessarily when ResourceLoader is finished.

2. In order to change the content state to kPending when loading is
   started, NotifyStartLoad() notifications are introduced.

3. Stricter assertions are added to |content_status_| transitions.
   |SetStatus(kPending)| and NotifyStartLoad() calls are added to
   non-ResourceLoader paths to make the state transition consistent.

4. This CL makes ImageResourceContent::GetContentStatus() to be kCached
   before ImageNotifyFinished() is called in successful loading,
   while Resource::GetStatus() is kPending at that time.

BUG=667641, 382170,  690480 

Review-Url: https://codereview.chromium.org/2746343002
Cr-Commit-Position: refs/heads/master@{#470104}

[modify] https://crrev.com/ebd2fa73e3046f3d6397545285a41fa1d78c4be8/third_party/WebKit/Source/core/css/cssom/CSSStyleImageValue.h
[modify] https://crrev.com/ebd2fa73e3046f3d6397545285a41fa1d78c4be8/third_party/WebKit/Source/core/html/HTMLObjectElement.cpp
[modify] https://crrev.com/ebd2fa73e3046f3d6397545285a41fa1d78c4be8/third_party/WebKit/Source/core/loader/ImageLoader.cpp
[modify] https://crrev.com/ebd2fa73e3046f3d6397545285a41fa1d78c4be8/third_party/WebKit/Source/core/loader/resource/ImageResource.cpp
[modify] https://crrev.com/ebd2fa73e3046f3d6397545285a41fa1d78c4be8/third_party/WebKit/Source/core/loader/resource/ImageResource.h
[modify] https://crrev.com/ebd2fa73e3046f3d6397545285a41fa1d78c4be8/third_party/WebKit/Source/core/loader/resource/ImageResourceContent.cpp
[modify] https://crrev.com/ebd2fa73e3046f3d6397545285a41fa1d78c4be8/third_party/WebKit/Source/core/loader/resource/ImageResourceContent.h
[modify] https://crrev.com/ebd2fa73e3046f3d6397545285a41fa1d78c4be8/third_party/WebKit/Source/core/loader/resource/ImageResourceInfo.h
[modify] https://crrev.com/ebd2fa73e3046f3d6397545285a41fa1d78c4be8/third_party/WebKit/Source/core/loader/resource/ImageResourceTest.cpp
[modify] https://crrev.com/ebd2fa73e3046f3d6397545285a41fa1d78c4be8/third_party/WebKit/Source/core/loader/resource/MockImageResourceObserver.cpp
[modify] https://crrev.com/ebd2fa73e3046f3d6397545285a41fa1d78c4be8/third_party/WebKit/Source/platform/loader/fetch/Resource.h
[modify] https://crrev.com/ebd2fa73e3046f3d6397545285a41fa1d78c4be8/third_party/WebKit/Source/platform/loader/fetch/ResourceFetcher.cpp
[modify] https://crrev.com/ebd2fa73e3046f3d6397545285a41fa1d78c4be8/third_party/WebKit/Source/web/tests/WebFrameTest.cpp

Project Member

Comment 33 by bugdroid1@chromium.org, May 11 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/29c12a26e644a95e6d53eb7e02c136ca9fdf4c94

commit 29c12a26e644a95e6d53eb7e02c136ca9fdf4c94
Author: hiroshige <hiroshige@chromium.org>
Date: Thu May 11 23:10:57 2017

Fix ImageResourceContent::Create()'s GetContentStatus()

Follow-up of a regression bug caused by [1]:
Previously, ImageResourceContent::Create() returned a content with
kCached, while after [1] it returns a content with kNotStarted.
This causes behavior changes if Create() is called with non-null
argument.

[1] https://codereview.chromium.org/2746343002/

This CL fixes this issue by renaming Create() with a non-null argument
to ImageResourceContent::CreateLoaded() that sets the status to kCached.
The callsites of CreateLoaded() were affected by the bug, i.e.
a callsite in HTMLImageElement and others in tests.

Create() with the nullptr argument is renamed to CreateNotStarted(),
which is only called from ImageResource and is not affected by the bug.

BUG=667641, 382170,  690480 

Review-Url: https://codereview.chromium.org/2869733004
Cr-Commit-Position: refs/heads/master@{#471113}

[modify] https://crrev.com/29c12a26e644a95e6d53eb7e02c136ca9fdf4c94/third_party/WebKit/Source/core/frame/ImageBitmapTest.cpp
[modify] https://crrev.com/29c12a26e644a95e6d53eb7e02c136ca9fdf4c94/third_party/WebKit/Source/core/html/HTMLImageElement.cpp
[modify] https://crrev.com/29c12a26e644a95e6d53eb7e02c136ca9fdf4c94/third_party/WebKit/Source/core/loader/resource/ImageResource.cpp
[modify] https://crrev.com/29c12a26e644a95e6d53eb7e02c136ca9fdf4c94/third_party/WebKit/Source/core/loader/resource/ImageResourceContent.cpp
[modify] https://crrev.com/29c12a26e644a95e6d53eb7e02c136ca9fdf4c94/third_party/WebKit/Source/core/loader/resource/ImageResourceContent.h

Project Member

Comment 34 by bugdroid1@chromium.org, May 16 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/396a1266fb7549e03e5bcff793ac138654e52d3a

commit 396a1266fb7549e03e5bcff793ac138654e52d3a
Author: hiroshige <hiroshige@chromium.org>
Date: Tue May 16 22:17:35 2017

Phase III Step 2: Call imageNotifyFinished() and image load event after SVG loading completes

design doc:
https://docs.google.com/document/d/1O-fB83mrE0B_V8gzXNqHgmRLCvstTB4MMi3RnVLr8bE/edit#
https://docs.google.com/document/d/1obEb8K4mJpG9Y_x4_ZhvnbFHojZoLLzMCFsalkl8voc/edit#

This CL calls ImageResourceObserver::ImageNotifyFinished()
after SVG image loading is completed, not necessarily the same time as
ResourceLoader completion (e.g. DidFinishLoading()).
This makes SVG's <img> load event to be fired after SVG image loading
is completed even when the SVG has subresources loaded asynchronously.

1. Image::SetData() can return SizeAvailableAndLoadingAsynchronously
   when SVG image loading is not completed synchronously (due to
   subresources), and notifies the completion asynchronously via
   ImageObserver:LoadCompleted().
   LocalFrameClient::DispatchDidHandleOnloadEvents() is used as a
   notification of SVG image loading completion in SVGImage.

2. ImageResourceContent delays calling ImageNotifyFinished() and
   setting |content_status_| to kCached until SVG image loading is
   completed.

3. ImageLoader additionally delays the Document load event
   - from the first ImageChanged() during image loading
   - until (potentially delayed) ImageNotifyFinished().

In image loading not by ImageLoader, the Document load event is not
delayed from ResourceLoader completion until ImageNotifyFinished(),
but this is not a regression.

In image loading by ImageLoader, the Document load event is delayed
until ImageNotifyFinished() by the combination of
ResourceFetcher::loaders_ and ImageLoader.
ImageLoader starts delaying the Document load event
- Not from |image_| is set, because we shouldn't delay the Document
  load event when image loading is deferred.
- Not from ImageResourceContent::NotifyStartLoad(), to avoid additional
  ImageResourceObserver callbacks.

BUG=382170,  690480 , 667641

Review-Url: https://codereview.chromium.org/2613853002
Cr-Commit-Position: refs/heads/master@{#472229}

[add] https://crrev.com/396a1266fb7549e03e5bcff793ac138654e52d3a/third_party/WebKit/LayoutTests/svg/as-image/data-font-in-css-crash.html
[add] https://crrev.com/396a1266fb7549e03e5bcff793ac138654e52d3a/third_party/WebKit/LayoutTests/svg/as-image/data-font-in-css-invalid-data-url-crash.html
[add] https://crrev.com/396a1266fb7549e03e5bcff793ac138654e52d3a/third_party/WebKit/LayoutTests/svg/as-image/data-font-in-css-invalid-font-crash.html
[add] https://crrev.com/396a1266fb7549e03e5bcff793ac138654e52d3a/third_party/WebKit/LayoutTests/svg/as-image/resources/data-font-in-css-invalid-data-url.svg
[add] https://crrev.com/396a1266fb7549e03e5bcff793ac138654e52d3a/third_party/WebKit/LayoutTests/svg/as-image/resources/data-font-in-css-invalid-font.svg
[add] https://crrev.com/396a1266fb7549e03e5bcff793ac138654e52d3a/third_party/WebKit/LayoutTests/svg/as-image/resources/data-font-in-css.svg
[modify] https://crrev.com/396a1266fb7549e03e5bcff793ac138654e52d3a/third_party/WebKit/Source/core/loader/ImageLoader.cpp
[modify] https://crrev.com/396a1266fb7549e03e5bcff793ac138654e52d3a/third_party/WebKit/Source/core/loader/ImageLoader.h
[modify] https://crrev.com/396a1266fb7549e03e5bcff793ac138654e52d3a/third_party/WebKit/Source/core/loader/resource/ImageResource.cpp
[modify] https://crrev.com/396a1266fb7549e03e5bcff793ac138654e52d3a/third_party/WebKit/Source/core/loader/resource/ImageResource.h
[modify] https://crrev.com/396a1266fb7549e03e5bcff793ac138654e52d3a/third_party/WebKit/Source/core/loader/resource/ImageResourceContent.cpp
[modify] https://crrev.com/396a1266fb7549e03e5bcff793ac138654e52d3a/third_party/WebKit/Source/core/loader/resource/ImageResourceContent.h
[modify] https://crrev.com/396a1266fb7549e03e5bcff793ac138654e52d3a/third_party/WebKit/Source/core/loader/resource/ImageResourceTest.cpp
[modify] https://crrev.com/396a1266fb7549e03e5bcff793ac138654e52d3a/third_party/WebKit/Source/core/svg/graphics/SVGImage.cpp
[modify] https://crrev.com/396a1266fb7549e03e5bcff793ac138654e52d3a/third_party/WebKit/Source/core/svg/graphics/SVGImage.h
[modify] https://crrev.com/396a1266fb7549e03e5bcff793ac138654e52d3a/third_party/WebKit/Source/core/svg/graphics/SVGImageTest.cpp
[modify] https://crrev.com/396a1266fb7549e03e5bcff793ac138654e52d3a/third_party/WebKit/Source/platform/graphics/BitmapImageTest.cpp
[modify] https://crrev.com/396a1266fb7549e03e5bcff793ac138654e52d3a/third_party/WebKit/Source/platform/graphics/Image.h
[modify] https://crrev.com/396a1266fb7549e03e5bcff793ac138654e52d3a/third_party/WebKit/Source/platform/graphics/ImageObserver.h
[modify] https://crrev.com/396a1266fb7549e03e5bcff793ac138654e52d3a/third_party/WebKit/Source/platform/loader/fetch/Resource.h

Project Member

Comment 35 by bugdroid1@chromium.org, May 22 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/8952091b786ce79e9e10810d2b047d39c420d4d7

commit 8952091b786ce79e9e10810d2b047d39c420d4d7
Author: hiroshige <hiroshige@chromium.org>
Date: Mon May 22 16:24:08 2017

Make ImageObserver::AsyncLoadCompleted() called asynchronously

- To avoid complex things (i.e. ImageNotifyFinished() calls)
  during Document::ImplicitClose() to prevent timing bugs, and
- To make LoadEventFinished() of SVG's document true when
  ImageNotifyFinished() is called, and make the CHECK() in
  https://codereview.chromium.org/2888953004/ hold.

BUG=382170,  690480 , 667641

Review-Url: https://codereview.chromium.org/2897533002
Cr-Commit-Position: refs/heads/master@{#473593}

[modify] https://crrev.com/8952091b786ce79e9e10810d2b047d39c420d4d7/third_party/WebKit/Source/core/svg/graphics/SVGImage.cpp
[modify] https://crrev.com/8952091b786ce79e9e10810d2b047d39c420d4d7/third_party/WebKit/Source/core/svg/graphics/SVGImage.h

Blockedon: 737392

Sign in to add a comment