Add UNDO_MANAGER flag
Created attachment 144968 [details] Patch
(In reply to comment #1) > Created an attachment (id=144968) [details] > Patch You need to update your checkout and resolve the conflicts.
Created attachment 145155 [details] Patch
Created attachment 145161 [details] Patch
Reviewers: I can't review this patch because much of the change is based on a git-diff I gave to Sukolsak.
Maybe you can add a RuntimeEnabledFeatures flag then the feature can be enabled only for the tests.
(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?
(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.
Created attachment 145632 [details] Patch
Created attachment 145635 [details] Patch
Zan has been working closely with our build flags lately. I'd like him to double-check this patch.
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).
Created attachment 145651 [details] Patch
Thanks for the quick CMake fixes!
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 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.
Created attachment 145866 [details] Patch
Comment on attachment 145866 [details] Patch Attachment 145866 [details] did not pass gtk-ews (gtk): Output: http://queues.webkit.org/results/12903626
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.
Created attachment 145874 [details] Patch
Created attachment 145879 [details] Patch
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.
Created attachment 146591 [details] Patch
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.
Created attachment 146654 [details] Patch for landing
Comment on attachment 146654 [details] Patch for landing Clearing flags on attachment: 146654 Committed r119895: <http://trac.webkit.org/changeset/119895>
All reviewed patches have been landed. Closing bug.