Chromium/Mac: Add support for yank (ctrl-y)
Created attachment 56031 [details] Patch
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).
Can we just re-use EditorMac? The RenderTheme code recently got unified iirc.
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)?
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.
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?
Created attachment 57072 [details] doh
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.
Created attachment 57075 [details] make stylebot happy(er)
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. :)
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.
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.
Created attachment 57115 [details] rebase
Comment on attachment 57115 [details] rebase I may be thinking of ThreadingNone.cpp in JavaScriptCore and similar. Looks fantastic! Thanks.
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
Created attachment 57249 [details] rebase once more
Created attachment 57250 [details] Add reviewer name
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
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 on attachment 57250 [details] Add reviewer name Clearing flags on attachment: 57250 Committed r60326: <http://trac.webkit.org/changeset/60326>
All reviewed patches have been landed. Closing bug.