1322565 - Remove gecko.handlerService.allowRegisterFromDifferentHost

Status: RESOLVED FIXED

(Firefox :: File Handling, defect, P5)

Description

[:Gijs (he/him)]

I don't believe this preference should ever be set to true, so we should remove it.

Comment 1

[Iulian Radu]

Attachment: Remove gecko.handlerService.allowRegisterFromDifferentHost

Hello,

I hope this is what you had in mind. Please let me know how I can improve it as it is one of my first few patches.

Comment 2

[:Gijs (he/him)]

Comment on attachment 8819100 [details] [diff] [review]
Remove gecko.handlerService.allowRegisterFromDifferentHost

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

Perfect, thanks!

Comment 3

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/a6e2d96c1274
Remove gecko.handlerService.allowRegisterFromDifferentHost. r=gijs

Comment 4

[Carsten Book [:Tomcat]]

backed out for eslint failure like https://treeherder.mozilla.org/logviewer.html#?job_id=40693746&repo=mozilla-inbound

Comment 5

[Pulsebot]

Backout by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/e04a75c36ec3
Backed out changeset a6e2d96c1274 for eslint failure

Comment 6

[:Gijs (he/him)]

(In reply to :Gijs Kruitbosch from comment #2)
> Comment on attachment 8819100 [details] [diff] [review]
> Remove gecko.handlerService.allowRegisterFromDifferentHost
> 
> Review of attachment 8819100 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Perfect, thanks!

So, eslint decided this wasn't quite perfect yet:

 [task 2016-12-16T15:35:14.084100Z] TEST-UNEXPECTED-ERROR | /home/worker/checkouts/gecko/browser/components/feeds/WebContentConverter.js:159:9 | 'pb' is assigned a value but never used. (no-unused-vars) 

>diff --git a/browser/components/feeds/WebContentConverter.js b/browser/components/feeds/WebContentConverter.js
>     // We also reject handlers registered from a different host (see bug 402287)
>     // The pref allows us to test the feature
>     let pb = Services.prefs;
>-    if (!pb.getBoolPref(PREF_ALLOW_DIFFERENT_HOST) &&
>-        (!["http:", "https:"].includes(aContentWindow.location.protocol) ||
>-         aContentWindow.location.hostname != uri.host)) {
>+    if (!["http:", "https:"].includes(aContentWindow.location.protocol) ||
>+         aContentWindow.location.hostname != uri.host) {


Can you also remove the assignment to 'pb', and for good measure, the comment line
>     // The pref allows us to test the feature

because we no longer test with this pref? Thanks!

Comment 7

[Iulian Radu]

Attachment: Bug1322565.patch

Oh, indeed. I now know about "mach lint" and I'll make sure to run it on future patches. Please let me know if I have set the right flags (review ?, needinfo).

Comment 8

[:Gijs (he/him)]

Comment on attachment 8819368 [details] [diff] [review]
Bug1322565.patch

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

::: browser/components/feeds/WebContentConverter.js
@@ +155,5 @@
>      }
>  
>      // We also reject handlers registered from a different host (see bug 402287)
> +    if (!["http:", "https:"].includes(aContentWindow.location.protocol) ||
> +         aContentWindow.location.hostname != uri.host) {

Nit: the second line should be aligned with the "!" so one space to the left.

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/ebcba38b2a82
Remove gecko.handlerService.allowRegisterFromDifferentHost pref, r=gijs

Comment 10

[:Gijs (he/him)]

(In reply to Iulian Radu from comment #7)
> Please let me know if I have set the right flags (review ?,
> needinfo).

review was good, needinfo wasn't necessary, but it didn't hurt. :-)

Comment 11

[Phil Ringnalda (:philor)]

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