Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(59)

Issue 59863003: window.scroll() without params should be ignored or throw an error (Closed)

Created:
7 years, 1 month ago by Srini
Modified:
7 years, 1 month ago
CC:
blink-reviews
Base URL:
https://chromium.googlesource.com/chromium/blink.git@master
Visibility:
Public.

Description

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

Patch Set 1 #

Total comments: 1

Patch Set 2 : Remove default values. #

Patch Set 3 : Rebase test & test results of non-numeric-values-numeric-parameters test. #

Unified diffs Side-by-side diffs Delta from patch set Stats (+23 lines, -41 lines) Patch
M LayoutTests/fast/dom/Window/window-scroll-arguments.html View 4 chunks +7 lines, -19 lines 0 comments Download
M LayoutTests/fast/dom/Window/window-scroll-arguments-expected.txt View 4 chunks +7 lines, -13 lines 0 comments Download
M LayoutTests/fast/dom/non-numeric-values-numeric-parameters-expected.txt View 1 2 1 chunk +3 lines, -3 lines 0 comments Download
M LayoutTests/fast/dom/script-tests/non-numeric-values-numeric-parameters.js View 1 2 1 chunk +3 lines, -3 lines 0 comments Download
M Source/core/frame/Window.idl View 1 1 chunk +3 lines, -3 lines 0 comments Download

Messages

Total messages: 26 (0 generated)
Srini
Hi, Can you review this patch. Thanks! -Srini.
7 years, 1 month ago (2013-11-05 15:34:55 UTC) #1
Inactive
On 2013/11/05 15:34:55, Srini wrote: > Hi, > > Can you review this patch. Thanks! ...
7 years, 1 month ago (2013-11-05 15:47:58 UTC) #2
Srini
7 years, 1 month ago (2013-11-05 15:53:20 UTC) #3
Srini
On 2013/11/05 15:47:58, Chris Dumez wrote: > On 2013/11/05 15:34:55, Srini wrote: > > Hi, ...
7 years, 1 month ago (2013-11-05 15:54:47 UTC) #4
pdr.
It looks like there's no spec for these methods so all we have is browser ...
7 years, 1 month ago (2013-11-05 18:43:44 UTC) #5
Srini
7 years, 1 month ago (2013-11-05 18:58:24 UTC) #6
Srini
On 2013/11/05 18:43:44, pdr wrote: > It looks like there's no spec for these methods ...
7 years, 1 month ago (2013-11-05 18:59:15 UTC) #7
pdr.
On 2013/11/05 18:59:15, Srini wrote: > On 2013/11/05 18:43:44, pdr wrote: > > It looks ...
7 years, 1 month ago (2013-11-05 19:05:17 UTC) #8
eseidel
So this is changing us to match FF instead of IE, correct? Do we believe ...
7 years, 1 month ago (2013-11-05 19:14:21 UTC) #9
Srini
On 2013/11/05 19:14:21, eseidel wrote: > So this is changing us to match FF instead ...
7 years, 1 month ago (2013-11-05 19:24:59 UTC) #10
eseidel
Wow. HTML5 window object is pretty empty compared to actual Window objects: http://www.w3.org/TR/html5/browsers.html#window
7 years, 1 month ago (2013-11-05 19:27:07 UTC) #11
Inactive
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#newcode96 Source/core/frame/Window.idl:96: void scrollBy([Default=Undefined] long x, [Default=Undefined] long y); It does ...
7 years, 1 month ago (2013-11-05 19:34:00 UTC) #12
Inactive
On 2013/11/05 19:14:21, eseidel wrote: > So this is changing us to match FF instead ...
7 years, 1 month ago (2013-11-05 19:42:11 UTC) #13
Srini
On 2013/11/05 19:42:11, Chris Dumez wrote: > On 2013/11/05 19:14:21, eseidel wrote: > > So ...
7 years, 1 month ago (2013-11-06 03:51:41 UTC) #14
pdr.
On 2013/11/06 03:51:41, Srini wrote: > On 2013/11/05 19:42:11, Chris Dumez wrote: > > On ...
7 years, 1 month ago (2013-11-06 04:10:57 UTC) #15
Srini
On 2013/11/06 04:10:57, pdr wrote: > On 2013/11/06 03:51:41, Srini wrote: > > On 2013/11/05 ...
7 years, 1 month ago (2013-11-06 05:49:05 UTC) #16
eseidel
It appears to be Working Draft status: http://www.w3.org/TR/cssom-view/#dom-window-scroll
7 years, 1 month ago (2013-11-06 07:07:46 UTC) #17
arv (Not doing code reviews)
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 ...
7 years, 1 month ago (2013-11-06 15:26:04 UTC) #18
Srini
On 2013/11/06 15:26:04, arv wrote: > On 2013/11/06 07:07:46, eseidel wrote: > > It appears ...
7 years, 1 month ago (2013-11-07 11:04:08 UTC) #19
Inactive
On 2013/11/07 11:04:08, Srini wrote: > On 2013/11/06 15:26:04, arv wrote: > > On 2013/11/06 ...
7 years, 1 month ago (2013-11-07 14:15:10 UTC) #20
Inactive
Technically the change is correct and is according to specification and Firefox so LGTM. *However, ...
7 years, 1 month ago (2013-11-07 14:22:02 UTC) #21
eseidel
lgtm I'm OK with this if arv is.
7 years, 1 month ago (2013-11-07 23:34:12 UTC) #22
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/srinivasa.ragavan.venkateswaran@intel.com/59863003/200001
7 years, 1 month ago (2013-11-12 06:17:28 UTC) #23
commit-bot: I haz the power
Retried try job too often on linux_blink_rel for step(s) webkit_tests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_blink_rel&number=12161
7 years, 1 month ago (2013-11-12 07:31:49 UTC) #24
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/srinivasa.ragavan.venkateswaran@intel.com/59863003/390001
7 years, 1 month ago (2013-11-12 09:02:54 UTC) #25
commit-bot: I haz the power
7 years, 1 month ago (2013-11-12 11:19:51 UTC) #26
Message was sent while issue was closed.
Change committed as 161798

Powered by Google App Engine
This is Rietveld 408576698