Closed Bug 1181523 Opened 9 years ago Closed 9 years ago

[Control Center] Hide tracking protection section for chromeUI pages

Categories

(Firefox :: General, defect, P1)

defect
Points:
1

Tracking

()

VERIFIED FIXED
Firefox 42
Iteration:
42.2 - Jul 27
Tracking Status
firefox42 --- verified

People

(Reporter: ttaubert, Assigned: ttaubert)

References

Details

(Whiteboard: [fxprivacy] [campaign])

Attachments

(1 file)

We shouldn't show the TP section for internal pages like about:home. Bug 1178163 should do the same for "local" pages.
Flags: qe-verify+
Flags: firefox-backlog+
Comment on attachment 8630961 [details] [diff] [review]
0001-Bug-1181523-Control-Center-Hide-tracking-protection-.patch

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

::: browser/base/content/browser.js
@@ +6968,5 @@
>     */
>    setPopupMessages : function(newMode) {
>  
>      this._identityPopup.className = newMode;
> +    this._identityPopupMainView.className = newMode;

I was worried about this before when you initially wrote the rest of this, but it seemed then that there wouldn't really be much else that would need classes, and using classList wasn't an option because we actually *want* to remove the other "secureUI" or "insecureUI" classes...

Now we might run into panelmultiview classes though (I don't think we have any yet, but I could be wrong?).

It seems like really, for all of these, an attribute would be neater (e.g. .setAttribute("seclevel", "chrome") or something) and would have fewer risks in terms of interfering with other attempts to style things.

I realize that the effort in refactoring all the styles and so on would be boring and not essential and by now we're uplifting..., so maybe this should be a followup? Your choice.

::: browser/themes/shared/controlcenter/panel.inc.css
@@ +8,5 @@
>  #identity-popup-security-content:not(.unknownIdentity) > .identity-popup-connection-not-secure,
>  #identity-popup-securityView:not(.verifiedIdentity) > #identity-popup-securityView-connection,
>  #identity-popup-security-content:not(.chromeUI) > .identity-popup-connection-internal,
> +#identity-popup-security-content.chromeUI + .identity-popup-expander,
> +#identity-popup-mainView.chromeUI > #tracking-protection-container {

It seems we didn't use my suggestion in bug 1170759 comment 19 to split this massive selector up with some comments explaining what it was doing. I don't mind terribly either way, but was there a particular reason not to?
Attachment #8630961 - Flags: review?(gijskruitbosch+bugs) → review+
Blocks: 1174986
Rank: 2
Priority: -- → P1
QA Contact: mwobensmith
(In reply to :Gijs Kruitbosch from comment #2)
> I realize that the effort in refactoring all the styles and so on would be
> boring and not essential and by now we're uplifting..., so maybe this should
> be a followup? Your choice.

Yes, will file a follow-up. I do like the suggestion and agree it's quite a mess currently. But we'd want to uplift this indeed.

> ::: browser/themes/shared/controlcenter/panel.inc.css
> @@ +8,5 @@
> >  #identity-popup-security-content:not(.unknownIdentity) > .identity-popup-connection-not-secure,
> >  #identity-popup-securityView:not(.verifiedIdentity) > #identity-popup-securityView-connection,
> >  #identity-popup-security-content:not(.chromeUI) > .identity-popup-connection-internal,
> > +#identity-popup-security-content.chromeUI + .identity-popup-expander,
> > +#identity-popup-mainView.chromeUI > #tracking-protection-container {
> 
> It seems we didn't use my suggestion in bug 1170759 comment 19 to split this
> massive selector up with some comments explaining what it was doing. I don't
> mind terribly either way, but was there a particular reason not to?

Forgot about that :/ Sorry. Will add it.
(In reply to Tim Taubert [:ttaubert] from comment #3)
> (In reply to :Gijs Kruitbosch from comment #2)
> > I realize that the effort in refactoring all the styles and so on would be
> > boring and not essential and by now we're uplifting..., so maybe this should
> > be a followup? Your choice.
> 
> Yes, will file a follow-up. I do like the suggestion and agree it's quite a
> mess currently. But we'd want to uplift this indeed.

Oh, nuts. We'll not uplift this here because we don't care about TP in 41 yet. Will file a follow-up nonetheless.
Depends on: 1183609
Iteration: 42.1 - Jul 13 → 42.2 - Jul 27
https://hg.mozilla.org/mozilla-central/rev/163c37ecd93e
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 42
Whiteboard: [fxprivacy] → [fxprivacy] [campaign]
Verified on Nightly Fx42.0a1, 2015-07-27.
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: