[webkit-reviews] review denied: [Bug 39429] Add DOMStringList idl, needed for IndexedDB and for HTML5 drag & drop : [Attachment 56699] Patch

[bugzilla-daemon at webkit.org]

Alexey Proskuryakov <ap at webkit.org> has denied Jeremy Orlow
<jorlow at chromium.org>'s request for review:
Bug 39429: Add DOMStringList idl, needed for IndexedDB and for HTML5 drag &
drop
https://bugs.webkit.org/show_bug.cgi?id=39429

Attachment 56699: Patch
https://bugs.webkit.org/attachment.cgi?id=56699&action=review

------- Additional Comments from Alexey Proskuryakov <ap at webkit.org>
+    void append(String string) { m_strings.append(string); }

"const String&"!

+    return 1 if $type eq "CSSStyleDeclaration" or $type eq "MediaList" or
$type eq "CSSVariablesDeclaration" or $type eq "DOMStringList";

This always makes me sad - why do we have IDLs if we encode knowledge about
specific classes into code generator. Not something to fix in this patch, of
course.

+	 No new tests since nothing uses this.	I'm happy to land it at the
same time as
+	 an IndexedDB change that uses this and adds a test, but IndexedDB is
behind
+	 a flag on all platforms besides chromium.  Another option is to put
this
+	 behind the INDEXED_DATABASE flag until there's another consumer of it.


I think that ChangeLog doesn't need a discussion of options you didn't take.

2) StaticStringList:

OK. I'd add a comment in DOMStringList.h explaining that the current
implementation only accommodates the case when all the strings are known in
advance. It could go right after "Implements the IDL" comment, which
conveniently lacks an ending period - so you need to fix that line anyway!

3) DOMStringList.contains():

I see it in <http://www.w3.org/TR/DOM-Level-3-Core/core.html>. What's the newer
version? The one you reference in ChangeLog is older, not newer.

We might actually need a better implementation of contains() than I had.

I'd prefer to have a quick look at the fixed code. Just curious - what time
zone are you in?