On Wed, Oct 1, 2014 at 4:53 PM, Joe Bowser <bows...@gmail.com> wrote:
> On Wed, Oct 1, 2014 at 1:18 PM, Andrew Grieve <agri...@chromium.org> > wrote: > > > On Wed, Oct 1, 2014 at 11:07 AM, Joe Bowser <bows...@gmail.com> wrote: > > > > > On Wed, Oct 1, 2014 at 7:24 AM, Andrew Grieve <agri...@chromium.org> > > > wrote: > > > > > > > Good idea (I think)! > > > > > > > > In my mind there's two main things wrong with our current > abstractions: > > > > 1. WebView plugins should extend a class instead of implement an > > > interface. > > > > - Reason: You can't make *any* changes to an interface without > > breaking > > > > the compile for everyone that implements it > > > > > > > > > > Reason we use an interface: Java does not support multiple inheritance. > > > Both Ian and I have said this numerous times, and you wishing that this > > > wasn't the case isn't going to magically make this change. This is the > > > only way this feature is even possible. Also, this is an API that > we're > > > designing, and we shouldn't be messing with the interface. We're stuck > > > with a lot of WebView-isms that may not even make sense on non-Chromium > > > webviews, but I think the surface has been decreased. > > > > > > > I don't think we need multiple inheritance. What would you want to > inherit > > from? > > > > AndroidWebView inherits form WebView, and the Mozilla WebView that I'm > working on inherits from GeckoView. They both can be used as their own > views, and most of our tests still work. By removing the interface and > saying that things aren't views anymore breaks the WebView feature that > we've worked on two releases ago that some people still use. Sure, you > don't use it, and you have made it clear that you don't care about these > users, but we need to have some sort of consistency. I'd rather have > people just rename a class than lose an entire feature because you don't > think it's important. > I think my concern here is similar, so let me try an give some examples: - 3.0.x has CordovaWebView as a class that extends WebView - 4.0.x has CordovaWebView as an interface. - Any code that previously used CordovaWebView as a View or a WebView will no longer compile, since CordovaWebView is an interface. Even though AndroidWebView extend WebView, the type system doesn't know that. You could update your code by casting to an AndroidWebView explicitly to fix the compile errors, but then you're losing out on the 4.0.x ability to use different WebViews. - What we did was add a getView() method, which you should use to get at the actual android.ui.View class in a WebView-independent way. Yes, this is a breaking change, but it's broken in the same way interface or not. Any embedder or plugins that assumes CordovaWebView is a WebView (or a View even) will not compile anymore with it being an Interface. Making it a class doesn't fix this either, but it does allow us to add new methods in the future without breaking things. True, we could create a new Interface each time we wanted to change it (CordovaWebviewV1, CordovaWebViewV2, etc), I think that will be a lot more complicated than following the pattern that we use for CordovaPlugin (a base class that we can add to, but not remove from) > > > > I think it's unrealistic to think that we'll never want to add any > methods > > or parameters to the interface. > > > > > And this thinking is why Cordova is so terribly unstable, and why quality > has gone down since 3.0. For all the talk about not changing APIs and not > screwing over our users, you seriously managed to do exactly that with your > refactors that you did on master without using a topic branch. That's > exactly why we had to re-do the tag at 3.6.4, because Marcel and Martin ran > into problems with your refactor. This is the line in the sand, this is an > API that we want third-party people to use, we can't keep making changes to > it in minor versions. > > > > > > > > > > > > 2. We need to reduce the amount of copy & pasted code between the > > > > implementations. > > > > > > > > > > Given that this is literally the AndroidWebView that exists in the > 4.0.x > > > repo ripped out, this is copy and pasted code so that it actually > works. > > > > > > > To clarify: I think the distinction should be "what code is specific to > my > > webview" vs. "what code is specific to any webview within Cordova". And > > right now there's a good amount of Cordova-specific code in the > cross-walk > > and AndroidWebView code. > > > > > There's a lot of code that needs to be in a WebView for Cordova. This is > because of the numerous use cases, such as multiple WebViews, where each > instance needs to keep track of its own set of plugins, for starters. > There are probably more use cases that need to be though of, but expecting > there to be no Cordova code is ridiculous. Also, part of the reason we're > doing a Mozilla WebView is because it's so radically different from the > Chromium based WebViews we can use it to re-think what is needed. > > Honestly, if you think you can do a better job on the API and keep some > backwards compatibility with the current API, spin up a topic branch, but > the 4.0.x branch already contains code that was approved months ago, and > your opposition to it now is ridiculous. It's like you're trying to derail > the work because of some unknown reason, which wouldn't be a new thing at > all. > I'm sorry you feel this way. I assure you I have no secret agenda. I just want the design to be as maintainable as possible. I did a good amount of work on 4.0.x a while ago, that's what made me think that the design is not quite right yet. I may be wrong, but I don't remember any sort of approval happening for this. > > > > > > > > > > > > > > > > > > On Tue, Sep 30, 2014 at 3:58 PM, Sergey Grebnov (Akvelon) < > > > > v-seg...@microsoft.com> wrote: > > > > > > > > > +1 > > > > > It would be also great if there is a way to extend/override some > > > default > > > > > webview functionality as well (not only provide full webview > > > > implementation) > > > > > > > > > > For example, what will be recommended way to extend default > > > > > onReceivedHttpAuthRequest functionality if I want to automatically > > show > > > > > credentials dialog in case of 401 response? > > > > > > > > > > > > > > > > > > > > > > > > > https://github.com/apache/cordova-android/blob/master/framework/src/org/apache/cordova/CordovaWebViewClient.java#L116 > > > > > > > > > > Thx! > > > > > Sergey > > > > > -----Original Message----- > > > > > From: Joe Bowser [mailto:bows...@gmail.com] > > > > > Sent: Tuesday, September 30, 2014 10:44 PM > > > > > To: dev > > > > > Subject: Re: [Android] Third Party WebView Reference Implementation > > > > > > > > > > On Tue, Sep 30, 2014 at 10:42 AM, Michal Mocny < > mmo...@chromium.org> > > > > > wrote: > > > > > > > > > > > To make sure I understand, this is implementing the system > webview > > as > > > > > > a pluggable-webview-via-plugin? > > > > > > > > > > > > > > > > > Yes. The idea was to use this to figure out our API surface and to > > see > > > > > what's necessary and what's not. Also, we need a way to test this > > > > without > > > > > having to rely on third party bits from Intel or anyone else. > While > > > it's > > > > > the same implementation, the fact is that it's a slightly different > > > class > > > > > so we could in theory test this and hopefully automate it. > > > > > > > > > > > > > > > > This, is interesting. > > > > > > > > > > > > On Tue, Sep 30, 2014 at 12:47 PM, Joe Bowser <bows...@gmail.com> > > > > wrote: > > > > > > > > > > > > > Hey > > > > > > > > > > > > > > So, as part of the work for Third Party Webviews, I decided to > > > write > > > > > > > up a super quick reference implementation by just copying the > > > > > > > AndroidWebView > > > > > > and > > > > > > > making it a plugin. I haven't fully tested this yet, but it > > should > > > > > > > work > > > > > > as > > > > > > > a third-party WebView. > > > > > > > > > > > > > > The tricky part that I found here is how much of Cordova we > have > > to > > > > > > > keep exposed for Third-Party WebViews to work. > > > > > > > > > > > > > > Anyway, the work is on GitHub with Apache Licences. Once it > gets > > > > > > massaged > > > > > > > a bit more, it may need a home at Apache. > > > > > > > > > > > > > > https://github.com/infil00p/ThirdPartyWebViewRef > > > > > > > > > > > > > > Feel free to fork it and go through it and put feedback on this > > > > thread. > > > > > > > It's the same code as AndroidWebView in the 4.0.x branch, so > some > > > of > > > > > > > this may be using exposed APIs out of convenience. > > > > > > > > > > > > > > Joe > > > > > > > > > > > > > > > > > > > > > > > > > > > >
