RESOLVED FIXED 39092
Chromium/Mac: Add support for yank (ctrl-y)
https://bugs.webkit.org/show_bug.cgi?id=39092
Summary Chromium/Mac: Add support for yank (ctrl-y)
Nico Weber
Reported 2010-05-13 16:17:27 PDT
Chromium/Mac: Add support for yank (ctrl-y)
Attachments
Patch (5.11 KB, patch)
2010-05-13 16:22 PDT, Nico Weber
no flags
Patch with comments (14.44 KB, patch)
2010-05-25 23:10 PDT, Nico Weber
no flags
doh (21.56 KB, patch)
2010-05-25 23:14 PDT, Nico Weber
no flags
make stylebot happy(er) (21.56 KB, patch)
2010-05-25 23:26 PDT, Nico Weber
eric: review-
address comments (22.28 KB, patch)
2010-05-26 10:36 PDT, Nico Weber
no flags
rebase (22.30 KB, patch)
2010-05-26 10:50 PDT, Nico Weber
eric: review+
commit-queue: commit-queue-
rebase once more (22.26 KB, patch)
2010-05-27 08:41 PDT, Nico Weber
no flags
Add reviewer name (22.25 KB, patch)
2010-05-27 08:51 PDT, Nico Weber
no flags
Nico Weber
Comment 1 2010-05-13 16:22:50 PDT
Nico Weber
Comment 2 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).
Eric Seidel (no email)
Comment 3 2010-05-20 01:11:07 PDT
Can we just re-use EditorMac? The RenderTheme code recently got unified iirc.
Nico Weber
Comment 4 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)?
Eric Seidel (no email)
Comment 5 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.
Nico Weber
Comment 6 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?
Nico Weber
Comment 7 2010-05-25 23:14:31 PDT
WebKit Review Bot
Comment 8 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.
Nico Weber
Comment 9 2010-05-25 23:26:25 PDT
Created attachment 57075 [details] make stylebot happy(er)
Eric Seidel (no email)
Comment 10 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. :)
Eric Seidel (no email)
Comment 11 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.
Nico Weber
Comment 12 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.
Nico Weber
Comment 13 2010-05-26 10:50:24 PDT
Eric Seidel (no email)
Comment 14 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.
WebKit Commit Bot
Comment 15 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
Nico Weber
Comment 16 2010-05-27 08:41:24 PDT
Created attachment 57249 [details] rebase once more
Nico Weber
Comment 17 2010-05-27 08:51:50 PDT
Created attachment 57250 [details] Add reviewer name
Antonio Gomes
Comment 18 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
Eric Seidel (no email)
Comment 19 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]".
WebKit Commit Bot
Comment 20 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>
WebKit Commit Bot
Comment 21 2010-05-27 15:42:12 PDT
All reviewed patches have been landed. Closing bug.
Note You need to log in before you can comment on or make changes to this bug.