Bug 29634 - Introduce Pasteboard::writePlaintext(const String&) for copying elements code to clipboard in WebInspector
Summary: Introduce Pasteboard::writePlaintext(const String&) for copying elements code...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit API (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC All
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks: 28357
  Show dependency treegraph
 
Reported: 2009-09-22 04:03 PDT by Alexander Pavlov (apavlov)
Modified: 2009-09-24 02:39 PDT (History)
3 users (show)

See Also:


Attachments
Pasteboard::writePlaintext implementation for all platforms (7.60 KB, patch)
2009-09-22 04:03 PDT, Alexander Pavlov (apavlov)
no flags Details | Formatted Diff | Diff
patch (fixed casing and Chromium implementation) (8.25 KB, patch)
2009-09-22 07:00 PDT, Alexander Pavlov (apavlov)
no flags Details | Formatted Diff | Diff
patch (added ChangeLog) (9.52 KB, patch)
2009-09-22 07:58 PDT, Alexander Pavlov (apavlov)
no flags Details | Formatted Diff | Diff
patch (explanation in ChangeLog, fixed style for wx) (10.40 KB, patch)
2009-09-23 04:46 PDT, Alexander Pavlov (apavlov)
eric: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Alexander Pavlov (apavlov) 2009-09-22 04:03:39 PDT
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.
Comment 1 Pavel Feldman 2009-09-22 05:07:08 PDT
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.
Comment 2 Alexander Pavlov (apavlov) 2009-09-22 06:51:58 PDT
(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.
Comment 3 Alexander Pavlov (apavlov) 2009-09-22 07:00:10 PDT
Created attachment 39921 [details]
patch (fixed casing and Chromium implementation)
Comment 4 Pavel Feldman 2009-09-22 07:12:21 PDT
ChangeLog files are missing.
Comment 5 Alexander Pavlov (apavlov) 2009-09-22 07:58:48 PDT
Created attachment 39924 [details]
patch (added ChangeLog)
Comment 6 Eric Seidel (no email) 2009-09-22 14:39:20 PDT
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.
Comment 7 Pavel Feldman 2009-09-22 22:35:21 PDT
> 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.
Comment 8 Alexander Pavlov (apavlov) 2009-09-23 04:44:30 PDT
(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.
Comment 9 Alexander Pavlov (apavlov) 2009-09-23 04:46:59 PDT
Created attachment 39989 [details]
patch (explanation in ChangeLog, fixed style for wx)
Comment 10 Eric Seidel (no email) 2009-09-23 17:35:47 PDT
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).
Comment 11 Pavel Feldman 2009-09-24 02:39:11 PDT
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