Closed Bug 799001 Opened 7 years ago Closed 7 years ago

Stub out the necessary UI to let us open a private browsing window

Categories

(Firefox :: Private Browsing, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 19

People

(Reporter: ehsan, Assigned: ehsan)

References

Details

Attachments

(4 files, 5 obsolete files)

It's time that we make per-window private browsing builds do something useful.  I have a set of patches which let you open up a private browsing window.  Not everything works right now, but that's ok, since this will not be part of the default build.  This will be part of bug 729865.
Attached patch Part 1 (obsolete) — Splinter Review
Assignee: nobody → ehsan
Status: NEW → ASSIGNED
Attachment #669002 - Flags: review?(dao)
Attached patch Part 2 (obsolete) — Splinter Review
Attachment #669003 - Flags: review?(dao)
Attached patch Part 3Splinter Review
Attachment #669004 - Flags: review?(josh)
Attachment #669004 - Flags: review?(josh) → review+
Comment on attachment 669002 [details] [diff] [review]
Part 1

>--- a/browser/base/content/browser-menubar.inc
>+++ b/browser/base/content/browser-menubar.inc
>@@ -22,6 +22,12 @@
>                           accesskey="&newNavigatorCmd.accesskey;"
>                           key="key_newNavigator"
>                           command="cmd_newNavigator"/>
>+#ifdef MOZ_PER_WINDOW_PRIVATE_BROWSING
>+                <menuitem id="menu_newPrivateWindow"
>+                          label="&newPrivateWindow.label;"
>+                          command="Tools:PrivateBrowsing"
>+                          key="key_privatebrowsing"/>
>+#endif

Needs an access key.
Attachment #669002 - Flags: review?(dao) → review-
Comment on attachment 669003 [details] [diff] [review]
Part 2

>+#ifdef MOZ_PER_WINDOW_PRIVATE_BROWSING
>+
>+// We define a new gPrivateBrowsingUI object, as the per-window PB implementation
>+// is completely different to the global PB one.  Specifically, the per-window
>+// PB implementation does not expose many APIs on the gPrivateBrowsingUI object,
>+// and only uses it as a way to initialize and uninitialize private browsing
>+// windows.  While we could use #ifdefs all around the global PB mode code to
>+// make it work in both modes, the amount of duplicated code is small and the
>+// code is much more readable this way.

Use # rather than // here.

>+  init: function PBUI_init() {
>+    // Do nothing for normal windows
>+    if (!PrivateBrowsingUtils.isWindowPrivate(window)) {
>+      return;
>+    }
>+
>+    // Disable the Clear Recent History... menu item when in PB mode
>+    // temporary fix until bug 463607 is fixed
>+    document.getElementById("Tools:Sanitize").setAttribute("disabled", "true");
>+
>+    // Adjust the window's title
>+    let docElement = document.documentElement;
>+    docElement.setAttribute("title",
>+      docElement.getAttribute("title_privatebrowsing"));
>+    docElement.setAttribute("titlemodifier",
>+      docElement.getAttribute("titlemodifier_privatebrowsing"));
>+    docElement.setAttribute("privatebrowsingmode", "temporary");
>+    gBrowser.updateTitlebar();

On OS X, this code runs in windows that don't have gBrowser. PBUI_onEnterPrivateBrowsing takes care of this by checking window.location.href == getBrowserURL().

>+  addInitializationCallback: function PBUI_addInitializationCallback(aCallback) {

It looks like this won't be needed if you use PrivateBrowsingUtils.isWindowPrivate(window) in utilityOverlay.js.
Attachment #669003 - Flags: review?(dao) → review-
OS: Mac OS X → All
Hardware: x86 → All
(In reply to Dão Gottwald [:dao] from comment #5)
> >+  addInitializationCallback: function PBUI_addInitializationCallback(aCallback) {
> 
> It looks like this won't be needed if you use
> PrivateBrowsingUtils.isWindowPrivate(window) in utilityOverlay.js.

That code also reads the autoStarted property, and in the global PB case, that depends on gPrivateBrowsingUI to be initialized.  And I just landed some patches in other bugs which replace the privateBrowsingEnabled usage with PrivateBrowsingUtils.  I'd rather keep the initialization callback logic around until we can remove the global PB mode code, at which point the code in utilityOverlay.js can be simplified and we can remove the initialization callback stuff from gPrivateBrowsingUI as well.
Attached patch Part 1 (obsolete) — Splinter Review
With the access key added.
Attachment #669002 - Attachment is obsolete: true
Attachment #669156 - Flags: review?(dao)
Attached patch Part 2Splinter Review
Attachment #669003 - Attachment is obsolete: true
Attachment #669157 - Flags: review?(dao)
(In reply to Ehsan Akhgari [:ehsan] from comment #6)
> (In reply to Dão Gottwald [:dao] from comment #5)
> > >+  addInitializationCallback: function PBUI_addInitializationCallback(aCallback) {
> > 
> > It looks like this won't be needed if you use
> > PrivateBrowsingUtils.isWindowPrivate(window) in utilityOverlay.js.
> 
> That code also reads the autoStarted property, and in the global PB case,
> that depends on gPrivateBrowsingUI to be initialized.  And I just landed
> some patches in other bugs which replace the privateBrowsingEnabled usage
> with PrivateBrowsingUtils.

It doesn't seem to make much sense to keep autoStarted on the gPrivateBrowsingUI object, as the former is window-independent. You could of course keep it for backwards compatibility, but utilityOverlay.js should just get it from PrivateBrowsingUtils.
Comment on attachment 669156 [details] [diff] [review]
Part 1

>--- a/browser/base/content/browser-appmenu.inc
>+++ b/browser/base/content/browser-appmenu.inc
>@@ -27,6 +27,13 @@
>                       label="&newNavigatorCmd.label;"
>                       command="cmd_newNavigator"
>                       key="key_newNavigator"/>
>+#ifdef MOZ_PER_WINDOW_PRIVATE_BROWSING
>+            <menuitem id="appmenu_newPrivateWindow"
>+                      label="&newPrivateWindow.label;"
>+                      accesskey="&newPrivateWindow.accesskey;"

Items in this menu don't get access keys. (We have a test to verify this.)

>+<!ENTITY newPrivateWindow.label     "New Private Browsing Window">

Can you get ui-review for this? "New Private Window" might be preferable.
(In reply to Dão Gottwald [:dao] from comment #9)
> (In reply to Ehsan Akhgari [:ehsan] from comment #6)
> > (In reply to Dão Gottwald [:dao] from comment #5)
> > > >+  addInitializationCallback: function PBUI_addInitializationCallback(aCallback) {
> > > 
> > > It looks like this won't be needed if you use
> > > PrivateBrowsingUtils.isWindowPrivate(window) in utilityOverlay.js.
> > 
> > That code also reads the autoStarted property, and in the global PB case,
> > that depends on gPrivateBrowsingUI to be initialized.  And I just landed
> > some patches in other bugs which replace the privateBrowsingEnabled usage
> > with PrivateBrowsingUtils.
> 
> It doesn't seem to make much sense to keep autoStarted on the
> gPrivateBrowsingUI object, as the former is window-independent. You could of
> course keep it for backwards compatibility, but utilityOverlay.js should
> just get it from PrivateBrowsingUtils.

Agreed.  As I said, I will make this change in the future.
(In reply to Dão Gottwald [:dao] from comment #10)
> Comment on attachment 669156 [details] [diff] [review]
> Part 1
> 
> >--- a/browser/base/content/browser-appmenu.inc
> >+++ b/browser/base/content/browser-appmenu.inc
> >@@ -27,6 +27,13 @@
> >                       label="&newNavigatorCmd.label;"
> >                       command="cmd_newNavigator"
> >                       key="key_newNavigator"/>
> >+#ifdef MOZ_PER_WINDOW_PRIVATE_BROWSING
> >+            <menuitem id="appmenu_newPrivateWindow"
> >+                      label="&newPrivateWindow.label;"
> >+                      accesskey="&newPrivateWindow.accesskey;"
> 
> Items in this menu don't get access keys. (We have a test to verify this.)

Oh ok, didn't know that!

> >+<!ENTITY newPrivateWindow.label     "New Private Browsing Window">
> 
> Can you get ui-review for this? "New Private Window" might be preferable.

I will, and I'll submit a new version when the ui-review is complete.
Comment on attachment 669156 [details] [diff] [review]
Part 1

Not sure who needs to ui-r this.  Please note that this UI will be disabled for now, and we can iterate on it later on and fix things before enabling it by default.
Attachment #669156 - Flags: ui-review?(madhava)
(In reply to Ehsan Akhgari [:ehsan] from comment #11)
> > It doesn't seem to make much sense to keep autoStarted on the
> > gPrivateBrowsingUI object, as the former is window-independent. You could of
> > course keep it for backwards compatibility, but utilityOverlay.js should
> > just get it from PrivateBrowsingUtils.
> 
> Agreed.  As I said, I will make this change in the future.

You said you'd make it dependent on the complete removal of global PB mode code. I don't see the connection. Why add this code now and drag it around till then?

(In reply to Ehsan Akhgari [:ehsan] from comment #13)
> Comment on attachment 669156 [details] [diff] [review]
> Part 1
> 
> Not sure who needs to ui-r this.  Please note that this UI will be disabled
> for now, and we can iterate on it later on and fix things before enabling it
> by default.

Each string change generates work for a large group of people. Please be deliberate here.
(In reply to comment #14)
> (In reply to Ehsan Akhgari [:ehsan] from comment #11)
> > > It doesn't seem to make much sense to keep autoStarted on the
> > > gPrivateBrowsingUI object, as the former is window-independent. You could of
> > > course keep it for backwards compatibility, but utilityOverlay.js should
> > > just get it from PrivateBrowsingUtils.
> > 
> > Agreed.  As I said, I will make this change in the future.
> 
> You said you'd make it dependent on the complete removal of global PB mode
> code. I don't see the connection. Why add this code now and drag it around till
> then?

No, sorry.  I meant making it dependent on MOZ_PER_WINDOW_PRIVATE_BROWSING.  Our plan is to flip that flag when we're ready to ship per-window PB, and remove all of the code hidden behind it some time after when we're sure that per-window PB is indeed ship quality.  Sorry for the confusion, we're on the same page here. :-)

> (In reply to Ehsan Akhgari [:ehsan] from comment #13)
> > Comment on attachment 669156 [details] [diff] [review]
> > Part 1
> > 
> > Not sure who needs to ui-r this.  Please note that this UI will be disabled
> > for now, and we can iterate on it later on and fix things before enabling it
> > by default.
> 
> Each string change generates work for a large group of people. Please be
> deliberate here.

Agreed.
Comment on attachment 669157 [details] [diff] [review]
Part 2

>+    this._inited = true;
>+
>+    this._initCallbacks.forEach(function (callback) callback.apply());
>+    this._initCallbacks = [];
>+  },
>+
>+  get autoStarted() {
>+    return false; // auto-started PB not supported for now
>+  },
>+
>+  get initialized() {
>+    return this._inited;
>+  },
>+
>+  addInitializationCallback: function PBUI_addInitializationCallback(aCallback) {
>+    if (this._inited)
>+      return;
>+
>+    this._initCallbacks.push(aCallback);
>+  }

So this code is being removed in bug 799780. Not sure why we needed a separate bug for this, but whatever...
Attachment #669157 - Flags: review?(dao) → review+
Attached patch Part 1 (obsolete) — Splinter Review
Attachment #669156 - Attachment is obsolete: true
Attachment #669156 - Flags: ui-review?(madhava)
Attachment #669156 - Flags: review?(dao)
Attachment #669928 - Flags: ui-review?(shorlander)
Attachment #669928 - Flags: review?(dao)
Attachment #669931 - Attachment is patch: false
Attachment #669931 - Attachment mime type: text/plain → image/png
Hey -

UI-r+ed, but with a preference for "New Private Window" rather than "New Private Browsing Window."
Attachment #669928 - Flags: ui-review?(shorlander) → ui-review+
Attached patch Part 1 (obsolete) — Splinter Review
Cool, thanks.  Switched the string to New Private Window.
Attachment #669928 - Attachment is obsolete: true
Attachment #669928 - Flags: review?(dao)
Attachment #670028 - Flags: review?(dao)
Comment on attachment 670028 [details] [diff] [review]
Part 1

>+<!ENTITY newPrivateWindow.label     "New Private Window">
>+<!ENTITY newPrivateWindow.accesskey "B">

needs a different access key
Attachment #670028 - Flags: review?(dao) → review-
Attached patch Part 1Splinter Review
Switched the accesskey to W, and switched Work Offline's accesskey to k.
Attachment #670028 - Attachment is obsolete: true
Attachment #670033 - Flags: review?(dao)
Comment on attachment 670033 [details] [diff] [review]
Part 1

>                 <menuitem id="goOfflineMenuitem"
>                           label="&goOfflineCmd.label;"
>-                          accesskey="&goOfflineCmd.accesskey;"
>+                          accesskey="&goOfflineCmd2.accesskey;"

>-<!ENTITY goOfflineCmd.accesskey "w">
>+<!ENTITY goOfflineCmd2.accesskey "k">

Keep goOfflineCmd.accesskey as the entity name since this change is en-US-specific.
Attachment #670033 - Flags: review?(dao) → review+
Depends on: 817422
Depends on: 854175
You need to log in before you can comment on or make changes to this bug.