1193576 - Add a setting to browse privately by default

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Gaia::System::Browser Chrome, defect)

Description

[Kevin Grandon :kgrandon]

View activities for URLs should open in a private browser window when this setting is turned on

Comment 1

[GitHub Autolander Bot]

Attachment: [gaia] jedireza:private-browsing > mozilla-b2g:master

Comment 2

[Reza Akhavan [:jedireza]]

Kevin, I'd like to review these integration test failures with you. The behavior using the web activity isn't producing the same results as using window.open directly. I have the integration tests running (and failing) locally, we need to determine if the assumptions these tests are asserting are still valid. 

https://treeherder.mozilla.org/#/jobs?repo=gaia&revision=95bd00e5c5ac2b3a40aa05ef6248dde602bb71c0

Comment 3

[Kevin Grandon :kgrandon]

Yes, the assertions in these tests look valid and we should make sure they continue to pass. Have you tried reproducing the steps manually with the updated patch? Does it work as expected?

Comment 4

[Reza Akhavan [:jedireza]]

No the behavior is different. Since we're using the web activity, which in turn creates a new `AppWindow` instead of simply calling window.open.

This is my first time reading into the `apps/system/js/app_window.js` code. Also I'm not intimately familiar with what exactly happens in Gaia when window.open is called vs an `AppWindow`.

I'll see if there are some other methods on `AppWindow` that I can call instead of `requestOpen()` that changes it's behavior.

Comment 5

[Reza Akhavan [:jedireza]]

Kevin, is it clear to you what needs to be happening with this integration test?

https://github.com/jedireza/gaia/blob/private-browsing/apps/system/test/marionette/browser_launch_navigate_window_test.js

> Browser - Launch a URL navigates the same window
https://github.com/jedireza/gaia/blob/private-browsing/apps/system/test/marionette/browser_launch_navigate_window_test.js#L7

> opens a new sheet with window.open()
https://github.com/jedireza/gaia/blob/private-browsing/apps/system/test/marionette/browser_launch_navigate_window_test.js#L33

Seems like this test is assuming the behavior of `window.open()`.

Comment 6

[Kevin Grandon :kgrandon]

Sorry, my local environment is a mess since upgrading to node v4 along with the mulet change - tests aren't working right.

I believe this is the problem though: https://github.com/mozilla-b2g/gaia/blob/efbffaf5e65387f0eb3a7ce528a46d8e7fc07204/apps/system/js/wrapper_factory.js#L113

I think we want similar logic here, but I'm not sure if there's a clean way to access it or abstract it. If you copy/paste that logic into your changes in browser.js does it work?

Comment 7

[Reza Akhavan [:jedireza]]

Bummer. I recommend using nvm (https://github.com/creationix/nvm)... it allows you to switch versions of node in each terminal window. It's been a life saver for me.

> I think we want similar logic here, but I'm not sure if there's a clean way to access it or abstract it. If you copy/paste that logic into your changes in browser.js does it work?

I'll give that a shot.

Comment 8

[Reza Akhavan [:jedireza]]

This got one of the failing integration tests to pass. I pushed a commit [1] to the PR. We'll see what the results [2] end up like.

[1] https://github.com/jedireza/gaia/commit/ac95ce94e7036129669a5b153ea49215d9ccf3b1
[2] https://treeherder.mozilla.org/#/jobs?repo=gaia&revision=ac95ce94e7036129669a5b153ea49215d9ccf3b1

Comment 9

[Reza Akhavan [:jedireza]]

The integration test results have changed :)

I'll take a look at them when I get some more time.

Comment 10

[Reza Akhavan [:jedireza]]

Kevin, I just had some of the Gij19 failing tests pass locally for me.

> $ TEST_FILES="apps/system/test/marionette/browser_navigate_chrome_test.js" make test-integration
> ...
>  Browser - Chrome on browser navigation
>    ✓ should show the progressbar (3433ms)
>  1 passing (9s)

and

> $ TEST_FILES="apps/system/test/marionette/browser_navigate_landing_page_test.js" make test-integration
> ...
>  Browser - Navigating from the landing page
>    ✓ navigates the landing page in place (4544ms)
>  1 passing (10s)

These same tests were failing for me locally yesterday and I haven't made any changes since, I just re-ran the tests.

I've re-ran the tests on Tree Herder a couple times today and they seem to still be failing.

Comment 11

[Kevin Grandon :kgrandon]

I'm looking, but this doens't make sense to me. Following the tests and looking at the screenshot things _seem_ to be ok. Doing a few retriggers to check.

Comment 12

[Kevin Grandon :kgrandon]

I took a closer look here, and I think we may be unable to change this to use activities. Looking at browser_navigate_chrome_test.js, it appears to be timing out on the progress bar, which is odd.

In master the progressBar content is: <gaia-progress aria-valuetext="loading" data-l10n-id="gaia-progress-loading" animated="" class="visible" style="" aria-live="polite" role="progressbar"><style scoped="">@import url(/shared/elements/gaia_progress/style.css);</style></gaia-progress>

And in your branch the test gets the progressBar content as: <gaia-progress style="" aria-live="polite" role="progressbar"><style scoped="">@import url(/shared/elements/gaia_progress/style.css);</style></gaia-progress>

As you can see in your branch it's not loading, likely due to the fact that every app has a progress bar, and the selector is fairly generic. It's getting the progress bar from the wrong app window. I believe this is happening because moz activities do not background the current app, and when browsing, we _want_ to background the current app.

We're going to need to handle this a different way - and instead do this in the location where window.open is handled. Perhaps browser.js can export the current setting so you can read it from there, and set the app window as private. Perhaps adding logic to here: https://github.com/mozilla-b2g/gaia/blob/master/apps/system/js/wrapper_factory.js#L32

Comment 13

[GitHub Autolander Bot]

Attachment: [gaia] jedireza:private-browsing-alt > mozilla-b2g:master

Comment 14

[Reza Akhavan [:jedireza]]

Comment on attachment 8664556 [details] [review]
[gaia] jedireza:private-browsing-alt > mozilla-b2g:master

Kevin, we may have a winner. Please take a look at the new PR [1]. All the tests are green [2]. Let me know what changes you'd like to see. I'll squash the commits before a merge.

[1] https://github.com/mozilla-b2g/gaia/pull/31988
[2] https://treeherder.mozilla.org/#/jobs?repo=gaia&revision=a981fa798f3986fbd10b1cdf8ca277d4bf90bc38

Comment 15

[Kevin Grandon :kgrandon]

Comment on attachment 8664556 [details] [review]
[gaia] jedireza:private-browsing-alt > mozilla-b2g:master

Woohoo, this looks good and I think you can land soon. I did leave a few comments on github that I would like you to address/respond to.

My main concern at this point is that opening the browser once the setting is on makes it look like a normal browser (even though tapping on a link will open a private window). I think in order to land this we should make the browser app open in private browsing mode as well.

To do that, the search app would likely just need to read the setting once on startup (probably not an observer), and perform the same logic here: https://github.com/mozilla-b2g/gaia/blob/master/apps/search/js/newtab.js#L29-L46

Let me know what you think. Thanks!

Comment 16

[Reza Akhavan [:jedireza]]

Comment on attachment 8664556 [details] [review]
[gaia] jedireza:private-browsing-alt > mozilla-b2g:master

Made some changes to the PR [1] and left some comments/replies, latest tests are all green [2].

[1] https://github.com/mozilla-b2g/gaia/pull/31988
[2] https://treeherder.mozilla.org/#/jobs?repo=gaia&revision=ea5e82f61239fcec55af8410ca9d0f532ebecf8c

Comment 17

[Kevin Grandon :kgrandon]

Comment on attachment 8664556 [details] [review]
[gaia] jedireza:private-browsing-alt > mozilla-b2g:master

This looks great, thanks for doing that!

Comment 18

[Kevin Grandon :kgrandon]

In master: https://github.com/mozilla-b2g/gaia/commit/8704dfd867d120a937339df5b7071e4d8302032d