Last Comment Bug 488472 - SeaMonkey integration for Network Geolocation Provider
: SeaMonkey integration for Network Geolocation Provider
Status: RESOLVED FIXED
:
Product: SeaMonkey
Classification: Client Software
Component: General (show other bugs)
: Trunk
: All All
: -- normal (vote)
: seamonkey2.0b1
Assigned To: Robert Kaiser
:
:
Mentors:
Depends on: 487467 488821
Blocks: 491835 494421 494422 494424
  Show dependency treegraph
 
Reported: 2009-04-15 05:37 PDT by Robert Kaiser
Modified: 2011-01-11 16:45 PST (History)
3 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
Port recent changes to SeaMonkey, enable network provider via Google (16.32 KB, patch)
2009-04-30 17:14 PDT, Robert Kaiser
neil: superreview-
Details | Diff | Splinter Review
address Neil's review comments (16.29 KB, patch)
2009-05-06 08:56 PDT, Robert Kaiser
neil: review+
neil: superreview+
Details | Diff | Splinter Review
Updated patch, v3 (17.39 KB, patch)
2009-05-22 14:50 PDT, Robert Kaiser
neil: review+
kairo: superreview+
Details | Diff | Splinter Review
test bustage fix (1.98 KB, patch)
2009-05-23 08:22 PDT, Robert Kaiser
neil: review+
Details | Diff | Splinter Review

Description Robert Kaiser 2009-04-15 05:37:26 PDT
Bug 487467 added a Network Geolocation Provider to the platform and integrated it with Firefox. We still need to do the SeaMonkey integration parts, but those should be pretty easy, it's merely a port of the few things that bug patched in browser/ (packaging, sanitize integration, and the pref, from what I see).
Comment 1 Robert Kaiser 2009-04-15 05:42:46 PDT
Of course, we could wait slightly for bug 488218 (UI improvements, once again) and do both in one go.
Comment 2 Robert Kaiser 2009-04-30 17:14:05 PDT
Created attachment 375264 [details] [diff] [review]
Port recent changes to SeaMonkey, enable network provider via Google

This ports bug 487467 and bug 488218 including the L10n fixups from bug 488574.

While testing with http://people.mozilla.org/~dougt/geo.html I sometimes encountered bug 490046 so if you see breakage with "update is null" in error console while reviewing, don't bother, fix is on the way. :)

BTW, I'm working on getting the "Learn more" page up, for now, we don't have any locale in, we possibly should look into that before final though.
Comment 3 neil@parkwaycc.co.uk 2009-05-01 13:34:35 PDT
Comment on attachment 375264 [details] [diff] [review]
Port recent changes to SeaMonkey, enable network provider via Google

>+pref("browser.geolocation.warning.infoURL", "http://www.seamonkey-project.org/doc/2.0/geolocation");
[Any point putting this in SeaMonkey help? Also the page is misleading when it refers to undoing permission, as this only applies to the temporary permission granted, there's no way to undo the remember function. Perhaps this could be done in a separate bug by introducing a separate manager along the lines of the cookie or image manager.]

>+    var prefService = Components.classes["@mozilla.org/content-pref/service;1"]
>+                                .getService(Components.interfaces.nsIContentPrefService);
Nit: variable name is confusing, I thought this was the pref service.

>+              if (notification.ownerDocument.getElementById("rememberChoice").checked)
I see Firefox fixed this in bug 488821.

>+      function geolocation_hacks_to_notification () {
Aptly named!

>+  skin/classic/communicator/icons/Geo.png                               (communicator/icons/Geo.png)
Don't we normally use lower case?
Comment 4 Robert Kaiser 2009-05-06 08:50:13 PDT
(In reply to comment #3)
> (From update of attachment 375264 [details] [diff] [review])
> >+pref("browser.geolocation.warning.infoURL", "http://www.seamonkey-project.org/doc/2.0/geolocation");
> [Any point putting this in SeaMonkey help? Also the page is misleading when it
> refers to undoing permission, as this only applies to the temporary permission
> granted, there's no way to undo the remember function. Perhaps this could be
> done in a separate bug by introducing a separate manager along the lines of the
> cookie or image manager.]

Putting this in help surely makes sense, at least in addition, maybe even as the primary thing linked from there, but I think having a web page for it makes sense in any case, so I think it's probably best to go with this for now and possibly do the help thing in a followup. Also, I only ripped off the text from the Firefox page, which was reworked since then, I need to do that rework on our as well.

The manager ideas sounds interesting, esp. if we make this a general content prefs manager, as we don't give users any access to content prefs right now, but it could be useful for more than this geolocation feature. But IMHO it's definitely a followup bug topic.

> >+              if (notification.ownerDocument.getElementById("rememberChoice").checked)
> I see Firefox fixed this in bug 488821.

I take it that this was the real reason for the r- as the other comments look more like nits than negative review to me...

> >+  skin/classic/communicator/icons/Geo.png                               (communicator/icons/Geo.png)
> Don't we normally use lower case?

Not sure if we have any guidelines on that, I just used the file name Firefox has as well, but it's easy to change to lower case. Note that we probably also need a followup to replace this image with one that better fits with the rest of the theme in Modern, I currently just have a copy of the default them one in there.
Comment 5 Robert Kaiser 2009-05-06 08:56:35 PDT
Created attachment 376018 [details] [diff] [review]
address Neil's review comments

Here's the patch that addresses the review comments. We still need to get in contact with Google to make sure it's OK for us to use their service there, dougt has promised to help there.
Comment 6 neil@parkwaycc.co.uk 2009-05-06 15:59:38 PDT
Comment on attachment 376018 [details] [diff] [review]
address Neil's review comments

>+          callback: function(notification) {                  
>+              if (notification.getElementsByClassName("rememberChoice")[0].checked)
>+                setPagePermission(aRequest.requestingURI, true);
>+              aRequest.allow(); 
>+          },
>         }, {
>           label: notificationBundle.GetStringFromName("geolocation.dontTellThem"),
>           accessKey: notificationBundle.GetStringFromName("geolocation.dontTellThemKey"),
>-          callback: function() { aRequest.cancel() },
>+          callback: function(notification) {
>+              if (notification.getElementsByClassName("rememberChoice")[0].checked)
>+                setPagePermission(aRequest.requestingURI, false);
>+              aRequest.cancel(); 
Nit: three trailing whitespaces crept in.
Comment 7 Robert Kaiser 2009-05-06 16:37:52 PDT
Oh, by the way, Firefox has bug 491732 for the issue of deleting the "remember" content pref.
Comment 8 neil@parkwaycc.co.uk 2009-05-08 03:17:16 PDT
(In reply to comment #5)
> We still need to get in contact with Google to make sure it's OK for
> us to use their service there, dougt has promised to help there.
That doesn't block updating the UI bits though, right?
Comment 9 Robert Kaiser 2009-05-22 09:58:52 PDT
(In reply to comment #8)
> (In reply to comment #5)
> > We still need to get in contact with Google to make sure it's OK for
> > us to use their service there, dougt has promised to help there.
> That doesn't block updating the UI bits though, right?

No, I filed bug 494421 for this and will do an updated patch here that will exclude the pref with the Google URL. Doug says that will make us just report to the content that the location is unavailable.
Comment 10 Robert Kaiser 2009-05-22 14:50:48 PDT
Created attachment 379252 [details] [diff] [review]
Updated patch, v3

This patch updates in a way that it can land right now and does sync up with what Firefox 3.5 ships (at least in RC1).

Changes compared to last patch:
* Not setting actual Google provider URL by default, deferred to bug 494421.
* Ported switch to permission mechanism from bug 491732.
* Ported string changes from bug 491739.
* Fixed nit about trailing whitespace.

No large architectural change, but some code changes, so carrying forward sr but re-requesting r?
Comment 11 neil@parkwaycc.co.uk 2009-05-22 16:05:27 PDT
Comment on attachment 379252 [details] [diff] [review]
Updated patch, v3

>+    var result = pm.testExactPermission(aRequest.requestingURI, "geo");
>+
>+    if (result == nsIPermissionManager.ALLOW_ACTION) {
>+      request.allow();
>+      return;
>+    }
>+
>+    if (result == nsIPermissionManager.DENY_ACTION) {
>+      request.cancel();
>+      return;
>+    }
I'd be tempted to write this as follows:
switch (pm.testExactPermission(aRequest.requestingURI, "geo")) {
  case nsIPermissionManager.ALLOW_ACTION:
    request.allow();
    return;
  case nsIPermissionManager.DENY_ACTION:
    request.cancel();
    return;
}

>+      if (allow == true)
>+        pm.add(uri, "geo", nsIPermissionManager.ALLOW_ACTION);
>+      else
>+        pm.add(uri, "geo", nsIPermissionManager.DENY_ACTION);
My first instinct was to rewrite this as follows:
pm.add(uri, "geo", allow ? nsIPermissionManager.ALLOW_ACTION : nsIPermissionManager.DENY_ACTION);

>+          callback: function(notification) {
>+              if (notification.getElementsByClassName("rememberChoice")[0].checked)
>+                setPagePermission(aRequest.requestingURI, true);
>+              aRequest.allow();
>+          },
... but then I noticed that this might as well call pm.add directly, or at least pass nsIPermssionManager.XXX_ACTION as the parameter to setPagePermission, if you think pm.add(uri, "geo", action); is worth keeping separate ;-)

>+          callback: function(notification) {
>+              if (notification.getElementsByClassName("rememberChoice")[0].checked)
>+                setPagePermission(aRequest.requestingURI, false);
>+              aRequest.cancel();
>+          },
Don't know why I didn't notice it before, but the indentation in these callbacks should be 2, not 4.
Comment 12 Robert Kaiser 2009-05-23 04:35:02 PDT
Pushed with comments/nits addressed as http://hg.mozilla.org/comm-central/rev/f93a4c731519
Comment 13 Serge Gautherie (:sgautherie) 2009-05-23 07:08:24 PDT
(In reply to comment #12)
> Pushed with comments/nits addressed as
> http://hg.mozilla.org/comm-central/rev/f93a4c731519

This causes a timeout:
{
http://tinderbox.mozilla.org/showlog.cgi?log=SeaMonkey/1243078632.1243082246.11678.gz
Linux comm-central unit test on 2009/05/23 04:37:12
http://tinderbox.mozilla.org/showlog.cgi?log=SeaMonkey/1243078633.1243084585.25036.gz
OS X 10.4 comm-central unit test on 2009/05/23 04:37:13
http://tinderbox.mozilla.org/showlog.cgi?log=SeaMonkey-Ports/1243079045.1243084211.24024.gz
Linux comm-1.9.1 unit test on 2009/05/23 04:44:05

*** 41751 INFO Running /tests/dom/tests/mochitest/geolocation/test_timeoutWatch.html...
*** 41752 INFO TEST-PASS | /tests/dom/tests/mochitest/geolocation/test_timeoutWatch.html | Got notification box
*** 41753 INFO TEST-PASS | /tests/dom/tests/mochitest/geolocation/test_timeoutWatch.html | Got geolocation notification
*** 41754 INFO TEST-PASS | /tests/dom/tests/mochitest/geolocation/test_timeoutWatch.html | Got button

command timed out: 300 seconds without output, killing pid 11600
process killed by signal 9
program finished with exit code -1

http://tinderbox.mozilla.org/showlog.cgi?log=SeaMonkey-Ports/1243079831.1243085179.26019.gz
WINNT 5.2 comm-1.9.1 unit test on 2009/05/23 04:57:11

...
command timed out: 300 seconds without output
program finished with exit code 1
}
Comment 14 Robert Kaiser 2009-05-23 08:03:43 PDT
OK, I can reproduce, and it's actually test_optional_api_params.html that fails. Let me find out why it ends up in this JS error:
Error: notification.getElementsByClassName("rememberChoice")[0] is undefined
Source File: file:///mnt/mozilla/build/seamonkey/mozilla/dist/bin/components/nsSuiteGlue.js
Line: 421

The bar that is up actually shows the checkbox so I wonder why it would fail to see it in the test.
Comment 15 Robert Kaiser 2009-05-23 08:22:24 PDT
Created attachment 379352 [details] [diff] [review]
test bustage fix

This fixes the test bustage. Apparently the test is so fast in clicking the button on the notification bar that our setTimeout() hack hasn't run to set up the checkbox yet.
It's safe to just ignore the "remember" checkbox if it's not there, so I'm doing this in the fix. I'm checking this in right away to fix the bustage, but would like post-facto-review on it nevertheless.

I actually wonder why Firefox doesn't run into this problem.
Comment 16 Robert Kaiser 2009-05-23 08:26:01 PDT
Test bustage fix pushed as http://hg.mozilla.org/comm-central/rev/e1cb651f2c53 - I wonder if Doug has any idea why we are seeing this race but Firefox isn't.
Comment 17 Serge Gautherie (:sgautherie) 2009-05-23 08:36:46 PDT
(In reply to comment #15)
> Apparently the test is so fast in clicking the
> button on the notification bar that our setTimeout() hack hasn't run to set up
> the checkbox yet.

Fwiw, if the issue is only (expected) with the test, maybe it just needs an executeSoon()?
Comment 18 neil@parkwaycc.co.uk 2009-05-23 11:29:58 PDT
Comment on attachment 379352 [details] [diff] [review]
test bustage fix

I would have had a very slight preference for .item(0) or .length as those avoid strict JS warnings during the tests.
Comment 19 Robert Kaiser 2009-05-23 11:41:55 PDT
OK, review is here, tinderboxes start passing the geolocation tests again, many things are mostly fine, marking fixed.

Note You need to log in before you can comment on or make changes to this bug.