883522 - message attribute of PositionError object is an empty string (geolocation)

Status: RESOLVED FIXED

(Core :: DOM: Geolocation, defect)

Description

[Daniel]

Attachment: testcase.html

User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:24.0) Gecko/20130614 Firefox/24.0 (Nightly/Aurora)
Build ID: 20130614031100

Steps to reproduce:

* Open testcase
* Choose "Never share location" from the doorhanger dropdown menu
* Look in the console and inspect the PositionError object



Actual results:

The message attribute is an empty string.


Expected results:

From http://www.w3.org/TR/geolocation-API/#message

"The message attribute must return an error message describing the details of the error encountered."

Chrome says "User denied Geolocation" and IE also has a message.

The bug applies to the other errors as well (2: position unavailable, 3: timeout).

Comment 1

[Maria Grazia Alastra [:mga]]

Confirmed on Firefox 22 beta 5 and on latest Nightly 24.0

- User agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:22.0) Gecko/20100101 Firefox/22.0, Build ID: 20130612084701

- User agent: Mozilla/5.0 (Windows NT 6.1; Win64; x64; rv:24.0) Gecko/20130618 Firefox/24.0, Build ID: 20130618031335

Comment 2

[Josh Matthews [:jdm]]

We'll need to add a message attribute to http://mxr.mozilla.org/mozilla-central/source/dom/interfaces/geolocation/nsIDOMGeoPositionError.idl, and a GetMessage method around http://hg.mozilla.org/mozilla-central/file/2bcc1f75895a/dom/src/geolocation/nsGeolocation.cpp#l255.

Comment 3

[Josh Matthews [:jdm]]

In addition, to allow localization of the error messages, we'll need to do something like nsDownloadManager::GetDefaultDownloadsDirectory - get a string bundle (see nsDownloadManager::Init), use GetStringFromName, and place the English error strings in a properties file.

Comment 4

[Daniel]

(In reply to Josh Matthews [:jdm] from comment #3)
> In addition, to allow localization of the error messages, we'll need to do
> something like nsDownloadManager::GetDefaultDownloadsDirectory - get a
> string bundle (see nsDownloadManager::Init), use GetStringFromName, and
> place the English error strings in a properties file.

The spec says "This attribute is primarily intended for debugging". Do we localize other debug messages? 

IE localizes these messages but Chrome does not.

Comment 5

[Josh Matthews [:jdm]]

Jonas, is this attribute something we should localize?

Comment 6

[Maria Grazia Alastra [:mga]]

Attachment: Add message attribute and GetMessage method.

Comment 7

[Doug Turner (:dougt)]

It isn't something that needs to be localized.

From the spec:
The message attribute must return an error message describing the details of the error encountered. This attribute is primarily intended for debugging and developers should not use it directly in their application user interface.

Comment 8

[Josh Matthews [:jdm]]

Comment on attachment 766733 [details] [diff] [review]
Add message attribute and GetMessage method.

No need for Jonas' feedback, in that case.

Comment 9

[Doug Turner (:dougt)]

Comment on attachment 766733 [details] [diff] [review]
Add message attribute and GetMessage method.

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

Nice work!

We moved to webidl.  I am not sure what changes there are required.  Adding ggp who did that.

http://mxr.mozilla.org/mozilla-central/source/dom/webidl/Geolocation.webidl?force=1

::: dom/src/geolocation/nsGeolocation.cpp
@@ +262,5 @@
> +NS_IMETHODIMP
> +PositionError::GetMessage(nsAString& aMessage)
> +{
> +  switch (mCode)
> +    {

nit: make brace directly under switch

@@ +263,5 @@
> +PositionError::GetMessage(nsAString& aMessage)
> +{
> +  switch (mCode)
> +    {
> +      case nsIDOMGeoPositionError::PERMISSION_DENIED: 

nit: white space at the end of the line.

Comment 10

[Guilherme Gonçalves [:ggp]]

Comment on attachment 766733 [details] [diff] [review]
Add message attribute and GetMessage method.

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

Nice work indeed. 

Do we want error events to always have messages from now on? If so, it could be a good idea to check that in our tests (something like https://mxr.mozilla.org/mozilla-central/source/dom/tests/mochitest/geolocation/geolocation_common.js#74).

::: dom/interfaces/geolocation/nsIDOMGeoPositionError.idl
@@ +12,5 @@
>    const unsigned short POSITION_UNAVAILABLE = 2;
>    const unsigned short TIMEOUT = 3;
>  
>    readonly attribute short code;
> +  readonly attribute AString message;

Should also change the uuid for this interface, since you added an attribute.

::: dom/src/geolocation/nsGeolocation.cpp
@@ +273,5 @@
> +      case nsIDOMGeoPositionError::TIMEOUT:
> +        aMessage = NS_LITERAL_STRING("Position acquisition timed out");
> +        break;
> +      default:
> +        break;

This shouldn't ever happen, right? There should probably be a MOZ_CRASH() instead of break; there.

Comment 11

[Josh Matthews [:jdm]]

I'm inclined to say that the break is fine; MOZ_CRASH seems like an overreaction to me.

Comment 12

[Guilherme Gonçalves [:ggp]]

(In reply to Josh Matthews [:jdm] from comment #11)
> I'm inclined to say that the break is fine; MOZ_CRASH seems like an
> overreaction to me.

That's a place where I would otherwise have used MOZ_NOT_REACHED, which apparently has been replaced with MOZ_CRASH everywhere (bug 820686), so that's why I suggested that.

I would like to see some type of assertion there, but if just the break sounds good to you (perhaps coupled with the old aMessage.Truncate()), I'm fine with that too.

Comment 13

[Josh Matthews [:jdm]]

Truncate's not necessary; DOMStrings are empty by default.

Comment 14

[Maria Grazia Alastra [:mga]]

Attachment: Add message attribute, change uuid and add GetMessage method

Comment 15

[Josh Matthews [:jdm]]

Comment on attachment 770901 [details] [diff] [review]
Add message attribute, change uuid and add GetMessage method

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

Doug said I can review this. Let's get a new version with the changed message, and this will be ready to check in :)

::: dom/src/geolocation/nsGeolocation.cpp
@@ +264,5 @@
> +{
> +  switch (mCode)
> +  {
> +    case nsIDOMGeoPositionError::PERMISSION_DENIED:
> +      aMessage = NS_LITERAL_STRING("User denied Geolocation");

Let's make this "User denied geolocation prompt"

Comment 16

[Maria Grazia Alastra [:mga]]

Attachment: Add message attribute, change uuid, add GetMessage method

Ok, message changed! The patch applies smoothly with the latest version of mozilla-central.

Comment 17

[Josh Matthews [:jdm]]

Comment on attachment 773866 [details] [diff] [review]
Add message attribute, change uuid, add GetMessage method

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

Eep, sorry for forgetting about this patch. Let's commit it!

Comment 18

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/c08b03cc4f32

Comment 19

[Ryan VanderMeulen [:RyanVM]]

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

Comment 20

[Jacek Caban]

I pushed mingw fixup for unicode macros conflicts. I'm not sure why it didn't cause problems on MSVC.

https://hg.mozilla.org/integration/mozilla-inbound/rev/5f6beab20174

Comment 21

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/5f6beab20174