incubator-callback-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Dave Johnson <dave.c.john...@gmail.com>
Subject Re: Proposal for a new Plugin.execute() signature on Android
Date Thu, 27 Sep 2012 23:50:18 GMT
There was a lot of discussion many moons ago about that plugin arch
and I think we ended up on IPlugin + Plugin just so that people could
go with IPlugin and implement everything themselves or use our
hopefully sane defaults in Plugin. There's likely very few people
using IPlugin.

Also, can we avoid the "executeV2" method name by any chance? Reminds
me of net.rim.device.api.browser.field2 :)

On Thu, Sep 27, 2012 at 10:28 PM, Andrew Grieve <agrieve@chromium.org> wrote:
> Here it is:
> https://github.com/mmocny/incubator-cordova-android/pull/2/files
>
> I delete IPlugin in this version, but that probably wasn't a great idea
> since other people may have used the symbol. Still not understanding what
> it's utility is though, but if we go ahead with this, I'll at *least*
> revert deleting IPlugin, and do that in a separate change, or more likely,
> just leave IPlugin alone.
>
>
> On Wed, Sep 26, 2012 at 2:58 PM, Andrew Grieve <agrieve@chromium.org> wrote:
>
>> Aha, okay. So on iOS they happen asynchronously to the webcore thread, but
>> all execute in order on the UI thread, I think even moving away from having
>> each execute on a new thread by default would bring the behaviour closer to
>> iOS.
>>
>> The other big unknown to me, is the question of why the IPlugin interface
>> exists instead of just using the Plugin base class. Does anyone know of any
>> other implementations of IPlugin besides Plugin?
>>
>> I'm going to take a stab at re-writing this change to use a super-class to
>> have the new signature, and have the existing Plugin class extend that and
>> provide the shim, and will report back for everyone to review again.
>> Probably won't get to it today, but maybe tomorrow.
>>
>>
>> On Wed, Sep 26, 2012 at 2:04 PM, Simon MacDonald <
>> simon.macdonald@gmail.com> wrote:
>>
>>> Yeah, sorry I meant to get back to you on that. The major reason for
>>> switching everything to async was that iOS can only do async and this
>>> helped keep the code bases/API consistent.
>>>
>>> Simon Mac Donald
>>> http://hi.im/simonmacdonald
>>>
>>>
>>> On Wed, Sep 26, 2012 at 1:47 PM, Andrew Grieve <agrieve@chromium.org>
>>> wrote:
>>> > Okay, so I think everyone is on the same page in terms of not breaking
>>> > existing plugins.
>>> >
>>> > Does anyone know what the reason was for making plugins async by
>>> default?
>>> >
>>> >
>>> > On Wed, Sep 26, 2012 at 1:38 PM, Mike Reinstein <
>>> reinstein.mike@gmail.com>wrote:
>>> >
>>> >> Agreed! If we can get to some kind of stability with the API exposed
to
>>> >> plugin developers it will go a long way.
>>> >>
>>> >>
>>> >>
>>> >> On Wed, Sep 26, 2012 at 1:32 PM, Simon MacDonald
>>> >> <simon.macdonald@gmail.com>wrote:
>>> >>
>>> >> > Agreed. We've broken the plugins so many times that I'm more that
>>> sure
>>> >> > that 3rd party devs are sick of it. The last time we broken the
>>> >> > interface on Android was in 1.9.0 and then we broke it again in
2.0.0
>>> >> > on the JavaScript side. I'd rather not break it again for 2.2.0.
>>> >> >
>>> >> > Also, when I say "break" I mean the code I wrote to the previous
>>> >> > specification will no longer compile so I need to make changes
to my
>>> >> > plugin. Often we can get around this by adding in a shim which
I
>>> >> > believe is the best way to go.
>>> >> >
>>> >> > Simon Mac Donald
>>> >> > http://hi.im/simonmacdonald
>>> >> >
>>> >> >
>>> >> > On Wed, Sep 26, 2012 at 3:56 AM, Brian LeRoux <b@brian.io>
wrote:
>>> >> > > The only concern I have is the deprecation path needs to be
long
>>> and
>>> >> > > noisy---this is probably the biggest possible breaking change
we
>>> could
>>> >> > > introduce to the platform.
>>> >> > >
>>> >> > > Maybe even longer than our usual 6months / but wait until
3.0
>>> >> > >
>>> >> > > Thoughts on that?
>>> >> > >
>>> >> > >
>>> >> > > On Tue, Sep 25, 2012 at 4:56 PM, Andrew Grieve <
>>> agrieve@chromium.org>
>>> >> > wrote:
>>> >> > >> Michal - Yep, good summary, that's exactly the case.
>>> >> > >> Simon - totally agree. I'll change what I've got to add
a second
>>> >> > executeV2
>>> >> > >> which takes in a JSONArray, and have the String-based
one just
>>> call
>>> >> > that.
>>> >> > >>
>>> >> > >> The reason to need an executeV2 is threading, so I'll
focus on
>>> that.
>>> >> > >>
>>> >> > >> My biggest gripe against the current signature, is that
it
>>> defaults to
>>> >> > >> running things on a background thread. I expect most calls
will be
>>> >> fast
>>> >> > >> enough to execute inline, some calls to need to run on
the UI
>>> thread,
>>> >> > and
>>> >> > >> then only some to require doing a lot of work on a background
>>> thread.
>>> >> > >> Furthermore, those that do require a background would
often
>>> benefit
>>> >> from
>>> >> > >> doing some param/state checking on the calling thread
before
>>> moving to
>>> >> > the
>>> >> > >> background thread.
>>> >> > >>
>>> >> > >> I wouldn't be proposing a new signature if there was a
way to
>>> change
>>> >> > >> isSync() from defaulting to false to defaulting to true,
but I
>>> don't
>>> >> > think
>>> >> > >> that's a safe thing to change.
>>> >> > >>
>>> >> > >> On iOS, plugins execute on the calling thread and it's
up to them
>>> to
>>> >> > >> dispatch background threads if they need them.
>>> >> > >>
>>> >> > >> Michal pointed out that you can't comment on a diff in
github, so
>>> I
>>> >> > opened
>>> >> > >> a pull request with the patch to enable commenting:
>>> >> > >>
>>> >> >
>>> >>
>>> https://github.com/agrieve/incubator-cordova-android/commit/a73dffc99847b14031c1138611bb8772dc9d7b7e
>>> >> > >>
>>> >> > >>
>>> >> > >>
>>> >> > >>
>>> >> > >>
>>> >> > >> On Tue, Sep 25, 2012 at 10:51 AM, Simon MacDonald <
>>> >> > simon.macdonald@gmail.com
>>> >> > >>> wrote:
>>> >> > >>
>>> >> > >>> Here is what I was thinking on:
>>> >> > >>>
>>> >> > >>> https://issues.apache.org/jira/browse/CB-1530
>>> >> > >>>
>>> >> > >>> In the PluginManager change the code so that is calls:
>>> >> > >>>
>>> >> > >>> plugin.execute(string, string, string);
>>> >> > >>>
>>> >> > >>> Then in the Plugin class add a new default method
that does the
>>> >> > following:
>>> >> > >>>
>>> >> > >>> public PluginResult execute(String action, String
args, String
>>> >> > callbackId)
>>> >> > >>> {
>>> >> > >>>     return execute(action, new JSONArrary(args), callbackId);
>>> >> > >>> }
>>> >> > >>>
>>> >> > >>> so that all the current plugins continue to work without
needing
>>> any
>>> >> > >>> changes. If someone wants to provide their own JSON
parsing they
>>> can
>>> >> > >>> override the plugin.execute(string, string, string)
method and
>>> do it
>>> >> > >>> themselves.
>>> >> > >>>
>>> >> > >>> Simon Mac Donald
>>> >> > >>> http://hi.im/simonmacdonald
>>> >> > >>>
>>> >> > >>>
>>> >> > >>> On Tue, Sep 25, 2012 at 10:33 AM, Michal Mocny <
>>> mmocny@chromium.org>
>>> >> > >>> wrote:
>>> >> > >>> >
>>> >> > >>> > Summarizing what I think I'm hearing:
>>> >> > >>> >
>>> >> > >>> > The current exec signature will currently:
>>> >> > >>> > (a) automatically parse JSON arguments, and
>>> >> > >>> > (b) automatically move async calls onto a background
thread.
>>> >> > >>> >
>>> >> > >>> > While both of the features simplify plugin developers
in most
>>> >> cases,
>>> >> > >>> > sometimes manual control is desired (ie, for
the two bugs you
>>> link
>>> >> > to).
>>> >> > >>> >
>>> >> > >>> > That sounds reasonable, however, I think I'm
also hearing a
>>> >> proposal
>>> >> > to
>>> >> > >>> > replace the existing execute signature (deprecating
the current
>>> >> > one).  If
>>> >> > >>> > for the majority of cases we are happy with the
current
>>> signature,
>>> >> > then
>>> >> > >>> is
>>> >> > >>> > there perhaps a less intrusive solution?  Or
maybe we aren't
>>> happy
>>> >> > with
>>> >> > >>> the
>>> >> > >>> > current signature, and this new signature is
generally more
>>> future
>>> >> > proof,
>>> >> > >>> > more performant, etc, giving us other reasons
for changing?
>>>  Also,
>>> >> > how
>>> >> > >>> does
>>> >> > >>> > this compare with other platforms?
>>> >> > >>> >
>>> >> > >>> > -Michal
>>> >> > >>> >
>>> >> > >>> >
>>> >> > >>> > On Mon, Sep 24, 2012 at 11:50 PM, Andrew Grieve
<
>>> >> agrieve@google.com>
>>> >> > >>> wrote:
>>> >> > >>> >
>>> >> > >>> > > Means to address two bugs:
>>> >> > >>> > >
>>> >> > >>> > > https://issues.apache.org/jira/browse/CB-1530
>>> >> > >>> > > https://issues.apache.org/jira/browse/CB-1532
>>> >> > >>> > >
>>> >> > >>> > > I wanted to gather some opinions from those
who have been
>>> around
>>> >> > for
>>> >> > >>> > > longer. Here is the proposed change:
>>> >> > >>> > >
>>> https://github.com/agrieve/incubator-cordova-android/compare/ft3
>>> >> > >>> > >
>>> >> > >>> > > My main motivation is for FileTransfer,
I need to register
>>> the
>>> >> > transfer
>>> >> > >>> > > synchronously so that a subsequent abort()
will not have a
>>> race
>>> >> > >>> condition.
>>> >> > >>> > > I then perform the transfer in a background
thread. I *could*
>>> >> > implement
>>> >> > >>> > > this using the current signature by returning
true in
>>> isSync()
>>> >> and
>>> >> > then
>>> >> > >>> > > returning a NO_RESULT result, but I think
the intentions are
>>> >> > clearer
>>> >> > >>> with
>>> >> > >>> > > the new signature.
>>> >> > >>> > >
>>> >> > >>>
>>> >> >
>>> >>
>>>
>>
>>

Mime
View raw message