Closed Bug 962212 Opened 6 years ago Closed 6 years ago

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

Categories

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

All
Windows 8.1
enhancement

Tracking

(Not tracked)

VERIFIED FIXED
Firefox 29

People

(Reporter: mbrubeck, Assigned: mbrubeck)

References

Details

(Whiteboard: [feature] p=3)

Attachments

(3 files, 1 obsolete file)

Attached patch patch (obsolete) — Splinter Review
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.)
Attachment #8363159 - Flags: review?(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">

:)
Attachment #8363159 - Flags: review?(ally) → feedback+
(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.)
(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.
This was split out from the main patch.
Attachment #8364705 - Flags: review?(ally)
Attached patch patch v2Splinter Review
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?
Attachment #8363159 - Attachment is obsolete: true
Attachment #8364707 - Flags: review?(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
Attachment #8364705 - Flags: review?(ally) → review+
Comment on attachment 8364707 [details] [diff] [review]
patch v2

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

Agreed on the followup bug. Ship it!
Attachment #8364707 - Flags: review?(ally) → review+
Blocks: metrov1it23
Priority: -- → P2
QA Contact: jbecerra
QA note: This can be tested by toggling "metro.private_browsing.enabled" in about:config.
https://hg.mozilla.org/mozilla-central/rev/0d1c2194dd03
https://hg.mozilla.org/mozilla-central/rev/05075ab03a2b
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 29
No longer blocks: metrobacklog
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?
Flags: needinfo?(mbrubeck)
Status: RESOLVED → VERIFIED
Attached patch typo fixSplinter Review
(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.
Attachment #8369468 - Flags: review?(azasypkin)
Flags: needinfo?(mbrubeck)
Attachment #8369468 - Flags: review?(azasypkin) → review+
You need to log in before you can comment on or make changes to this bug.