[webkit-reviews] review denied: [Bug 53727] Clone WebClipboard to be frame-specific : [Attachment 81159] Patch

[bugzilla-daemon at webkit.org]

Dmitry Titov <dimich at chromium.org> has denied Daniel Cheng
<dcheng at chromium.org>'s request for review:
Bug 53727: Clone WebClipboard to be frame-specific
https://bugs.webkit.org/show_bug.cgi?id=53727

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

------- Additional Comments from Dmitry Titov <dimich at chromium.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=81159&action=review

A few comments (r- to clarify the lifetime of stored Document*):

> Source/WebCore/ChangeLog:16
> +	   No new tests.

Lets explain more here. Is it because this code path is not yet enabled and
requires more changes in Chromium so when it'll be enabled the existing tests
will cover?

> Source/WebCore/platform/chromium/ChromiumDataObject.cpp:60
> +    const Document* document, Clipboard::ClipboardType clipboardType)

WebKit doesn't try to fit in 80 chars. Same for the other files in patch.

> Source/WebCore/platform/chromium/ReadableDataObject.h:67
> +    const Document* m_document;

2 questions here: 
- If it is not a RefPtr, we should have a good theory why the document is
guaranteed to be alive for the lifetime of ReadableDataObject, or we should
clear it (and check in getClipboard), or make this a RefPtr.
- Why isn't it Frame?

> Source/WebKit/chromium/public/WebFrameClient.h:92
> +    // A frame specific clipboard. May return null, in which case... you're
done.

It's be better to say a bit more, "the caller should assume there is no data in
clipboard" or something like that?