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

[bugzilla-daemon at webkit.org]

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





--- Comment #17 from David Levin <levin at chromium.org>  2011-03-07 18:03:38 PST ---
(From update of attachment 84992)
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"

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

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

add space before !=

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

missing space "typeaccessible"

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

Do we need to regenerate the output files?

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

> Source/WebCore/dom/Clipboard.h:29
> +#include "DataTransferItems.h"

Can this be replaced with a forward declaration?

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

> Source/WebCore/platform/chromium/DataTransferItemChromium.cpp:67
> +        || m_kind != kindString)

Why so many lines?

> Source/WebCore/platform/chromium/DataTransferItemChromium.h:43
> +class DataTransferItemChromium : public DataTransferItem {

Why is this class chromium specific? It looks very generic.

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

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

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