Bug 87908

Summary: Add UNDO_MANAGER build flag
Product: WebKit Reporter: Sukolsak Sakshuwong <sukolsak>
Component: HTML EditingAssignee: Sukolsak Sakshuwong <sukolsak>
Status: RESOLVED FIXED    
Severity: Normal CC: abarth, dbates, enrica, eric, gns, morrita, mrobinson, ojan, philn, rakeshchaitan, rakuco, rniwa, tkent, tony, vestbo, webkit.review.bot, xan.lopez, zan
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 87189    
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch for landing none

Description Sukolsak Sakshuwong 2012-05-30 16:06:28 PDT
Add UNDO_MANAGER flag
Comment 1 Sukolsak Sakshuwong 2012-05-30 18:34:26 PDT
Created attachment 144968 [details]
Patch
Comment 2 Ryosuke Niwa 2012-05-30 19:22:10 PDT
(In reply to comment #1)
> Created an attachment (id=144968) [details]
> Patch

You need to update your checkout and resolve the conflicts.
Comment 3 Sukolsak Sakshuwong 2012-05-31 14:11:46 PDT
Created attachment 145155 [details]
Patch
Comment 4 Sukolsak Sakshuwong 2012-05-31 14:48:54 PDT
Created attachment 145161 [details]
Patch
Comment 5 Ryosuke Niwa 2012-05-31 17:19:26 PDT
Reviewers: I can't review this patch because much of the change is based on a git-diff I gave to Sukolsak.
Comment 6 Hajime Morrita 2012-05-31 17:32:44 PDT
Maybe you can add a RuntimeEnabledFeatures flag then the feature can be enabled only for the tests.
Comment 7 Ryosuke Niwa 2012-05-31 17:54:15 PDT
(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?
Comment 8 Hajime Morrita 2012-05-31 18:02:20 PDT
(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.
Comment 9 Sukolsak Sakshuwong 2012-06-04 14:55:58 PDT
Created attachment 145632 [details]
Patch
Comment 10 Sukolsak Sakshuwong 2012-06-04 15:01:21 PDT
Created attachment 145635 [details]
Patch
Comment 11 Martin Robinson 2012-06-04 15:03:13 PDT
Zan has been working closely with our build flags lately. I'd like him to double-check this patch.
Comment 12 Raphael Kubo da Costa (:rakuco) 2012-06-04 15:25:02 PDT
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).
Comment 13 Sukolsak Sakshuwong 2012-06-04 16:30:27 PDT
Created attachment 145651 [details]
Patch
Comment 14 Raphael Kubo da Costa (:rakuco) 2012-06-04 17:05:35 PDT
Thanks for the quick CMake fixes!
Comment 15 Gyuyoung Kim 2012-06-04 18:06:32 PDT
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.
Comment 16 Zan Dobersek 2012-06-05 01:14:47 PDT
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.
Comment 17 Sukolsak Sakshuwong 2012-06-05 14:01:48 PDT
Created attachment 145866 [details]
Patch
Comment 18 Gustavo Noronha (kov) 2012-06-05 14:10:46 PDT
Comment on attachment 145866 [details]
Patch

Attachment 145866 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/12903626
Comment 19 Martin Robinson 2012-06-05 14:31:14 PDT
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.
Comment 20 Sukolsak Sakshuwong 2012-06-05 14:55:01 PDT
Created attachment 145874 [details]
Patch
Comment 21 Sukolsak Sakshuwong 2012-06-05 15:13:03 PDT
Created attachment 145879 [details]
Patch
Comment 22 Hajime Morrita 2012-06-07 18:00:53 PDT
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.
Comment 23 Sukolsak Sakshuwong 2012-06-08 10:11:47 PDT
Created attachment 146591 [details]
Patch
Comment 24 Tony Chang 2012-06-08 10:36:57 PDT
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.
Comment 25 Sukolsak Sakshuwong 2012-06-08 14:49:14 PDT
Created attachment 146654 [details]
Patch for landing
Comment 26 WebKit Review Bot 2012-06-09 02:05:39 PDT
Comment on attachment 146654 [details]
Patch for landing

Clearing flags on attachment: 146654

Committed r119895: <http://trac.webkit.org/changeset/119895>
Comment 27 WebKit Review Bot 2012-06-09 02:05:46 PDT
All reviewed patches have been landed.  Closing bug.