Closed
Bug 459893
Opened 16 years ago
Closed 16 years ago
Geolocation prompt stops working if initializer window is closed
Categories
(Core :: DOM: Geolocation, defect)
Tracking
()
RESOLVED
FIXED
People
(Reporter: kairo, Assigned: dougt)
References
Details
(Keywords: fixed1.9.1)
Attachments
(3 files, 1 obsolete file)
13.52 KB,
patch
|
smaug
:
review+
Gavin
:
review+
|
Details | Diff | Splinter Review |
16.80 KB,
patch
|
Details | Diff | Splinter Review | |
1.24 KB,
patch
|
Details | Diff | Splinter Review |
The geolocation prompt stops working if the window that initialized it is being closed. STR: 1) Call up http://people.mozilla.org/~dougt/geo.html in a current Minefield nightly 2) Acknowledge the notification bar with either exact location or neighborhood and see the test working fine. 3) Open a new window 4) Close the first window 5) Load http://people.mozilla.org/~dougt/geo.html in the new window With those steps, you don't get another notification bar. This is probably related to bug 459413 comment #2, setting a service callback to a window object might just be a bad idea.
Assignee | ||
Comment 1•16 years ago
|
||
the worry here: http://mxr.mozilla.org/mozilla-central/source/browser/base/content/browser.js#915 is that the browser will close, and the service callback that is set on geolocationService.prompt will be gone.
Assignee: nobody → doug.turner
Updated•16 years ago
|
Flags: blocking-firefox3.1?
Comment 2•16 years ago
|
||
We should probably set the callback in browserglue somewhere, I guess? We don't want to init the geolocation service earlier than it's needed, though... it would probably cleaner to have the geolocation service get a contract ID via the category manager and instantiate its nsIGeolocationPrompt directly when needed, or something like that. Alternatively we could have it send out a notification when it wants to prompt, and have observers return the nsIGeolocationPrompt via aSubject, though that's hackier.
Reporter | ||
Comment 3•16 years ago
|
||
Neil has hacked up something that loads it on ui startup in bug 459413 (nsSuiteGlue is our version of nsBrowserGlue), but your proposal of only loading it when needed sounds even better to me...
Assignee | ||
Updated•16 years ago
|
Component: General → Geolocation
Flags: blocking-firefox3.1?
Product: Firefox → Core
Updated•16 years ago
|
QA Contact: general → geolocation
Updated•16 years ago
|
Flags: blocking1.9.1?
Assignee | ||
Comment 4•16 years ago
|
||
Attachment #344129 -
Flags: superreview?
Attachment #344129 -
Flags: review?
Assignee | ||
Updated•16 years ago
|
Attachment #344129 -
Flags: superreview?
Attachment #344129 -
Flags: review?(gavin.sharp)
Attachment #344129 -
Flags: review?(Olli.Pettay)
Attachment #344129 -
Flags: review?
Assignee | ||
Comment 5•16 years ago
|
||
geolocation will just look up a contract id for the nsIGeolocationPrompt and use that.
Reporter | ||
Comment 6•16 years ago
|
||
I just tested the patch (needed some manual help to apply, might also work with higher fuzz settings), and it works fine for the case described in comment #0. I also did test the core changes with a new SeaMonkey patch in bug 459413 and it also works fine there.
Comment 7•16 years ago
|
||
Comment on attachment 344129 [details] [diff] [review] patch v.1 You need to fix dom/tests/mochitest/geolocation/geolocation_common.js as well, right? Is there any reason to keep NS_GEOLOCATION_SERVICE_CONTRACTID? It would be nice if you could address Neil's second comment from bug 459413 comment 2.
Attachment #344129 -
Flags: review?(gavin.sharp) → review+
Assignee | ||
Comment 8•16 years ago
|
||
gavin, right. I do not need the service contract id now. I was thinking about using it for the extension developers, but for now I can remove it. I will also fix up the mochitest (and figure out why they all pass despite this changeset!)
Updated•16 years ago
|
Attachment #344129 -
Flags: review?(Olli.Pettay) → review+
Comment 9•16 years ago
|
||
Please add some some documentation that nsIGeolocationPrompt must be implemented, otherwise geolocation API doesn't really work. This is especially needed for embedders. Or file a new bug to make geolocation API somehow useable without prompts. Either way.
Assignee | ||
Comment 10•16 years ago
|
||
pushed 3b47c0e2f6d9.
Status: NEW → RESOLVED
Closed: 16 years ago
Resolution: --- → FIXED
Assignee | ||
Comment 11•16 years ago
|
||
reopened due to possible leak.
Assignee | ||
Comment 12•16 years ago
|
||
Assignee | ||
Comment 13•16 years ago
|
||
that last patch included my merge. sorry.
Assignee | ||
Comment 14•16 years ago
|
||
Attachment #344548 -
Attachment is obsolete: true
Assignee | ||
Comment 15•16 years ago
|
||
Comment on attachment 344549 [details] [diff] [review] without merge i am thinking that the way we leak is caused by the way we change the prompt.
Comment 16•16 years ago
|
||
This fixes the leak on linux
Assignee | ||
Comment 17•16 years ago
|
||
thanks smaug. wfm. pushed 89c97fd8a881. Lets see how it goes.
Assignee | ||
Comment 18•16 years ago
|
||
removing blocking1.9.1 flag. bz -- this actually landed on 1.9.1 either before we created the 1.9.1 branch or sometime there after. in either case, 1.9.1 _has_ this fix.
Flags: blocking1.9.1?
Comment 19•16 years ago
|
||
That doesn't affect the blocking status. If it would block if it were to regress tomorrow, it should be marked blocking. This was discussed at the Gecko meeting on Tuesday.
Comment 21•16 years ago
|
||
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/3b47c0e2f6d9
Keywords: fixed1.9.1
You need to log in
before you can comment on or make changes to this bug.
Description
•