Bug 108826 - [Qt][WK2] Replace more uses of WebPageProxy with WKPage in QQuickWebView
Summary: [Qt][WK2] Replace more uses of WebPageProxy with WKPage in QQuickWebView
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: 108471
  Show dependency treegraph
 
Reported: 2013-02-04 08:08 PST by Simon Hausmann
Modified: 2013-02-05 03:00 PST (History)
6 users (show)

See Also:


Attachments
Patch (12.72 KB, patch)
2013-02-04 08:10 PST, Simon Hausmann
kenneth: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Simon Hausmann 2013-02-04 08:08:12 PST
[Qt][WK2] Replace more uses of WebPageProxy with WKPage in QQuickWebView
Comment 1 Simon Hausmann 2013-02-04 08:10:04 PST
Created attachment 186377 [details]
Patch
Comment 2 Kenneth Rohde Christiansen 2013-02-04 08:15:13 PST
Comment on attachment 186377 [details]
Patch

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

LGTM

> Source/WebKit2/UIProcess/API/qt/qquickwebview.cpp:1016
> +    static WKStringRef messageName = WKStringCreateWithUTF8CString("MessageToNavigatorQtObject");

hmm ok, so static and no adoption here... guess that is fine

> Source/WebKit2/UIProcess/API/qt/qquickwebview.cpp:1514
> +    WKPageGoBack(d->webPage.get());

why not add an inline wkPage() const accessor?
Comment 3 Simon Hausmann 2013-02-05 00:37:25 PST
Comment on attachment 186377 [details]
Patch

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

>> Source/WebKit2/UIProcess/API/qt/qquickwebview.cpp:1514
>> +    WKPageGoBack(d->webPage.get());
> 
> why not add an inline wkPage() const accessor?

Not sure that it's worth it :)
Comment 4 Benjamin Poulain 2013-02-05 01:07:29 PST
Comment on attachment 186377 [details]
Patch

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

This looks like a step in the right direction. I sign off on this for WebKit2.

> Source/WebKit2/UIProcess/API/qt/qquickwebview_p_p.h:165
>      RefPtr<WebKit::WebPageProxy> webPageProxy;
> +    WKRetainPtr<WKPageRef> webPage;

Why keep a reference to webPageProxy? This defeats the purpose. :(
Comment 5 Simon Hausmann 2013-02-05 01:17:03 PST
Comment on attachment 186377 [details]
Patch

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

Thanks for the sign-off, Benjamin!

>> Source/WebKit2/UIProcess/API/qt/qquickwebview_p_p.h:165
>> +    WKRetainPtr<WKPageRef> webPage;
> 
> Why keep a reference to webPageProxy? This defeats the purpose. :(

There are still a few references left, our plan is to definitely get rid of this member as the last step of the increments.
Comment 6 Kenneth Rohde Christiansen 2013-02-05 02:14:38 PST
Comment on attachment 186377 [details]
Patch

r=me as signed off by Benjamin
Comment 7 Simon Hausmann 2013-02-05 03:00:35 PST
Committed r141882: <http://trac.webkit.org/changeset/141882>