Bug 108826

Summary: [Qt][WK2] Replace more uses of WebPageProxy with WKPage in QQuickWebView
Product: WebKit Reporter: Simon Hausmann <hausmann>
Component: New BugsAssignee: Simon Hausmann <hausmann>
Status: RESOLVED FIXED    
Severity: Normal CC: abecsi, benjamin, cmarcelo, kling, menard, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 108471    
Attachments:
Description Flags
Patch kenneth: review+

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>