Bug 29634 – Introduce Pasteboard::writePlaintext(const String&) for copying elements code to clipboard in WebInspector

• Component: WebKit API
• Status: RESOLVED FIXED

[Alexander Pavlov (apavlov)]

Created attachment 39911 [details]
Pasteboard::writePlaintext implementation for all platforms

writePlaintext() is necessary for WebInspector to be able to copy the selected element HTML to the system clipboard.

The suggested implementation never substitutes spaces for   since we want to get the associated HTML as-is.

[Pavel Feldman]

Comment on attachment 39911 [details]
Pasteboard::writePlaintext implementation for all platforms

> +    void writePlaintext(const String&);

writePlainText ?

>      ChromiumBridge::clipboardWriteSelection(html, url, plainText, canSmartCopyOrDelete);

In this case pasting html flavor would lead to the original content being pasted, not the markup.

> +    md->setText(qtext);
> +    QApplication::clipboard()->setMimeData(md, m_selectionMode ?
> +            QClipboard::Selection : QClipboard::Clipboard);

Indent.

[Alexander Pavlov (apavlov)]

(In reply to comment #1)
> (From update of attachment 39911 [details])
> > +    void writePlaintext(const String&);
> 
> writePlainText ?
Fixed.

> >      ChromiumBridge::clipboardWriteSelection(html, url, plainText, canSmartCopyOrDelete);
> 
> In this case pasting html flavor would lead to the original content being
> pasted, not the markup.
Correct. Added ChromiumBridge::clipboardWritePlainText().

> > +    md->setText(qtext);
> > +    QApplication::clipboard()->setMimeData(md, m_selectionMode ?
> > +            QClipboard::Selection : QClipboard::Clipboard);
> 
> Indent.
This same indent is found in writeURL and is likely to be caused by the ternary operator wrapping Qt coding style guide.

[Alexander Pavlov (apavlov)]

Created attachment 39921 [details]
patch (fixed casing and Chromium implementation)

[Pavel Feldman]

ChangeLog files are missing.

[Alexander Pavlov (apavlov)]

Created attachment 39924 [details]
patch (added ChangeLog)

[Eric Seidel (no email)]

Comment on attachment 39924 [details]
patch (added ChangeLog)

I'm impressed that you did all these implementations!

     if (wxTheClipboard->Open())
 65     {

{ should be on the same line as the if.

The ChangeLog is missing any explanation as to why we would add this.

[Pavel Feldman]

> The ChangeLog is missing any explanation as to why we would add this.

I can see the ChangeLog in the latest patch; there is a bug in the Blocks list that shows how new functionality is used. However adding more into the bug description / changelog entry surely makes sense.

Eric, do you feel comfortable reviewing it? Timothy thought it was Ok, but wanted someone else to take a look at it as well. I am trying to minimize the number of over-night-reviews and get an r+ for this. Will make sure description and style comments are fixed prior to landing. Thanks.

[Alexander Pavlov (apavlov)]

(In reply to comment #6)
> (From update of attachment 39924 [details])
> I'm impressed that you did all these implementations!
> 
>      if (wxTheClipboard->Open())
>  65     {
> 
> { should be on the same line as the if.
Fixed along with similar cases in other methods.

> The ChangeLog is missing any explanation as to why we would add this.
Added an explanation into ChangeLog.

[Alexander Pavlov (apavlov)]

Created attachment 39989 [details]
patch (explanation in ChangeLog, fixed style for wx)

[Eric Seidel (no email)]

Comment on attachment 39989 [details]
patch (explanation in ChangeLog, fixed style for wx)

OK.  Looks sane enough.  We don't generally commit commented-out code, but I can understand the difficulties here with the chromium stuff being split between two repositories.  If you wanted this auto-committed by the bot, you'll need to set commit-queue=?.  Otherwise you can find someone to commit this for you since I don't think you're a committer yet (at least I don't remember that being the case).

[Pavel Feldman]

Committing to http://svn.webkit.org/repository/webkit/trunk ...
	M	WebCore/ChangeLog
	M	WebCore/platform/Pasteboard.h
	M	WebCore/platform/android/TemporaryLinkStubs.cpp
	M	WebCore/platform/chromium/ChromiumBridge.h
	M	WebCore/platform/chromium/PasteboardChromium.cpp
	M	WebCore/platform/gtk/PasteboardGtk.cpp
	M	WebCore/platform/haiku/PasteboardHaiku.cpp
	M	WebCore/platform/mac/PasteboardMac.mm
	M	WebCore/platform/qt/PasteboardQt.cpp
	M	WebCore/platform/win/PasteboardWin.cpp
	M	WebCore/platform/wince/PasteboardWince.cpp
	M	WebCore/platform/wx/PasteboardWx.cpp
Committed r48710