The default bug view has changed. See this FAQ.

SeaMonkey integration for Network Geolocation Provider

RESOLVED FIXED in seamonkey2.0b1

Status

SeaMonkey
General
RESOLVED FIXED
8 years ago
6 years ago

People

(Reporter: Robert Kaiser, Assigned: Robert Kaiser)

Tracking

Trunk
seamonkey2.0b1
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 2 obsolete attachments)

(Assignee)

Description

8 years ago
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).
(Assignee)

Comment 1

8 years ago
Of course, we could wait slightly for bug 488218 (UI improvements, once again) and do both in one go.
(Assignee)

Comment 2

8 years ago
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.
Assignee: nobody → kairo
Status: NEW → ASSIGNED
Attachment #375264 - Flags: superreview?(neil)
Attachment #375264 - Flags: review?(neil)

Comment 3

8 years ago
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?
Attachment #375264 - Flags: superreview?(neil)
Attachment #375264 - Flags: superreview-
Attachment #375264 - Flags: review?(neil)
(Assignee)

Comment 4

8 years ago
(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.
(Assignee)

Comment 5

8 years ago
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.
Attachment #375264 - Attachment is obsolete: true
Attachment #376018 - Flags: superreview?(neil)
Attachment #376018 - Flags: review?(neil)

Comment 6

8 years ago
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.
Attachment #376018 - Flags: superreview?(neil)
Attachment #376018 - Flags: superreview+
Attachment #376018 - Flags: review?(neil)
Attachment #376018 - Flags: review+
(Assignee)

Comment 7

8 years ago
Oh, by the way, Firefox has bug 491732 for the issue of deleting the "remember" content pref.
(Assignee)

Updated

8 years ago
Blocks: 491835

Comment 8

8 years ago
(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?
(Assignee)

Updated

8 years ago
Blocks: 494421
(Assignee)

Comment 9

8 years ago
(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.
(Assignee)

Updated

8 years ago
Blocks: 494422
(Assignee)

Updated

8 years ago
Blocks: 494424
(Assignee)

Comment 10

8 years ago
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?
Attachment #376018 - Attachment is obsolete: true
Attachment #379252 - Flags: superreview+
Attachment #379252 - Flags: review?(neil)
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.
Attachment #379252 - Flags: review?(neil) → review+
(Assignee)

Comment 12

8 years ago
Pushed with comments/nits addressed as http://hg.mozilla.org/comm-central/rev/f93a4c731519
Status: ASSIGNED → RESOLVED
Last Resolved: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → seamonkey2.0b1
No longer blocks: 467530
(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
}
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(Assignee)

Comment 14

8 years ago
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.
(Assignee)

Comment 15

8 years ago
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.
Attachment #379352 - Flags: review?(neil)
(Assignee)

Comment 16

8 years ago
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.
(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 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.
Attachment #379352 - Flags: review?(neil) → review+
(Assignee)

Comment 19

8 years ago
OK, review is here, tinderboxes start passing the geolocation tests again, many things are mostly fine, marking fixed.
Status: REOPENED → RESOLVED
Last Resolved: 8 years ago8 years ago
Resolution: --- → FIXED
Depends on: 488821
You need to log in before you can comment on or make changes to this bug.