1175327 - Move existing Tracking Protection functionality from shield doorhanger to Control Center

Status: VERIFIED FIXED

(Firefox :: General, defect, P1)

Description

[Brian Grinstead [:bgrins]]

We need to move the existing tracking protection content out of the doorhanger that's currently shared with the Mixed Content Blocking feature.

Comment 1

[Brian Grinstead [:bgrins]]

I think that this depends on the subpanel changes happening in Bug 1170759

Comment 2

[Brian Grinstead [:bgrins]]

Looks like this is going to require migrating the current XBL implementation from urlbarBindings.xml into controlcenter/content/panel.inc.xul and browser.js.

Comment 3

[Brian Grinstead [:bgrins]]

Going to claim this one since I've started to look into it a bit

Comment 4

[Brian Grinstead [:bgrins]]

Attachment: tp-control-center.patch

I've gotten to a point with this where it'd be good to stop and get some feedback on the direction I've taken so far.  I've ported the existing UI from urlbarBindings.xml into to the controlcenter panel and added a new browser-trackingprotection.js file that gets preprocessed into browser.js.  I've tried to decouple it from gIdentityHandler as much as possible.

Other notes:
* Using a pref observer instead of reading privacy.trackingprotection.enabled on every change
* I've left the existing MCB panel in place (although moved the TP telemetry parts from there into the new file prevent double reporting).  Seems like removing that functionality will have to wait until MCB is fully moved into CC.
* The CSS changes are not finalized.  I'd like to reuse some of the identity styling but I'm waiting for the subpanel stuff in Bug 1170759 to land before trying to clean that up, since anything in that file will probably need a big rebase after that.

Comment 5

[Tim Taubert [:ttaubert] (inactive)]

Comment on attachment 8624939 [details] [diff] [review]
tp-control-center.patch

Canceling and waiting for the rebased version.

Comment 6

[Tim Nguyen :ntim]

Comment on attachment 8624939 [details] [diff] [review]
tp-control-center.patch

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

A few comments about the SVG (applies to both files).

::: browser/themes/shared/controlcenter/tracking-protection.svg
@@ +2,5 @@
> +<!-- This Source Code Form is subject to the terms of the Mozilla Public
> +   - License, v. 2.0. If a copy of the MPL was not distributed with this
> +   - file, You can obtain one at http://mozilla.org/MPL/2.0/. -->
> +<!DOCTYPE svg PUBLIC "-//W3C//DTD SVG 1.1//EN" "http://www.w3.org/Graphics/SVG/1.1/DTD/svg11.dtd">
> +<svg version="1.1"

nit : DOCTYPE and svg version aren't needed
Also, can we have the attributes on the same line as the tagname ?

@@ +24,5 @@
> +      <use xlink:href="#shape-shield-inner" fill="#000" />
> +      <use xlink:href="#shape-shield-detail" fill="#fff" />
> +    </mask>
> +  </defs>
> +  <use xlink:href="#shape-shield-outer" mask="url(#mask-shield-cutout)" class="icon-default" />

Since the icon-default class is only used once, can't we just set fill="#808080" directly and remove the style declaration ?

Comment 7

[Brian Grinstead [:bgrins]]

Attachment: tp-control-center.patch

Same as Comment 4, but now rebased on top of the subpanel work

Comment 8

[Tim Taubert [:ttaubert] (inactive)]

Comment on attachment 8626643 [details] [diff] [review]
tp-control-center.patch

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

This looks great! Just a few things:

(Sorry for the review delay.)

::: browser/base/content/browser-trackingprotection.js
@@ +2,5 @@
> +# License, v. 2.0. If a copy of the MPL was not distributed with this
> +# file, You can obtain one at http://mozilla.org/MPL/2.0/.
> +
> +let TrackingProtection = {
> +  PREF_ENABLED: "privacy.trackingprotection.enabled",

We need to also support "privacy.trackingprotection.pbmode.enabled". It would be easy to check whether the parent browser window is private and then just consider that for TP.enabled, however we'd probably change the meaning of a few telemetry measurements here...

For TRACKING_PROTECTION_SHIELD we'd probably want to have .enabled report both prefs as we're interested in knowing how many pages track and how many don't

For TRACKING_PROTECTION_ENABLED we shouldn't use both prefs because that would skew the number of people that enabled it manually.

We could as well postpone that to a follow-up to keep bugs smaller. It doesn't change behavior or telemetry as currently implemented.

@@ +12,5 @@
> +
> +  init: function() {
> +    this.updateEnabled();
> +    Services.prefs.addObserver(this.PREF_ENABLED,
> +                               this.updateEnabled.bind(this), false);

Guess we might leak the window if we don't remove it when the window is closed. Should remove the observer in an uninit() method.

@@ +82,5 @@
> +
> +    if (this.isTrackingBlocked) {
> +      this.unblockButton.hidden = false;
> +      this.labelTrackingBlocked.hidden = false;
> +      this.actions.hidden = false;

I think we should do all this in CSS, just have two attributes: one for trackers blocked (tbd) and one for trackers loaded ("block-disabled"). Based on that we can simply have a few CSS rules that will show the right elements. We then wouldn't need resetUI() and updateUI(), onSecurityChange() would simply set the correct attributes.

::: browser/base/content/browser.js
@@ +1450,2 @@
>        Services.telemetry.getHistogramById("TRACKING_PROTECTION_ENABLED")
> +        .add(TrackingProtection.enabled);

Can we move this to the new file too?

::: browser/components/controlcenter/content/panel.inc.xul
@@ +37,5 @@
>        </hbox>
>  
> +      <!-- Tracking Protection Section -->
> +      <hbox id="tracking-protection-container" class="identity-popup-section">
> +        <vbox>

<vbox id="tracking-protection-content" flex="1">

All the #tracking-protection-container selectors should be "...-content" then.

@@ +38,5 @@
>  
> +      <!-- Tracking Protection Section -->
> +      <hbox id="tracking-protection-container" class="identity-popup-section">
> +        <vbox>
> +          <label class="identity-popup-text identity-popup-headline"

Should probably switch to <description> as labels should only go with control elements.

@@ +46,5 @@
> +          <label id="tracking-blocked"
> +                 class="identity-popup-text tracking-protection-label"
> +                 crop="end">
> +            &trackingProtection.detectedBlocked;
> +          </label>

Please don't leave white space here or they will show up when rendering.

crop="end">&trackingProtection.detectedBlocked;</label>

@@ +60,5 @@
> +          </label>
> +
> +          <button id="tracking-actions"
> +            type="menu" label="&trackingContentBlocked.options;"
> +            sizetopopup="none">

Please align those and the following attributes as done in the rest of the file.

@@ +62,5 @@
> +          <button id="tracking-actions"
> +            type="menu" label="&trackingContentBlocked.options;"
> +            sizetopopup="none">
> +            <menupopup>
> +              <menuitem

Can you please turn this into the button that Aislinn shows in her designs? The dropdown doesn't look too nice anyway :)

::: browser/locales/en-US/chrome/browser/browser.dtd
@@ +769,5 @@
>  <!ENTITY mixedContentBlocked2.disabled.message "Protection is disabled">
>  
> +<!ENTITY trackingProtection.title "Tracking Protection">
> +<!ENTITY trackingProtection.detectedBlocked "Attempts to track your online behavior have been blocked.">
> +<!ENTITY trackingProtection.detectedNotBlocked "Tracking elements detected.  You have disabled protection on this site.">

Nit: double space

::: browser/themes/shared/controlcenter/panel.inc.css
@@ +56,5 @@
>  
>  #identity-popup-securityView,
>  #identity-popup-security-content,
> +#identity-popup-permissions-content,
> +#tracking-protection-container {

#tracking-protection-content

@@ +68,5 @@
>  
>  #identity-popup-securityView:-moz-locale-dir(rtl),
>  #identity-popup-security-content:-moz-locale-dir(rtl),
> +#identity-popup-permissions-content:-moz-locale-dir(rtl),
> +#tracking-protection-container:-moz-locale-dir(rtl) {

#tracking-protection-content:...

@@ +237,5 @@
>    background-color: hsla(210,4%,10%,.12);
>    box-shadow: 0 1px 0 hsla(210,4%,10%,.05) inset;
>  }
> +
> +/* Tracking protection section */

Please move this up to be above "/* PERMISSIONS */".

@@ +250,5 @@
> +
> +.tracking-protection-label {
> +  white-space: normal;
> +  max-width: 260px;
> +  margin: 5px 0;

We don't need ".tracking-protection-label", should simply re-use .identity-popup-text.

(By doing that we also get the same margins below the last label in the security and the tracking protection section.)

Comment 9

[Brian Grinstead [:bgrins]]

(In reply to Tim Taubert [:ttaubert] from comment #8)
> @@ +62,5 @@
> > +          <button id="tracking-actions"
> > +            type="menu" label="&trackingContentBlocked.options;"
> > +            sizetopopup="none">
> > +            <menupopup>
> > +              <menuitem
> 
> Can you please turn this into the button that Aislinn shows in her designs?
> The dropdown doesn't look too nice anyway :)

I'll attach a second patch that converts this into a normal button.  I'll use the in-content pref button style (which the designs appear to be using).

Comment 10

[Tim Taubert [:ttaubert] (inactive)]

Can also do this as a follow-up if you'd like that more?

Comment 11

[Brian Grinstead [:bgrins]]

(In reply to Tim Taubert [:ttaubert] from comment #10)
> Can also do this as a follow-up if you'd like that more?

I can attach the patch to Bug 1175239.  May as well use that one since it's already filed.

Comment 12

[Brian Grinstead [:bgrins]]

(In reply to Tim Taubert [:ttaubert] from comment #8)
> ::: browser/base/content/browser-trackingprotection.js
> @@ +2,5 @@
> > +# License, v. 2.0. If a copy of the MPL was not distributed with this
> > +# file, You can obtain one at http://mozilla.org/MPL/2.0/.
> > +
> > +let TrackingProtection = {
> > +  PREF_ENABLED: "privacy.trackingprotection.enabled",
> 
> We need to also support "privacy.trackingprotection.pbmode.enabled". It
> would be easy to check whether the parent browser window is private and then
> just consider that for TP.enabled, however we'd probably change the meaning
> of a few telemetry measurements here...
> 
> For TRACKING_PROTECTION_SHIELD we'd probably want to have .enabled report
> both prefs as we're interested in knowing how many pages track and how many
> don't
> 
> For TRACKING_PROTECTION_ENABLED we shouldn't use both prefs because that
> would skew the number of people that enabled it manually.
> 
> We could as well postpone that to a follow-up to keep bugs smaller. It
> doesn't change behavior or telemetry as currently implemented.

Going to break this part into another bug - I have the rest of the feedback addressed so will resubmit for review and then try to tackle this separately.

Comment 13

[Brian Grinstead [:bgrins]]

Attachment: tp-control-center.patch

Thanks for the review.  I've updated the patch as noted in the comments.  The only things I haven't done are update it to include pbmode and update the buttons.  I also didn't change the whitespace on the menupopup elements since those will immediately go away in Bug 1175239, but I can do that if you'd like.

Comment 14

[Tim Taubert [:ttaubert] (inactive)]

Comment on attachment 8627298 [details] [diff] [review]
tp-control-center.patch

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

Great, thank you! r=me with all the comments addressed.

::: browser/base/content/browser-trackingprotection.js
@@ +7,5 @@
> +
> +  // States for the current page
> +  isTrackingBlocked: false,
> +  isTrackingLoaded: false,
> +  isNoTracking: false,

We don't need those anymore.

@@ +9,5 @@
> +  isTrackingBlocked: false,
> +  isTrackingLoaded: false,
> +  isNoTracking: false,
> +
> +  init: function() {

init() {

And please do the same for the rest of the method definitions in this file.

@@ +17,5 @@
> +
> +    this.updateEnabled = this.updateEnabled.bind(this);
> +    this.updateEnabled();
> +    Services.prefs.addObserver(this.PREF_ENABLED,
> +                               this.updateEnabled, false);

Could also just do:

Services.prefs.addObserver(this.PREF_ENABLED, this, false);

And then rename updateEnabled() to observe(), we could get rid of the awkward .bind(this) dance above that way.

@@ +22,5 @@
> +  },
> +
> +  uninit: function() {
> +    Services.prefs.removeObserver(this.PREF_ENABLED,
> +                               	  this.updateEnabled, false);

Services.prefs.removeObserver(this.PREF_ENABLED, this);

(No third argument.)

@@ +28,5 @@
> +
> +  notifyTelemetryEnabled: function() {
> +	// Telemetry for tracking protection.
> +	Services.telemetry.getHistogramById("TRACKING_PROTECTION_ENABLED")
> +		.add(this.enabled);

Nit: indentation is a little off

@@ +33,5 @@
> +  },
> +
> +  getTelemetryHistogram: function() {
> +    return Services.telemetry.getHistogramById("TRACKING_PROTECTION_EVENTS");
> +  },

Can we please define this above init() like:

get histogram() {
  return Services.telemetry.getHistogramById("TRACKING_PROTECTION_EVENTS");
}

I think that's more in line with other code.

@@ +43,5 @@
> +
> +  onSecurityChange: function(state) {
> +    this.isTrackingBlocked = false;
> +    this.isTrackingLoaded = false;
> +    this.isNoTracking = false;

These can be local variables now.

@@ +62,5 @@
> +      this.isNoTracking = true;
> +    }
> +
> +    this.content.removeAttribute("block-disabled");
> +    this.content.removeAttribute("block-active");

Maybe move those calls to the else-branch of the respective condition below?

::: browser/base/content/browser.js
@@ +1447,5 @@
>            Services.telemetry.getHistogramById("MASTER_PASSWORD_ENABLED").add(mpEnabled);
>          }
>        }, 5000);
>  
> +      TrackingProtection.notifyTelemetryEnabled();

I don't know why this was added down here, it doesn't need to be in the delayed startup routine, it doesn't need to wait for sessionstore.

Please just add that telemetry measurement in TP.init() and remove the extra method.

(Side remark: Why do we even measure that for every browser window we open?? Fine.)

::: browser/themes/shared/controlcenter/panel.inc.css
@@ +214,5 @@
> +
> +.tracking-protection-label {
> +  max-width: 260px;
> +  margin: 5px 0;
> +}

Please remove this class and all its references from the .xul file.

@@ +218,5 @@
> +}
> +
> +#tracking-actions {
> +  margin: 0;
> +}

Without .tracking-protection-label, we can give it a margin of "1em 0 0" and it already looks a little nicer and not too close to the text.

@@ +230,5 @@
> +}
> +#tracking-not-detected,
> +#tracking-protection-content[block-active] #tracking-blocked,
> +#tracking-protection-content[block-disabled] #tracking-loaded {
> +  display: -moz-box;

All these elements are visible by default and we only want to hide them when not needed. Please replace all of the above and below display:none|-moz-box rules with:

#tracking-protection-content[block-active] > #tracking-not-detected,
#tracking-protection-content[block-disabled] > #tracking-not-detected,
#tracking-protection-content:not([block-active]) > #tracking-blocked,
#tracking-protection-content:not([block-active]) > #tracking-action-unblock,
#tracking-protection-content:not([block-disabled]) > #tracking-loaded,
#tracking-protection-content:not([block-disabled]) > #tracking-action-block,
#tracking-protection-content:not([block-active]):not([block-disabled]) > #tracking-actions {
  display: none;
}

And please check again too that these do what we want, I think they should :)

Comment 15

[Brian Grinstead [:bgrins]]

Attachment: tp-control-center.patch

https://treeherder.mozilla.org/#/jobs?repo=try&revision=1e1c4df31ec6

Comment 16

[Brian Grinstead [:bgrins]]

remote:   https://hg.mozilla.org/integration/fx-team/rev/4201305112ff

Comment 17

[Brian Grinstead [:bgrins]]

(In reply to Brian Grinstead [:bgrins] from comment #12)
> (In reply to Tim Taubert [:ttaubert] from comment #8)
> > We could as well postpone that to a follow-up to keep bugs smaller. It
> > doesn't change behavior or telemetry as currently implemented.
> 
> Going to break this part into another bug - I have the rest of the feedback
> addressed so will resubmit for review and then try to tackle this separately.

Filed Bug 1178985 for handling the pbmode changes.

Comment 18

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/4201305112ff

Comment 19

[Matt Wobensmith [:mwobensmith][:matt:]]

Old UI still exists (bug ?) but TP functionality has been correctly implemented in the new CC.

Verified fixed in Fx42.0a1, 2015-07-06.