Bug 108808 – [Qt] Restore URL Scheme Delegates after QtWebProcess crash

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

[Milian Wolff]

When the QtWebProcess crashes, the registered URL Scheme Delegates are not properly restored over IPC in the newly launched process instance.

[Milian Wolff]

Created attachment 186334 [details]
Patch

[Zeno Albisser]

LGTM

[Simon Hausmann]

Comment on attachment 186334 [details]
Patch

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

> Source/WebKit2/UIProcess/API/qt/qquickwebview.cpp:790
> +    for (int i = 0; i < experimental->schemeDelegates_Count(&schemes); ++i) {

Why not call _Count before the loop instead of for each iteration? It cannot be inlined.

LGTM otherwise.

[Milian Wolff]

Created attachment 186364 [details]
Patch

[Simon Hausmann]

LGTM. Perhaps Benjamin can help us with the review :)

[Michael Brüning]

Comment on attachment 186364 [details]
Patch

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

> Source/WebKit2/UIProcess/API/qt/qquickwebview.cpp:789
> +    QQmlListProperty<QQuickUrlSchemeDelegate> schemes = experimental->schemeDelegates();

I think we have to add this here in order to keep qrc-schemes working:
webPageProxy->registerApplicationScheme(ASCIILiteral("qrc"));

[Benjamin Poulain]

Comment on attachment 186364 [details]
Patch

I have nothing against this. Simon, feel free to review.

On a related node: shouldn't you switch to the CustomProtocolManager instead of the custom ApplicationScheme thingy?
On a slightly related side note: It would be good to split WebPage and WebPageProxy between the stuff that are core, and the stuff that are platform quirks. This could be done by supplements and/or subclasses. Maybe you want to start something like this to clean custom handling like ApplicationScheme?

[Zeno Albisser]

(In reply to comment #7)
> (From update of attachment 186364 [details])
> I have nothing against this. Simon, feel free to review.
> 
> On a related node: shouldn't you switch to the CustomProtocolManager instead of the custom ApplicationScheme thingy?
> On a slightly related side note: It would be good to split WebPage and WebPageProxy between the stuff that are core, and the stuff that are platform quirks. This could be done by supplements and/or subclasses. Maybe you want to start something like this to clean custom handling like ApplicationScheme?

Hi Benjamin,
I am currently working on exactly that issue. I think the CustomProtocolManager will fit relatively well.
The only thing that I am aware of, that might be missing is a way to register/unregister schemes at runtime through the C-API. - Or did I overlook anything there?

[Benjamin Poulain]

> The only thing that I am aware of, that might be missing is a way to register/unregister schemes at runtime through the C-API. - Or did I overlook anything there?

RegisterScheme/UnregisterScheme? http://trac.webkit.org/browser/trunk/Source/WebKit2/Shared/Network/CustomProtocols/CustomProtocolManager.messages.in :)

[Benjamin Poulain]

Oh, through C API! Yep, it is possible there isn't one.

[Simon Hausmann]

Comment on attachment 186364 [details]
Patch

r=me, signed off for WK2 by Benjamin, but Michael's proposed change seems required before landing (cq-)

[Milian Wolff]

Created attachment 188553 [details]
Patch

[Milian Wolff]

OK, qrc scheme handler gets reregistered now as well. I furthermore created https://bugs.webkit.org/show_bug.cgi?id=109934 to figure out how we can disable the qrc scheme handler if not desired.

Anything else?

[WebKit Review Bot]

Comment on attachment 188553 [details]
Patch

Clearing flags on attachment: 188553

Committed r142997: <http://trac.webkit.org/changeset/142997>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.