incubator-callback-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Braden Shepherdson <bra...@chromium.org>
Subject Re: Proposal for a new Plugin.execute() signature on Android
Date Fri, 28 Sep 2012 15:43:26 GMT
I didn't actually add onReset to the IPlugin interface, just to the Plugin
class, for exactly this reason.


On Fri, Sep 28, 2012 at 11:20 AM, Andrew Grieve <agrieve@chromium.org>wrote:

> Did some more thinking about this yesterday, and I think that we should
> remove IPlugin (regardless of other changes). Here's my thinking:
>
> If we provide an interface for plugins, then we're promising to support it.
> Supporting it means not changing it, since *any* change to it will break
> classes that try to implement it. We just broke this rule recently by
> adding an "onReset()" function to it, so once 2.2.0 goes out with this
> change, any non-Plugin-based implementation will have compile errors.
>
> If we change this to supporting a Plugin base class, then that gives us a
> *lot* more flexibility to change things without breaking existing plugins.
> e.g. We can add an onReset() and just have an empty default implementation.
> Another straight-forward tweak is to add the execute(String, String,
> String), which forwards to execute(String, JSONArray, String) by default.
> With a base class, this is a safe change, but with an interface, this will
> break your compile.
>
>
> My second attempt got rid of the executeV2 name. I hate that as well.
>
>
> On Thu, Sep 27, 2012 at 7:50 PM, Dave Johnson <dave.c.johnson@gmail.com
> >wrote:
>
> > 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
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message