1500+1st B2G can't use geolocation

RESOLVED WONTFIX

Status

()

RESOLVED WONTFIX
6 years ago
4 months ago

People

(Reporter: dougt, Unassigned, Mentored)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [lang=c++])

Attachments

(3 attachments)

(Reporter)

Description

6 years ago
Because of the limit in geo:

http://mxr.mozilla.org/mozilla-central/source/dom/src/geolocation/nsGeolocation.cpp#71

It looks like we will not be able to have the 1501st application use geolocation.
(Reporter)

Comment 1

6 years ago
josh, maybe we should make this list handling smarter?

Comment 2

6 years ago
For WatchPosition that would make a lot of sense. I doubt that the pending callbacks list needs anything better.

Comment 3

6 years ago
The other way we could sidestep the issue is by making each ContentParent hold on to a unique Geolocation object instead of having one global one for all IPC listeners.
(Reporter)

Comment 4

6 years ago
good first bug?

Updated

6 years ago
Whiteboard: [mentor=jdm][lang=c++]

Comment 5

6 years ago
I want to work on this, please suggest where can I start?

Comment 6

6 years ago
In places in ContentParent where we get a global geolocation object (such as http://hg.mozilla.org/mozilla-central/annotate/4ac6c72b06c8/dom/ipc/ContentParent.cpp#l2374), we instead want to try storing a member variable in the constructor and using do_CreateInstance instead.

Comment 7

6 years ago
If i am getting this right?

I should store member variable in ContentParent.h

  nsCOMPtr<nsIDOMGeoGeolocation> mGeo;  //private

Then in ContentParent.cpp (under constructor)

  mGeo = do_CreateInstance("@mozilla.org/geolocation;1");

Then instead of 

 nsCOMPtr<nsIDOMGeoGeolocation> geo = do_GetService("@mozilla.org/geolocation;1");

just use mGeo pointer everywhere instead of creating as above in file ContentParent.cpp

Comment 8

6 years ago
Exactly right.

Comment 9

6 years ago
I applied these http://pastebin.mozilla.org/2440348 changes.
But there is compilation error-
 "d:\mozilla-source\mozilla-central\obj-i686-pc-mingw32\dist\include\nsIDOMGeoGeolocation.h(21) : fatal error C1083: Cannot open include file: 'DictionaryHelpers.h': No such file or directory"
You can get around that by simply forward-declaring nsIDOMGeolocation in the header file (ie. "class nsIDOMGeolocation;").
To be specific, you can forward-declare the class instead of including the header file.

Comment 12

6 years ago
Created attachment 754499 [details] [diff] [review]
Changed ContentParent class to use its own gelocation object instead of global geolocation object used through out application

Updated

6 years ago
Attachment #754499 - Flags: review?(josh)

Updated

6 years ago
Assignee: nobody → manu_talkin
Comment on attachment 754499 [details] [diff] [review]
Changed ContentParent class to use its own gelocation object instead of global geolocation object used through out application

Review of attachment 754499 [details] [diff] [review]:
-----------------------------------------------------------------

Great! There are a couple changes to make, and then you should be sure to follow https://developer.mozilla.org/en/Creating_a_patch_that_can_be_checked_in, so that your change has a commit message like "Bug 874069 - Use one geolocation instance object per ContentParent. r=jdm"

::: dom/ipc/ContentParent.cpp
@@ +1051,5 @@
>      // No more than one of !!aApp, aIsForBrowser, and aIsForPreallocated should
>      // be true.
>      MOZ_ASSERT(!!aApp + aIsForBrowser + aIsForPreallocated <= 1);
>  
> +	// Create Geolocation object for this ContentParent

This comment doesn't really add much.

@@ +1052,5 @@
>      // be true.
>      MOZ_ASSERT(!!aApp + aIsForBrowser + aIsForPreallocated <= 1);
>  
> +	// Create Geolocation object for this ContentParent
> +	mGeo = do_CreateInstance("@mozilla.org/geolocation;1");

Nit: please use spaces instead of tabs.

@@ +2371,5 @@
>      return true;
>  }
>  
> +int32_t
> +ContentParent::AddGeolocationListener(nsIDOMGeoPositionCallback* watcher, bool highAccuracy)

Now that this is a member function, get rid of the watcher parameter and just use |this| instead.

@@ +2376,2 @@
>  {
> +  

Nit: delete this blank line.
Attachment #754499 - Flags: review?(josh) → feedback+

Comment 14

6 years ago
Created attachment 755809 [details] [diff] [review]
Use one geolocation instance object per ContentParent.

Updated

6 years ago
Attachment #755809 - Flags: review?(josh)
Comment on attachment 755809 [details] [diff] [review]
Use one geolocation instance object per ContentParent.

Review of attachment 755809 [details] [diff] [review]:
-----------------------------------------------------------------

Great! I apologize for taking so long on this.
Attachment #755809 - Flags: review?(josh) → review+

Updated

6 years ago
Keywords: checkin-needed
Manu, can you look into the test failures that occurred?
Flags: needinfo?(manu_talkin)

Comment 19

6 years ago
Created attachment 759624 [details]
./mach xpcshell-test Output

Observation
 I ran ./mach xpcshell-test and found out that test passes for test_contentPrefs_parentipc.js
but there are other failures.
Any suggestions?
Flags: needinfo?(manu_talkin)
Oh, one thing that could make a difference is if you're not using a debug build. https://developer.mozilla.org/en-US/docs/Configuring_Build_Options explains that you need a .mozconfig file in the mozilla-central root that contains "ac_add_options --enable-debug". You'll need to do a full rebuild, unfortunately.

Updated

5 years ago
Assignee: manu_talkin → nobody
(Assignee)

Updated

5 years ago
Mentor: josh
Whiteboard: [mentor=jdm][lang=c++] → [lang=c++]
Mass closing as we are no longer working on b2g/firefox os.
Status: NEW → RESOLVED
Last Resolved: 4 months ago
Resolution: --- → WONTFIX
Mass closing as we are no longer working on b2g/firefox os.
You need to log in before you can comment on or make changes to this bug.