[Webkit-unassigned] [Bug 55115] Add support for DataTransferItems

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=55115





--- Comment #21 from Daniel Cheng <dcheng at chromium.org>  2011-03-07 19:03:11 PST ---

(In reply to comment #17)
> (From update of attachment 84992 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=84992&action=review
> 
> A few more comments.
> 
> > LayoutTests/editing/pasteboard/data-transfer-items.html:4
> > +<div>This test checks that reading/writing functionality of DataTransferItems. This test requires DRT.</div>
> 
> I don't understand this sentence "This test checks that reading/writing functionality of DataTransferItems"
> 

Um. Is that any better? =P

> > LayoutTests/editing/pasteboard/data-transfer-items.html:25
> > +      items.add('Moo', 'text/plain');
> 
> inconsistent indentation. (2 spaces vs 4 in other places in this file).
> 

Done.

> > LayoutTests/editing/pasteboard/data-transfer-items.html:86
> > +    if (savedDataTransferItem.type!= '') {
> 
> add space before !=
> 

Done.

> > LayoutTests/editing/pasteboard/data-transfer-items.html:88
> > +        console.log('DataTransferItem.typeaccessible outside event handler!');
> 
> missing space "typeaccessible"
> 

Done.

> > Source/WebCore/bindings/scripts/CodeGeneratorV8.pm:131
> > +    if (!$codeGenerator->IsPrimitiveType($type) and !$codeGenerator->IsStringType($type) and !$codeGenerator->AvoidInclusionOfType($type) and $type ne "Date") {
> 
> Would be nice to explain this change in the ChangeLog.

Done.

> 
> Do we need to regenerate the output files?
> 

This happens magically.

> Also do we need an equivalent change in CodeGeneratorJS.pm?
> 

I have no idea. It seemed to work last time I tried it.

> > Source/WebCore/dom/Clipboard.h:29
> > +#include "DataTransferItems.h"
> 
> Can this be replaced with a forward declaration?
> 

Already done.

> > Source/WebCore/dom/Clipboard.idl:46
> > +        readonly attribute [Conditional=DATA_TRANSFER_ITEMS] DataTransferItems items;
> 
> This should have a runtime flag to turn it on, so that it isn't exposed until it is cleared for shipping in chromium.
> 

Done.

> > Source/WebCore/platform/chromium/DataTransferItemChromium.cpp:67
> > +        || m_kind != kindString)
> 
> Why so many lines?
> 

Done.

> > Source/WebCore/platform/chromium/DataTransferItemChromium.h:43
> > +class DataTransferItemChromium : public DataTransferItem {
> 
> Why is this class chromium specific? It looks very generic.
> 

It's getting some pasteboard specific logic in a follow up patch.

> > Source/WebCore/platform/chromium/DataTransferItemsChromium.cpp:63
> > +    if (m_owner->policy() != ClipboardWritable) {
> 
> Why doesn't this also check for ClipboardNumb?
> 
> >>> Source/WebCore/platform/chromium/DataTransferItemsChromium.cpp:84
> >>> +    if (m_owner->policy() != ClipboardWritable)
> >> 
> >> Why doesn't this cause an exception code to be set?
> > 
> > Strict interpretation of the spec.
> 
> Why doesn't this also check for ClipboardNumb?

I don't follow. We only allow these operations if the data store should be writable.

> 
> > Source/WebCore/platform/chromium/DataTransferItemsChromium.cpp:87
> > +    for (size_t i = 0; i < m_items.size(); ++i) {
> 
> Would be nice to add a comment right before the for loop.
> 
> Something like:
> // Only one string item with a given type is allowed in the items collection.

Done.

(In reply to comment #19)
> (From update of attachment 85003 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=85003&action=review
> 
> See comments above and one last comment. Once these are all addressed this should be good to go. Thanks!
> 
> > Source/WebCore/dom/DataTransferItems.h:45
> > +class DataTransferItems : public RefCounted<DataTransferItems> {
> 
> Do we need a virtual destructor here?

Done.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.