Bug 38939 - [Qt] Implement a detachCurrentPage method for QGraphicsWebView and QWebView
Summary: [Qt] Implement a detachCurrentPage method for QGraphicsWebView and QWebView
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit Qt (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC All
: P2 Normal
Assignee: Antonio Gomes
URL:
Keywords: Qt, QtTriaged
Depends on:
Blocks:
 
Reported: 2010-05-11 15:21 PDT by Antonio Gomes
Modified: 2010-05-12 08:08 PDT (History)
3 users (show)

See Also:


Attachments
patch v1 (9.07 KB, patch)
2010-05-11 15:30 PDT, Antonio Gomes
kenneth: review+
tonikitoo: commit-queue-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Antonio Gomes 2010-05-11 15:21:12 PDT
This detachPage method extands the current unsetPageIfExists implementation, by being responsible of unseting the view and client of the |page| -- it was done in QWebView and QGraphicsWebView destructor routines previously.

work is basically a code clean up and was idealized by kenneth originally.

patch coming...
Comment 1 Antonio Gomes 2010-05-11 15:30:38 PDT
Created attachment 55764 [details]
patch v1
Comment 2 Jesus Sanchez-Palencia 2010-05-11 15:36:22 PDT
(In reply to comment #1)
> Created an attachment (id=55764) [details]
> patch v1


LGTM, but from our earlier chat I thought that "if (page->d->client && page->d->client->isQWidgetClient())" was going to be removed from QGraphicsWebViewPrivate::unsetPageIfExists() .

Did you miss that or am I just wrong?!
Comment 3 Antonio Gomes 2010-05-11 19:08:24 PDT
(In reply to comment #2)
> (In reply to comment #1)
> > Created an attachment (id=55764) [details] [details]
> > patch v1
> 
> 
> LGTM, but from our earlier chat I thought that "if (page->d->client && page->d->client->isQWidgetClient())" was going to be removed from QGraphicsWebViewPrivate::unsetPageIfExists() .
> 
> Did you miss that or am I just wrong?!

Thank you, Jesus. The other patch is coming next to this, in a separated pAtch
Comment 4 Kenneth Rohde Christiansen 2010-05-12 07:27:36 PDT
Comment on attachment 55764 [details]
patch v1

Maybe use detachCurrentPage? I would also like an autotest actually testing the fix you did for always clearing the view when setting a new page, but it can be a separate patch.
Comment 5 Antonio Gomes 2010-05-12 08:08:37 PDT
Committed r59233: <http://trac.webkit.org/changeset/59233>