484074 - Update Geolocation UI to match latest madhava mockups

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Doug Turner (:dougt)]

Attachment: patch v.1

+++ This bug was initially created as a clone of Bug #483845 +++

http://mxr.mozilla.org/mozilla-central/source/browser/locales/en-US/chrome/browser/browser.properties#148

The current strings reflect a time when there was 3 states (exact, fuzzed, no).  During recent discussions, we want to change this text such that it reflects the binary decision the user has (yes, or no).

Comment 1

[Doug Turner (:dougt)]

Comment on attachment 368128 [details] [diff] [review]
patch v.1

gavin, in the firefox version of this, mconnor asked me to use a lower case "E" for the tellThemKey.  Do we want to use different keys here for fennec?

Comment 2

[Doug Turner (:dougt)]

Comment on attachment 368128 [details] [diff] [review]
patch v.1

fennec hasn't has a string freeze, so there is no rush on this until the UI is present that uses the extra strings:

+geolocation.learnMore=Learn more.
+geolocation.thisTime=This time
+geolocation.always=Always

Comment 3

[Stuart Parmenter]

doug: does someone need to review this?

Comment 4

[Doug Turner (:dougt)]

after the recent ff geolocation ui changes, this patch is obsolete.

Comment 5

[Doug Turner (:dougt)]

Attachment: wip

Comment 6

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 374531 [details] [diff] [review]
wip

Please use a special notification XBL binding in notifications.xml. Do not use the JS DOM approach.

<notification>s in Fennec are not structured the same as in Firefox.

Comment 7

[Doug Turner (:dougt)]

Attachment: wip2

Comment 8

[Doug Turner (:dougt)]

1) remember doesn't work.  getAttribute is not returning the right thing (not sure what is actually going on yet)
2) can't set the label of the checkbox programmatically.

Comment 9

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 375823 [details] [diff] [review]
wip2

drive by comments

>diff --git a/chrome/content/notification.xml b/chrome/content/notification.xml
> 
>+  <binding id="geo-notification" extends="chrome://global/content/bindings/notification.xml#notification">

Might be better to extend the notification in this file instead of the toolkit one.

>+        <xul:hbox>
>+          <xul:checkbox anonid="rememberChoice"
>+                        xbl:inherits="value=remember,xbl:labelon=rememberChoiceOn,xbl:labeloff=rememberChoiceOff"/>

I'm not the biggest fan of camel-cased attribute names, but I guess I can live with it

>+          <xul:spacer flex="1"/>
>+          <xul:label anonid="learnMore"
>+                     class="text-link"
>+                     xbl:inherits="xbl:text=learnMoreText,xbl:href=learnMoreLink"
>+                     onclick="Browser.addTab(this.href, true); return false;"/>

Use an xbl:inherits for "onlearnmoreclick" instead of hardcoding the behavior in the binding

>diff --git a/components/geolocationPrompt.js b/components/geolocationPrompt.js
>+
>+     var prefService = Cc["@mozilla.org/content-pref/service;1"].getService(Ci.nsIContentPrefService);
>+ 
>+     if (prefService.hasPref(request.requestingURI, "geo.request.remember")) {
>+         if (prefService.getPref(request.requestingURI, "geo.request.remember"))
>+             request.allow();
>+         else
>+             request.cancel();
>+         return;
>+     }
>+ 
>+     function setPagePermission(uri, allow) {
>+         var prefService = Cc["@mozilla.org/content-pref/service;1"].getService(Ci.nsIContentPrefService);
>+         prefService.setPref(uri, "geo.request.remember", allow);
>+     }

2 spaces for indents
 
>+              label: browserBundle.GetStringFromName("geolocation.tellThem"),
>+              accessKey: browserBundle.GetStringFromName("geolocation.tellThemKey"),
>+              callback: function(notification) {                  
>+                  requestingWindow.alert(notification.getAttribute("remember"));
>+                  if (notification.getAttribute("remember"))
>+                      setPagePermission(request.requestingURI, true);
>+                  request.allow(); 
>+              },
>+          },
>+          {
>+              label: browserBundle.GetStringFromName("geolocation.dontTellThem"),
>+              accessKey: browserBundle.GetStringFromName("geolocation.dontTellThemKey"),
>+              callback: function(notification) {
>+                  requestingWindow.alert(notification.getAttribute("remember"));
>+                  if (notification.getAttribute("remember"))
>+                      setPagePermission(request.requestingURI, false);
>+                  request.cancel();
>+              },
>+          }];

2 spaces

>+      try {
>+          newBar.setAttribute("type", "geo"); 
>+
>+          var formatter = Cc["@mozilla.org/toolkit/URLFormatterService;1"].getService(Ci.nsIURLFormatter);
>+          var href = formatter.formatURLPref("browser.geolocation.warning.infoURL");
>+
>+          newBar.setAttribute("learnMoreLink", href);
>+          newBar.setAttribute("learnMoreText", browserBundle.GetStringFromName("geolocation.learnMore"));
>+
>+          newBar.setAttribute("labelon", "asdfasdfasdf");
>+          newBar.setAttribute("rememberChoiceOn", "asdfasdfasdf");
>+          newBar.setAttribute("rememberChoiceOff", "asdfasdfasdf");
>+          newBar.setAttribute("labeloff", "asdfasdfasdf");
>+
>+      }
>+      catch (e) {
>+          requestingWindow.alert(e);
>+      }
>+      //      newBar.ownerDocument.offsetWidth =       newBar.ownerDocument.offsetWidth;
>+      //      newBar.setAttribute("rememberChoice").label = "asdf";
>+

2 spaces

Comment 10

[Doug Turner (:dougt)]

Attachment: wip3

Comment 11

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 376435 [details] [diff] [review]
wip3

>diff --git a/chrome/content/notification.xml b/chrome/content/notification.xml

>+  <binding id="geo-notification" extends="chrome://global/content/bindings/notification.xml#notification">

>+                     xbl:inherits="xbl:href=learnMoreLink"

>+                        xbl:inherits="xbl:checked=geoRememberDecision"/>

Don't think you want the inner "xbl:"s here.

>diff --git a/components/geolocationPrompt.js b/components/geolocationPrompt.js

>+                  if (notification.getAttribute("geoRememberDecision") == true) {

This won't work... I would add a property on geo-notification binding that returns the checkbox's checked state, and check that.

so:
<field name="_checkbox">document.getAnonymousElementByAttribute(this, "anonid", "rememberChoice")</field>
<property name="rememberChoice"
          onget="return this._checkbox.checked;"
          readonly="true"/>

and then just check notification.rememberChoice

>+              label: browserBundle.GetStringFromName("geolocation.share"),
>+              accessKey: null,

Why are you removing access keys again? :)

Comment 12

[Doug Turner (:dougt)]

Attachment: patch v.1

Comment 13

[Doug Turner (:dougt)]

Attachment: patch v.2

mark, can you give this a glance.

Comment 14

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 378528 [details] [diff] [review]
patch v.2

>diff --git a/chrome/content/downloads.js b/chrome/content/downloads.js

>   getDownloads: function dv_getDownloads() {
>-    clearTimeout(this._timeoutID);
>+    if (!this._stmt)  // why do I need this ?
>+       return;
>+		
>+	clearTimeout(this._timeoutID);

What's this for?

>diff --git a/chrome/content/notification.xml b/chrome/content/notification.xml

>+          <xul:label anonid="learnMore"
>+                     class="text-link"
>+                     value="&geolocation.learnMore;"
>+                     onclick="Browser.addTab(this.href, true); return false;"/>

Kinda wish this wasn't hard coded, but I can live with it

>diff --git a/chrome/content/sanitize.js b/chrome/content/sanitize.js

>+    geolocation: {
>+      clear: function ()
>+      {
>+	    // clear any network geolocation provider sessions
>+	    var psvc = Components.classes["@mozilla.org/preferences-service;1"]
>+	                         .getService(Components.interfaces.nsIPrefService);
>+	    try {
>+	        var branch = psvc.getBranch("geo.wifi.access_token.");
>+	        branch.deleteBranch("");
>+
>+            branch = psvc.getBranch("geo.request.remember.");
>+	        branch.deleteBranch("");
>+	
>+	    } catch (e) {dump(e);}

use 2 spaces for indent

>diff --git a/chrome/content/sanitize.xul b/chrome/content/sanitize.xul

>-    <checkbox label="&itemCache.label;"
>+    <checkbox label="&itemGeolocation.label;"

Intentional? I see &itemGeolocation.label; below too

>+	<checkbox label="&itemGeolocation.label;"
>+		      accesskey="&itemGeolocation.accesskey;"
>+		      preference="privacy.item.geolocation"
>+		      onsyncfrompreference="return gSanitizePromptDialog.onReadGeneric();"/>

Indent alignment nit

>diff --git a/components/geolocationPrompt.js b/components/geolocationPrompt.js

>+
>+const XULNS = "http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul";

Still needed?

>   prompt: function(request) {
>+

Remove the blank line

>+    var pm = Cc["@mozilla.org/permissionmanager;1"].getService(Ci.nsIPermissionManager);
>+    var result = pm.testExactPermission(request.requestingURI, "geo");
>+    
>+    if (result == Ci.nsIPermissionManager.ALLOW_ACTION) {
>+      request.allow();
>+      return;
>+    }
>+    if (result == Ci.nsIPermissionManager.DENY_ACTION) {
>+      request.cancel();
>+      return;
>+    }

else if ?

>+    function setPagePermission(uri, allow) {
>+        var prefService = Cc["@mozilla.org/content-pref/service;1"].getService(Ci.nsIContentPrefService);
>+        
>+        if (! prefService.hasPref(request.requestingURI, "geo.request.remember"))
>+            prefService.setPref(request.requestingURI, "geo.request.remember", 0);
>+    
>+    
>+        var count = prefService.getPref(request.requestingURI, "geo.request.remember");
>+    
>+        if (allow == false)
>+            count--;
>+        else
>+            count++;
>+
>+        prefService.setPref(request.requestingURI, "geo.request.remember", count);
>+        
>+        if (count == 2)
>+            pm.add(uri, "geo", Ci.nsIPermissionManager.ALLOW_ACTION);
>+        else if (count == -2)
>+            pm.add(uri, "geo", Ci.nsIPermissionManager.DENY_ACTION);
>+    }

"count" is weird. it looks like it starts at "0", then can be ++ or -- and then we check for 2 or -2. Is this some kind of "if you allow twice we default to allowing?" same for deny?

use 2 spaces for indents in this function

>+              label: browserBundle.GetStringFromName("geolocation.share"),
>+              accessKey: browserBundle.GetStringFromName("geolocation.share.accessKey"),
>+              callback: function(notification) {                  
>+                  setPagePermission(request.requestingURI, true);
>+                  request.allow(); 
>+              },
>+          },
>+          {
>+              label: browserBundle.GetStringFromName("geolocation.dontShare"),
>+              accessKey: browserBundle.GetStringFromName("geolocation.dontShare.accessKey"),
>+              callback: function(notification) {
>+                  setPagePermission(request.requestingURI, false);
>+                  request.cancel();
>+              },
>+          }];

use 2 spaces for indents

>@@ -31,8 +31,9 @@ classic.jar:
>+  images/geo.png                       (images/geo.png)

Can you use the -xx suffix on the image name so we know if it is large or small? geo-24.png or whatever. And remember we'll need 16px for lowres and 24px for hires

Comment 15

[Doug Turner (:dougt)]

Attachment: patch v.3

Comment 16

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 378670 [details] [diff] [review]
patch v.3

>diff --git a/chrome/content/sanitize.js b/chrome/content/sanitize.js
>+    geolocation: {
>+      clear: function ()
>+      {
>+        // clear any network geolocation provider sessions
>+        var psvc = Components.classes["@mozilla.org/preferences-service;1"]
>+                             .getService(Components.interfaces.nsIPrefService);
>+				try {
>+	      	var branch = psvc.getBranch("geo.wifi.access_token.");
>+	        branch.deleteBranch("");
>+
>+					branch = psvc.getBranch("geo.request.remember.");
>+	        branch.deleteBranch("");
>+	    		} catch (e) {dump(e);}

still bad indenting (I see \t in there)

>diff --git a/components/geolocationPrompt.js b/components/geolocationPrompt.js

> Cu.import("resource://gre/modules/XPCOMUtils.jsm");
>+
>+const CountBeforeWeRemember = 5;

use "k" prefix for constants?

>   prompt: function(request) {
>+		var pm = Cc["@mozilla.org/permissionmanager;1"].getService(Ci.nsIPermissionManager);

I see a \t

>+    function setPagePermission(uri, allow) {
>+        var prefService = Cc["@mozilla.org/content-pref/service;1"].getService(Ci.nsIContentPrefService);
>+        
>+        if (! prefService.hasPref(request.requestingURI, "geo.request.remember"))
>+            prefService.setPref(request.requestingURI, "geo.request.remember", 0);
>+    
>+    
>+        var count = prefService.getPref(request.requestingURI, "geo.request.remember");
>+    
>+        if (allow == false)
>+            count--;
>+        else
>+            count++;
>+
>+        prefService.setPref(request.requestingURI, "geo.request.remember", count);
>+        
>+        if (count == CountBeforeWeRemember)
>+            pm.add(uri, "geo", Ci.nsIPermissionManager.ALLOW_ACTION);
>+        else if (count == -CountBeforeWeRemember)
>+            pm.add(uri, "geo", Ci.nsIPermissionManager.DENY_ACTION);
>+    }

This function body is using 4 spaces to indent. Should use 2

>+				label: browserBundle.GetStringFromName("geolocation.share"),
>+				accessKey: browserBundle.GetStringFromName("geolocation.share.accessKey"),
>+				callback: function(notification) {                  
>+					setPagePermission(request.requestingURI, true);
>+					request.allow(); 
>+				},
>+			},
>+			{
>+				label: browserBundle.GetStringFromName("geolocation.dontShare"),
>+				accessKey: browserBundle.GetStringFromName("geolocation.dontShare.accessKey"),
>+				callback: function(notification) {
>+					setPagePermission(request.requestingURI, false);
>+					request.cancel();
>+				},
>+			}];

Holy \t indenting Batman!

>+      var newBar = notificationBox.appendNotification(message,
>+                                                      "geolocation",
>+                                                      "chrome://browser/skin/images/geo-xx.png",
>+                                                      notificationBox.PRIORITY_WARNING_MEDIUM,
>+                                                      buttons);

LOL! geo-xx.png

I meant that you should put the size in there too. geo-24.png, for example. if the current image is 16px and you don't have a 24px - fine, but we'll need a 24px image to ship

>+  images/geo-xx.png                    (images/geo-xx.png)

Next time, r+; I can feel it

Comment 17

[Doug Turner (:dougt)]

Attachment: patch v.4

Comment 18

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 379637 [details] [diff] [review]
patch v.4


>diff --git a/components/geolocationPrompt.js b/components/geolocationPrompt.js

>   prompt: function(request) {
>+        var pm = Cc["@mozilla.org/permissionmanager;1"].getService(Ci.nsIPermissionManager);

use 2 spaces

>+              label: browserBundle.GetStringFromName("geolocation.share"),
>+              accessKey: browserBundle.GetStringFromName("geolocation.share.accessKey"),
>+              callback: function(notification) {
>+                  setPagePermission(request.requestingURI, true);
>+                  request.allow(); 
>+              },
>+          },
>+          {
>+              label: browserBundle.GetStringFromName("geolocation.dontShare"),
>+              accessKey: browserBundle.GetStringFromName("geolocation.dontShare.accessKey"),
>+              callback: function(notification) {
>+                  setPagePermission(request.requestingURI, false);
>+                  request.cancel();
>+              },
>+          }];

use 2 spaces

>+      var newBar = notificationBox.appendNotification(message,
>+                                                      "geolocation",
>+                                                      "chrome://browser/skin/images/geo-16.png",
>+                                                      notificationBox.PRIORITY_WARNING_MEDIUM,
>+                                                      buttons);

geo-16.png is going to look tiny, but we can fix later

>diff --git a/themes/hildon/jar.mn b/themes/hildon/jar.mn

>+  images/geo-xx.png                    (images/geo-xx.png)

don't use geo-xx.png here. use geo-16.png

>+  images/geo-xx.png                    (images/geo-xx.png)

same for wince/jar.mn


ADD geo-16.png to the patch!!

r+ with those fixed

Comment 19

[Doug Turner (:dougt)]

http://hg.mozilla.org/mobile-browser/rev/13c558ea5707

Comment 20

[Aakash Desai [:aakashd]]

verified FIXED on builds:

Mozilla/5.0 (X11; U; Linux armv6l; en-US; rv:1.9.3a1pre) Gecko/20090821
Fennec/1.0b3pre

and

Mozilla/5.0 (Macintosh; U; Intel Mac OSX 10.5; en-US; rv:1.9.2a2pre)
Gecko/20090808 Fennec/1.0b3pre