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 Mon, 01 Oct 2012 19:16:27 GMT
Just confirmed that it looks like you did actually add the method to the
interface. Any reasons for keeping this interface? Otherwise, I'll go ahead
and remove it.


On Fri, Sep 28, 2012 at 11:43 AM, Braden Shepherdson <braden@chromium.org>wrote:

> 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