incubator-callback-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Andrew Grieve <agri...@chromium.org>
Subject Re: Proposal for a new Plugin.execute() signature on Android
Date Thu, 27 Sep 2012 20:28:51 GMT
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
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message