Bug 65315 - [Qt] Make QDesktopWebView/QTouchWebView loadProgress property more usable in QML.
Summary: [Qt] Make QDesktopWebView/QTouchWebView loadProgress property more usable in ...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Alexis Menard (darktears)
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-07-28 07:19 PDT by Alexis Menard (darktears)
Modified: 2011-07-29 13:27 PDT (History)
4 users (show)

See Also:

Attachments
Patch (3.37 KB, patch)
2011-07-28 07:21 PDT, Alexis Menard (darktears) 		no flags Details | Formatted Diff | Diff
Patch (7.41 KB, patch)
2011-07-28 07:56 PDT, Alexis Menard (darktears) 		no flags Details | Formatted Diff | Diff
Patch (13.12 KB, patch)
2011-07-28 10:08 PDT, Alexis Menard (darktears) 		no flags Details | Formatted Diff | Diff
Show Obsolete (2) View All Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
Description Alexis Menard (darktears) 2011-07-28 07:19:01 PDT 
[Qt] Make QDesktopWebView loadProgress property more usable in QML.
Comment 1 Alexis Menard (darktears) 2011-07-28 07:21:55 PDT 
Created attachment 102254 [details]
Patch
Comment 2 Noam Rosenthal 2011-07-28 07:26:11 PDT 
Comment on attachment 102254 [details]
Patch

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

> Source/WebKit2/UIProcess/API/qt/qdesktopwebview.h:54
> +    Q_PROPERTY(int loadProgress READ loadProgress NOTIFY loadProgress)

Shouldn't NOTIFY point to a signal?
Comment 3 Benjamin Poulain 2011-07-28 07:34:15 PDT 
Comment on attachment 102254 [details]
Patch

I am pretty sure the other view could enjoy the same kind of love :)
Please update both view together when they provide the same API. Storing the loadProgress should probably be done on QtWebPageProxy.
Comment 4 Caio Marcelo de Oliveira Filho 2011-07-28 07:43:22 PDT 
Comment on attachment 102254 [details]
Patch

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

>> Source/WebKit2/UIProcess/API/qt/qdesktopwebview.h:54
>> +    Q_PROPERTY(int loadProgress READ loadProgress NOTIFY loadProgress)
> 
> Shouldn't NOTIFY point to a signal?

Yes, and it does :-)... The problem here is that the signal and the getter have the same name. Alexis, what about changing the signal name to loadProgressChanged?
Comment 5 Alexis Menard (darktears) 2011-07-28 07:56:49 PDT 
Created attachment 102259 [details]
Patch
Comment 6 Benjamin Poulain 2011-07-28 09:10:33 PDT 
Comment on attachment 102259 [details]
Patch

QTouchWebPage already has the property. qtouchwebview_p.h is not the implementation of ViewInterface like on the desktop case. The update should be on QTouchWebPage :)
Would you mind addind an API test on the way? :-D
Comment 7 Alexis Menard (darktears) 2011-07-28 10:08:28 PDT 
Created attachment 102267 [details]
Patch
Comment 8 Benjamin Poulain 2011-07-29 04:02:05 PDT 
Comment on attachment 102267 [details]
Patch

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

> Source/WebKit2/UIProcess/API/qt/tests/commonviewtests/tst_commonviewtests.cpp:160
> +    QVERIFY(waitForSignal(viewAbstraction.data(), SIGNAL(loadProgressChanged(int))));
> +

I think this should be a signal spy over loadProgressChanged(), then you check that you got the signal. The wait for signal should be on the loadSucceeded only, like the next line.
This is just for robustness, in case a future refactoring make it simultaneous to have loadProgressChanged(100) and loadSucceeded() for local file.
Comment 9 Alexis Menard (darktears) 2011-07-29 04:55:24 PDT 
Committed r91985: <http://trac.webkit.org/changeset/91985>