Add Maplike<> utility mixin class for implementing maplike interfaces

Add Maplike<> utility mixin class for implementing maplike interfaces

This utility class inherits PairIterable<> and currently implements has()
and get() based on a single virtual getMapEntry() function that a sub-class
needs to implement. It takes care of the slight complication that get()
should return undefined if the requested key is not in the map, which is
somewhat impractical to implement (requires constructing a ScriptValue to
return.)

Update the maplike implementations MIDIPortMap and MediaKeyStatusMap to
inherit Maplike<> instead of its base-class PairIterable<>, and remove
their has()/get() implementations.

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

[Jens Widell, 2015-02-12 13:22:28]

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

[Jens Widell, 2015-02-12 13:22:29]

PTAL

[haraken, 2015-02-12 13:39:39]

https://codereview.chromium.org/920713002/diff/1/Source/core/dom/Maplike.h
File Source/core/dom/Maplike.h (right):

https://codereview.chromium.org/920713002/diff/1/Source/core/dom/Maplike.h#ne...
Source/core/dom/Maplike.h:20: return getMapEntry(scriptState, key, value,
exceptionState);

It's a bit inefficient to call getMapEntry() to implement has(). For
performance, get() should be implemented using has() (just like the current
getForBinding is doing). What do you think?

[haraken, 2015-02-12 13:41:36]

https://codereview.chromium.org/920713002/diff/1/Source/core/dom/Maplike.h
File Source/core/dom/Maplike.h (right):

https://codereview.chromium.org/920713002/diff/1/Source/core/dom/Maplike.h#ne...
Source/core/dom/Maplike.h:10: #include "core/dom/Iterable.h"

Nit: Would core/dom/ be a right place for Iterable, Maplike etc? Another
candidate would be bindings/core/v8/. (I don't have a strong opinion.)

[Jens Widell, 2015-02-12 13:50:49]

https://codereview.chromium.org/920713002/diff/1/Source/core/dom/Maplike.h
File Source/core/dom/Maplike.h (right):

https://codereview.chromium.org/920713002/diff/1/Source/core/dom/Maplike.h#ne...
Source/core/dom/Maplike.h:10: #include "core/dom/Iterable.h"
On 2015/02/12 13:41:36, haraken wrote:
> 
> Nit: Would core/dom/ be a right place for Iterable, Maplike etc? Another
> candidate would be bindings/core/v8/. (I don't have a strong opinion.)

I don't know. Maybe bindings/core/v8/ makes more sense. (I have no strong
opinion either.)

I put it here because this is where Iterator.h is, but they don't have to be in
the same place. (And maybe it should move too...)

https://codereview.chromium.org/920713002/diff/1/Source/core/dom/Maplike.h#ne...
Source/core/dom/Maplike.h:20: return getMapEntry(scriptState, key, value,
exceptionState);
On 2015/02/12 13:39:39, haraken wrote:
> 
> It's a bit inefficient to call getMapEntry() to implement has(). For
> performance, get() should be implemented using has() (just like the current
> getForBinding is doing). What do you think?

It's not a lot inefficient though in most cases. In the current uses, I'm not
worried at all about it. I did it this way to reduce the amount of
implementation needed in a sub-class.

If extracting the value is especially expensive in some case, it's always
possible to explicitly declare has() (and/or get()) in the IDL and implement it
manually, or to simply override this hasForBinding() manually in the sub-class,
and, of course, to not use this class at all.

[haraken, 2015-02-12 13:56:50]

LGTM

https://codereview.chromium.org/920713002/diff/1/Source/core/dom/Maplike.h
File Source/core/dom/Maplike.h (right):

https://codereview.chromium.org/920713002/diff/1/Source/core/dom/Maplike.h#ne...
Source/core/dom/Maplike.h:10: #include "core/dom/Iterable.h"
On 2015/02/12 13:50:47, Jens Widell wrote:
> On 2015/02/12 13:41:36, haraken wrote:
> > 
> > Nit: Would core/dom/ be a right place for Iterable, Maplike etc? Another
> > candidate would be bindings/core/v8/. (I don't have a strong opinion.)
> 
> I don't know. Maybe bindings/core/v8/ makes more sense. (I have no strong
> opinion either.)
> 
> I put it here because this is where Iterator.h is, but they don't have to be
in
> the same place. (And maybe it should move too...)

Either way let's consider in a separate CL.

https://codereview.chromium.org/920713002/diff/1/Source/core/dom/Maplike.h#ne...
Source/core/dom/Maplike.h:20: return getMapEntry(scriptState, key, value,
exceptionState);
On 2015/02/12 13:50:48, Jens Widell wrote:
> On 2015/02/12 13:39:39, haraken wrote:
> > 
> > It's a bit inefficient to call getMapEntry() to implement has(). For
> > performance, get() should be implemented using has() (just like the current
> > getForBinding is doing). What do you think?
> 
> It's not a lot inefficient though in most cases. In the current uses, I'm not
> worried at all about it. I did it this way to reduce the amount of
> implementation needed in a sub-class.
> 
> If extracting the value is especially expensive in some case, it's always
> possible to explicitly declare has() (and/or get()) in the IDL and implement
it
> manually, or to simply override this hasForBinding() manually in the
sub-class,
> and, of course, to not use this class at all.

Sounds reasonable.

[Jens Widell, 2015-02-12 14:05:57]

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

[jrummell, 2015-02-12 18:18:10]

I like it. modules/encryptedmedia lgtm.

[Jens Widell, 2015-02-13 06:11:05]

The CQ bit was checked by jl@opera.com

[commit-bot: I haz the power, 2015-02-13 06:11:15]

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

[commit-bot: I haz the power, 2015-02-13 06:13:53]

Committed patchset #2 (id:20001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=190113