962212 - Add basic private browsing support to Metro Firefox, disabled by default

Status: VERIFIED FIXED

(Firefox for Metro Graveyard :: General, enhancement, P2)

Description

[Matt Brubeck (:mbrubeck)]

Attachment: patch

This is the first chunk of work for private browsing in Metro (bug 771188).  It adds a preference to enable private browsing.  The pref is disabled by default until UX work and testing are complete, but can be enabled in about:config for testing and development.

When the pref is enabled, there is a "New Private Tab" button in the navbar menu.  Private tabs are displayed in the tab strip like regular tabs, but with a solid purple background in place of a thumbnail.  (Both of these UI elements will probably change during the design work in bug 957165.)

Comment 1

[Allison Naaktgeboren :ally]

Comment on attachment 8363159 [details] [diff] [review]
patch

Review of attachment 8363159 [details] [diff] [review]:
-----------------------------------------------------------------

I'm really concerned that the patch crashes metro when one tries to switch from metro to desktop with a private tab open. This is a work in progress feature, so there will be follow up work, but I don't see anything in the other bug that would address that. What do we want to do about that?

Otherwise, you rock.

::: browser/metro/base/content/browser-ui.js
@@ +197,5 @@
>      messageManager.removeMessageListener("Browser:MozApplicationManifest", OfflineApps);
> +
> +    Services.prefs.removeObserver(debugServerStateChanged, this);
> +    Services.prefs.removeObserver(debugServerPortChanged, this);
> +    Services.prefs.removeObserver("app.crashreporter.autosubmit", this);

I'm curious why these appear in this patch. Is this just unrelated clean up?

@@ +904,5 @@
>      }
>      // FIXME Bug 720575 - Don't capture thumbnails for SVG or XML documents as
>      //       that currently regresses Talos SVG tests.
> +    let browser = aTab.browser;
> +    let doc = browser.contentDocument;

This is also clean up to better handle background tabs or is this directly related?

::: browser/metro/base/content/browser.js
@@ +1260,5 @@
> +  this._private = false;
> +  if ("private" in aParams) {
> +    this._private = aParams.private;
> +  } else if (aOwner) {
> +    this._private = aOwner.private;

Why do we need this case? (aka what do you know about the aOwner param that I don't? :) )

::: browser/metro/locales/en-US/chrome/browser.dtd
@@ +104,5 @@
>  <!ENTITY newTab.key "t">
>  <!ENTITY newTab2.key "n">
>  <!ENTITY closeTab.key "w">
> +<!-- Private browsing (control+shift+key) -->
> +<!ENTITY newPrivateTab.key "p">

:)

Comment 2

[Matt Brubeck (:mbrubeck)]

(In reply to :Ally Naaktgeboren from comment #1)
> I'm really concerned that the patch crashes metro when one tries to switch
> from metro to desktop with a private tab open.

I couldn't reproduce this.  Could you get any details about the crash?  Was this in a debug build?  (I just realized I haven't tested with --enable-debug yet.)

> > +    Services.prefs.removeObserver(debugServerStateChanged, this);
> > +    Services.prefs.removeObserver(debugServerPortChanged, this);
> > +    Services.prefs.removeObserver("app.crashreporter.autosubmit", this);
> 
> I'm curious why these appear in this patch. Is this just unrelated clean up?

Yes.  I can split this into a separate patch.

> > +    let browser = aTab.browser;
> 
> This is also clean up to better handle background tabs or is this directly
> related?

We need to pass the tab instead of the browser to this function, so it can access aTab.isPrivate.

> > +  } else if (aOwner) {
> > +    this._private = aOwner.private;
> 
> Why do we need this case? (aka what do you know about the aOwner param that
> I don't? :) )

When a link from one tab opens in a new tab, the new tab should inherit the private flag from the old tab.  (Otherwise, clicking links in a private tab would not be safe.)

Comment 3

[Allison Naaktgeboren :ally]

(In reply to Matt Brubeck (:mbrubeck) from comment #2)
> (In reply to :Ally Naaktgeboren from comment #1)
> > I'm really concerned that the patch crashes metro when one tries to switch
> > from metro to desktop with a private tab open.
> 
> I couldn't reproduce this.  Could you get any details about the crash?  Was
> this in a debug build?  (I just realized I haven't tested with
> --enable-debug yet.)
> 

Yes it was. I'll look for about:crashes, but since I wipe profiles regularly during development, I might have deleted it already.

> > Why do we need this case? (aka what do you know about the aOwner param that
> > I don't? :) )
> 
> When a link from one tab opens in a new tab, the new tab should inherit the
> private flag from the old tab.  (Otherwise, clicking links in a private tab
> would not be safe.)

Gotcha. Thanks for educating me.

Comment 4

[Matt Brubeck (:mbrubeck)]

Attachment: preliminary cleanup: add missing removeObserver calls

This was split out from the main patch.

Comment 5

[Matt Brubeck (:mbrubeck)]

Attachment: patch v2

No significant changes from the previous version, except for splitting out the cleanup patch.

I still couldn't reproduce the crash on "Relaunch in desktop" (though I did notice that my debug build hangs visibly for a couple of seconds before restarting).  If you can still reproduce that after this lands, could you file a follow-up bug?

Comment 6

[Allison Naaktgeboren :ally]

Comment on attachment 8364705 [details] [diff] [review]
preliminary cleanup: add missing removeObserver calls

Review of attachment 8364705 [details] [diff] [review]:
-----------------------------------------------------------------

thank you for setting a good example. :) Ship it

Comment 7

[Allison Naaktgeboren :ally]

Comment on attachment 8364707 [details] [diff] [review]
patch v2

Review of attachment 8364707 [details] [diff] [review]:
-----------------------------------------------------------------

Agreed on the followup bug. Ship it!

Comment 8

[Matt Brubeck (:mbrubeck)]

https://hg.mozilla.org/integration/fx-team/rev/0d1c2194dd03
https://hg.mozilla.org/integration/fx-team/rev/05075ab03a2b

Comment 9

[Matt Brubeck (:mbrubeck)]

QA note: This can be tested by toggling "metro.private_browsing.enabled" in about:config.

Comment 10

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/0d1c2194dd03
https://hg.mozilla.org/mozilla-central/rev/05075ab03a2b

Comment 11

[Manuela Muntean [Away]]

Verified as fixed, for iteration #23, with latest Nightly on Win 8.1 64-bit: after toggling "metro.private_browsing.enabled" in about:config, I can indeed see a "New Private Tab" button in the navbar menu, which opens a private tab.

Also, I was able to smoothly switch from Metro to Desktop mode.

The issue that I encountered though is that if I want to open a link from a private tab, the new tab opening is a regular one, not a private one. Does anyone have any suggestions/thoughts?

Comment 12

[Matt Brubeck (:mbrubeck)]

Attachment: typo fix

(In reply to Manuela Muntean [:Manuela] [QA] from comment #11)
> The issue that I encountered though is that if I want to open a link from a
> private tab, the new tab opening is a regular one, not a private one. Does
> anyone have any suggestions/thoughts?

This was caused by a line of code that I updated incorrectly when I renamed a property.  Here's the 1-character fix, and an update to the test to catch this bug if it recurs.

Comment 13

[Matt Brubeck (:mbrubeck)]

Comment on attachment 8369468 [details] [diff] [review]
typo fix

https://hg.mozilla.org/mozilla-central/rev/067123df4f77