Summary: | [Qt] Restore URL Scheme Delegates after QtWebProcess crash | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Milian Wolff <milian.wolff> | ||||||||
Component: | WebKit Qt | Assignee: | Milian Wolff <milian.wolff> | ||||||||
Status: | RESOLVED FIXED | ||||||||||
Severity: | Normal | CC: | abecsi, benjamin, cmarcelo, hausmann, menard, webkit.review.bot, zeno | ||||||||
Priority: | P2 | ||||||||||
Version: | 528+ (Nightly build) | ||||||||||
Hardware: | Unspecified | ||||||||||
OS: | Unspecified | ||||||||||
Attachments: |
|
Description
Milian Wolff
2013-02-04 03:17:20 PST
Created attachment 186334 [details]
Patch
LGTM 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. Created attachment 186364 [details]
Patch
LGTM. Perhaps Benjamin can help us with the review :) 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")); 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?
(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? > 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 :) Oh, through C API! Yep, it is possible there isn't one. Comment on attachment 186364 [details]
Patch
r=me, signed off for WK2 by Benjamin, but Michael's proposed change seems required before landing (cq-)
Created attachment 188553 [details]
Patch
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? Comment on attachment 188553 [details] Patch Clearing flags on attachment: 188553 Committed r142997: <http://trac.webkit.org/changeset/142997> All reviewed patches have been landed. Closing bug. |