New issue
Advanced search Search tips

Issue 673698 link

Starred by 3 users

Issue metadata

Status: Started
Owner:
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 3
Type: Bug


Sign in to add a comment

Blink IDL compatibility: fix missing / additional optional arguments

Project Member Reported by lunalu@chromium.org, Dec 13 2016

Issue description

The following interfaces have optional arguments mismatching the spec:
CSSStyleSheet.insertRule.arg1:index
CanvasRenderingContext2D.scrollPathIntoView.arg0:path
Document.createTouch.arg1:target
Document.createTouch.arg2:identifier
Document.createTouch.arg3:pageX
Document.createTouch.arg4:pageY
Document.createTouch.arg5:screenX
Document.createTouch.arg6:screenY
Event.initEvent.arg0:type
Event.initEvent.arg1:bubbles
Event.initEvent.arg2:cancelable
MediaDevices.getUserMedia.arg0:constraints
MimeTypeArray.item.arg0:index
MimeTypeArray.namedItem.arg0:name
NodeFilter.acceptNode.arg0:node
Plugin.item.arg0:index
Plugin.namedItem.arg0:name
PluginArray.item.arg0:index
PluginArray.namedItem.arg0:name
RTCPeerConnection.getStats.arg0:selector
RTCPeerConnection.setLocalDescription.arg2:failureCallback



The following interfaces should have optional arguments:
Bluetooth.requestDevice.arg0:options
CanvasPattern.setTransform.arg0:transform
CredentialsContainer.get.arg0:options
Document.createElement.arg1:options
Document.createElementNS.arg2:options
MutationObserver.observe.arg1:options
WebGL2RenderingContext.compressedTexSubImage2D.arg8:srcOffset
WebGL2RenderingContextBase.compressedTexSubImage2D.arg8:srcOffset
Window.open.arg0:url
Window.open.arg1:target
ImageCapture.setOptions.arg0:photoSettings



List generated by web idl tool (https://github.com/mdittmer/web-apis)
 

Comment 1 by foolip@chromium.org, Dec 13 2016

Blocking: 673852

Comment 2 by foolip@chromium.org, Dec 16 2016

From the list, I think starting with Event#initEvent() makes sense, and also investigating the setSelectionRange ones. Are the optional in other engines?

Comment 3 by lunalu@chromium.org, Dec 16 2016

Working on Event#initEvent() now
Is this list still accurate? I was expecting to see many more init*Event methods here.

Comment 5 Deleted

Comment 6 Deleted

ERRR  CSSStyleSheet.insertRule
ERRR  Event.initEvent
ERRR  Element.scrollIntoView
ERRR  Document.createTouch
ERRR  NodeFilter.acceptNode
ERRR  HTMLInputElement.setSelectionRange
ERRR  HTMLCanvasElement.getContext
ERRR  CanvasRenderingContext2D.scrollPathIntoView
ERRR  MimeTypeArray.item
ERRR  MimeTypeArray.namedItem
ERRR  HTMLTextAreaElement.setSelectionRange
ERRR  MessageEvent.initMessageEvent
ERRR  OffscreenCanvas.getContext
ERRR  PluginArray.item
ERRR  PluginArray.namedItem
ERRR  Plugin.item
ERRR  Plugin.namedItem
ERRR  MediaDevices.getUserMedia
ERRR  WebGL2RenderingContextBase.compressedTexImage3D
ERRR  WebGL2RenderingContextBase.compressedTexSubImage3D
ERRR  WebGL2RenderingContext.compressedTexImage3D
ERRR  WebGL2RenderingContext.compressedTexSubImage3D
Top worth-fixing ones are: 

OffscreenCanvas.getContext
HTMLCanvasElement.getContext
MediaDevices.getUserMedia
Project Member

Comment 9 by bugdroid1@chromium.org, Feb 7 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/48d992b90f2a9a95bb591cf2a5d48baea27567df

commit 48d992b90f2a9a95bb591cf2a5d48baea27567df
Author: chasej <chasej@chromium.org>
Date: Tue Feb 07 23:06:14 2017

Make MutationObserver observe parameter optional as per spec.

The spec for MutationObserver:
https://dom.spec.whatwg.org/#interface-mutationobserver

According to the spec, the observe() method has an "options" dictionary
parameter, which should be optional. The IDL definition of observe()
was missing the optional modifier, making the parameter required.

This CL corrects the IDL so that it conforms exactly with the spec. As it
turns out, the options parameter is effectively required, as the spec
requires at least one of the values to be set. The result of this change
is only observable in different error messages (the type thrown is the
same).

BUG=673698

Review-Url: https://codereview.chromium.org/2678583002
Cr-Commit-Position: refs/heads/master@{#448769}

[modify] https://crrev.com/48d992b90f2a9a95bb591cf2a5d48baea27567df/third_party/WebKit/LayoutTests/external/wpt/dom/interfaces-expected.txt
[modify] https://crrev.com/48d992b90f2a9a95bb591cf2a5d48baea27567df/third_party/WebKit/LayoutTests/fast/dom/MutationObserver/observe-exceptions-expected.txt
[modify] https://crrev.com/48d992b90f2a9a95bb591cf2a5d48baea27567df/third_party/WebKit/Source/core/dom/MutationObserver.idl

Description: Show this description
initEvent() has now been tweaked in the spec: https://github.com/whatwg/dom/issues/387

That would be a simple and useful fix, if you'd like to do it Luna. I don't think that it's risky enough to require an intent, but we can ask for a second opinion on the eventual review.
This has now been fixed in WebKit: https://trac.webkit.org/changeset/213517
Blockedon: 319695
Blockedon: 701086
Blockedon: 701539
Blockedon: 701549
Blockedon: 701550
Blockedon: 701555
Blockedon: 701557
Blockedon: 701560
Blockedon: 701561
Blockedon: 701562
Blockedon: 701565
Blockedon: 701566
Blockedon: 701567
Blockedon: 701568
Blockedon: 701569
Blockedon: 701570
Blockedon: 701572
I filed a bug for everything that is in the list. I will start to work on them one by one.
Project Member

Comment 32 by bugdroid1@chromium.org, Mar 20 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/e682298d296d225092eb3bc9ffc98e0563365bbe

commit e682298d296d225092eb3bc9ffc98e0563365bbe
Author: dimich <dimich@chromium.org>
Date: Mon Mar 20 23:15:21 2017

Revert of Make initEvent's first argument non-optional (patchset #4 id:60001 of https://codereview.chromium.org/2579993002/ )

Reason for revert:
It looks this broke the external/wpt/dom/interfaces.html test:

https://uberchromegw.corp.google.com/i/chromium.webkit/builders/WebKit%20Mac10.12/builds/448
https://uberchromegw.corp.google.com/i/chromium.webkit/builders/WebKit%20Mac10.12/builds/438
...

Original issue's description:
> Make initEvent's first argument non-optional
>
> https://github.com/whatwg/dom/issues/387
>
> BUG=673698
>
> Review-Url: https://codereview.chromium.org/2579993002
> Cr-Commit-Position: refs/heads/master@{#458108}
> Committed: https://chromium.googlesource.com/chromium/src/+/2948f11e04fba5f0dc7244229c542152c97c1624

TBR=foolip@chromium.org,lunalu@chromium.org
# Skipping CQ checks because original CL landed less than 1 days ago.
NOPRESUBMIT=true
NOTREECHECKS=true
NOTRY=true
BUG=673698

Review-Url: https://codereview.chromium.org/2760173002
Cr-Commit-Position: refs/heads/master@{#458227}

[rename] https://crrev.com/e682298d296d225092eb3bc9ffc98e0563365bbe/third_party/WebKit/LayoutTests/external/wpt/dom/events/Event-initEvent-expected.txt
[modify] https://crrev.com/e682298d296d225092eb3bc9ffc98e0563365bbe/third_party/WebKit/LayoutTests/external/wpt/dom/interfaces-expected.txt
[delete] https://crrev.com/cf3002e06dd5a690354d8308732a4867762713d9/third_party/WebKit/LayoutTests/platform/mac-mac10.11/external/wpt/dom/interfaces-expected.txt
[delete] https://crrev.com/cf3002e06dd5a690354d8308732a4867762713d9/third_party/WebKit/LayoutTests/platform/mac-retina/external/wpt/dom/interfaces-expected.txt
[delete] https://crrev.com/cf3002e06dd5a690354d8308732a4867762713d9/third_party/WebKit/LayoutTests/platform/win/external/wpt/dom/interfaces-expected.txt
[modify] https://crrev.com/e682298d296d225092eb3bc9ffc98e0563365bbe/third_party/WebKit/Source/core/events/Event.idl

Project Member

Comment 33 by bugdroid1@chromium.org, Mar 21 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/f23d7fd62a4239f5cd2498129b4aeadac32984c3

commit f23d7fd62a4239f5cd2498129b4aeadac32984c3
Author: lunalu <lunalu@chromium.org>
Date: Tue Mar 21 16:59:12 2017

Make initEvent's first argument non-optional

https://github.com/whatwg/dom/issues/387

BUG=673698

Review-Url: https://codereview.chromium.org/2579993002
Cr-Original-Commit-Position: refs/heads/master@{#458108}
Committed: https://chromium.googlesource.com/chromium/src/+/2948f11e04fba5f0dc7244229c542152c97c1624
Review-Url: https://codereview.chromium.org/2579993002
Cr-Commit-Position: refs/heads/master@{#458443}

[modify] https://crrev.com/f23d7fd62a4239f5cd2498129b4aeadac32984c3/third_party/WebKit/LayoutTests/external/wpt/dom/interfaces-expected.txt
[add] https://crrev.com/f23d7fd62a4239f5cd2498129b4aeadac32984c3/third_party/WebKit/LayoutTests/platform/mac-mac10.11/external/wpt/dom/interfaces-expected.txt
[rename] https://crrev.com/f23d7fd62a4239f5cd2498129b4aeadac32984c3/third_party/WebKit/LayoutTests/platform/mac-retina/external/wpt/dom/events/Event-initEvent-expected.txt
[add] https://crrev.com/f23d7fd62a4239f5cd2498129b4aeadac32984c3/third_party/WebKit/LayoutTests/platform/mac-retina/external/wpt/dom/interfaces-expected.txt
[add] https://crrev.com/f23d7fd62a4239f5cd2498129b4aeadac32984c3/third_party/WebKit/LayoutTests/platform/win/external/wpt/dom/interfaces-expected.txt
[modify] https://crrev.com/f23d7fd62a4239f5cd2498129b4aeadac32984c3/third_party/WebKit/Source/core/events/Event.idl

Owner: loonyb...@chromium.org
Components: Blink>Internals

Comment 36 by tkent@chromium.org, Jan 24 2018

Blockedon: 660758

Sign in to add a comment