I think I can get something up by the end of the day here (should still be the middle of your day :) ) I'll let you know when I have anything that you can evaluate.
On Thu, May 1, 2014 at 11:39 AM, Joe Bowser <[email protected]> wrote: > 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 <[email protected]> > 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 <[email protected]> > wrote: > > > >> On Thu, May 1, 2014 at 9:00 AM, Ian Clelland <[email protected] > >wrote: > >> > >>> On Wed, Apr 30, 2014 at 5:44 PM, Andrew Grieve <[email protected] > >wrote: > >>> > >>>> On Wed, Apr 30, 2014 at 5:35 PM, Joe Bowser <[email protected]> > wrote: > >>>> > >>>> > This is a perfect example of this XKCD: https://xkcd.com/1172/ > >>>> > > >>>> > On Wed, Apr 30, 2014 at 2:25 PM, Ian Clelland < > [email protected]> > >>>> > wrote: > >>>> > > On Wed, Apr 30, 2014 at 3:58 PM, Andrew Grieve < > [email protected] > >>>> > > >>>> > 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 < > [email protected]> > >>>> > wrote: > >>>> > >> > >>>> > >> > On Wed, Apr 30, 2014 at 1:30 PM, Joe Bowser <[email protected] > > > >>>> > wrote: > >>>> > >> > > >>>> > >> > > On Wed, Apr 30, 2014 at 9:35 AM, Michal Mocny < > >>>> [email protected]> > >>>> > >> > wrote: > >>>> > >> > > > On Wed, Apr 30, 2014 at 12:23 PM, Joe Bowser < > >>>> [email protected]> > >>>> > >> > 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 ;) > >>>> > >> > > >>>> > >> > >>>> > > >>>> > >>> > >>> > >> >
