569680 - Move the platform specific geolocation files

Status: RESOLVED FIXED

(Core :: DOM: Geolocation, defect)

Description

[Mike Kristoffersen (:MikeK)]

We are creating a new dom/system directory for system specific code, the system specific geolocation code should be moved to this place.

Comment 1

[Mike Kristoffersen (:MikeK)]

Attachment: Part 1/4 Movement of win mobile specific files

Patch depends on the patch series in bug 562181

Comment 2

[Mike Kristoffersen (:MikeK)]

Attachment: Part 2/4 Movement of maemo specific files

Comment 3

[Mike Kristoffersen (:MikeK)]

Attachment: Part 3/4 Movement of shared files

Comment 4

[Mike Kristoffersen (:MikeK)]

Attachment: Part 4/4 Movement of interface files

Comment 5

[Doug Turner (:dougt)]

the patches do not apply cleanly to m-c.

It might be easier for me to review the patches if you just provided the hg move commands, and then one patch that contains the diffs to get everything to build again.

Comment 6

[Mike Kristoffersen (:MikeK)]

Did you apply the patches from bug 562181 before you realized they didn't apply cleanly?

Your wish is my command- I'll reshuffle the content of the patches providing two new patches, one that moves the files, and one that contains the rest of the changes, and update it to trunk if needed.

Comment 7

[Mike Kristoffersen (:MikeK)]

Ahh... now I understand what you mean - like:
hg mv xxxx zzzz
hg mv yyyy tttt

and then a patch....

np - will do

Comment 8

[Mike Kristoffersen (:MikeK)]

Attachment: Updated to trunk as of this morning

Mark, can you have a look at this with Android eyes? - and if you or someone else could try to do an Android compile, then I can fix any fatal issues before it hits the tree...

Comment 9

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 452324 [details] [diff] [review]
Updated to trunk as of this morning


>diff --git a/dom/src/geolocation/Makefile.in b/dom/src/geolocation/Makefile.in
> ifdef WINCE_WINDOWS_MOBILE
>-CPPSRCS         += WinMobileLocationProvider.cpp nsGeoPosition.cpp
>+CPPSRCS         += nsGeoPosition.cpp
>+LOCAL_INCLUDES  += -I$(topsrcdir)/dom/system/android \

Do you want "/dom/system/android" for Windows Mobile?

I don't have an Android build environment yet. Passing to Brad for that part.

Comment 10

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 452324 [details] [diff] [review]
Updated to trunk as of this morning

Looks ok in general

Comment 11

[Doug Turner (:dougt)]

(note that the prompt interface is going to change when e10s lands)

Comment 12

[Mike Kristoffersen (:MikeK)]

Patch depends on the patch in 562181

Comment 13

[Mike Kristoffersen (:MikeK)]

(In reply to comment #9)
> 
> Do you want "/dom/system/android" for Windows Mobile?
> 
Probably not, will post an updated version

Comment 14

[Mike Kristoffersen (:MikeK)]

Attachment: Updated to trunk and fixed the android instead of windows include issue

Comment 15

[Brad Lassey [:blassey] (use needinfo?)]

Comment on attachment 452324 [details] [diff] [review]
Updated to trunk as of this morning

neither this patch, the updated patch or the patch this bug depends on apply cleanly to trunk, so I can't build with it which I'm assuming is what finkle was looking for. The only thing that jumps out at me is the android include path in the windows mobile ifdef that mfinkle already pointed out.

Comment 16

[Mike Kristoffersen (:MikeK)]

The patch that this bug depends on has been updated to trunk, this should allow the latest patch here to apply cleanly.

Comment 17

[Brad Lassey [:blassey] (use needinfo?)]

Comment on attachment 453048 [details] [diff] [review]
Updated to trunk and fixed the android instead of windows include issue

with the updated patch on bug 562181 this applies and seems to work fine

Comment 18

[Mike Kristoffersen (:MikeK)]

Thank you for trying it out - will move this bug to next stage after the patch in bug 562181 has landed

Comment 19

[Doug Turner (:dougt)]

Comment on attachment 453048 [details] [diff] [review]
Updated to trunk and fixed the android instead of windows include issue

> CPPSRCS     = \
>         nsAccelerometerSystem.cpp \
>+	AndroidLocationProvider.cpp \
>         $(NULL)

line up.



>+ifdef MOZ_MAEMO_LIBLOCATION
>+        CPPSRCS         += MaemoLocationProvider.cpp
>+        LOCAL_INCLUDES	+= $(MOZ_PLATFORM_MAEMO_CFLAGS)	\
>+                           -I$(topsrcdir)/dom/src/geolocation \
>+                           $(NULL)	

line up the operators.


>+endif
>+	
> include $(topsrcdir)/config/rules.mk
>\ No newline at end of file

add new line.


>+ifdef WINCE_WINDOWS_MOBILE
>+CPPSRCS         += WinMobileLocationProvider.cpp 
>+
>+LOCAL_INCLUDES	+= -I$(topsrcdir)/dom/src/geolocation \
>+                   $(NULL)
>+endif


line up operators


>         nsIBlocklistService.idl \
>         nsIGIOService.idl \
>         nsIAccelerometer.idl \
>+	nsIGeolocationProvider.idl \
>         $(NULL)

line up.


with ws address r+

Comment 20

[Doug Turner (:dougt)]

note that this should NOT land until e10s lands which is days away.

Comment 21

[Mike Kristoffersen (:MikeK)]

Attachment: Updated with review comments

for (int i=0;i<100;i++) printf("Remember to turn on visible white spaces, that it aligns nicely on your monitor doesn't mean that it looks nice to anyone else...\n");

Comment 22

[Mike Kristoffersen (:MikeK)]

Attachment: Updated to trunk

Comment 23

[Dão Gottwald [:dao]]

http://hg.mozilla.org/mozilla-central/rev/aae7ff3ffc78

Comment 24

[John Daggett (:jtd)]

Mercurial spit out this warning when I pulled just a little while ago:

added 115 changesets with 634 changes to 611 files
warning: detected divergent renames of dom/interfaces/geolocation/nsIGeolocationProvider.idl to:
 dom/interfaces/geolocation/nsIGeolocationPrompt.idl
 xpcom/system/nsIGeolocationProvider.idl

Doesn't seem like a problem but just in case it is...

Comment 25

[Mike Kristoffersen (:MikeK)]

Any idea what that means?