Remove stale comment

Remove stale comment

cachedImage->imageForLayoutObject() no longer returns
BitmapImages. The surrounding code is still correct since
cachedImage->imageForLayoutObject() may return a SVGImageForContainer
which does not propagate the correct filename extension.

NOTRY=true
BUG=128055
Committed: https://crrev.com/1d915855466f9637c6171c689fffe66bf23d4359
Cr-Commit-Position: refs/heads/master@{#357105}

[davve, 2015-10-30 10:44:33]

Description was changed from

==========
Remove stale comment

cachedImage->imageForLayoutObject() no longer returns
BitmapImages. The surrounding code is still correct since
cachedImage->imageForLayoutObject() would possibly return
SVGImageForContainer object which does not propagate the filename
extension.

NOTRY=true
BUG=128055
==========

to

==========
Remove stale comment

cachedImage->imageForLayoutObject() no longer returns
BitmapImages. The surrounding code is still correct since
cachedImage->imageForLayoutObject() may return a SVGImageForContainer
which does not propagate the correct filename extension.

NOTRY=true
BUG=128055
==========

[davve, 2015-10-30 10:54:22]

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

[davve, 2015-10-30 10:54:23]

Grepping for imageForLayoutObject made me find this comment. I have a
plan for possibly getting rid of ImageResource::imageForLayoutObject
(in favor of plain ImageResource::image()) so this comment required
some kind of action.

I first attempted a rephrase

// Don't use cachedImage->imageForLayoutObject() here as that may return
SVGImageForContainer for SVG Images.
// Users of getImage() want access to the SVGImage, in order to figure out the
filename extensions,
// and the extra container information is irrelevant and filename extension
currently not propagated through.

but didn't really like the result over just removing the
comment, especially if we're trying to get rid of imageForLayoutObject
anyway. WDYT?

[fs, 2015-10-30 11:41:53]

On 2015/10/30 at 10:54:23, davve wrote:
> Grepping for imageForLayoutObject made me find this comment. I have a
> plan for possibly getting rid of ImageResource::imageForLayoutObject
> (in favor of plain ImageResource::image()) so this comment required
> some kind of action.
> 
> I first attempted a rephrase
> 
> // Don't use cachedImage->imageForLayoutObject() here as that may return
SVGImageForContainer for SVG Images.
> // Users of getImage() want access to the SVGImage, in order to figure out the
filename extensions,
> // and the extra container information is irrelevant and filename extension
currently not propagated through.
> 
> but didn't really like the result over just removing the
> comment, especially if we're trying to get rid of imageForLayoutObject
> anyway. WDYT?

My initial feeling is that going for the "underlying image" here will just mean
that the DragImage could be different from what the image actually looks like...
So, it seems if you were to say something like:

// Use the underlying image to make sure the filename extension is correct.

you'd then need to follow it with:

// TODO(...): Won't this mean that the Image used may be incorrectly sized etc.?

I figure that your follow ups will possibly fix this though and remove the need
for a comment?

And, FWIW, just removing it now LGTM, but then maybe we should consider checking
the behavior and possibly file and bug (that your changes would fix)?

[davve, 2015-10-30 14:05:31]

On 2015/10/30 11:41:53, fs wrote:
> My initial feeling is that going for the "underlying image" here will just
mean
> that the DragImage could be different from what the image actually looks
like...
> So, it seems if you were to say something like:
> 
> // Use the underlying image to make sure the filename extension is correct.
> 
> you'd then need to follow it with:
> 
> // TODO(...): Won't this mean that the Image used may be incorrectly sized
etc.?

Using SVGImageForContainer instead of SVGImage wouldn't help the
dragging code as long as it uses Image::imageForCurrentFrame() for
generating the bitmap. As I see it, no use of
SVGImageForContainer::imageForCurrentFrame() can give any sensible
results since the sizes just won't match up.

I suppose the dragging code would have to head towards something like
WebGLRenderingContextBase::drawImageIntoBuffer for a chance to avoid
pixelation, using the hitTestResult.imageRect() as destRect.

> I figure that your follow ups will possibly fix this though and remove the
need
> for a comment?

Well, follow ups won't fix what I speak of above but the need for a
comment goes away.

> And, FWIW, just removing it now LGTM, but then maybe we should consider
checking
> the behavior and possibly file and bug (that your changes would fix)?

https://code.google.com/p/chromium/issues/detail?id=549576 but as
stated above it won't be fixed "automatically".

[fs, 2015-10-30 14:16:19]

On 2015/10/30 at 14:05:31, davve wrote:
> On 2015/10/30 11:41:53, fs wrote:
> > My initial feeling is that going for the "underlying image" here will just
mean
> > that the DragImage could be different from what the image actually looks
like...
> > So, it seems if you were to say something like:
> > 
> > // Use the underlying image to make sure the filename extension is correct.
> > 
> > you'd then need to follow it with:
> > 
> > // TODO(...): Won't this mean that the Image used may be incorrectly sized
etc.?
> 
> Using SVGImageForContainer instead of SVGImage wouldn't help the
> dragging code as long as it uses Image::imageForCurrentFrame() for
> generating the bitmap. As I see it, no use of
> SVGImageForContainer::imageForCurrentFrame() can give any sensible
> results since the sizes just won't match up.

Ah, I see... I thought SVGImageForContainer::imageForCurrentFrame() would
actually make use of the container size... =/

> I suppose the dragging code would have to head towards something like
> WebGLRenderingContextBase::drawImageIntoBuffer for a chance to avoid
> pixelation, using the hitTestResult.imageRect() as destRect.
> 
> > I figure that your follow ups will possibly fix this though and remove the
need
> > for a comment?
> 
> Well, follow ups won't fix what I speak of above but the need for a
> comment goes away.
> 
> > And, FWIW, just removing it now LGTM, but then maybe we should consider
checking
> > the behavior and possibly file and bug (that your changes would fix)?
> 
> https://code.google.com/p/chromium/issues/detail?id=549576 but as
> stated above it won't be fixed "automatically".

Understood.

[davve, 2015-10-30 14:46:51]

The CQ bit was checked by davve@opera.com

[commit-bot: I haz the power, 2015-10-30 14:47:03]

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

[commit-bot: I haz the power, 2015-10-30 14:52:29]

Committed patchset #1 (id:1)

[commit-bot: I haz the power, 2015-10-30 14:53:19]

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