[webkit-reviews] review requested: [Bug 82888] Consider replacing return type of Clipboard::types() from ListHashSet<String> to Vector<String> : [Attachment 186932] updated_patch

[bugzilla-daemon at webkit.org]

Vineet Chaudhary (vineetc) <rgf748 at motorola.com> has asked  for review:
Bug 82888: Consider replacing return type of Clipboard::types() from
ListHashSet<String> to Vector<String>
https://bugs.webkit.org/show_bug.cgi?id=82888

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

------- Additional Comments from Vineet Chaudhary (vineetc)
<rgf748 at motorola.com>
(In reply to comment #26)
> (From update of attachment 160053 [details])
> View in context:
https://bugs.webkit.org/attachment.cgi?id=160053&action=review
> 
> I commented about ASCIILiteral() only for Mac but that applies on the other
platforms.

I am trying using ASCIILiteral() for other platforms too,
but haven't checked for chrome/win build.

> > Source/WebCore/bindings/v8/custom/V8ClipboardCustom.cpp:60
> > -	 HashSet<String>::const_iterator end = types.end();
> > +	 Vector<String>::const_iterator end = types.end();
> >	 int index = 0;
> > -	 for (HashSet<String>::const_iterator it = types.begin(); it != end;
++it, ++index)
> > +	 for (Vector<String>::const_iterator it = types.begin(); it != end;
++it, ++index)
> >	     result->Set(v8Integer(index, info.GetIsolate()), v8String(*it,
info.GetIsolate()));
> 
> I have the feeling this could be made much clearer with:
> for (size_t i = 0; i < types.size() ++i) {
>     ...

Thanks for suggestion..
but after landing this patch anyways these custom bindings will be removed
soon.

> 
> > Source/WebCore/platform/mac/ClipboardMac.mm:127
> > +	     resultTypes.append("text/plain");
> 
> This should use ASCIILiteral().
> 
> > Source/WebCore/platform/mac/ClipboardMac.mm:131
> > -	     resultTypes.add("text/uri-list");
> > +	     resultTypes.append("text/uri-list");
> 
> ditto.
> 
> > Source/WebCore/platform/mac/ClipboardMac.mm:144
> > -		 resultTypes.add("text/uri-list");
> > -		 resultTypes.add("Files");
> > +		 resultTypes.append("text/uri-list");
> > +		 resultTypes.append("Files");
> 
> ditto.
Done.