Add keys(), values() and entries() methods on iterable<> interfaces

Add keys(), values() and entries() methods on iterable<> interfaces

Change the IDL code generator to add keys(), values() and entries()
methods to interfaces with an iterable<> definition.

Also add utility (mixin) base-classes for iterable interfaces,
ValueIterable<> and PairIterable<>, that take care of most of the work
of supporting iterable<> IDL definitions on the C++ side, i.e.
implementing keys(), values(), entries() and @@iterator.

Finally, update the two interfaces with iterable<> definitions
(core/testing/Internals.idl and modules/fetch/Headers.idl) to inherit
the new utility base classes. Internals has a "value iterator" and thus
inherits ValueIterable<>, while Headers has a "pair iterator" and
inherits PairIterable<>. Update existing tests for each to test the new
keys(), values() and entries() methods.

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

[Jens Widell, 2015-01-12 16:00:25]

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

[Jens Widell, 2015-01-12 16:00:25]

PTAL

[jsbell, 2015-01-12 17:14:08]

https://codereview.chromium.org/848673002/diff/20001/LayoutTests/http/tests/f...
File LayoutTests/http/tests/fetch/script-tests/headers.js (right):

https://codereview.chromium.org/848673002/diff/20001/LayoutTests/http/tests/f...
LayoutTests/http/tests/fetch/script-tests/headers.js:103: assert_true(key !=
deleteKey.toLowerCase());
I know this is copy/paste but..

assert_not_equals

https://codereview.chromium.org/848673002/diff/20001/LayoutTests/http/tests/f...
LayoutTests/http/tests/fetch/script-tests/headers.js:115: assert_true(key !=
deleteKey.toLowerCase());
assert_not_equals

https://codereview.chromium.org/848673002/diff/20001/LayoutTests/http/tests/f...
LayoutTests/http/tests/fetch/script-tests/headers.js:121: assert_true(key !=
deleteKey.toLowerCase());
assert_not_equals

https://codereview.chromium.org/848673002/diff/20001/Source/core/dom/Iterable.h
File Source/core/dom/Iterable.h (right):

https://codereview.chromium.org/848673002/diff/20001/Source/core/dom/Iterable...
Source/core/dom/Iterable.h:1: // Copyright 2015 The Chromium Authors. All rights
reserved.
New file needs Source/core/core.gypi entry?

https://codereview.chromium.org/848673002/diff/20001/Source/core/dom/Iterable...
Source/core/dom/Iterable.h:144: unsigned m_index;
Are there cases where this would be updated by a derived class? If not, should
it be private w/ an accessor instead?

https://codereview.chromium.org/848673002/diff/20001/Source/modules/fetch/Hea...
File Source/modules/fetch/Headers.cpp (right):

https://codereview.chromium.org/848673002/diff/20001/Source/modules/fetch/Hea...
Source/modules/fetch/Headers.cpp:47: size_t m_current;
Why not use m_index here?

[jsbell, 2015-01-12 17:15:17]

otherwise lgtm but I didn't look deeply at the code gen or its output

[Jens Widell, 2015-01-12 17:24:14]

https://codereview.chromium.org/848673002/diff/20001/Source/core/dom/Iterable.h
File Source/core/dom/Iterable.h (right):

https://codereview.chromium.org/848673002/diff/20001/Source/core/dom/Iterable...
Source/core/dom/Iterable.h:1: // Copyright 2015 The Chromium Authors. All rights
reserved.
On 2015/01/12 17:14:07, jsbell wrote:
> New file needs Source/core/core.gypi entry?

Compiles fine without it (apparently) but most header files appear to be listed.
Iterator.h (which also doesn't have a matching source file) is listed. I
randomly noticed just now that Node.h isn't listed though.

Will add.

https://codereview.chromium.org/848673002/diff/20001/Source/modules/fetch/Hea...
File Source/modules/fetch/Headers.cpp (right):

https://codereview.chromium.org/848673002/diff/20001/Source/modules/fetch/Hea...
Source/modules/fetch/Headers.cpp:47: size_t m_current;
On 2015/01/12 17:14:08, jsbell wrote:
> Why not use m_index here?

There is no m_index in scope here, since we're inheriting PairIterable<>, not
ValueIterable<>, and only ValueIterable<> has its own IterationSource which
tracks an index (since that's implicitly the key.)

That could be changed of course. But there's no guarantee an "iteration source"
uses an index counter to produce values; it could be using some other underlying
iteration mechanism.

[bashi, 2015-01-12 23:42:46]

https://codereview.chromium.org/848673002/diff/40001/Source/core/dom/Iterable.h
File Source/core/dom/Iterable.h (right):

https://codereview.chromium.org/848673002/diff/40001/Source/core/dom/Iterable...
Source/core/dom/Iterable.h:73: v8::Local<v8::Object> creationContext =
scriptState->context()->Global();
Help me understand: Always passing the global object to toV8() as a
creationContext here is OK?

[yhirano, 2015-01-13 05:18:42]

https://codereview.chromium.org/848673002/diff/40001/Source/core/dom/Iterable.h
File Source/core/dom/Iterable.h (right):

https://codereview.chromium.org/848673002/diff/40001/Source/core/dom/Iterable...
Source/core/dom/Iterable.h:48: // Otherwise: set |key| and |value| and return
true.
If end of iteration has been reached or an exception is thrown, you mean?

https://codereview.chromium.org/848673002/diff/40001/Source/core/dom/Iterable...
Source/core/dom/Iterable.h:55: virtual IterationSource*
startIteration(ScriptState*, ExceptionState&) = 0;
It is OK to make this function private?

https://codereview.chromium.org/848673002/diff/40001/Source/core/dom/Iterable...
Source/core/dom/Iterable.h:86: IterableIterator(IterationSource* source)
explicit

https://codereview.chromium.org/848673002/diff/40001/Source/core/dom/Iterable...
Source/core/dom/Iterable.h:129: virtual ~IterationSource() { }
[opt] ~IterasionSource() override { }

https://codereview.chromium.org/848673002/diff/40001/Source/core/dom/Iterable...
Source/core/dom/Iterable.h:136: virtual void trace(Visitor*) { }
void trace(Visitor* visitor) override
{
  Iterable<unsigned, ValueType>::IterationSource::trace(visitor);
}

https://codereview.chromium.org/848673002/diff/40001/Source/core/dom/Iterable...
Source/core/dom/Iterable.h:139: IterationSource()
We can make this constructor public -- in any case, we can't instantiate this
class without implementing the pure virtual function.

https://codereview.chromium.org/848673002/diff/40001/Source/core/dom/Iterable...
Source/core/dom/Iterable.h:144: unsigned m_index;
Is it necessary to have this member as protected? If not, I think having it as
private is better.

https://codereview.chromium.org/848673002/diff/40001/Source/core/dom/Iterable...
Source/core/dom/Iterable.h:147: bool next(ScriptState* scriptState, unsigned&
key, ValueType& value, ExceptionState& exceptionState) override
Can we mark this function as final?

https://codereview.chromium.org/848673002/diff/40001/Source/core/testing/Inte...
File Source/core/testing/Internals.cpp (right):

https://codereview.chromium.org/848673002/diff/40001/Source/core/testing/Inte...
Source/core/testing/Internals.cpp:161: virtual bool next(ScriptState*
scriptState, int& value, ExceptionState& exceptionState) override
No virtual is needed.

https://codereview.chromium.org/848673002/diff/40001/Source/modules/fetch/Hea...
File Source/modules/fetch/Headers.cpp (right):

https://codereview.chromium.org/848673002/diff/40001/Source/modules/fetch/Hea...
Source/modules/fetch/Headers.cpp:26: virtual bool next(ScriptState* scriptState,
String& key, String& value, ExceptionState& exception) override
No virtual is needed.

[Jens Widell, 2015-01-13 07:50:06]

https://codereview.chromium.org/848673002/diff/40001/Source/core/dom/Iterable.h
File Source/core/dom/Iterable.h (right):

https://codereview.chromium.org/848673002/diff/40001/Source/core/dom/Iterable...
Source/core/dom/Iterable.h:48: // Otherwise: set |key| and |value| and return
true.
On 2015/01/13 05:18:41, yhirano wrote:
> If end of iteration has been reached or an exception is thrown, you mean?

Done.

Strictly speaking it doesn't matter what you do if an exception is thrown; it
gets propagated and the rest of the "return value" ignored. But it certainly
makes sense to return false, since that leads to less unnecessary work being
done, so I'll adjust the comment.

https://codereview.chromium.org/848673002/diff/40001/Source/core/dom/Iterable...
Source/core/dom/Iterable.h:55: virtual IterationSource*
startIteration(ScriptState*, ExceptionState&) = 0;
On 2015/01/13 05:18:42, yhirano wrote:
> It is OK to make this function private?

Done.

https://codereview.chromium.org/848673002/diff/40001/Source/core/dom/Iterable...
Source/core/dom/Iterable.h:86: IterableIterator(IterationSource* source)
On 2015/01/13 05:18:41, yhirano wrote:
> explicit

Done.

https://codereview.chromium.org/848673002/diff/40001/Source/core/dom/Iterable...
Source/core/dom/Iterable.h:129: virtual ~IterationSource() { }
On 2015/01/13 05:18:42, yhirano wrote:
> [opt] ~IterasionSource() override { }

Done.

https://codereview.chromium.org/848673002/diff/40001/Source/core/dom/Iterable...
Source/core/dom/Iterable.h:136: virtual void trace(Visitor*) { }
On 2015/01/13 05:18:41, yhirano wrote:
> void trace(Visitor* visitor) override
> {
>   Iterable<unsigned, ValueType>::IterationSource::trace(visitor);
> }

After discussion with sof, I dropped this trace() instead as it served no
purpose.

https://codereview.chromium.org/848673002/diff/40001/Source/core/dom/Iterable...
Source/core/dom/Iterable.h:139: IterationSource()
On 2015/01/13 05:18:42, yhirano wrote:
> We can make this constructor public -- in any case, we can't instantiate this
> class without implementing the pure virtual function.

Done.

https://codereview.chromium.org/848673002/diff/40001/Source/core/dom/Iterable...
Source/core/dom/Iterable.h:144: unsigned m_index;
On 2015/01/13 05:18:41, yhirano wrote:
> Is it necessary to have this member as protected? If not, I think having it as
> private is better.

It's quite likely that a sub-class will use the current index to fetch the
current value. Not guaranteed, but since this class does track the current index
anyway, it seems a bit silly to hide it from the sub-class.

https://codereview.chromium.org/848673002/diff/40001/Source/core/dom/Iterable...
Source/core/dom/Iterable.h:147: bool next(ScriptState* scriptState, unsigned&
key, ValueType& value, ExceptionState& exceptionState) override
On 2015/01/13 05:18:42, yhirano wrote:
> Can we mark this function as final?

Done.

https://codereview.chromium.org/848673002/diff/40001/Source/core/testing/Inte...
File Source/core/testing/Internals.cpp (right):

https://codereview.chromium.org/848673002/diff/40001/Source/core/testing/Inte...
Source/core/testing/Internals.cpp:161: virtual bool next(ScriptState*
scriptState, int& value, ExceptionState& exceptionState) override
On 2015/01/13 05:18:42, yhirano wrote:
> No virtual is needed.

Done.

https://codereview.chromium.org/848673002/diff/40001/Source/modules/fetch/Hea...
File Source/modules/fetch/Headers.cpp (right):

https://codereview.chromium.org/848673002/diff/40001/Source/modules/fetch/Hea...
Source/modules/fetch/Headers.cpp:26: virtual bool next(ScriptState* scriptState,
String& key, String& value, ExceptionState& exception) override
On 2015/01/13 05:18:42, yhirano wrote:
> No virtual is needed.

Done.

[Jens Widell, 2015-01-13 07:54:36]

https://codereview.chromium.org/848673002/diff/40001/Source/core/dom/Iterable.h
File Source/core/dom/Iterable.h (right):

https://codereview.chromium.org/848673002/diff/40001/Source/core/dom/Iterable...
Source/core/dom/Iterable.h:73: v8::Local<v8::Object> creationContext =
scriptState->context()->Global();
On 2015/01/12 23:42:46, bashi1 wrote:
> Help me understand: Always passing the global object to toV8() as a
> creationContext here is OK?

I'm the wrong person to answer this question, but I can note that this is what
v8IteratorResult() does. (I copied it from there.)

[haraken, 2015-01-13 07:58:17]

https://codereview.chromium.org/848673002/diff/40001/Source/core/dom/Iterable.h
File Source/core/dom/Iterable.h (right):

https://codereview.chromium.org/848673002/diff/40001/Source/core/dom/Iterable...
Source/core/dom/Iterable.h:73: v8::Local<v8::Object> creationContext =
scriptState->context()->Global();
On 2015/01/13 07:54:35, Jens Widell wrote:
> On 2015/01/12 23:42:46, bashi1 wrote:
> > Help me understand: Always passing the global object to toV8() as a
> > creationContext here is OK?
> 
> I'm the wrong person to answer this question, but I can note that this is what
> v8IteratorResult() does. (I copied it from there.)

Am I correct in that select() is always called via JavaScript and the
|scriptState| points to the current ScriptState of the JavaScript? Then using
scriptState->context()->Global() looks correct.

[Jens Widell, 2015-01-13 08:01:44]

https://codereview.chromium.org/848673002/diff/40001/Source/core/dom/Iterable.h
File Source/core/dom/Iterable.h (right):

https://codereview.chromium.org/848673002/diff/40001/Source/core/dom/Iterable...
Source/core/dom/Iterable.h:73: v8::Local<v8::Object> creationContext =
scriptState->context()->Global();
On 2015/01/13 07:58:17, haraken wrote:
> On 2015/01/13 07:54:35, Jens Widell wrote:
> > On 2015/01/12 23:42:46, bashi1 wrote:
> > > Help me understand: Always passing the global object to toV8() as a
> > > creationContext here is OK?
> > 
> > I'm the wrong person to answer this question, but I can note that this is
what
> > v8IteratorResult() does. (I copied it from there.)
> 
> Am I correct in that select() is always called via JavaScript and the
> |scriptState| points to the current ScriptState of the JavaScript? Then using
> scriptState->context()->Global() looks correct.

It's only called from the iterator's next() method implementation, from
JavaScript.

[yhirano, 2015-01-13 10:44:04]

lgtm

https://codereview.chromium.org/848673002/diff/60001/Source/core/dom/Iterable.h
File Source/core/dom/Iterable.h (right):

https://codereview.chromium.org/848673002/diff/60001/Source/core/dom/Iterable...
Source/core/dom/Iterable.h:58: static KeyType select(ScriptState*, KeyType key,
ValueType value)
const KeyType& and const ValueType& are better, perhaps?

[Jens Widell, 2015-01-13 10:53:17]

https://codereview.chromium.org/848673002/diff/60001/Source/core/dom/Iterable.h
File Source/core/dom/Iterable.h (right):

https://codereview.chromium.org/848673002/diff/60001/Source/core/dom/Iterable...
Source/core/dom/Iterable.h:58: static KeyType select(ScriptState*, KeyType key,
ValueType value)
On 2015/01/13 10:44:04, yhirano wrote:
> const KeyType& and const ValueType& are better, perhaps?

Done.

[haraken, 2015-01-13 12:46:31]

LGTM

https://codereview.chromium.org/848673002/diff/80001/Source/core/dom/Iterable.h
File Source/core/dom/Iterable.h (right):

https://codereview.chromium.org/848673002/diff/80001/Source/core/dom/Iterable...
Source/core/dom/Iterable.h:75: Vector<ScriptValue> entry;

Let's use Vector<ScriptValue, 2>.

https://codereview.chromium.org/848673002/diff/80001/Source/modules/fetch/Hea...
File Source/modules/fetch/Headers.cpp (right):

https://codereview.chromium.org/848673002/diff/80001/Source/modules/fetch/Hea...
Source/modules/fetch/Headers.cpp:24: HeadersIterationSource(FetchHeaderList*
headers) : m_headers(headers), m_current(0) { }

Add explicit.

https://codereview.chromium.org/848673002/diff/80001/Source/modules/fetch/Hea...
Source/modules/fetch/Headers.cpp:30: if (m_current >= m_headers->size())

m_current => m_index

https://codereview.chromium.org/848673002/diff/80001/Source/modules/fetch/Hea...
Source/modules/fetch/Headers.cpp:41: PairIterable<String,
String>::IterationSource::trace(visitor);

Make this a tail call of trace() (Just a convention.)

[Jens Widell, 2015-01-13 12:57:41]

https://codereview.chromium.org/848673002/diff/80001/Source/core/dom/Iterable.h
File Source/core/dom/Iterable.h (right):

https://codereview.chromium.org/848673002/diff/80001/Source/core/dom/Iterable...
Source/core/dom/Iterable.h:75: Vector<ScriptValue> entry;
On 2015/01/13 12:46:31, haraken wrote:
> 
> Let's use Vector<ScriptValue, 2>.

Done.

https://codereview.chromium.org/848673002/diff/80001/Source/modules/fetch/Hea...
File Source/modules/fetch/Headers.cpp (right):

https://codereview.chromium.org/848673002/diff/80001/Source/modules/fetch/Hea...
Source/modules/fetch/Headers.cpp:24: HeadersIterationSource(FetchHeaderList*
headers) : m_headers(headers), m_current(0) { }
On 2015/01/13 12:46:31, haraken wrote:
> 
> Add explicit.

Done.

https://codereview.chromium.org/848673002/diff/80001/Source/modules/fetch/Hea...
Source/modules/fetch/Headers.cpp:41: PairIterable<String,
String>::IterationSource::trace(visitor);
On 2015/01/13 12:46:31, haraken wrote:
> 
> Make this a tail call of trace() (Just a convention.)

Done.

[Jens Widell, 2015-01-13 13:07:22]

https://codereview.chromium.org/848673002/diff/80001/Source/modules/fetch/Hea...
File Source/modules/fetch/Headers.cpp (right):

https://codereview.chromium.org/848673002/diff/80001/Source/modules/fetch/Hea...
Source/modules/fetch/Headers.cpp:30: if (m_current >= m_headers->size())
On 2015/01/13 12:46:31, haraken wrote:
> 
> m_current => m_index

Joshua made a similar comment earlier, so I'll just copy my response to that:

There is no m_index in scope here, since we're inheriting PairIterable<>, not
ValueIterable<>, and only ValueIterable<> has its own IterationSource which
tracks an index (since that's implicitly the key.)

That could be changed of course. But there's no guarantee an "iteration source"
uses an index counter to produce values; it could be using some other underlying
iteration mechanism.

[bashi, 2015-01-13 23:33:13]

LGTM

[Jens Widell, 2015-01-14 08:08:10]

The CQ bit was checked by jl@opera.com

[commit-bot: I haz the power, 2015-01-14 08:09:11]

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

[commit-bot: I haz the power, 2015-01-14 08:13:01]

Committed patchset #7 (id:120001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=188364