Bug 39429

Created attachment 56601 [details] Patch

Comment on attachment 56601 [details] Patch There should also be a "constructor" on window. FWIW, I added and removed DOMStringList before, because it was unused. See r44943.

Created attachment 56697 [details] Patch

Thanks a lot for the pointer, Alexey! Sorry about forgetting the constructor. Fixed now. I noticed a couple differences between our code: 1) Custom bindings: Thankfully, it looks as though these are no longer required for either V8 or JSC. 2) StaticStringList: I could refactor my code to match what was originally there, but I guess I'm not sure I see a point at the moment. My (yet to be uploaded) IndexedDB patch simply builds up the list on demand (as the actual objectStore and index data is stored in objects, not simply as a list of strings) with the add method. I believe the standard procedure in cases like this is to keep it simple for now and let someone refactor later if something more complicated is necessary in the future. 3) DOMStringList.contains(): I double checked, but this is not in the latest version of the spec, so I have not included it. Please take another look? Given that we're in different time zones, if this patch is close, an "r+ with items to address before landing" would be greatly appreciated if nothing's too far off. Thanks!

Created attachment 56699 [details] Patch

Comment on attachment 56699 [details] Patch + 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?

(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. :-)

Created attachment 56731 [details] Patch

Comment on attachment 56731 [details] Patch +bool DOMStringList::contains(const String& str) const There's no need to abbreviate. +// FIXME: Some consumers of this class may benefit from lazily fetching items rather +// than creating the list statically as is currently the only option. If so, +// you may want to take a look at the original implementation which was removed +// in r44943 and make this class an interface in a similar way. I don't think there was anything particularly high-tech in r44943 to quote it, it's just as easy to write from scratch :). The FIXME should be shorter. + ] DOMStringList { + readonly attribute unsigned long length; + DOMString item(in [IsIndex] unsigned long index); + }; Please add contains(). + * 3. Neither the name of Apple Computer, Inc. ("Apple") nor the names of There is no "Apple Computer" any more, and we use a two-clause license - please see <http://webkit.org/coding/bsd-license.html>. r=me with these fixes.

(In reply to comment #9) > (From update of attachment 56731 [details]) > +bool DOMStringList::contains(const String& str) const > > There's no need to abbreviate. > > +// FIXME: Some consumers of this class may benefit from lazily fetching items rather > +// than creating the list statically as is currently the only option. If so, > +// you may want to take a look at the original implementation which was removed > +// in r44943 and make this class an interface in a similar way. > > I don't think there was anything particularly high-tech in r44943 to quote it, it's just as easy to write from scratch :). The FIXME should be shorter. done > + ] DOMStringList { > + readonly attribute unsigned long length; > + DOMString item(in [IsIndex] unsigned long index); > + }; > > Please add contains(). Oops! > + * 3. Neither the name of Apple Computer, Inc. ("Apple") nor the names of > > There is no "Apple Computer" any more, and we use a two-clause license - please see <http://webkit.org/coding/bsd-license.html>. Changed...but is there anything wrong with using a 3 clause license or using the ones with apple computer? Because I just copied this from another file. And it seems as though there's a lot with these mistakes. And, as far as I can tell, every mention (when submitting a patch and what I remember of the contributors agreement) only mentions "BSD License" not a specific one or one with a particular number of clauses.

Committed r60067: <http://trac.webkit.org/changeset/60067>

http://trac.webkit.org/changeset/60067 might have broken Qt Linux ARMv7 Release

I don't think that three-clause is disallowed, it just seemed that you intended to use the most common license form, so I suggested taking the one from webkit.org. We have bug 37076 and bug 28072 tracking various aspects of cleaning up the situation.