IDL: Auto-declare remaining methods implied by maplike<>/setlike<>

IDL: Auto-declare remaining methods implied by maplike<>/setlike<>

For maplike<K, V>, declare these:

  boolean has(K key);
  any get(K key);

  // read-write only:
  void clear();
  boolean delete(K key);
  X set(K key, V value); // returns self

For setlike<V>, declare these:

  boolean has(V value);

  // read-write only:
  X add(V value); // returns self
  void clear();
  boolean delete(V value);

In interface declarations that contains explicit declarations of methods
with these names already, those existing declarations override the auto-
declared ones. This is the case for all existing uses of maplike<> and
setlike<> in Blink, so this CL does not add any new methods in practice.

To avoid naming conflicts in a uniform way, also add the suffix
"ForBinding" to all auto-declared methods' names on the C++ side, via
the [ImplementedAs=...] extended attribute.

BUG=432683
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=189880

[Jens Widell, 2015-02-09 12:29:19]

jl@opera.com changed reviewers:
+ haraken@chromium.org

[Jens Widell, 2015-02-09 12:29:20]

PTAL

https://codereview.chromium.org/911433003/diff/1/Source/bindings/tests/result...
File Source/bindings/tests/results/core/V8TestInterfaceGarbageCollected.cpp
(right):

https://codereview.chromium.org/911433003/diff/1/Source/bindings/tests/result...
Source/bindings/tests/results/core/V8TestInterfaceGarbageCollected.cpp:225:
RawPtr<TestInterfaceGarbageCollected> result = impl->add(scriptState, value,
exceptionState);
A somewhat appealing alternative would be to implement something like
[ReturnsThis] and let the C++-side have no return value.

Not that "return this;" at the end of the C++ implementation is particularly
burdensome, of course.

Also note that there's a WebIDL spec bug here, I think. Both Maplike and Setlike
are described in terms of there being an internal Map or Set object to which
these operations redirect. And ECMAScript's Map.set()/Set.add() returns the
Map/Set object, which would mean that Maplike's set() would return the internal
Map and Setlike's add() the internal Set, thus exposing the internal objects. I
doubt that's intentional.

[haraken, 2015-02-09 13:53:12]

LGTM

> In interface declarations that contains explicit declarations of methods
> with these names already, those existing declarations override the auto-
> declared ones.

Are these overridings intentional or are we planning to remove the overridings
once this CL lands?

https://codereview.chromium.org/911433003/diff/1/Source/bindings/tests/result...
File Source/bindings/tests/results/core/V8TestInterfaceGarbageCollected.cpp
(right):

https://codereview.chromium.org/911433003/diff/1/Source/bindings/tests/result...
Source/bindings/tests/results/core/V8TestInterfaceGarbageCollected.cpp:225:
RawPtr<TestInterfaceGarbageCollected> result = impl->add(scriptState, value,
exceptionState);
On 2015/02/09 12:29:19, Jens Widell wrote:
> A somewhat appealing alternative would be to implement something like
> [ReturnsThis] and let the C++-side have no return value.
> 
> Not that "return this;" at the end of the C++ implementation is particularly
> burdensome, of course.
> 
> Also note that there's a WebIDL spec bug here, I think. Both Maplike and
Setlike
> are described in terms of there being an internal Map or Set object to which
> these operations redirect. And ECMAScript's Map.set()/Set.add() returns the
> Map/Set object, which would mean that Maplike's set() would return the
internal
> Map and Setlike's add() the internal Set, thus exposing the internal objects.
I
> doubt that's intentional.

Good point, shall we file a bug against the spec?

[Jens Widell, 2015-02-09 15:03:05]

> > In interface declarations that contains explicit declarations of methods
> > with these names already, those existing declarations override the auto-
> > declared ones.
> 
> Are these overridings intentional or are we planning to remove the overridings
> once this CL lands?

It's two methods each in two nearly identical interfaces
(MIDI{Input,Output}Map), and neither is exactly like what's being auto-declared
here, so if we drop the overridings we'd need to make some code changes (that
just adds some unused arguments in the calls.)

So in those cases I think it would be better to leave things as they are,
actually.

I think auto-declaring these in some way (that might not be exactly what any
interface implementation actually wants) is still useful, in that it makes sure
that the required methods get declared one way or the other. If you don't
declare one yourself, you get one anyway, that you must implement to compile the
code. IOW, we might end up actually overriding these in all maplike<>/setlike<>
interfaces, and that might be fine.

(Although if we discover that all implementations want the same thing, we could
change the auto-declarations to match that later on, and simplify some IDL
files.)

https://codereview.chromium.org/911433003/diff/1/Source/bindings/tests/result...
File Source/bindings/tests/results/core/V8TestInterfaceGarbageCollected.cpp
(right):

https://codereview.chromium.org/911433003/diff/1/Source/bindings/tests/result...
Source/bindings/tests/results/core/V8TestInterfaceGarbageCollected.cpp:225:
RawPtr<TestInterfaceGarbageCollected> result = impl->add(scriptState, value,
exceptionState);
On 2015/02/09 13:53:11, haraken wrote:
> On 2015/02/09 12:29:19, Jens Widell wrote:
> > A somewhat appealing alternative would be to implement something like
> > [ReturnsThis] and let the C++-side have no return value.
> > 
> > Not that "return this;" at the end of the C++ implementation is particularly
> > burdensome, of course.
> > 
> > Also note that there's a WebIDL spec bug here, I think. Both Maplike and
> Setlike
> > are described in terms of there being an internal Map or Set object to which
> > these operations redirect. And ECMAScript's Map.set()/Set.add() returns the
> > Map/Set object, which would mean that Maplike's set() would return the
> internal
> > Map and Setlike's add() the internal Set, thus exposing the internal
objects.
> I
> > doubt that's intentional.
> 
> Good point, shall we file a bug against the spec?

Yes, I will look into it. Last time I looked at this part of the spec before
today the problematic text didn't even exist, so it's apparently an area being
worked on.

https://codereview.chromium.org/911433003/diff/1/Source/bindings/tests/result...
Source/bindings/tests/results/core/V8TestInterfaceGarbageCollected.cpp:273: bool
result = impl->delete(scriptState, value, exceptionState);
It occurs to me that "delete" is not a good function name in C++. :-)

Is there a standard naming scheme for this type of conflict that would make
sense to use here? "deleteForBindings"?

[haraken, 2015-02-09 15:53:53]

https://codereview.chromium.org/911433003/diff/1/Source/bindings/tests/result...
File Source/bindings/tests/results/core/V8TestInterfaceGarbageCollected.cpp
(right):

https://codereview.chromium.org/911433003/diff/1/Source/bindings/tests/result...
Source/bindings/tests/results/core/V8TestInterfaceGarbageCollected.cpp:273: bool
result = impl->delete(scriptState, value, exceptionState);
On 2015/02/09 15:03:05, Jens Widell wrote:
> It occurs to me that "delete" is not a good function name in C++. :-)
> 
> Is there a standard naming scheme for this type of conflict that would make
> sense to use here? "deleteForBindings"?

We normally use deleteForBinding or deleteFunction.

[Jens Widell, 2015-02-09 15:56:12]

New patchsets have been uploaded after l-g-t-m from haraken@chromium.org

[Jens Widell, 2015-02-09 15:56:48]

https://codereview.chromium.org/911433003/diff/1/Source/bindings/tests/result...
File Source/bindings/tests/results/core/V8TestInterfaceGarbageCollected.cpp
(right):

https://codereview.chromium.org/911433003/diff/1/Source/bindings/tests/result...
Source/bindings/tests/results/core/V8TestInterfaceGarbageCollected.cpp:273: bool
result = impl->delete(scriptState, value, exceptionState);
On 2015/02/09 15:53:53, haraken wrote:
> On 2015/02/09 15:03:05, Jens Widell wrote:
> > It occurs to me that "delete" is not a good function name in C++. :-)
> > 
> > Is there a standard naming scheme for this type of conflict that would make
> > sense to use here? "deleteForBindings"?
> 
> We normally use deleteForBinding or deleteFunction.

Changed to "deleteForBinding".

[haraken, 2015-02-09 15:58:42]

On 2015/02/09 15:56:48, Jens Widell wrote:
>
https://codereview.chromium.org/911433003/diff/1/Source/bindings/tests/result...
> File Source/bindings/tests/results/core/V8TestInterfaceGarbageCollected.cpp
> (right):
> 
>
https://codereview.chromium.org/911433003/diff/1/Source/bindings/tests/result...
> Source/bindings/tests/results/core/V8TestInterfaceGarbageCollected.cpp:273:
bool
> result = impl->delete(scriptState, value, exceptionState);
> On 2015/02/09 15:53:53, haraken wrote:
> > On 2015/02/09 15:03:05, Jens Widell wrote:
> > > It occurs to me that "delete" is not a good function name in C++. :-)
> > > 
> > > Is there a standard naming scheme for this type of conflict that would
make
> > > sense to use here? "deleteForBindings"?
> > 
> > We normally use deleteForBinding or deleteFunction.
> 
> Changed to "deleteForBinding".

BTW, clear() is going to be easily conflict with RefCounted::clear or
GarbageCollected::clear (if the object inherits from RefCounted or
GarbageCollected), isn't it?

If that is the case, we might want to add a common suffix for these
maplike/setlike methods.

[Jens Widell, 2015-02-09 16:05:06]

On 2015/02/09 15:58:42, haraken wrote:
> On 2015/02/09 15:56:48, Jens Widell wrote:
> >
>
https://codereview.chromium.org/911433003/diff/1/Source/bindings/tests/result...
> > File Source/bindings/tests/results/core/V8TestInterfaceGarbageCollected.cpp
> > (right):
> > 
> >
>
https://codereview.chromium.org/911433003/diff/1/Source/bindings/tests/result...
> > Source/bindings/tests/results/core/V8TestInterfaceGarbageCollected.cpp:273:
> bool
> > result = impl->delete(scriptState, value, exceptionState);
> > On 2015/02/09 15:53:53, haraken wrote:
> > > On 2015/02/09 15:03:05, Jens Widell wrote:
> > > > It occurs to me that "delete" is not a good function name in C++. :-)
> > > > 
> > > > Is there a standard naming scheme for this type of conflict that would
> make
> > > > sense to use here? "deleteForBindings"?
> > > 
> > > We normally use deleteForBinding or deleteFunction.
> > 
> > Changed to "deleteForBinding".
> 
> BTW, clear() is going to be easily conflict with RefCounted::clear or
> GarbageCollected::clear (if the object inherits from RefCounted or
> GarbageCollected), isn't it?

I think you mean RefPtr::clear() and Member::clear()? Those won't conflict.

> If that is the case, we might want to add a common suffix for these
> maplike/setlike methods.

This may be true still. The existing maplike interfaces use "getForBinding"
currently, since that's implemented by a common base class, and the actual
classes have another get() that hides the base class's.

Should we use -ForBinding for all of them, or something like
-FromMaplike/-FromSetlike? (Or something else altogether?)

[haraken, 2015-02-09 16:10:00]

On 2015/02/09 16:05:06, Jens Widell wrote:
> On 2015/02/09 15:58:42, haraken wrote:
> > On 2015/02/09 15:56:48, Jens Widell wrote:
> > >
> >
>
https://codereview.chromium.org/911433003/diff/1/Source/bindings/tests/result...
> > > File
Source/bindings/tests/results/core/V8TestInterfaceGarbageCollected.cpp
> > > (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/911433003/diff/1/Source/bindings/tests/result...
> > >
Source/bindings/tests/results/core/V8TestInterfaceGarbageCollected.cpp:273:
> > bool
> > > result = impl->delete(scriptState, value, exceptionState);
> > > On 2015/02/09 15:53:53, haraken wrote:
> > > > On 2015/02/09 15:03:05, Jens Widell wrote:
> > > > > It occurs to me that "delete" is not a good function name in C++. :-)
> > > > > 
> > > > > Is there a standard naming scheme for this type of conflict that would
> > make
> > > > > sense to use here? "deleteForBindings"?
> > > > 
> > > > We normally use deleteForBinding or deleteFunction.
> > > 
> > > Changed to "deleteForBinding".
> > 
> > BTW, clear() is going to be easily conflict with RefCounted::clear or
> > GarbageCollected::clear (if the object inherits from RefCounted or
> > GarbageCollected), isn't it?
> 
> I think you mean RefPtr::clear() and Member::clear()? Those won't conflict.

Ah, you're right.

> > If that is the case, we might want to add a common suffix for these
> > maplike/setlike methods.
> 
> This may be true still. The existing maplike interfaces use "getForBinding"
> currently, since that's implemented by a common base class, and the actual
> classes have another get() that hides the base class's.
> 
> Should we use -ForBinding for all of them, or something like
> -FromMaplike/-FromSetlike? (Or something else altogether?)

A common suffix sounds good, although I don't have any preference on naming.
Something common between setlike and maplike would sound good, so ForBinding?

[Jens Widell, 2015-02-09 16:23:49]

On 2015/02/09 16:10:00, haraken wrote:
> On 2015/02/09 16:05:06, Jens Widell wrote:
> > On 2015/02/09 15:58:42, haraken wrote:
> > > On 2015/02/09 15:56:48, Jens Widell wrote:
> > > >
> > >
> >
>
https://codereview.chromium.org/911433003/diff/1/Source/bindings/tests/result...
> > > > File
> Source/bindings/tests/results/core/V8TestInterfaceGarbageCollected.cpp
> > > > (right):
> > > > 
> > > >
> > >
> >
>
https://codereview.chromium.org/911433003/diff/1/Source/bindings/tests/result...
> > > >
> Source/bindings/tests/results/core/V8TestInterfaceGarbageCollected.cpp:273:
> > > bool
> > > > result = impl->delete(scriptState, value, exceptionState);
> > > > On 2015/02/09 15:53:53, haraken wrote:
> > > > > On 2015/02/09 15:03:05, Jens Widell wrote:
> > > > > > It occurs to me that "delete" is not a good function name in C++.
:-)
> > > > > > 
> > > > > > Is there a standard naming scheme for this type of conflict that
would
> > > make
> > > > > > sense to use here? "deleteForBindings"?
> > > > > 
> > > > > We normally use deleteForBinding or deleteFunction.
> > > > 
> > > > Changed to "deleteForBinding".
> > > 
> > > BTW, clear() is going to be easily conflict with RefCounted::clear or
> > > GarbageCollected::clear (if the object inherits from RefCounted or
> > > GarbageCollected), isn't it?
> > 
> > I think you mean RefPtr::clear() and Member::clear()? Those won't conflict.
> 
> Ah, you're right.
> 
> > > If that is the case, we might want to add a common suffix for these
> > > maplike/setlike methods.
> > 
> > This may be true still. The existing maplike interfaces use "getForBinding"
> > currently, since that's implemented by a common base class, and the actual
> > classes have another get() that hides the base class's.
> > 
> > Should we use -ForBinding for all of them, or something like
> > -FromMaplike/-FromSetlike? (Or something else altogether?)
> 
> A common suffix sounds good, although I don't have any preference on naming.
> Something common between setlike and maplike would sound good, so ForBinding?

Common suffix "ForBinding" added.

There's now some CG changes, and the implementations in core/dom/Iterable.h were
renamed to match.

[haraken, 2015-02-10 01:05:45]

LGTM

[Jens Widell, 2015-02-10 06:00:56]

The CQ bit was checked by jl@opera.com

[commit-bot: I haz the power, 2015-02-10 06:01:59]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/911433003/40001

[commit-bot: I haz the power, 2015-02-10 08:25:29]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-02-10 08:25:30]

Try jobs failed on following builders:
  mac_blink_rel on tryserver.blink (JOB_FAILED,
http://build.chromium.org/p/tryserver.blink/builders/mac_blink_rel/builds/42236)

[Jens Widell, 2015-02-10 08:26:46]

The CQ bit was checked by jl@opera.com

[commit-bot: I haz the power, 2015-02-10 08:27:13]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/911433003/40001

[commit-bot: I haz the power, 2015-02-10 09:31:06]

Committed patchset #3 (id:40001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=189880