[webkit-reviews] review denied: [Bug 26573] [Gtk] Add Undo/Redo support to WebKitGtk : [Attachment 31605] repatch follow the coding style guidelines

[bugzilla-daemon at webkit.org]

Holger Freyther <zecke at selfish.org> has denied Jiahua Huang
<jhuangjiahua at gmail.com>'s request for review:
Bug 26573: [Gtk] Add Undo/Redo support to WebKitGtk
https://bugs.webkit.org/show_bug.cgi?id=26573

Attachment 31605: repatch follow the coding style guidelines
https://bugs.webkit.org/attachment.cgi?id=31605&action=review

------- Additional Comments from Holger Freyther <zecke at selfish.org>
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 at 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?