WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
REOPENED
82888
Consider replacing return type of Clipboard::types() from ListHashSet<String> to Vector<String>
https://bugs.webkit.org/show_bug.cgi?id=82888
Summary
Consider replacing return type of Clipboard::types() from ListHashSet<String>...
Vineet Chaudhary (vineetc)
Reported
2012-04-02 05:11:51 PDT
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.
Attachments
proposed patch
(20.58 KB, patch)
2012-04-02 06:36 PDT
,
Vineet Chaudhary (vineetc)
no flags
Details
Formatted Diff
Diff
patch
(20.59 KB, patch)
2012-04-02 12:43 PDT
,
Vineet Chaudhary (vineetc)
eric
: review-
Details
Formatted Diff
Diff
Patch
(20.82 KB, patch)
2012-05-25 02:36 PDT
,
Vineet Chaudhary (vineetc)
no flags
Details
Formatted Diff
Diff
updated_patch
(20.68 KB, patch)
2012-08-22 17:50 PDT
,
Vineet Chaudhary (vineetc)
no flags
Details
Formatted Diff
Diff
updated_patch
(19.55 KB, patch)
2013-02-06 15:19 PST
,
Vineet Chaudhary (vineetc)
no flags
Details
Formatted Diff
Diff
patch
(22.71 KB, patch)
2013-02-08 15:39 PST
,
Vineet Chaudhary (vineetc)
no flags
Details
Formatted Diff
Diff
Patch
(23.75 KB, patch)
2013-03-12 15:05 PDT
,
Daniel Cheng
no flags
Details
Formatted Diff
Diff
Patch
(23.73 KB, patch)
2013-03-13 13:27 PDT
,
Daniel Cheng
no flags
Details
Formatted Diff
Diff
Show Obsolete
(7)
View All
Add attachment
proposed patch, testcase, etc.
Vineet Chaudhary (vineetc)
Comment 1
2012-04-02 06:36:29 PDT
Created
attachment 135087
[details]
proposed patch
Kentaro Hara
Comment 2
2012-04-02 06:40:36 PDT
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)
Comment 3
2012-04-02 12:43:55 PDT
Created
attachment 135158
[details]
patch reattaching patch to get the ews result.
Vineet Chaudhary (vineetc)
Comment 4
2012-04-02 12:52:33 PDT
(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
Comment 5
2012-04-02 15:24:28 PDT
>
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
Comment 6
2012-04-02 19:36:22 PDT
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 7
2012-04-03 02:01:58 PDT
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)
Comment 8
2012-04-04 04:19:42 PDT
Hi Anders Carlsson/Darin Can you please provide your feedback on this patch. Thanks.
Vineet Chaudhary (vineetc)
Comment 9
2012-04-09 05:45:29 PDT
Hi Darin Can you please provide your feedback on this patch. Please let me know if some more inputs required.
Kentaro Hara
Comment 10
2012-04-09 05:47:33 PDT
vineetc: Pinging in IRC might be another option to catch him in his time zone.
Darin Adler
Comment 11
2012-04-19 09:28:19 PDT
I suggest posting a new patch that applies so the EWS can process it.
Eric Seidel (no email)
Comment 12
2012-04-19 15:44:51 PDT
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)
Comment 13
2012-05-25 02:36:51 PDT
Created
attachment 144017
[details]
Patch
Vineet Chaudhary (vineetc)
Comment 14
2012-05-25 02:43:45 PDT
(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 15
2012-08-22 15:44:15 PDT
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 16
2012-08-22 15:46:34 PDT
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)
Comment 17
2012-08-22 15:54:59 PDT
(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)
Comment 18
2012-08-22 17:50:30 PDT
Created
attachment 160053
[details]
updated_patch Attaching rebased patch.
Kentaro Hara
Comment 19
2012-08-22 17:53:09 PDT
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 20
2012-08-22 18:56:32 PDT
Comment on
attachment 160053
[details]
updated_patch Clearing flags on attachment: 160053 Committed
r126383
: <
http://trac.webkit.org/changeset/126383
>
WebKit Review Bot
Comment 21
2012-08-22 18:56:37 PDT
All reviewed patches have been landed. Closing bug.
Kentaro Hara
Comment 22
2012-08-22 21:48:53 PDT
Reverted
r126383
for reason: Chromium win build crash Committed
r126390
: <
http://trac.webkit.org/changeset/126390
>
Kentaro Hara
Comment 23
2012-08-22 21:49:04 PDT
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)
Comment 24
2012-08-23 04:35:35 PDT
(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
Comment 25
2012-08-23 04:38:53 PDT
(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 26
2012-08-23 14:48:48 PDT
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)
Comment 27
2013-02-06 11:06:46 PST
Reopening as patch was rolled out.
Vineet Chaudhary (vineetc)
Comment 28
2013-02-06 15:19:56 PST
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 29
2013-02-06 17:40:44 PST
Comment on
attachment 186932
[details]
updated_patch LGTM
Build Bot
Comment 30
2013-02-06 19:38:26 PST
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 31
2013-02-07 10:35:31 PST
Comment on
attachment 186932
[details]
updated_patch I need to watch cr/win build to pass this patch.
WebKit Review Bot
Comment 32
2013-02-07 11:02:33 PST
Comment on
attachment 186932
[details]
updated_patch Clearing flags on attachment: 186932 Committed
r142155
: <
http://trac.webkit.org/changeset/142155
>
WebKit Review Bot
Comment 33
2013-02-07 11:02:42 PST
All reviewed patches have been landed. Closing bug.
Gavin Peters
Comment 34
2013-02-07 13:28:22 PST
Reverted
r142155
for reason: cr/win build broke. Committed
r142170
: <
http://trac.webkit.org/changeset/142170
>
Gavin Peters
Comment 35
2013-02-07 13:28:43 PST
Sorry, it happened again. See
http://build.chromium.org/p/chromium.webkit/builders/Win%20Builder/builds/34703/steps/compile/logs/stdio
Vineet Chaudhary (vineetc)
Comment 36
2013-02-07 13:55:52 PST
(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
Comment 37
2013-02-07 16:13:44 PST
(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)
Comment 38
2013-02-07 17:00:37 PST
(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
Comment 39
2013-02-07 20:04:11 PST
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
Comment 40
2013-02-07 20:04:56 PST
(In reply to
comment #39
)
> the type functions
I meant the *two* functions
Vineet Chaudhary (vineetc)
Comment 41
2013-02-08 15:39:47 PST
Created
attachment 187372
[details]
patch
Vineet Chaudhary (vineetc)
Comment 42
2013-02-08 15:56:40 PST
(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 43
2013-03-01 10:39:46 PST
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
Comment 44
2013-03-11 18:32:55 PDT
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)
Comment 45
2013-03-11 18:44:46 PDT
(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
Comment 46
2013-03-12 15:05:57 PDT
Created
attachment 192817
[details]
Patch
Daniel Cheng
Comment 47
2013-03-12 15:08:34 PDT
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 48
2013-03-13 13:12:20 PDT
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
Comment 49
2013-03-13 13:15:19 PDT
(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 50
2013-03-13 13:16:14 PDT
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
Comment 51
2013-03-13 13:27:20 PDT
Created
attachment 192984
[details]
Patch
Daniel Cheng
Comment 52
2013-03-13 13:27:50 PDT
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 53
2013-03-13 13:47:21 PDT
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
Comment 54
2013-03-13 13:58:33 PDT
(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)
Comment 55
2013-03-13 14:19:35 PDT
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
Comment 56
2013-03-13 14:26:15 PDT
(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)
Comment 57
2013-03-13 14:31:26 PDT
(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 58
2013-03-13 15:09:30 PDT
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.
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug