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

Issue 978223002: Add [TypeChecking=Interface] to Node interface (Closed)

Created:
5 years, 9 months ago by Jens Widell
Modified:
5 years, 9 months ago
Reviewers:
pdr., philipj_slow
CC:
blink-reviews, arv+blink, vivekg, sof, eae+blinkwatch, blink-reviews-dom_chromium.org, dglazkov+blink, vivekg_samsung, Inactive, rwlbuis
Base URL:
https://chromium.googlesource.com/chromium/blink.git@master
Target Ref:
refs/heads/master
Project:
blink
Visibility:
Public.

Description

Add [TypeChecking=Interface] to Node interface This changes the behavior of the methods compareDocumentPosition(Node) contains(Node?) isEqualNode(Node?) isSameNode(Node?) that now throw TypeError exceptions instead of returning false when called with invalid arguments. The argument to isSameNode() is also made required instead of optional, meaning it will also throw TypeError when called without arguments. The new behavior matches Firefox, except in the case of isSameNode() which Firefox doesn't support. (The method has been removed from the spec.) The new behavior of compareDocumentPosition() and isEqualNode() also matches IE11. BUG=462561 Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=191440

Patch Set 1 #

Patch Set 2 : adjust isSameNode #

Patch Set 3 : update test wrt isSameNode() change #

Unified diffs Side-by-side diffs Delta from patch set Stats (+12 lines, -15 lines) Patch
M LayoutTests/fast/dom/Node/contains-method-expected.txt View 1 chunk +1 line, -1 line 0 comments Download
M LayoutTests/fast/dom/Node/script-tests/contains-method.js View 1 chunk +1 line, -1 line 0 comments Download
M LayoutTests/fast/dom/incompatible-operations.html View 1 2 1 chunk +2 lines, -2 lines 0 comments Download
M LayoutTests/fast/dom/incompatible-operations-expected.txt View 1 2 1 chunk +2 lines, -2 lines 0 comments Download
M Source/core/dom/Node.cpp View 1 chunk +0 lines, -4 lines 0 comments Download
M Source/core/dom/Node.idl View 1 3 chunks +6 lines, -5 lines 0 comments Download

Messages

Total messages: 22 (6 generated)
Jens Widell
PTAL
5 years, 9 months ago (2015-03-05 12:36:38 UTC) #2
philipj_slow
The contains change looks like the biggest risk, but since it's nullable it would have ...
5 years, 9 months ago (2015-03-05 14:54:52 UTC) #3
philipj_slow
Philip, second opinion on the risk? I think it looks safe enough to bypass all ...
5 years, 9 months ago (2015-03-05 14:56:13 UTC) #5
Jens Widell
On 2015/03/05 14:54:52, philipj_UTC7 wrote: > The contains change looks like the biggest risk, but ...
5 years, 9 months ago (2015-03-05 14:59:28 UTC) #6
philipj_slow
On 2015/03/05 14:59:28, Jens Widell wrote: > On 2015/03/05 14:54:52, philipj_UTC7 wrote: > > The ...
5 years, 9 months ago (2015-03-05 15:02:57 UTC) #7
Jens Widell
On 2015/03/05 15:02:57, philipj_UTC7 wrote: > On 2015/03/05 14:59:28, Jens Widell wrote: > > On ...
5 years, 9 months ago (2015-03-05 15:05:15 UTC) #8
Jens Widell
On 2015/03/05 15:02:57, philipj_UTC7 wrote: > On 2015/03/05 14:59:28, Jens Widell wrote: > > On ...
5 years, 9 months ago (2015-03-05 15:37:52 UTC) #9
philipj_slow
Can you update the description wrt isSameNode as well? This all LGTM, but out of ...
5 years, 9 months ago (2015-03-05 16:25:44 UTC) #10
pdr.
On 2015/03/05 at 16:25:44, philipj wrote: > Can you update the description wrt isSameNode as ...
5 years, 9 months ago (2015-03-05 21:24:39 UTC) #11
Jens Widell
On 2015/03/05 16:25:44, philipj_UTC7 wrote: > Can you update the description wrt isSameNode as well? ...
5 years, 9 months ago (2015-03-06 07:43:24 UTC) #12
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/978223002/20001
5 years, 9 months ago (2015-03-06 07:43:39 UTC) #14
philipj_slow
In light of the test regression from the Range change, have you checked if any ...
5 years, 9 months ago (2015-03-06 08:22:36 UTC) #15
Jens Widell
On 2015/03/06 08:22:36, philipj_UTC7 wrote: > In light of the test regression from the Range ...
5 years, 9 months ago (2015-03-06 08:34:33 UTC) #16
philipj_slow
On 2015/03/06 08:34:33, Jens Widell wrote: > On 2015/03/06 08:22:36, philipj_UTC7 wrote: > > In ...
5 years, 9 months ago (2015-03-06 11:07:12 UTC) #17
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/978223002/40001
5 years, 9 months ago (2015-03-06 13:24:58 UTC) #21
commit-bot: I haz the power
5 years, 9 months ago (2015-03-06 15:14:02 UTC) #22
Message was sent while issue was closed.
Committed patchset #3 (id:40001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=191440

Powered by Google App Engine
This is Rietveld 408576698