Rename imageSizeForLayoutObject() to imageSize()

Rename imageSizeForLayoutObject() to imageSize()

No need to pass the layout object anymore. It only uses the layout
object for knowing whether to respect image orientation or not. We
might as well pass that explicitly.

BUG=559131
Committed: https://crrev.com/38a64c16c73b14e52fc8246eb7819a64d36f95d4
Cr-Commit-Position: refs/heads/master@{#361892}

[davve, 2015-11-23 11:10:19]

The CQ bit was checked by davve@opera.com to run a CQ dry run

[commit-bot: I haz the power, 2015-11-23 11:10:28]

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

[davve, 2015-11-23 11:11:25]

davve@opera.com changed reviewers:
+ fmalita@chromium.org, fs@opera.com

[davve, 2015-11-23 11:11:26]

PTAL

[fs, 2015-11-23 11:34:36]

fs@opera.com changed reviewers:
+ dmazzoni@chromium.org, junov@chromium.org

[fs, 2015-11-23 11:34:36]

LGTM, but it would be nice if we could cut down on the boilerplate a little
bit...

+junov for modules/imagebitmap
+dmazzoni for modules/accessibility

[commit-bot: I haz the power, 2015-11-23 12:19:04]

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

[commit-bot: I haz the power, 2015-11-23 12:19:05]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[davve, 2015-11-23 12:49:17]

The CQ bit was checked by davve@opera.com to run a CQ dry run

[commit-bot: I haz the power, 2015-11-23 12:49:28]

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

[commit-bot: I haz the power, 2015-11-23 13:57:35]

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

[commit-bot: I haz the power, 2015-11-23 13:57:36]

Dry run: This issue passed the CQ dry run.

[Yoav Weiss, 2015-11-24 10:29:06]

yoav@yoav.ws changed reviewers:
+ yoav@yoav.ws

[Yoav Weiss, 2015-11-24 10:29:07]

A few comments to tidy things up :)

https://codereview.chromium.org/1468023002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/fetch/ImageResource.h (right):

https://codereview.chromium.org/1468023002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/fetch/ImageResource.h:72: bool canRender() {
return !errorOccurred() && !imageSize(DoNotRespectImageOrientation,
1).isEmpty(); }
It adds an extra call, but it might be better to have the
shouldRespectImageOrientation in a single location and just call it here with
nullptr rather than `DoNotRespectImageOrientation`

https://codereview.chromium.org/1468023002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/html/HTMLImageElement.cpp (right):

https://codereview.chromium.org/1468023002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/HTMLImageElement.cpp:442: return
imageLoader().image()->imageSize(DoNotRespectImageOrientation, 1.0f).width();
Are you sure this does not change behavior? 

I'd go with
s/DoNotRespectImageOrientation/shouldRespectImageOrientation(layoutObject())/

https://codereview.chromium.org/1468023002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/HTMLImageElement.cpp:463: return
imageLoader().image()->imageSize(DoNotRespectImageOrientation, 1.0f).height();
ditto

https://codereview.chromium.org/1468023002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/html/forms/ImageInputType.cpp (right):

https://codereview.chromium.org/1468023002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/forms/ImageInputType.cpp:209: return
imageLoader->image()->imageSize(DoNotRespectImageOrientation, 1).height();
Again, I'm not sure this doesn't change behavior

https://codereview.chromium.org/1468023002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/forms/ImageInputType.cpp:231: return
imageLoader->image()->imageSize(DoNotRespectImageOrientation, 1).width();
ditto

https://codereview.chromium.org/1468023002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/LayoutImageResource.cpp (right):

https://codereview.chromium.org/1468023002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutImageResource.cpp:99: LayoutSize
size = m_cachedImage->imageSize(shouldRespectImageOrientation, multiplier);
This repetition of the shouldRespectImageOrientation logic is not needed. You
could replace it with `shouldRespectImageOrientation(m_layoutObject)`

https://codereview.chromium.org/1468023002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/style/StyleFetchedImage.cpp (right):

https://codereview.chromium.org/1468023002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/style/StyleFetchedImage.cpp:88: return
m_image->imageSize(shouldRespectImageOrientation, multiplier);
Here too, no need to repeat the same logic

https://codereview.chromium.org/1468023002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/style/StyleFetchedImageSet.cpp (right):

https://codereview.chromium.org/1468023002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/style/StyleFetchedImageSet.cpp:90: LayoutSize
scaledImageSize = m_bestFitImage->imageSize(shouldRespectImageOrientation,
multiplier);
and here as well :)

https://codereview.chromium.org/1468023002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/accessibility/AXLayoutObject.cpp (right):

https://codereview.chromium.org/1468023002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/accessibility/AXLayoutObject.cpp:768:
LayoutSize imageSize =
image->cachedImage()->imageSize(shouldRespectImageOrientation,
image->view()->zoomFactor());
ditto

https://codereview.chromium.org/1468023002/diff/20001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/modules/imagebitmap/WindowImageBitmapFactories.cpp
(right):

https://codereview.chromium.org/1468023002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/imagebitmap/WindowImageBitmapFactories.cpp:62:
return cachedImage->imageSize(shouldRespectImageOrientation, 1.0f); // FIXME:
Not sure about this.
ditto

[davve, 2015-11-24 12:49:14]

https://codereview.chromium.org/1468023002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/html/HTMLImageElement.cpp (right):

https://codereview.chromium.org/1468023002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/HTMLImageElement.cpp:442: return
imageLoader().image()->imageSize(DoNotRespectImageOrientation, 1.0f).width();
On 2015/11/24 10:29:07, Yoav Weiss wrote:
> Are you sure this does not change behavior? 
> 
> I'd go with
> s/DoNotRespectImageOrientation/shouldRespectImageOrientation(layoutObject())/

This is the layoutObject() == null code path.

The alternative would be shouldRespectImageOrientation(nullptr). The upside of
that would be to keep the number of places we encode "no layout object means
DoNotRespectImageOrientation" down.

https://codereview.chromium.org/1468023002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/HTMLImageElement.cpp:463: return
imageLoader().image()->imageSize(DoNotRespectImageOrientation, 1.0f).height();
On 2015/11/24 10:29:07, Yoav Weiss wrote:
> ditto

See other reply.

https://codereview.chromium.org/1468023002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/html/forms/ImageInputType.cpp (right):

https://codereview.chromium.org/1468023002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/forms/ImageInputType.cpp:209: return
imageLoader->image()->imageSize(DoNotRespectImageOrientation, 1).height();
On 2015/11/24 10:29:07, Yoav Weiss wrote:
> Again, I'm not sure this doesn't change behavior

See other reply.

https://codereview.chromium.org/1468023002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/forms/ImageInputType.cpp:231: return
imageLoader->image()->imageSize(DoNotRespectImageOrientation, 1).width();
On 2015/11/24 10:29:07, Yoav Weiss wrote:
> ditto

See other reply.

https://codereview.chromium.org/1468023002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/LayoutImageResource.cpp (right):

https://codereview.chromium.org/1468023002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutImageResource.cpp:99: LayoutSize
size = m_cachedImage->imageSize(shouldRespectImageOrientation, multiplier);
On 2015/11/24 10:29:07, Yoav Weiss wrote:
> This repetition of the shouldRespectImageOrientation logic is not needed. You
> could replace it with `shouldRespectImageOrientation(m_layoutObject)`

Having such a global function has struck my mind, yes. I haven't any yet, but I
could of course make one.

https://codereview.chromium.org/1468023002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/style/StyleFetchedImage.cpp (right):

https://codereview.chromium.org/1468023002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/style/StyleFetchedImage.cpp:88: return
m_image->imageSize(shouldRespectImageOrientation, multiplier);
On 2015/11/24 10:29:07, Yoav Weiss wrote:
> Here too, no need to repeat the same logic

If I had such a function, yes. (again)

https://codereview.chromium.org/1468023002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/style/StyleFetchedImageSet.cpp (right):

https://codereview.chromium.org/1468023002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/style/StyleFetchedImageSet.cpp:90: LayoutSize
scaledImageSize = m_bestFitImage->imageSize(shouldRespectImageOrientation,
multiplier);
On 2015/11/24 10:29:07, Yoav Weiss wrote:
> and here as well :)

If I had such a function, yes. (again)

https://codereview.chromium.org/1468023002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/accessibility/AXLayoutObject.cpp (right):

https://codereview.chromium.org/1468023002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/accessibility/AXLayoutObject.cpp:768:
LayoutSize imageSize =
image->cachedImage()->imageSize(shouldRespectImageOrientation,
image->view()->zoomFactor());
On 2015/11/24 10:29:07, Yoav Weiss wrote:
> ditto

If I had such a function, yes. (again)

https://codereview.chromium.org/1468023002/diff/20001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/modules/imagebitmap/WindowImageBitmapFactories.cpp
(right):

https://codereview.chromium.org/1468023002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/imagebitmap/WindowImageBitmapFactories.cpp:62:
return cachedImage->imageSize(shouldRespectImageOrientation, 1.0f); // FIXME:
Not sure about this.
On 2015/11/24 10:29:07, Yoav Weiss wrote:
> ditto

If I had such a function, yes. (again)

[Yoav Weiss, 2015-11-24 13:06:57]

https://codereview.chromium.org/1468023002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/html/HTMLImageElement.cpp (right):

https://codereview.chromium.org/1468023002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/HTMLImageElement.cpp:442: return
imageLoader().image()->imageSize(DoNotRespectImageOrientation, 1.0f).width();
On 2015/11/24 12:49:13, David Vest wrote:
> On 2015/11/24 10:29:07, Yoav Weiss wrote:
> > Are you sure this does not change behavior? 
> > 
> > I'd go with
> >
s/DoNotRespectImageOrientation/shouldRespectImageOrientation(layoutObject())/
> 
> This is the layoutObject() == null code path.
> 
> The alternative would be shouldRespectImageOrientation(nullptr). The upside of
> that would be to keep the number of places we encode "no layout object means
> DoNotRespectImageOrientation" down.

Fair enough. I would prefer shouldRespectImageOrientation(nullptr) then

https://codereview.chromium.org/1468023002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/LayoutImageResource.cpp (right):

https://codereview.chromium.org/1468023002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutImageResource.cpp:99: LayoutSize
size = m_cachedImage->imageSize(shouldRespectImageOrientation, multiplier);
On 2015/11/24 12:49:13, David Vest wrote:
> On 2015/11/24 10:29:07, Yoav Weiss wrote:
> > This repetition of the shouldRespectImageOrientation logic is not needed.
You
> > could replace it with `shouldRespectImageOrientation(m_layoutObject)`
> 
> Having such a global function has struck my mind, yes. I haven't any yet, but
I
> could of course make one.

Can you just call shouldRespectImageOrientation() for that purpose?

[davve, 2015-11-24 13:33:34]

https://codereview.chromium.org/1468023002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/fetch/ImageResource.h (right):

https://codereview.chromium.org/1468023002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/fetch/ImageResource.h:72: bool canRender() {
return !errorOccurred() && !imageSize(DoNotRespectImageOrientation,
1).isEmpty(); }
On 2015/11/24 10:29:07, Yoav Weiss wrote:
> It adds an extra call, but it might be better to have the
> shouldRespectImageOrientation in a single location and just call it here with
> nullptr rather than `DoNotRespectImageOrientation`

fetch/ shouldn't depend on the rest of core/ (see fetch/DEPS). Not having to
call LayoutObject code from fetch/ is a big part of this CL, even though I'm not
sure we're able to remove every dependency at this point.

If you really like to, I can add the enum parameter to canRender() and pass it
along to imageSize?

[davve, 2015-11-24 14:25:27]

The CQ bit was checked by davve@opera.com to run a CQ dry run

[commit-bot: I haz the power, 2015-11-24 14:26:13]

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

[Yoav Weiss, 2015-11-24 14:59:05]

LGTM % nit

I have a slight preference to having the shouldRespectImageOrientation function
be a static on LayoutObject (So, slight preference to PS3)

https://codereview.chromium.org/1468023002/diff/40001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/modules/imagebitmap/WindowImageBitmapFactories.cpp
(right):

https://codereview.chromium.org/1468023002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/imagebitmap/WindowImageBitmapFactories.cpp:59:
LayoutObject* layoutObject = image->layoutObject();
I think the extra variable is not really needed

[davve, 2015-11-24 17:00:45]

dmazzoni and Justin Novosad, a friendly ping for a look at modules?

[davve, 2015-11-24 17:01:17]

The CQ bit was checked by davve@opera.com to run a CQ dry run

[commit-bot: I haz the power, 2015-11-24 17:04:32]

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

[commit-bot: I haz the power, 2015-11-24 18:51:47]

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

[commit-bot: I haz the power, 2015-11-24 18:51:48]

Dry run: This issue passed the CQ dry run.

[xidachen, 2015-11-26 02:49:00]

davve@: Is there a timeline on this CL? I would hope that this CL can be
committed soon because my CL here: https://codereview.chromium.org/1455763002/
will be using your imageSize() function.

Thanks.

[davve, 2015-11-26 07:55:57]

On 2015/11/26 02:49:00, xidachen wrote:
> davve@: Is there a timeline on this CL? I would hope that this CL can be
> committed soon because my CL here: https://codereview.chromium.org/1455763002/
> will be using your imageSize() function.
> 
> Thanks.

I'm waiting for review in modules/. I pinged one and a half day ago, but perhaps
it's time again. Or add more reviewers. I'm not sure.

[Yoav Weiss, 2015-11-26 08:00:19]

On 2015/11/26 07:55:57, David Vest wrote:
> On 2015/11/26 02:49:00, xidachen wrote:
> > davve@: Is there a timeline on this CL? I would hope that this CL can be
> > committed soon because my CL here:
https://codereview.chromium.org/1455763002/
> > will be using your imageSize() function.
> > 
> > Thanks.
> 
> I'm waiting for review in modules/. I pinged one and a half day ago, but
perhaps
> it's time again. Or add more reviewers. I'm not sure.

Thanksgiving week can be rough when it comes to reviews (or if you're a turkey).
Maybe try to ping non-US based reviewers (modules/OWNERS has a bunch of them
listed)

[davve, 2015-11-26 08:02:28]

davve@opera.com changed reviewers:
+ jochen@chromium.org

[davve, 2015-11-26 08:02:28]

Jochen, do you think you can rubberstamp modules/? Core owners has approved and
Justin has expressed positive remarks on this review at
https://codereview.chromium.org/1455763002/#msg8 but hasn't gotten around to
looking at it. (Thanksgiving and all?)

[davve, 2015-11-26 13:16:06]

davve@opera.com changed reviewers:
+ philipj@opera.com

[davve, 2015-11-26 13:16:06]

Philip, can you take a look at modules/?

[jochen (gone - plz use gerrit), 2015-11-26 13:18:08]

lgtm

[davve, 2015-11-26 13:38:30]

The CQ bit was checked by davve@opera.com

[davve, 2015-11-26 13:38:31]

The patchset sent to the CQ was uploaded after l-g-t-m from fs@opera.com,
yoav@yoav.ws
Link to the patchset: https://codereview.chromium.org/1468023002/#ps80001
(title: "Bring back ps#3 and fix nit")

[commit-bot: I haz the power, 2015-11-26 13:38:42]

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

[xidachen, 2015-11-26 14:16:24]

On 2015/11/26 13:38:42, commit-bot: I haz the power wrote:
> CQ is trying da patch. Follow status at
>  https://chromium-cq-status.appspot.com/patch-status/1468023002/80001
> View timeline at
>  https://chromium-cq-status.appspot.com/patch-timeline/1468023002/80001

Thank you David, two thumbs up!

[commit-bot: I haz the power, 2015-11-26 14:57:41]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2015-11-26 14:59:09]

Description was changed from

==========
Rename imageSizeForLayoutObject() to imageSize()

No need to pass the layout object anymore. It only uses the layout
object for knowing whether to respect image orientation or not. We
might as well pass that explicitly.

BUG=559131
==========

to

==========
Rename imageSizeForLayoutObject() to imageSize()

No need to pass the layout object anymore. It only uses the layout
object for knowing whether to respect image orientation or not. We
might as well pass that explicitly.

BUG=559131

Committed: https://crrev.com/38a64c16c73b14e52fc8246eb7819a64d36f95d4
Cr-Commit-Position: refs/heads/master@{#361892}
==========

[commit-bot: I haz the power, 2015-11-26 14:59:10]

Patchset 5 (id:??) landed as
https://crrev.com/38a64c16c73b14e52fc8246eb7819a64d36f95d4
Cr-Commit-Position: refs/heads/master@{#361892}

[Justin Novosad, 2015-11-27 00:58:26]

post commit lgtm