Bug 26573 – [Gtk] Add Undo/Redo support to WebKitGtk

• Component: WebKitGTK
• Status: CLOSED FIXED

[Jiahua Huang]

Please add Undo/Redo support to WebKitGtk. Safari, QtWebkit and Chromium already supports this.

[Jiahua Huang]

Created attachment 31595 [details]
Add Undo/Redo support to WebKitGtk

[Jiahua Huang]

This patch extracted from chromium linux.(In reply to comment #1)

[Jan Alonzo]

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

[Jiahua Huang]

Created attachment 31597 [details]
patch with a ChangeLog

add a ChangeLog.
Create the patch using the svn-create-patch script.

[Jiahua Huang]

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

[Jan Alonzo]

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!

[Jiahua Huang]

Created attachment 31605 [details]
repatch follow the coding style guidelines

[Jiahua Huang]

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

[Holger Freyther]

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 @@
> [phone]  Jiahua Huang  <[e-mail]>
> +
> +        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?

[Jiahua Huang]

Created attachment 31651 [details]
revise the patch to use the wtf/Deque.h

[Jan Alonzo]

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.

[Jiahua Huang]

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.

[Holger Freyther]

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.

[Holger Freyther]

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.

[Jiahua Huang]

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

[Jiahua Huang]

It works.
Should I mark this bug as CLOSED