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

Description Michal Pakula vel Rutka 2012-07-27 07:15:56 PDT
EFL WebKit2 currently lacks of undo stack, this implementation bases on WebUndoController from WebKit2 QT port.
Comment 1 Michal Pakula vel Rutka 2012-07-27 07:17:22 PDT
Created attachment 154940 [details]
proposed patch
Comment 2 WebKit Review Bot 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.
Comment 3 Gyuyoung Kim 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.
Comment 4 Chris Dumez 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.
Comment 5 Michal Pakula vel Rutka 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.
Comment 6 Michal Pakula vel Rutka 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.
Comment 7 Michal Pakula vel Rutka 2012-08-01 02:05:29 PDT
Created attachment 155759 [details]
fixed patch

Fixed changelog, copyright order, access specifiers according to comments.
Comment 8 Chris Dumez 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.
Comment 9 Simon Hausmann 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*?
Comment 10 Gyuyoung Kim 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.
Comment 11 Simon Hausmann 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 :)
Comment 12 Michal Pakula vel Rutka 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.
Comment 13 Chris Dumez 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
Comment 14 Michal Pakula vel Rutka 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.
Comment 15 Michal Pakula vel Rutka 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
Comment 16 Andras Becsi 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.
Comment 17 Early Warning System Bot 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
Comment 18 Simon Hausmann 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 :)
Comment 19 Michal Pakula vel Rutka 2012-09-10 07:14:22 PDT
Created attachment 163123 [details]
rebased patch
Comment 20 Early Warning System Bot 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
Comment 21 Chris Dumez 2012-09-10 08:31:11 PDT
Comment on attachment 163123 [details]
rebased patch

Please unskip the corresponding test cases if they pass.
Comment 22 Michal Pakula vel Rutka 2012-09-10 08:46:15 PDT
Created attachment 163141 [details]
rebased patch2

fixed build break in QtPageClient
Comment 23 Gyuyoung Kim 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.
Comment 24 Michal Pakula vel Rutka 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.
Comment 25 Grzegorz Czajkowski 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
Comment 26 Gyuyoung Kim 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.
Comment 27 Michal Pakula vel Rutka 2012-09-13 02:28:44 PDT
Created attachment 163821 [details]
fixed changelog typo
Comment 28 Kenneth Rohde Christiansen 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.
Comment 29 Michal Pakula vel Rutka 2012-09-13 02:46:32 PDT
Created attachment 163823 [details]
change log fixes
Comment 30 WebKit Review Bot 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>
Comment 31 WebKit Review Bot 2012-09-13 03:10:47 PDT
All reviewed patches have been landed.  Closing bug.