window.scroll() without params should be ignored or throw an error

window.scroll() without params should be ignored or throw an error

The parameters are marked non-optional to make sure that a x,y parameter is specified to scroll. Modified the test case to consider this scenario. This is compatible with Firefox. IE reports no error and scrolls to page top, like chrome before the patch.

BUG=113962
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=161798

[Srini, 2013-11-05 15:34:55]

Hi,

Can you review this patch. Thanks!

-Srini.

[Inactive, 2013-11-05 15:47:58]

On 2013/11/05 15:34:55, Srini wrote:
> Hi,
> 
> Can you review this patch. Thanks!
> 
> -Srini.

Is this change consistent with the specification and other major browsers?
Please update the CL description accordingly.
We start throwing exceptions which is risky for backward compatibility so we
need to make sure other browsers behave the same way.

Also, since this a web-exposed change, an API owner will have to approve.

[Srini, 2013-11-05 15:54:47]

On 2013/11/05 15:47:58, Chris Dumez wrote:
> On 2013/11/05 15:34:55, Srini wrote:
> > Hi,
> > 
> > Can you review this patch. Thanks!
> > 
> > -Srini.
> 
> Is this change consistent with the specification and other major browsers?
> Please update the CL description accordingly.
I checked with Firefox it is consistent. I'll find a windows machine to check
with IE.

> We start throwing exceptions which is risky for backward compatibility so we
> need to make sure other browsers behave the same way.

Sure. I get it.
> 
> Also, since this a web-exposed change, an API owner will have to approve.

Can you add anyone who can review? I don't know whom to add/where to find the
owner. Thanks.

[pdr., 2013-11-05 18:43:44]

It looks like there's no spec for these methods so all we have is browser
compatibility. As chdumez said, please check IE and include your FF and IE
findings in the change description.

Abarth or eseidel would be good API owners for this review.

[Srini, 2013-11-05 18:59:15]

On 2013/11/05 18:43:44, pdr wrote:
> It looks like there's no spec for these methods so all we have is browser
> compatibility. As chdumez said, please check IE and include your FF and IE
> findings in the change description.
> 
> Abarth or eseidel would be good API owners for this review.

Done. Thanks for the review/help.

[pdr., 2013-11-05 19:05:17]

On 2013/11/05 18:59:15, Srini wrote:
> On 2013/11/05 18:43:44, pdr wrote:
> > It looks like there's no spec for these methods so all we have is browser
> > compatibility. As chdumez said, please check IE and include your FF and IE
> > findings in the change description.
> > 
> > Abarth or eseidel would be good API owners for this review.
> 
> Done. Thanks for the review/help.

Thanks for updating with the IE information. This feels like changing for the
sake of changing and will likely break a couple of users but without a benefit
to web compatibility. I'm not sure we should make this change.

[eseidel, 2013-11-05 19:14:21]

So this is changing us to match FF instead of IE, correct?

Do we believe that more content is written which matches FF?  Is this behavior
spec'd anywhere?

[Srini, 2013-11-05 19:24:59]

On 2013/11/05 19:14:21, eseidel wrote:
> So this is changing us to match FF instead of IE, correct?
> 
> Do we believe that more content is written which matches FF?  Is this behavior
> spec'd anywhere?

I don't think it is spec'd anywhere. I started fixing the bug crbug.com/113962
and later saw that it is complaint with IE and this patch makes it complaint
with Firefox. Honestly I don't know which is good or which one to match. If
there is someway I could try to add more information to support either point, I
would be willing to go for that. Thanks.

[eseidel, 2013-11-05 19:27:07]

Wow.  HTML5 window object is pretty empty compared to actual Window objects:
http://www.w3.org/TR/html5/browsers.html#window

[Inactive, 2013-11-05 19:34:00]

https://codereview.chromium.org/59863003/diff/1/Source/core/frame/Window.idl
File Source/core/frame/Window.idl (right):

https://codereview.chromium.org/59863003/diff/1/Source/core/frame/Window.idl#...
Source/core/frame/Window.idl:96: void scrollBy([Default=Undefined] long x,
[Default=Undefined] long y);
It does not make sense to keep [Default=Undefined] if the argument is no longer
optional.

[Inactive, 2013-11-05 19:42:11]

On 2013/11/05 19:14:21, eseidel wrote:
> So this is changing us to match FF instead of IE, correct?

Correct. I have just checked. IE10 / Blink / WebKit have those arguments
optional.
Only Firefox has them mandatory.

> 
> Do we believe that more content is written which matches FF?  Is this behavior
> spec'd anywhere?

It does not appear to be part of any standard (confirmed by
https://developer.mozilla.org/en-US/docs/Web/API/Window.scroll).

It looks to me that not throwing (i.e. keeping the arguments optional) is the
safest (compatibility-wise) given the current context.

[Srini, 2013-11-06 03:51:41]

On 2013/11/05 19:42:11, Chris Dumez wrote:
> On 2013/11/05 19:14:21, eseidel wrote:
> > So this is changing us to match FF instead of IE, correct?
> 
> Correct. I have just checked. IE10 / Blink / WebKit have those arguments
> optional.
> Only Firefox has them mandatory.
> 
> > 
> > Do we believe that more content is written which matches FF?  Is this
behavior
> > spec'd anywhere?
> 
> It does not appear to be part of any standard (confirmed by
> https://developer.mozilla.org/en-US/docs/Web/API/Window.scroll).
> 
> It looks to me that not throwing (i.e. keeping the arguments optional) is the
> safest (compatibility-wise) given the current context.

Ok. I'll leave the patch at this stage and we can revisit if a need arises.
Thanks for the help.

[pdr., 2013-11-06 04:10:57]

On 2013/11/06 03:51:41, Srini wrote:
> On 2013/11/05 19:42:11, Chris Dumez wrote:
> > On 2013/11/05 19:14:21, eseidel wrote:
> > > So this is changing us to match FF instead of IE, correct?
> > 
> > Correct. I have just checked. IE10 / Blink / WebKit have those arguments
> > optional.
> > Only Firefox has them mandatory.
> > 
> > > 
> > > Do we believe that more content is written which matches FF?  Is this
> behavior
> > > spec'd anywhere?
> > 
> > It does not appear to be part of any standard (confirmed by
> > https://developer.mozilla.org/en-US/docs/Web/API/Window.scroll).
> > 
> > It looks to me that not throwing (i.e. keeping the arguments optional) is
the
> > safest (compatibility-wise) given the current context.
> 
> Ok. I'll leave the patch at this stage and we can revisit if a need arises.
> Thanks for the help.

It would be nice to align on this. I went ahead and filed a bug against Gecko
with the findings from this patch:
https://bugzilla.mozilla.org/show_bug.cgi?id=935365

[Srini, 2013-11-06 05:49:05]

On 2013/11/06 04:10:57, pdr wrote:
> On 2013/11/06 03:51:41, Srini wrote:
> > On 2013/11/05 19:42:11, Chris Dumez wrote:
> > > On 2013/11/05 19:14:21, eseidel wrote:
> > > > So this is changing us to match FF instead of IE, correct?
> > > 
> > > Correct. I have just checked. IE10 / Blink / WebKit have those arguments
> > > optional.
> > > Only Firefox has them mandatory.
> > > 
> > > > 
> > > > Do we believe that more content is written which matches FF?  Is this
> > behavior
> > > > spec'd anywhere?
> > > 
> > > It does not appear to be part of any standard (confirmed by
> > > https://developer.mozilla.org/en-US/docs/Web/API/Window.scroll).
> > > 
> > > It looks to me that not throwing (i.e. keeping the arguments optional) is
> the
> > > safest (compatibility-wise) given the current context.
> > 
> > Ok. I'll leave the patch at this stage and we can revisit if a need arises.
> > Thanks for the help.
> 
> It would be nice to align on this. I went ahead and filed a bug against Gecko
> with the findings from this patch:
> https://bugzilla.mozilla.org/show_bug.cgi?id=935365

Thx pdr. I just saw the gecko bug. Seems like it is spec'ed at 
http://dev.w3.org/csswg/cssom-view/#extensions-to-the-window-interface

It says that these parameters aren't optional and FF is complaint to this.
Chrome/IE/Safari 
aren't. It appears that either the spec & FF should be changed for compliance or
rest of the 
browsers should change.

[eseidel, 2013-11-06 07:07:46]

It appears to be Working Draft status:
http://www.w3.org/TR/cssom-view/#dom-window-scroll

[arv (Not doing code reviews), 2013-11-06 15:26:04]

On 2013/11/06 07:07:46, eseidel wrote:
> It appears to be Working Draft status:
> http://www.w3.org/TR/cssom-view/#dom-window-scroll

It makes little sense to have these as optional parameters. It seems safe enough
to make these required if Firefox already throws on missing params.

[Srini, 2013-11-07 11:04:08]

On 2013/11/06 15:26:04, arv wrote:
> On 2013/11/06 07:07:46, eseidel wrote:
> > It appears to be Working Draft status:
> > http://www.w3.org/TR/cssom-view/#dom-window-scroll
> 
> It makes little sense to have these as optional parameters. It seems safe
enough
> to make these required if Firefox already throws on missing params.

pdr, eseidel, chris, arv: Any conclusion on what should be done? In anycase I
updated the patch to incorporate Chris's review comments.

[Inactive, 2013-11-07 14:15:10]

On 2013/11/07 11:04:08, Srini wrote:
> On 2013/11/06 15:26:04, arv wrote:
> > On 2013/11/06 07:07:46, eseidel wrote:
> > > It appears to be Working Draft status:
> > > http://www.w3.org/TR/cssom-view/#dom-window-scroll
> > 
> > It makes little sense to have these as optional parameters. It seems safe
> enough
> > to make these required if Firefox already throws on missing params.
> 
> pdr, eseidel, chris, arv: Any conclusion on what should be done? In anycase I
> updated the patch to incorporate Chris's review comments.

I trust arv :) if he says it makes sense then fine by me. However, an API owner
will need to approve since this is a webexposed change.

[Inactive, 2013-11-07 14:22:02]

Technically the change is correct and is according to specification and Firefox
so LGTM.

*However, please wait for API Owner approval as well before landing.*

[eseidel, 2013-11-07 23:34:12]

lgtm

I'm OK with this if arv is.

[commit-bot: I haz the power, 2013-11-12 06:17:28]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/srinivasa.ragavan.venkateswaran@intel....

[commit-bot: I haz the power, 2013-11-12 07:31:49]

Retried try job too often on linux_blink_rel for step(s) webkit_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_blin...

[commit-bot: I haz the power, 2013-11-12 09:02:54]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/srinivasa.ragavan.venkateswaran@intel....

[commit-bot: I haz the power, 2013-11-12 11:19:51]

Change committed as 161798