1433596 - "Data Collection and Use" displayed although MOZ_DATA_REPORTING is false

Status: RESOLVED FIXED

(Firefox :: Settings UI, enhancement)

Description

[pubkeypin]

As discussed in
https://bugzilla.mozilla.org/show_bug.cgi?id=1431942#c7
the header for "Data Collection and Use" is always displayed in the "Privacy & Security" page of "Preferences",

https://dxr.mozilla.org/mozilla-central/source/browser/components/preferences/in-content/privacy.xul#562

while actual content is only available, if MOZ_DATA_REPORTING is true:

https://dxr.mozilla.org/mozilla-central/source/browser/components/preferences/in-content/privacy.xul#570

When MOZ_DATA_REPORTING is false and therefore nothing displayed the user can interact with or see, this sub category (the heading or the <hbox> element) seems superfluous.

Comment 1

[pubkeypin]

Attachment: Move the <hbox> with id="dataCollectionCategory"

The patch proposes to align the display of the header with the availibility of content by putting both under the same conditional.

Currently there are no items in the UI for this sub category listed, if MOZ_DATA_REPORTING is false.


Please help me to find an accurate commit message.

I've tried

Bug 1433596 - Include the header for "Data Collection and Use" in about:preferences#privacy only, if there is content to display. r?jaws

but I'm unsure what to include and which wording is considered correct.

Especially whether the review part (and what ?/-/+/= chars in it) should be added by me from the beginning or later on is still a bit unclear to me.


If there is special UI review necessary, it might be useful to add, that to my limited knowledge 
MOZ_DATA_REPORTING on Mozilla release builds is always true, so the Mozilla user won't see any change in the UI.


Thank you very much Jared for all the help.

Comment 2

[Jared Wein [:jaws] (please needinfo? me)]

So, there are two different ways that you can upload a patch for review.

The "old" way to do it is to add the patch as an attachment to the bug (the way that this patch has been added). When using this way, you can upload with "r=jaws" at the end, otherwise you will have to re-upload the patch when review has been granted. The suggested way is to use "r?jaws", but using "r=jaws" is less work even though slightly incorrect.

The "new" way to do it is to push your commit to MozReview. See [1] and [2] for how to use MozReview. When using this way, you can upload with "r?jaws" at the end. Once review has been granted, I can land your patch immediately, and the MozReview system will change the "r?jaws" to "r=jaws" for us. It is also easier to review code using the MozReview system since it shows interdiffs better and allows for expanding the diff to include more context.

You'll never need to use "r-" or "r+" in your patch. Those are basically used for just giving a thumbs-down or thumbs-up on a patch review, but the patch commit message will always just have "r=" or "r?".

[1] http://mozilla-version-control-tools.readthedocs.io/en/latest/mozreview/install.html
[2] http://mozilla-version-control-tools.readthedocs.io/en/latest/mozreview/commits.html

Comment 3

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 8945980 [details] [diff] [review]
Move the <hbox> with id="dataCollectionCategory"

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

Looks good, we'll need to make sure that tests pass too. I've pushed this patch to our tryserver to run our automated tests on all platforms.

Comment 4

[Jared Wein [:jaws] (please needinfo? me)]

Pushed to tryserver, results should be available in a couple hours at most:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=7b75701072d635aade50be24ec4e37ef03f051fa

Comment 5

[pubkeypin]

(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #2)
> So, there are two different ways that you can upload a patch for review.
[...]

Thank you for these very detailed explanations!
Newcomers are't easy but the r* thing is now understood.

I've already seen parts of these documentation but will look more into the details.

Up to now I've prefered the "manual" way of attaching the patch, because that gives me more control of what is actually sent upstream and because I didn't want to use a "wizard" setting up my mercurial. I'll see if the documentation describes the details how to do it "by hand".

With
https://mozilla-version-control-tools.readthedocs.io/en/latest/mozreview/install-mercurial.html#manual-configuration
this seems to be the case, but first I've to examine this "hgext/reviewboard/client.py" before trusting and using it.


The patch will be re-uploaded with "r=jaws", given it passes the tests.

Thanks for all these steps that have to be climbed.

Comment 6

[Jared Wein [:jaws] (please needinfo? me)]

The patch passed all the tests so you can go ahead and upload a version with an updated commit message. Thanks!

Comment 7

[pubkeypin]

Attachment: Reviewed: Move the <hbox> with id="dataCollectionCategory"

The only difference between the previously reviewed and this patch is the change from "r?jaws" to "r=jaws" in the commit message.

I've seen the "checkin" flag but haven't set it, because that is perhaps reserved to someone with special permissions?

The needinfo is only set to remind you, that at least this seems to be still missing.

Thank you for all the instructive help.

Comment 8

[Jared Wein [:jaws] (please needinfo? me)]

You're welcome!

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/8c9d955dbe63
Include the header for "Data Collection and Use" in about:preferences#privacy only, if there is content to display. r=jaws

Comment 10

[Natalia Csoregi [:nataliaCs]]

https://hg.mozilla.org/mozilla-central/rev/8c9d955dbe63