Move existing Tracking Protection functionality from shield doorhanger to Control Center

VERIFIED FIXED in Firefox 42

Status

()

defect
P1
normal
Rank:
2
VERIFIED FIXED
4 years ago
3 years ago

People

(Reporter: bgrins, Assigned: bgrins)

Tracking

40 Branch
Firefox 42
Points:
13
Dependency tree / graph
Bug Flags:
firefox-backlog +
qe-verify +

Firefox Tracking Flags

(firefox42 verified)

Details

(Whiteboard: [fxprivacy] [campaign])

Attachments

(1 attachment, 3 obsolete attachments)

We need to move the existing tracking protection content out of the doorhanger that's currently shared with the Mixed Content Blocking feature.
Flags: firefox-backlog?
Flags: firefox-backlog? → firefox-backlog+
Priority: -- → P1
Rank: 3
I think that this depends on the subpanel changes happening in Bug 1170759
Depends on: 1170759
Looks like this is going to require migrating the current XBL implementation from urlbarBindings.xml into controlcenter/content/panel.inc.xul and browser.js.
Going to claim this one since I've started to look into it a bit
Assignee: nobody → bgrinstead
Status: NEW → ASSIGNED
Rank: 3 → 2
Posted patch tp-control-center.patch (obsolete) — Splinter Review
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.
Attachment #8624939 - Flags: feedback?(ttaubert)
Comment on attachment 8624939 [details] [diff] [review]
tp-control-center.patch

Canceling and waiting for the rebased version.
Attachment #8624939 - Flags: feedback?(ttaubert)
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 ?
Points: --- → 13
Flags: qe-verify+
Posted patch tp-control-center.patch (obsolete) — Splinter Review
Same as Comment 4, but now rebased on top of the subpanel work
Attachment #8624939 - Attachment is obsolete: true
Attachment #8626643 - Flags: review?(ttaubert)
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.)
Attachment #8626643 - Flags: review?(ttaubert) → feedback+
Iteration: --- → 41.3 - Jun 29
(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).
Can also do this as a follow-up if you'd like that more?
(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.
(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.
Posted patch tp-control-center.patch (obsolete) — Splinter Review
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.
Attachment #8626643 - Attachment is obsolete: true
Attachment #8627298 - Flags: review?(ttaubert)
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 :)
Attachment #8627298 - Flags: review?(ttaubert) → review+
Iteration: 41.3 - Jun 29 → 42.1 - Jul 13
remote:   https://hg.mozilla.org/integration/fx-team/rev/4201305112ff
Whiteboard: [fxprivacy] → [fixed-in-fx-team][fxprivacy]
(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.
https://hg.mozilla.org/mozilla-central/rev/4201305112ff
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team][fxprivacy] → [fxprivacy]
Target Milestone: --- → Firefox 41
Target Milestone: Firefox 41 → Firefox 42
QA Contact: mwobensmith
Old UI still exists (bug ?) but TP functionality has been correctly implemented in the new CC.

Verified fixed in Fx42.0a1, 2015-07-06.
Status: RESOLVED → VERIFIED
Whiteboard: [fxprivacy] → [fxprivacy] [campaign]
You need to log in before you can comment on or make changes to this bug.