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 Fri, 28 Sep 2012 15:20:17 GMT
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