Bug 39092 – Chromium/Mac: Add support for yank (ctrl-y)

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Nico Weber]

Chromium/Mac: Add support for yank (ctrl-y)

[Nico Weber]

Created attachment 56031 [details]
Patch

[Nico Weber]

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).

[Eric Seidel (no email)]

Can we just re-use EditorMac?  The RenderTheme code recently got unified iirc.

[Nico Weber]

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)?

[Eric Seidel (no email)]

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.

[Nico Weber]

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?

[Nico Weber]

Created attachment 57072 [details]
doh

[WebKit Review Bot]

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.

[Nico Weber]

Created attachment 57075 [details]
make stylebot happy(er)

[Eric Seidel (no email)]

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. :)

[Eric Seidel (no email)]

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.

[Nico Weber]

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.

[Nico Weber]

Created attachment 57115 [details]
rebase

[Eric Seidel (no email)]

Comment on attachment 57115 [details]
rebase

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

Looks fantastic!  Thanks.

[WebKit Commit Bot]

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

[Nico Weber]

Created attachment 57249 [details]
rebase once more

[Nico Weber]

Created attachment 57250 [details]
Add reviewer name

[Antonio Gomes]

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

[Eric Seidel (no email)]

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]".

[WebKit Commit Bot]

Comment on attachment 57250 [details]
Add reviewer name

Clearing flags on attachment: 57250

Committed r60326: <http://trac.webkit.org/changeset/60326>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.