[Webkit-unassigned] [Bug 39429] Add DOMStringList idl, needed for IndexedDB and for HTML5 drag & drop

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=39429





--- Comment #7 from Jeremy Orlow <jorlow at chromium.org>  2010-05-21 09:53:58 PST ---
(In reply to comment #6)
> (From update of attachment 56699 [details])
> +    void append(String string) { m_strings.append(string); }
> 
> "const String&"!

Oops.  Sorry!

> +        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.

This was more for the reviewer.  I'm happy to take it out though.

> 
> 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!

I don't understand why you want me to add a comment.  As far as I can tell, this code is quite straightforward and I can't think of any potential land mines that might trip up a typical WebKit developer.


> 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.

Oops, I was looking at an old copy of the spec apparently!  Good catch.

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

What do you mean by this?

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

I'm in London these days.  Np having another look.  I just know that sometimes people r- even for tiny nits (which these aren't) so I added that in there.  :-)

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.