Bug 74691 – [Qt] Eliminate dependency to QUndoStack

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Simon Hausmann]

[Qt] Eliminate dependency to QUndoStack

[Simon Hausmann]

Created attachment 119577 [details]
Patch

[Simon Hausmann]

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 :)

[Caio Marcelo de Oliveira Filho]

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 ;-)

[Alexis Menard (darktears)]

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

[Simon Hausmann]

(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 :)

[Kenneth Rohde Christiansen]

Comment on attachment 119577 [details]
Patch

im fine with it.

[Simon Hausmann]

Comment on attachment 119577 [details]
Patch

Thanks Kenneth :)

[WebKit Review Bot]

Comment on attachment 119577 [details]
Patch

Clearing flags on attachment: 119577

Committed r103097: <http://trac.webkit.org/changeset/103097>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.