Merge lp:~michael-sheldon/webbrowser-app/fix-1354388 into lp:webbrowser-app

Proposed by Michael Sheldon
Status: Merged
Approved by: Olivier Tilloy
Approved revision: 736
Merged at revision: 721
Proposed branch: lp:~michael-sheldon/webbrowser-app/fix-1354388
Merge into: lp:webbrowser-app
Prerequisite: lp:~michael-sheldon/webbrowser-app/ssl-status
Diff against target: 55 lines (+10/-2)
3 files modified
src/app/webbrowser/AddressBar.qml (+7/-2)
src/app/webbrowser/Browser.qml (+2/-0)
src/app/webbrowser/Chrome.qml (+1/-0)
To merge this branch: bzr merge lp:~michael-sheldon/webbrowser-app/fix-1354388
Reviewer Review Type Date Requested Status
PS Jenkins bot continuous-integration Approve
Olivier Tilloy Approve
Review via email: mp+235796@code.launchpad.net

Commit message

Ensure that the url of the page we're loading is displayed, instead of the currently loaded url.

Description of the change

Ensure that the url of the page we're loading is displayed, instead of the currently loaded url.

To post a comment you must log in.
Revision history for this message
Michael Sheldon (michael-sheldon) wrote :

Are there any related MPs required for this MP to build/function as expected? Please list.

 * No

Is your branch in sync with latest trunk (e.g. bzr pull lp:trunk -> no changes)

 * Yes

Did you perform an exploratory manual test run of your code change and any related functionality on device or emulator?

 * Yes

Did you successfully run all tests found in your component's Test Plan (https://wiki.ubuntu.com/Process/Merges/TestPlan/webbrowser-app) on device or emulator?

 * Yes

If you changed the UI, was the change specified/approved by design?

 * No change

If you changed UI labels, did you update the pot file?

 * No change

If you changed the packaging (debian), did you add a core-dev as a reviewer to this MP?

 * No change

Revision history for this message
Olivier Tilloy (osomon) wrote :

Looks good, and works as expected. Thanks!

I’m seeing an additional use case that doesn’t seem to be covered (although I don’t think it’s a new regression, so it may be fine to tackle it separately):

  - browse to any page that triggers the display of a certificate warning page (e.g. https://testssl-expire.disig.sk/) and choose to proceed anyway
  - close the browser and open it again, the previously open page is re-loaded
  - the certificate warning page is displayed, but the address bar is empty, until you choose to proceed anyway

If you think it can be addressed easily and with minimal changes to this MR, then go ahead, otherwise I’m happy to report it separately afterwards.

review: Approve
Revision history for this message
PS Jenkins bot (ps-jenkins) wrote :
review: Approve (continuous-integration)
734. By Michael Sheldon

When a certificate error is displayed set that page's URL in the address bar to ensure we show the URL if a page with an error is loaded at start up

Revision history for this message
Olivier Tilloy (osomon) wrote :

That seems to work well, however I’m wondering if there isn’t a more general way to fix that, because in the case where there is no certificate error, opening the browser on a previously open page will still display a blank address bar for a short while.

Basically we need to set chrome.requestedUrl accordingly when the currently active webview starts loading, regardless of whether there is a certificate error. Not sure exactly where in the code this needs to be done though, I’d need to take a closer look.

Revision history for this message
PS Jenkins bot (ps-jenkins) wrote :
review: Approve (continuous-integration)
735. By Michael Sheldon

Display the URL in the address bar as soon as a tab is created

Revision history for this message
Olivier Tilloy (osomon) wrote :

19 + if(actualUrl.length > 0) {

actualUrl is of type url, which doesn’t have a 'length' property, so actualUrl.length will always return undefined. You’ll need to do "actualUrl.toString().length".

review: Needs Fixing
736. By Michael Sheldon

Fix check for empty string

Revision history for this message
PS Jenkins bot (ps-jenkins) wrote :
review: Needs Fixing (continuous-integration)
Revision history for this message
Olivier Tilloy (osomon) wrote :

Looks good now, thanks!

review: Approve
Revision history for this message
PS Jenkins bot (ps-jenkins) wrote :
review: Approve (continuous-integration)

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'src/app/webbrowser/AddressBar.qml'
2--- src/app/webbrowser/AddressBar.qml 2014-09-25 12:34:49 +0000
3+++ src/app/webbrowser/AddressBar.qml 2014-09-25 12:34:51 +0000
4@@ -351,7 +351,7 @@
5 onActiveFocusChanged: {
6 if (activeFocus) {
7 addressbar.textFieldFocused();
8- } else if (addressbar.actualUrl.toString()) {
9+ } else if (!addressbar.loading && addressbar.actualUrl.toString()) {
10 text = addressbar.simplifyUrl(addressbar.actualUrl)
11 }
12 }
13@@ -455,5 +455,10 @@
14 }
15 }
16
17- onActualUrlChanged: text = simplifyUrl(actualUrl)
18+ onActualUrlChanged: {
19+ if(actualUrl.toString().length > 0) {
20+ text = simplifyUrl(actualUrl)
21+ }
22+ }
23+ onRequestedUrlChanged: text = simplifyUrl(requestedUrl)
24 }
25
26=== modified file 'src/app/webbrowser/Browser.qml'
27--- src/app/webbrowser/Browser.qml 2014-09-25 12:34:49 +0000
28+++ src/app/webbrowser/Browser.qml 2014-09-25 12:34:51 +0000
29@@ -252,6 +252,7 @@
30 onSelected: {
31 browser.currentWebview.url = url
32 browser.currentWebview.forceActiveFocus()
33+ chrome.requestedUrl = url
34 }
35 }
36 }
37@@ -475,6 +476,7 @@
38 var index = tabsModel.add(tab)
39 if (setCurrent) {
40 tabsModel.setCurrent(index)
41+ chrome.requestedUrl = tab.initialUrl
42 if (focusAddressBar) {
43 internal.focusAddressBar()
44 }
45
46=== modified file 'src/app/webbrowser/Chrome.qml'
47--- src/app/webbrowser/Chrome.qml 2014-09-25 12:34:49 +0000
48+++ src/app/webbrowser/Chrome.qml 2014-09-25 12:34:51 +0000
49@@ -28,6 +28,7 @@
50 property alias bookmarked: addressbar.bookmarked
51 property list<Action> drawerActions
52 readonly property bool drawerOpen: internal.openDrawer
53+ property alias requestedUrl: addressbar.requestedUrl
54
55 signal validated()
56

Subscribers

People subscribed via source and target branches

to status/vote changes: