incubator-callback-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Filip Maj <...@adobe.com>
Subject Re: Geolocation: Adhering to W3C Specification
Date Fri, 30 Mar 2012 16:40:55 GMT
OK: one last big push for this.

I have a branch on my fork of cordova-android with the necessary native
changes [1].

I have updated the manual tests for location in mobile-spec [2] so that it
is easy to explicitly test the cordova plugin version vs. the version
attached to navigator.geolocation (presumably the "native" version). I
might also add a few more Qunit tests but there isn't much to add there,
as it's difficult to assert things about the state of GPS visibility on a
device, etc.

SO: the iOS native bits seem to be doing OK following Shaz's/Becky's
review. Android peeps: can you take a look? I did testing last night on my
ICS phone and it worked well.

I would like to see this get in today but I understand if we don't want to
get it into master before we tag 1.6.0 :)

[1] https://github.com/filmaj/incubator-cordova-android/tree/geoupdate
[2] https://github.com/filmaj/incubator-cordova-mobile-spec/tree/geoupdate

On 3/29/12 12:56 PM, "Filip Maj" <fil@adobe.com> wrote:

>Updated my branch with that change now.
>
>On 3/29/12 11:31 AM, "Filip Maj" <fil@adobe.com> wrote:
>
>>Good catch Becky, thanks. I'll patch that shortly.
>>
>>On 3/29/12 11:02 AM, "Becky Gibson" <gibson.becky@gmail.com> wrote:
>>
>>>I think there is an issue in the LocationManager: didUpdateToLocation:
>>>fromLocation method.  The NSFastEnumeration protocol for NSDictionary
>>>iterates over the keys and based how watchCallbacks is created I think
>>>you
>>>want the object.
>>>
>>>Thus, I think this:
>>>
>>>for (NSString *callbackId in self.locationData.watchCallbacks) {
>>>            [self returnLocationInfo:callbackId];
>>>        }
>>>
>>>should be:
>>>
>>>for (NSString *timerId in self.locationData.watchCallbacks) {
>>>            [self returnLocationInfo:[self.locationData.watchCallbacks
>>>objectForKey: timerId ]];
>>>        }
>>>
>>>-becky
>>>
>>>On Wed, Mar 28, 2012 at 9:37 PM, Shazron <shazron@gmail.com> wrote:
>>>
>>>> Never mind - saw that NetworkStatus commit was already in the mainline
>>>>as
>>>> well
>>>>
>>>> 2012/3/28 Shazron <shazron@gmail.com>:
>>>> > Oh - when you merge it in, don't squash your commits in this case
>>>> > unless you list all the changes in the squash. Good to know about
>>>>the
>>>> > "Network Status" change since I needed to know about this in the
>>>> > updated guides I'm writing.
>>>> >
>>>> > 2012/3/28 Filip Maj <fil@adobe.com>:
>>>> >> Thanks for looking it over Shaz, much appreciated - the link helps
>>>>too.
>>>> I
>>>> >> am slowly getting over my aversion of invoking functions using
>>>>square
>>>> >> brackets and accepting objective-c for what it is.
>>>> >>
>>>> >> On 3/28/12 4:26 PM, "Shazron" <shazron@gmail.com> wrote:
>>>> >>
>>>> >>>Looks good Fil - good to get the iOS only cruft out of there.
>>>> >>>
>>>> >>>Minor point but there's a way to hide the private vars in the
>>>> >>>implementation file since users won't need to really see them
in
>>>>the
>>>> >>>public headers, through class extensions:
>>>> >>>
>>>>
>>>>http://stackoverflow.com/questions/5826345/how-to-declare-instance-vari
>>>>a
>>>>b
>>>>l
>>>> >>>es-and-methods-not-visible-or-usable-outside-of-t
>>>> >>>Not that prevents access, just visibility, and we are open source
>>>>after
>>>> >>>all...
>>>> >>>
>>>> >>>2012/3/28 Filip Maj <fil@adobe.com>:
>>>> >>>> I've sent a pull request to both apache/cordova-js and
>>>> >>>>apache/cordova-ios
>>>> >>>> for the geo stuff (side note: github pull requests might
be
>>>>broken
>>>> again
>>>> >>>> :/)
>>>> >>>>
>>>> >>>> https://github.com/apache/incubator-cordova-ios/pull/8
>>>> >>>>
>>>> >>>>
>>>> >>>> The above gets the native iOS geolocation plugin lined up
to the
>>>>new
>>>> >>>> interface imposed by cordova-js.
>>>> >>>>
>>>> >>>> The JS file is bundled in the branch so you don't need to
rebuild
>>>> >>>> cordova-js to see these improvements.
>>>> >>>>
>>>> >>>> Shaz and anyone else keen on the iOS platform: can you guys
take
>>>>a
>>>> look?
>>>> >>>> My tests on my end look good but would love another set
of eyes.
>>>> >>>>
>>>> >>>> On 3/27/12 5:00 PM, "Filip Maj" <fil@adobe.com> wrote:
>>>> >>>>
>>>> >>>>>So we'll keep it around (even though we won't override
the
>>>> >>>>>implementation
>>>> >>>>>that we get for free in the WebView), and thus we'll
need to
>>>>update
>>>> the
>>>> >>>>>native implementation. Gotcha. I will update the JIRA
issue as
>>>>such
>>>> >>>>>then.
>>>> >>>>>
>>>> >>>>>On 3/27/12 4:57 PM, "Joe Bowser" <bowserj@gmail.com>
wrote:
>>>> >>>>>
>>>> >>>>>>We should axe it/move it to its own plugins repo.
 Someone may
>>>>want
>>>> it,
>>>> >>>>>>but
>>>> >>>>>>I don't want to make promises for it.
>>>> >>>>>>
>>>> >>>>>>On Tue, Mar 27, 2012 at 4:55 PM, Filip Maj <fil@adobe.com>
>>>>wrote:
>>>> >>>>>>
>>>> >>>>>>> I am going to assume then that will be merged
in and we'll be
>>>> making
>>>> >>>>>>>the
>>>> >>>>>>> necessary native tweaks across the platforms
we want to
>>>>support.
>>>> >>>>>>>
>>>> >>>>>>> ANDROID peeps: should we axe native geo code,
or should we
>>>>keep
>>>>it
>>>> >>>>>>>around
>>>> >>>>>>> and thus update the implementation to follow
this new
>>>>approach?
>>>> >>>>>>>
>>>> >>>>>>> On 3/27/12 3:33 PM, "Shazron" <shazron@gmail.com>
wrote:
>>>> >>>>>>>
>>>> >>>>>>> >Apple should be following W3C, although
I don't know if they
>>>>fixed
>>>> >>>>>>> >this bug yet, it's still unresolved for
me in Radar:
>>>> >>>>>>> >http://openradar.appspot.com/radar?id=1160403
but based on
>>>>what
>>>> my
>>>> >>>>>>> >test on 5.1, they've fixed it.
>>>> >>>>>>> >
>>>> >>>>>>> >Sure ping me on email/jabber let's set up
a time.
>>>> >>>>>>> >
>>>> >>>>>>> >On Tue, Mar 27, 2012 at 3:07 PM, Filip Maj
<fil@adobe.com>
>>>>wrote:
>>>> >>>>>>> >> Assuming that the native WebView implementations
across
>>>>whatever
>>>> >>>>>>> >>platforms
>>>> >>>>>>> >> adhere to the W3C Geo spec, then these
native changes would
>>>>line
>>>> >>>>>>>up
>>>> >>>>>>>our
>>>> >>>>>>> >> implementation with what users are
expecting in their
>>>>browser.
>>>> >>>>>>> >>
>>>> >>>>>>> >> I can help with tweaking the implementation
on iOS, but
>>>>would
>>>> love
>>>> >>>>>>>if
>>>> >>>>>>> >>you
>>>> >>>>>>> >> could once-over it, Shaz, and perhaps
jump on a quick
>>>>remote
>>>> hack
>>>> >>>>>>>sesh
>>>> >>>>>>> >> with me for 15-20 mins to make sure
we are looking good.
>>>> >>>>>>> >>
>>>> >>>>>>> >> On 3/27/12 2:46 PM, "Shazron" <shazron@gmail.com>
wrote:
>>>> >>>>>>> >>
>>>> >>>>>>> >>>Thanks Fil - I'm all for fixing
geolocation in iOS. There's
>>>> >>>>>>>several
>>>> >>>>>>> >>>jira issues for it, and I've been
attempting to fix it as
>>>>best I
>>>> >>>>>>>can,
>>>> >>>>>>> >>>but users are still reporting problems
with it since it
>>>>doesn't
>>>> >>>>>>>match
>>>> >>>>>>> >>>the native implementation of UIWebView.
>>>> >>>>>>> >>>
>>>> >>>>>>> >>>On Mon, Mar 26, 2012 at 4:00 PM,
Filip Maj <fil@adobe.com>
>>>> wrote:
>>>> >>>>>>> >>>> Hey all,
>>>> >>>>>>> >>>>
>>>> >>>>>>> >>>> The past week or so I've been
working on revamping the
>>>> >>>>>>>geolocation
>>>> >>>>>>> >>>>tests according to what is laid
out by the W3C API [1].
>>>>Been
>>>> >>>>>>>tracking
>>>> >>>>>>> >>>>progress and whatnot in a JIRA
issue [2].
>>>> >>>>>>> >>>>
>>>> >>>>>>> >>>> Good news: I've got the tests
implemented plus cordova-js
>>>> >>>>>>>passing
>>>> >>>>>>>said
>>>> >>>>>>> >>>>tests (compare view to see diff
available @ [3]).
>>>> >>>>>>> >>>>
>>>> >>>>>>> >>>> Bad news: we've been doing
it wrong in our native
>>>> >>>>>>>implementations
>>>> >>>>>>>forĊ 
>>>> >>>>>>> >>>>well, ever, it seems.
>>>> >>>>>>> >>>>
>>>> >>>>>>> >>>> Moving forward would like to
hear suggestions from
>>>>everyone.
>>>> >>>>>>> >>>>
>>>> >>>>>>> >>>> Breaking down what we didn't
do in the past that the spec
>>>> >>>>>>>mandates:
>>>> >>>>>>> >>>>
>>>> >>>>>>> >>>>  *   Properly implementing
a timeout. It is one of the
>>>> available
>>>> >>>>>>> >>>>options that you can pass into
getCurrentPosition /
>>>> >>>>>>>watchPosition.
>>>> >>>>>>> >>>>However, we've been using it
to date as essentially a
>>>> "frequency"
>>>> >>>>>>> >>>>parameter for watchPosition,
I.e. "give me position
>>>>updates
>>>> every
>>>> >>>>>>> >>>><options.timeout> milliseconds".
This is wrong. According
>>>>to
>>>> the
>>>> >>>>>>>spec,
>>>> >>>>>>> >>>>the timeout option defines how
long after invoking a
>>>> >>>>>>>watch/getCurrent
>>>> >>>>>>> >>>>the error callback should wait
before it fires with a
>>>>TIMEOUT
>>>> >>>>>>> >>>>PositionError object.
>>>> >>>>>>> >>>>  *   There is no control over
how often watchPosition
>>>>should
>>>> >>>>>>>fire
>>>> >>>>>>> >>>>success callbacks. Instead,
the spec says: "In step 5.2.2
>>>>of
>>>> the
>>>> >>>>>>>watch
>>>> >>>>>>> >>>>process, the successCallback
is only invoked when a new
>>>> position
>>>> >>>>>>>is
>>>> >>>>>>> >>>>obtained and this position differs
signifficantly from the
>>>> >>>>>>>previously
>>>> >>>>>>> >>>>reported position. The definition
of what consitutes a
>>>> >>>>>>>significant
>>>> >>>>>>> >>>>difference is left to the implementation."
>>>> >>>>>>> >>>>  *   I've also added tests
+ control of comparing the
>>>> >>>>>>>"maximumAge"
>>>> >>>>>>> >>>>parameter on the JS side, and
keeping a reference to the
>>>>last
>>>> >>>>>>> >>>>successful
>>>> >>>>>>> >>>>position retrieved from the
native framework and comparing
>>>>its
>>>> >>>>>>> >>>>timestamp
>>>> >>>>>>> >>>>together with maximumAge. This
should implement proper
>>>>caching
>>>> of
>>>> >>>>>>> >>>>positioning on the WebView side
and hopefully save some
>>>>native
>>>> >>>>>>>round
>>>> >>>>>>> >>>>trips.
>>>> >>>>>>> >>>>
>>>> >>>>>>> >>>> All of this means the the API
on the native side for
>>>> geolocation
>>>> >>>>>>>will
>>>> >>>>>>> >>>>change (sorry iOS!). Basically
we have three actions that
>>>>the
>>>> >>>>>>> >>>>Geolocation plugin should listen
for:
>>>> >>>>>>> >>>>
>>>> >>>>>>> >>>>  *   getLocation, which takes
as parameters
>>>>enableHighAccuracy
>>>> >>>>>>> >>>>(boolean) and maximumAge (int
as milliseconds).
>>>> >>>>>>> >>>>  *   addWatch, parameter: only
the usual callbackID
>>>>required.
>>>> >>>>>>> >>>>  *   clearWatch, parameter:
only the usual callbackID
>>>> required.
>>>> >>>>>>> >>>>
>>>> >>>>>>> >>>> getLocation should require
very little changing (other
>>>>than
>>>> not
>>>> >>>>>>> >>>>needing
>>>> >>>>>>> >>>>the timeout parameter anymore,
since that is handled on
>>>>the
>>>>JS
>>>> >>>>>>>side
>>>> >>>>>>>in
>>>> >>>>>>> >>>>my patch).
>>>> >>>>>>> >>>>
>>>> >>>>>>> >>>> addWatch should keep a list
of callback Ids, and, as soon
>>>>as
>>>> we
>>>> >>>>>>>have
>>>> >>>>>>> >>>>one watch started, the native
framework should start
>>>>watching
>>>> the
>>>> >>>>>>> >>>>position for a "significant
position difference". Once
>>>>that
>>>> >>>>>>>happens, it
>>>> >>>>>>> >>>>should fire the success callback(s)
for all stored watch
>>>> callback
>>>> >>>>>>>Ids.
>>>> >>>>>>> >>>>If there is an issue retrieving
position, it should fire
>>>>the
>>>> >>>>>>>error
>>>> >>>>>>> >>>>callback(s) for all stored watch
callback Ids.
>>>> >>>>>>> >>>>
>>>> >>>>>>> >>>> I commented out a bunch of
iOS-specific code that already
>>>>did
>>>> a
>>>> >>>>>>> >>>>"distance filter" type of approach
to position
>>>>acquisition,
>>>>but
>>>> >>>>>>>was
>>>> >>>>>>> >>>>only
>>>> >>>>>>> >>>>available if you provided undocumented
parameters in the
>>>> options
>>>> >>>>>>> >>>>object.
>>>> >>>>>>> >>>>Not sure about how feasible
a distance filter is in
>>>>Android, or
>>>> >>>>>>>Windows
>>>> >>>>>>> >>>>Phone, or our other supported
platforms.
>>>> >>>>>>> >>>>
>>>> >>>>>>> >>>> One final point of discussion
worth bringing up about
>>>>this
>>>> >>>>>>>issue.
>>>> >>>>>>> >>>>BlackBerry and Android use the
default implementation of
>>>> >>>>>>>geolocation
>>>> >>>>>>> >>>>abilities in their respective
WEbViews. Because of this I
>>>>would
>>>> >>>>>>>mandate
>>>> >>>>>>> >>>>removal of any Geolocation java
code from the Android +
>>>> >>>>>>>BlackBerry
>>>> >>>>>>> >>>>implementations. Saves some
bytes. Originally we had the
>>>> Android
>>>> >>>>>>>plugin
>>>> >>>>>>> >>>>classes in there for support
for devices before 2.0. Since
>>>>we
>>>> are
>>>> >>>>>>>only
>>>> >>>>>>> >>>>supporting 2.0 and above, this
is no longer needed. Are
>>>>there
>>>> any
>>>> >>>>>>> >>>>issues
>>>> >>>>>>> >>>>with this?
>>>> >>>>>>> >>>>
>>>> >>>>>>> >>>> Appreciate you guys looking
this over.
>>>> >>>>>>> >>>>
>>>> >>>>>>> >>>> Cheers,
>>>> >>>>>>> >>>> Fil
>>>> >>>>>>> >>>>
>>>> >>>>>>> >>>> [1]
>>>> http://dev.w3.org/geo/api/spec-source.html#api_description
>>>> >>>>>>> >>>> [2] https://issues.apache.org/jira/browse/CB-359
>>>> >>>>>>> >>>> [3]
>>>> >>>>>>> >>>>
>>>> >>>>>>>
>>>> >>>>>>>
>>>> https://github.com/filmaj/incubator-cordova-js/compare/master...geotes
>>>> >>>>>>>t
>>>> >>>>>>> >>>>s
>>>> >>>>>>> >>
>>>> >>>>>>>
>>>> >>>>>>>
>>>> >>>>>
>>>> >>>>
>>>> >>
>>>>
>>
>


Mime
View raw message