Sounds like a pretty serious change, but I think we should do it. That being said, we're going to have to toss out all our existing JUnit tests. How soon can you get the changes into the branch?
On Thu, May 1, 2014 at 8:02 AM, Ian Clelland <iclell...@chromium.org> wrote: > The only way I can see out of this mess is to *not* turn CordovaWebView > into an interface, but instead make it an abstract base class. (I'm not > sure yet whether it has to inherit from anything; maybe Joe's original > refactorings mean that it can inherit only from Object, but I'm not sure > that it doesn't need to be a View) > > What I would do, then, is create a concrete AndroidCordovaWebView class > which extends it, and which has an AndroidWebView as a member. > (AndroidWebView still extends android.webkit.WebView, like it does now). > This would parallel the XWalkCordovaWebView class, which contains a private > XWalkView member. > > I think that the job of AndroidCordovaWebView would be to manage > cordova-related things, like plugins, and to delegate all of the web-viewy > things to its member WebView. I'm trying to work out the exact dividing > line between them; I suspect that I require more coffee to figure that one > out. > > Joe -- does this seem like a workable solution? It's essentially already > the way that the crosswalk plugin works, and I think that it solves the > issue of pluginManager needing to remain a public member. There may be > other implications that I just haven't seen yet, though. > > Ian > > > On Thu, May 1, 2014 at 9:32 AM, Ian Clelland <iclell...@chromium.org> wrote: > >> On Thu, May 1, 2014 at 9:00 AM, Ian Clelland <iclell...@chromium.org>wrote: >> >>> On Wed, Apr 30, 2014 at 5:44 PM, Andrew Grieve <agri...@chromium.org>wrote: >>> >>>> On Wed, Apr 30, 2014 at 5:35 PM, Joe Bowser <bows...@gmail.com> wrote: >>>> >>>> > This is a perfect example of this XKCD: https://xkcd.com/1172/ >>>> > >>>> > On Wed, Apr 30, 2014 at 2:25 PM, Ian Clelland <iclell...@chromium.org> >>>> > wrote: >>>> > > On Wed, Apr 30, 2014 at 3:58 PM, Andrew Grieve <agri...@chromium.org >>>> > >>>> > wrote: >>>> > > >>>> > >> Config.xml is not a very sane way to do things for the embedded >>>> webview >>>> > >> case. E.g. you may want two webviews with different configs. >>>> > Config.java is >>>> > >> a singleton right now, and I think it would be much nicer as a >>>> parameter >>>> > >> you could give to the WebView upon initialization & plugins should >>>> say: >>>> > >> this.getConfig().getPreference() rather than using it as a >>>> singleton. >>>> > So... >>>> > >> If we could leave setPreference() in for now, I think we should. >>>> When we >>>> > >> remove it, we should provide a nice API for the embedding case >>>> (e.g. a >>>> > >> Config without the need to hit the filesystem). >>>> > >>>> >>>> Although I still think it shouldn't be a singleton, I've realized that >>>> plugins require it reading from config.xml, so an all-in-memory config >>>> doesn't make sense (would break plugins). Maybe for prefs it still >>>> would... >>>> not sure. Just thinking out loud on this one. Don't have a proposal. >>>> >>>> >>>> >>>> > >> >>>> > >> >>>> > >> pluginManager being public is unfortunate. That said, other than >>>> > >> getPlugin(), I don't see any methods in it that plugins should >>>> need. If >>>> > >> we're to remove the property, I don't think we should expose >>>> > PluginManager >>>> > >> to plugins, but rather try and keep that an internal detail. >>>> > >> >>>> > > >>>> > > This is actually what I was working on last night -- the problem is >>>> that >>>> > > plugins *do* use that field right now. File, File-Transfer, and Media >>>> > > Capture all use it to get access to other plugins to use their APIs. >>>> > > (through this.webView.pluginManager.getPlugin("somePlugin") ) >>>> > > >>>> > > It does make sense for plugins to have native APIs as well as >>>> JavaScript >>>> > > APIs, and the only way to expose that right now is through the plugin >>>> > > manager. >>>> > > >>>> > > Unfortunately, it's been a public field on the plugin's webView >>>> object, >>>> > and >>>> > > there's no easy way to transition that to a setter. At least, not in >>>> a >>>> > way >>>> > > that ensures that both existing and new plugins can work with pre- >>>> and >>>> > > post-3.5.0 Cordova. >>>> > >>>> >>>> I think we're saying the same thing. I called out getPlugin() as the one >>>> plugins would actually need. It's things like init() shouldn't be touched >>>> by plugins. >>>> >>> >>> Yep -- we appear to be in violent agreement with each other. I just >>> missed your original "other than getPlugin" disclaimer :) >>> >>> >>>> >>>> I don't see a clean way to not break this. I doubt it's used much by >>>> non-core plugins, so I think it's worth changing & doing a major semver >>>> bump for Android. >>>> >>> >>> This seems like it should be a last resort -- it probably means that >>> there would be no more releases of those plugins for Android engines < >>> 3.5.0(4?), without a lot of work on our part. It would probably involve >>> maintaining two streams of releases for some time. >>> >> >> Ultimately, I think you're right, though. >> >> The issue isn't making a new release of the plugins that works with old >> and new cordova engines. That's possible (though ugly) with reflection; I >> got it working a couple of nights ago when the problem was first apparent. >> The real issue is that there's no way for the existing published plugins >> to possibly work with this change to the API. That's a >> backwards-incompatibility no matter how you look at it, and means that we >> need to bump the major. >> >> Maybe we can use this as an opportunity to define a real inter-plugin >> native API, as a feature of a Cordova 4 release? >> >> >>> >>> >>>> >>>> >>>> >>>> > > >>>> > > >>>> > >> >>>> > >> >>>> > >> >>>> > >> >>>> > >> >>>> > >> >>>> > >> >>>> > >> On Wed, Apr 30, 2014 at 1:58 PM, Michal Mocny <mmo...@chromium.org> >>>> > wrote: >>>> > >> >>>> > >> > On Wed, Apr 30, 2014 at 1:30 PM, Joe Bowser <bows...@gmail.com> >>>> > wrote: >>>> > >> > >>>> > >> > > On Wed, Apr 30, 2014 at 9:35 AM, Michal Mocny < >>>> mmo...@chromium.org> >>>> > >> > wrote: >>>> > >> > > > On Wed, Apr 30, 2014 at 12:23 PM, Joe Bowser < >>>> bows...@gmail.com> >>>> > >> > wrote: >>>> > >> > > > >>>> > >> > > >> Hey >>>> > >> > > >> >>>> > >> > > >> So, once again, we're dealing with some major API changes >>>> once we >>>> > >> > > >> introduce pluggable webview. The first change that was done >>>> for >>>> > >> > > >> sanity was finally deprecating setProperty. This was slated >>>> to >>>> > be >>>> > >> > > >> removed by 3.5 or in six months from the deprecation date, >>>> but we >>>> > >> kept >>>> > >> > > >> it in too long. While I would like to assume that everyone >>>> has >>>> > >> moved >>>> > >> > > >> over to setting their preferences in config.xml, which is the >>>> > much >>>> > >> > > >> more sane way of doing things, we can't do that. We need to >>>> > >> publicize >>>> > >> > > >> this in some blog posts, as well as in our documentation >>>> somehow. >>>> > >> > > >> There will obviously be some pissed off users, as we've seen >>>> in >>>> > past >>>> > >> > > >> posts, but I think having the ability to use a WebView other >>>> than >>>> > >> > > >> Chrome 30 is worth these changes. >>>> > >> > > >> >>>> > >> > > > >>>> > >> > > > Is it feasible to leave setProperty working only for default >>>> > WebView? >>>> > >> > > This >>>> > >> > > > would mean that custom webviews won't work with older plugins, >>>> > but I >>>> > >> > > think >>>> > >> > > > thats fine. >>>> > >> > > > >>>> > >> > > >>>> > >> > > The setProperty methods are actually in Cordova-Activity, and we >>>> > could >>>> > >> > > re-add those. The thing is that these aren't actually used by >>>> > >> > > plugins, and instead are legacy methods that only our unit tests >>>> > use. >>>> > >> > > I'll put them back in. >>>> > >> > > >>>> > >> > > > >>>> > >> > > >> >>>> > >> > > >> The other change, which says more about our design is adding >>>> a >>>> > >> getter >>>> > >> > > >> method for pluginManager. We need to access the >>>> pluginManager to >>>> > >> get >>>> > >> > > >> plugins, and it's expected that everyone who implements a >>>> > >> > > >> CordovaWebView will have this method produce a >>>> pluginManager. In >>>> > >> the >>>> > >> > > >> past, it was just publicly exposed, which was not the >>>> greatest >>>> > idea >>>> > >> > > >> and was kinda sloppy. >>>> > >> > > >> >>>> > >> > > > >>>> > >> > > > Similar to above question, could we leave it (deprecated) as >>>> an >>>> > >> exposed >>>> > >> > > > property only on the default webview? And only support the >>>> new >>>> > >> getter >>>> > >> > > for >>>> > >> > > > new webviews (xwalk, gecko)? Again, only updated plugins >>>> would >>>> > work >>>> > >> > with >>>> > >> > > > custom webview, but I think thats fine. >>>> > >> > > > >>>> > >> > > >>>> > >> > > No, I don't think so. It's probably better to make a clean >>>> break >>>> > and >>>> > >> > > have all the WebViews expected to function the same than to have >>>> > some >>>> > >> > > plugins simply fail with certain webviews. Plugins breaking >>>> across >>>> > >> > > all the WebViews will force people to fix them, while things >>>> > breaking >>>> > >> > > with only Crosswalk will put crosswalk at an unfair >>>> disadvantage. >>>> > >> > > >>>> > >> > >>>> > >> > Trust me, Crosswalk is going to have an unfair *advantage* >>>> regardless >>>> > of >>>> > >> > plugin support ;) >>>> > >> > >>>> > >> >>>> > >>>> >>> >>> >>
