Document SVGImageForContainer with a class-level comment

Document SVGImageForContainer with a class-level comment

SVGImageForContainer is confusing and needs better documentation. This
patch adds a class-level comment in SVGImageForContainer.h that
describes the dimension and fragment url quirks that require this class.

BUG=644913
NOTRY=true

Committed: https://crrev.com/51023c152e2acb949eedd5e5cc500622f1bfcc87
Cr-Commit-Position: refs/heads/master@{#417400}

[pdr., 2016-09-07 23:27:34]

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

[pdr., 2016-09-07 23:27:45]

Description was changed from

==========
Document SVGImageForContainer with a class-level comment

SVGImageForContainer is confusing and needs better documentation. This
patch adds a class-level comment in SVGImageForContainer.h that
describes the dimension and fragment url quirks that require this class.

BUG=644913
==========

to

==========
Document SVGImageForContainer with a class-level comment

SVGImageForContainer is confusing and needs better documentation. This
patch adds a class-level comment in SVGImageForContainer.h that
describes the dimension and fragment url quirks that require this class.

BUG=644913
NOTRY=true
==========

[pdr., 2016-09-07 23:29:50]

Happy to shed bikes on this one if you have suggestions for making it clearer.

[fs, 2016-09-08 09:23:48]

LGTM w/ some freebie suggestions

https://codereview.chromium.org/2322803002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/svg/graphics/SVGImageForContainer.h (right):

https://codereview.chromium.org/2322803002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/svg/graphics/SVGImageForContainer.h:38: //
SVGImageForContainer contains a reference to an Image (SVGImage*) and
Nit: Maybe drop the '*'. This seems to refer to the actual reference in
SVGImageForContainer, but maybe it would suffice to just mention the concrete
(sub)class of Image (and it's always an SVGImage after all - by necessity.)

https://codereview.chromium.org/2322803002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/svg/graphics/SVGImageForContainer.h:44: // single
SVGImage is created per svg image resource, but this SVGImage can
Nit: s/svg image/SVG image/ for consistency

https://codereview.chromium.org/2322803002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/svg/graphics/SVGImageForContainer.h:45: // be
referenced multiple times by different container sizes. Similarly, each
Nit: ...by containers of different sizes.

(I.e the SVGImage is referenced by the sizes, but the containers may be sized
differently.)

https://codereview.chromium.org/2322803002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/svg/graphics/SVGImageForContainer.h:46: // use of
an image can contain a different fragment url (foo.svg#abc) which
Nit: ...may have a different fragment identifier as part of its URL
(foo.svg#abc), which can influence rendering.

https://codereview.chromium.org/2322803002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/svg/graphics/SVGImageForContainer.h:51: // <img
src='foo.svg#a' width='20'><img src='foo.svg#b' width='10'>
This would be the case even without the fragment parts. Maybe make this two
examples:

<img src="foo.svg" width="20"><img src="foo.svg" width="10">

or

<img src="foo.svg#a" width="20">...

[pdr., 2016-09-08 21:06:46]

Thanks!

Could you do one more pass? If it looks good to you, please click commit.

https://codereview.chromium.org/2322803002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/svg/graphics/SVGImageForContainer.h (right):

https://codereview.chromium.org/2322803002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/svg/graphics/SVGImageForContainer.h:38: //
SVGImageForContainer contains a reference to an Image (SVGImage*) and
On 2016/09/08 at 09:23:48, fs wrote:
> Nit: Maybe drop the '*'. This seems to refer to the actual reference in
SVGImageForContainer, but maybe it would suffice to just mention the concrete
(sub)class of Image (and it's always an SVGImage after all - by necessity.)

Done.

I also slightly rephrased this to mention that it's more than the size we care
about:
// SVGImageForContainer contains a reference to an SVGImage and includes context
// about how the image is being used (size, fragment identifier).

https://codereview.chromium.org/2322803002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/svg/graphics/SVGImageForContainer.h:44: // single
SVGImage is created per svg image resource, but this SVGImage can
On 2016/09/08 at 09:23:48, fs wrote:
> Nit: s/svg image/SVG image/ for consistency

Done

https://codereview.chromium.org/2322803002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/svg/graphics/SVGImageForContainer.h:45: // be
referenced multiple times by different container sizes. Similarly, each
On 2016/09/08 at 09:23:47, fs wrote:
> Nit: ...by containers of different sizes.
> 
> (I.e the SVGImage is referenced by the sizes, but the containers may be sized
differently.)

Done

https://codereview.chromium.org/2322803002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/svg/graphics/SVGImageForContainer.h:46: // use of
an image can contain a different fragment url (foo.svg#abc) which
On 2016/09/08 at 09:23:48, fs wrote:
> Nit: ...may have a different fragment identifier as part of its URL
(foo.svg#abc), which can influence rendering.

Done

https://codereview.chromium.org/2322803002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/svg/graphics/SVGImageForContainer.h:51: // <img
src='foo.svg#a' width='20'><img src='foo.svg#b' width='10'>
On 2016/09/08 at 09:23:48, fs wrote:
> This would be the case even without the fragment parts. Maybe make this two
examples:
> 
> <img src="foo.svg" width="20"><img src="foo.svg" width="10">
> 
> or
> 
> <img src="foo.svg#a" width="20">...

I changed this example to just list three images:
// <img src='foo.svg#a' width='20'>
// <img src='foo.svg#a' width='10'>
// <img src='foo.svg#b' width='10'>

[fs, 2016-09-08 21:10:42]

The CQ bit was checked by fs@opera.com

[fs, 2016-09-08 21:10:43]

The patchset sent to the CQ was uploaded after l-g-t-m from fs@opera.com
Link to the patchset: https://codereview.chromium.org/2322803002/#ps20001
(title: "Reduce job security by making the comment even cleaner")

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

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

[commit-bot: I haz the power, 2016-09-08 21:16:44]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2016-09-08 21:19:10]

Description was changed from

==========
Document SVGImageForContainer with a class-level comment

SVGImageForContainer is confusing and needs better documentation. This
patch adds a class-level comment in SVGImageForContainer.h that
describes the dimension and fragment url quirks that require this class.

BUG=644913
NOTRY=true
==========

to

==========
Document SVGImageForContainer with a class-level comment

SVGImageForContainer is confusing and needs better documentation. This
patch adds a class-level comment in SVGImageForContainer.h that
describes the dimension and fragment url quirks that require this class.

BUG=644913
NOTRY=true

Committed: https://crrev.com/51023c152e2acb949eedd5e5cc500622f1bfcc87
Cr-Commit-Position: refs/heads/master@{#417400}
==========

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

Patchset 2 (id:??) landed as
https://crrev.com/51023c152e2acb949eedd5e5cc500622f1bfcc87
Cr-Commit-Position: refs/heads/master@{#417400}