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.
Created attachment 135087 [details] proposed patch
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.
Created attachment 135158 [details] patch reattaching patch to get the ews result.
(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?
> 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.
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.
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.
Hi Anders Carlsson/Darin Can you please provide your feedback on this patch. Thanks.
Hi Darin Can you please provide your feedback on this patch. Please let me know if some more inputs required.
vineetc: Pinging in IRC might be another option to catch him in his time zone.
I suggest posting a new patch that applies so the EWS can process it.
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.
Created attachment 144017 [details] Patch
(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.
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.
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
(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.
Created attachment 160053 [details] updated_patch Attaching rebased patch.
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.
Comment on attachment 160053 [details] updated_patch Clearing flags on attachment: 160053 Committed r126383: <http://trac.webkit.org/changeset/126383>
All reviewed patches have been landed. Closing bug.
Reverted r126383 for reason: Chromium win build crash Committed r126390: <http://trac.webkit.org/changeset/126390>
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?
(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));
(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...
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.
Reopening as patch was rolled out.
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.
Comment on attachment 186932 [details] updated_patch LGTM
Comment on attachment 186932 [details] updated_patch Attachment 186932 [details] did not pass win-ews (win): Output: http://queues.webkit.org/results/16416013
Comment on attachment 186932 [details] updated_patch I need to watch cr/win build to pass this patch.
Comment on attachment 186932 [details] updated_patch Clearing flags on attachment: 186932 Committed r142155: <http://trac.webkit.org/changeset/142155>
Reverted r142155 for reason: cr/win build broke. Committed r142170: <http://trac.webkit.org/changeset/142170>
Sorry, it happened again. See http://build.chromium.org/p/chromium.webkit/builders/Win%20Builder/builds/34703/steps/compile/logs/stdio
(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 :(.
(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.
(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*’"
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.
(In reply to comment #39) > the type functions I meant the *two* functions
Created attachment 187372 [details] patch
(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.
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.
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?
(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
Created attachment 192817 [details] Patch
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.
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?
(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.
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
Created attachment 192984 [details] Patch
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.
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.
(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.
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.
(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.
(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?
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.