| Summary: | [EFL][Qt][WK2] Implement shared undo controller for EFL and Qt port | ||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Michal Pakula vel Rutka <mpakulavelrutka> | ||||||||||||||||||
| Component: | WebKit EFL | Assignee: | 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
Michal Pakula vel Rutka
2012-07-27 07:15:56 PDT
Created attachment 154940 [details]
proposed patch
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.
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. 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. (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. (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. Created attachment 155759 [details]
fixed patch
Fixed changelog, copyright order, access specifiers according to comments.
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. 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*? (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. Comment on attachment 155759 [details]
fixed patch
Then let's take this out of the review queue for another iteration :)
(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. 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 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.
(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 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. 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 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 :) Created attachment 163123 [details]
rebased patch
Comment on attachment 163123 [details] rebased patch Attachment 163123 [details] did not pass qt-wk2-ews (qt): Output: http://queues.webkit.org/results/13804586 Comment on attachment 163123 [details]
rebased patch
Please unskip the corresponding test cases if they pass.
Created attachment 163141 [details]
rebased patch2
fixed build break in QtPageClient
Looks good to me. Please check if there are unskip the test cases by this patch, and request review. 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.
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 Comment on attachment 163321 [details]
added layout tests gardening
Looks good to me too except for Greg's comment.
Created attachment 163821 [details]
fixed changelog typo
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. Created attachment 163823 [details]
change log fixes
Comment on attachment 163823 [details] change log fixes Clearing flags on attachment: 163823 Committed r128436: <http://trac.webkit.org/changeset/128436> All reviewed patches have been landed. Closing bug. |