webbrowser-app

Merge lp://qastaging/~artmello/webbrowser-app/webbrowser-app-statesaver into lp://qastaging/webbrowser-app

webbrowser-app-statesaver
Merge into trunk

Proposed by Arthur Mello on 2014-07-22

Status:

Rejected

Rejected by:

Olivier Tilloy on 2014-08-12

Proposed branch:

lp://qastaging/~artmello/webbrowser-app/webbrowser-app-statesaver

Merge into:

lp://qastaging/webbrowser-app

Diff against target:

180 lines (+31/-1)

9 files modified

src/app/webbrowser/Browser.qml (+7/-0)
src/app/webbrowser/webbrowser-app.cpp (+3/-0)
src/app/webbrowser/webbrowser-app.qml (+2/-0)
src/app/webcontainer/WebApp.qml (+2/-0)
src/app/webcontainer/WebViewImplOxide.qml (+4/-0)
src/app/webcontainer/WebViewImplWebkit.qml (+4/-0)
src/app/webcontainer/WebappContainerWebview.qml (+3/-1)
src/app/webcontainer/webapp-container.cpp (+4/-0)
src/app/webcontainer/webapp-container.qml (+2/-0)

To merge this branch:

bzr merge lp://qastaging/~artmello/webbrowser-app/webbrowser-app-statesaver

High

Fix Released

Link a bug report

Reviewer	Review Type	Date Requested	Status
Olivier Tilloy			Disapprove on 2014-08-12
PS Jenkins bot	continuous-integration		Approve on 2014-07-31
Bill Filler (community)		2014-07-22	Needs Fixing on 2014-07-27
Review via email: mp+227737@code.qastaging.launchpad.net

Commit message

Save the last visited url for a webapp

Description of the change

Save the last visited url for a webapp

lp://qastaging/~artmello/webbrowser-app/webbrowser-app-statesaver updated on 2014-07-22

633. By Arthur Mello on 2014-07-22: Add state saver to the webbrowser last visited url

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2014-07-22:

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/webbrowser-app-ci/961/rebuild

review: Approve (continuous-integration)

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2014-07-22:

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/webbrowser-app-ci/963/rebuild

review: Approve (continuous-integration)

lp://qastaging/~artmello/webbrowser-app/webbrowser-app-statesaver updated on 2014-07-22

634. By Arthur Mello on 2014-07-22: If a specialized url was provided to the webapp it is used instead of the state saved one

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2014-07-22:

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/webbrowser-app-ci/965/rebuild

review: Approve (continuous-integration)

Revision history for this message

Bill Filler (bfiller) wrote on 2014-07-23:

tried this out and there is one problem:

- for webapps (tested amazon and gmail), the state save/restore works only once and does not work correctly the second time
To test:
- open amazon
- navigate to a page
- kill -2 on the pid
- relaunch amazon
- verify that the last url was restored
- then navigate to a different page
- kill -2 on the pid
- relaunch amazon
- the first url that was saved is restored, not the last visited page

This works fine for webbrowser-app, just not webapps

review: Needs Fixing

lp://qastaging/~artmello/webbrowser-app/webbrowser-app-statesaver updated on 2014-07-23

635. By Arthur Mello on 2014-07-23: Change the property saved to fix the issue of not saving the correct values

Revision history for this message

Arthur Mello (artmello) wrote on 2014-07-23:

> tried this out and there is one problem:
>
> - for webapps (tested amazon and gmail), the state save/restore works only
> once and does not work correctly the second time
> To test:
> - open amazon
> - navigate to a page
> - kill -2 on the pid
> - relaunch amazon
> - verify that the last url was restored
> - then navigate to a different page
> - kill -2 on the pid
> - relaunch amazon
> - the first url that was saved is restored, not the last visited page
>
> This works fine for webbrowser-app, just not webapps

Seems to work on r635

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2014-07-23:

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/webbrowser-app-ci/966/rebuild

review: Approve (continuous-integration)

Revision history for this message

Olivier Tilloy (osomon) wrote on 2014-07-23:

I’ve tested only the browser app on desktop so far, and the general use case seems to work fine, however I’m seeing a case that doesn’t:

    webbrowser-app http://example.org
    killall -9 webbrowser-app
    webbrowser-app http://ubuntu.com

I expect the second invokation of webbrowser-app to open ubuntu.com, however the previous state is restored and it browses to example.org instead.

Note that this issue is most likely affecting desktop only, not phone, where this kind of URL opening will go through the UriHandler, which will ignore the saved state, according to documentation.

To address this, you probably need a mechanism similar to what you did for the webapp-container, where you recognize whether a URL was explicitly requested on the command line.

review: Needs Fixing

Revision history for this message

Olivier Tilloy (osomon) wrote on 2014-07-23:

As a side note, I wonder whether this kind of functionality can be tested with autopilot? Would you mind checking with someone from QA? Since the StateSaver works only if all the ancestors of a given item have an id explicitly set, it seems very prone to regressions (if someone accidentally removes a seemingly unused id), and having an integration test would prevent such regressions.

Revision history for this message

Olivier Tilloy (osomon) wrote on 2014-07-23:

From the looks of the diff, it seems WebViewImplWebkit doesn’t actually implement state restore.
While we want to get rid of the webkit implementation ASAP, for the time being we have to support it, so there should be feature parity where possible. You can easily force the use of webkit instead of oxide by passing "--webkit" as a command line parameter when launching webapp-container.

Revision history for this message

Olivier Tilloy (osomon) wrote on 2014-07-23:

What exactly is the use case that requires an extra 'specializedUrlProvided' property (I remember you mentioned it yesterday, but I don’t remember exactly what it was)?

lp://qastaging/~artmello/webbrowser-app/webbrowser-app-statesaver updated on 2014-07-24

636. By Arthur Mello on 2014-07-23: Merge with trunk
637. By Arthur Mello on 2014-07-24: Make sure that if a url is provided as a parameter to webbrowser it will override the state saved url

Revision history for this message

Arthur Mello (artmello) wrote on 2014-07-24:

> I’ve tested only the browser app on desktop so far, and the general use case
> seems to work fine, however I’m seeing a case that doesn’t:
>
> webbrowser-app http://example.org
> killall -9 webbrowser-app
> webbrowser-app http://ubuntu.com
>
> I expect the second invokation of webbrowser-app to open ubuntu.com, however
> the previous state is restored and it browses to example.org instead.
>
> Note that this issue is most likely affecting desktop only, not phone, where
> this kind of URL opening will go through the UriHandler, which will ignore the
> saved state, according to documentation.
>
> To address this, you probably need a mechanism similar to what you did for the
> webapp-container, where you recognize whether a URL was explicitly requested
> on the command line.

Fixed on r637

lp://qastaging/~artmello/webbrowser-app/webbrowser-app-statesaver updated on 2014-07-24

638. By Arthur Mello on 2014-07-24: Implement the same strategy used on Oxide to WebKit

Revision history for this message

Arthur Mello (artmello) wrote on 2014-07-24:

> From the looks of the diff, it seems WebViewImplWebkit doesn’t actually
> implement state restore.
> While we want to get rid of the webkit implementation ASAP, for the time being
> we have to support it, so there should be feature parity where possible. You
> can easily force the use of webkit instead of oxide by passing "--webkit" as a
> command line parameter when launching webapp-container.

Fixed on r638

Revision history for this message

Arthur Mello (artmello) wrote on 2014-07-24:

> As a side note, I wonder whether this kind of functionality can be tested with
> autopilot? Would you mind checking with someone from QA? Since the StateSaver
> works only if all the ancestors of a given item have an id explicitly set, it
> seems very prone to regressions (if someone accidentally removes a seemingly
> unused id), and having an integration test would prevent such regressions.

I will check with QA guys to check how that could be done.

lp://qastaging/~artmello/webbrowser-app/webbrowser-app-statesaver updated on 2014-07-24

639. By Arthur Mello on 2014-07-24: Change the WebApp implementation to work on the same way as webbrowser

Revision history for this message

Arthur Mello (artmello) wrote on 2014-07-24:

> What exactly is the use case that requires an extra 'specializedUrlProvided'
> property (I remember you mentioned it yesterday, but I don’t remember exactly
> what it was)?

specializedUrlProvided was used to store the provided url to a webapp and, if it was set, we should ignore the state saved url. But, I think I found a better way to do that, it was changed on r639. This property is not used anymore

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2014-07-24:

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/webbrowser-app-ci/968/rebuild

review: Needs Fixing (continuous-integration)

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2014-07-24:

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/webbrowser-app-ci/969/rebuild

review: Approve (continuous-integration)

Revision history for this message

Olivier Tilloy (osomon) wrote on 2014-07-24:

This might be working (I haven’t actually tested this time round), but the implementation doesn’t look right: from a semantics point of view, it doesn’t make sense to add a 'restoreUrl' parameter to the newTab(…) function in webbrowser-app.qml and to the openUrlInNewTab(…) function in Browser.qml. openUrlInNewTab(…) is being called in several other places where this parameter is meaningless.

Since in this iteration we are saving only one URL (that of the current tab), we should probably add a 'currentUrl' property to the top-level Browser component (or maybe even to the top-level BrowserView component, so that it’s shared between the two apps) that will be updated whenever the URL of the current webview changes (but not be a direct binding to currentWebview.url to avoid it being set when we don’t want to), and upon restore can be used to selectively open a new tab with the saved URL depending on the situation.

I have not tested the above suggestion, so there may be caveats, but the approach looks sounder to me.

On a related note, did you get feedback from the QA team on the feasability of autopilot tests for the feature? We would need a test that launches the app, browses to a given URL, stops the app, then launches it again and verifies that the URL is restored.

lp://qastaging/~artmello/webbrowser-app/webbrowser-app-statesaver updated on 2014-07-25

640. By Arthur Mello on 2014-07-25: Make sure that a new tab is not restored when created

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2014-07-25:

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/webbrowser-app-ci/973/rebuild

review: Approve (continuous-integration)

lp://qastaging/~artmello/webbrowser-app/webbrowser-app-statesaver updated on 2014-07-25

641. By Arthur Mello on 2014-07-25: Create a property on TabsModel to store all opened urls
642. By Arthur Mello on 2014-07-25: Restore all opened urls

Revision history for this message

Arthur Mello (artmello) wrote on 2014-07-25:

Removed the restoreUrl parameter from the newTab function, as mentioned. Using the currentUrl property to store the last visited url.

Added to support to restore all the open tabs. I am trying to use the StateSaver to do that, but since it does not seem to be able to restore a model I am saving a list of open urls and creating new tabs during the restore process.

I am not restoring any tab if a url/list of urls is passed as a command line parameter. Should we keep that or open the provided urls as new tabs?

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2014-07-25:

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/webbrowser-app-ci/977/rebuild

review: Needs Fixing (continuous-integration)

Revision history for this message

Bill Filler (bfiller) wrote on 2014-07-27:

tested latest debs on device build 153
Things are not working correctly for webbrowser-app. First time I opened two tabs and did kill -2 and it worked correctly, restoring tabs. Now everytime after that it won't save restore any urls regardless if I have single or multiple tabs. So something is broken.

webapp-container seems to be working correctly

review: Needs Fixing

lp://qastaging/~artmello/webbrowser-app/webbrowser-app-statesaver updated on 2014-07-29

643. By Arthur Mello on 2014-07-29: Remove option to restore all tabs
Add option to restore the last visited url

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2014-07-29:

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/webbrowser-app-ci/983/rebuild

review: Needs Fixing (continuous-integration)

Revision history for this message

Arthur Mello (artmello) wrote on 2014-07-29:

I removed the support for restoring all the tabs and return the code to restore only the last current url. I tested the support for only the last current url on desktop and on last device image and it seems to be working as expected.

Took me some time debugging the error reported by Bill about restoring tabs. So basically I had added a "urls" property to the TabsModel component that stored a list of all urls opened on tabs. Since when restoring this list I need to create all webviews and populate the model correctly I was using a property on the Browser component with the value of this list. This Browser's property was the one stored by StateSaver. When this property was restored I call the properly function to create the tabs.

This seems to work correctly on desktop but on device the Browser property seems not to be updated as frequently as the TabsModel one. With this the wrong data was stored by StateSaver. If we try to print the value of the property (for debug purposes) the value is the same as the TabModel one. But if we call something like this console.log(browser["tabsUrls"]) the values was not always updated with the one from TabModels. Not sure why this is happening and I was not able to reproduce that on a simple qml code.

While we think on a better way to do that. I keep only the last current url code

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2014-07-31:

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/webbrowser-app-ci/994/rebuild

review: Approve (continuous-integration)

Revision history for this message

Olivier Tilloy (osomon) wrote on 2014-08-12:

Superseded by https://code.launchpad.net/~osomon/webbrowser-app/persist-open-tabs/+merge/229920.
Thanks for your initial work on this Arthur!

review: Disapprove

Unmerged revisions

643. By Arthur Mello on 2014-07-29: Remove option to restore all tabs
Add option to restore the last visited url
642. By Arthur Mello on 2014-07-25: Restore all opened urls
641. By Arthur Mello on 2014-07-25: Create a property on TabsModel to store all opened urls
640. By Arthur Mello on 2014-07-25: Make sure that a new tab is not restored when created
639. By Arthur Mello on 2014-07-24: Change the WebApp implementation to work on the same way as webbrowser
638. By Arthur Mello on 2014-07-24: Implement the same strategy used on Oxide to WebKit
637. By Arthur Mello on 2014-07-24: Make sure that if a url is provided as a parameter to webbrowser it will override the state saved url
636. By Arthur Mello on 2014-07-23: Merge with trunk
635. By Arthur Mello on 2014-07-23: Change the property saved to fix the issue of not saving the correct values
634. By Arthur Mello on 2014-07-22: If a specialized url was provided to the webapp it is used instead of the state saved one