New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 773272 link

Starred by 2 users

Issue metadata

Status: Assigned
Owner:
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 3
Type: Task



Sign in to add a comment

Whittle down ImageResourceContent::ImageSize and GetImage

Project Member Reported by f...@opera.com, Oct 10 2017

Issue description

ImageResourceContent contains a lot of functionality that would be better aligned with layout/paint code (primarily the handling of scaling and fallback/broken images.)
 
Project Member

Comment 1 by bugdroid1@chromium.org, Oct 10 2017

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

commit bb8f4f91cccdaf8f0baeb564aee2c9d2f117b21d
Author: Fredrik Söderquist <fs@opera.com>
Date: Tue Oct 10 22:21:16 2017

Simplify image size handling in ImageDocument

Move the static CachedImageSize(...) helper to the ImageDocument itself,
renaming it to ImageSize() and using |image_element_| as implicit input.
Simplify expression previously involving CachedImageSize to use the
ImageSize() method.

Change other users of ImageResourceContent::ImageSize to use the new
method instead. (Passing a zoom multiplier is redundant if all you want
to check is if the dimensions are empty.)

Simplify ImageDocument::Scale() by eliminating the "page zoom" factor,
which is part of both the numerator and denominator and hence cancel
out. This removes that last user of the PageZoomFactor(...) helper
function, so it can be removed.

Bug: 773272
Change-Id: Iaaa4a39879b9770a68ad95c045747581eeaa0a00
Reviewed-on: https://chromium-review.googlesource.com/709216
Reviewed-by: Philip Rogers <pdr@chromium.org>
Commit-Queue: Fredrik Söderquist <fs@opera.com>
Cr-Commit-Position: refs/heads/master@{#507796}
[modify] https://crrev.com/bb8f4f91cccdaf8f0baeb564aee2c9d2f117b21d/third_party/WebKit/Source/core/html/ImageDocument.cpp
[modify] https://crrev.com/bb8f4f91cccdaf8f0baeb564aee2c9d2f117b21d/third_party/WebKit/Source/core/html/ImageDocument.h

Project Member

Comment 2 by bugdroid1@chromium.org, Oct 13 2017

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

commit 936948502c48693965289070e089df54cc32a273
Author: Fredrik Söderquist <fs@opera.com>
Date: Fri Oct 13 08:38:13 2017

Handle small/zero pixel densities correctly for img.naturalWidth/Height

ImageResourceContent::ImageSize has some special logic for clamping
image dimension that end up as "zero" (LayoutUnits) to one after having
been scaled by the multiplier it is passed.
This "correction" means that for images with a small/zero pixel density,
we'll end up returning 1 even when the density corrected dimension is
(integral) 0.

To handle this, pass 1 as the multiplier to avoid the dimension clamping
and perform the pixel scaling "manually".
While doing this also move the pixel density correction code to
HTMLImageElement, and get rid of the code from ImageResourceContent,
since it only used for <img> naturalWidth/naturalHeight.
This makes ImageResourceContent::SizeType only have a single value, so
remove the enumeration and the argument.

Bug: 773272,  773284 
Change-Id: Ie4a907d6cb7f262c876a5010bb7dd6a92af1cb9a
Reviewed-on: https://chromium-review.googlesource.com/708815
Reviewed-by: Philip Rogers <pdr@chromium.org>
Commit-Queue: Fredrik Söderquist <fs@opera.com>
Cr-Commit-Position: refs/heads/master@{#508646}
[modify] https://crrev.com/936948502c48693965289070e089df54cc32a273/third_party/WebKit/LayoutTests/external/wpt/html/semantics/embedded-content/the-img-element/current-pixel-density/basic-expected.txt
[modify] https://crrev.com/936948502c48693965289070e089df54cc32a273/third_party/WebKit/Source/core/css/cssom/CSSStyleImageValue.h
[modify] https://crrev.com/936948502c48693965289070e089df54cc32a273/third_party/WebKit/Source/core/html/HTMLImageElement.cpp
[modify] https://crrev.com/936948502c48693965289070e089df54cc32a273/third_party/WebKit/Source/core/html/HTMLImageElement.h
[modify] https://crrev.com/936948502c48693965289070e089df54cc32a273/third_party/WebKit/Source/core/loader/resource/ImageResourceContent.cpp
[modify] https://crrev.com/936948502c48693965289070e089df54cc32a273/third_party/WebKit/Source/core/loader/resource/ImageResourceContent.h

Project Member

Comment 3 by bugdroid1@chromium.org, Oct 18 2017

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

commit c308c18b97e5176bb9501a6b8fd19cb081742cf7
Author: Fredrik Söderquist <fs@opera.com>
Date: Wed Oct 18 15:04:39 2017

Introduce ImageResourceContent::IntrinsicSize

Most non-layout/paint users that query the size of an image are
interested in the intrinsic size of the image (usually the physical
pixel dimensions.) This is currently achieved by calling the ImageSize()
method with a multiplier of 1.
Split the ImageSize() method into a part that extracts the size of the
image (if any) and one that applies the multiplier (and follow-on
adjustments.) This should allow a future cleanup where the scaling is
moved closer to the layer that uses it - i.e layout and paint.

This also reduces the type-impedance a bit since IntrinsicSize can just
return an IntSize (like Image::Size()) rather than passing through a
LayoutSize.

Bug: 773272
Change-Id: I18a1c11e9fc891dc6f19c4b2c5bd5dc23c1a5da7
Reviewed-on: https://chromium-review.googlesource.com/718747
Reviewed-by: Stephen Chenney <schenney@chromium.org>
Commit-Queue: Fredrik Söderquist <fs@opera.com>
Cr-Commit-Position: refs/heads/master@{#509770}
[modify] https://crrev.com/c308c18b97e5176bb9501a6b8fd19cb081742cf7/third_party/WebKit/Source/core/css/cssom/CSSStyleImageValue.cpp
[modify] https://crrev.com/c308c18b97e5176bb9501a6b8fd19cb081742cf7/third_party/WebKit/Source/core/css/cssom/CSSStyleImageValue.h
[modify] https://crrev.com/c308c18b97e5176bb9501a6b8fd19cb081742cf7/third_party/WebKit/Source/core/css/cssom/CSSStyleImageValueTest.cpp
[modify] https://crrev.com/c308c18b97e5176bb9501a6b8fd19cb081742cf7/third_party/WebKit/Source/core/html/HTMLImageElement.cpp
[modify] https://crrev.com/c308c18b97e5176bb9501a6b8fd19cb081742cf7/third_party/WebKit/Source/core/html/ImageDocument.cpp
[modify] https://crrev.com/c308c18b97e5176bb9501a6b8fd19cb081742cf7/third_party/WebKit/Source/core/html/canvas/ImageElementBase.cpp
[modify] https://crrev.com/c308c18b97e5176bb9501a6b8fd19cb081742cf7/third_party/WebKit/Source/core/html/forms/ImageInputType.cpp
[modify] https://crrev.com/c308c18b97e5176bb9501a6b8fd19cb081742cf7/third_party/WebKit/Source/core/loader/resource/ImageResourceContent.cpp
[modify] https://crrev.com/c308c18b97e5176bb9501a6b8fd19cb081742cf7/third_party/WebKit/Source/core/loader/resource/ImageResourceContent.h

Project Member

Comment 4 by bugdroid1@chromium.org, Oct 19 2017

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

commit b340c736f1e0358cbc5bc1505aed76595f740431
Author: Fredrik Söderquist <fs@opera.com>
Date: Thu Oct 19 12:44:11 2017

Fold away ImageResourceContent::ImageSize

This moves scale-handling out into the (now former) consumers of the
ImageResourceContent::ImageSize method.

Bug: 773272
Change-Id: I08760b181a48f8a5dcc5ceff68a19c67eba3601a
Reviewed-on: https://chromium-review.googlesource.com/726087
Commit-Queue: Fredrik Söderquist <fs@opera.com>
Reviewed-by: Stephen Chenney <schenney@chromium.org>
Cr-Commit-Position: refs/heads/master@{#510061}
[modify] https://crrev.com/b340c736f1e0358cbc5bc1505aed76595f740431/third_party/WebKit/Source/core/layout/LayoutImageResource.cpp
[modify] https://crrev.com/b340c736f1e0358cbc5bc1505aed76595f740431/third_party/WebKit/Source/core/loader/resource/ImageResourceContent.cpp
[modify] https://crrev.com/b340c736f1e0358cbc5bc1505aed76595f740431/third_party/WebKit/Source/core/loader/resource/ImageResourceContent.h
[modify] https://crrev.com/b340c736f1e0358cbc5bc1505aed76595f740431/third_party/WebKit/Source/core/style/StyleFetchedImage.cpp
[modify] https://crrev.com/b340c736f1e0358cbc5bc1505aed76595f740431/third_party/WebKit/Source/core/style/StyleFetchedImageSet.cpp
[modify] https://crrev.com/b340c736f1e0358cbc5bc1505aed76595f740431/third_party/WebKit/Source/core/style/StyleImage.cpp
[modify] https://crrev.com/b340c736f1e0358cbc5bc1505aed76595f740431/third_party/WebKit/Source/core/style/StyleImage.h

Project Member

Comment 5 by bugdroid1@chromium.org, Oct 25 2017

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

commit 34e720ee228365ca85295f0ab6f98b22656c6eb7
Author: Fredrik Söderquist <fs@opera.com>
Date: Wed Oct 25 18:08:57 2017

Simplify HTMLImageElement.currentSrc 'current request' heuristics

Rather than depending on whatever ImageResourceContent::GetImage()
returns (which is also always something non-null), just explicitly check
for there being an error or an actual image.

Bug: 773272
Change-Id: I1e71837ccdc4f9d2bf2a066cece6451154fecbf3
Reviewed-on: https://chromium-review.googlesource.com/730755
Reviewed-by: Hiroshige Hayashizaki <hiroshige@chromium.org>
Commit-Queue: Fredrik Söderquist <fs@opera.com>
Cr-Commit-Position: refs/heads/master@{#511517}
[modify] https://crrev.com/34e720ee228365ca85295f0ab6f98b22656c6eb7/third_party/WebKit/Source/core/html/HTMLImageElement.cpp

Project Member

Comment 6 by bugdroid1@chromium.org, Oct 25 2017

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

commit 31104dd2cd39ac16c637c9a27e066ecdf486c0f0
Author: Fredrik Söderquist <fs@opera.com>
Date: Wed Oct 25 18:38:21 2017

Move ImageResourceContent::BrokenImage to LayoutImageResource

A "broken image" (fallback image) only make sense for "content images",
so LayoutImageResource seems like a better home for it. This also makes
it trivial to provide a decent device scale factor.
Fold the creation of a fallback image into LayoutImageResource too.

Bug: 773272
Change-Id: I3732f87ff8b433188920bfee9ab44acee255f6ae
Reviewed-on: https://chromium-review.googlesource.com/727904
Reviewed-by: Stephen Chenney <schenney@chromium.org>
Commit-Queue: Fredrik Söderquist <fs@opera.com>
Cr-Commit-Position: refs/heads/master@{#511533}
[modify] https://crrev.com/31104dd2cd39ac16c637c9a27e066ecdf486c0f0/third_party/WebKit/Source/core/html/HTMLImageElement.cpp
[modify] https://crrev.com/31104dd2cd39ac16c637c9a27e066ecdf486c0f0/third_party/WebKit/Source/core/layout/LayoutImageResource.cpp
[modify] https://crrev.com/31104dd2cd39ac16c637c9a27e066ecdf486c0f0/third_party/WebKit/Source/core/layout/LayoutImageResource.h
[modify] https://crrev.com/31104dd2cd39ac16c637c9a27e066ecdf486c0f0/third_party/WebKit/Source/core/loader/resource/ImageResourceContent.cpp
[modify] https://crrev.com/31104dd2cd39ac16c637c9a27e066ecdf486c0f0/third_party/WebKit/Source/core/loader/resource/ImageResourceContent.h

Project Member

Comment 7 by bugdroid1@chromium.org, Oct 31 2017

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

commit e6bc5e85493b29ec1f7606bcf2cd5cede242d118
Author: Fredrik Söderquist <fs@opera.com>
Date: Tue Oct 31 19:48:47 2017

Fold ImageResourceContent::{Uses,}Image{Container,HasRelative}Size

These methods - UsesImageContainerSize and ImageHasRelativeSize - are
only simple wrappers for similarly named methods on Image. Callers have
access to Image without any hoop-jumping, so just fold these methods.

Bug: 773272
Change-Id: Ie6a02b7e1c4643c5ebfafa0b6281503194904852
Reviewed-on: https://chromium-review.googlesource.com/747102
Reviewed-by: Stephen Chenney <schenney@chromium.org>
Commit-Queue: Fredrik Söderquist <fs@opera.com>
Cr-Commit-Position: refs/heads/master@{#512912}
[modify] https://crrev.com/e6bc5e85493b29ec1f7606bcf2cd5cede242d118/third_party/WebKit/Source/core/layout/LayoutImageResource.cpp
[modify] https://crrev.com/e6bc5e85493b29ec1f7606bcf2cd5cede242d118/third_party/WebKit/Source/core/layout/LayoutImageResource.h
[modify] https://crrev.com/e6bc5e85493b29ec1f7606bcf2cd5cede242d118/third_party/WebKit/Source/core/loader/resource/ImageResourceContent.cpp
[modify] https://crrev.com/e6bc5e85493b29ec1f7606bcf2cd5cede242d118/third_party/WebKit/Source/core/loader/resource/ImageResourceContent.h
[modify] https://crrev.com/e6bc5e85493b29ec1f7606bcf2cd5cede242d118/third_party/WebKit/Source/core/style/StyleFetchedImage.cpp
[modify] https://crrev.com/e6bc5e85493b29ec1f7606bcf2cd5cede242d118/third_party/WebKit/Source/core/style/StyleFetchedImageSet.cpp

Project Member

Comment 8 by bugdroid1@chromium.org, Nov 14 2017

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

commit f0b40393c8e73520c792f33139b972470a7c6da0
Author: Fredrik Söderquist <fs@opera.com>
Date: Tue Nov 14 21:37:51 2017

Reimplement ImageElementBase::IsOpaque

Don't use Element::ImageContents(), which is a method primarily meant
for the WebElement interface (and not actually implemented by
SVGImageElement.) Use the local ImageLoader and related helpers instead.

Bug: 773272
Change-Id: Idbbf8a85b6905e072db2bf4ac12e38ff9756025d
Reviewed-on: https://chromium-review.googlesource.com/768872
Reviewed-by: Stephen Chenney <schenney@chromium.org>
Commit-Queue: Fredrik Söderquist <fs@opera.com>
Cr-Commit-Position: refs/heads/master@{#516437}
[modify] https://crrev.com/f0b40393c8e73520c792f33139b972470a7c6da0/third_party/WebKit/Source/core/html/canvas/ImageElementBase.cpp

Project Member

Comment 9 by bugdroid1@chromium.org, Nov 15 2017

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

commit 59ba463d065e7949ba1949b7adcd791118d0c795
Author: Fredrik Söderquist <fs@opera.com>
Date: Wed Nov 15 00:02:18 2017

Tidy ImageResourceContent Image accesses and related code

Make better use of local variables to make the code a bit more compact
and improve readability. Also rename variables to attempt to better
differentiate between ImageResourceContent and Image. In general call
the former |image_content| and the latter |image|.

Bug: 773272
Cq-Include-Trybots: master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
Change-Id: I0c1a1a1020cfc3a515459104e1bdb37827553455
Reviewed-on: https://chromium-review.googlesource.com/768819
Reviewed-by: Stephen Chenney <schenney@chromium.org>
Commit-Queue: Fredrik Söderquist <fs@opera.com>
Cr-Commit-Position: refs/heads/master@{#516511}
[modify] https://crrev.com/59ba463d065e7949ba1949b7adcd791118d0c795/third_party/WebKit/Source/core/clipboard/DataTransfer.cpp
[modify] https://crrev.com/59ba463d065e7949ba1949b7adcd791118d0c795/third_party/WebKit/Source/core/html/HTMLImageElement.cpp
[modify] https://crrev.com/59ba463d065e7949ba1949b7adcd791118d0c795/third_party/WebKit/Source/core/html/canvas/ImageElementBase.cpp
[modify] https://crrev.com/59ba463d065e7949ba1949b7adcd791118d0c795/third_party/WebKit/Source/core/layout/LayoutImage.cpp
[modify] https://crrev.com/59ba463d065e7949ba1949b7adcd791118d0c795/third_party/WebKit/Source/core/layout/LayoutImageResource.cpp
[modify] https://crrev.com/59ba463d065e7949ba1949b7adcd791118d0c795/third_party/WebKit/Source/core/loader/ImageLoader.cpp
[modify] https://crrev.com/59ba463d065e7949ba1949b7adcd791118d0c795/third_party/WebKit/Source/core/paint/SVGImagePainter.cpp
[modify] https://crrev.com/59ba463d065e7949ba1949b7adcd791118d0c795/third_party/WebKit/Source/core/style/StyleFetchedImage.cpp
[modify] https://crrev.com/59ba463d065e7949ba1949b7adcd791118d0c795/third_party/WebKit/Source/core/style/StyleFetchedImageSet.cpp
[modify] https://crrev.com/59ba463d065e7949ba1949b7adcd791118d0c795/third_party/WebKit/Source/core/svg/SVGFEImageElement.cpp
[modify] https://crrev.com/59ba463d065e7949ba1949b7adcd791118d0c795/third_party/WebKit/Source/core/svg/SVGImageElement.cpp

Project Member

Comment 11 by bugdroid1@chromium.org, Nov 27 2017

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

commit ea033d2f5cd22369395948cad3c0f1317198f4ff
Author: Fredrik Söderquist <fs@opera.com>
Date: Mon Nov 27 21:04:45 2017

Override CanRender in StyleInvalidImage

A StyleInvalidImage can never be rendered (GetImage always return
nullptr), so returning true - like the base class does - seems to be
sending the wrong signals.

Bug: 773272
Change-Id: I5ca20db512ce8b0e92546d626a4a6a2d85ed7908
Reviewed-on: https://chromium-review.googlesource.com/785651
Reviewed-by: Stephen Chenney <schenney@chromium.org>
Commit-Queue: Fredrik Söderquist <fs@opera.com>
Cr-Commit-Position: refs/heads/master@{#519390}
[modify] https://crrev.com/ea033d2f5cd22369395948cad3c0f1317198f4ff/third_party/WebKit/Source/core/style/StyleInvalidImage.h

Project Member

Comment 12 by bugdroid1@chromium.org, Apr 30 2018

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

commit 8e638dae34742b3a9e78b575ed089ad6cda3a59e
Author: Fredrik Söderquist <fs@opera.com>
Date: Mon Apr 30 15:44:45 2018

Remove (trivially) unneeded ImageResourceContent::IntrinsicSize calls

Since SVGImage is handled explicitly already, and we pass
kDoNotRespectImageOrientation to IntrinsicSize() in these functions,
just call Image::Size() directly rather than passing through the
ImageResourceContent method.

Bug: 773272
Change-Id: I6b7c24f5be425f04cd2f68cbc4612054b635bc2b
Reviewed-on: https://chromium-review.googlesource.com/1032745
Reviewed-by: Stephen Chenney <schenney@chromium.org>
Commit-Queue: Fredrik Söderquist <fs@opera.com>
Cr-Commit-Position: refs/heads/master@{#554763}
[modify] https://crrev.com/8e638dae34742b3a9e78b575ed089ad6cda3a59e/third_party/blink/renderer/core/style/style_fetched_image.cc
[modify] https://crrev.com/8e638dae34742b3a9e78b575ed089ad6cda3a59e/third_party/blink/renderer/core/style/style_fetched_image_set.cc

Sign in to add a comment