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+

Jocelyn Turcotte
Reported 2013-02-14 07:15:53 PST
[Qt][WK2] Allow QtWebContext to call directly to QQuickWebView with a WKPageRef
Attachments
Patch (11.99 KB, patch)
2013-02-14 07:18 PST, Jocelyn Turcotte 		allan.jensen: review+
DetailsFormatted DiffDiff
View All Add attachment proposed patch, testcase, etc.
Jocelyn Turcotte
Comment 1 2013-02-14 07:18:18 PST
Created attachment 188345 [details] Patch
Allan Sandfeld Jensen
Comment 2 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.
Jocelyn Turcotte
Comment 3 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.
Allan Sandfeld Jensen
Comment 4 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.
Allan Sandfeld Jensen
Comment 5 2013-02-14 08:45:38 PST
Comment on attachment 188345 [details] Patch LGTM.
Build Bot
Comment 6 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
Allan Sandfeld Jensen
Comment 7 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.
Simon Hausmann
Comment 8 2013-02-20 06:59:26 PST
Benjamin, can you help us sign off on this one, too? :)
Simon Hausmann
Comment 9 2013-02-27 01:00:18 PST
WK2-sign-off-ping :)
Benjamin Poulain
Comment 10 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? :(
Jocelyn Turcotte
Comment 11 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.
Benjamin Poulain
Comment 12 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.
Simon Hausmann
Comment 13 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 :)
Jocelyn Turcotte
Comment 14 2013-03-12 04:15:57 PDT
Committed r145515: <http://trac.webkit.org/changeset/145515>
Note You need to log in before you can comment on or make changes to this bug.