Bug 109829

Summary: [Qt][WK2] Allow QtWebContext to call directly to QQuickWebView with a WKPageRef
Product: WebKit Reporter: Jocelyn Turcotte <jturcotte>
Component: New BugsAssignee: Jocelyn Turcotte <jturcotte>
Status: RESOLVED FIXED    
Severity: Normal CC: abecsi, allan.jensen, benjamin, buildbot, cmarcelo, hausmann, jturcotte, menard, rniwa, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on: 109827    
Bug Blocks: 108475    
Attachments:
Description Flags
Patch allan.jensen: review+

Description Jocelyn Turcotte 2013-02-14 07:15:53 PST
[Qt][WK2] Allow QtWebContext to call directly to QQuickWebView with a WKPageRef
Comment 1 Jocelyn Turcotte 2013-02-14 07:18:18 PST
Created attachment 188345 [details]
Patch
Comment 2 Allan Sandfeld Jensen 2013-02-14 08:17:20 PST
Comment on attachment 188345 [details]
Patch

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

> Source/WebKit2/UIProcess/API/qt/qquickwebview.cpp:92
> +typedef QMap<WKPageRef, QQuickWebViewPrivate*> PageToViewMap;
> +Q_GLOBAL_STATIC(PageToViewMap, pageToView)

Wouldn't an extra pointer in qquickwebviewprivate be more efficient. This doesn't seem like a rare property where we can save memory from using a map over using normal pointers.
Comment 3 Jocelyn Turcotte 2013-02-14 08:31:35 PST
(In reply to comment #2)
> (From update of attachment 188345 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=188345&action=review
> 
> > Source/WebKit2/UIProcess/API/qt/qquickwebview.cpp:92
> > +typedef QMap<WKPageRef, QQuickWebViewPrivate*> PageToViewMap;
> > +Q_GLOBAL_STATIC(PageToViewMap, pageToView)
> 
> Wouldn't an extra pointer in qquickwebviewprivate be more efficient. This doesn't seem like a rare property where we can save memory from using a map over using normal pointers.

We already have a WKPageRef in qquickwebviewprivate.
The matter is more the lookup, I need to find a qquickwebviewprivate using a WKPageRef. The only other solution I see is to keep a vector of all views and iterate them to find my page, that doesn't sound much better.
Comment 4 Allan Sandfeld Jensen 2013-02-14 08:45:10 PST
(In reply to comment #3)
> (In reply to comment #2)
> > (From update of attachment 188345 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=188345&action=review
> > 
> > > Source/WebKit2/UIProcess/API/qt/qquickwebview.cpp:92
> > > +typedef QMap<WKPageRef, QQuickWebViewPrivate*> PageToViewMap;
> > > +Q_GLOBAL_STATIC(PageToViewMap, pageToView)
> > 
> > Wouldn't an extra pointer in qquickwebviewprivate be more efficient. This doesn't seem like a rare property where we can save memory from using a map over using normal pointers.
> 
> We already have a WKPageRef in qquickwebviewprivate.
> The matter is more the lookup, I need to find a qquickwebviewprivate using a WKPageRef. The only other solution I see is to keep a vector of all views and iterate them to find my page, that doesn't sound much better.

Ah, right. I read the map the wrong way.
Comment 5 Allan Sandfeld Jensen 2013-02-14 08:45:38 PST
Comment on attachment 188345 [details]
Patch

LGTM.
Comment 6 Build Bot 2013-02-15 05:05:01 PST
Comment on attachment 188345 [details]
Patch

Attachment 188345 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/16595050

New failing tests:
media/video-controls-captions-trackmenu.html
Comment 7 Allan Sandfeld Jensen 2013-02-15 05:12:31 PST
Comment on attachment 188345 [details]
Patch

LGTM, but needs approval of owners. I am assuming the failure by the mac build-bot is an unmarked flaky test.
Comment 8 Simon Hausmann 2013-02-20 06:59:26 PST
Benjamin, can you help us sign off on this one, too? :)
Comment 9 Simon Hausmann 2013-02-27 01:00:18 PST
WK2-sign-off-ping :)
Comment 10 Benjamin Poulain 2013-02-27 01:06:29 PST
Comment on attachment 188345 [details]
Patch

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

That is great, sorry I did not see it earlier.
I sign of on the change.

> Source/WebKit2/UIProcess/API/qt/qquickwebview.cpp:91
> +typedef QMap<WKPageRef, QQuickWebViewPrivate*> PageToViewMap;

No love for HashTable? :(
Comment 11 Jocelyn Turcotte 2013-02-27 05:27:16 PST
(In reply to comment #10)
> (From update of attachment 188345 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=188345&action=review
> 
> That is great, sorry I did not see it earlier.
> I sign of on the change.
> 
> > Source/WebKit2/UIProcess/API/qt/qquickwebview.cpp:91
> > +typedef QMap<WKPageRef, QQuickWebViewPrivate*> PageToViewMap;
> 
> No love for HashTable? :(

Thanks! :)
I'm trying to use Qt containers to avoid dependencies to WTF.
There will generally be less than 5 pages open and rarely more than 50, so QMap should fit better than QHash, theoretically.
Comment 12 Benjamin Poulain 2013-02-27 12:47:00 PST
> I'm trying to use Qt containers to avoid dependencies to WTF.
> There will generally be less than 5 pages open and rarely more than 50, so QMap should fit better than QHash, theoretically.

In Qt4, QMap was a fairly bad linked list.
Comment 13 Simon Hausmann 2013-02-27 13:36:01 PST
(In reply to comment #12)
> > I'm trying to use Qt containers to avoid dependencies to WTF.
> > There will generally be less than 5 pages open and rarely more than 50, so QMap should fit better than QHash, theoretically.
> 
> In Qt4, QMap was a fairly bad linked list.

In QT 5 it's a red black tree again :)
Comment 14 Jocelyn Turcotte 2013-03-12 04:15:57 PDT
Committed r145515: <http://trac.webkit.org/changeset/145515>