Last Comment Bug 722990 - NewTabUtils.jsm uses global Private Browsing state to make decisions
: NewTabUtils.jsm uses global Private Browsing state to make decisions
Status: RESOLVED FIXED
:
Product: Firefox
Classification: Client Software
Component: Tabbed Browser (show other bugs)
: Trunk
: All All
: -- normal (vote)
: Firefox 16
Assigned To: Saurabh Anand [:sawrubh]
:
: Dão Gottwald [:dao]
Mentors:
Depends on: 722857 762938 763468 771892
Blocks: PBnGen 704882 765226
  Show dependency treegraph
 
Reported: 2012-01-31 22:43 PST by Josh Matthews [:jdm] (on vacation until Dec 5)
Modified: 2013-12-27 14:24 PST (History)
6 users (show)
ehsan: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Patch v1 (12.18 KB, patch)
2012-05-29 21:02 PDT, Saurabh Anand [:sawrubh]
no flags Details | Diff | Splinter Review
Patch v2 (14.70 KB, patch)
2012-06-07 00:41 PDT, Saurabh Anand [:sawrubh]
no flags Details | Diff | Splinter Review
Patch v3 (5.07 KB, patch)
2012-06-18 16:48 PDT, Saurabh Anand [:sawrubh]
ttaubert: feedback+
Details | Diff | Splinter Review
Patch v4 (6.65 KB, patch)
2012-06-20 00:49 PDT, Saurabh Anand [:sawrubh]
ttaubert: review+
Details | Diff | Splinter Review
Patch v5 (6.67 KB, patch)
2012-06-20 02:17 PDT, Saurabh Anand [:sawrubh]
no flags Details | Diff | Splinter Review

Description Josh Matthews [:jdm] (on vacation until Dec 5) 2012-01-31 22:43:17 PST
The global state is going away. This should probably be checking a docshell or something instead, but we may need to create a window-level PB status for this to hook into instead.
Comment 1 Josh Matthews [:jdm] (on vacation until Dec 5) 2012-05-12 03:39:09 PDT
http://mxr.mozilla.org/mozilla-central/source/browser/modules/NewTabUtils.jsm is the problem file here, but its various singletons that access the Storage object are used in a number of JS files (http://mxr.mozilla.org/mozilla-central/source/browser/base/content/newtab/newTab.js#23). I propose that we change the gLinks, gPinnedLinks and gBlockedLinks into objects that take a window parameter and store a Storage object that remembers the window for future use. We can get rid of the PrivateBrowsingStorage object and add a third parameter to the getLocalStorageForPrincipal that is true or false depending on the value of window.gPrivateBrowsingUI.privateWindow.

So, the main takeaway points:
* most of the singletons have to become doubletons (Links, BlockedLinks, PinnedLinks, and Storage)
* the removal of PrivateBrowsingStorage should wait until bug 722857 lands
* the actual users of g[Pinned|Blocked]Links shouldn't need to be changed, since the objects should just store a reference to a Storage object that is appropriate for the window that initializes them
Comment 2 Josh Matthews [:jdm] (on vacation until Dec 5) 2012-05-12 03:52:08 PDT
Saurabh said he's interested in working on this.
Comment 3 Saurabh Anand [:sawrubh] 2012-05-29 21:02:10 PDT
Created attachment 628203 [details] [diff] [review]
Patch v1
Comment 4 Tim Taubert [:ttaubert] 2012-06-06 08:47:49 PDT
Comment on attachment 628203 [details] [diff] [review]
Patch v1

Removing feedback? flag from this patch as per conversation on IRC. Saurabh said he's following up with an updated patch soon.
Comment 5 Saurabh Anand [:sawrubh] 2012-06-07 00:41:59 PDT
Created attachment 630882 [details] [diff] [review]
Patch v2
Comment 6 Josh Matthews [:jdm] (on vacation until Dec 5) 2012-06-07 03:30:32 PDT
Further changes we'll need to make:
* the storage observers should be unnecessary - private windows have different storage instances than public ones, and there shouldn't be any reason to reset the caches once we leave PB mode for the same reason
* the telemetry for the pref in the patch is incorrect - we need to check the actual value of the pref (eg. http://mxr.mozilla.org/mozilla-central/source/browser/modules/NewTabUtils.jsm#219)
Comment 7 Tim Taubert [:ttaubert] 2012-06-08 04:15:40 PDT
Hey Saurabh, thank you for diving so deep into the code!

I'm actually not sure that we want to keep the current behavior for the New Tab Page and that we really need a separate PrivateBrowsingStorage.

IMHO we shouldn't allow any modification to the newtab page at all when the user is in private browsing mode. This way they still can access all their shortcuts but not drag any strange links on it.

Another option would be to handle it kind of like Chrome does. We could have a specialized newtab page (e.g. about:privatebrowsing) for private browsing mode that shows some useful information to the user. We could also modify about:newtab itself to reflect the browser's state change?

Anyway we didn't really spec out the behavior in PB mode, yet. Adding some people involved in the newtab page feature to find out what kind of behavior we want before we continue spending time in the wrong direction.

(Also, bug 748530 comment #7 seems relevant.)
Comment 8 Tim Taubert [:ttaubert] 2012-06-08 04:24:08 PDT
(In reply to Tim Taubert [:ttaubert] from comment #7)
> Another option would be to handle it kind of like Chrome does. We could have
> a specialized newtab page (e.g. about:privatebrowsing) for private browsing
> mode that shows some useful information to the user.

This would solve confusion for users affected by bug 762438.
Comment 9 :Ehsan Akhgari 2012-06-08 07:35:17 PDT
The goal of this bug is not to design a new interaction for new tab pages in PB mode.  The goal here is to make NewTabUtils.jsm and friends stop accessing the global PB mode flag.

Once we get all of the foundation pieces, we can discuss the UI details of per-window PB mode, but we're not quite there yet, so let's keep this bug in the scope it was filed for!  :-)
Comment 10 Tim Taubert [:ttaubert] 2012-06-08 08:07:27 PDT
(In reply to Ehsan Akhgari [:ehsan] from comment #9)
> The goal of this bug is not to design a new interaction for new tab pages in
> PB mode.  The goal here is to make NewTabUtils.jsm and friends stop
> accessing the global PB mode flag.

Yes, you're right. I thought about this too when writing the comment. The thing is though that the patch converts singletons to doubletons and touches a lot of stuff that isn't necessary and would have to be reverted if we can decide about a simpler solution for new tab pages in PB mode.

I could probably open another bug, blocking this one.
Comment 11 :Ehsan Akhgari 2012-06-08 08:15:11 PDT
The question is, do we have a UI decision on how the new tab should work in PB mode *right now*?  If we don't, then I don't want to stall this bug waiting for that to happen.

Arguably this is a decision which should have been made when the new tab window was being implemented.  Unfortunately it didn't happen, and I wasn't looped in either...
Comment 12 Tim Taubert [:ttaubert] 2012-06-08 08:50:14 PDT
(In reply to Ehsan Akhgari [:ehsan] from comment #11)
> The question is, do we have a UI decision on how the new tab should work in
> PB mode *right now*?  If we don't, then I don't want to stall this bug
> waiting for that to happen.

I want to find out if this involves a longer discussion or if UX agrees about a certain behavior. I don't want to block per-window private browsing any longer. Will ping all the people involved to get some quick feedback.

> Arguably this is a decision which should have been made when the new tab
> window was being implemented.  Unfortunately it didn't happen, and I wasn't
> looped in either...

Right :|
Comment 13 Tim Taubert [:ttaubert] 2012-06-11 07:42:17 PDT
With bug 763468 implemented users can still open "about:newtab" by simply typing it into the address bar. I'm not sure how we should behave in this probably very rare situation. Should we just make "about:newtab" immutable? Should we redirect to "about:privatebrowsing"?
Comment 14 :Ehsan Akhgari 2012-06-11 08:17:23 PDT
(In reply to Tim Taubert [:ttaubert] from comment #13)
> With bug 763468 implemented users can still open "about:newtab" by simply
> typing it into the address bar. I'm not sure how we should behave in this
> probably very rare situation. Should we just make "about:newtab" immutable?
> Should we redirect to "about:privatebrowsing"?

I don't think we should do anything if the user types in the URL manually.  We should just show them the new tab page.
Comment 15 Tim Taubert [:ttaubert] 2012-06-11 08:21:27 PDT
(In reply to Ehsan Akhgari [:ehsan] from comment #14)
> (In reply to Tim Taubert [:ttaubert] from comment #13)
> > With bug 763468 implemented users can still open "about:newtab" by simply
> > typing it into the address bar. I'm not sure how we should behave in this
> > probably very rare situation. Should we just make "about:newtab" immutable?
> > Should we redirect to "about:privatebrowsing"?
> 
> I don't think we should do anything if the user types in the URL manually. 
> We should just show them the new tab page.

And allow them to modify its data persistently in PB mode? Or behave like we do now and forget all the modifications after returning from PB?
Comment 16 :Ehsan Akhgari 2012-06-11 08:31:12 PDT
(In reply to Tim Taubert [:ttaubert] from comment #15)
> (In reply to Ehsan Akhgari [:ehsan] from comment #14)
> > (In reply to Tim Taubert [:ttaubert] from comment #13)
> > > With bug 763468 implemented users can still open "about:newtab" by simply
> > > typing it into the address bar. I'm not sure how we should behave in this
> > > probably very rare situation. Should we just make "about:newtab" immutable?
> > > Should we redirect to "about:privatebrowsing"?
> > 
> > I don't think we should do anything if the user types in the URL manually. 
> > We should just show them the new tab page.
> 
> And allow them to modify its data persistently in PB mode? Or behave like we
> do now and forget all the modifications after returning from PB?

Ideally, disallowing pages to be added to the list (instead of allowing that at first and then forgetting about it), and allow all other modifications, as they won't have any privacy implications.
Comment 17 Tim Taubert [:ttaubert] 2012-06-13 08:50:40 PDT
(In reply to Ehsan Akhgari [:ehsan] from comment #16)
> Ideally, disallowing pages to be added to the list (instead of allowing that
> at first and then forgetting about it), and allow all other modifications,
> as they won't have any privacy implications.

Sounds like a good solution to me. So we'd just need to remove the PrivateBrowsingStorage and the associated observers from the JSM because we don't need them anymore. If about:newtab detects it has been loaded into a private-browsing docShell then its cells should just ignore any attempts to drop links on it.

Saurabh, is that still something you want to work on? I can definitely give some more guidance if needed.
Comment 18 Saurabh Anand [:sawrubh] 2012-06-18 16:48:51 PDT
Created attachment 634235 [details] [diff] [review]
Patch v3
Comment 19 Saurabh Anand [:sawrubh] 2012-06-18 16:51:50 PDT
As Josh pointed, I have also removed the pin and delete icon(basically frozen the links) in case of PB mode. I have also removed the Storage singleton observer.
Comment 20 Tim Taubert [:ttaubert] 2012-06-19 02:06:57 PDT
Comment on attachment 634235 [details] [diff] [review]
Patch v3

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

We're almost there. Looks pretty good to me.

Please do also remove the lazy service getter definition for "gPrivateBrowsing" at the top of the JSM.

::: browser/base/content/newtab/cells.js
@@ +99,5 @@
>     */
>    handleEvent: function Cell_handleEvent(aEvent) {
> +    // We are basically going to avoid doing anything when the user is in PB mode
> +    // or when the draggedSite is external.
> +    // gDrag.draggedSite is true-ish for internal dragged page.

Let's say: "We're not responding to external drag/drop events when our parent window is in private browsing mode." No need to mention gDrag.draggedSite.

@@ +100,5 @@
>    handleEvent: function Cell_handleEvent(aEvent) {
> +    // We are basically going to avoid doing anything when the user is in PB mode
> +    // or when the draggedSite is external.
> +    // gDrag.draggedSite is true-ish for internal dragged page.
> +    if (inPrivateBrowsingMode && !(!!gDrag.draggedSite))

if (inPrivateBrowsingMode && !gDrag.draggedSite)

::: browser/base/content/newtab/grid.js
@@ +128,5 @@
>      for (let i = 0; i < length; i++) {
>        if (links[i])
>          this.createSite(links[i], cells[i]);
> +        if (inPrivateBrowsingMode)
> +          cells[i].site.node.setAttribute("frozen", "true");

Please revert this change. We allow modifications to the grid, not additions.

::: browser/base/content/newtab/newTab.js
@@ +28,5 @@
> +                                  .rootTreeItem
> +                                  .QueryInterface(Ci.nsIInterfaceRequestor)
> +                                  .getInterface(Ci.nsIDOMWindow)
> +                                  .wrappedJSObject
> +                                  .gPrivateBrowsingUI.privateWindow;

We should also check if ("gPrivateBrowsingUI" in domWindow), just to make sure.

::: browser/modules/NewTabUtils.jsm
@@ +59,1 @@
>    get currentStorage() {

We can remove this getter and its usage. Just s/currentStorage/domStorage/ everywhere.
Comment 21 Tim Taubert [:ttaubert] 2012-06-19 02:10:26 PDT
Did you run the tests in browser/base/content/test/newtab/? There should be some in there that should fail :) Please make sure the tests are running.
Comment 22 Saurabh Anand [:sawrubh] 2012-06-20 00:49:17 PDT
Created attachment 634806 [details] [diff] [review]
Patch v4
Comment 23 Tim Taubert [:ttaubert] 2012-06-20 00:57:24 PDT
Comment on attachment 634806 [details] [diff] [review]
Patch v4

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

Thank you, Saurabh. This looks really good to me. Can you please fix the two nits I mentioned below, upload the updated patch and mark it as checkin-needed?

::: browser/base/content/newtab/newTab.js
@@ +32,5 @@
> +
> +let inPrivateBrowsingMode = false;
> +
> +if ("gPrivateBrowsingUI" in chromeWin)
> +  inPrivateBrowsingMode = chromeWin.gPrivateBrowsingUI.privateWindow;                                  

Nit: There's a lot of trailing white space at this line, please remove it.

::: browser/modules/NewTabUtils.jsm
@@ +19,1 @@
>  XPCOMUtils.defineLazyModuleGetter(this, "Dict", "resource://gre/modules/Dict.jsm");

I forgot about this, please remove it as well. The PrivateBrowsingStorage was the only consumer of it.
Comment 24 Saurabh Anand [:sawrubh] 2012-06-20 02:17:21 PDT
Created attachment 634817 [details] [diff] [review]
Patch v5
Comment 25 Tim Taubert [:ttaubert] 2012-06-20 02:43:28 PDT
Comment on attachment 634817 [details] [diff] [review]
Patch v5

As per conversation on IRC, waiting for try push before checking in.
Comment 26 Saurabh Anand [:sawrubh] 2012-06-20 03:01:17 PDT
For breaking news(pun intended), check out : https://tbpl.mozilla.org/?tree=Try&rev=81ad018dc43a
Comment 27 Saurabh Anand [:sawrubh] 2012-06-20 09:39:30 PDT
Proudly present a lush green tree : https://tbpl.mozilla.org/?tree=Try&rev=81ad018dc43a. Enjoy landing ;)
Comment 29 Ed Morley [:emorley] 2012-06-21 04:07:01 PDT
https://hg.mozilla.org/mozilla-central/rev/cbba65c8d180

Note You need to log in before you can comment on or make changes to this bug.