|Summary:||Add UNDO_MANAGER build flag|
|Product:||WebKit||Reporter:||Sukolsak Sakshuwong <sukolsak>|
|Component:||HTML Editing||Assignee:||Sukolsak Sakshuwong <sukolsak>|
|Severity:||Normal||CC:||abarth, dbates, enrica, eric, gns, morrita, mrobinson, ojan, philn, rakeshchaitan, rakuco, rniwa, tkent, tony, vestbo, webkit.review.bot, xan.lopez, zan|
|Version:||528+ (Nightly build)|
|Bug Depends on:|
Description Sukolsak Sakshuwong 2012-05-30 16:06:28 PDT
Add UNDO_MANAGER flag
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 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 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 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 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 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 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.