Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2752103003:
Make item and namedItem args in MimeTypeArray/PluginArray/Plugin non-optional (Closed)
DescriptionMake item and namedItem args in MimeTypeArray/PluginArray/Plugin non-optional
Intent to Ship:
https://groups.google.com/a/chromium.org/d/msg/blink-dev/mGKzBiGe_Co/hMzVBa9vBwAJ
BUG=701560, 701561
Review-Url: https://codereview.chromium.org/2752103003
Cr-Commit-Position: refs/heads/master@{#461726}
Committed: https://chromium.googlesource.com/chromium/src/+/a758d4d5c23e748d3db4d07e5f9ee310238419c3
Patch Set 1 #
Total comments: 2
Patch Set 2 : Codereview update #
Total comments: 9
Patch Set 3 : Codereview: fixed nullability and removed commented code #Patch Set 4 : Codereview update #
Messages
Total messages: 39 (27 generated)
The CQ bit was checked by lunalu@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
lunalu@chromium.org changed reviewers: + foolip@chromium.org
PTAL.
These are good changes that I think we should make, but with a non-zero compat risk. What do you think about sending an joint Intent to Implement and Ship for a whole bunch of optional argument changes? It looks like there's no test coverage for this, even though LayoutTests/external/wpt/html/dom/resources/interfaces.idl seems to enough information to assert that PluginArray.prototype.item.length===1. https://codereview.chromium.org/2752103003/diff/1/third_party/WebKit/Source/m... File third_party/WebKit/Source/modules/plugins/MimeTypeArray.idl (right): https://codereview.chromium.org/2752103003/diff/1/third_party/WebKit/Source/m... third_party/WebKit/Source/modules/plugins/MimeTypeArray.idl:27: [NotEnumerable, ImplementedAs=namedItem] getter MimeType ([Default=Undefined] optional DOMString name); Can you try folding this into the above setter as per spec and see if that results in any interesting changes to the generated code? I suspect that it doesn't, or that the changed code is unreachable, because there's no syntax for invoking an anonymous getter without the argument.
The CQ bit was checked by lunalu@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Done. PTAL What kind of layout tests should we be providing though? I think just the length of the getters should be 1 is sufficient enough. https://codereview.chromium.org/2752103003/diff/1/third_party/WebKit/Source/m... File third_party/WebKit/Source/modules/plugins/MimeTypeArray.idl (right): https://codereview.chromium.org/2752103003/diff/1/third_party/WebKit/Source/m... third_party/WebKit/Source/modules/plugins/MimeTypeArray.idl:27: [NotEnumerable, ImplementedAs=namedItem] getter MimeType ([Default=Undefined] optional DOMString name); On 2017/03/17 00:31:46, foolip_UTC9_slow wrote: > Can you try folding this into the above setter as per spec and see if that > results in any interesting changes to the generated code? I suspect that it > doesn't, or that the changed code is unreachable, because there's no syntax for > invoking an anonymous getter without the argument. My bad. I updated it to match the spec.
Description was changed from ========== Make item and namedItem args in MimeTypeArray/PluginArray/Plugin non-optional BUG=701560, 701561 ========== to ========== Make item and namedItem args in MimeTypeArray/PluginArray/Plugin non-optional Intent to Ship: https://groups.google.com/a/chromium.org/forum/?hl=en#!searchin/blink-dev/lun... BUG=701560, 701561 ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2752103003/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/plugins/MimeTypeArray.idl (right): https://codereview.chromium.org/2752103003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/plugins/MimeTypeArray.idl:27: // TODO(lunalu): the return type should be nullable. You can just make that change, it won't affect the generated code, or at least it wouldn't have last time I tried a change like this. https://codereview.chromium.org/2752103003/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/plugins/Plugin.idl (right): https://codereview.chromium.org/2752103003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/plugins/Plugin.idl:31: [NotEnumerable] getter MimeType? namedItem(DOMString name); Adding [NotEnumerable] here should change the enumerability of the namedItem method, which we don't want. I don't know why [NotEnumerable] was used originally below, but just omitting should be fine, because by definition an anonymous getter doesn't have a name to enumerate. https://codereview.chromium.org/2752103003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/plugins/Plugin.idl:32: // [NotEnumerable, ImplementedAs=namedItem] getter MimeType ([Default=Undefined] optional DOMString name); Remove this line rather than commenting it out. https://codereview.chromium.org/2752103003/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/plugins/PluginArray.idl (right): https://codereview.chromium.org/2752103003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/plugins/PluginArray.idl:28: [NotEnumerable] getter Plugin? namedItem(DOMString name); Same issues here, can match spec almost verbatim I think.
Please see the comments below. https://codereview.chromium.org/2752103003/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/plugins/Plugin.idl (right): https://codereview.chromium.org/2752103003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/plugins/Plugin.idl:31: [NotEnumerable] getter MimeType? namedItem(DOMString name); On 2017/03/21 08:57:52, foolip_UTC9 wrote: > Adding [NotEnumerable] here should change the enumerability of the namedItem > method, which we don't want. I don't know why [NotEnumerable] was used > originally below, but just omitting should be fine, because by definition an > anonymous getter doesn't have a name to enumerate. I tried to do it without NotEnumerable, but the generated code was changed and also required additional implementation of namedPRopertyQuery and namedPropertyEnumerator. LegacyUnenumerableNamedProperties suggests that named properties shouldn't be enumerable: https://heycam.github.io/webidl/#LegacyUnenumerableNamedProperties
The CQ bit was checked by lunalu@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2752103003/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/plugins/Plugin.idl (right): https://codereview.chromium.org/2752103003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/plugins/Plugin.idl:31: [NotEnumerable] getter MimeType? namedItem(DOMString name); On 2017/03/21 17:56:50, loonybear wrote: > On 2017/03/21 08:57:52, foolip_UTC9 wrote: > > Adding [NotEnumerable] here should change the enumerability of the namedItem > > method, which we don't want. I don't know why [NotEnumerable] was used > > originally below, but just omitting should be fine, because by definition an > > anonymous getter doesn't have a name to enumerate. > > I tried to do it without NotEnumerable, but the generated code was changed and > also > required additional implementation of namedPRopertyQuery and > namedPropertyEnumerator. > LegacyUnenumerableNamedProperties suggests that named properties shouldn't be > enumerable: > https://heycam.github.io/webidl/#LegacyUnenumerableNamedProperties > Ah, I missed the [LegacyUnenumerableNamedProperties] on the interface level. https://software.hixie.ch/utilities/js/live-dom-viewer/?saved=4962 shows that the namedItem name is enumerable, so adding [NotEnumerable] here would change that. I guess the only choice is to remove [NotEnumerable] here and to keep the second line for now, exception the argument doesn't need to be optional. I filed https://bugs.chromium.org/p/chromium/issues/detail?id=703990, can you add a TODO to use it here?
The CQ bit was checked by lunalu@chromium.org to run a CQ dry run
Please take another look https://codereview.chromium.org/2752103003/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/plugins/Plugin.idl (right): https://codereview.chromium.org/2752103003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/plugins/Plugin.idl:31: [NotEnumerable] getter MimeType? namedItem(DOMString name); On 2017/03/22 05:04:52, foolip_UTC9 wrote: > On 2017/03/21 17:56:50, loonybear wrote: > > On 2017/03/21 08:57:52, foolip_UTC9 wrote: > > > Adding [NotEnumerable] here should change the enumerability of the namedItem > > > method, which we don't want. I don't know why [NotEnumerable] was used > > > originally below, but just omitting should be fine, because by definition an > > > anonymous getter doesn't have a name to enumerate. > > > > I tried to do it without NotEnumerable, but the generated code was changed and > > also > > required additional implementation of namedPRopertyQuery and > > namedPropertyEnumerator. > > LegacyUnenumerableNamedProperties suggests that named properties shouldn't be > > enumerable: > > https://heycam.github.io/webidl/#LegacyUnenumerableNamedProperties > > > > Ah, I missed the [LegacyUnenumerableNamedProperties] on the interface level. > https://software.hixie.ch/utilities/js/live-dom-viewer/?saved=4962 shows that > the namedItem name is enumerable, so adding [NotEnumerable] here would change > that. > > I guess the only choice is to remove [NotEnumerable] here and to keep the second > line for now, exception the argument doesn't need to be optional. > > I filed https://bugs.chromium.org/p/chromium/issues/detail?id=703990, can you > add a TODO to use it here? "If the [LegacyUnenumerableNamedProperties] extended attribute appears on a interface that supports named properties, it indicates that all the interface’s named properties are unenumerable"? Our WebIDL compiler indicates that by an attribute on the named property getter rather than one on the interface. But isn't it functionally equivalent?
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
Patchset #4 (id:60001) has been deleted
The CQ bit was checked by lunalu@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2752103003/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/plugins/Plugin.idl (right): https://codereview.chromium.org/2752103003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/plugins/Plugin.idl:31: [NotEnumerable] getter MimeType? namedItem(DOMString name); On 2017/03/24 15:42:17, loonybear wrote: > On 2017/03/22 05:04:52, foolip_UTC9 wrote: > > On 2017/03/21 17:56:50, loonybear wrote: > > > On 2017/03/21 08:57:52, foolip_UTC9 wrote: > > > > Adding [NotEnumerable] here should change the enumerability of the > namedItem > > > > method, which we don't want. I don't know why [NotEnumerable] was used > > > > originally below, but just omitting should be fine, because by definition > an > > > > anonymous getter doesn't have a name to enumerate. > > > > > > I tried to do it without NotEnumerable, but the generated code was changed > and > > > also > > > required additional implementation of namedPRopertyQuery and > > > namedPropertyEnumerator. > > > LegacyUnenumerableNamedProperties suggests that named properties shouldn't > be > > > enumerable: > > > https://heycam.github.io/webidl/#LegacyUnenumerableNamedProperties > > > > > > > Ah, I missed the [LegacyUnenumerableNamedProperties] on the interface level. > > https://software.hixie.ch/utilities/js/live-dom-viewer/?saved=4962 shows that > > the namedItem name is enumerable, so adding [NotEnumerable] here would change > > that. > > > > I guess the only choice is to remove [NotEnumerable] here and to keep the > second > > line for now, exception the argument doesn't need to be optional. > > > > I filed https://bugs.chromium.org/p/chromium/issues/detail?id=703990, can you > > add a TODO to use it here? > > "If the [LegacyUnenumerableNamedProperties] extended attribute appears on a > interface that supports named properties, it indicates that all the interface’s > named properties are unenumerable"? > > Our WebIDL compiler indicates that by an attribute on the named property getter > rather than one on the interface. But isn't it functionally equivalent? Not entirely, it seems like [NotEnumerable] also suppresses the generation of a namedPropertyQuery call, which I think should still work even if things aren't enumerable.
PTAL at the new patch https://codereview.chromium.org/2752103003/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/plugins/Plugin.idl (right): https://codereview.chromium.org/2752103003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/plugins/Plugin.idl:31: [NotEnumerable] getter MimeType? namedItem(DOMString name); On 2017/03/30 04:52:21, foolip_UTC9 wrote: > On 2017/03/24 15:42:17, loonybear wrote: > > On 2017/03/22 05:04:52, foolip_UTC9 wrote: > > > On 2017/03/21 17:56:50, loonybear wrote: > > > > On 2017/03/21 08:57:52, foolip_UTC9 wrote: > > > > > Adding [NotEnumerable] here should change the enumerability of the > > namedItem > > > > > method, which we don't want. I don't know why [NotEnumerable] was used > > > > > originally below, but just omitting should be fine, because by > definition > > an > > > > > anonymous getter doesn't have a name to enumerate. > > > > > > > > I tried to do it without NotEnumerable, but the generated code was changed > > and > > > > also > > > > required additional implementation of namedPRopertyQuery and > > > > namedPropertyEnumerator. > > > > LegacyUnenumerableNamedProperties suggests that named properties shouldn't > > be > > > > enumerable: > > > > https://heycam.github.io/webidl/#LegacyUnenumerableNamedProperties > > > > > > > > > > Ah, I missed the [LegacyUnenumerableNamedProperties] on the interface level. > > > https://software.hixie.ch/utilities/js/live-dom-viewer/?saved=4962 shows > that > > > the namedItem name is enumerable, so adding [NotEnumerable] here would > change > > > that. > > > > > > I guess the only choice is to remove [NotEnumerable] here and to keep the > > second > > > line for now, exception the argument doesn't need to be optional. > > > > > > I filed https://bugs.chromium.org/p/chromium/issues/detail?id=703990, can > you > > > add a TODO to use it here? > > > > "If the [LegacyUnenumerableNamedProperties] extended attribute appears on a > > interface that supports named properties, it indicates that all the > interface’s > > named properties are unenumerable"? > > > > Our WebIDL compiler indicates that by an attribute on the named property > getter > > rather than one on the interface. But isn't it functionally equivalent? > > Not entirely, it seems like [NotEnumerable] also suppresses the generation of a > namedPropertyQuery call, which I think should still work even if things aren't > enumerable. I see. I guess for now we will just keep both the getter and the the method.
Description was changed from ========== Make item and namedItem args in MimeTypeArray/PluginArray/Plugin non-optional Intent to Ship: https://groups.google.com/a/chromium.org/forum/?hl=en#!searchin/blink-dev/lun... BUG=701560, 701561 ========== to ========== Make item and namedItem args in MimeTypeArray/PluginArray/Plugin non-optional Intent to Ship: https://groups.google.com/a/chromium.org/d/msg/blink-dev/mGKzBiGe_Co/hMzVBa9v... BUG=701560, 701561 ==========
lgtm
The CQ bit was checked by lunalu@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 80001, "attempt_start_ts": 1491314246560950,
"parent_rev": "b3279fff89f248e0bf264586cc2c71f19d97de40", "commit_rev":
"a758d4d5c23e748d3db4d07e5f9ee310238419c3"}
Message was sent while issue was closed.
Description was changed from ========== Make item and namedItem args in MimeTypeArray/PluginArray/Plugin non-optional Intent to Ship: https://groups.google.com/a/chromium.org/d/msg/blink-dev/mGKzBiGe_Co/hMzVBa9v... BUG=701560, 701561 ========== to ========== Make item and namedItem args in MimeTypeArray/PluginArray/Plugin non-optional Intent to Ship: https://groups.google.com/a/chromium.org/d/msg/blink-dev/mGKzBiGe_Co/hMzVBa9v... BUG=701560, 701561 Review-Url: https://codereview.chromium.org/2752103003 Cr-Commit-Position: refs/heads/master@{#461726} Committed: https://chromium.googlesource.com/chromium/src/+/a758d4d5c23e748d3db4d07e5f9e... ==========
Message was sent while issue was closed.
Committed patchset #4 (id:80001) as https://chromium.googlesource.com/chromium/src/+/a758d4d5c23e748d3db4d07e5f9e... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
