Closed Bug 1400829 Opened 6 years ago Closed 6 years ago

Remove trackingProtectionPBM5 string from preferences/privacy.dtd

Categories

(Firefox :: Settings UI, enhancement, P3)

Product:
Firefox
Component:
Settings UI
Type:
enhancement

Tracking

()

Status:
RESOLVED FIXED
Milestone:
Firefox 58
Tracking Flags:
Tracking Status
firefox58 --- fixed

People

(Reporter: flod, Assigned: evanxd)

References

Details

(Whiteboard: [photon-preference])

Attachments

(1 file)

Bug 1400829 - Remove trackingProtectionPBM5 string since it is no longer being used anymore.
59 bytes, text/x-review-board-request
flod
: review+
Details 
While checking bug 1400815, I discovered that:
* Comments in bug 1376420 are wrong, PBM6 is displayed when privacy.trackingprotection.ui.enabled is set to false
* trackingProtectionPBM5 is not used anymore, after bug 1349689
On second thoughts, is it an error in the code that trackingProtectionPBM5 is missing in the pref reorg?
https://mozilla.invisionapp.com/share/P4ACQT1E3#/screens/237159280
Flags: needinfo?(rchien)
Yep, I think you're right. trackingProtectionPBM5 is no longer being used anymore. We should remove it. Thanks for finding out the issue.
Flags: needinfo?(rchien)
Whiteboard: [photon-preference][triage]
(In reply to Ricky Chien [:rickychien] from comment #2)
> Yep, I think you're right. trackingProtectionPBM5 is no longer being used
> anymore. We should remove it. Thanks for finding out the issue.

Does it mean that's expected to allow users to enable TP in normal mode in Beta and Release, like it happens in Nightly?
https://mozilla.invisionapp.com/share/P4ACQT1E3#/screens/239744864
Flags: qe-verify-
Priority: -- → P3
Whiteboard: [photon-preference][triage] → [photon-preference]
Assignee: nobody → evan
Status: NEW → ASSIGNED
Priority: P3 → P1
Comment on attachment 8912119 [details]
Bug 1400829 - Remove trackingProtectionPBM5 string since it is no longer being used anymore.

https://reviewboard.mozilla.org/r/183494/#review188648

::: browser/locales/en-US/chrome/browser/preferences/privacy.dtd:18
(Diff revision 2)
>  <!ENTITY  trackingProtectionExceptions.accesskey "x">
>  
> -<!-- LOCALIZATION NOTE (trackingProtectionPBM5.label): This string is displayed if privacy.trackingprotection.ui.enabled is set to true. This currently happens on the release and beta channel. -->
> -<!ENTITY trackingProtectionPBM5.label         "Use Tracking Protection in Private Windows">
> -<!ENTITY trackingProtectionPBM5.accesskey     "v">
>  <!-- LOCALIZATION NOTE (trackingProtectionPBM6.label): This string is displayed if privacy.trackingprotection.ui.enabled is set to false. This currently happens on the nightly channel. -->

If the string above is not used anymore, this comment is obsolete too.
Comment on attachment 8912119 [details]
Bug 1400829 - Remove trackingProtectionPBM5 string since it is no longer being used anymore.

https://reviewboard.mozilla.org/r/183494/#review188654

::: browser/locales/en-US/chrome/browser/preferences/privacy.dtd:18
(Diff revision 2)
>  <!ENTITY  trackingProtectionExceptions.accesskey "x">
>  
> -<!-- LOCALIZATION NOTE (trackingProtectionPBM5.label): This string is displayed if privacy.trackingprotection.ui.enabled is set to true. This currently happens on the release and beta channel. -->
> -<!ENTITY trackingProtectionPBM5.label         "Use Tracking Protection in Private Windows">
> -<!ENTITY trackingProtectionPBM5.accesskey     "v">
>  <!-- LOCALIZATION NOTE (trackingProtectionPBM6.label): This string is displayed if privacy.trackingprotection.ui.enabled is set to false. This currently happens on the nightly channel. -->

Hi Francesco,

I'm not sure why we need to remove this comment since the `trackingProtectionPBM6.label` string wil be displayed if privacy.trackingprotection.ui.enabled is set to false.
Hi 

What do you think of Comment 8? And could you help review the patch?

Thank you.
Flags: needinfo?(francesco.lodolo)
(In reply to Evan Tseng [:evanxd] from comment #8)
> I'm not sure why we need to remove this comment since the
> `trackingProtectionPBM6.label` string wil be displayed if
> privacy.trackingprotection.ui.enabled is set to false.

I've asked questions in comment 0 (I think that comment is wrong) and comment 3 and I didn't get any answer.
Flags: needinfo?(francesco.lodolo)
Priority: P1 → P3
(In reply to Francesco Lodolo [:flod] (workweek until Sep 28, slower than usual) from comment #0)
> While checking bug 1400815, I discovered that:
> * Comments in bug 1376420 are wrong, PBM6 is displayed when
> privacy.trackingprotection.ui.enabled is set to false

I think the comment is correct. I set privacy.trackingprotection.ui.enabled as fasle and the PBM6 is displayed. Did I misunderstand something?


(In reply to Francesco Lodolo [:flod] (workweek until Sep 28, slower than usual) from comment #3)
> (In reply to Ricky Chien [:rickychien] from comment #2)
> > Yep, I think you're right. trackingProtectionPBM5 is no longer being used
> > anymore. We should remove it. Thanks for finding out the issue.
> 
> Does it mean that's expected to allow users to enable TP in normal mode in
> Beta and Release, like it happens in Nightly?
> https://mozilla.invisionapp.com/share/P4ACQT1E3#/screens/239744864
No, it only happens in Nightly build.

And this[1] is the spec for the Release build. User can enable TP in Privacy Browsing mode.

[1]: https://mozilla.invisionapp.com/share/P4ACQT1E3#/screens/237159280
Hi Francesco,

What do you think of Comment 11?
Flags: needinfo?(francesco.lodolo)
(In reply to Evan Tseng [:evanxd] from comment #11)
> No, it only happens in Nightly build.
> 
> And this[1] is the spec for the Release build. User can enable TP in Privacy
> Browsing mode.
> 
> [1]: https://mozilla.invisionapp.com/share/P4ACQT1E3#/screens/237159280

You just pointed me to specs that show release, and trackingProtectionPBM6.label used.

The comment in the file says that:

> <!-- LOCALIZATION NOTE (trackingProtectionPBM6.label): This 
> string is displayed if privacy.trackingprotection.ui.enabled 
> is set to false. This currently happens on the nightly channel. -->

http://searchfox.org/mozilla-central/rev/3dbb47302e114219c53e99ebaf50c5cb727358ab/browser/locales/en-US/chrome/browser/preferences/privacy.dtd#21

And I'm saying it's not correct, privacy.trackingprotection.ui.enabled is set to false in Beta and Release, not Nightly.
Flags: needinfo?(francesco.lodolo)
(In reply to Francesco Lodolo [:flod] (workweek until Sep 28, slower than usual) from comment #13)
> And I'm saying it's not correct, privacy.trackingprotection.ui.enabled is
> set to false in Beta and Release, not Nightly.

As far as I can tell, that string is used independently from the pref at this point.
(In reply to Francesco Lodolo [:flod] (workweek until Sep 28, slower than usual) from comment #14)
> (In reply to Francesco Lodolo [:flod] (workweek until Sep 28, slower than
> usual) from comment #13)
> > And I'm saying it's not correct, privacy.trackingprotection.ui.enabled is
> > set to false in Beta and Release, not Nightly.
> 
> As far as I can tell, that string is used independently from the pref at
> this point.

Forget this part, I'm getting confused myself.

trackingProtectionPBM6.label ("Use Tracking Protection in Private Browsing to block known trackers") is used in Beta and Release, when privacy.trackingprotection.ui.enabled=false.

In Nightly, trackingProtection2.radioGroupLabel ("Use Tracking Protection to block known trackers") is used instead.

Still, that comment is not correct.
Hi Francesco,

Yes, you're right. The comment is not correct. And I've updated it.

> <!-- LOCALIZATION NOTE (trackingProtectionPBM6.label): This 
> string is displayed if privacy.trackingprotection.ui.enabled 
> is set to false. This currently happens on the nightly channel. -->

Could you help review it again?

Thank you.
Flags: needinfo?(francesco.lodolo)
Comment on attachment 8912119 [details]
Bug 1400829 - Remove trackingProtectionPBM5 string since it is no longer being used anymore.

https://reviewboard.mozilla.org/r/183494/#review189102
Attachment #8912119 - Flags: review?(francesco.lodolo) → review+
Flags: needinfo?(francesco.lodolo)
Pushed by rchien@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/6cd5181ec29d
Remove trackingProtectionPBM5 string since it is no longer being used anymore. r=flod
https://hg.mozilla.org/mozilla-central/rev/6cd5181ec29d
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
status-firefox58: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 58
You need to log in before you can comment on or make changes to this bug.