[Qt][WK2] Make QWebError more friendly to QML.
Created attachment 106750 [details] Patch
Attachment 106750 [details] did not pass style-queue: Failed to run "['Tools/Scripts/update-webkit', '--chromium']" exit_code: 2 Last 3072 characters of output: 393 parsed_url: http://src.chromium.org/svn/trunk/src/build@99393 should_process: True processed: True requirements: set(['./']) name: net url: http://src.chromium.org/svn/trunk/src/net@99393 parsed_url: http://src.chromium.org/svn/trunk/src/net@99393 should_process: True requirements: set(['./']) name: tools/data_pack url: http://src.chromium.org/svn/trunk/src/tools/data_pack@99393 parsed_url: http://src.chromium.org/svn/trunk/src/tools/data_pack@99393 should_process: True processed: True requirements: set(['./']) name: third_party/ffmpeg url: From('chromium_deps', 'src/third_party/ffmpeg') should_process: True requirements: set(['third_party', 'chromium_deps', './']) name: tools/generate_stubs url: http://src.chromium.org/svn/trunk/src/tools/generate_stubs@99393 parsed_url: http://src.chromium.org/svn/trunk/src/tools/generate_stubs@99393 should_process: True processed: True requirements: set(['./']) name: third_party/libjpeg_turbo url: From('chromium_deps', 'src/third_party/libjpeg_turbo') should_process: True requirements: set(['third_party', 'chromium_deps', './']) name: third_party/v8-i18n url: From('chromium_deps', 'src/third_party/v8-i18n') should_process: True requirements: set(['third_party', 'chromium_deps', './']) name: tools/grit url: http://src.chromium.org/svn/trunk/src/tools/grit@99393 should_process: True requirements: set(['./']) name: base url: http://src.chromium.org/svn/trunk/src/base@99393 should_process: True requirements: set(['./']) name: sql url: http://src.chromium.org/svn/trunk/src/sql@99393 should_process: True requirements: set(['./']) name: v8 url: From('chromium_deps', 'src/v8') should_process: True requirements: set(['chromium_deps', './']) name: testing/gtest url: From('chromium_deps', 'src/testing/gtest') should_process: True requirements: set(['testing', 'chromium_deps', './']) name: third_party/libwebp url: http://src.chromium.org/svn/trunk/src/third_party/libwebp@99393 should_process: True requirements: set(['third_party', './']) name: googleurl url: From('chromium_deps', 'src/googleurl') should_process: True requirements: set(['chromium_deps', './']) name: third_party/skia/include url: From('chromium_deps', 'src/third_party/skia/include') should_process: True requirements: set(['third_party', 'chromium_deps', './']) name: third_party/ots url: From('chromium_deps', 'src/third_party/ots') should_process: True requirements: set(['third_party', 'chromium_deps', './']) name: third_party/snappy/src url: From('chromium_deps', 'src/third_party/snappy/src') should_process: True requirements: set(['third_party', 'chromium_deps', './']) Died at Tools/Scripts/update-webkit-chromium line 80. No such file or directory at Tools/Scripts/update-webkit line 104. If any of these errors are false positives, please file a bug against check-webkit-style.
(In reply to comment #2) > Attachment 106750 [details] did not pass style-queue: > > Failed to run "['Tools/Scripts/update-webkit', '--chromium']" exit_code: 2 > > Last 3072 characters of output: > 393 > parsed_url: http://src.chromium.org/svn/trunk/src/build@99393 > should_process: True > processed: True > requirements: set(['./']) > > name: net > url: http://src.chromium.org/svn/trunk/src/net@99393 > parsed_url: http://src.chromium.org/svn/trunk/src/net@99393 > should_process: True > requirements: set(['./']) > > name: tools/data_pack > url: http://src.chromium.org/svn/trunk/src/tools/data_pack@99393 > parsed_url: http://src.chromium.org/svn/trunk/src/tools/data_pack@99393 > should_process: True > processed: True > requirements: set(['./']) > > name: third_party/ffmpeg > url: From('chromium_deps', 'src/third_party/ffmpeg') > should_process: True > requirements: set(['third_party', 'chromium_deps', './']) > > name: tools/generate_stubs > url: http://src.chromium.org/svn/trunk/src/tools/generate_stubs@99393 > parsed_url: http://src.chromium.org/svn/trunk/src/tools/generate_stubs@99393 > should_process: True > processed: True > requirements: set(['./']) > > name: third_party/libjpeg_turbo > url: From('chromium_deps', 'src/third_party/libjpeg_turbo') > should_process: True > requirements: set(['third_party', 'chromium_deps', './']) > > name: third_party/v8-i18n > url: From('chromium_deps', 'src/third_party/v8-i18n') > should_process: True > requirements: set(['third_party', 'chromium_deps', './']) > > name: tools/grit > url: http://src.chromium.org/svn/trunk/src/tools/grit@99393 > should_process: True > requirements: set(['./']) > > name: base > url: http://src.chromium.org/svn/trunk/src/base@99393 > should_process: True > requirements: set(['./']) > > name: sql > url: http://src.chromium.org/svn/trunk/src/sql@99393 > should_process: True > requirements: set(['./']) > > name: v8 > url: From('chromium_deps', 'src/v8') > should_process: True > requirements: set(['chromium_deps', './']) > > name: testing/gtest > url: From('chromium_deps', 'src/testing/gtest') > should_process: True > requirements: set(['testing', 'chromium_deps', './']) > > name: third_party/libwebp > url: http://src.chromium.org/svn/trunk/src/third_party/libwebp@99393 > should_process: True > requirements: set(['third_party', './']) > > name: googleurl > url: From('chromium_deps', 'src/googleurl') > should_process: True > requirements: set(['chromium_deps', './']) > > name: third_party/skia/include > url: From('chromium_deps', 'src/third_party/skia/include') > should_process: True > requirements: set(['third_party', 'chromium_deps', './']) > > name: third_party/ots > url: From('chromium_deps', 'src/third_party/ots') > should_process: True > requirements: set(['third_party', 'chromium_deps', './']) > > name: third_party/snappy/src > url: From('chromium_deps', 'src/third_party/snappy/src') > should_process: True > requirements: set(['third_party', 'chromium_deps', './']) > > Died at Tools/Scripts/update-webkit-chromium line 80. > No such file or directory at Tools/Scripts/update-webkit line 104. > > > If any of these errors are false positives, please file a bug against check-webkit-style. Obviously some problem. Though the style checker will complain about the two names for signal parameters but it's on purpose.
Comment on attachment 106750 [details] Patch Attachment 106750 [details] did not pass cr-mac-ews (chromium): Output: http://queues.webkit.org/results/9617905
Comment on attachment 106750 [details] Patch Clearing cq flags, the cr-mac is sick and the patch only touch Qt specific files.
Comment on attachment 106750 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=106750&action=review > Source/WebKit2/UIProcess/API/qt/qdesktopwebview.cpp:147 > + QWebError lastError(error); > + emit q->loadFailed(&lastError); Hmmm... lifecycle problems coming. This is a classic by-value object; I wish it could be passed as a QVariantMap or as a heap-allocated QObject that is transferred to JS scope. Is there a way to do that in QML? Otherwise what I suggest something like: loadFailed() and QWebError* lastError(), saving the last QWebError as a child object of the view - this way lifecycle is safe. > Source/WebKit2/UIProcess/API/qt/qmlplugin/plugin.cpp:28 > +#include <QtNetwork/QNetworkReply> Seems unrelated. Crumbs from a separate patch? > Source/WebKit2/UIProcess/API/qt/qmlplugin/plugin.cpp:43 > + qmlRegisterUncreatableType<QNetworkReply>(uri, 5, 0, "NetworkReply", QObject::tr("Cannot create separate instance of NetworkReply")); Ditto.
(In reply to comment #6) > (From update of attachment 106750 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=106750&action=review > > > Source/WebKit2/UIProcess/API/qt/qdesktopwebview.cpp:147 > > + QWebError lastError(error); > > + emit q->loadFailed(&lastError); > > Hmmm... lifecycle problems coming. This is a classic by-value object; I wish it could be passed as a QVariantMap or as a heap-allocated QObject that is transferred to JS scope. Is there a way to do that in QML? Otherwise what I suggest something like: > loadFailed() and QWebError* lastError(), saving the last QWebError as a child object of the view - this way lifecycle is safe. > I knew about the life cycle object as I put in the changelog. If you take a look at mouse area implementation you will see that they do the same for the event handler. It brings here an important topic : What do we want with those views : a nice C++ API or a nice primary QML API and by extension an okish C++ API. For example lastError is a suggestion I though about but this is a really awkward QML API. I think it is very convenient to have the error valid only in the handler : onLoadFailed { if (error.errorCode() == NetworkReply.TimeOut) //do something } > > Source/WebKit2/UIProcess/API/qt/qmlplugin/plugin.cpp:28 > > +#include <QtNetwork/QNetworkReply> > > Seems unrelated. Crumbs from a separate patch? > > > Source/WebKit2/UIProcess/API/qt/qmlplugin/plugin.cpp:43 > > + qmlRegisterUncreatableType<QNetworkReply>(uri, 5, 0, "NetworkReply", QObject::tr("Cannot create separate instance of NetworkReply")); > > Ditto. Could be done in a separate patch though I think it was related to that one. QWebError gives you an error code, if you don't export QNetworkReply you can't do anything with this error code as the enum NetworkError tells you what is the problem (Host not found, timeout,...).
Comment on attachment 106750 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=106750&action=review >>> Source/WebKit2/UIProcess/API/qt/qdesktopwebview.cpp:147 >>> + emit q->loadFailed(&lastError); >> >> Hmmm... lifecycle problems coming. This is a classic by-value object; I wish it could be passed as a QVariantMap or as a heap-allocated QObject that is transferred to JS scope. Is there a way to do that in QML? Otherwise what I suggest something like: >> loadFailed() and QWebError* lastError(), saving the last QWebError as a child object of the view - this way lifecycle is safe. > > I knew about the life cycle object as I put in the changelog. If you take a look at mouse area implementation you will see that they do the same for the event handler. It brings here an important topic : What do we want with those views : a nice C++ API or a nice primary QML API and by extension an okish C++ API. For example lastError is a suggestion I though about but this is a really awkward QML API. I think it is very convenient to have the error valid only in the handler : > > onLoadFailed { if (error.errorCode() == NetworkReply.TimeOut) //do something } I don't think it's an awkward QML API. onLoadFailed { doSomethingWith(webView.lastError.url); } If you really want to, you can add it as a member AND send it in the signal; this way it at least doesn't die, but just replaced when a new error comes in. >>> Source/WebKit2/UIProcess/API/qt/qmlplugin/plugin.cpp:28 >>> +#include <QtNetwork/QNetworkReply> >> >> Seems unrelated. Crumbs from a separate patch? > > Could be done in a separate patch though I think it was related to that one. QWebError gives you an error code, if you don't export QNetworkReply you can't do anything with this error code as the enum NetworkError tells you what is the problem (Host not found, timeout,...). Fair enough; my oversight for missing parts in the Changelog.
Created attachment 107059 [details] Patch
(In reply to comment #9) > Created an attachment (id=107059) [details] > Patch Removed the network reply, let see if we can get it in Qt : https://qt.gitorious.org/qt/qtdeclarative/merge_requests/7 Otherwise use a lastError member to have a valid object. For now it's on purpose that I didn't add the Q_PROPERTY for the last error as it brings more complexity on when I should reset this last error.
Attachment 107059 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebKit2/ChangeLog', u'Source/WebKit..." exit_code: 1 Source/WebKit2/UIProcess/API/qt/qtouchwebpage.h:68: The parameter name "error" adds no information, so it should be removed. [readability/parameter_name] [5] Source/WebKit2/UIProcess/API/qt/qdesktopwebview.h:84: The parameter name "error" adds no information, so it should be removed. [readability/parameter_name] [5] Source/WebKit2/UIProcess/qt/QtWebPageProxy.h:30: Alphabetical sorting problem. [build/include_order] [4] Total errors found: 3 in 11 files If any of these errors are false positives, please file a bug against check-webkit-style.
(In reply to comment #11) > Attachment 107059 [details] did not pass style-queue: > > Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebKit2/ChangeLog', u'Source/WebKit..." exit_code: 1 > > Source/WebKit2/UIProcess/API/qt/qtouchwebpage.h:68: The parameter name "error" adds no information, so it should be removed. [readability/parameter_name] [5] > Source/WebKit2/UIProcess/API/qt/qdesktopwebview.h:84: The parameter name "error" adds no information, so it should be removed. [readability/parameter_name] [5] Explained why in the changelog. > Source/WebKit2/UIProcess/qt/QtWebPageProxy.h:30: Alphabetical sorting problem. [build/include_order] [4] Fixed in http://trac.webkit.org/changeset/94960 . > Total errors found: 3 in 11 files > > > If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 107233 [details] Patch
Created attachment 107489 [details] Patch
Created attachment 107491 [details] Patch
Comment on attachment 107491 [details] Patch Rejecting attachment 107491 [details] from commit-queue. Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 2 Last 500 characters of output: WebKit2/UIProcess/qt/TouchViewInterface.h Hunk #2 succeeded at 77 with fuzz 2 (offset 2 lines). patching file Source/WebKit2/UIProcess/qt/ViewInterface.h Hunk #1 succeeded at 26 with fuzz 2 (offset 1 line). Hunk #2 succeeded at 75 (offset 6 lines). Hunk #3 FAILED at 77. 1 out of 3 hunks FAILED -- saving rejects to file Source/WebKit2/UIProcess/qt/ViewInterface.h.rej Failed to run "[u'/mnt/git/webkit-commit-queue/Tools/Scripts/svn-apply', u'--reviewer', u'Simon Hausmann', u'--f..." exit_code: 1 Full output: http://queues.webkit.org/results/9686084
Committed r95197: <http://trac.webkit.org/changeset/95197>
It broke the wk2 bot because it has a too old Qt5. Unfortunately this cannot be fixed with an update until #68181 would have been solved.