Bug 73364 – [Qt][WK2] Expose WebContext as an experimental API and move downloadRequested to it

• Status: RESOLVED
• Resolution: LATER
• Component: WebKit Qt

[Jesus Sanchez-Palencia]

Downloads are not view-dependent but context-dependent instead. We should expose the WebContext through our experimental set of APIs for the time being and move the signal onDownloadRequested into it as a proof of concept.

[Jesus Sanchez-Palencia]

Created attachment 117051 [details]
Patch

[WebKit Review Bot]

Attachment 117051 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'ChangeLog', u'Source/WebKit/qt/ChangeLog',..." exit_code: 1

Source/WebKit2/UIProcess/API/qt/qquickwebcontext_p_p.h:36:  q_ptr is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Source/WebKit2/UIProcess/API/qt/qquickwebcontext_p.h:41:  d_ptr is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Total errors found: 2 in 16 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Alexis Menard (darktears)]

Comment on attachment 117051 [details]
Patch

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

Could be a first step. Maybe later this context should be a "global" object.

> Source/WebKit2/UIProcess/API/qt/qquickwebcontext_p.h:25
> +#include <QtDeclarative/qquickitem.h>

Should be QtQuick

> Source/WebKit2/UIProcess/API/qt/qquickwebcontext_p.h:32
> +

Unneeded space

> Source/WebKit2/UIProcess/qt/QtDownloadManager.cpp:92
> +

Why?

[Jesus Sanchez-Palencia]

Created attachment 119489 [details]
Patch

[WebKit Review Bot]

Attachment 119489 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'ChangeLog', u'Source/WebKit/qt/ChangeLog',..." exit_code: 1

Source/WebKit2/UIProcess/API/qt/qquickwebcontext_p_p.h:36:  q_ptr is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Source/WebKit2/UIProcess/API/qt/qquickwebcontext_p.h:40:  d_ptr is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Total errors found: 2 in 14 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Kenneth Rohde Christiansen]

Comment on attachment 119489 [details]
Patch

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

> Source/WebKit2/ChangeLog:10
> +        The download API is the perfect example for this, so thi patch also moves

thi?

[Simon Hausmann]

Comment on attachment 119489 [details]
Patch

I don't think this is the way we should expose the context, as just another property.

I suppose we _could_ make it a separate entity and move our main webview initialization code to componentComplete(). Then perhaps it would be easier to support an optional _settable_ context property on the view:

WebView {
    id: standaloneView
}

WebContext {
    id: myContext

    onDownloadRequest: { ...}
}

WebView {
    context: myContext
}

WebView {
    id: anotherInstance
    context: myContext
}

What do you think?

[Kenneth Rohde Christiansen]

(In reply to comment #7)
> (From update of attachment 119489 [details])
> I don't think this is the way we should expose the context, as just another property.
> 
> I suppose we _could_ make it a separate entity and move our main webview initialization code to componentComplete(). Then perhaps it would be easier to support an optional _settable_ context property on the view:
> 
> WebView {
>     id: standaloneView
> }
> 
> WebContext {
>     id: myContext
> 
>     onDownloadRequest: { ...}
> }
> 
> WebView {
>     context: myContext
> }
> 
> WebView {
>     id: anotherInstance
>     context: myContext
> }
> 
> What do you think?

Is that easily possible? Also how are we going to handle page groups in the future etc? I think we need to consider all these things

[Jesus Sanchez-Palencia]

Created attachment 121654 [details]
WIP Patch

This is a WIP patch that is in a good shape. I would like to get some "informal" feedback from you guys in order to check if this aligned with your thoughts and if I should keep going or not.

Thanks in advance!

[Jesus Sanchez-Palencia]

Created attachment 121656 [details]
QML test case

Simple QML file to be used as a test case.

[Tor Arne Vestbø]

Just a few questions (out of curiosity): 

 - What are the use-cases for this api?
 - When will you have more than one WebView?
 - Or more than one webcontext?

[Jesus Sanchez-Palencia]

(In reply to comment #9)
> Created an attachment (id=121654) [details]
> WIP Patch


(In reply to comment #10)
> Created an attachment (id=121656) [details]
> QML test case


Please bare in mind that they work on top of the WIP patch available at https://bugs.webkit.org/show_bug.cgi?id=75489 .

[Jesus Sanchez-Palencia]

(In reply to comment #11)
> Just a few questions (out of curiosity): 
> 
>  - What are the use-cases for this api?
Having another entry point in our API for non view-dependent stuff, like downloads for instance. In the future, for creating multiple webcontexts.

>  - When will you have more than one WebView?
Snowshoe uses multiple webviews already, for tabs.

>  - Or more than one webcontext?
When we having more than one WebProcess we'll have more than one webcontext.

[Tor Arne Vestbø]

(In reply to comment #13)
> (In reply to comment #11)
> > Just a few questions (out of curiosity): 
> > 
> >  - What are the use-cases for this api?
> Having another entry point in our API for non view-dependent stuff, like downloads for instance. In the future, for creating multiple webcontexts.
> 
> >  - When will you have more than one WebView?
> Snowshoe uses multiple webviews already, for tabs.
> 
> >  - Or more than one webcontext?
> When we having more than one WebProcess we'll have more than one webcontext.

Ok, that's more or less what I expected.

Wouldn't this mean that a single WebView would no longer have onDownloadRequested? 

And what about multiple tabs with multiple web contexts? 

WebView {
    id: firstTab
    context: firstContext
}

WebContext {
    id: firstContext
    onDownloadRequest: {}
}

WebView {
    id: secondTab
    context: secondContext
}

WebContext {
    id: secondContext
    onDownloadRequest: {}
}

You'd want to share the onDownloadRequest handling right? And you wouldn't statically build these WebContexts in the QML file anyways, you'd manage them dynamically based on tabs opened from other tabs etc? 

I'm not convinced this needs to be QML API. We're complicating the single-webview case for the build-a-browser-with-muiltiple-tabs-and-multiple-webcontexts-case. 

Is there some other way we can solve this?

[Jesus Sanchez-Palencia]

(In reply to comment #14)
> Wouldn't this mean that a single WebView would no longer have onDownloadRequested? 

> I'm not convinced this needs to be QML API. We're complicating the single-webview case for the build-a-browser-with-muiltiple-tabs-and-multiple-webcontexts-case. 


The simple webview case is working on the same way, as it creates its own QQuickWebContext if none is set. The onDownloadRequested will still be there but through webView.context.onDownloadRequested , instead of webView.experimental.onDownloadRequested.


> You'd want to share the onDownloadRequest handling right? And you wouldn't statically build these WebContexts in the QML file anyways, you'd manage them dynamically based on tabs opened from other tabs etc?

I'm not sure I'm following you here, sorry. =/


Thanks for the questions and feedback.

[Jocelyn Turcotte]

Other concerns:
1. It's weird to have the context exposed as a visual Component:
- it requires another visual Item as it's parent, not possible to handle it on the C++ side.
- sharing the context across web views will be difficult, it will require it to be stored in an Item property and created dynamically if we want one per tab.
Maybe have it as a straight QObject creatable through JS?

2. The context has to play well with the way we will handle the createNewPage signal to be able to pass it between pages that have JS affinities. Ideally this should be done automatically, but if we expose it this could be difficult.

3. Trying to have one context per tab is going to force the developer to manage the connections between the context and its download manager to make sure that all contexts are connected to the download manager and that the connection isn't duplicated through intermediates if a context is used in more than one tab.

Overall I don't have a better solution, but I think that the real problem is that download requests lost the association to their page once on the UI process side. WebView { onDownloadRequested: ... } would make perfect sense. For all those reasons it also feels to me that the WebContext is too low level to be exposed in QML.

[Jesus Sanchez-Palencia]

Created attachment 122270 [details]
WIP - rebased patch after updates on componentComplete bug

[Jesus Sanchez-Palencia]

Created attachment 122280 [details]
WIP - rebased after latest bug fixing on componentComplete

[Jesus Sanchez-Palencia]

We decided to not expose WebContext in QML for now.