Bug 82888 – Consider replacing return type of Clipboard::types() from ListHashSet<String> to Vector<String>

• Component: HTML Editing
• Status: REOPENED

[Vineet Chaudhary (vineetc)]

As part of removing custom bindings of types Array Clipboard.idl also needs to have modified to have sequence<String>.

But according to current implementation of the Clipboard::types() it returns HashSet<String>.
In order to remove the custom bindings it should of type Vector<String>.

Please let me know If you have any suggestions/thoughts on this.

[Vineet Chaudhary (vineetc)]

Created attachment 135087 [details]
proposed patch

[Kentaro Hara]

Comment on attachment 135087 [details]
proposed patch

View in context: https://bugs.webkit.org/attachment.cgi?id=135087&action=review

> Source/WebCore/ChangeLog:14
> +        * bindings/js/JSClipboardCustom.cpp: Replace data type from HashSet<> to Vector<>.

Why do you think that JSClipboardCustom.cpp had been using HashSet<> instead of Vector<>? If there is any performance concern, we need to confirm it. If no strong reason, let me r+ it.

[Vineet Chaudhary (vineetc)]

Created attachment 135158 [details]
patch

reattaching patch to get the ews result.

[Vineet Chaudhary (vineetc)]

(In reply to comment #2)
> (From update of attachment 135087 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=135087&action=review
> 
> > Source/WebCore/ChangeLog:14
> > +        * bindings/js/JSClipboardCustom.cpp: Replace data type from HashSet<> to Vector<>.
> 
> Why do you think that JSClipboardCustom.cpp had been using HashSet<> instead of Vector<>? If there is any performance concern, we need to confirm it. If no strong reason, let me r+ it.

I dont think HashSet was introduced (http://trac.webkit.org/changeset/16479/) for the performance concerns here but yes I agree In general HashSet are quite faster than Vector in some cases.
Please let me know I should add some performance test?

[Alexey Proskuryakov]

> http://trac.webkit.org/changeset/16479/

Please author and reviewer of a patch when discussing it. They are the ones most likely to have insight.

[Kentaro Hara]

acarlsson, darin: Do you have any concern (in terms of performance or something) to change HashSet<> to Vector<>? We can clean up bindings code by the change.

[Vineet Chaudhary (vineetc)]

Comment on attachment 135158 [details]
patch

Setting review flag to get comments. May be I have to upload patch again as buildbot was not able to apply patch after review comments.

[Vineet Chaudhary (vineetc)]

Hi Anders Carlsson/Darin 
Can you please provide your feedback on this patch.  Thanks.

[Vineet Chaudhary (vineetc)]

Hi Darin 
Can you please provide your feedback on this patch.
Please let me know if some more inputs required.

[Kentaro Hara]

vineetc: Pinging in IRC might be another option to catch him in his time zone.

[Darin Adler]

I suggest posting a new patch that applies so the EWS can process it.

[Eric Seidel (no email)]

Comment on attachment 135158 [details]
patch

How do we avoid duplicates in the list?  Or are duplicates OK?  We need a test which shows one or the other.

[Vineet Chaudhary (vineetc)]

Created attachment 144017 [details]
Patch

[Vineet Chaudhary (vineetc)]

(In reply to comment #12)
> (From update of attachment 135158 [details])
> How do we avoid duplicates in the list?  Or are duplicates OK?  We need a test which shows one or the other.

I checked the code actually types() are readonly to JS and hard-codeded in platform specific files. eg. in ClipboardGtk::types() every time it will return new vector with below values "text/plain", "Text" and "text" so it shouldn't contain duplicates.

[Eric Seidel (no email)]

Comment on attachment 144017 [details]
Patch

I think the Mac port could easily end up with duplicates here, which could be confusing.   But I guess we'll deal with those when they come up.

[WebKit Review Bot]

Comment on attachment 144017 [details]
Patch

Rejecting attachment 144017 [details] from commit-queue.

Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 2

Last 500 characters of output:
1 line).
patching file Source/WebCore/platform/qt/ClipboardQt.h
patching file Source/WebCore/platform/win/ClipboardWin.cpp
patching file Source/WebCore/platform/win/ClipboardWin.h
patching file Source/WebCore/platform/wx/ClipboardWx.cpp
Hunk #1 succeeded at 72 (offset 2 lines).
patching file Source/WebCore/platform/wx/ClipboardWx.h

Failed to run "[u'/mnt/git/webkit-commit-queue/Tools/Scripts/svn-apply', u'--force', u'--reviewer', u'Eric Seidel']" exit_code: 1 cwd: /mnt/git/webkit-commit-queue/

Full output: http://queues.webkit.org/results/13571393

[Vineet Chaudhary (vineetc)]

(In reply to comment #15)
> (From update of attachment 144017 [details])
> I think the Mac port could easily end up with duplicates here, which could be confusing.   But I guess we'll deal with those when they come up.

Thanks Eric, I will rebased patch again for review.

[Vineet Chaudhary (vineetc)]

Created attachment 160053 [details]
updated_patch

Attaching rebased patch.

[Kentaro Hara]

Comment on attachment 160053 [details]
updated_patch

No explicit concerns about this change for a long time in this thread... So it looks OK to land it now.

[WebKit Review Bot]

Comment on attachment 160053 [details]
updated_patch

Clearing flags on attachment: 160053

Committed r126383: <http://trac.webkit.org/changeset/126383>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Kentaro Hara]

Reverted r126383 for reason:

Chromium win build crash

Committed r126390: <http://trac.webkit.org/changeset/126390>

[Kentaro Hara]

This crashes Chromium/Win build. I rolled out the patch.
http://build.webkit.org/builders/Chromium%20Win%20Release/builds/47706/steps/compile-webkit/logs/stdio

vineetc: Would you take a look?

[Vineet Chaudhary (vineetc)]

(In reply to comment #23)
> This crashes Chromium/Win build. I rolled out the patch.
> http://build.webkit.org/builders/Chromium%20Win%20Release/builds/47706/steps/compile-webkit/logs/stdio
> 
> vineetc: Would you take a look?

Sorry for late reply!! Actually I don't have WIN machine so I couldn't check issue, but looking at the build error 

DragDataChromium.cpp(119): error C2664: 'bool WTF::Vector<T>::contains<const char[]>(U (&)) const' : cannot convert parameter 1 from 'const char []' to 'const char (&)[1]'
with
[
    T=WTF::String,
    U=const char []
]
Reason: cannot convert from 'const char []' to 'const char [1]'
There is no context in which this conversion is possible

Still I couldn't figure out how come this builds on Linux/Chrome and not Win/Chrome?
I checked WTFString.h and Vector.h but nothing found anything compiler or platform specific code there.

One quick solution could be passing String object than char array
Can this be valid solution? like

> Source/WebCore/platform/chromium/ChromiumDataObject.cpp:142
> -        results.append(mimeTypeFiles);
  +        results.append(String(mimeTypeFiles));

[Kentaro Hara]

(In reply to comment #24)
> DragDataChromium.cpp(119): error C2664: 'bool WTF::Vector<T>::contains<const char[]>(U (&)) const' : cannot convert parameter 1 from 'const char []' to 'const char (&)[1]'
> Reason: cannot convert from 'const char []' to 'const char [1]'
> 
> Still I couldn't figure out how come this builds on Linux/Chrome and not Win/Chrome?
> I checked WTFString.h and Vector.h but nothing found anything compiler or platform specific code there.

I couldn't figure out either. Another observation is that HashSet.contains() didn't cause this problem but Vector.contains() caused this problem.

> One quick solution could be passing String object than char array
> Can this be valid solution? like
> 
> > Source/WebCore/platform/chromium/ChromiumDataObject.cpp:142
> > -        results.append(mimeTypeFiles);
>   +        results.append(String(mimeTypeFiles));

It might fix the error, but we might want to avoid it if possible...

[Benjamin Poulain]

Comment on attachment 160053 [details]
updated_patch

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.

> Source/WebCore/ChangeLog:11
> +        No new tests. Exixting test should pass with this change as no behavoural changes.

Exixting -> Existing

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

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

[Vineet Chaudhary (vineetc)]

Reopening as patch was rolled out.

[Vineet Chaudhary (vineetc)]

Created attachment 186932 [details]
updated_patch

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

[Kentaro Hara]

Comment on attachment 186932 [details]
updated_patch

LGTM

[Build Bot]

Comment on attachment 186932 [details]
updated_patch

Attachment 186932 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/16416013

[Vineet Chaudhary (vineetc)]

Comment on attachment 186932 [details]
updated_patch

I need to watch cr/win build to pass this patch.

[WebKit Review Bot]

Comment on attachment 186932 [details]
updated_patch

Clearing flags on attachment: 186932

Committed r142155: <http://trac.webkit.org/changeset/142155>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Gavin Peters]

Reverted r142155 for reason:

cr/win build broke.

Committed r142170: <http://trac.webkit.org/changeset/142170>

[Gavin Peters]

Sorry, it happened again. See http://build.chromium.org/p/chromium.webkit/builders/Win%20Builder/builds/34703/steps/compile/logs/stdio

[Vineet Chaudhary (vineetc)]

(In reply to comment #35)
> Sorry, it happened again. See http://build.chromium.org/p/chromium.webkit/builders/Win%20Builder/builds/34703/steps/compile/logs/stdio

No I should be sorry for this!! Unfortunately I don't have cr/win port to check the issue :(.

[Daniel Cheng]

(In reply to comment #36)
> (In reply to comment #35)
> > Sorry, it happened again. See http://build.chromium.org/p/chromium.webkit/builders/Win%20Builder/builds/34703/steps/compile/logs/stdio
> 
> No I should be sorry for this!! Unfortunately I don't have cr/win port to check the issue :(.

Would using ASCIILiteral() in the call to contains() in DragDataChromium.cpp help? All the contains() calls there are actually just char[] constants anyway, so I think this would be an appropriate use of ASCIILiteral.

[Vineet Chaudhary (vineetc)]

(In reply to comment #37)
> (In reply to comment #36)
> > (In reply to comment #35)
> > > Sorry, it happened again. See http://build.chromium.org/p/chromium.webkit/builders/Win%20Builder/builds/34703/steps/compile/logs/stdio
> > 
> > No I should be sorry for this!! Unfortunately I don't have cr/win port to check the issue :(.
> 
> Would using ASCIILiteral() in the call to contains() in DragDataChromium.cpp help? All the contains() calls there are actually just char[] constants anyway, so I think this would be an appropriate use of ASCIILiteral.

No using ASCIILiteral() wouldn't help atleast on linux/cr it gives another error in Vector::find() //find called from contains() because of no overloaded == operator in WTHString.h for ASCIILiteral.

"no known conversion for argument 2 from ‘const WTF::ASCIILiteral’ to ‘const char*’"

[Darin Adler]

The problem comes because Ben Poulain added code to the WTF String class that assumes that char arrays are string literals with lengths that can be determined at compile time, whereas char pointers require a strlen at runtime. The definition of mimeTypeTextHTML is a char array with an undefined length and one that is not a string literal, so it violates this assumption.

A simple fix would be to change this:

extern const char mimeTypeTextHTML[];

in ClipboardMimeTypes.h to this:

extern const char* const mimeTypeTextHTML;

And update the .cpp file to match.

Another fix would be to put the mimeTypeTextHTML value into a local variable of type const char* in the type functions its used in, inside DragDataChromium.cpp.

[Darin Adler]

(In reply to comment #39)
> the type functions

I meant the *two* functions

[Vineet Chaudhary (vineetc)]

Created attachment 187372 [details]
patch

[Vineet Chaudhary (vineetc)]

(In reply to comment #39)
Thanks Darin for suggestions:

> The problem comes because Ben Poulain added code to the WTF String class that assumes that char arrays are string literals with lengths that can be determined at compile time, whereas char pointers require a strlen at runtime. The definition of mimeTypeTextHTML is a char array with an undefined length and one that is not a string literal, so it violates this assumption.
> 
> A simple fix would be to change this:
> 
> extern const char mimeTypeTextHTML[];
> 
> in ClipboardMimeTypes.h to this:
> 
> extern const char* const mimeTypeTextHTML;
> 
> And update the .cpp file to match.

I not sure if this needs to change files in chromium port too.

> Another fix would be to put the mimeTypeTextHTML value into a local variable of type const char* in the type functions its used in, inside DragDataChromium.cpp.

I have attached patch with this approach, It builds for cr-linux.
Asking for help if someone has access to cr-win port needs to verify patch.
I understand commit|fail|rollout is bad path  again :(.

BTW can I build/test local patches on bots? I tried with "webkit-patch cr-win-ews" but it doesn't worked.

[Eric Seidel (no email)]

Comment on attachment 144017 [details]
Patch

Cleared Eric Seidel's review+ from obsolete attachment 144017 [details] so that this bug does not appear in http://webkit.org/pending-commit.

[Daniel Cheng]

FYI, I noticed a bug in the custom bindings. We're returning a JS null if the types array is empty; I think removing the custom bindings should fix this issue (but it would be good to confirm). When do you anticipate you'd be able to get this change into the tree?

[Vineet Chaudhary (vineetc)]

(In reply to comment #44)
> FYI, I noticed a bug in the custom bindings. We're returning a JS null if the types array is empty; I think removing the custom bindings should fix this issue (but it would be good to confirm). When do you anticipate you'd be able to get this change into the tree?

Hi Daniel,
earlier patches for this bugs failed to build on the cr/windows though I have attached probable fix I just wanted to sure if this works on cr/win but I don't have build for that. 
Could/Can you please take this patch forward.. thanks

[Daniel Cheng]

Created attachment 192817 [details]
Patch

[Daniel Cheng]

Per Vineet's request, I've uploaded a patch that builds on Chrome.

I've opted not to alias the constants or change their definition; instead, I merely wrap it in ASCIILiteral(). Unfortunately, using ASCIILiteral results in ambiguous overload resolution for == (since it could convert to either AtomicString or String), so I explicitly coerce it to String() as well.

[Eric Seidel (no email)]

Comment on attachment 192817 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=192817&action=review

> Source/WebCore/platform/chromium/DragDataChromium.cpp:60
> +    return m_platformDragData->types().contains(String(ASCIILiteral(mimeTypeTextURIList)))

Does String not have an implicit constructor from ASCIILiteral?

[Daniel Cheng]

(In reply to comment #48)
> (From update of attachment 192817 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=192817&action=review
> 
> > Source/WebCore/platform/chromium/DragDataChromium.cpp:60
> > +    return m_platformDragData->types().contains(String(ASCIILiteral(mimeTypeTextURIList)))
> 
> Does String not have an implicit constructor from ASCIILiteral?

It does. However, AtomicString also has an implicit constructor from ASCIILiteral. This is a problem because that causes overload resolution for operator== (used by Vector<T>::contains) becomes ambiguous as a result.

[Eric Seidel (no email)]

Comment on attachment 192817 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=192817&action=review

> Source/WebCore/ChangeLog:9
> +        Vector<String> than ListHashSet<String>

Can you help me undestand what's differnet between these two types? If I call listHashSet.append('foo') twice, will I end up wiht one 'foo' or two?

> Source/WebCore/ChangeLog:11
> +        No new tests. Existing test should pass with this change as no behavoural changes.

behavioural

[Daniel Cheng]

Created attachment 192984 [details]
Patch

[Daniel Cheng]

Comment on attachment 192817 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=192817&action=review

>> Source/WebCore/ChangeLog:9
>> +        Vector<String> than ListHashSet<String>
> 
> Can you help me undestand what's differnet between these two types? If I call listHashSet.append('foo') twice, will I end up wiht one 'foo' or two?

You will end up with one 'foo'. I don't think this is likely to be a problem in practice (most platforms populated the types array with a set list of types anyway).
While duplicates would be undesirable, this is not likely to cause problems even if it does occur, since usage in JS is typically an indexOf() check.

>> Source/WebCore/ChangeLog:11
>> +        No new tests. Existing test should pass with this change as no behavoural changes.
> 
> behavioural

Fixed.

[Eric Seidel (no email)]

Comment on attachment 192817 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=192817&action=review

>>> Source/WebCore/ChangeLog:9
>>> +        Vector<String> than ListHashSet<String>
>> 
>> Can you help me undestand what's differnet between these two types? If I call listHashSet.append('foo') twice, will I end up wiht one 'foo' or two?
> 
> You will end up with one 'foo'. I don't think this is likely to be a problem in practice (most platforms populated the types array with a set list of types anyway).
> While duplicates would be undesirable, this is not likely to cause problems even if it does occur, since usage in JS is typically an indexOf() check.

To be clear, this is then a web-exposed change, even if a minor one.  Do we have any way to test this, or is it just that code which previously was accidentally ending up with one entry will now end up with two?  I assume we expose an Array to JS, not a dictionary/set.

[Daniel Cheng]

(In reply to comment #53)
> (From update of attachment 192817 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=192817&action=review
> 
> >>> Source/WebCore/ChangeLog:9
> >>> +        Vector<String> than ListHashSet<String>
> >> 
> >> Can you help me undestand what's differnet between these two types? If I call listHashSet.append('foo') twice, will I end up wiht one 'foo' or two?
> > 
> > You will end up with one 'foo'. I don't think this is likely to be a problem in practice (most platforms populated the types array with a set list of types anyway).
> > While duplicates would be undesirable, this is not likely to cause problems even if it does occur, since usage in JS is typically an indexOf() check.
> 
> To be clear, this is then a web-exposed change, even if a minor one.  Do we have any way to test this,

Not cleanly. How types() gets populated is largely dependent on platform specific-code in Source/WebCore/platform/*/Clipboard*.cpp, so it'd be very hard (if not impossible in the current state) to force duplicate entries in the return value from types().

> or is it just that code which previously was accidentally ending up with one entry will now end up with two? 

Yes.

> I assume we expose an Array to JS, not a dictionary/set.

Yes.

[Eric Seidel (no email)]

We could make this change w/o changing behavior if we added a step to the bindings layer which uniqued these before returning them.  Or at least ASSERTed they were unique.

[Daniel Cheng]

(In reply to comment #55)
> We could make this change w/o changing behavior if we added a step to the bindings layer which uniqued these before returning them.  Or at least ASSERTed they were unique.

We could do that. But the whole point of this patch is to eventually remove the custom bindings. And it will still be just as hard to test then.

[Eric Seidel (no email)]

(In reply to comment #56)
> (In reply to comment #55)
> > We could make this change w/o changing behavior if we added a step to the bindings layer which uniqued these before returning them.  Or at least ASSERTed they were unique.
> 
> We could do that. But the whole point of this patch is to eventually remove the custom bindings. And it will still be just as hard to test then.

The implementation code does not have to be dictacted by the bindings objects here.  It appears Clipboard is the bindings object as well as the implementation code.  Often in WebKit they are separate.  It appears the implementation code may want to use ListHashSet, but Clipboard might want to use Vector.

We're just suffering the curse of custom bindings here.  The modern pattern is to have "dom implementation" objects, like CSSStyleRule, which just implement the JS API, and then internally always use whatever non-DOM-api objects we like.  Clipboard ended up as both the DOM API object as well as the platform object, which is really binding our hands here.  It's trying to serve two masters.  1 the DOM api, and 2 the platform APIs.  We'd rather have:

[auto-generated bindings]
[DOM API object "Clipboard"]
[platform abstraction layer]

Then we don't care what types the platform abstraction layer uses, and the DOM API c++ object can assert things about what the platform implmentation returns, and change types for returning to the actual bindings layer.

I'm not saying you need to fix this layering trouble.  But that appears to me to be the problem you're running into here?

[Daniel Cheng]

Comment on attachment 192984 [details]
Patch

Per our discussion, this patch is on hold until someone gets around to separating the DOM and platform layers in clipboard.