Summary: | [Gtk] Add Undo/Redo support to WebKitGtk | ||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Jiahua Huang <jhuangjiahua> | ||||||||||||
Component: | WebKitGTK | Assignee: | Nobody <webkit-unassigned> | ||||||||||||
Status: | CLOSED FIXED | ||||||||||||||
Severity: | Enhancement | CC: | jhuangjiahua, jmalonzo, zecke | ||||||||||||
Priority: | P2 | ||||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||||
Hardware: | All | ||||||||||||||
OS: | All | ||||||||||||||
Attachments: |
|
Description
Jiahua Huang
2009-06-20 06:19:16 PDT
Created attachment 31595 [details]
Add Undo/Redo support to WebKitGtk
This patch extracted from chromium linux.(In reply to comment #1) (In reply to comment #2) > This patch extracted from chromium linux.(In reply to comment #1) Thanks for the patch. This requires a ChangeLog, see http://webkit.org/coding/contributing.html on how to contribute code. The style in this patch needs to adhere to our coding style too - see http://webkit.org/coding/coding-style.html for more info. Created attachment 31597 [details]
patch with a ChangeLog
add a ChangeLog.
Create the patch using the svn-create-patch script.
(In reply to comment #4) > Created an attachment (id=31597) [review] > patch with a ChangeLog > > add a ChangeLog. > Create the patch using the svn-create-patch script. > Thanks. add a ChangeLog, and create a new patch using the svn-create-patch script. Comment on attachment 31597 [details] patch with a ChangeLog Thanks for updating the patch. > -void EditorClient::registerCommandForUndo(WTF::PassRefPtr<WebCore::EditCommand>) > +void EditorClient::registerCommandForUndo(WTF::PassRefPtr<WebCore::EditCommand> command) > { > - notImplemented(); > + if (undo_stack_.size() == kMaximumUndoStackDepth) > + undo_stack_.pop_front(); // drop oldest item off the far end The variable names need to be fixed - should be undoStack for example. Please see the coding style guidelines I referenced earlier. > void EditorClient::undo() > { > - notImplemented(); > + if (canUndo()) { > + RefPtr<WebCore::EditCommand> command(undo_stack_.back()); > + undo_stack_.pop_back(); > + command->unapply(); > + // unapply will call us back to push this command onto the redo stack. Please put the comment before the operation it's trying to give context to. Not after. > void EditorClient::redo() > { > - notImplemented(); > + if (canRedo()) { > + RefPtr<WebCore::EditCommand> command(redo_stack_.back()); > + redo_stack_.pop_back(); > + > + ASSERT(!in_redo_); > + in_redo_ = true; > + command->reapply(); > + // reapply will call us back to push this command onto the undo stack. Ditto. > class EditorClient : public WebCore::EditorClient { > + protected: > + bool use_editor_delegate_; Where is this variable used? Looks fine nevertheless. Please mark the review flag as r? next time so it will end up in the review queue and other reviewers can review it too. Thanks! Created attachment 31605 [details]
repatch follow the coding style guidelines
(In reply to comment #6) > (From update of attachment 31597 [details] [review]) > Looks fine nevertheless. Please mark the review flag as r? next time so it will > end up in the review queue and other reviewers can review it too. > Thanks, it has been revised. Comment on attachment 31605 [details] repatch follow the coding style guidelines First of all great you are working on it, and I assumed that such a implementation would be much bigger than what you came up with... A general remark is please read: http://webkit.org/coding/contributing.html http://webkit.org/coding/coding-style.html > Index: WebKit/gtk/ChangeLog > =================================================================== > --- WebKit/gtk/ChangeLog (revision 44907) > +++ WebKit/gtk/ChangeLog (working copy) > @@ -1,3 +1,18 @@ > +2009-06-21 Jiahua Huang <jhuangjiahua@gmail.com> > + > + Unreviewed. Add Undo/Redo support to WebKitGtk. Please us the prepare-ChangeLog tool to generate the changelog. > +// Arbitrary depth limit for the undo stack, to keep it from using > +// unbounded memory. This is the maximum number of distinct undoable > +// actions -- unbroken stretches of typed characters are coalesced > +// into a single action. > +static const size_t kMaximumUndoStackDepth = 1000; the naming of the variable is a bit awkward... we don't use the 'k' for konstants or such... just make it maximumUndoStackDepth, or to reduce the binary size make it a #define.. > bool EditorClient::shouldInsertNode(Node*, Range*, EditorInsertAction) > Index: WebKit/gtk/WebCoreSupport/EditorClientGtk.h > =================================================================== > --- WebKit/gtk/WebCoreSupport/EditorClientGtk.h (revision 44907) > +#include <deque> > + Main reason to say know right now. I don't think we want to have more stl dependencies. Could you please revise the patch to use the wtf/Deque.h and check if that works? > #include "EditorClient.h" > > #include <wtf/Forward.h> > @@ -43,6 +45,13 @@ namespace WebCore { > namespace WebKit { > > class EditorClient : public WebCore::EditorClient { > + protected: > + bool isInRedo; To make the paranoid happy, could you please add an EditorClient constructor and initialize isInRedo? Created attachment 31651 [details]
revise the patch to use the wtf/Deque.h
Comment on attachment 31651 [details] revise the patch to use the wtf/Deque.h > +void EditorClient::registerCommandForUndo(WTF::PassRefPtr<WebCore::EditCommand> command) > { > - notImplemented(); > + if (undoStack.size() == maximumUndoStackDepth) > + undoStack.removeFirst(); Kindly fix the spacing. > bool EditorClient::shouldInsertNode(Node*, Range*, EditorInsertAction) > @@ -524,6 +547,7 @@ EditorClient::EditorClient(WebKitWebView > : m_webView(webView) > { > WebKitWebViewPrivate* priv = m_webView->priv; > + isInRedo = false; This needs to be in the initialization list. > class EditorClient : public WebCore::EditorClient { > + protected: > + bool isInRedo; Member variables need to be prefixed with m_. See coding style for details. > + > + typedef WTF::Deque<WTF::RefPtr<WebCore::EditCommand> > EditCommandStack; > + EditCommandStack undoStack; > + EditCommandStack redoStack; Is the typedef necessary since you're only using EditCommandStack in these two lines? r- for now until those the style issues are fixed. Patch looks fine otherwise. Created attachment 31779 [details] fixed the style issues (In reply to comment #11) > r- for now until those the style issues are fixed. Patch looks fine otherwise. > Thanks, the style issues are fixed. Comment on attachment 31779 [details] fixed the style issues > EditorClient::EditorClient(WebKitWebView* webView) > - : m_webView(webView) > + : m_isInRedo(false), m_webView(webView) The last style issue... I will fix it when landing! Thanks for the work. Thanks landed in r45080. Some more remarks on the style... normally we put protected/private to the bottom of the file... and at least I have the old Qt habbit to prefix members with m_ so you can easily see what is a member and what is not. (In reply to comment #14) > Thanks landed in r45080. Some more remarks on the style... normally we put > protected/private to the bottom of the file... and at least I have the old Qt > habbit to prefix members with m_ so you can easily see what is a member and > what is not. > I appreciate your comments. I see. It works. Should I mark this bug as CLOSED |