CustomizableUI should have a way to not add widgets to private windows

RESOLVED FIXED in Firefox 28

Status

()

Firefox
Toolbars and Customization
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: zer0, Assigned: mconley)

Tracking

(Blocks: 1 bug)

Trunk
Firefox 28
x86
All
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [Australis:M8][Australis:P1])

Attachments

(1 attachment, 3 obsolete attachments)

(Reporter)

Description

5 years ago
CustomizableUI.createWidget adds the widgets to all windows, even the windows that are in private mode. In jetpack, by default, an add-on is not supposed to be applied on windows in private browsing mode unless is explicitly requested. Unfortunately I didn't found a way to do so using CustomizableUI. The workaround I'm using now is set the `display` to `none` for the nodes are added in private windows if the add-on doesn't have the "privilege" to do that.

It would be great if it could be possible avoiding adding such node at all (maybe returns `null` in `onBuild` instead of returns the node?).
(Reporter)

Updated

5 years ago
Blocks: 872617
Whiteboard: [Australis:M?]
(Reporter)

Updated

5 years ago
Summary: CustomizableUI should have a way to don't add widgets to private window (if specified) → CustomizableUI should have a way to don't add widgets to private windows
Whiteboard: [Australis:M?] → [Australis:M?][Australis:P1]

Updated

5 years ago
Summary: CustomizableUI should have a way to don't add widgets to private windows → CustomizableUI should have a way to not add widgets to private windows
Assignee: nobody → mconley
Created attachment 772876 [details] [diff] [review]
Patch v1.1

Whoops - forget the tests.
Attachment #772870 - Attachment is obsolete: true
Comment on attachment 772876 [details] [diff] [review]
Patch v1.1

Hey Gijs, how does this look to you?

I don't believe the isWindowPrivate function is too expensive, performance-wise...I'll push to try though, just to be sure.
Attachment #772876 - Flags: review?(gijskruitbosch+bugs)
Comment on attachment 772876 [details] [diff] [review]
Patch v1.1

Er - scratch that. With this patch, I can't seem to move items around anymore in customize mode. Needs more time.
Attachment #772876 - Flags: review?(gijskruitbosch+bugs)
Created attachment 773284 [details] [diff] [review]
Patch v1.2

Ah - I forgot to set showInPrivateBrowsing to true for the built-ins.
Attachment #772876 - Attachment is obsolete: true
Comment on attachment 773284 [details] [diff] [review]
Patch v1.2

Ok, I think this is ready for review.
Attachment #773284 - Flags: review?(gijskruitbosch+bugs)
Just a note Gijs, if you wanted to test this, you'll need to apply bug 877450 first, since that's what I based the patch on top of.

Comment 9

5 years ago
Comment on attachment 773284 [details] [diff] [review]
Patch v1.2

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

r=me, assuming you've audited all our uses of build area nodes (and some of the wrappers, I guess) so that we're not going to error out in places because there's no node corresponding to the widget. I think you have but Splinter not having full-file context means I'm not 100% sure and I figured I'd bring it up anyway.

::: browser/components/customizableui/src/CustomizableUI.jsm
@@ +296,5 @@
>  
>    buildArea: function(aArea, aPlacements, aAreaNode) {
>      let document = aAreaNode.ownerDocument;
>      let window = document.defaultView;
> +    let inPrivateWindow = PrivateBrowsingUtils.isWindowPrivate(window);

Part of me thinks we should delay checking this until we really need it, as this is run on startup a few times and will be a little sadmaking. Another part of me thinks it's unlikely to make a difference as it's a small self-contained module... Maybe leave for now but move if we do regress performance? I'll trust your judgment either way.

@@ +330,5 @@
>        }
>  
> +      if (provider == CustomizableUI.PROVIDER_API) {
> +        let widget = gPalette.get(id);
> +        if (!widget.showInPrivateBrowsing && inPrivateWindow) {

If we do keep the module load early, add the inPrivateWindow to the top if of this block.

@@ +493,5 @@
>      }
>  
> +    let showInPrivateBrowsing = gPalette.has(aWidgetId)
> +                              ? gPalette.get(aWidgetId).showInPrivateBrowsing
> +                              : true;

All the XUL widgets default to true, but API widgets default to false? I understand that defaulting to false might make more sense to prevent (Jetpack) add-ons from doing things they shouldn't do in private browsing, but I'm not sure I like this. Making it default to true also solves the other point I had with the repetition in CustomizableWidgets.jsm. Unless you object, I guess I'd prefer if all widgets defaulted to true for showInPrivateBrowsing, and then jetpack's wrapper can choose to always pass false instead.

::: browser/components/customizableui/src/CustomizableWidgets.jsm
@@ +72,5 @@
>      viewId: "PanelUI-history",
>      removable: true,
>      defaultArea: CustomizableUI.AREA_PANEL,
>      allowedAreas: [CustomizableUI.AREA_PANEL, CustomizableUI.AREA_NAVBAR],
> +    showInPrivateBrowsing: true,

Possibly moot given my comment in CustomizableUI.jsm, but: I don't think we have builtin widgets that we don't show for private windows, do we? In which case, instead of doing this for all the widgets, can we make normalizeWidget default to true for builtin widgets?

::: browser/components/customizableui/test/browser_885530_showInPrivateBrowsing.js
@@ +32,5 @@
> +        is(priv1.document.getElementById("some-widget"), null,
> +           "Private window priv1 should not have found test widget.");
> +        is(priv2.document.getElementById("some-widget"), null,
> +           "Private window priv2 should not have found test widget.");
> +      }

Some nits:
1) Not sure why we need more than 1 window of each?
2) Can you refactor the checking function(s) to reduce duplication? In particular, given only 2 windows, I think this might help:

function checkCorrectPresence(win, privateWin, present, privatePresent) {
 is(win.document.getElementById("some-widget"), present, "Plain window should " + (present ? "" : "not ") + " have the test widget");
 is(privateWin.document.getElementById("some-widget"), privatePresent, "Plain window should " + (privatePresent ? "" : "not ") + " have the test widget");
}

and then call checkCorrectPresence(plain1, priv1, true, false);, for *all* checks whether or not the widget is there or not. Maybe you can come up with something simpler still, I didn't think about it tooooo much.

3) can we test new/existing windows in the same test? If you believe it's better not to, then can you add tests for new private windows for widgets that do exist in private windows? They're currently 'missing' from the matrix of 'widget is allowed yes/no; window is pre-existing yes/no'. :-)
Attachment #773284 - Flags: review?(gijskruitbosch+bugs) → review+
Created attachment 773365 [details] [diff] [review]
Patch v1.3 (r+'d by Gijs)

(In reply to :Gijs Kruitbosch from comment #9)
> Comment on attachment 773284 [details] [diff] [review]
> Patch v1.2
> 
> Review of attachment 773284 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> r=me, assuming you've audited all our uses of build area nodes (and some of
> the wrappers, I guess) so that we're not going to error out in places
> because there's no node corresponding to the widget. I think you have but
> Splinter not having full-file context means I'm not 100% sure and I figured
> I'd bring it up anyway.

Yeah, I did a sweep through CustomizableUI, and I *think* I got everybody.

> 
> ::: browser/components/customizableui/src/CustomizableUI.jsm
> @@ +296,5 @@
> >  
> >    buildArea: function(aArea, aPlacements, aAreaNode) {
> >      let document = aAreaNode.ownerDocument;
> >      let window = document.defaultView;
> > +    let inPrivateWindow = PrivateBrowsingUtils.isWindowPrivate(window);
> 
> Part of me thinks we should delay checking this until we really need it, as
> this is run on startup a few times and will be a little sadmaking. Another
> part of me thinks it's unlikely to make a difference as it's a small
> self-contained module... Maybe leave for now but move if we do regress
> performance? I'll trust your judgment either way.
> 

I think I'll leave it for now. Looking at the talos numbers from my try push, I'm not concerned that this adds much in the way of overhead.

> @@ +330,5 @@
> >        }
> >  
> > +      if (provider == CustomizableUI.PROVIDER_API) {
> > +        let widget = gPalette.get(id);
> > +        if (!widget.showInPrivateBrowsing && inPrivateWindow) {
> 
> If we do keep the module load early, add the inPrivateWindow to the top if
> of this block.
> 

Done.

> @@ +493,5 @@
> >      }
> >  
> > +    let showInPrivateBrowsing = gPalette.has(aWidgetId)
> > +                              ? gPalette.get(aWidgetId).showInPrivateBrowsing
> > +                              : true;
> 
> All the XUL widgets default to true, but API widgets default to false? I
> understand that defaulting to false might make more sense to prevent
> (Jetpack) add-ons from doing things they shouldn't do in private browsing,
> but I'm not sure I like this. Making it default to true also solves the
> other point I had with the repetition in CustomizableWidgets.jsm. Unless you
> object, I guess I'd prefer if all widgets defaulted to true for
> showInPrivateBrowsing, and then jetpack's wrapper can choose to always pass
> false instead.
> 

Yeah, good idea. We now default to true.

> ::: browser/components/customizableui/src/CustomizableWidgets.jsm
> @@ +72,5 @@
> >      viewId: "PanelUI-history",
> >      removable: true,
> >      defaultArea: CustomizableUI.AREA_PANEL,
> >      allowedAreas: [CustomizableUI.AREA_PANEL, CustomizableUI.AREA_NAVBAR],
> > +    showInPrivateBrowsing: true,
> 
> Possibly moot given my comment in CustomizableUI.jsm, but: I don't think we
> have builtin widgets that we don't show for private windows, do we? In which
> case, instead of doing this for all the widgets, can we make normalizeWidget
> default to true for builtin widgets?
> 
> :::
> browser/components/customizableui/test/browser_885530_showInPrivateBrowsing.
> js
> @@ +32,5 @@
> > +        is(priv1.document.getElementById("some-widget"), null,
> > +           "Private window priv1 should not have found test widget.");
> > +        is(priv2.document.getElementById("some-widget"), null,
> > +           "Private window priv2 should not have found test widget.");
> > +      }
> 
> Some nits:
> 1) Not sure why we need more than 1 window of each?
> 2) Can you refactor the checking function(s) to reduce duplication? In
> particular, given only 2 windows, I think this might help:
> 
> function checkCorrectPresence(win, privateWin, present, privatePresent) {
>  is(win.document.getElementById("some-widget"), present, "Plain window
> should " + (present ? "" : "not ") + " have the test widget");
>  is(privateWin.document.getElementById("some-widget"), privatePresent,
> "Plain window should " + (privatePresent ? "" : "not ") + " have the test
> widget");
> }
> 
> and then call checkCorrectPresence(plain1, priv1, true, false);, for *all*
> checks whether or not the widget is there or not. Maybe you can come up with
> something simpler still, I didn't think about it tooooo much.
> 

Ok, I think I've made something simpler.

> 3) can we test new/existing windows in the same test? If you believe it's
> better not to, then can you add tests for new private windows for widgets
> that do exist in private windows? They're currently 'missing' from the
> matrix of 'widget is allowed yes/no; window is pre-existing yes/no'. :-)

Done. Good catch.
Attachment #773284 - Attachment is obsolete: true
Attachment #773365 - Flags: review+
Landed in UX as https://hg.mozilla.org/projects/ux/rev/c81f497d7e95
Status: NEW → ASSIGNED
Whiteboard: [Australis:M?][Australis:P1] → [Australis:M8][Australis:P1][fixed-in-ux]

Comment 12

5 years ago
https://hg.mozilla.org/mozilla-central/rev/c81f497d7e95
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Whiteboard: [Australis:M8][Australis:P1][fixed-in-ux] → [Australis:M8][Australis:P1]
Target Milestone: --- → Firefox 28
You need to log in before you can comment on or make changes to this bug.