Use unsigned longs in HTMLImageElement idl

Use unsigned longs in HTMLImageElement idl

Use unsigned longs instead of longs in
HTMLImageElement idl like specified:
https://html.spec.whatwg.org/#the-img-element

BUG=496400
Committed: https://crrev.com/d4c998bd79ab0b96c844f7df3e3c6c8683e7da53
Cr-Commit-Position: refs/heads/master@{#431620}

[rwlbuis, 2016-11-10 16:44:53]

Description was changed from

==========
Foo


LI WIP

BUG=
==========

to

==========
Use unsigned longs in HTMLImageElement idl

Use unsigned longs instead of longs in HTMLImageElement
idl like specified:
https://html.spec.whatwg.org/#the-img-element

BUG=496400
==========

[rwlbuis, 2016-11-10 16:45:07]

Description was changed from

==========
Use unsigned longs in HTMLImageElement idl

Use unsigned longs instead of longs in HTMLImageElement
idl like specified:
https://html.spec.whatwg.org/#the-img-element

BUG=496400
==========

to

==========
Use unsigned longs in HTMLImageElement idl

Use unsigned longs instead of longs in
HTMLImageElement idl like specified:
https://html.spec.whatwg.org/#the-img-element

BUG=496400
==========

[rwlbuis, 2016-11-10 16:45:43]

rob.buis@samsung.com changed reviewers:
+ foolip@chromium.org

[rwlbuis, 2016-11-10 16:45:43]

PTAL. Hope I have the correct Bug number :)

[foolip, 2016-11-10 16:53:33]

https://codereview.chromium.org/2490233002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/html/HTMLImageElement.idl (right):

https://codereview.chromium.org/2490233002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/HTMLImageElement.idl:37: readonly attribute
unsigned long naturalWidth;
It looks these and x/y aren't actually changed in the implementation?

[rwlbuis, 2016-11-10 17:07:45]

https://codereview.chromium.org/2490233002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/html/HTMLImageElement.idl (right):

https://codereview.chromium.org/2490233002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/HTMLImageElement.idl:37: readonly attribute
unsigned long naturalWidth;
On 2016/11/10 16:53:33, foolip wrote:
> It looks these and x/y aren't actually changed in the implementation?

For naturalWidth/Height it should not matter, since we are working with
width/height which are supposed to be unsigned.

For x/y, I do not see them listed here?
https://html.spec.whatwg.org/#the-img-element

If we can set them to negative x/y then obviously I'll not convert them to
unsigned long, let me know.

[rwlbuis, 2016-11-10 17:48:04]

On 2016/11/10 17:07:45, rwlbuis wrote:
>
https://codereview.chromium.org/2490233002/diff/40001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/core/html/HTMLImageElement.idl (right):
> 
>
https://codereview.chromium.org/2490233002/diff/40001/third_party/WebKit/Sour...
> third_party/WebKit/Source/core/html/HTMLImageElement.idl:37: readonly
attribute
> unsigned long naturalWidth;
> On 2016/11/10 16:53:33, foolip wrote:
> > It looks these and x/y aren't actually changed in the implementation?
> 
> For naturalWidth/Height it should not matter, since we are working with
> width/height which are supposed to be unsigned.
> 
> For x/y, I do not see them listed here?
> https://html.spec.whatwg.org/#the-img-element
> 
> If we can set them to negative x/y then obviously I'll not convert them to
> unsigned long, let me know.

Ah, I see it can become negative (tested with FF) for instance when put in a
layer with negative margins. So I think long is correct after all. Still would
be interested if I missed it in the spec anywhere.

[foolip, 2016-11-11 09:47:22]

https://codereview.chromium.org/2490233002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/html/HTMLImageElement.idl (right):

https://codereview.chromium.org/2490233002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/HTMLImageElement.idl:37: readonly attribute
unsigned long naturalWidth;
On 2016/11/10 17:07:45, rwlbuis wrote:
> On 2016/11/10 16:53:33, foolip wrote:
> > It looks these and x/y aren't actually changed in the implementation?
> 
> For naturalWidth/Height it should not matter, since we are working with
> width/height which are supposed to be unsigned.

I mean that HTMLImageElement::naturalWidth() should be changed to unsigned, and
any callsites also need to be checked to see if they're treating it as signed or
unsigned.

> For x/y, I do not see them listed here?
> https://html.spec.whatwg.org/#the-img-element
> 
> If we can set them to negative x/y then obviously I'll not convert them to
> unsigned long, let me know.

They're in
https://drafts.csswg.org/cssom-view/#extensions-to-the-htmlimageelement-inter...
and aren't unsigned. Maybe file a spec issue if it makes sense to change them.

[rwlbuis, 2016-11-11 16:17:01]

On 2016/11/11 09:47:22, foolip wrote:
>
https://codereview.chromium.org/2490233002/diff/40001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/core/html/HTMLImageElement.idl (right):
> 
>
https://codereview.chromium.org/2490233002/diff/40001/third_party/WebKit/Sour...
> third_party/WebKit/Source/core/html/HTMLImageElement.idl:37: readonly
attribute
> unsigned long naturalWidth;
> On 2016/11/10 17:07:45, rwlbuis wrote:
> > On 2016/11/10 16:53:33, foolip wrote:
> > > It looks these and x/y aren't actually changed in the implementation?
> > 
> > For naturalWidth/Height it should not matter, since we are working with
> > width/height which are supposed to be unsigned.
> 
> I mean that HTMLImageElement::naturalWidth() should be changed to unsigned,
and
> any callsites also need to be checked to see if they're treating it as signed
or
> unsigned.

Done. The FaceDetector assumes uint32 with the DetectFace call, so I think
casting to unsigned is ok for the DCHECK_EQ.

> > For x/y, I do not see them listed here?
> > https://html.spec.whatwg.org/#the-img-element
> > 
> > If we can set them to negative x/y then obviously I'll not convert them to
> > unsigned long, let me know.
> 
> They're in
>
https://drafts.csswg.org/cssom-view/#extensions-to-the-htmlimageelement-inter...
> and aren't unsigned. Maybe file a spec issue if it makes sense to change them.

Actually I do think they can become negative, so I want to leave it as is:
>Ah, I see it can become negative (tested with FF) for instance when put in a
>layer with negative margins. So I think long is correct after all. Still would
>be interested if I missed it in the spec anywhere.

In WebKit it is signed too.

PTAL.

[foolip, 2016-11-11 16:25:31]

lgtm

[rwlbuis, 2016-11-11 16:27:20]

rob.buis@samsung.com changed reviewers:
+ haraken@chromium.org

[rwlbuis, 2016-11-11 16:27:21]

Adding haraken for modules review.

[haraken, 2016-11-11 17:06:13]

On 2016/11/11 16:27:21, rwlbuis wrote:
> Adding haraken for modules review.

LGTM

[rwlbuis, 2016-11-11 18:10:29]

The CQ bit was checked by rob.buis@samsung.com

[commit-bot: I haz the power, 2016-11-11 18:10:50]

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-11-11 19:47:20]

Description was changed from

==========
Use unsigned longs in HTMLImageElement idl

Use unsigned longs instead of longs in
HTMLImageElement idl like specified:
https://html.spec.whatwg.org/#the-img-element

BUG=496400
==========

to

==========
Use unsigned longs in HTMLImageElement idl

Use unsigned longs instead of longs in
HTMLImageElement idl like specified:
https://html.spec.whatwg.org/#the-img-element

BUG=496400
==========

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

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2016-11-11 20:00:10]

Description was changed from

==========
Use unsigned longs in HTMLImageElement idl

Use unsigned longs instead of longs in
HTMLImageElement idl like specified:
https://html.spec.whatwg.org/#the-img-element

BUG=496400
==========

to

==========
Use unsigned longs in HTMLImageElement idl

Use unsigned longs instead of longs in
HTMLImageElement idl like specified:
https://html.spec.whatwg.org/#the-img-element

BUG=496400

Committed: https://crrev.com/d4c998bd79ab0b96c844f7df3e3c6c8683e7da53
Cr-Commit-Position: refs/heads/master@{#431620}
==========

[commit-bot: I haz the power, 2016-11-11 20:00:11]

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