Remove gecko.handlerService.allowRegisterFromDifferentHost

RESOLVED FIXED in Firefox 53

Status

()

Firefox
File Handling
P5
normal
RESOLVED FIXED
8 months ago
7 months ago

People

(Reporter: Gijs, Assigned: Iulian Radu, Mentored)

Tracking

({good-first-bug})

unspecified
Firefox 53
good-first-bug
Points:
---

Firefox Tracking Flags

(firefox53 fixed)

Details

(Whiteboard: [good first bug])

Attachments

(1 attachment, 1 obsolete attachment)

(Reporter)

Description

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

Updated

7 months ago
Priority: -- → P5
Whiteboard: [good first bug]
(Reporter)

Updated

7 months ago
Mentor: paolo.mozmail@amadzone.org, gijskruitbosch+bugs@gmail.com
Keywords: good-first-bug
(Assignee)

Comment 1

7 months ago
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.
Attachment #8819100 - Flags: review?(gijskruitbosch+bugs)
(Reporter)

Comment 2

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

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

Perfect, thanks!
Attachment #8819100 - Flags: review?(gijskruitbosch+bugs) → review+
(Reporter)

Updated

7 months ago
Keywords: checkin-needed

Comment 3

7 months ago
Pushed by cbook@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/a6e2d96c1274
Remove gecko.handlerService.allowRegisterFromDifferentHost. r=gijs
Keywords: checkin-needed
backed out for eslint failure like https://treeherder.mozilla.org/logviewer.html#?job_id=40693746&repo=mozilla-inbound
Flags: needinfo?(gijskruitbosch+bugs)

Comment 5

7 months ago
Backout by cbook@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/e04a75c36ec3
Backed out changeset a6e2d96c1274 for eslint failure
(Reporter)

Comment 6

7 months ago
(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!
Flags: needinfo?(gijskruitbosch+bugs) → needinfo?(iulian.radu67)
(Reporter)

Updated

7 months ago
Assignee: nobody → iulian.radu67
Status: NEW → ASSIGNED
(Assignee)

Comment 7

7 months ago
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).
Attachment #8819100 - Attachment is obsolete: true
Flags: needinfo?(iulian.radu67) → needinfo?(gijskruitbosch+bugs)
Attachment #8819368 - Flags: review?(gijskruitbosch+bugs)
(Reporter)

Comment 8

7 months ago
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.
Attachment #8819368 - Flags: review?(gijskruitbosch+bugs) → review+

Comment 9

7 months ago
Pushed by gijskruitbosch@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/ebcba38b2a82
Remove gecko.handlerService.allowRegisterFromDifferentHost pref, r=gijs
(Reporter)

Comment 10

7 months ago
(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. :-)
Flags: needinfo?(gijskruitbosch+bugs)
https://hg.mozilla.org/mozilla-central/rev/ebcba38b2a82
Status: ASSIGNED → RESOLVED
Last Resolved: 7 months ago
status-firefox53: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 53
You need to log in before you can comment on or make changes to this bug.