Bug 39092

Summary: Chromium/Mac: Add support for yank (ctrl-y)
Product: WebKit Reporter: Nico Weber <thakis>
Component: New BugsAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: ap, commit-queue, eric, ojan, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Other   
OS: OS X 10.5   
Attachments:
Description Flags
Patch
none
Patch with comments
none
doh
none
make stylebot happy(er)
eric: review-
address comments
none
rebase
eric: review+, commit-queue: commit-queue-
rebase once more
none
Add reviewer name none

Description Nico Weber 2010-05-13 16:17:27 PDT
Chromium/Mac: Add support for yank (ctrl-y)
Comment 1 Nico Weber 2010-05-13 16:22:50 PDT
Created attachment 56031 [details]
Patch
Comment 2 Nico Weber 2010-05-13 17:30:15 PDT
The new code is just a copy from some of the code in editing/mac/EditorMac.mm. jamesr suggested I pull that into a new file, but I'm not sure where to put that new file (editing/macchromium? surely not). avi says that all the rendertheme code is forked like this too (chromium's code is mostly a copy of the mac port's code).
Comment 3 Eric Seidel (no email) 2010-05-20 01:11:07 PDT
Can we just re-use EditorMac?  The RenderTheme code recently got unified iirc.
Comment 4 Nico Weber 2010-05-20 07:57:23 PDT
We can't use all of EditorMac (we don't want its Editor::newGeneralClipboard(), Editor::showFontPanel(),  Editor::showStylesPanel(), Editor::showColorPanel(), and Editor::paste()), only about 50% of it.

If we want to share the yank stuff, I could
1.) put that into a file that's used by both the mac port and the chromium port on mac. In that case I need to know what directory put the file with the shared parts.

2.) The RenderTheme stuff now uses inheritance. If we wanted to do that here, I'd need to give Editor a static create() method, change all calling sites of Editor::Editor() to use the factory method, and make some functions virtual. Then I'd put EditorChromiumMac and EditorMac directly into editing/, and make EditorMac a subclass of EditorChromiumMac. This feels intrusive to me.

Do you want me to do one of these (or something else)?
Comment 5 Eric Seidel (no email) 2010-05-20 12:31:10 PDT
Seems the original design for this would have been cleaner with a platform/KillRing.h class.  This is the current hack for other ports:

#if !PLATFORM(MAC)

void Editor::appendToKillRing(const String&)
{
}

void Editor::prependToKillRing(const String&)
{
}

String Editor::yankFromKillRing()
{
    return String();
}

void Editor::startNewKillRingSequence()
{
}

void Editor::setKillRingToYankedState()
{
}

#endif

I think we should put these calls into an object.  We can either make a KillRingNone for other ports, or we can make all the callers check non-zero.  But the Mac and ChromiumMac ports want to return a KillRingMac object.

Does that design sound sane?  You're more versed in this code than I am by now.
Comment 6 Nico Weber 2010-05-25 23:10:33 PDT
Created attachment 57071 [details]
Patch with comments

Makes sense; done.

Is there a good way to add files? I manually updated all project files I could find, I hope I didn't miss anything. Is there something easier?
Comment 7 Nico Weber 2010-05-25 23:14:31 PDT
Created attachment 57072 [details]
doh
Comment 8 WebKit Review Bot 2010-05-25 23:17:53 PDT
Attachment 57072 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style', '--no-squash']" exit_code: 1
WebCore/platform/mac/KillRingMac.h:42:  One space before end of line comments  [whitespace/comments] [5]
WebCore/platform/mac/KillRingMac.h:44:  One space before end of line comments  [whitespace/comments] [5]
WebCore/platform/KillRing.h:43:  One space before end of line comments  [whitespace/comments] [5]
WebCore/platform/KillRing.h:45:  One space before end of line comments  [whitespace/comments] [5]
Total errors found: 4 in 12 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 9 Nico Weber 2010-05-25 23:26:25 PDT
Created attachment 57075 [details]
make stylebot happy(er)
Comment 10 Eric Seidel (no email) 2010-05-26 01:11:16 PDT
Comment on attachment 57075 [details]
make stylebot happy(er)

We don't need this to be run-time switchable.  So we should just make KillRing.h non-virtual declarations and then add a KillRingNone.cpp which is used by every non-mac port and has empty implementations.

We could also just use a killRing() helper function in KillRing.h which returns a function-level static KillRing; which would then effectively be the killring.

Using a global function to access the killring like that makes mocking harder... but we don't really have C++ unit testing in WebCore anyway.  It would get rid of the need to have any #ifdefs in editor since everywhere would just call killRing() like it does now, but that would be a global function returning the global killring object.

Otherwise this patch looks FANTASTIC. :)
Comment 11 Eric Seidel (no email) 2010-05-26 01:12:24 PDT
The pattern I describe above of sharing declarations via the header and non-virtual implementations via *Mac or *None, etc. is a common pattern used in WebCore/platform.
Comment 12 Nico Weber 2010-05-26 10:36:26 PDT
Created attachment 57114 [details]
address comments

Created KillRingNone.cpp, removed virtual. The static function doesn't seem necessary.

find WebCore/platform -name '*None*' doesn't find a whole bunch of stuff, though.
Comment 13 Nico Weber 2010-05-26 10:50:24 PDT
Created attachment 57115 [details]
rebase
Comment 14 Eric Seidel (no email) 2010-05-26 12:30:07 PDT
Comment on attachment 57115 [details]
rebase

I may be thinking of ThreadingNone.cpp in JavaScriptCore and similar.

Looks fantastic!  Thanks.
Comment 15 WebKit Commit Bot 2010-05-27 07:51:08 PDT
Comment on attachment 57115 [details]
rebase

Rejecting patch 57115 from commit-queue.

Failed to run "[u'/Users/eseidel/Projects/CommitQueue/WebKitTools/Scripts/svn-apply', u'--reviewer', u'Eric Seidel', u'--force']" exit_code: 1
Last 500 characters of output:
f 6 hunks FAILED -- saving rejects to file WebCore/WebCore.xcodeproj/project.pbxproj.rej
patching file WebCore/editing/Editor.cpp
patching file WebCore/editing/Editor.h
patching file WebCore/editing/EditorCommand.cpp
Hunk #1 succeeded at 47 with fuzz 2 (offset 1 line).
Hunk #2 succeeded at 1047 (offset 1 line).
patching file WebCore/editing/mac/EditorMac.mm
patching file WebCore/platform/KillRing.h
patching file WebCore/platform/KillRingNone.cpp
patching file WebCore/platform/mac/KillRingMac.mm

Full output: http://webkit-commit-queue.appspot.com/results/2556021
Comment 16 Nico Weber 2010-05-27 08:41:24 PDT
Created attachment 57249 [details]
rebase once more
Comment 17 Nico Weber 2010-05-27 08:51:50 PDT
Created attachment 57250 [details]
Add reviewer name
Comment 18 Antonio Gomes 2010-05-27 08:55:51 PDT
Comment on attachment 57250 [details]
Add reviewer name

<tonikitoo> thakis, you do not need to r+ it yourself, but just get a cq+ from a committer/reviewer
<tonikitoo> thakis, also you have no commit message. generally tools combine the changes you have done in changeLog files as the commit message
<tonikitoo> not sure if it will be processed correctly by the tools, but lets try
Comment 19 Eric Seidel (no email) 2010-05-27 11:11:55 PDT
You can use "webkit-patch land-safely" and the tools will apply the reviewer to your current diff and then upload it with commit-queue=?  That's the automated version of what you just did. :)  If the patch isnt' in your working copy before you do that you woudl have to run "webkit-patch apply-from-bug 39092" or "webkit-patch apply-attachment 57115 [details]".
Comment 20 WebKit Commit Bot 2010-05-27 15:42:04 PDT
Comment on attachment 57250 [details]
Add reviewer name

Clearing flags on attachment: 57250

Committed r60326: <http://trac.webkit.org/changeset/60326>
Comment 21 WebKit Commit Bot 2010-05-27 15:42:12 PDT
All reviewed patches have been landed.  Closing bug.