Last Comment Bug 1322565 - Remove gecko.handlerService.allowRegisterFromDifferentHost
: Remove gecko.handlerService.allowRegisterFromDifferentHost
Status: RESOLVED FIXED
[good first bug]
: good-first-bug
Product: Firefox
Classification: Client Software
Component: File Handling (show other bugs)
: unspecified
: Unspecified Unspecified
P5 normal (vote)
: Firefox 53
Assigned To: Iulian Radu
:
: :Paolo Amadini
Mentors: :Gijs
:Paolo Amadini
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2016-12-08 16:07 PST by :Gijs
Modified: 2016-12-17 14:06 PST (History)
3 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
fixed


Attachments
Remove gecko.handlerService.allowRegisterFromDifferentHost (2.32 KB, patch)
2016-12-15 16:00 PST, Iulian Radu
gijskruitbosch+bugs: review+
Details | Diff | Splinter Review
Bug1322565.patch (2.33 KB, patch)
2016-12-16 11:02 PST, Iulian Radu
gijskruitbosch+bugs: review+
Details | Diff | Splinter Review

Description User image :Gijs 2016-12-08 16:07:20 PST
I don't believe this preference should ever be set to true, so we should remove it.
Comment 1 User image Iulian Radu 2016-12-15 16:00:36 PST
Created attachment 8819100 [details] [diff] [review]
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 User image :Gijs 2016-12-16 02:10:32 PST
Comment on attachment 8819100 [details] [diff] [review]
Remove gecko.handlerService.allowRegisterFromDifferentHost

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

Perfect, thanks!
Comment 3 User image Pulsebot 2016-12-16 07:30:03 PST
Pushed by cbook@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/a6e2d96c1274
Remove gecko.handlerService.allowRegisterFromDifferentHost. r=gijs
Comment 4 User image Carsten Book [:Tomcat] 2016-12-16 07:45:42 PST
backed out for eslint failure like https://treeherder.mozilla.org/logviewer.html#?job_id=40693746&repo=mozilla-inbound
Comment 5 User image Pulsebot 2016-12-16 07:51:39 PST
Backout by cbook@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/e04a75c36ec3
Backed out changeset a6e2d96c1274 for eslint failure
Comment 6 User image :Gijs 2016-12-16 08:36:55 PST
(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 User image Iulian Radu 2016-12-16 11:02:05 PST
Created attachment 8819368 [details] [diff] [review]
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 User image :Gijs 2016-12-17 05:15:04 PST
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 User image Pulsebot 2016-12-17 05:16:27 PST
Pushed by gijskruitbosch@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/ebcba38b2a82
Remove gecko.handlerService.allowRegisterFromDifferentHost pref, r=gijs
Comment 10 User image :Gijs 2016-12-17 05:16:41 PST
(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 User image Phil Ringnalda (:philor) 2016-12-17 14:06:06 PST
https://hg.mozilla.org/mozilla-central/rev/ebcba38b2a82

Note You need to log in before you can comment on or make changes to this bug.