Bug 92504 – [EFL][Qt][WK2] Implement shared undo controller for EFL and Qt port

• Component: WebKit EFL
• Status: RESOLVED FIXED

[Michal Pakula vel Rutka]

EFL WebKit2 currently lacks of undo stack, this implementation bases on WebUndoController from WebKit2 QT port.

[Michal Pakula vel Rutka]

Created attachment 154940 [details]
proposed patch

[WebKit Review Bot]

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 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 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]

(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]

(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]

Created attachment 155759 [details]
fixed patch

Fixed changelog, copyright order, access specifiers according to comments.

[Chris Dumez]

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]

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]

(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 on attachment 155759 [details]
fixed patch

Then let's take this out of the review queue for another iteration :)

[Michal Pakula vel Rutka]

(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]

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]

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]

(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 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 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 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]

Created attachment 163123 [details]
rebased patch

[Early Warning System Bot]

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 on attachment 163123 [details]
rebased patch

Please unskip the corresponding test cases if they pass.

[Michal Pakula vel Rutka]

Created attachment 163141 [details]
rebased patch2

fixed build break in QtPageClient

[Gyuyoung Kim]

Looks good to me. Please check if there are unskip the test cases by this patch, and request review.

[Michal Pakula vel Rutka]

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 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 on attachment 163321 [details]
added layout tests gardening

Looks good to me too except for Greg's comment.

[Michal Pakula vel Rutka]

Created attachment 163821 [details]
fixed changelog typo

[Kenneth Rohde Christiansen]

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]

Created attachment 163823 [details]
change log fixes

[WebKit Review Bot]

Comment on attachment 163823 [details]
change log fixes

Clearing flags on attachment: 163823

Committed r128436: <http://trac.webkit.org/changeset/128436>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.