1434405 - Clean up unnecessary if statements in Safe Browsing preferences

Status: RESOLVED FIXED

(Toolkit :: Safe Browsing, enhancement, P5)

Description

[François Marier [:francois]]

As per https://bugzilla.mozilla.org/show_bug.cgi?id=1410522#c3, the extra if statements added in bug 1394053 could be removed now that bug 1410522 has re-enabled download protection for all builds.

Comment 1

[Mai Truong [:maiquynhtruong]]

Hello,

Can I work on this?

Thank you.

Comment 2

[Mai Truong [:maiquynhtruong]]

Hello Francois,

I tried to find you on IRC but couldn't find your nickname there.

I was just trying to ask you how to get started on this. It'd be great if you can point me to the right IRC channel.

Thanks a lot.

Comment 3

[Emma Humphries ☕️🎸🧞‍♀️✨ (she/they) [:emceeaich] (Pacific Time) use needinfo]

François, will you be mentoring this bug?

If you are, please check with Mai on what they need to get started. 

Thank you.

Comment 4

[François Marier [:francois]]

Hi Mai,

If you'd like to work on this, please assign this bug to yourself and then look at this patch:

https://hg.mozilla.org/mozilla-central/rev/05907bcce126

You will find that it added a number of if statements like this one:

    if (blockDownloads) {
       ...
    } else {
       ...
    }

Now we can assume that "blockDownloads" is always valid, so we can remove the if and the else clause.

Comment 5

[vaishnavireddy0405]

hi francois,
Can I work on this?

Comment 6

[Mai Truong [:maiquynhtruong]]

Hi Vaishnavi,

It's been only 6 days but sure I can leave the bug to you. This bug is a great way to get introduced to the codebase. Please see francois' comment to get started.

Thanks.

Comment 7

[Tri]

Hey Mai,

Is this still available to be fixed? I'd like to work on it.

Thanks,
Tri

Comment 8

[Tri]

Attachment: Bug 1434405 - Removed the if and else clauses for blockDownloads

Review commit: https://reviewboard.mozilla.org/r/226374/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/226374/

Comment 9

[Tri]

Attachment: Bug 1434405 - Removed the if and else clauses for blockDownloads

Review commit: https://reviewboard.mozilla.org/r/226536/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/226536/

Comment 10

[François Marier [:francois]]

Comment on attachment 8957641 [details]
Bug 1434405 - Removed the if and else clauses for blockDownloads

https://reviewboard.mozilla.org/r/226536/#review233318

Looks good. Only two small things to fix and this is ready to go.

::: browser/components/preferences/in-content/privacy.js:1252
(Diff revision 1)
>          blockUncommonUnwanted.setAttribute("disabled", "true");
>        }
>      });
>  
> -    if (blockDownloads) {
>        blockDownloads.addEventListener("command", function() {

nit: this block needs to be reindented as well

::: browser/components/preferences/in-content/privacy.js:1307
(Diff revision 1)
> -    if (blockDownloads) {
> -      blockDownloads.checked = blockDownloadsPref.value;
> +    blockDownloads.checked = blockDownloadsPref.value;
> -      if (!blockDownloadsPref.value) {
> +    if (!blockDownloadsPref.value) {
> -        blockUncommonUnwanted.setAttribute("disabled", "true");
> +      blockUncommonUnwanted.setAttribute("disabled", "true");
> -      }
> +    }
> -    }
> +    

nit: that line can be removed and the trailing whitespace will disappear.

Comment 11

[Tri]

Attachment: Bug 1434405 - Reindented lines 1252, remove line 1307

Review commit: https://reviewboard.mozilla.org/r/227648/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/227648/

Comment 12

[Tri]

Attachment: Bug 1434405 - reindented the correct lines

Review commit: https://reviewboard.mozilla.org/r/227734/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/227734/

Comment 13

[François Marier [:francois]]

Did you amend your original commit or add another commit on top of the original one?

The way to make it work with MozReview is to amend the original commit and push to MozReview again. That way it will show up as a second revision and can be merged as a single commit.

Comment 14

[Tri]

I added another commit on top of the original one. That is my mistake, sorry about that. I will do that in the future.
I'm still kind of new to mercurial, but can you let me know if I understand this correctly. 

Should I strip the last commit and revert the changes? i.e., hg strip --keep --rev -1. And then commit again with the amend flag to add to the latest commit. i.e., hg commit --ammend "msg" and then "hg push review". Is that correct?

Also, from what I'm seeing is the latest commit still has the changes of the original commit.

Comment 15

[François Marier [:francois]]

An easier way to merge all of the commits together would be to use "hg histedit" and then "fold" the later commits into the first one.

Note that you'll need to have the "histedit" extension enabled in your ~/.hgrc file:

    [extensions]
    histedit = 

Once you've done that, you can push to MozReview and that should create a new revision of your patch automatically.

Comment 16

[Divyansh Sharma [:spiro]]

Attachment: Bug 1434405 : Clean up unnecessary if statements

Bug 1434405 : Removed unnecessary if-else clauses related to |blockDownloads|

Comment 17

[François Marier [:francois]]

I'm no longer a module owner for Safe Browsing so you'll want to ask :dimi for a review instead.

Comment 18

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/b25d115216a2
Clean up unnecessary if statements r=dimi

Comment 19

[Dorel Luca [:dluca]]

https://hg.mozilla.org/mozilla-central/rev/b25d115216a2