Bug 67785 – [Qt][WK2] Make QWebError more friendly to QML.

• Component: WebKit Qt
• Status: RESOLVED FIXED

[Alexis Menard (darktears)]

[Qt][WK2] Make QWebError more friendly to QML.

[Alexis Menard (darktears)]

Created attachment 106750 [details]
Patch

[WebKit Review Bot]

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.

[Alexis Menard (darktears)]

(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.

[WebKit Review Bot]

Comment on attachment 106750 [details]
Patch

Attachment 106750 [details] did not pass cr-mac-ews (chromium):
Output: http://queues.webkit.org/results/9617905

[Alexis Menard (darktears)]

Comment on attachment 106750 [details]
Patch

Clearing cq flags, the cr-mac is sick and the patch only touch Qt specific files.

[Noam Rosenthal]

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.

[Alexis Menard (darktears)]

(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,...).

[Noam Rosenthal]

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.

[Alexis Menard (darktears)]

Created attachment 107059 [details]
Patch

[Alexis Menard (darktears)]

(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.

[WebKit Review Bot]

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.

[Alexis Menard (darktears)]

(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.

[Alexis Menard (darktears)]

Created attachment 107233 [details]
Patch

[Alexis Menard (darktears)]

Created attachment 107489 [details]
Patch

[Alexis Menard (darktears)]

Created attachment 107491 [details]
Patch

[WebKit Review Bot]

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

[Alexis Menard (darktears)]

Committed r95197: <http://trac.webkit.org/changeset/95197>

[Balazs Kelemen]

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.