Bug 74691 - [Qt] Eliminate dependency to QUndoStack
Summary: [Qt] Eliminate dependency to QUndoStack
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Simon Hausmann
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-12-16 00:09 PST by Simon Hausmann
Modified: 2011-12-16 12:55 PST (History)
5 users (show)

See Also:

Attachments
Patch (5.20 KB, patch)
2011-12-16 00:12 PST, Simon Hausmann 		no flags Details | Formatted Diff | Diff
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 Simon Hausmann 2011-12-16 00:09:43 PST 
[Qt] Eliminate dependency to QUndoStack
Comment 1 Simon Hausmann 2011-12-16 00:12:02 PST 
Created attachment 119577 [details]
Patch
Comment 2 Simon Hausmann 2011-12-16 00:12:59 PST 
Once this is in, the next step is to get rid of the class entirely and just fold the few lines of code straight to the page client, if that's okay with you folks :)
Comment 3 Caio Marcelo de Oliveira Filho 2011-12-16 05:51:08 PST 
Looks good to me. I don't think we are getting any benefit from using QUndoStack.

Regarding moving this to QtPageClient, I like the fact that QtPageClient is super-thin and just forward things to the proper object handling them. I'm afraid folding even this small chunk of code will put page client in another route... but that could be me just being a bit over-cautious ;-)
Comment 4 Alexis Menard (darktears) 2011-12-16 06:49:27 PST 
(In reply to comment #3)
> Looks good to me. I don't think we are getting any benefit from using QUndoStack.
> 
> Regarding moving this to QtPageClient, I like the fact that QtPageClient is super-thin and just forward things to the proper object handling them. I'm afraid folding even this small chunk of code will put page client in another route... but that could be me just being a bit over-cautious ;-)

+1 tend to agree with that. This is exactly how we ended up with QtWebPageProxy.
Comment 5 Simon Hausmann 2011-12-16 12:06:37 PST 
(In reply to comment #3)
> Looks good to me. I don't think we are getting any benefit from using QUndoStack.
> 
> Regarding moving this to QtPageClient, I like the fact that QtPageClient is super-thin and just forward things to the proper object handling them. I'm afraid folding even this small chunk of code will put page client in another route... but that could be me just being a bit over-cautious ;-)

I understand your fear, but I'm not sure if it is really another classes responsibility to handle the undo command storage. It is the _intend_ of the C API that the page ui client handles it, so why should we delegate it even further?

The QtWebPageProxy became big because it implemented multiple client APIs in one shot.

Anyway, I'll try to convince you with code :) - in the meantime we need to get this patch reviewed :)
Comment 6 Kenneth Rohde Christiansen 2011-12-16 12:12:53 PST 
Comment on attachment 119577 [details]
Patch

im fine with it.
Comment 7 Simon Hausmann 2011-12-16 12:40:48 PST 
Comment on attachment 119577 [details]
Patch

Thanks Kenneth :)
Comment 8 WebKit Review Bot 2011-12-16 12:55:26 PST 
Comment on attachment 119577 [details]
Patch

Clearing flags on attachment: 119577

Committed r103097: <http://trac.webkit.org/changeset/103097>
Comment 9 WebKit Review Bot 2011-12-16 12:55:31 PST 
All reviewed patches have been landed.  Closing bug.