Bug 62809 - [Qt] [WK2] Support undo/redo in MiniBrowser
Summary: [Qt] [WK2] Support undo/redo in MiniBrowser
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: 528+ (Nightly build)
Hardware: All Linux
: P2 Normal
Assignee: Chang Shu
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-06-16 13:10 PDT by Chang Shu
Modified: 2011-06-17 08:34 PDT (History)
1 user (show)

See Also:

Attachments
fix patch (4.33 KB, patch)
2011-06-16 13:18 PDT, Chang Shu 		kling: review-
Details | Formatted Diff | Diff
fix patch 2 (3.19 KB, patch)
2011-06-17 06:49 PDT, Chang Shu 		no flags Details | Formatted Diff | Diff
Show Obsolete (1) View All Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
Description Chang Shu 2011-06-16 13:10:14 PDT 
Add UI support of undo/redo in MiniBrowser and WebKit2.
Comment 1 Chang Shu 2011-06-16 13:18:16 PDT 
Created attachment 97482 [details]
fix patch
Comment 2 Yael 2011-06-16 13:40:32 PDT 
Comment on attachment 97482 [details]
fix patch

View in context: https://bugs.webkit.org/attachment.cgi?id=97482&action=review

> Source/WebKit2/UIProcess/API/qt/qwkpage.cpp:875
> +QUndoStack* QWKPage::undoStack() const

Who is using this new API?
Comment 3 Chang Shu 2011-06-16 13:42:18 PDT 
(In reply to comment #2)
> (From update of attachment 97482 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=97482&action=review
> > Source/WebKit2/UIProcess/API/qt/qwkpage.cpp:875
> > +QUndoStack* QWKPage::undoStack() const
> Who is using this new API?

Just in my patch. But it's true that we can remove it and use d->undoStack.
Comment 4 Andreas Kling 2011-06-17 03:26:17 PDT 
Comment on attachment 97482 [details]
fix patch

View in context: https://bugs.webkit.org/attachment.cgi?id=97482&action=review

> Source/WebKit2/UIProcess/API/qt/qwkpage.h:131
> +    bool isModified() const;
> +#ifndef QT_NO_UNDOSTACK
> +    QUndoStack* undoStack() const;
> +#endif

undoStack() should be replaced by d->undoStack inside QWKPage.
Why do we need isModified() and the 'modified' property?

> Tools/MiniBrowser/qt/BrowserWindow.cpp:89
> +    undo->setShortcut(QKeySequence(Qt::CTRL | Qt::Key_Z));

Use the QKeySequence constructor that takes a StandardKey (QKeySequence::Undo in this case.)

> Tools/MiniBrowser/qt/BrowserWindow.cpp:92
> +    redo->setShortcut(QKeySequence(Qt::CTRL | Qt::SHIFT | Qt::Key_Z));

Ditto (QKeySequence::Redo.)
Comment 5 Chang Shu 2011-06-17 06:49:19 PDT 
> > Source/WebKit2/UIProcess/API/qt/qwkpage.h:131
> > +    bool isModified() const;
> > +#ifndef QT_NO_UNDOSTACK
> > +    QUndoStack* undoStack() const;
> > +#endif
> 
> undoStack() should be replaced by d->undoStack inside QWKPage.
> Why do we need isModified() and the 'modified' property?

Right. We don't seem to need isModified(). It was in WebKit1, though.
Comment 6 Chang Shu 2011-06-17 06:49:48 PDT 
Created attachment 97592 [details]
fix patch 2
Comment 7 Andreas Kling 2011-06-17 07:53:06 PDT 
Comment on attachment 97592 [details]
fix patch 2

LGTM!
Comment 8 WebKit Review Bot 2011-06-17 08:34:25 PDT 
Comment on attachment 97592 [details]
fix patch 2

Clearing flags on attachment: 97592

Committed r89139: <http://trac.webkit.org/changeset/89139>
Comment 9 WebKit Review Bot 2011-06-17 08:34:30 PDT 
All reviewed patches have been landed.  Closing bug.