[Merge] lp:~artmello/webbrowser-app/webbrowser-app-private_browsing into lp:webbrowser-app

[Olivier Tilloy]

Review: Needs Fixing

Thanks for working on this. I found a number of issues, so here they are, in no particular order:

Error message when switching to private mode: "OxideQQuickWebView: incognito can only be set during construction". The value of the incognito property needs to be set at construction time. In BrowserTab, in load() and in the Component.onCompleted handler, you need to pass its value in the second argument of incubateObject(). Which means that the BrowserTab component needs to grow a new 'incognito' property. And similarly, in Browser.qml, the three instances of "tabComponent.createObject(…)" need to be passed the value of the incognito property.

In onTriggered of private mode action (and everywhere else), please use enclosing curly brackets for if/else clauses, even one-liners.

In NewPrivateTabView.qml, there doesn’t seem to be a good reason for using a column with two labels. Please use one label, with only one string (with a "\n" to separate the two sentences).

Tab previews are being stored under ~/.cache/webbrowser-app/captures/. Ideally, previews for private tabs should never be written to disk. Can you experiment with this? In BrowserTab.qml, instead of calling result.saveToFile(…), try setting preview to result.url. Hopefully, result.url is valid throughout the lifetime of the app. If it’s not, we might need to store previews on disk, but in that case we would do it under a different directory, and ensure that this directory is entirely deleted when leaving private mode, and when the app is closed/started (in case the app crashed and the directory wasn’t deleted).
Another option that comes to mind (just thinking out loud here) is a custom image provider for tab previews, that would write files to disk in normal mode, and cache them in memory only in private mode. If we end up needing this, let’s agree to do it in a subsequent MR though.

In Chrome.qml, the type of property buttonIconColor should be color, not string. And it shouldn’t be a top-level property. Put it on the FocusScope, or alternatively in the internal object.

In Browser.qml, there is a lot of code duplication between newTabViewLoader and newPrivateTabViewLoader. Can you please merge them into one loader that loads either one component or the other based on the value of the 'incognito' property of the tab/webview?

In Browser.qml, in the onLoadEvent handler, I would do the check on browser.state in a separate if() branch:
  if (browser.state == "private") {
      return
  }

Instead of using a state to reflect the current mode, I would suggest using a boolean property, and I would call it "incognito", for consistency with the oxide API. This way, it will make it easier to grep for code that deals with private mode. Of course the user-visible strings are fine with the "private" vocabulary, no need to change that.

In Browser.qml, there is some code duplication between the instance of TabsModel and privateTabsModelComponent. This can be factored out using a Connections element which targets the current tabs model.

Can the leavePrivateModeDialog component be made into a standalone component, in its own QML file? That would simplify writing an autopilot emulator for it, and would avoid adding too much code to Browser.qml, which is already way too big.

In BrowserTestCaseBase, toggle_private_mode() should probably check the current state before toggling it, and verify that it has changed after clicking the action.

In test_private.py:
  - the copyright notice is incorrect, the only year this file is copyrighted for is 2015.
  - go_into_private_mode() doesn’t seem very useful, as all it does is call toggle_private_mode()
  - as mentioned above, if the dialog is made a standalone component, you’ll be able to write a custom autopilot emulator for it, and thus it can have custom method to accept() and cancel() nicely encapsulated
  - we will need at least an additional test to check that URLs are not stored in the history database while in private mode

-- 
https://code.launchpad.net/~artmello/webbrowser-app/webbrowser-app-private_browsing/+merge/257673
Your team Ubuntu Phablet Team is subscribed to branch lp:webbrowser-app.