Closed Bug 484074 Opened 12 years ago Closed 11 years ago

Update Geolocation UI to match latest madhava mockups

Categories

(Firefox for Android Graveyard :: General, defect)

defect
Not set
major

Tracking

(fennec1.0b2+)

VERIFIED FIXED
Tracking Status
fennec 1.0b2+ ---

People

(Reporter: dougt, Assigned: dougt)

References

Details

(Whiteboard: [geo])

Attachments

(1 file, 7 obsolete files)

Attached patch patch v.1 (obsolete) — Splinter Review
+++ 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).
Attachment #368128 - Flags: review?(gavin.sharp)
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 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
Attachment #368128 - Flags: review?(gavin.sharp)
tracking-fennec: --- → ?
doug: does someone need to review this?
Attachment #368128 - Attachment is obsolete: true
after the recent ff geolocation ui changes, this patch is obsolete.
Summary: Update Geolocation Strings to match latest madhava mockups → Update Geolocation UI to match latest madhava mockups
Whiteboard: [geo]
tracking-fennec: ? → 1.0b2+
Attached patch wip (obsolete) — Splinter Review
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.
Attached patch wip2 (obsolete) — Splinter Review
Attachment #374531 - Attachment is obsolete: true
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 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
Attached patch wip3 (obsolete) — Splinter Review
Attachment #375823 - Attachment is obsolete: true
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? :)
Attached patch patch v.1 (obsolete) — Splinter Review
Attachment #376435 - Attachment is obsolete: true
Attachment #376524 - Flags: review?(gavin.sharp)
Attached patch patch v.2 (obsolete) — Splinter Review
mark, can you give this a glance.
Attachment #376524 - Attachment is obsolete: true
Attachment #378528 - Flags: review?(mark.finkle)
Attachment #376524 - Flags: review?(gavin.sharp)
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
Attachment #378528 - Flags: review?(mark.finkle) → review-
Attached patch patch v.3 (obsolete) — Splinter Review
Attachment #378528 - Attachment is obsolete: true
Attachment #378670 - Flags: review?(mark.finkle)
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
Attachment #378670 - Flags: review?(mark.finkle) → review-
Attached patch patch v.4Splinter Review
Attachment #378670 - Attachment is obsolete: true
Attachment #379637 - Flags: review?(mark.finkle)
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
Attachment #379637 - Flags: review?(mark.finkle) → review+
http://hg.mozilla.org/mobile-browser/rev/13c558ea5707
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
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
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.