Regex special-casing removed from WebIDL
Reported by
tobie.la...@gmail.com,
Nov 4 2016
|
||||||||
Issue descriptionUserAgent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_10_5) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/54.0.2840.71 Safari/537.36 Steps to reproduce the problem: Heads-up that we've stopped special casing Regexp in the WebIDL spec as of https://github.com/heycam/webidl/commit/bbb2bde Here's a diff of what that looks like in the rendered HTML spec: [diff]: https://services.w3.org/htmldiff?doc1=https%3A%2F%2Fcdn.rawgit.com%2Fheycam%2Fwebidl%2F001ba52%2Findex.html&doc2=https%3A%2F%2Fcdn.rawgit.com%2Ftobie%2Fwebidl%2F00b9786%2Findex.html That means that the following algorithms now allow regexes in places where they previously didn't: 1. to convert an ES value to a dictionary: https://heycam.github.io/webidl/#es-dictionary 2. to convert an ES value to a sequence: https://heycam.github.io/webidl/#es-sequence 3. to convert an ES value to a union type (step 5 and 11): https://heycam.github.io/webidl/#es-union 4. the substeps of step 11 of the overload resolution algorithm: https://heycam.github.io/webidl/#es-overloads There are also some modifications to the section on User objects implementing callback interfaces: https://heycam.github.io/webidl/#es-user-objects I opened an issue against web-platform-tests's testharness to account for these changes: https://github.com/w3c/testharness.js/issues/223 Please raise any concerns by opening an issue against the spec: https://github.com/heycam/webidl/issues/new Thanks. What is the expected behavior? What went wrong? Nothing ;) Did this work before? N/A Does this work in other browsers? No Mozillan: https://bugzilla.mozilla.org/show_bug.cgi?id=1315221 WebKit: https://bugs.webkit.org/show_bug.cgi?id=164410 Chrome version: 54.0.2840.71 Channel: n/a OS Version: OS X 10.10.5 Flash Version: Shockwave Flash 23.0 r0
,
Nov 7 2016
,
Nov 7 2016
Adding TE-NeedsTriageHelp label for further investigation. Thanks..!!
,
Nov 15 2016
No further triage needed - on the binding team's backlog.
,
Nov 15 2017
This issue has been Available for over a year. If it's no longer important or seems unlikely to be fixed, please consider closing it out. If it is important, please re-triage the issue. Sorry for the inconvenience if the bug really should have been left as Available. If you change it back, also remove the "Hotlist-Recharge-Cold" label. For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
,
Nov 16 2017
At a glance, I found only one leftover: https://cs.chromium.org/chromium/src/third_party/WebKit/Source/bindings/core/v8/V8BindingForCore.h?q=RegExp+file:bindings/&sq=package:chromium&l=536&dr=C Maybe I overlooked a few more cases.
,
Nov 16 2017
Let me work on web platform tests for this.
,
Nov 16 2017
Curious. I wasn't able to trigger that exception in https://github.com/w3c/web-platform-tests/pull/8282. Any help figuring that out would be appreciated. (Blink currently passes all tests in window and only fails the worker ones because ErrorEvent is not correctly exposed in workers.)
,
Nov 17 2017
Interesting. MessageEventInit is an IDL dictionary, then Blink is using NativeValueTraits<IDLSequence<MessagePort>>::NativeValue() to convert the sequence, and doesn't use ToImplSequence() that was mentioned at #6. https://cs.chromium.org/chromium/src/third_party/WebKit/Source/bindings/core/v8/NativeValueTraitsImpl.h?rcl=1016b760146885abb36fca61ea36d45c630d8b61&l=416 Seeing the code search's cross reference results, there seems no caller of ToImplSequence(). Maybe there is no longer special handling of RegExp in Blink. +cc: rakuco, I don't remember exactly, but were we planning to replace ToImplSequence() with NativeValueTraits? Do you remember something?
,
Nov 20 2017
FYI, rakuco is off until Dec 10th. domenic@, may I remove ToImplSequence()? Or do you like to wait for a while? I'm fine with either way or other options.
,
Nov 20 2017
Please feel free to remove it! No need to wait.
,
Nov 21 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/ca0eec0a0d9f1f896b0c77bac3d1312bf1f0c190 commit ca0eec0a0d9f1f896b0c77bac3d1312bf1f0c190 Author: Yuki Shiino <yukishiino@chromium.org> Date: Tue Nov 21 09:46:23 2017 v8binding: Removes dead code: ToImplSequence. ToImplSequence is no longer used. Removes it. Bug: 662379 Change-Id: I5d02be18d5dd556eab7a374267d76ce8c1d40e71 Reviewed-on: https://chromium-review.googlesource.com/781481 Reviewed-by: Hitoshi Yoshida <peria@chromium.org> Reviewed-by: Kentaro Hara <haraken@chromium.org> Commit-Queue: Yuki Shiino <yukishiino@chromium.org> Cr-Commit-Position: refs/heads/master@{#518197} [modify] https://crrev.com/ca0eec0a0d9f1f896b0c77bac3d1312bf1f0c190/third_party/WebKit/Source/bindings/core/v8/V8BindingForCore.h
,
Nov 21 2017
I see no more special-casing for RegExp in Blink. I close this issue.
,
Dec 19 2017
> I don't remember exactly, but were we planning to replace ToImplSequence() with NativeValueTraits? Do you remember something? I _think_ I just forgot to remove it in https://codereview.chromium.org/2840563002 ("bindings: Remove unused To*Array() and ToV8Sequence() functions"), as I even removed the ToImplSequence() unit tests in that patch. For the record, the code was almost entirely duplicated when I added NativeValueTraits<IDLSequence<T>>, and currently lives here: https://cs.chromium.org/chromium/src/third_party/WebKit/Source/bindings/core/v8/NativeValueTraitsImpl.h?rcl=1016b760146885abb36fca61ea36d45c630d8b61&l=345 So thanks for getting rid of that one :-) |
||||||||
►
Sign in to add a comment |
||||||||
Comment 1 by bashi@chromium.org
, Nov 7 2016Labels: -Pri-2 Pri-3
Status: Available (was: Unconfirmed)