Bug 109829 – [Qt][WK2] Allow QtWebContext to call directly to QQuickWebView with a WKPageRef

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

[Jocelyn Turcotte]

[Qt][WK2] Allow QtWebContext to call directly to QQuickWebView with a WKPageRef

[Jocelyn Turcotte]

Created attachment 188345 [details]
Patch

[Allan Sandfeld Jensen]

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.

[Jocelyn Turcotte]

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

[Allan Sandfeld Jensen]

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

[Allan Sandfeld Jensen]

Comment on attachment 188345 [details]
Patch

LGTM.

[Build Bot]

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

[Allan Sandfeld Jensen]

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.

[Simon Hausmann]

Benjamin, can you help us sign off on this one, too? :)

[Simon Hausmann]

WK2-sign-off-ping :)

[Benjamin Poulain]

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

[Jocelyn Turcotte]

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

[Benjamin Poulain]

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

[Simon Hausmann]

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

[Jocelyn Turcotte]

Committed r145515: <http://trac.webkit.org/changeset/145515>