Closed Bug 1162863 Opened 9 years ago Closed 9 years ago

Display the requesting site more prominently in the geolocation notification

Categories

(Firefox :: General, defect)

defect
Not set
normal
Points:
3

Tracking

()

VERIFIED FIXED
Firefox 41
Iteration:
41.1 - May 25
Tracking Status
firefox40 --- verified
firefox41 --- verified

People

(Reporter: dao, Assigned: dao)

References

Details

Attachments

(3 files, 1 obsolete file)

Bug 1139656 added an originHost option to PopupNotifications. The geolocation notification should use that rather than putting the host into the notification message.
Flags: qe-verify+
Flags: firefox-backlog+
Attachment #8603210 - Flags: review?(gavin.sharp)
Status: NEW → ASSIGNED
Attachment #8603210 - Flags: review?(gavin.sharp) → review?(paolo.mozmail)
Attachment #8603211 - Flags: review?(gavin.sharp) → review?(paolo.mozmail)
Attachment #8603212 - Flags: review?(gavin.sharp) → review?(paolo.mozmail)
Comment on attachment 8603210 [details] [diff] [review]
part 1: rename originHost to origin

Gavin, while the code changes in all three patches are fairly trivial, I picked you as the reviewer because I also wanted some kind of higher-level API review.

E.g. it might make sense for 'origin' to be an nsIURI rather than a string. PopupNotifications.jsm would then have to decide whether to display the host or the path for file URIs or whatever. Do you think this would generally meet callers' needs? For instance, I can imagine some callers wanting to display eTLD+1 rather than the full host, since I think we map some site permissions this way (or was it only cookies?). Then again, when it comes to that maybe we could add another boolean option for callers to specify their special needs...
Attachment #8603210 - Flags: superreview?(gavin.sharp)
(In reply to Dão Gottwald [:dao] from comment #4)
> Then again, when it comes to
> that maybe we could add another boolean option for callers to specify their
> special needs...

Or we could support originURI now, and possibly add an 'origin' string option later if there's a need for that.

Or we could accept 'origin' as either an nsIURI or a string.
Comment on attachment 8603210 [details] [diff] [review]
part 1: rename originHost to origin

Using a string now and leaving it up to callers seems fine. As we get more consumers we can add restrictions / enforce consistency as needed.
Attachment #8603210 - Flags: superreview?(gavin.sharp) → superreview+
I think in the long run it will make sense for callers to just specify the URI which caused the request to be displayed, like suggested in comment 4, and the notification would decide which portion of it is relevant, based also on options providing additional context when needed. For the API we could use a full URI spec string including scheme (we're moving away from nsIURI).

For the moment we can keep the option as a free-from string instead. Since you're renaming it anyways, I suggest calling it "displayOrigin". We use "origin" to denote a properly formatted origin in the format returned by "location.origin", including the scheme, host and port.
Comment on attachment 8603210 [details] [diff] [review]
part 1: rename originHost to origin

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

r+ suggesting renaming to "displayOrigin" instead.
Attachment #8603210 - Flags: review?(paolo.mozmail) → review+
Attachment #8603211 - Flags: review?(paolo.mozmail) → review+
Attachment #8603212 - Flags: review?(paolo.mozmail) → review+
(In reply to :Paolo Amadini from comment #8)
> r+ suggesting renaming to "displayOrigin" instead.

...or any other name that conveys the meaning. I have no special preference for this one.
I can't convince myself that displayOrigin is a great name. Feels somewhat clumsy. At the same time, I'm unable to come up with a better name that conveys the meaning you're looking for. Are you ok with just sticking with "origin"?
Flags: needinfo?(paolo.mozmail)
(In reply to Dão Gottwald [:dao] from comment #10)
> Are you ok with just sticking with "origin"?

Then why not "originHost"? You save a rename.
Flags: needinfo?(paolo.mozmail)
Because some URIs don't have a host, e.g. we use the file path for file URIs in attachment 8603212 [details] [diff] [review].
File paths are definitely not a well-formed "origin" either, so both names are technically incorrect.

But if you really feel like renaming the field to the name you originally thought of, feel free to go ahead as I don't think I would feel worth it blocking this bug further on deciding a field name.
Ok, so, neither origin nor displayOrigin seem great, so I'll just use the latter as you suggested.
Iteration: 40.3 - 11 May → 41.1 - May 25
Blocks: 1165438
Approval Request Comment

I'd like to get this part uplifted to make it easier to uplift other patches to browser-addons.js (such as the one in bug 1147808) without conflicts

[Feature/regressing bug #]: bug 1139656
[User impact if declined]: n/a
[Describe test coverage new/current, TreeHerder]: browser-addons.js is covered well by tests
[Risks and why]: just a rename, low risk
[String/UUID change made/needed]: none
Attachment #8603210 - Attachment is obsolete: true
Attachment #8606610 - Flags: approval-mozilla-aurora?
Comment on attachment 8606610 [details] [diff] [review]
part 1: rename originHost to displayOrigin

Approved for uplift to aurora. Has test coverage, should make other uplifts easier.
Attachment #8606610 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
https://hg.mozilla.org/releases/mozilla-aurora/rev/5c45c9c49a20

Setting the status to fixed for tracking purposes, but note that Fx40 did *not* receive the full set of patches for this bug.
(In reply to Ryan VanderMeulen [:RyanVM UTC-4] from comment #19)
> https://hg.mozilla.org/releases/mozilla-aurora/rev/5c45c9c49a20
> 
> Setting the status to fixed for tracking purposes, but note that Fx40 did
> *not* receive the full set of patches for this bug.

Is manual testing needed here? If yes what should we test in Firefox 40, and what in Firefox 41?
Flags: needinfo?(dao)
(In reply to Florin Mezei, QA (:FlorinMezei) from comment #20)
> (In reply to Ryan VanderMeulen [:RyanVM UTC-4] from comment #19)
> > https://hg.mozilla.org/releases/mozilla-aurora/rev/5c45c9c49a20
> > 
> > Setting the status to fixed for tracking purposes, but note that Fx40 did
> > *not* receive the full set of patches for this bug.
> 
> Is manual testing needed here? If yes what should we test in Firefox 40, and
> what in Firefox 41?

Firefox 40 should display the host as part of the message like Firefox 39 did. Firefox 41 should display it more prominently as a heading.
Flags: needinfo?(dao)
Tested this fix across platforms [1] on 40.0b7 (Build ID: 20150723165742) and latest 41.0a2 (from 2015-07-23) builds, by using 3 test pages [2]. With all the 3 test pages on 40.0b7 the host is displayed as part of the message [3] and with 41.0a2, above the message and bolded [4] - which is the correct behavior as per comment 21. 

Although, with http://davidwalsh.name/demo/geolocation.php, I get "Your position:  <unavailable>" thrown by geolocation.php:26 in the Browse console on latest Developer Edition and Nightly with e10s enabled. Any ideas why (especially since the other 2 test pages work just fine)? Thanks in advance!


[1] Windows 7 64-bit, Windows 10 32-bit, Mac OS X 10.10.4 and Ubuntu 14.04 32-bit
[2] http://html5demos.com/geo; https://bug897696.bmoattachments.org/attachment.cgi?id=781078; http://davidwalsh.name/demo/geolocation.php
[3] http://i.imgur.com/fsf6jUA.png
[4] http://i.imgur.com/7aWhVjC.png
Flags: needinfo?(dao)
(In reply to Alexandra Lucinet, QA Mentor [:adalucinet] from comment #22)
> Although, with http://davidwalsh.name/demo/geolocation.php, I get "Your
> position:  <unavailable>" thrown by geolocation.php:26 in the Browse console
> on latest Developer Edition and Nightly with e10s enabled. Any ideas why
> (especially since the other 2 test pages work just fine)? Thanks in advance!

No idea, sorry. Seems unrelated to this bug though.
Flags: needinfo?(dao)
(In reply to Dão Gottwald [:dao] from comment #23)
> (In reply to Alexandra Lucinet, QA Mentor [:adalucinet] from comment #22)
> > Although, with http://davidwalsh.name/demo/geolocation.php, I get "Your
> > position:  <unavailable>" thrown by geolocation.php:26 in the Browse console
> > on latest Developer Edition and Nightly with e10s enabled. Any ideas why
> > (especially since the other 2 test pages work just fine)? Thanks in advance!
> 
> No idea, sorry. Seems unrelated to this bug though.

Logged bug 1192797 for the mentioned issue; Marking accordingly based on comment 22.
Status: RESOLVED → VERIFIED
Flags: qe-verify+
You need to log in before you can comment on or make changes to this bug.