Bug 109829 - [Qt][WK2] Allow QtWebContext to call directly to QQuickWebView with a WKPageRef
Summary: [Qt][WK2] Allow QtWebContext to call directly to QQuickWebView with a WKPageRef
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: Jocelyn Turcotte
URL:
Keywords:
Depends on: 109827
Blocks: 108475
  Show dependency treegraph
 
Reported: 2013-02-14 07:15 PST by Jocelyn Turcotte
Modified: 2013-03-12 04:15 PDT (History)
10 users (show)

See Also:


Attachments
Patch (11.99 KB, patch)
2013-02-14 07:18 PST, Jocelyn Turcotte
allan.jensen: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
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>