Bug 87908 – Add UNDO_MANAGER build flag

• Status: RESOLVED
• Resolution: FIXED
• Component: HTML Editing

[Sukolsak Sakshuwong]

Add UNDO_MANAGER flag

[Sukolsak Sakshuwong]

Created attachment 144968 [details]
Patch

[Ryosuke Niwa]

(In reply to comment #1)
> Created an attachment (id=144968) [details]
> Patch

You need to update your checkout and resolve the conflicts.

[Sukolsak Sakshuwong]

Created attachment 145155 [details]
Patch

[Sukolsak Sakshuwong]

Created attachment 145161 [details]
Patch

[Ryosuke Niwa]

Reviewers: I can't review this patch because much of the change is based on a git-diff I gave to Sukolsak.

[Hajime Morrita]

Maybe you can add a RuntimeEnabledFeatures flag then the feature can be enabled only for the tests.

[Ryosuke Niwa]

(In reply to comment #6)
> Maybe you can add a RuntimeEnabledFeatures flag then the feature can be enabled only for the tests.

That's a good idea. Maybe in the next iteration?

[Hajime Morrita]

(In reply to comment #7)
> (In reply to comment #6)
> > Maybe you can add a RuntimeEnabledFeatures flag then the feature can be enabled only for the tests.
> 
> That's a good idea. Maybe in the next iteration?
Yeah, that will work.

[Sukolsak Sakshuwong]

Created attachment 145632 [details]
Patch

[Sukolsak Sakshuwong]

Created attachment 145635 [details]
Patch

[Martin Robinson]

Zan has been working closely with our build flags lately. I'd like him to double-check this patch.

[Raphael Kubo da Costa (:rakuco)]

Comment on attachment 145635 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=145635&action=review

It looks like you haven't added the new files to the CMake build system at all, have you?

> Source/cmake/WebKitFeatures.cmake:90
> +    WEBKIT_OPTION_DEFINE(ENABLE_UNDO_MANAGER "Toggle Undo Manager support" OFF)

You also need to add a define to Source/cmakeconfig.h.cmake.

> LayoutTests/platform/efl/Skipped:808
> +# UndoManager is not yet enabled.
> +editing/undomanager

I'd appreciate it if you could add this to TestExpectations instead (we're progressively moving away from Skipped).

[Sukolsak Sakshuwong]

Created attachment 145651 [details]
Patch

[Raphael Kubo da Costa (:rakuco)]

Thanks for the quick CMake fixes!

[Gyuyoung Kim]

Comment on attachment 145651 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=145651&action=review

> Source/WebCore/editing/UndoManager.idl:39
> +//        getter Object[] item(in unsigned long index);

If these attributes are not supported yet, in my opinion, it is better to remove these.

[Zan Dobersek]

Comment on attachment 145651 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=145651&action=review

> configure.ac:1521
> + Undo manager support                                     : $enable_undo_manager

Please modify the patch to not include any changes to configure.ac. The new feature enabling policy for the Gtk port intends to enable features that do not introduce external dependencies by default in WebCore when the feature is well-supported and stable.

> Source/WebCore/GNUmakefile.am:294
> +# Undo manager support

No need for this at this moment as the feature won't be enabled yet on the Gtk port.

> Source/WebCore/GNUmakefile.list.am:6112
> +if ENABLE_UNDO_MANAGER

Please move these files into general build lists. Despite the feature being disabled, in future the Gtk port would want all the files to be included in the build, regardless of the feature status.

Also, this would cause an error as the ENABLE_UNDO_MANAGER macro would not be present.

[Sukolsak Sakshuwong]

Created attachment 145866 [details]
Patch

[Gustavo Noronha (kov)]

Comment on attachment 145866 [details]
Patch

Attachment 145866 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/12903626

[Martin Robinson]

About the GTK+ build error. This is a list of directories that contain IDLs in Source/WebCore/GNUmakefile.am (search for "IDL_PATH" in the file). It seems that this is the first editing IDL, so you probably need to add the editing directory to that list.

[Sukolsak Sakshuwong]

Created attachment 145874 [details]
Patch

[Sukolsak Sakshuwong]

Created attachment 145879 [details]
Patch

[Hajime Morrita]

Comment on attachment 145879 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=145879&action=review

> Source/WebCore/editing/UndoManager.h:56
> +    RefPtr<Node> m_undoScopeHost;

It looks this RefPtr will make a circular reference between Document and UndoManager.
Please take a look at ScriptedAnimationController or MediaQueryMatcher to see how we can avoid this.

[Sukolsak Sakshuwong]

Created attachment 146591 [details]
Patch

[Tony Chang]

Comment on attachment 146591 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=146591&action=review

Did you want to be able to build using a command line flag (e.g., build-webkit --enable-undo-manager)?  That code is in Tools/Scripts/webkitperl/FeatureList.pm.

> Source/WebCore/dom/Document.cpp:217
> +#if ENABLE(UNDO_MANAGER)
> +#include "UndoManager.h"
> +#endif

Nit: I think it's OK to include this without the ENABLE check because the header also does the check.  Other features don't seem to follow this pattern, so I guess either style is OK.

> LayoutTests/platform/win/Skipped:1807
> +# UndoManager is not yet enabled.

Nit: Maybe include a link to the bug in the comment.

> LayoutTests/platform/wincairo/Skipped:2113
> +# UndoManager is not yet enabled.

Nit: Maybe include a link to the bug in the comment.

[Sukolsak Sakshuwong]

Created attachment 146654 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 146654 [details]
Patch for landing

Clearing flags on attachment: 146654

Committed r119895: <http://trac.webkit.org/changeset/119895>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.