Bug 92504

Summary: [EFL][Qt][WK2] Implement shared undo controller for EFL and Qt port
Product: WebKit Reporter: Michal Pakula vel Rutka <mpakulavelrutka>
Component: WebKit EFLAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: abecsi, cdumez, cmarcelo, gyuyoung.kim, hausmann, kenneth, lucas.de.marchi, menard, rakuco, webkit.review.bot, zoltan
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: PC   
OS: Linux   
Bug Depends on: 95947    
Bug Blocks:    
Attachments:
Description Flags
proposed patch
none
fixed patch
hausmann: review-
new approach with shared undo controller
hausmann: review-, webkit-ews: commit-queue-
rebased patch
webkit-ews: commit-queue-
rebased patch2
none
added layout tests gardening
none
fixed changelog typo
kenneth: review+, kenneth: commit-queue-
change log fixes none

Michal Pakula vel Rutka
Reported 2012-07-27 07:15:56 PDT
EFL WebKit2 currently lacks of undo stack, this implementation bases on WebUndoController from WebKit2 QT port.
Attachments
proposed patch (8.21 KB, patch)
2012-07-27 07:17 PDT, Michal Pakula vel Rutka
no flags
fixed patch (8.21 KB, patch)
2012-08-01 02:05 PDT, Michal Pakula vel Rutka
hausmann: review-
new approach with shared undo controller (16.41 KB, patch)
2012-09-07 06:54 PDT, Michal Pakula vel Rutka
hausmann: review-
webkit-ews: commit-queue-
rebased patch (16.29 KB, patch)
2012-09-10 07:14 PDT, Michal Pakula vel Rutka
webkit-ews: commit-queue-
rebased patch2 (17.22 KB, patch)
2012-09-10 08:46 PDT, Michal Pakula vel Rutka
no flags
added layout tests gardening (65.43 KB, patch)
2012-09-11 03:21 PDT, Michal Pakula vel Rutka
no flags
fixed changelog typo (65.54 KB, patch)
2012-09-13 02:28 PDT, Michal Pakula vel Rutka
kenneth: review+
kenneth: commit-queue-
change log fixes (65.57 KB, patch)
2012-09-13 02:46 PDT, Michal Pakula vel Rutka
no flags
Michal Pakula vel Rutka
Comment 1 2012-07-27 07:17:22 PDT
Created attachment 154940 [details] proposed patch
WebKit Review Bot
Comment 2 2012-07-27 07:37:30 PDT
Attachment 154940 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebKit2/ChangeLog', u'Source/WebKit..." exit_code: 1 Source/WebKit2/ChangeLog:1: ChangeLog entry has no bug number [changelog/bugnumber] [5] Total errors found: 1 in 6 files If any of these errors are false positives, please file a bug against check-webkit-style.
Gyuyoung Kim
Comment 3 2012-07-27 09:47:44 PDT
Comment on attachment 154940 [details] proposed patch View in context: https://bugs.webkit.org/attachment.cgi?id=154940&action=review > Source/WebKit2/ChangeLog:4 > + https://bugs.webkit.org/show_bug.cgi?id=XXXXX Wrong bug url. > Source/WebKit2/UIProcess/efl/WebUndoControllerEfl.cpp:2 > + Copyright (C) 2012 Samsung Electronics Generally, copyright of latest year is placed below. > Source/WebKit2/UIProcess/efl/WebUndoControllerEfl.cpp:6 > + This library is free software; you can redistribute it and/or If possible, could you use BSD license instead of LGPL. I was told BSD is more useful.
Chris Dumez
Comment 4 2012-07-27 10:08:27 PDT
Comment on attachment 154940 [details] proposed patch View in context: https://bugs.webkit.org/attachment.cgi?id=154940&action=review > Source/WebKit2/ChangeLog:8 > + Add an undo stack for WebKit2 EFL basing on WebUndoController from QT port. "QtWebUndoController from Qt port". >> Source/WebKit2/UIProcess/efl/WebUndoControllerEfl.cpp:6 >> + This library is free software; you can redistribute it and/or > > If possible, could you use BSD license instead of LGPL. I was told BSD is more useful. Considering there are other copyright holders on this file (Nokia, Staikos), he cannot change license like that. > Source/WebKit2/UIProcess/efl/WebUndoControllerEfl.h:32 > + void registerEditCommand(PassRefPtr<WebKit::WebEditCommandProxy>, WebKit::WebPageProxy::UndoOrRedo); You don't need all the WebKit:: in this file. > Source/WebKit2/UIProcess/efl/WebUndoControllerEfl.h:38 > + CommandVector m_undoStack; Should probably be private. > Source/WebKit2/UIProcess/efl/WebUndoControllerEfl.h:39 > + CommandVector m_redoStack; Ditto.
Michal Pakula vel Rutka
Comment 5 2012-08-01 02:02:27 PDT
(In reply to comment #3) > (From update of attachment 154940 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=154940&action=review > > > Source/WebKit2/ChangeLog:4 > > + https://bugs.webkit.org/show_bug.cgi?id=XXXXX > > Wrong bug url. > My bad, fixed. > > Source/WebKit2/UIProcess/efl/WebUndoControllerEfl.cpp:2 > > + Copyright (C) 2012 Samsung Electronics > > Generally, copyright of latest year is placed below. > I have changed all copyrights order to ascending. > > Source/WebKit2/UIProcess/efl/WebUndoControllerEfl.cpp:6 > > + This library is free software; you can redistribute it and/or > > If possible, could you use BSD license instead of LGPL. I was told BSD is more useful.
Michal Pakula vel Rutka
Comment 6 2012-08-01 02:03:08 PDT
(In reply to comment #4) > (From update of attachment 154940 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=154940&action=review > > > Source/WebKit2/ChangeLog:8 > > + Add an undo stack for WebKit2 EFL basing on WebUndoController from QT port. > > "QtWebUndoController from Qt port". > > >> Source/WebKit2/UIProcess/efl/WebUndoControllerEfl.cpp:6 > >> + This library is free software; you can redistribute it and/or > > > > If possible, could you use BSD license instead of LGPL. I was told BSD is more useful. > > Considering there are other copyright holders on this file (Nokia, Staikos), he cannot change license like that. > > > Source/WebKit2/UIProcess/efl/WebUndoControllerEfl.h:32 > > + void registerEditCommand(PassRefPtr<WebKit::WebEditCommandProxy>, WebKit::WebPageProxy::UndoOrRedo); > > You don't need all the WebKit:: in this file. > Done. > > Source/WebKit2/UIProcess/efl/WebUndoControllerEfl.h:38 > > + CommandVector m_undoStack; > > Should probably be private. > > > Source/WebKit2/UIProcess/efl/WebUndoControllerEfl.h:39 > > + CommandVector m_redoStack; > > Ditto. Both moved to private.
Michal Pakula vel Rutka
Comment 7 2012-08-01 02:05:29 PDT
Created attachment 155759 [details] fixed patch Fixed changelog, copyright order, access specifiers according to comments.
Chris Dumez
Comment 8 2012-08-01 03:42:07 PDT
Comment on attachment 155759 [details] fixed patch View in context: https://bugs.webkit.org/attachment.cgi?id=155759&action=review LGTM. > Source/WebKit2/ChangeLog:8 > + Add an undo stack for WebKit2 EFL basing on QTWebUndoController from QT port. For future reference, QT != Qt. I don't think you're referring to QuickTime.
Simon Hausmann
Comment 9 2012-08-27 12:16:17 PDT
This is a good example where we can share code between EFL and Qt instead of duplicating it. How about Source/WebKit2/UIProcess/DefaultUndoController.cpp/h for example? or Simple*?
Gyuyoung Kim
Comment 10 2012-08-27 18:14:59 PDT
(In reply to comment #9) > This is a good example where we can share code between EFL and Qt instead of duplicating it. How about Source/WebKit2/UIProcess/DefaultUndoController.cpp/h for example? or Simple*? I agree to Simon suggestion. I think this is the way to go.
Simon Hausmann
Comment 11 2012-08-27 23:06:09 PDT
Comment on attachment 155759 [details] fixed patch Then let's take this out of the review queue for another iteration :)
Michal Pakula vel Rutka
Comment 12 2012-09-04 02:59:20 PDT
(In reply to comment #9) > This is a good example where we can share code between EFL and Qt instead of duplicating it. How about Source/WebKit2/UIProcess/DefaultUndoController.cpp/h for example? or Simple*? Sounds good to me also, I will prepare a new patch soon.
Chris Dumez
Comment 13 2012-09-06 00:58:56 PDT
Please check if you can unskip test cases with your patch. Those are likely candidates: fast/forms/plaintext-mode-1.html editing/execCommand/delete-selection-has-style.html editing/execCommand/format-block-multiple-paragraphs-in-pre.html fast/css/relative-positioned-block-crash.html
Michal Pakula vel Rutka
Comment 14 2012-09-07 06:54:40 PDT
Created attachment 162758 [details] new approach with shared undo controller As Simon suggested to share the implementation I have moved the undo controller to UIProcess with some minor changes, so now it can be used by both ports.
Michal Pakula vel Rutka
Comment 15 2012-09-07 07:09:04 PDT
(In reply to comment #13) > Please check if you can unskip test cases with your patch. Those are likely candidates: > fast/forms/plaintext-mode-1.html > editing/execCommand/delete-selection-has-style.html > editing/execCommand/format-block-multiple-paragraphs-in-pre.html > fast/css/relative-positioned-block-crash.html Yes, you are right, they are passing after this patch is applied
Andras Becsi
Comment 16 2012-09-07 07:24:54 PDT
Comment on attachment 162758 [details] new approach with shared undo controller View in context: https://bugs.webkit.org/attachment.cgi?id=162758&action=review > Source/WebKit2/Target.pri:787 > - UIProcess/qt/QtWebUndoController.h \ > + UIProcess/qt/QtViewportHandler.h \ This hunk does not seem right to me. There is no QtViewportHandler.h any more, probably slipped in when you rebased your patch.
Early Warning System Bot
Comment 17 2012-09-07 07:31:09 PDT
Comment on attachment 162758 [details] new approach with shared undo controller Attachment 162758 [details] did not pass qt-wk2-ews (qt): Output: http://queues.webkit.org/results/13776704
Simon Hausmann
Comment 18 2012-09-08 04:00:19 PDT
Comment on attachment 162758 [details] new approach with shared undo controller View in context: https://bugs.webkit.org/attachment.cgi?id=162758&action=review >> Source/WebKit2/Target.pri:787 >> + UIProcess/qt/QtViewportHandler.h \ > > This hunk does not seem right to me. There is no QtViewportHandler.h any more, probably slipped in when you rebased your patch. Looks overall good, but it's going to need a rebase to adjust to the changes on the Qt side, as Andras pointed out :)
Michal Pakula vel Rutka
Comment 19 2012-09-10 07:14:22 PDT
Created attachment 163123 [details] rebased patch
Early Warning System Bot
Comment 20 2012-09-10 07:59:04 PDT
Comment on attachment 163123 [details] rebased patch Attachment 163123 [details] did not pass qt-wk2-ews (qt): Output: http://queues.webkit.org/results/13804586
Chris Dumez
Comment 21 2012-09-10 08:31:11 PDT
Comment on attachment 163123 [details] rebased patch Please unskip the corresponding test cases if they pass.
Michal Pakula vel Rutka
Comment 22 2012-09-10 08:46:15 PDT
Created attachment 163141 [details] rebased patch2 fixed build break in QtPageClient
Gyuyoung Kim
Comment 23 2012-09-10 17:33:17 PDT
Looks good to me. Please check if there are unskip the test cases by this patch, and request review.
Michal Pakula vel Rutka
Comment 24 2012-09-11 03:21:47 PDT
Created attachment 163321 [details] added layout tests gardening Unskipped whole editing/undo category, added missing expected results and test expectations for two flaky tests in WebKit1.
Grzegorz Czajkowski
Comment 25 2012-09-13 01:13:46 PDT
Comment on attachment 163321 [details] added layout tests gardening LGTM. View in context: https://bugs.webkit.org/attachment.cgi?id=163321&action=review > LayoutTests/ChangeLog:10 > + where added. where -> were
Gyuyoung Kim
Comment 26 2012-09-13 01:48:20 PDT
Comment on attachment 163321 [details] added layout tests gardening Looks good to me too except for Greg's comment.
Michal Pakula vel Rutka
Comment 27 2012-09-13 02:28:44 PDT
Created attachment 163821 [details] fixed changelog typo
Kenneth Rohde Christiansen
Comment 28 2012-09-13 02:33:38 PDT
Comment on attachment 163821 [details] fixed changelog typo View in context: https://bugs.webkit.org/attachment.cgi?id=163821&action=review > LayoutTests/ChangeLog:10 > + After WebKit2 undo implementation is done, whole editing/undo > + category can be unskipped. Also missing expected results > + were added. You should write it differently Unskip the editing/undo category now that the tests are passing. Also add missing expected results.
Michal Pakula vel Rutka
Comment 29 2012-09-13 02:46:32 PDT
Created attachment 163823 [details] change log fixes
WebKit Review Bot
Comment 30 2012-09-13 03:10:41 PDT
Comment on attachment 163823 [details] change log fixes Clearing flags on attachment: 163823 Committed r128436: <http://trac.webkit.org/changeset/128436>
WebKit Review Bot
Comment 31 2012-09-13 03:10:47 PDT
All reviewed patches have been landed. Closing bug.
Note You need to log in before you can comment on or make changes to this bug.