New FormData methods: get, getAll, has, set, delete and iterable

New FormData methods: get, getAll, has, set, delete and iterable

Previously, FormData was write-only via append(). This
fleshes out the object with other ways to manipulate and
inspect the data.

Also adds ES6 iterator support and the implied
keys(), entries() and values() methods.

All the new functionality is hidden behind the experimental
features flag per the "intent to implement" discussion.

Spec: http://xhr.spec.whatwg.org/#interface-formdata
Intent: https://groups.google.com/a/chromium.org/forum/#!msg/blink-dev/ksXEB6uB7RY/ZwtCdZGyIVgJ

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

[jsbell, 2014-09-11 20:37:23]

jsbell@chromium.org changed reviewers:
+ sigbjornf@opera.com, tyoshino@chromium.org

[jsbell, 2014-09-11 23:53:04]

Added getAll() support.

The File-with-filename case is outstanding.

[tyoshino (SeeGerritForStatus), 2014-09-12 07:29:32]

https://codereview.chromium.org/564963002/diff/20001/Source/core/html/FormDat...
File Source/core/html/FormDataList.cpp (right):

https://codereview.chromium.org/564963002/diff/20001/Source/core/html/FormDat...
Source/core/html/FormDataList.cpp:117: return EntryValue(File::create(filename,
currentTimeMS(), value.blob()->blobDataHandle()));
this looks equivalent to what's specified in the spec. just curious if we should
make the spec also create a File with name="blob" on get(), getAll() or not.
regarding editorial cost, current spec seems shorter than writing that in get(),
getAll() definition.

[tyoshino (SeeGerritForStatus), 2014-09-12 07:52:54]

https://codereview.chromium.org/564963002/diff/20001/LayoutTests/http/tests/l...
File LayoutTests/http/tests/local/formdata/formdata-methods.html (right):

https://codereview.chromium.org/564963002/diff/20001/LayoutTests/http/tests/l...
LayoutTests/http/tests/local/formdata/formdata-methods.html:48: message =
message || 'Blob daata should match';
daata -> data

[sof, 2014-09-12 15:21:26]

https://codereview.chromium.org/564963002/diff/20001/Source/core/html/DOMForm...
File Source/core/html/DOMFormData.h (right):

https://codereview.chromium.org/564963002/diff/20001/Source/core/html/DOMForm...
Source/core/html/DOMFormData.h:65: void deleteFunction(const String& name);
nit: some precedence for using ImplementedAs=remove for delete() operations.

https://codereview.chromium.org/564963002/diff/20001/Source/core/html/FormDat...
File Source/core/html/FormDataList.cpp (right):

https://codereview.chromium.org/564963002/diff/20001/Source/core/html/FormDat...
Source/core/html/FormDataList.cpp:85: Vector<FormDataList::EntryValue> matches;
Vector => WillBeHeapVector

https://codereview.chromium.org/564963002/diff/20001/Source/core/html/FormDat...
File Source/core/html/FormDataList.h (right):

https://codereview.chromium.org/564963002/diff/20001/Source/core/html/FormDat...
Source/core/html/FormDataList.h:36: class EntryValue {
Add ALLOW_ONLY_INLINE_ALLOCATION();

https://codereview.chromium.org/564963002/diff/20001/Source/core/html/FormDat...
Source/core/html/FormDataList.h:36: class EntryValue {
Add FINAL

https://codereview.chromium.org/564963002/diff/20001/Source/core/html/FormDat...
Source/core/html/FormDataList.h:50: 
Add 

  void trace(Visitor*);

and define it as

  void FormDataList::EntryValue::trace(Visitor* visitor)
  {
      visitor->trace(m_file);
  }

https://codereview.chromium.org/564963002/diff/20001/Source/core/html/FormDat...
Source/core/html/FormDataList.h:101: Vector<EntryValue> getAll(const String&
key) const;
Vector => WillBeHeapVector

[jsbell, 2014-09-12 16:30:10]

Thanks, for the reviews!

FYI, I'm just doing this "for fun" since it looked pretty easy, so I may not
finish it up promptly. I also haven't checked to see if Gecko supports this yet,
and may need to send out an "Intent".

https://codereview.chromium.org/564963002/diff/20001/LayoutTests/http/tests/l...
File LayoutTests/http/tests/local/formdata/formdata-methods.html (right):

https://codereview.chromium.org/564963002/diff/20001/LayoutTests/http/tests/l...
LayoutTests/http/tests/local/formdata/formdata-methods.html:48: message =
message || 'Blob daata should match';
On 2014/09/12 07:52:54, tyoshino wrote:
> daata -> data

Done.

https://codereview.chromium.org/564963002/diff/20001/Source/core/html/DOMForm...
File Source/core/html/DOMFormData.h (right):

https://codereview.chromium.org/564963002/diff/20001/Source/core/html/DOMForm...
Source/core/html/DOMFormData.h:65: void deleteFunction(const String& name);
On 2014/09/12 15:21:26, sof wrote:
> nit: some precedence for using ImplementedAs=remove for delete() operations. 

Done.

https://codereview.chromium.org/564963002/diff/20001/Source/core/html/FormDat...
File Source/core/html/FormDataList.cpp (right):

https://codereview.chromium.org/564963002/diff/20001/Source/core/html/FormDat...
Source/core/html/FormDataList.cpp:85: Vector<FormDataList::EntryValue> matches;
On 2014/09/12 15:21:26, sof wrote:
> Vector => WillBeHeapVector

Done.

https://codereview.chromium.org/564963002/diff/20001/Source/core/html/FormDat...
Source/core/html/FormDataList.cpp:117: return EntryValue(File::create(filename,
currentTimeMS(), value.blob()->blobDataHandle()));
On 2014/09/12 07:29:32, tyoshino wrote:
> this looks equivalent to what's specified in the spec.

Thanks! 

> just curious if we should
> make the spec also create a File with name="blob" on get(), getAll() or not.
> regarding editorial cost, current spec seems shorter than writing that in
get(),
> getAll() definition.

In the spec this is done during append/set, when adding the value to the
internal entries list. I'm currently leaving that existing logic alone and doing
the work on extraction from the list, which is why it's more complicated than
the spec.

I should at least add a FIXME to consider moving this to the append/set methods,
and it would simplify Item, and Blob->File conversion should not be a
performance hit. I need figure out how to handle File+filename, though.

https://codereview.chromium.org/564963002/diff/20001/Source/core/html/FormDat...
File Source/core/html/FormDataList.h (right):

https://codereview.chromium.org/564963002/diff/20001/Source/core/html/FormDat...
Source/core/html/FormDataList.h:36: class EntryValue {
On 2014/09/12 15:21:26, sof wrote:
> Add FINAL

Done.

https://codereview.chromium.org/564963002/diff/20001/Source/core/html/FormDat...
Source/core/html/FormDataList.h:36: class EntryValue {
On 2014/09/12 15:21:26, sof wrote:
> Add ALLOW_ONLY_INLINE_ALLOCATION();

Done.

https://codereview.chromium.org/564963002/diff/20001/Source/core/html/FormDat...
Source/core/html/FormDataList.h:50: 
On 2014/09/12 15:21:26, sof wrote:
> Add 
> 
>   void trace(Visitor*);
> 
> and define it as
> 
>   void FormDataList::EntryValue::trace(Visitor* visitor)
>   {
>       visitor->trace(m_file);
>   }

Done.

https://codereview.chromium.org/564963002/diff/20001/Source/core/html/FormDat...
Source/core/html/FormDataList.h:101: Vector<EntryValue> getAll(const String&
key) const;
On 2014/09/12 15:21:26, sof wrote:
> Vector => WillBeHeapVector

Done.

[jsbell, 2014-09-12 22:48:16]

Added encoding tests and timestamp tests (which found a bug, wheee!)

So far as I can tell, when creating FormData from script we always initialize it
with a UTF-8 encoding, so I did not add test for different encodings, just that
a handful of non-ASCII characters round trip.

Assuming this looks good I'll double-check FF and send out an 'Intent'

[jsbell, 2014-09-12 23:37:39]

Looks like FF hasn't implemented this yet. I'll ping Moz folks and see if they
plan to.

[jsbell, 2014-09-15 18:22:55]

On 2014/09/12 23:37:39, jsbell wrote:
> Looks like FF hasn't implemented this yet. I'll ping Moz folks and see if they
> plan to.

... and no explicit plans. Not even a tracking bug. Oh bother.

[sof, 2014-09-16 05:21:18]

On 2014/09/12 22:48:16, jsbell wrote:
> Added encoding tests and timestamp tests (which found a bug, wheee!)
> 
> So far as I can tell, when creating FormData from script we always initialize
it
> with a UTF-8 encoding, so I did not add test for different encodings, just
that
> a handful of non-ASCII characters round trip.
> 
> Assuming this looks good I'll double-check FF and send out an 'Intent'

Added tests do look good.

[jsbell, 2014-09-16 17:40:45]

jsbell@chromium.org changed reviewers:
+ arv@chromium.org

[jsbell, 2014-09-16 19:18:42]

I added the [Symbol.iterator()], entries(), keys() and values() methods, and
dropped everything behind a RuntimeEnabled flag.

This leaves a few FIXMEs:

* The [Iterable] IDL attribute (which enables [Symbol.iterator]() method) is not
behind the RuntimeEnabled flag. I'm not sure if we can do that w/o codegen
changes - anyone know?

* Two iterator edge cases are not handled: passing a target object into
`iter.next(obj)` and mutations during iteration.

[jsbell, 2014-09-16 19:39:40]

On 2014/09/16 19:18:42, jsbell wrote:
> I added the [Symbol.iterator()], entries(), keys() and values() methods, and
> dropped everything behind a RuntimeEnabled flag.
> 
> This leaves a few FIXMEs:
> 
> * The [Iterable] IDL attribute (which enables [Symbol.iterator]() method) is
not
> behind the RuntimeEnabled flag. I'm not sure if we can do that w/o codegen
> changes - anyone know?
> 
> * Two iterator edge cases are not handled: passing a target object into
> `iter.next(obj)` and mutations during iteration.

Also, I did not yet expose a `size` readonly attribute to script. Trivial, but I
don't know if we'd count that as part of the "MapLikeIterator" package or not.

[arv (Not doing code reviews), 2014-09-17 19:49:38]

https://codereview.chromium.org/564963002/diff/220001/Source/core/html/DOMFor...
File Source/core/html/DOMFormData.cpp (right):

https://codereview.chromium.org/564963002/diff/220001/Source/core/html/DOMFor...
Source/core/html/DOMFormData.cpp:68: // entries being skipped.
That is pretty unfortunate. Can we make sure this fixme is fixed before we
enable this?

https://codereview.chromium.org/564963002/diff/220001/Source/core/html/DOMFor...
Source/core/html/DOMFormData.cpp:101: // FIXME: Implement the "bring your own
payload object" variant of next().
Not sure what this means?

https://codereview.chromium.org/564963002/diff/220001/Source/core/html/DOMFor...
Source/core/html/DOMFormData.cpp:150: void DOMFormData::remove(const String&
name)
Why this extra indirection? Can you just use implementedAs in the IDL?

https://codereview.chromium.org/564963002/diff/220001/Source/core/html/DOMFor...
Source/core/html/DOMFormData.cpp:176: return hasEntry(name);
Maybe use implementedAs in the IDL here too?

https://codereview.chromium.org/564963002/diff/220001/Source/core/html/FormDa...
File Source/core/html/FormDataList.cpp (right):

https://codereview.chromium.org/564963002/diff/220001/Source/core/html/FormDa...
Source/core/html/FormDataList.cpp:58: m_items.remove(i, 2);
I'm not sure what your plan is to fix this to make iteration behave correctly
when items are removed?

[jsbell, 2014-09-18 16:18:59]

Thanks for taking a look.

https://codereview.chromium.org/564963002/diff/220001/Source/core/html/DOMFor...
File Source/core/html/DOMFormData.cpp (right):

https://codereview.chromium.org/564963002/diff/220001/Source/core/html/DOMFor...
Source/core/html/DOMFormData.cpp:68: // entries being skipped.
On 2014/09/17 19:49:38, arv wrote:
> That is pretty unfortunate. Can we make sure this fixme is fixed before we
> enable this?

Agreed. 

I think we need to spec to stabilize, too. DOM APIs should be consistent
(snapshot vs. smarter live iterators)... IIRC there was some W3C bug about that.

https://codereview.chromium.org/564963002/diff/220001/Source/core/html/DOMFor...
Source/core/html/DOMFormData.cpp:101: // FIXME: Implement the "bring your own
payload object" variant of next().
On 2014/09/17 19:49:38, arv wrote:
> Not sure what this means?

Huh... I thought at one point you could call iter.next(obj) passing your own
object, onto which value/done would be applied, to avoid GC overhead. But I'm
not seeing that in the latest ES6 draft. Removed? Never made it?

So... I can remove the FIXME. I copy/pasted this next() overload from elsewhere.


@arv: Is it future-proofing by rejecting a passed argument? Or should it be
removed (here and elsewhere)?

https://codereview.chromium.org/564963002/diff/220001/Source/core/html/DOMFor...
Source/core/html/DOMFormData.cpp:150: void DOMFormData::remove(const String&
name)
On 2014/09/17 19:49:38, arv wrote:
> Why this extra indirection? Can you just use implementedAs in the IDL?

Yeah, this doesn't gain anything. I'll flatten this.

https://codereview.chromium.org/564963002/diff/220001/Source/core/html/DOMFor...
Source/core/html/DOMFormData.cpp:176: return hasEntry(name);
On 2014/09/17 19:49:38, arv wrote:
> Maybe use implementedAs in the IDL here too?

Ditto.

https://codereview.chromium.org/564963002/diff/220001/Source/core/html/FormDa...
File Source/core/html/FormDataList.cpp (right):

https://codereview.chromium.org/564963002/diff/220001/Source/core/html/FormDa...
Source/core/html/FormDataList.cpp:58: m_items.remove(i, 2);
On 2014/09/17 19:49:38, arv wrote:
> I'm not sure what your plan is to fix this to make iteration behave correctly
> when items are removed?

I don't have a firm plan yet. IMHO we should remove the Vector<Item> and use a
Vector<Entry> instead as the definitive storage, which would simplify these new
methods a fair bit, and align more closely with the spec (it would touch the
previously existing code, though, which I was initially trying to avoid). We
could then either follow the model of the ES6 spec and allow 'empty' values in
the list to maintain iteration ordering, or expose the Vector<Entry> itself to
the Iterator object (if WTF Vector iterators behave properly; I haven't checked)

Feedback welcome.

[arv (Not doing code reviews), 2014-09-18 17:33:45]

https://codereview.chromium.org/564963002/diff/220001/Source/core/html/DOMFor...
File Source/core/html/DOMFormData.cpp (right):

https://codereview.chromium.org/564963002/diff/220001/Source/core/html/DOMFor...
Source/core/html/DOMFormData.cpp:101: // FIXME: Implement the "bring your own
payload object" variant of next().
On 2014/09/18 16:18:59, jsbell wrote:
> On 2014/09/17 19:49:38, arv wrote:
> > Not sure what this means?
> 
> Huh... I thought at one point you could call iter.next(obj) passing your own
> object, onto which value/done would be applied, to avoid GC overhead. But I'm
> not seeing that in the latest ES6 draft. Removed? Never made it?
> 
> So... I can remove the FIXME. I copy/pasted this next() overload from
elsewhere.
> 
> 
> @arv: Is it future-proofing by rejecting a passed argument? Or should it be
> removed (here and elsewhere)?

You should not reject an argument, just ignore it. The argument passed to next
is used by generators.

function* f() {
  return yield 1;
}
var g = f();
g.next();  // {value: 1, done: false}
g.next(2);  // {value: 2, done: true}

https://codereview.chromium.org/564963002/diff/220001/Source/core/html/FormDa...
File Source/core/html/FormDataList.cpp (right):

https://codereview.chromium.org/564963002/diff/220001/Source/core/html/FormDa...
Source/core/html/FormDataList.cpp:58: m_items.remove(i, 2);
On 2014/09/18 16:18:59, jsbell wrote:
> On 2014/09/17 19:49:38, arv wrote:
> > I'm not sure what your plan is to fix this to make iteration behave
correctly
> > when items are removed?
> 
> I don't have a firm plan yet. IMHO we should remove the Vector<Item> and use a
> Vector<Entry> instead as the definitive storage, which would simplify these
new
> methods a fair bit, and align more closely with the spec (it would touch the
> previously existing code, though, which I was initially trying to avoid). We
> could then either follow the model of the ES6 spec and allow 'empty' values in
> the list to maintain iteration ordering, or expose the Vector<Entry> itself to
> the Iterator object (if WTF Vector iterators behave properly; I haven't
checked)
> 
> Feedback welcome.

There are two ways to do this.

1. Keep a list if weak refs to you iterators from the form data. When an item is
removed, update all the iterators.

2. The iterator points to the storage (not the form data). When the storage
changes due to rehash (in your case due to removing holes?) create a new storage
object and an object with the removed indexes. Then when the iterator next
method is called, you check if there is a newer storage version, and if so
transition the iterator and make it point to the newer storage. Once there are
no iterators depending on the storage the storage gets gc'ed (ref counting
should work too). This is what V8 Map/Set does and the main reason for this
approach was so that we did not depend on any weak refs. Take a look at

[jsbell, 2014-09-18 23:56:29]

Given that this and URLSearchParams have similar interfaces (they're multimaps)
perhaps we can try and share iterator implementations.

https://codereview.chromium.org/564963002/diff/220001/Source/core/html/DOMFor...
File Source/core/html/DOMFormData.cpp (right):

https://codereview.chromium.org/564963002/diff/220001/Source/core/html/DOMFor...
Source/core/html/DOMFormData.cpp:101: // FIXME: Implement the "bring your own
payload object" variant of next().
On 2014/09/18 17:33:45, arv wrote:
> On 2014/09/18 16:18:59, jsbell wrote:
> > On 2014/09/17 19:49:38, arv wrote:
> > > Not sure what this means?
> > 
> > Huh... I thought at one point you could call iter.next(obj) passing your own
> > object, onto which value/done would be applied, to avoid GC overhead. But
I'm
> > not seeing that in the latest ES6 draft. Removed? Never made it?
> > 
> > So... I can remove the FIXME. I copy/pasted this next() overload from
> elsewhere.
> > 
> > 
> > @arv: Is it future-proofing by rejecting a passed argument? Or should it be
> > removed (here and elsewhere)?
> 
> You should not reject an argument, just ignore it. The argument passed to next
> is used by generators.

Got it. I was looking at the InternalsIterator for guidance rather than the
MIDIPort one. :P

[arv (Not doing code reviews), 2014-09-19 14:22:13]

On 2014/09/18 at 23:56:29, jsbell wrote:
> Given that this and URLSearchParams have similar interfaces (they're
multimaps) perhaps we can try and share iterator implementations.

To me it is not clear what behavior we want any more. Do they need to adjust for
removal like MapIterator or can they just carry on like ArrayIterator?

[jsbell, 2014-10-23 18:15:41]

Quick update - Web IDL now has: 

* IDL syntax: `iterable<value-type>;` or `iterable<key-type, value-type>;`
* ECMAScript binding should produce: keys(), values(), entries(), and
@@iterator()
* Spec prose defines what the list is, and whether it's snapshotted or live
* Iteration is implicitly index-based (even if it's not exposed by index to
script), so no need to adjust for insertions/removals.

(Ignoring the whole "values() breaks the web" issue, for now...)

To ship this behind a flag we need to first support
`[RuntimeEnabled=FormDataNewMethods] iterable<USVString, FormDataValue>;` in the
code generator. Tabling this CL for a while longer.

[jsbell, 2015-01-15 01:29:38]

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

[jsbell, 2015-01-15 01:34:15]

I believe this is ready for a peek again. New functionality should all be behind
the --enable-experimental-web-platform-features flag.

Added jl@ to take a peek at the iterable<> usage/impl, which should be the only
change since the last round of reviews.

[Jens Widell, 2015-01-15 07:35:06]

Iterable stuff looks good to me.

https://codereview.chromium.org/564963002/diff/300001/Source/core/html/DOMFor...
File Source/core/html/DOMFormData.cpp (right):

https://codereview.chromium.org/564963002/diff/300001/Source/core/html/DOMFor...
Source/core/html/DOMFormData.cpp:36: #include "core/dom/Iterator.h"
You don't need to include this now, I think?

https://codereview.chromium.org/564963002/diff/300001/Source/core/html/DOMFor...
Source/core/html/DOMFormData.cpp:75: PairIterable<String,
ScriptValue>::IterationSource::trace(visitor);
The convention is to make this a tail call, said haraken@ in
https://codereview.chromium.org/848673002.

https://codereview.chromium.org/564963002/diff/300001/Source/core/html/FormDa...
File Source/core/html/FormData.idl (right):

https://codereview.chromium.org/564963002/diff/300001/Source/core/html/FormDa...
Source/core/html/FormData.idl:33: // FIXME: Replace 'any' with union type, per
spec.
Does the current union support not handle this?

[jsbell, 2015-01-15 18:57:13]

https://codereview.chromium.org/564963002/diff/300001/Source/core/html/DOMFor...
File Source/core/html/DOMFormData.cpp (right):

https://codereview.chromium.org/564963002/diff/300001/Source/core/html/DOMFor...
Source/core/html/DOMFormData.cpp:36: #include "core/dom/Iterator.h"
On 2015/01/15 07:35:06, Jens Widell wrote:
> You don't need to include this now, I think?

Done.

https://codereview.chromium.org/564963002/diff/300001/Source/core/html/DOMFor...
Source/core/html/DOMFormData.cpp:75: PairIterable<String,
ScriptValue>::IterationSource::trace(visitor);
On 2015/01/15 07:35:06, Jens Widell wrote:
> The convention is to make this a tail call, said haraken@ in
> https://codereview.chromium.org/848673002.

Done.

https://codereview.chromium.org/564963002/diff/300001/Source/core/html/FormDa...
File Source/core/html/FormData.idl (right):

https://codereview.chromium.org/564963002/diff/300001/Source/core/html/FormDa...
Source/core/html/FormData.idl:33: // FIXME: Replace 'any' with union type, per
spec.
On 2015/01/15 07:35:06, Jens Widell wrote:
> Does the current union support not handle this?

get() can... ah! It looks like we can return Vector<UnionType> from methods now.
So getAll() can unionize as well... oh, and the Iterable<> templates just work
too. Huzzah!

[Jens Widell, 2015-01-15 19:20:25]

https://codereview.chromium.org/564963002/diff/300001/Source/core/html/FormDa...
File Source/core/html/FormData.idl (right):

https://codereview.chromium.org/564963002/diff/300001/Source/core/html/FormDa...
Source/core/html/FormData.idl:33: // FIXME: Replace 'any' with union type, per
spec.
On 2015/01/15 18:57:13, jsbell wrote:
> On 2015/01/15 07:35:06, Jens Widell wrote:
> > Does the current union support not handle this?
> 
> get() can... ah! It looks like we can return Vector<UnionType> from methods
now.
> So getAll() can unionize as well... oh, and the Iterable<> templates just work
> too. Huzzah!

Sweet. :-)

https://codereview.chromium.org/564963002/diff/340001/Source/core/html/FormDa...
File Source/core/html/FormData.idl (right):

https://codereview.chromium.org/564963002/diff/340001/Source/core/html/FormDa...
Source/core/html/FormData.idl:47: [RuntimeEnabled=FormDataNewMethods,
ImplementedAs=hasEntry, ] boolean has(USVString name);
The trailing ", " in the extended attributes list is a bit unusual.

[jsbell, 2015-01-15 19:29:47]

https://codereview.chromium.org/564963002/diff/340001/Source/core/html/FormDa...
File Source/core/html/FormData.idl (right):

https://codereview.chromium.org/564963002/diff/340001/Source/core/html/FormDa...
Source/core/html/FormData.idl:47: [RuntimeEnabled=FormDataNewMethods,
ImplementedAs=hasEntry, ] boolean has(USVString name);
On 2015/01/15 19:20:25, Jens Widell wrote:
> The trailing ", " in the extended attributes list is a bit unusual. 

Done.

[Jens Widell, 2015-01-15 20:03:28]

LGTM

[jsbell, 2015-01-22 00:37:31]

tyoshino@, sof@, arv@ - any comments before I sent this off for OWNERS review?

[sof, 2015-01-22 07:33:40]

lgtm

https://codereview.chromium.org/564963002/diff/360001/Source/core/html/DOMFor...
File Source/core/html/DOMFormData.h (right):

https://codereview.chromium.org/564963002/diff/360001/Source/core/html/DOMFor...
Source/core/html/DOMFormData.h:51: class Iterator;
Not used in what follows. IterationSource is, but perhaps we can assume that
Iterable.h brings all blink::Iter* types into scope?

https://codereview.chromium.org/564963002/diff/360001/Source/core/html/FormDa...
File Source/core/html/FormDataList.cpp (right):

https://codereview.chromium.org/564963002/diff/360001/Source/core/html/FormDa...
Source/core/html/FormDataList.cpp:307: } // namespace
nit: namespace blink

https://codereview.chromium.org/564963002/diff/360001/Source/core/html/FormDa...
File Source/core/html/FormDataList.h (right):

https://codereview.chromium.org/564963002/diff/360001/Source/core/html/FormDa...
Source/core/html/FormDataList.h:146:
WTF_ALLOW_MOVE_AND_INIT_WITH_MEM_FUNCTIONS(blink::FormDataList::Item);
As Entry is used in a heap vector, the same trait is also needed for it.

Actually, we only need WTF_ALLOW_INIT_WITH_MEM_FUNCTIONS() here, somight as well
have both use that instead.

[tyoshino (SeeGerritForStatus), 2015-01-22 12:53:10]

lgtm!

https://codereview.chromium.org/564963002/diff/360001/Source/core/html/DOMFor...
File Source/core/html/DOMFormData.cpp (right):

https://codereview.chromium.org/564963002/diff/360001/Source/core/html/DOMFor...
Source/core/html/DOMFormData.cpp:48: virtual bool next(ScriptState* scriptState,
String& key, FormDataEntryValue& value, ExceptionState& exceptionState) override
can omit virtual?

https://codereview.chromium.org/564963002/diff/360001/Source/core/html/DOMFor...
Source/core/html/DOMFormData.cpp:64: virtual void trace(Visitor* visitor)
override?

[jsbell, 2015-01-22 19:05:39]

Thanks, all!

https://codereview.chromium.org/564963002/diff/360001/Source/core/html/DOMFor...
File Source/core/html/DOMFormData.cpp (right):

https://codereview.chromium.org/564963002/diff/360001/Source/core/html/DOMFor...
Source/core/html/DOMFormData.cpp:48: virtual bool next(ScriptState* scriptState,
String& key, FormDataEntryValue& value, ExceptionState& exceptionState) override
On 2015/01/22 12:53:10, tyoshino wrote:
> can omit virtual?

Done.

https://codereview.chromium.org/564963002/diff/360001/Source/core/html/DOMFor...
Source/core/html/DOMFormData.cpp:64: virtual void trace(Visitor* visitor)
On 2015/01/22 12:53:10, tyoshino wrote:
> override?

Done.

https://codereview.chromium.org/564963002/diff/360001/Source/core/html/DOMFor...
File Source/core/html/DOMFormData.h (right):

https://codereview.chromium.org/564963002/diff/360001/Source/core/html/DOMFor...
Source/core/html/DOMFormData.h:51: class Iterator;
On 2015/01/22 07:33:40, sof wrote:
> Not used in what follows. IterationSource is, but perhaps we can assume that
> Iterable.h brings all blink::Iter* types into scope?

Done.

https://codereview.chromium.org/564963002/diff/360001/Source/core/html/FormDa...
File Source/core/html/FormDataList.cpp (right):

https://codereview.chromium.org/564963002/diff/360001/Source/core/html/FormDa...
Source/core/html/FormDataList.cpp:307: } // namespace
On 2015/01/22 07:33:40, sof wrote:
> nit: namespace blink

Done.

https://codereview.chromium.org/564963002/diff/360001/Source/core/html/FormDa...
File Source/core/html/FormDataList.h (right):

https://codereview.chromium.org/564963002/diff/360001/Source/core/html/FormDa...
Source/core/html/FormDataList.h:146:
WTF_ALLOW_MOVE_AND_INIT_WITH_MEM_FUNCTIONS(blink::FormDataList::Item);
On 2015/01/22 07:33:40, sof wrote:
> As Entry is used in a heap vector, the same trait is also needed for it.
> 
> Actually, we only need WTF_ALLOW_INIT_WITH_MEM_FUNCTIONS() here, somight as
well
> have both use that instead.

Done.

[jsbell, 2015-01-22 19:11:01]

jsbell@chromium.org changed reviewers:
+ dglazkov@chromium.org

[jsbell, 2015-01-22 19:11:01]

dglazkov: OWNERS lgtm for Source/platform ?

[dglazkov, 2015-01-22 19:12:28]

lgtm

[jsbell, 2015-01-22 19:41:26]

The CQ bit was checked by jsbell@chromium.org

[commit-bot: I haz the power, 2015-01-22 19:42:20]

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

[commit-bot: I haz the power, 2015-01-22 21:14:55]

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

[commit-bot: I haz the power, 2015-01-22 21:14:56]

Try jobs failed on following builders:
  linux_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/linux_blink_rel/builds/4...)

[jsbell, 2015-01-22 22:06:49]

The CQ bit was checked by jsbell@chromium.org

[commit-bot: I haz the power, 2015-01-22 22:07:15]

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

[commit-bot: I haz the power, 2015-01-23 00:12:45]

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

[commit-bot: I haz the power, 2015-01-23 00:12:46]

Exceeded time limit waiting for builds to trigger.

[jsbell, 2015-01-23 05:52:12]

The CQ bit was checked by jsbell@chromium.org

[commit-bot: I haz the power, 2015-01-23 05:52:31]

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

[commit-bot: I haz the power, 2015-01-23 06:50:38]

Committed patchset #21 (id:400001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=188863