WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
78820
[Qt] Signal and property cleanup in QQuickWebView
https://bugs.webkit.org/show_bug.cgi?id=78820
Summary
[Qt] Signal and property cleanup in QQuickWebView
Simon Hausmann
Reported
2012-02-16 08:13:17 PST
As discussed in the API review session in Szeged, there are a few straight-forward changes to the properties and signals of QQuickWebView that we should do before the release: * Remove canReload * Remove parameter from titleChanged, urlChanged, iconChanged, loadProgressChanged * loadFailed: change signature to loadFailed(failedUrl, description, errorDomain, errorCode) * linkHovered should be linkHovered(hoveredUrl, hoveredTitle) * Get rid of loadStarted and loadSucceeded, add loadingChanged instead * Remove navigationStateChanged, add canGoBackChanged and canGoForwardChanged For future reference let me try to recall the reasons we came up with in the discussion: * canReload is redundant in the face of "loading", the implementation of the one is the reverse of the other. * The parameters to the *Changed signals are redundant and usually not present in other QML modules. * The parameters in loadFailed should be ordered by importance and we should use failedUrl instead of url to avoid shadowing the WebView's url property * loadStarted and loadSuceeded are redundant in the face of a notification signal of the loading property: loadingChanged. If the loading property is true in the slot connected to the loadingChanged signal, then the load started, if the value is false the load finished. * The url and title properties in linkHovered should be renamed to avoid shadowing
Attachments
Patch
(14.71 KB, patch)
2012-02-24 06:57 PST
,
Jocelyn Turcotte
no flags
Details
Formatted Diff
Diff
View All
Add attachment
proposed patch, testcase, etc.
Jocelyn Turcotte
Comment 1
2012-02-24 06:47:47 PST
The loading signals part is tracked in
bug #79486
.
Jocelyn Turcotte
Comment 2
2012-02-24 06:57:48 PST
Created
attachment 128727
[details]
Patch
Jocelyn Turcotte
Comment 3
2012-02-24 09:57:11 PST
Committed
r108816
: <
http://trac.webkit.org/changeset/108816
>
Csaba Osztrogonác
Comment 4
2012-02-24 23:45:32 PST
(In reply to
comment #3
)
> Committed
r108816
: <
http://trac.webkit.org/changeset/108816
>
Reopen, because it made many tests crash and timeout. :(
http://build.webkit.sed.hu/results/x86-32%20Linux%20Qt%20Release%20WebKit2/r108817%20%2820727%29/results.html
It made layout tests exiting early, so the test coverage is near 0% now. :((
Csaba Osztrogonác
Comment 5
2012-02-25 00:19:35 PST
Rollout landed in
http://trac.webkit.org/changeset/108889
Jocelyn Turcotte
Comment 6
2012-02-28 10:43:46 PST
Comment on
attachment 128727
[details]
Patch Resubmitting after
http://trac.webkit.org/changeset/109121
landed, which should fix the crash. I'm still seeing one or two crashes on my machine that I can't sucessfully tie to this patch, so I'm pushing it once more with a 80% confidence it shouldn't crash.
WebKit Review Bot
Comment 7
2012-02-28 10:53:31 PST
Comment on
attachment 128727
[details]
Patch Rejecting
attachment 128727
[details]
from commit-queue. Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 1 ERROR: /mnt/git/webkit-commit-queue/Source/WebKit2/ChangeLog neither lists a valid reviewer nor contains the string "Unreviewed" or "Rubber stamp" (case insensitive). Full output:
http://queues.webkit.org/results/11711013
Noam Rosenthal
Comment 8
2012-02-28 11:25:06 PST
Comment on
attachment 128727
[details]
Patch To Jocelyn's request, reapplying r+ and cq+.
WebKit Review Bot
Comment 9
2012-02-28 11:41:01 PST
Comment on
attachment 128727
[details]
Patch Clearing flags on attachment: 128727 Committed
r109130
: <
http://trac.webkit.org/changeset/109130
>
WebKit Review Bot
Comment 10
2012-02-28 11:41:06 PST
All reviewed patches have been landed. Closing bug.
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug