[Cronet] Change interface APIs to abstract classes

[Cronet] Change interface APIs to abstract classes

This allows for adding members in the future with default implementations,
thus allowing easier upgrading to the new APIs for existing embedders.

Committed: https://crrev.com/d4ac94937db989b40acd8f6fa51465d6174f608f
Cr-Commit-Position: refs/heads/master@{#348605}

[pauljensen, 2015-09-03 00:48:04]

pauljensen@chromium.org changed reviewers:
+ xunjieli@chromium.org

[pauljensen, 2015-09-03 00:48:04]

Helen, PTAL, thank you!
(Misha, you're CC'd so you're aware of this change that will require updating
embedders.)

[xunjieli, 2015-09-03 04:32:02]

https://codereview.chromium.org/1307863006/diff/20001/components/cronet/READM...
File components/cronet/README.md (right):

https://codereview.chromium.org/1307863006/diff/20001/components/cronet/READM...
components/cronet/README.md:11: First you will need to implement the
`UrlRequestListener` class to handle
I am not sure... Should we also use "extend" instead of "implement" in
sentences? Loosely speaking, "implement" does convey the right idea, so I am not
sure if we need to change it here and elsewhere to use "extend" instead,  though
these two words are a bit different in the Java land and we probably should
avoid confusing those two when explaining?

https://codereview.chromium.org/1307863006/diff/20001/components/cronet/READM...
components/cronet/README.md:80: In the above example, `MyListener` implements
the `UrlRequestListener`
Here, I also wonder whether we should s/implements/extends.

https://codereview.chromium.org/1307863006/diff/20001/components/cronet/READM...
components/cronet/README.md:106: In the above example, `MyUploadDataProvider`
implements the
And here.

https://codereview.chromium.org/1307863006/diff/20001/components/cronet/andro...
File components/cronet/android/java/src/org/chromium/net/UploadDataProvider.java
(right):

https://codereview.chromium.org/1307863006/diff/20001/components/cronet/andro...
components/cronet/android/java/src/org/chromium/net/UploadDataProvider.java:11:
* Interface allowing the embedder to provide an upload body to
nit: s/Interface/Abstract class

https://codereview.chromium.org/1307863006/diff/20001/components/cronet/andro...
components/cronet/android/java/src/org/chromium/net/UploadDataProvider.java:19:
public abstract class UploadDataProvider {
Although the guideline says we should *always* prefer abstract classes over
interfaces, I am not sure if we should change this one. I see why we might want
to change UrlRequestListener to an abstract class. But for UploadDataProvider,
if we do not know the form of underlying data (whether it's an array, buffer,
channel), it will be pretty hard to provide default states/implementations. Do
you have any use case in mind that we might need to augment this?

https://codereview.chromium.org/1307863006/diff/20001/components/cronet/andro...
File components/cronet/android/java/src/org/chromium/net/UrlRequestListener.java
(right):

https://codereview.chromium.org/1307863006/diff/20001/components/cronet/andro...
components/cronet/android/java/src/org/chromium/net/UrlRequestListener.java:10:
* Users of Cronet implement this class to receive callbacks indicating the
nit: s/implement/extend/

https://codereview.chromium.org/1307863006/diff/20001/components/cronet/andro...
components/cronet/android/java/src/org/chromium/net/UrlRequestListener.java:19:
public abstract class UrlRequestListener {
I was really confused by the guideline on the naming convention, "Use Listener
for single-method interfaces, Callback otherwise." A quick search on Android
APIs, I see all the *Listener named ones are interfaces,  so I guess in order to
conform to the guideline, we also need to change the name from
UrlRequestListener to UrlRequestCallback? (although I think it sounds a bit
weird.)

https://codereview.chromium.org/1307863006/diff/20001/components/cronet/andro...
File
components/cronet/android/java/src/org/chromium/net/urlconnection/CronetChunkedOutputStream.java
(right):

https://codereview.chromium.org/1307863006/diff/20001/components/cronet/andro...
components/cronet/android/java/src/org/chromium/net/urlconnection/CronetChunkedOutputStream.java:112:
public UploadDataProvider getUploadDataProvider() {
nit: can remove "public" modifier.

[xunjieli, 2015-09-03 04:35:11]

Also, could you link your design doc in the CL description? I think it provides
a bit more context and will benefit any future users of the code :)

[pauljensen, 2015-09-11 15:20:41]

https://codereview.chromium.org/1307863006/diff/20001/components/cronet/READM...
File components/cronet/README.md (right):

https://codereview.chromium.org/1307863006/diff/20001/components/cronet/READM...
components/cronet/README.md:11: First you will need to implement the
`UrlRequestListener` class to handle
On 2015/09/03 04:32:02, xunjieli wrote:
> I am not sure... Should we also use "extend" instead of "implement" in
> sentences? Loosely speaking, "implement" does convey the right idea, so I am
not
> sure if we need to change it here and elsewhere to use "extend" instead, 
though
> these two words are a bit different in the Java land and we probably should
> avoid confusing those two when explaining?

Ya, I think we should stick with "implement" in English while the class is
mostly abstract members.  Even if only a minority of the class members are
abstract, "extend" doesn't seem to imply the right action, more like "override";
but for now when it's 100% abstract members, I think "implement" is the best
description of the action embedders will take.

Now that I read it, saying "implement the class" doesn't sound right; embedders
will be implementing a subclass.  I've reworded. PTAL.

https://codereview.chromium.org/1307863006/diff/20001/components/cronet/READM...
components/cronet/README.md:80: In the above example, `MyListener` implements
the `UrlRequestListener`
On 2015/09/03 04:32:02, xunjieli wrote:
> Here, I also wonder whether we should s/implements/extends.

I feel the same way as on line 11.  I reworded like I did on line 11.

https://codereview.chromium.org/1307863006/diff/20001/components/cronet/READM...
components/cronet/README.md:106: In the above example, `MyUploadDataProvider`
implements the
On 2015/09/03 04:32:02, xunjieli wrote:
> And here.

I feel the same way as on line 11.  I reworded like I did on line 11.

https://codereview.chromium.org/1307863006/diff/20001/components/cronet/andro...
File components/cronet/android/java/src/org/chromium/net/UploadDataProvider.java
(right):

https://codereview.chromium.org/1307863006/diff/20001/components/cronet/andro...
components/cronet/android/java/src/org/chromium/net/UploadDataProvider.java:11:
* Interface allowing the embedder to provide an upload body to
On 2015/09/03 04:32:02, xunjieli wrote:
> nit: s/Interface/Abstract class

Done.

https://codereview.chromium.org/1307863006/diff/20001/components/cronet/andro...
components/cronet/android/java/src/org/chromium/net/UploadDataProvider.java:19:
public abstract class UploadDataProvider {
On 2015/09/03 04:32:02, xunjieli wrote:
> Although the guideline says we should *always* prefer abstract classes over
> interfaces, I am not sure if we should change this one. I see why we might
want
> to change UrlRequestListener to an abstract class. But for UploadDataProvider,
> if we do not know the form of underlying data (whether it's an array, buffer,
> channel), it will be pretty hard to provide default states/implementations. Do
> you have any use case in mind that we might need to augment this?
> 

I'm just trying to future-proof us.  I could image wacky and weird things being
necessary in the future like:
   boolean isIdempotent()
   boolean canGZip()
   boolean isHangingPost()

https://codereview.chromium.org/1307863006/diff/20001/components/cronet/andro...
File components/cronet/android/java/src/org/chromium/net/UrlRequestListener.java
(right):

https://codereview.chromium.org/1307863006/diff/20001/components/cronet/andro...
components/cronet/android/java/src/org/chromium/net/UrlRequestListener.java:10:
* Users of Cronet implement this class to receive callbacks indicating the
On 2015/09/03 04:32:02, xunjieli wrote:
> nit: s/implement/extend/

I changed it to "implement a subclass of this class".  WDYT?

https://codereview.chromium.org/1307863006/diff/20001/components/cronet/andro...
components/cronet/android/java/src/org/chromium/net/UrlRequestListener.java:19:
public abstract class UrlRequestListener {
On 2015/09/03 04:32:02, xunjieli wrote:
> I was really confused by the guideline on the naming convention, "Use Listener
> for single-method interfaces, Callback otherwise." A quick search on Android
> APIs, I see all the *Listener named ones are interfaces,  so I guess in order
to
> conform to the guideline, we also need to change the name from
> UrlRequestListener to UrlRequestCallback? (although I think it sounds a bit
> weird.)

Let's leave Android API guideline conformance for another CL.  I agree the
naming convention guideline seems odd.

https://codereview.chromium.org/1307863006/diff/20001/components/cronet/andro...
File
components/cronet/android/java/src/org/chromium/net/urlconnection/CronetChunkedOutputStream.java
(right):

https://codereview.chromium.org/1307863006/diff/20001/components/cronet/andro...
components/cronet/android/java/src/org/chromium/net/urlconnection/CronetChunkedOutputStream.java:112:
public UploadDataProvider getUploadDataProvider() {
On 2015/09/03 04:32:02, xunjieli wrote:
> nit: can remove "public" modifier.

Done.

[pauljensen, 2015-09-11 15:31:17]

On 2015/09/03 04:35:11, xunjieli wrote:
> Also, could you link your design doc in the CL description? I think it
provides
> a bit more context and will benefit any future users of the code :)

That document is Google-internal and includes some Google-internal terms and
email addresses.  I think I included all the relevant reasons/motivation for
this change in the description.

[xunjieli, 2015-09-11 16:26:35]

Should we also include Chris in the cc list so he's aware that this change is
going on?

https://codereview.chromium.org/1307863006/diff/20001/components/cronet/READM...
File components/cronet/README.md (right):

https://codereview.chromium.org/1307863006/diff/20001/components/cronet/READM...
components/cronet/README.md:11: First you will need to implement the
`UrlRequestListener` class to handle
On 2015/09/11 15:20:41, pauljensen wrote:
> On 2015/09/03 04:32:02, xunjieli wrote:
> > I am not sure... Should we also use "extend" instead of "implement" in
> > sentences? Loosely speaking, "implement" does convey the right idea, so I am
> not
> > sure if we need to change it here and elsewhere to use "extend" instead, 
> though
> > these two words are a bit different in the Java land and we probably should
> > avoid confusing those two when explaining?
> 
> Ya, I think we should stick with "implement" in English while the class is
> mostly abstract members.  Even if only a minority of the class members are
> abstract, "extend" doesn't seem to imply the right action, more like
"override";
> but for now when it's 100% abstract members, I think "implement" is the best
> description of the action embedders will take.
> 
> Now that I read it, saying "implement the class" doesn't sound right;
embedders
> will be implementing a subclass.  I've reworded. PTAL.

Acknowledged. SGTM

https://codereview.chromium.org/1307863006/diff/20001/components/cronet/andro...
File components/cronet/android/java/src/org/chromium/net/UrlRequestListener.java
(right):

https://codereview.chromium.org/1307863006/diff/20001/components/cronet/andro...
components/cronet/android/java/src/org/chromium/net/UrlRequestListener.java:10:
* Users of Cronet implement this class to receive callbacks indicating the
On 2015/09/11 15:20:41, pauljensen wrote:
> On 2015/09/03 04:32:02, xunjieli wrote:
> > nit: s/implement/extend/
> 
> I changed it to "implement a subclass of this class".  WDYT?

I personally prefer "extend" here. It is somewhat ingrained in me that I will
think about interfaces when I see "implement" :(

https://codereview.chromium.org/1307863006/diff/20001/components/cronet/andro...
components/cronet/android/java/src/org/chromium/net/UrlRequestListener.java:19:
public abstract class UrlRequestListener {
On 2015/09/11 15:20:41, pauljensen wrote:
> On 2015/09/03 04:32:02, xunjieli wrote:
> > I was really confused by the guideline on the naming convention, "Use
Listener
> > for single-method interfaces, Callback otherwise." A quick search on Android
> > APIs, I see all the *Listener named ones are interfaces,  so I guess in
order
> to
> > conform to the guideline, we also need to change the name from
> > UrlRequestListener to UrlRequestCallback? (although I think it sounds a bit
> > weird.)
> 
> Let's leave Android API guideline conformance for another CL.  I agree the
> naming convention guideline seems odd.

Acknowledged.

https://codereview.chromium.org/1307863006/diff/40001/components/cronet/READM...
File components/cronet/README.md (right):

https://codereview.chromium.org/1307863006/diff/40001/components/cronet/READM...
components/cronet/README.md:94: valid for the duration of the
`UrlRequestListener.onReadCompleted` callback.
Maybe we should delete "The data in `byteBuffer` is only guaranteed to be valid
for the duration of the `UrlRequestListener.onReadCompleted` callback." too,
since you are already here. But I am fine if you want to keep things separate as
well.

[pauljensen, 2015-09-11 16:31:33]

On 2015/09/11 16:26:35, xunjieli wrote:
> Should we also include Chris in the cc list so he's aware that this change is
> going on?

cbentzel?  I think he's already CC'd via a watch-list he has.  I'm not sure he'd
be too interested in this though.

[xunjieli, 2015-09-11 16:35:33]

Sorry, I meant cpovirk@. He wrote a new outline assuming that our classes
are interfaces. https://paste.googleplex.com/5979637112897536. Should we
inform him that we are changing them to abstract classes?

On Fri, Sep 11, 2015 at 12:31 PM, <pauljensen@chromium.org> wrote:

> On 2015/09/11 16:26:35, xunjieli wrote:
>
>> Should we also include Chris in the cc list so he's aware that this
>> change is
>> going on?
>>
>
> cbentzel?  I think he's already CC'd via a watch-list he has.  I'm not
> sure he'd
> be too interested in this though.
>
>
>
> https://codereview.chromium.org/1307863006/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[pauljensen, 2015-09-11 16:48:38]

On 2015/09/11 16:35:33, xunjieli wrote:
> Sorry, I meant cpovirk@. He wrote a new outline assuming that our classes
> are interfaces. https://paste.googleplex.com/5979637112897536. Should we
> inform him that we are changing them to abstract classes?

His proposal relates to Cronet's implementation, not the embedder's
implementation, which is somewhat orthogonal with different goals I believe. 
I'll share my API change proposal doc with them.

[pauljensen, 2015-09-11 18:54:40]

https://codereview.chromium.org/1307863006/diff/20001/components/cronet/andro...
File components/cronet/android/java/src/org/chromium/net/UrlRequestListener.java
(right):

https://codereview.chromium.org/1307863006/diff/20001/components/cronet/andro...
components/cronet/android/java/src/org/chromium/net/UrlRequestListener.java:10:
* Users of Cronet implement this class to receive callbacks indicating the
On 2015/09/11 16:26:35, xunjieli wrote:
> On 2015/09/11 15:20:41, pauljensen wrote:
> > On 2015/09/03 04:32:02, xunjieli wrote:
> > > nit: s/implement/extend/
> > 
> > I changed it to "implement a subclass of this class".  WDYT?
> 
> I personally prefer "extend" here.

I think the English definition of "extend" doesn't really apply.  "Extend" in
the context of a class, to me at least, connotes adding methods, which isn't
required of embedders.  Embedders must only implement/override abstract methods.
 

> It is somewhat ingrained in me that I will
> think about interfaces when I see "implement" :(

Our class is really an interface, in the English or programming sense, just not
in Java.  I think the abstract vs implementation differentiation is key here and
that's why I feel "implement a subclass of" is the right way to explain it.

There may not be a clear way to word this correctly or optimally.  Perhaps we
should rope in another opinion?  Matt or Misha?

https://codereview.chromium.org/1307863006/diff/40001/components/cronet/READM...
File components/cronet/README.md (right):

https://codereview.chromium.org/1307863006/diff/40001/components/cronet/READM...
components/cronet/README.md:94: valid for the duration of the
`UrlRequestListener.onReadCompleted` callback.
On 2015/09/11 16:26:35, xunjieli wrote:
> Maybe we should delete "The data in `byteBuffer` is only guaranteed to be
valid
> for the duration of the `UrlRequestListener.onReadCompleted` callback." too,
> since you are already here. But I am fine if you want to keep things separate
as
> well.

I assume this sentence is left over from before we changed to a pull interface.
I think the prior sentence is also misleading.  I've deleted the second and
reworded the first.

[xunjieli, 2015-09-11 19:01:42]

https://codereview.chromium.org/1307863006/diff/20001/components/cronet/andro...
File components/cronet/android/java/src/org/chromium/net/UrlRequestListener.java
(right):

https://codereview.chromium.org/1307863006/diff/20001/components/cronet/andro...
components/cronet/android/java/src/org/chromium/net/UrlRequestListener.java:10:
* Users of Cronet implement this class to receive callbacks indicating the
On 2015/09/11 18:54:40, pauljensen wrote:
> On 2015/09/11 16:26:35, xunjieli wrote:
> > On 2015/09/11 15:20:41, pauljensen wrote:
> > > On 2015/09/03 04:32:02, xunjieli wrote:
> > > > nit: s/implement/extend/
> > > 
> > > I changed it to "implement a subclass of this class".  WDYT?
> > 
> > I personally prefer "extend" here.
> 
> I think the English definition of "extend" doesn't really apply.  "Extend" in
> the context of a class, to me at least, connotes adding methods, which isn't
> required of embedders.  Embedders must only implement/override abstract
methods.
>  
I thought the goal of changing interface to abstract class is that it'd be
easier to add concrete methods. If we have concrete methods and variable
definitions, I think it makes sense both in the English and Java to say "extend"
here. But I think we should get a second opinion.
> 
> > It is somewhat ingrained in me that I will
> > think about interfaces when I see "implement" :(
> 
> Our class is really an interface, in the English or programming sense, just
not
> in Java.  I think the abstract vs implementation differentiation is key here
and
> that's why I feel "implement a subclass of" is the right way to explain it.
> 
> There may not be a clear way to word this correctly or optimally.  Perhaps we
> should rope in another opinion?  Matt or Misha?  
Doesn't hurt to loop in both :)

https://codereview.chromium.org/1307863006/diff/40001/components/cronet/READM...
File components/cronet/README.md (right):

https://codereview.chromium.org/1307863006/diff/40001/components/cronet/READM...
components/cronet/README.md:94: valid for the duration of the
`UrlRequestListener.onReadCompleted` callback.
On 2015/09/11 18:54:40, pauljensen wrote:
> On 2015/09/11 16:26:35, xunjieli wrote:
> > Maybe we should delete "The data in `byteBuffer` is only guaranteed to be
> valid
> > for the duration of the `UrlRequestListener.onReadCompleted` callback." too,
> > since you are already here. But I am fine if you want to keep things
separate
> as
> > well.
> 
> I assume this sentence is left over from before we changed to a pull
interface.
> I think the prior sentence is also misleading.  I've deleted the second and
> reworded the first.

Acknowledged.

[pauljensen, 2015-09-11 19:16:11]

https://codereview.chromium.org/1307863006/diff/20001/components/cronet/andro...
File components/cronet/android/java/src/org/chromium/net/UrlRequestListener.java
(right):

https://codereview.chromium.org/1307863006/diff/20001/components/cronet/andro...
components/cronet/android/java/src/org/chromium/net/UrlRequestListener.java:10:
* Users of Cronet implement this class to receive callbacks indicating the
On 2015/09/11 19:01:42, xunjieli wrote:
> On 2015/09/11 18:54:40, pauljensen wrote:
> > On 2015/09/11 16:26:35, xunjieli wrote:
> > > On 2015/09/11 15:20:41, pauljensen wrote:
> > > > On 2015/09/03 04:32:02, xunjieli wrote:
> > > > > nit: s/implement/extend/
> > > > 
> > > > I changed it to "implement a subclass of this class".  WDYT?
> > > 
> > > I personally prefer "extend" here.
> > 
> > I think the English definition of "extend" doesn't really apply.  "Extend"
in
> > the context of a class, to me at least, connotes adding methods, which isn't
> > required of embedders.  Embedders must only implement/override abstract
> methods.
> >  
> I thought the goal of changing interface to abstract class is that it'd be
> easier to add concrete methods. If we have concrete methods and variable
> definitions, I think it makes sense both in the English and Java to say
"extend"
> here. But I think we should get a second opinion.
> > 
> > > It is somewhat ingrained in me that I will
> > > think about interfaces when I see "implement" :(
> > 
> > Our class is really an interface, in the English or programming sense, just
> not
> > in Java.  I think the abstract vs implementation differentiation is key here
> and
> > that's why I feel "implement a subclass of" is the right way to explain it.
> > 
> > There may not be a clear way to word this correctly or optimally.  Perhaps
we
> > should rope in another opinion?  Matt or Misha?  
> Doesn't hurt to loop in both :)
> 

After typing it up, your proposal sounds better frankly.  "implement a subclass
of this class" sounds awful compared to simply "extend this class".  I've
changed it to your suggestion.  I also changed README.md similarly.  PTAL.

[xunjieli, 2015-09-11 19:32:23]

LGTM! thanks

[pauljensen, 2015-09-14 11:10:24]

The CQ bit was checked by pauljensen@chromium.org

[commit-bot: I haz the power, 2015-09-14 11:10:37]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1307863006/80001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1307863006/80001

[commit-bot: I haz the power, 2015-09-14 11:18:55]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-09-14 11:18:55]

Try jobs failed on following builders:
  android_clang_dbg_recipe on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/android_clang_d...)

[pauljensen, 2015-09-14 11:46:35]

The CQ bit was checked by pauljensen@chromium.org

[pauljensen, 2015-09-14 11:46:35]

The patchset sent to the CQ was uploaded after l-g-t-m from
xunjieli@chromium.org
Link to the patchset: https://codereview.chromium.org/1307863006/#ps100001
(title: "update CronetSampleActivity too")

[commit-bot: I haz the power, 2015-09-14 11:46:42]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1307863006/100001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1307863006/100001

[commit-bot: I haz the power, 2015-09-14 11:54:11]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-09-14 11:54:11]

Try jobs failed on following builders:
  android_clang_dbg_recipe on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/android_clang_d...)

[pauljensen, 2015-09-14 12:11:56]

The CQ bit was checked by pauljensen@chromium.org

[pauljensen, 2015-09-14 12:11:56]

The patchset sent to the CQ was uploaded after l-g-t-m from
xunjieli@chromium.org
Link to the patchset: https://codereview.chromium.org/1307863006/#ps120001
(title: "update CronetPerfTestActivity too")

[commit-bot: I haz the power, 2015-09-14 12:12:04]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1307863006/120001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1307863006/120001

[commit-bot: I haz the power, 2015-09-14 12:57:23]

Committed patchset #7 (id:120001)

[commit-bot: I haz the power, 2015-09-14 12:58:12]

Patchset 7 (id:??) landed as
https://crrev.com/d4ac94937db989b40acd8f6fa51465d6174f608f
Cr-Commit-Position: refs/heads/master@{#348605}

[commit-bot: I haz the power, 2015-09-23 12:31:49]

Patchset 7 (id:??) landed as
https://crrev.com/d4ac94937db989b40acd8f6fa51465d6174f608f
Cr-Commit-Position: refs/heads/master@{#348605}