[webkit-reviews] review denied: [Bug 56330] [chromium] Implement glue between DataTransferItems and the pasteboard. : [Attachment 86347] Patch

[bugzilla-daemon at webkit.org]

Tony Chang <tony at chromium.org> has denied Daniel Cheng <dcheng at chromium.org>'s
request for review:
Bug 56330: [chromium] Implement glue between DataTransferItems and the
pasteboard.
https://bugs.webkit.org/show_bug.cgi?id=56330

Attachment 86347: Patch
https://bugs.webkit.org/attachment.cgi?id=86347&action=review

------- Additional Comments from Tony Chang <tony at chromium.org>
(In reply to comment #7)
> (In reply to comment #5)
> > (From update of attachment 86254 [details] [details])
> > View in context:
https://bugs.webkit.org/attachment.cgi?id=86254&action=review
> > 
> > > Source/WebCore/ChangeLog:9
> > > +        Support retrieving clipboard data in a paste through
DataTransferItems.
> > > +
> > 
> > There should be a "Test: editing/pasteboard/data-transfer-items.html" line
there (normally prepare-ChangeLog does that for you, but you're probably just
editing the previous message).
> 
> Done. For some reason, it's not emitting that line. I added it manually.

Hmm, maybe it only adds it for new tests.  Anyway, it's nice to have either
way.

> > > Source/WebCore/platform/chromium/DataTransferItemChromium.h:49
> > > +    enum FromPasteboardType {
> > > +        FromPasteboard,
> > > +    };
> > 
> > I'm confused as to what this is for.
> > 
> 
> It's just a marker to distinguish the overloads of
DataTransferItem::create().

It seems like it would be a bit clearer to name the method
createFromPasteboard.  I would probably also just have a single constructor
that takes DataSource as a parameter (so you don't have to keep 2 sets of
constructor initializer lists in sync).


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

> Source/WebCore/platform/chromium/DataTransferItemsChromium.h:60
> +    friend class ClipboardChromium;

What is this for?