Move DownloadControllerAndroid from content/ to chrome/

Move DownloadControllerAndroid from content/ to chrome/

This change lets embedder to decide how to handle downloads,
and makes content layer more self-contained. Accompanying changes are:

- Now WebContents holds the reference to download delegate via WebContentsUserData.
- ContentViewDownloadDelegate interface was removed
- DownloadControlerAndroid -> DownloadControllerBase
- DownloadControlerAndroidImpl -> DownloadController to
  have the name matched with the Java class

BUG=598880, 617313, 617361
Committed: https://crrev.com/3da95ee0a9da46764cc912e3b36f29dd171eab40
Cr-Commit-Position: refs/heads/master@{#401127}

[Jinsuk Kim, 2016-05-26 07:21:47]

Description was changed from

==========
Let WebContents handle DownloadDelegate

ContentViewCore was used for a medium to keep the reference to
DownloadDelegate instance for DownloadController to invoke
the delegate. Moved it to WebContents.

BUG=598880
==========

to

==========
Let WebContents handle DownloadDelegate

ContentViewCore was used simply as a holder of the reference to                 
                                                                                
                                                                                
                                                                            
DownloadDelegate instance for DownloadController to invoke                      
                                                                                
                                                                                
                                                                            
the delegate later. Had WebContents take the role instead.         
BUG=598880
==========

[Jinsuk Kim, 2016-05-26 07:21:56]

Description was changed from

==========
Let WebContents handle DownloadDelegate

ContentViewCore was used simply as a holder of the reference to                 
                                                                                
                                                                                
                                                                            
DownloadDelegate instance for DownloadController to invoke                      
                                                                                
                                                                                
                                                                            
the delegate later. Had WebContents take the role instead.         
BUG=598880
==========

to

==========
Let WebContents handle DownloadDelegate

ContentViewCore was used simply as a holder of the reference to                 
                                                                                
                                                                                
                                                                            
DownloadDelegate instance for DownloadController to invoke                      
                                                                                
                                                                                
                                                                            
the delegate later. Had WebContents take the role instead.
         
BUG=598880
==========

[Jinsuk Kim, 2016-05-27 02:15:46]

Patchset #3 (id:40001) has been deleted

[Jinsuk Kim, 2016-05-27 03:59:12]

Patchset #3 (id:60001) has been deleted

[Jinsuk Kim, 2016-05-27 04:56:32]

jinsukkim@chromium.org changed reviewers:
+ dtrainor@chromium.org, sievers@chromium.org

[Jinsuk Kim, 2016-05-27 04:56:33]

For reviewers:
- Moving DownloadDelegate-related methods from ContentViewCore to WebContents,
which required ContentViewDownloadDelegate/DownloadInfo be moved as well.
- Main changes are in Tab.java/DownloadController.java/download_controller.*.
All the other classes have trivial changes in the import stmts.

[no sievers, 2016-05-27 19:08:55]

sievers@chromium.org changed reviewers:
- dtrainor@chromium.org

[no sievers, 2016-05-27 19:09:26]

https://codereview.chromium.org/2014803002/diff/100001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/download/ChromeDownloadDelegate.java
(right):

https://codereview.chromium.org/2014803002/diff/100001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/download/ChromeDownloadDelegate.java:35:
import org.chromium.content.browser.DownloadController;
So DownloadController is not public here. So it either shouldn't use it, or it
should be public. We can clean that up separately though. The native
DownloadController is public and has a few call sites from outside content, so
maybe this should be public too.
It all seems really spaghetti'ish though. It'd be nice to revisit what the
cleanest and most minimal API boundaries could be between public and private
content layer and Java and native.

https://codereview.chromium.org/2014803002/diff/100001/content/browser/androi...
File content/browser/android/download_controller_android_impl.cc (left):

https://codereview.chromium.org/2014803002/diff/100001/content/browser/androi...
content/browser/android/download_controller_android_impl.cc:364: //
ContentViewCore is created.
That would be really neat if we can get rid of this logic.

But if we can, then you can delete more code related to |deferred_downloads_| in
here and the whole DeferredDownLoadObserver class.

https://codereview.chromium.org/2014803002/diff/100001/content/browser/androi...
File content/browser/android/download_controller_android_impl.h (right):

https://codereview.chromium.org/2014803002/diff/100001/content/browser/androi...
content/browser/android/download_controller_android_impl.h:145:
base::android::ScopedJavaLocalRef<jobject> GetJavWebContents(
typo... but actually it's unused

https://codereview.chromium.org/2014803002/diff/100001/content/public/android...
File
content/public/android/java/src/org/chromium/content/browser/DownloadController.java
(right):

https://codereview.chromium.org/2014803002/diff/100001/content/public/android...
content/public/android/java/src/org/chromium/content/browser/DownloadController.java:217:
ContentViewDownloadDelegate downloadDelegate =
downloadDelegateFromWebContents(webContents);
This shim layer in DownloadController to forward callbacks out to the delegate
from native still seems a bit convoluted and unnecessary.
Would it be easier to just forward that from native to the delegate?

And why is the delegate not a native interface? Wouldn't that make things
simpler?

Same for the static notifications that are routed through here. Then this Java
class can really go away.

[Jinsuk Kim, 2016-05-30 00:52:00]

jinsukkim@chromium.org changed reviewers:
+ qinmin@chromium.org

[Jinsuk Kim, 2016-05-30 01:20:57]

Thanks for the review. As I replied inline, I'd like to work on your suggestion
in a follow-up CL as the refactoring the flow for DownloadController itself
would be a big change that needs discussion with the owner of the class.

https://codereview.chromium.org/2014803002/diff/100001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/download/ChromeDownloadDelegate.java
(right):

https://codereview.chromium.org/2014803002/diff/100001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/download/ChromeDownloadDelegate.java:35:
import org.chromium.content.browser.DownloadController;
On 2016/05/27 19:09:25, sievers wrote:
> So DownloadController is not public here. So it either shouldn't use it, or it
> should be public. We can clean that up separately though. The native
> DownloadController is public and has a few call sites from outside content, so
> maybe this should be public too.
> It all seems really spaghetti'ish though. It'd be nice to revisit what the
> cleanest and most minimal API boundaries could be between public and private
> content layer and Java and native.

Let me make DownloadController public in a follow-up CL after a discussion with
the owner.

https://codereview.chromium.org/2014803002/diff/100001/content/browser/androi...
File content/browser/android/download_controller_android_impl.cc (left):

https://codereview.chromium.org/2014803002/diff/100001/content/browser/androi...
content/browser/android/download_controller_android_impl.cc:364: //
ContentViewCore is created.
On 2016/05/27 19:09:25, sievers wrote:
> That would be really neat if we can get rid of this logic.
> 
> But if we can, then you can delete more code related to |deferred_downloads_|
in
> here and the whole DeferredDownLoadObserver class.

I don't think we need it any more once the dependency on ContentViewCore is
gone. I can see a TODO in DeferredDownloadObserver left by him about removing
the class. I'd like to hear what Min thinks about this.

https://codereview.chromium.org/2014803002/diff/100001/content/browser/androi...
File content/browser/android/download_controller_android_impl.h (right):

https://codereview.chromium.org/2014803002/diff/100001/content/browser/androi...
content/browser/android/download_controller_android_impl.h:145:
base::android::ScopedJavaLocalRef<jobject> GetJavWebContents(
On 2016/05/27 19:09:25, sievers wrote:
> typo... but actually it's unused

Removed.

https://codereview.chromium.org/2014803002/diff/100001/content/public/android...
File
content/public/android/java/src/org/chromium/content/browser/DownloadController.java
(right):

https://codereview.chromium.org/2014803002/diff/100001/content/public/android...
content/public/android/java/src/org/chromium/content/browser/DownloadController.java:217:
ContentViewDownloadDelegate downloadDelegate =
downloadDelegateFromWebContents(webContents);
On 2016/05/27 19:09:26, sievers wrote:
> This shim layer in DownloadController to forward callbacks out to the delegate
> from native still seems a bit convoluted and unnecessary.
> Would it be easier to just forward that from native to the delegate?
> 
> And why is the delegate not a native interface? Wouldn't that make things
> simpler?
> 
> Same for the static notifications that are routed through here. Then this Java
> class can really go away.

That will indeed make the flow simpler. I'd like to split the work on it into a
new CL and keep this one for removing the dependency on ContentViewCore.

[Jinsuk Kim, 2016-05-31 22:40:15]

https://codereview.chromium.org/2014803002/diff/100001/content/public/android...
File
content/public/android/java/src/org/chromium/content/browser/DownloadController.java
(right):

https://codereview.chromium.org/2014803002/diff/100001/content/public/android...
content/public/android/java/src/org/chromium/content/browser/DownloadController.java:217:
ContentViewDownloadDelegate downloadDelegate =
downloadDelegateFromWebContents(webContents);
On 2016/05/30 01:20:57, Jinsuk wrote:
> On 2016/05/27 19:09:26, sievers wrote:
> > This shim layer in DownloadController to forward callbacks out to the
delegate
> > from native still seems a bit convoluted and unnecessary.
> > Would it be easier to just forward that from native to the delegate?
> > 
> > And why is the delegate not a native interface? Wouldn't that make things
> > simpler?
> > 
> > Same for the static notifications that are routed through here. Then this
Java
> > class can really go away.
> 
> That will indeed make the flow simpler. I'd like to split the work on it into
a
> new CL and keep this one for removing the dependency on ContentViewCore.

One question - having a native download delegate interface can let the native
download controller call native download delegate -> java download delegate,
rather than routing through native download controller -> java download
controller -> download delegate like it is done now. Then we don't pass java
layer download delegate but need to create a native and java delegate from
native side . Is this a common way to make use of content module from chrome? So
far I see Java layer create its native counterpart instance in most cases (via
nativeInit), not the other way around. Or is there other way to have your
suggestion work?

[no sievers, 2016-06-02 00:44:48]

On 2016/05/31 22:40:15, Jinsuk wrote:
>
https://codereview.chromium.org/2014803002/diff/100001/content/public/android...
> File
>
content/public/android/java/src/org/chromium/content/browser/DownloadController.java
> (right):
> 
>
https://codereview.chromium.org/2014803002/diff/100001/content/public/android...
>
content/public/android/java/src/org/chromium/content/browser/DownloadController.java:217:
> ContentViewDownloadDelegate downloadDelegate =
> downloadDelegateFromWebContents(webContents);
> On 2016/05/30 01:20:57, Jinsuk wrote:
> > On 2016/05/27 19:09:26, sievers wrote:
> > > This shim layer in DownloadController to forward callbacks out to the
> delegate
> > > from native still seems a bit convoluted and unnecessary.
> > > Would it be easier to just forward that from native to the delegate?
> > > 
> > > And why is the delegate not a native interface? Wouldn't that make things
> > > simpler?
> > > 
> > > Same for the static notifications that are routed through here. Then this
> Java
> > > class can really go away.
> > 
> > That will indeed make the flow simpler. I'd like to split the work on it
into
> a
> > new CL and keep this one for removing the dependency on ContentViewCore.
> 
> One question - having a native download delegate interface can let the native
> download controller call native download delegate -> java download delegate,
> rather than routing through native download controller -> java download
> controller -> download delegate like it is done now. Then we don't pass java
> layer download delegate but need to create a native and java delegate from
> native side . Is this a common way to make use of content module from chrome?
So
> far I see Java layer create its native counterpart instance in most cases (via
> nativeInit), not the other way around. Or is there other way to have your
> suggestion work?

How about just having tab_android.cc implement the
content/public/browser/android_download_delegate.h interface?
There are only 4 methods and if we wanted to keep the code as is otherwise, it
could just forward to the implementation in ChromeDownloadDelegate in java.

[no sievers, 2016-06-02 00:45:17]

On 2016/06/02 00:44:48, sievers wrote:
> On 2016/05/31 22:40:15, Jinsuk wrote:
> >
>
https://codereview.chromium.org/2014803002/diff/100001/content/public/android...
> > File
> >
>
content/public/android/java/src/org/chromium/content/browser/DownloadController.java
> > (right):
> > 
> >
>
https://codereview.chromium.org/2014803002/diff/100001/content/public/android...
> >
>
content/public/android/java/src/org/chromium/content/browser/DownloadController.java:217:
> > ContentViewDownloadDelegate downloadDelegate =
> > downloadDelegateFromWebContents(webContents);
> > On 2016/05/30 01:20:57, Jinsuk wrote:
> > > On 2016/05/27 19:09:26, sievers wrote:
> > > > This shim layer in DownloadController to forward callbacks out to the
> > delegate
> > > > from native still seems a bit convoluted and unnecessary.
> > > > Would it be easier to just forward that from native to the delegate?
> > > > 
> > > > And why is the delegate not a native interface? Wouldn't that make
things
> > > > simpler?
> > > > 
> > > > Same for the static notifications that are routed through here. Then
this
> > Java
> > > > class can really go away.
> > > 
> > > That will indeed make the flow simpler. I'd like to split the work on it
> into
> > a
> > > new CL and keep this one for removing the dependency on ContentViewCore.
> > 
> > One question - having a native download delegate interface can let the
native
> > download controller call native download delegate -> java download delegate,
> > rather than routing through native download controller -> java download
> > controller -> download delegate like it is done now. Then we don't pass java
> > layer download delegate but need to create a native and java delegate from
> > native side . Is this a common way to make use of content module from
chrome?
> So
> > far I see Java layer create its native counterpart instance in most cases
(via
> > nativeInit), not the other way around. Or is there other way to have your
> > suggestion work?
> 
> How about just having tab_android.cc implement the
> content/public/browser/android_download_delegate.h

content/public/browser/android/download_delegate.h
 
interface?
> There are only 4 methods and if we wanted to keep the code as is otherwise, it
> could just forward to the implementation in ChromeDownloadDelegate in java.

[Jinsuk Kim, 2016-06-02 08:46:31]

PTAL. Never mind some rough edges (extra header inclusion, wrong style, etc)
that I will clean up soon. 

Ended up using ChromeDownloadDelegate native instead of having tab_android
implementing download delegate since it is already there.

[no sievers, 2016-06-02 23:29:06]

That works generally I think.

Normally functionality belonging to a WebContents gets added through
WebContents::SetUserData() which then also takes ownership of the class. It
avoids having to add a getter/setter for everything in web_contents.h.
Maybe we could do that here too... just have to make sure the Tab unsets it, see
my other comments regarding circular dependencies.

https://codereview.chromium.org/2014803002/diff/140001/chrome/browser/android...
File chrome/browser/android/download/chrome_download_delegate.h (right):

https://codereview.chromium.org/2014803002/diff/140001/chrome/browser/android...
chrome/browser/android/download/chrome_download_delegate.h:40:
base::android::ScopedJavaGlobalRef<jobject> obj_;
If the native instance holds a global ref, we have to make sure that the native
instance gets deleted when the Tab tears down, because DownloadController(.java)
holds a back reference to the Tab. Or you might end up with a circular
dependency.

https://codereview.chromium.org/2014803002/diff/140001/chrome/browser/android...
File chrome/browser/android/tab_android.cc (right):

https://codereview.chromium.org/2014803002/diff/140001/chrome/browser/android...
chrome/browser/android/tab_android.cc:410: new
ChromeDownloadDelegate(jdownload_delegate));
Who takes ownership of the new ChromeDownloadDelegate i.e. when does it get
deleted?

[qinmin, 2016-06-03 00:17:25]

On 2016/06/02 23:29:06, sievers wrote:
> That works generally I think.
> 
> Normally functionality belonging to a WebContents gets added through
> WebContents::SetUserData() which then also takes ownership of the class. It
> avoids having to add a getter/setter for everything in web_contents.h.
> Maybe we could do that here too... just have to make sure the Tab unsets it,
see
> my other comments regarding circular dependencies.
> 
>
https://codereview.chromium.org/2014803002/diff/140001/chrome/browser/android...
> File chrome/browser/android/download/chrome_download_delegate.h (right):
> 
>
https://codereview.chromium.org/2014803002/diff/140001/chrome/browser/android...
> chrome/browser/android/download/chrome_download_delegate.h:40:
> base::android::ScopedJavaGlobalRef<jobject> obj_;
> If the native instance holds a global ref, we have to make sure that the
native
> instance gets deleted when the Tab tears down, because
DownloadController(.java)
> holds a back reference to the Tab. Or you might end up with a circular
> dependency.
> 
>
https://codereview.chromium.org/2014803002/diff/140001/chrome/browser/android...
> File chrome/browser/android/tab_android.cc (right):
> 
>
https://codereview.chromium.org/2014803002/diff/140001/chrome/browser/android...
> chrome/browser/android/tab_android.cc:410: new
> ChromeDownloadDelegate(jdownload_delegate));
> Who takes ownership of the new ChromeDownloadDelegate i.e. when does it get
> deleted?

The network stack team is trying to remove  the binding between webcontents and
download. Download should be related to profiles, rather than webContents. 
If a tab is closed, that shouldn't impact download.
We previously use ContetnViewCore is because we want to show infobars. But we
don't really need them after the infobar is shown.

[Jinsuk Kim, 2016-06-03 01:12:42]

On 2016/06/03 00:17:25, qinmin wrote:
> On 2016/06/02 23:29:06, sievers wrote:
> > That works generally I think.
> > 
> > Normally functionality belonging to a WebContents gets added through
> > WebContents::SetUserData() which then also takes ownership of the class. It
> > avoids having to add a getter/setter for everything in web_contents.h.
> > Maybe we could do that here too... just have to make sure the Tab unsets it,
> see
> > my other comments regarding circular dependencies.
> > 
> >
>
https://codereview.chromium.org/2014803002/diff/140001/chrome/browser/android...
> > File chrome/browser/android/download/chrome_download_delegate.h (right):
> > 
> >
>
https://codereview.chromium.org/2014803002/diff/140001/chrome/browser/android...
> > chrome/browser/android/download/chrome_download_delegate.h:40:
> > base::android::ScopedJavaGlobalRef<jobject> obj_;
> > If the native instance holds a global ref, we have to make sure that the
> native
> > instance gets deleted when the Tab tears down, because
> DownloadController(.java)
> > holds a back reference to the Tab. Or you might end up with a circular
> > dependency.
> > 
> >
>
https://codereview.chromium.org/2014803002/diff/140001/chrome/browser/android...
> > File chrome/browser/android/tab_android.cc (right):
> > 
> >
>
https://codereview.chromium.org/2014803002/diff/140001/chrome/browser/android...
> > chrome/browser/android/tab_android.cc:410: new
> > ChromeDownloadDelegate(jdownload_delegate));
> > Who takes ownership of the new ChromeDownloadDelegate i.e. when does it get
> > deleted?
> 

Let me sort out the dependency and the lifecycle of the delegate.

> The network stack team is trying to remove  the binding between webcontents
and
> download. Download should be related to profiles, rather than webContents. 
> If a tab is closed, that shouldn't impact download.
> We previously use ContetnViewCore is because we want to show infobars. But we
> don't really need them after the infobar is shown.

Then how about we keep the delegate in the 'native' side of content view core
which
I believe will remain as the impl of ViewAndroid after all the refactoring.

WDYT daniel?

[Jinsuk Kim, 2016-06-03 11:04:15]

Moved the delegate interface to the native ContentViewCore. I admit this is less
than ideal but still helps slim down java CVC and make the download flow
simpler.

Given that WebContents will be out of the picture, I'm thinking that it may work
to keep the delegate closer to rvh, with a set of mapping from delegate to Tab
(or its infobar container) managed somewhere in the browser process. Maybe in a
follow-up CL.

https://codereview.chromium.org/2014803002/diff/140001/chrome/browser/android...
File chrome/browser/android/download/chrome_download_delegate.h (right):

https://codereview.chromium.org/2014803002/diff/140001/chrome/browser/android...
chrome/browser/android/download/chrome_download_delegate.h:40:
base::android::ScopedJavaGlobalRef<jobject> obj_;
On 2016/06/02 23:29:06, sievers wrote:
> If the native instance holds a global ref, we have to make sure that the
native
> instance gets deleted when the Tab tears down, because
DownloadController(.java)
> holds a back reference to the Tab. Or you might end up with a circular
> dependency.

Replaced with weak reference to it so that ChromeDownloadDelegate (java) can get
deleted when Tab loads a new WebContents/ContentViewCore.

https://codereview.chromium.org/2014803002/diff/140001/chrome/browser/android...
File chrome/browser/android/tab_android.cc (right):

https://codereview.chromium.org/2014803002/diff/140001/chrome/browser/android...
chrome/browser/android/tab_android.cc:410: new
ChromeDownloadDelegate(jdownload_delegate));
On 2016/06/02 23:29:06, sievers wrote:
> Who takes ownership of the new ChromeDownloadDelegate i.e. when does it get
> deleted?

Now ContentViewCoreImpl owns it and has the same lifecycle as
WebContents/ContentViewCore.

[no sievers, 2016-06-03 19:17:11]

On 2016/06/03 01:12:42, Jinsuk wrote:
> On 2016/06/03 00:17:25, qinmin wrote:
> > On 2016/06/02 23:29:06, sievers wrote:
> > > That works generally I think.
> > > 
> > > Normally functionality belonging to a WebContents gets added through
> > > WebContents::SetUserData() which then also takes ownership of the class.
It
> > > avoids having to add a getter/setter for everything in web_contents.h.
> > > Maybe we could do that here too... just have to make sure the Tab unsets
it,
> > see
> > > my other comments regarding circular dependencies.
> > > 
> > >
> >
>
https://codereview.chromium.org/2014803002/diff/140001/chrome/browser/android...
> > > File chrome/browser/android/download/chrome_download_delegate.h (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/2014803002/diff/140001/chrome/browser/android...
> > > chrome/browser/android/download/chrome_download_delegate.h:40:
> > > base::android::ScopedJavaGlobalRef<jobject> obj_;
> > > If the native instance holds a global ref, we have to make sure that the
> > native
> > > instance gets deleted when the Tab tears down, because
> > DownloadController(.java)
> > > holds a back reference to the Tab. Or you might end up with a circular
> > > dependency.
> > > 
> > >
> >
>
https://codereview.chromium.org/2014803002/diff/140001/chrome/browser/android...
> > > File chrome/browser/android/tab_android.cc (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/2014803002/diff/140001/chrome/browser/android...
> > > chrome/browser/android/tab_android.cc:410: new
> > > ChromeDownloadDelegate(jdownload_delegate));
> > > Who takes ownership of the new ChromeDownloadDelegate i.e. when does it
get
> > > deleted?
> > 
> 
> Let me sort out the dependency and the lifecycle of the delegate.
> 
> > The network stack team is trying to remove  the binding between webcontents
> and
> > download. Download should be related to profiles, rather than webContents. 
> > If a tab is closed, that shouldn't impact download.
> > We previously use ContetnViewCore is because we want to show infobars. But
we
> > don't really need them after the infobar is shown.
> 
> Then how about we keep the delegate in the 'native' side of content view core
> which
> I believe will remain as the impl of ViewAndroid after all the refactoring.
> 
> WDYT daniel?

That would also go away.

ContentViewCore is really mostly something like a WebContentsViewAndroid (i.e.
the View- and platform-specific parts of the WebContents).
Delegating up to the embedder through WebContents(Delegate) or other related
interfaces is the common pattern for this.

There is also a dependency injection here for the embedder to inject its
implementation.
Often this sort of feature extension is done as a WebContents::UserData where we
want the WebContents to take ownership of the object since its lifetime is tied
to the WebContents.

Min said:
> The network stack team is trying to remove  the binding between webcontents
and
> download. Download should be related to profiles, rather than webContents. 
> If a tab is closed, that shouldn't impact download.
> We previously use ContetnViewCore is because we want to show infobars. But we
> don't really need them after the infobar is shown.

Yea, I noticed that there already is a global 'DownloadNotificationService'
which is not tied to the tab and then the UI-related logic which is tied to the
current WebContents container. So that didn't look totally unreasonable and we
were mostly trying to simplify JNI flows (and potentially object lifetime) and
separate better the public API vs. private implementation.

Doesn't it still make sense to associate the delegate interface with a
WebContents container since the notification are specific to that WebContents
instance? How would the app know in which tab to show the infobar?

[qinmin, 2016-06-03 19:23:27]

On 2016/06/03 19:17:11, sievers wrote:
> On 2016/06/03 01:12:42, Jinsuk wrote:
> > On 2016/06/03 00:17:25, qinmin wrote:
> > > On 2016/06/02 23:29:06, sievers wrote:
> > > > That works generally I think.
> > > > 
> > > > Normally functionality belonging to a WebContents gets added through
> > > > WebContents::SetUserData() which then also takes ownership of the class.
> It
> > > > avoids having to add a getter/setter for everything in web_contents.h.
> > > > Maybe we could do that here too... just have to make sure the Tab unsets
> it,
> > > see
> > > > my other comments regarding circular dependencies.
> > > > 
> > > >
> > >
> >
>
https://codereview.chromium.org/2014803002/diff/140001/chrome/browser/android...
> > > > File chrome/browser/android/download/chrome_download_delegate.h (right):
> > > > 
> > > >
> > >
> >
>
https://codereview.chromium.org/2014803002/diff/140001/chrome/browser/android...
> > > > chrome/browser/android/download/chrome_download_delegate.h:40:
> > > > base::android::ScopedJavaGlobalRef<jobject> obj_;
> > > > If the native instance holds a global ref, we have to make sure that the
> > > native
> > > > instance gets deleted when the Tab tears down, because
> > > DownloadController(.java)
> > > > holds a back reference to the Tab. Or you might end up with a circular
> > > > dependency.
> > > > 
> > > >
> > >
> >
>
https://codereview.chromium.org/2014803002/diff/140001/chrome/browser/android...
> > > > File chrome/browser/android/tab_android.cc (right):
> > > > 
> > > >
> > >
> >
>
https://codereview.chromium.org/2014803002/diff/140001/chrome/browser/android...
> > > > chrome/browser/android/tab_android.cc:410: new
> > > > ChromeDownloadDelegate(jdownload_delegate));
> > > > Who takes ownership of the new ChromeDownloadDelegate i.e. when does it
> get
> > > > deleted?
> > > 
> > 
> > Let me sort out the dependency and the lifecycle of the delegate.
> > 
> > > The network stack team is trying to remove  the binding between
webcontents
> > and
> > > download. Download should be related to profiles, rather than webContents.

> > > If a tab is closed, that shouldn't impact download.
> > > We previously use ContetnViewCore is because we want to show infobars. But
> we
> > > don't really need them after the infobar is shown.
> > 
> > Then how about we keep the delegate in the 'native' side of content view
core
> > which
> > I believe will remain as the impl of ViewAndroid after all the refactoring.
> > 
> > WDYT daniel?
> 
> That would also go away.
> 
> ContentViewCore is really mostly something like a WebContentsViewAndroid (i.e.
> the View- and platform-specific parts of the WebContents).
> Delegating up to the embedder through WebContents(Delegate) or other related
> interfaces is the common pattern for this.
> 
> There is also a dependency injection here for the embedder to inject its
> implementation.
> Often this sort of feature extension is done as a WebContents::UserData where
we
> want the WebContents to take ownership of the object since its lifetime is
tied
> to the WebContents.
> 
> Min said:
> > The network stack team is trying to remove  the binding between webcontents
> and
> > download. Download should be related to profiles, rather than webContents. 
> > If a tab is closed, that shouldn't impact download.
> > We previously use ContetnViewCore is because we want to show infobars. But
we
> > don't really need them after the infobar is shown.
> 
> Yea, I noticed that there already is a global 'DownloadNotificationService'
> which is not tied to the tab and then the UI-related logic which is tied to
the
> current WebContents container. So that didn't look totally unreasonable and we
> were mostly trying to simplify JNI flows (and potentially object lifetime) and
> separate better the public API vs. private implementation.
> 
> Doesn't it still make sense to associate the delegate interface with a
> WebContents container since the notification are specific to that WebContents
> instance? How would the app know in which tab to show the infobar?

Yes. The delegate is needed before the download starts, which is for user to
confirm the download if the file is dangerous, or if it already exists.

[no sievers, 2016-06-03 21:56:14]

Regarding the deferred download logic:

I feel like this belongs in the embedder to decide what to do for a download
that's triggered from a non-showing WebContents or whatever.
Min, what do you think?

I think in the very least in this patch we could decouple this deferral logic
from anything that says ContentViewCore, since it really just waits for the
WebContents to become visible.

[qinmin, 2016-06-03 21:57:25]

On 2016/06/03 21:56:14, sievers wrote:
> Regarding the deferred download logic:
> 
> I feel like this belongs in the embedder to decide what to do for a download
> that's triggered from a non-showing WebContents or whatever.
> Min, what do you think?
> 
> I think in the very least in this patch we could decouple this deferral logic
> from anything that says ContentViewCore, since it really just waits for the
> WebContents to become visible.

ok, sounds fair

[no sievers, 2016-06-03 22:02:11]

On 2016/06/03 21:57:25, qinmin wrote:
> On 2016/06/03 21:56:14, sievers wrote:
> > Regarding the deferred download logic:
> > 
> > I feel like this belongs in the embedder to decide what to do for a download
> > that's triggered from a non-showing WebContents or whatever.
> > Min, what do you think?
> > 
> > I think in the very least in this patch we could decouple this deferral
logic
> > from anything that says ContentViewCore, since it really just waits for the
> > WebContents to become visible.
> 
> ok, sounds fair

// TODO(qinmin): Remove this when java Tab object creation is no longer pending
// on Activity creation.

Do you remember what that comment was for?

I'm just worried about us breaking something so if we changed the current logic
from
'defer start download if !content_view_core'
to
'defer start download if !web_contents->IsShowing()'
for example, I wonder if that could break anything.

When would the WebContents not have had a ContentViewCore? Prerender maybe?
Anything else you can remember?

[no sievers, 2016-06-03 22:19:03]

Description was changed from

==========
Let WebContents handle DownloadDelegate

ContentViewCore was used simply as a holder of the reference to                 
                                                                                
                                                                                
                                                                            
DownloadDelegate instance for DownloadController to invoke                      
                                                                                
                                                                                
                                                                            
the delegate later. Had WebContents take the role instead.
         
BUG=598880
==========

to

==========
Let WebContents handle DownloadDelegate

ContentViewCore was used simply as a holder of the reference to                 
                                                                                
                                                                                
                                                                            
DownloadDelegate instance for DownloadController to invoke                      
                                                                                
                                                                                
                                                                            
the delegate later. Had WebContents take the role instead.
         
BUG=598880,617313
==========

[qinmin, 2016-06-03 23:33:41]

On 2016/06/03 22:02:11, sievers wrote:
> On 2016/06/03 21:57:25, qinmin wrote:
> > On 2016/06/03 21:56:14, sievers wrote:
> > > Regarding the deferred download logic:
> > > 
> > > I feel like this belongs in the embedder to decide what to do for a
download
> > > that's triggered from a non-showing WebContents or whatever.
> > > Min, what do you think?
> > > 
> > > I think in the very least in this patch we could decouple this deferral
> logic
> > > from anything that says ContentViewCore, since it really just waits for
the
> > > WebContents to become visible.
> > 
> > ok, sounds fair
> 
> // TODO(qinmin): Remove this when java Tab object creation is no longer
pending
> // on Activity creation.
> 
> Do you remember what that comment was for?

I vaguely remember this is used for document mode. In certain cases that the tab
will not be ready until activity is created. Maria should have fixed that.

> 
> I'm just worried about us breaking something so if we changed the current
logic
> from
> 'defer start download if !content_view_core'
> to
> 'defer start download if !web_contents->IsShowing()'
> for example, I wonder if that could break anything.
> 
> When would the WebContents not have had a ContentViewCore? Prerender maybe?
> Anything else you can remember?

I don't know anything else about this.

[no sievers, 2016-06-04 00:15:16]

Maybe we don't have to worry about WebContents and ownership so much at all and
can simplify this even more, see  crbug.com/617361 which i filed.

[Jinsuk Kim, 2016-06-07 07:25:31]

Description was changed from

==========
Let WebContents handle DownloadDelegate

ContentViewCore was used simply as a holder of the reference to                 
                                                                                
                                                                                
                                                                            
DownloadDelegate instance for DownloadController to invoke                      
                                                                                
                                                                                
                                                                            
the delegate later. Had WebContents take the role instead.
         
BUG=598880,617313
==========

to

==========
Let WebContents handle DownloadDelegate

ContentViewCore was used simply as a holder of the reference to                 
                                                                                
                                                                                
                                                                            
DownloadDelegate instance for DownloadController to invoke                      
                                                                                
                                                                                
                                                                            
the delegate later. Had WebContents take the role instead.
         
BUG=598880,617313, 617361
==========

[Jinsuk Kim, 2016-06-07 07:28:23]

Moved DownloadControllerAndroid(Impl) to chrome/.

I pulled GetURLRequest() out to the interface ResourceDispatcherHost
tentatively. It is used to fill out the struct DownloadAndroidInfo as well as
cookie.

[no sievers, 2016-06-07 21:35:23]

nice

https://codereview.chromium.org/2014803002/diff/180001/chrome/browser/android...
File chrome/browser/android/download/chrome_download_delegate.h (right):

https://codereview.chromium.org/2014803002/diff/180001/chrome/browser/android...
chrome/browser/android/download/chrome_download_delegate.h:39:
~ChromeDownloadDelegate() override;
nit: I think the destructor can be private too since WC owns this through the
base class.

https://codereview.chromium.org/2014803002/diff/180001/chrome/browser/android...
chrome/browser/android/download/chrome_download_delegate.h:45:
JavaObjectWeakGlobalRef weak_java_ref_;
I think that pattern makes sense for a delegate (weak ref, no null checking),
but I'd maybe put a comment here saying something like "The receiver is expected
to outlive |this|."

https://codereview.chromium.org/2014803002/diff/180001/chrome/browser/android...
File chrome/browser/android/download/download_controller_android.h (right):

https://codereview.chromium.org/2014803002/diff/180001/chrome/browser/android...
chrome/browser/android/download/download_controller_android.h:19: class
DownloadControllerAndroid : public content::DownloadItem::Observer {
nit: you don't need the base class interface anymore now

https://codereview.chromium.org/2014803002/diff/180001/chrome/browser/android...
File chrome/browser/android/download/download_controller_android_impl.cc
(right):

https://codereview.chromium.org/2014803002/diff/180001/chrome/browser/android...
chrome/browser/android/download/download_controller_android_impl.cc:201: if
(HasFileAccessPermission(window_android)) {
|window_android| can be null (with custom tabs), see
ContentViewCoreImpl::UpdateWindowAndroid(). SO you should check that too.

I *think* GetViewAndroid() on the other hand shouldn't be null since we only get
here if we are dealing with a Tab, and that would create the WebContents with
the ContentViewCore/ViewAndroid.

https://codereview.chromium.org/2014803002/diff/180001/chrome/browser/android...
chrome/browser/android/download/download_controller_android_impl.cc:466:
ChromeDownloadDelegate::FromWebContents(web_contents)->OnDownloadStarted(
not OnDangerousDownload()?

https://codereview.chromium.org/2014803002/diff/180001/chrome/browser/android...
File chrome/browser/android/download/download_controller_android_impl.h (right):

https://codereview.chromium.org/2014803002/diff/180001/chrome/browser/android...
chrome/browser/android/download/download_controller_android_impl.h:51: class
DownloadControllerAndroidImpl : public DownloadControllerAndroid {
nit: I think this could just be DownloadController now, which would also match
the Java class name then.

https://codereview.chromium.org/2014803002/diff/180001/chrome/browser/android...
File chrome/browser/android/tab_android.cc (right):

https://codereview.chromium.org/2014803002/diff/180001/chrome/browser/android...
chrome/browser/android/tab_android.cc:405: SetDelegate(env, jdownload_delegate);
nit: I'd call this Init() or something instead of SetDelegate().

https://codereview.chromium.org/2014803002/diff/180001/content/public/android...
File
content/public/android/java/src/org/chromium/content_public/browser/DownloadInfo.java
(right):

https://codereview.chromium.org/2014803002/diff/180001/content/public/android...
content/public/android/java/src/org/chromium/content_public/browser/DownloadInfo.java:5:
package org.chromium.content_public.browser;
So can this move up also now and the corresponding test?

Seems like it's not used in content anymore.

https://codereview.chromium.org/2014803002/diff/180001/content/public/browser...
File content/public/browser/resource_dispatcher_host.h (right):

https://codereview.chromium.org/2014803002/diff/180001/content/public/browser...
content/public/browser/resource_dispatcher_host.h:58: const
content::GlobalRequestID& request_id) = 0;
@Min: you think it's straightforward to maybe remove the need for the cookies in
a prerequisite patch so we don't have to expose this?

[no sievers, 2016-06-07 21:37:26]

https://codereview.chromium.org/2014803002/diff/180001/chrome/browser/android...
File chrome/browser/android/download/download_controller_android.h (right):

https://codereview.chromium.org/2014803002/diff/180001/chrome/browser/android...
chrome/browser/android/download/download_controller_android.h:19: class
DownloadControllerAndroid : public content::DownloadItem::Observer {
On 2016/06/07 21:35:23, sievers wrote:
> nit: you don't need the base class interface anymore now 

Oops I missed the mock implementation. Maybe DownloadControllerBase here?
So that the implementation can have its name match the Java class.

[Jinsuk Kim, 2016-06-08 03:35:04]

Description was changed from

==========
Let WebContents handle DownloadDelegate

ContentViewCore was used simply as a holder of the reference to                 
                                                                                
                                                                                
                                                                            
DownloadDelegate instance for DownloadController to invoke                      
                                                                                
                                                                                
                                                                            
the delegate later. Had WebContents take the role instead.
         
BUG=598880,617313, 617361
==========

to

==========
Move DownloadControllerAndroid from content/ to chrome/

This change lets embedder to decide how to handle downloads,
and makes content layer more self-contained. Accompanying changes are:

- Now WebContents holds the reference to download delegate via
WebContentsUserData.

- ContentViewDownloadDelegate interface was removed
       
BUG=598880,617313, 617361
==========

[Jinsuk Kim, 2016-06-08 04:34:17]

Patchset #9 (id:200001) has been deleted

[Jinsuk Kim, 2016-06-08 04:34:38]

Patchset #9 (id:220001) has been deleted

[Jinsuk Kim, 2016-06-08 05:52:54]

Patchset #9 (id:240001) has been deleted

[Jinsuk Kim, 2016-06-08 06:29:09]

https://codereview.chromium.org/2014803002/diff/180001/chrome/browser/android...
File chrome/browser/android/download/chrome_download_delegate.h (right):

https://codereview.chromium.org/2014803002/diff/180001/chrome/browser/android...
chrome/browser/android/download/chrome_download_delegate.h:39:
~ChromeDownloadDelegate() override;
On 2016/06/07 21:35:22, sievers wrote:
> nit: I think the destructor can be private too since WC owns this through the
> base class.

Done.

https://codereview.chromium.org/2014803002/diff/180001/chrome/browser/android...
chrome/browser/android/download/chrome_download_delegate.h:45:
JavaObjectWeakGlobalRef weak_java_ref_;
On 2016/06/07 21:35:23, sievers wrote:
> I think that pattern makes sense for a delegate (weak ref, no null checking),
> but I'd maybe put a comment here saying something like "The receiver is
expected
> to outlive |this|."

Done.

https://codereview.chromium.org/2014803002/diff/180001/chrome/browser/android...
File chrome/browser/android/download/download_controller_android.h (right):

https://codereview.chromium.org/2014803002/diff/180001/chrome/browser/android...
chrome/browser/android/download/download_controller_android.h:19: class
DownloadControllerAndroid : public content::DownloadItem::Observer {
On 2016/06/07 21:35:23, sievers wrote:
> nit: you don't need the base class interface anymore now 

Acknowledged.

https://codereview.chromium.org/2014803002/diff/180001/chrome/browser/android...
chrome/browser/android/download/download_controller_android.h:19: class
DownloadControllerAndroid : public content::DownloadItem::Observer {
On 2016/06/07 21:37:26, sievers wrote:
> On 2016/06/07 21:35:23, sievers wrote:
> > nit: you don't need the base class interface anymore now 
> 
> Oops I missed the mock implementation. Maybe DownloadControllerBase here?
> So that the implementation can have its name match the Java class.

Done.

https://codereview.chromium.org/2014803002/diff/180001/chrome/browser/android...
File chrome/browser/android/download/download_controller_android_impl.cc
(right):

https://codereview.chromium.org/2014803002/diff/180001/chrome/browser/android...
chrome/browser/android/download/download_controller_android_impl.cc:201: if
(HasFileAccessPermission(window_android)) {
On 2016/06/07 21:35:23, sievers wrote:
> |window_android| can be null (with custom tabs), see
> ContentViewCoreImpl::UpdateWindowAndroid(). SO you should check that too.
> 
> I *think* GetViewAndroid() on the other hand shouldn't be null since we only
get
> here if we are dealing with a Tab, and that would create the WebContents with
> the ContentViewCore/ViewAndroid.

Done.

https://codereview.chromium.org/2014803002/diff/180001/chrome/browser/android...
chrome/browser/android/download/download_controller_android_impl.cc:466:
ChromeDownloadDelegate::FromWebContents(web_contents)->OnDownloadStarted(
On 2016/06/07 21:35:23, sievers wrote:
> not OnDangerousDownload()?

Thanks for catching. Done.

https://codereview.chromium.org/2014803002/diff/180001/chrome/browser/android...
File chrome/browser/android/download/download_controller_android_impl.h (right):

https://codereview.chromium.org/2014803002/diff/180001/chrome/browser/android...
chrome/browser/android/download/download_controller_android_impl.h:51: class
DownloadControllerAndroidImpl : public DownloadControllerAndroid {
On 2016/06/07 21:35:23, sievers wrote:
> nit: I think this could just be DownloadController now, which would also match
> the Java class name then.

Done.

https://codereview.chromium.org/2014803002/diff/180001/chrome/browser/android...
File chrome/browser/android/tab_android.cc (right):

https://codereview.chromium.org/2014803002/diff/180001/chrome/browser/android...
chrome/browser/android/tab_android.cc:405: SetDelegate(env, jdownload_delegate);
On 2016/06/07 21:35:23, sievers wrote:
> nit: I'd call this Init() or something instead of SetDelegate().

Done.

https://codereview.chromium.org/2014803002/diff/180001/content/public/android...
File
content/public/android/java/src/org/chromium/content_public/browser/DownloadInfo.java
(right):

https://codereview.chromium.org/2014803002/diff/180001/content/public/android...
content/public/android/java/src/org/chromium/content_public/browser/DownloadInfo.java:5:
package org.chromium.content_public.browser;
On 2016/06/07 21:35:23, sievers wrote:
> So can this move up also now and the corresponding test?
> 
> Seems like it's not used in content anymore.

Also moved to chrome.

https://codereview.chromium.org/2014803002/diff/180001/content/public/browser...
File content/public/browser/resource_dispatcher_host.h (right):

https://codereview.chromium.org/2014803002/diff/180001/content/public/browser...
content/public/browser/resource_dispatcher_host.h:58: const
content::GlobalRequestID& request_id) = 0;
On 2016/06/07 21:35:23, sievers wrote:
> @Min: you think it's straightforward to maybe remove the need for the cookies
in
> a prerequisite patch so we don't have to expose this?

URLRequest instance is necessary not only for cookie but also for building
DownloadInfo used in download_controller.cc.

[no sievers, 2016-06-08 18:28:22]

https://codereview.chromium.org/2014803002/diff/180001/content/public/browser...
File content/public/browser/resource_dispatcher_host.h (right):

https://codereview.chromium.org/2014803002/diff/180001/content/public/browser...
content/public/browser/resource_dispatcher_host.h:58: const
content::GlobalRequestID& request_id) = 0;
On 2016/06/08 06:29:08, Jinsuk wrote:
> On 2016/06/07 21:35:23, sievers wrote:
> > @Min: you think it's straightforward to maybe remove the need for the
cookies
> in
> > a prerequisite patch so we don't have to expose this?
> 
> URLRequest instance is necessary not only for cookie but also for building
> DownloadInfo used in download_controller.cc.

It's actually also obsolete with kSystemDownloadManager. But you are right and
the removal of all that code shouldn't be in this patch.

I'm wondering if the PrepareDownloadInfo() code can move up to the call site
which is  InterceptDownloadResourceThrottle::ProcessDownloadRequest(). That
class actually gets the URLRequest in its contructor.

[qinmin, 2016-06-08 18:31:37]

https://codereview.chromium.org/2014803002/diff/180001/content/public/browser...
File content/public/browser/resource_dispatcher_host.h (right):

https://codereview.chromium.org/2014803002/diff/180001/content/public/browser...
content/public/browser/resource_dispatcher_host.h:58: const
content::GlobalRequestID& request_id) = 0;
On 2016/06/08 18:28:22, sievers wrote:
> On 2016/06/08 06:29:08, Jinsuk wrote:
> > On 2016/06/07 21:35:23, sievers wrote:
> > > @Min: you think it's straightforward to maybe remove the need for the
> cookies
> > in
> > > a prerequisite patch so we don't have to expose this?
> > 
> > URLRequest instance is necessary not only for cookie but also for building
> > DownloadInfo used in download_controller.cc.
> 
> It's actually also obsolete with kSystemDownloadManager. But you are right and
> the removal of all that code shouldn't be in this patch.
> 
> I'm wondering if the PrepareDownloadInfo() code can move up to the call site
> which is  InterceptDownloadResourceThrottle::ProcessDownloadRequest(). That
> class actually gets the URLRequest in its contructor.

Due to some user feedback, we are not yet deprecating kSystemDownloadManager, so
the cookies are still needed

[Jinsuk Kim, 2016-06-08 22:33:49]

On 2016/06/08 18:31:37, qinmin wrote:
>
https://codereview.chromium.org/2014803002/diff/180001/content/public/browser...
> File content/public/browser/resource_dispatcher_host.h (right):
> 
>
https://codereview.chromium.org/2014803002/diff/180001/content/public/browser...
> content/public/browser/resource_dispatcher_host.h:58: const
> content::GlobalRequestID& request_id) = 0;
> On 2016/06/08 18:28:22, sievers wrote:
> > On 2016/06/08 06:29:08, Jinsuk wrote:
> > > On 2016/06/07 21:35:23, sievers wrote:
> > > > @Min: you think it's straightforward to maybe remove the need for the
> > cookies
> > > in
> > > > a prerequisite patch so we don't have to expose this?
> > > 
> > > URLRequest instance is necessary not only for cookie but also for building
> > > DownloadInfo used in download_controller.cc.
> > 
> > It's actually also obsolete with kSystemDownloadManager. But you are right
and
> > the removal of all that code shouldn't be in this patch.
> > 
> > I'm wondering if the PrepareDownloadInfo() code can move up to the call site
> > which is  InterceptDownloadResourceThrottle::ProcessDownloadRequest(). That
> > class actually gets the URLRequest in its contructor.
> 
> Due to some user feedback, we are not yet deprecating kSystemDownloadManager,
so
> the cookies are still needed

Sounds like we need to have GetURLRequest() in the interface for now.
Min, would you review the overall changes and let me know if it looks good?
Thanks.

[no sievers, 2016-06-08 22:37:29]

On 2016/06/08 22:33:49, Jinsuk wrote:
> On 2016/06/08 18:31:37, qinmin wrote:
> >
>
https://codereview.chromium.org/2014803002/diff/180001/content/public/browser...
> > File content/public/browser/resource_dispatcher_host.h (right):
> > 
> >
>
https://codereview.chromium.org/2014803002/diff/180001/content/public/browser...
> > content/public/browser/resource_dispatcher_host.h:58: const
> > content::GlobalRequestID& request_id) = 0;
> > On 2016/06/08 18:28:22, sievers wrote:
> > > On 2016/06/08 06:29:08, Jinsuk wrote:
> > > > On 2016/06/07 21:35:23, sievers wrote:
> > > > > @Min: you think it's straightforward to maybe remove the need for the
> > > cookies
> > > > in
> > > > > a prerequisite patch so we don't have to expose this?
> > > > 
> > > > URLRequest instance is necessary not only for cookie but also for
building
> > > > DownloadInfo used in download_controller.cc.
> > > 
> > > It's actually also obsolete with kSystemDownloadManager. But you are right
> and
> > > the removal of all that code shouldn't be in this patch.
> > > 
> > > I'm wondering if the PrepareDownloadInfo() code can move up to the call
site
> > > which is  InterceptDownloadResourceThrottle::ProcessDownloadRequest().
That
> > > class actually gets the URLRequest in its contructor.
> > 
> > Due to some user feedback, we are not yet deprecating
kSystemDownloadManager,
> so
> > the cookies are still needed
> 
> Sounds like we need to have GetURLRequest() in the interface for now.
> Min, would you review the overall changes and let me know if it looks good?
> Thanks.

> I'm wondering if the PrepareDownloadInfo() code can move up to the call site
which is  InterceptDownloadResourceThrottle::ProcessDownloadRequest(). That
class actually gets the URLRequest in its contructor.

^^ What about that idea?

[Jinsuk Kim, 2016-06-08 22:56:23]

On 2016/06/08 22:37:29, sievers wrote:
> On 2016/06/08 22:33:49, Jinsuk wrote:
> > On 2016/06/08 18:31:37, qinmin wrote:
> > >
> >
>
https://codereview.chromium.org/2014803002/diff/180001/content/public/browser...
> > > File content/public/browser/resource_dispatcher_host.h (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/2014803002/diff/180001/content/public/browser...
> > > content/public/browser/resource_dispatcher_host.h:58: const
> > > content::GlobalRequestID& request_id) = 0;
> > > On 2016/06/08 18:28:22, sievers wrote:
> > > > On 2016/06/08 06:29:08, Jinsuk wrote:
> > > > > On 2016/06/07 21:35:23, sievers wrote:
> > > > > > @Min: you think it's straightforward to maybe remove the need for
the
> > > > cookies
> > > > > in
> > > > > > a prerequisite patch so we don't have to expose this?
> > > > > 
> > > > > URLRequest instance is necessary not only for cookie but also for
> building
> > > > > DownloadInfo used in download_controller.cc.
> > > > 
> > > > It's actually also obsolete with kSystemDownloadManager. But you are
right
> > and
> > > > the removal of all that code shouldn't be in this patch.
> > > > 
> > > > I'm wondering if the PrepareDownloadInfo() code can move up to the call
> site
> > > > which is  InterceptDownloadResourceThrottle::ProcessDownloadRequest().
> That
> > > > class actually gets the URLRequest in its contructor.
> > > 
> > > Due to some user feedback, we are not yet deprecating
> kSystemDownloadManager,
> > so
> > > the cookies are still needed
> > 
> > Sounds like we need to have GetURLRequest() in the interface for now.
> > Min, would you review the overall changes and let me know if it looks good?
> > Thanks.
> 
> > I'm wondering if the PrepareDownloadInfo() code can move up to the call site
> which is  InterceptDownloadResourceThrottle::ProcessDownloadRequest(). That
> class actually gets the URLRequest in its contructor.
> 
> ^^ What about that idea?

Oh I realized you meant to get both cookies and download info in
InterceptDownloadResourceThrottle. Thanks for the suggestion. Will do that.

[Jinsuk Kim, 2016-06-09 00:49:19]

Let InterceptDownloadResourceThrottle::ProcessDownloadRequest() pass
net::URLRequest instance to DownloadController::CreateGETDownload(). PTAL

[qinmin, 2016-06-09 17:07:45]

https://codereview.chromium.org/2014803002/diff/280001/chrome/browser/android...
File chrome/browser/android/download/download_controller.cc (right):

https://codereview.chromium.org/2014803002/diff/280001/chrome/browser/android...
chrome/browser/android/download/download_controller.cc:262:
base::Unretained(this), info, callback, request));
is it safe to pass request instead of global_id? Request can get deleted when
the async call runs.

[no sievers, 2016-06-09 20:22:25]

On 2016/06/09 17:07:45, qinmin wrote:
>
https://codereview.chromium.org/2014803002/diff/280001/chrome/browser/android...
> File chrome/browser/android/download/download_controller.cc (right):
> 
>
https://codereview.chromium.org/2014803002/diff/280001/chrome/browser/android...
> chrome/browser/android/download/download_controller.cc:262:
> base::Unretained(this), info, callback, request));
> is it safe to pass request instead of global_id? Request can get deleted when
> the async call runs.

Well now that you say that, isn't the code racy and flaky then as is?

InterceptDownloadResourceThrottle::::ProcessDownloadRequest() calls
controller()->Cancel() which seems to initiate the async destruction of the
request.
So doesn't it race with PrepareDownloadInfo()?

Seems like we want to pull out all the information we can right when we
intercept on the IO thread somehow.

[no sievers, 2016-06-09 20:26:51]

On 2016/06/09 20:22:25, sievers wrote:
> On 2016/06/09 17:07:45, qinmin wrote:
> >
>
https://codereview.chromium.org/2014803002/diff/280001/chrome/browser/android...
> > File chrome/browser/android/download/download_controller.cc (right):
> > 
> >
>
https://codereview.chromium.org/2014803002/diff/280001/chrome/browser/android...
> > chrome/browser/android/download/download_controller.cc:262:
> > base::Unretained(this), info, callback, request));
> > is it safe to pass request instead of global_id? Request can get deleted
when
> > the async call runs.
> 
> Well now that you say that, isn't the code racy and flaky then as is?
> 
> InterceptDownloadResourceThrottle::::ProcessDownloadRequest() calls
> controller()->Cancel() which seems to initiate the async destruction of the
> request.
> So doesn't it race with PrepareDownloadInfo()?
> 
> Seems like we want to pull out all the information we can right when we
> intercept on the IO thread somehow.

Actually sorry, it does happen on the right thread I think. It just confused us
both?
Doesn't look like there is a task being posted before we reach into the
|request|.

Maybe it would be simpler to follow and harder to break if the code to retrieve
all that move up to InterceptDownloadResourceThrottle.

[qinmin, 2016-06-09 20:36:24]

On 2016/06/09 20:26:51, sievers wrote:
> On 2016/06/09 20:22:25, sievers wrote:
> > On 2016/06/09 17:07:45, qinmin wrote:
> > >
> >
>
https://codereview.chromium.org/2014803002/diff/280001/chrome/browser/android...
> > > File chrome/browser/android/download/download_controller.cc (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/2014803002/diff/280001/chrome/browser/android...
> > > chrome/browser/android/download/download_controller.cc:262:
> > > base::Unretained(this), info, callback, request));
> > > is it safe to pass request instead of global_id? Request can get deleted
> when
> > > the async call runs.
> > 
> > Well now that you say that, isn't the code racy and flaky then as is?
> > 
> > InterceptDownloadResourceThrottle::::ProcessDownloadRequest() calls
> > controller()->Cancel() which seems to initiate the async destruction of the
> > request.
> > So doesn't it race with PrepareDownloadInfo()?
> > 
> > Seems like we want to pull out all the information we can right when we
> > intercept on the IO thread somehow.
> 
> Actually sorry, it does happen on the right thread I think. It just confused
us
> both?
> Doesn't look like there is a task being posted before we reach into the
> |request|.
> 
> Maybe it would be simpler to follow and harder to break if the code to
retrieve
> all that move up to InterceptDownloadResourceThrottle.

Or we can pass a callback to cancel the request to
download_controller_android_impl, after it finishes all the cookie stuff, it
will call controller()->cancel() on the right thread

[no sievers, 2016-06-10 00:40:29]

On 2016/06/09 20:36:24, qinmin wrote:
> On 2016/06/09 20:26:51, sievers wrote:
> > On 2016/06/09 20:22:25, sievers wrote:
> > > On 2016/06/09 17:07:45, qinmin wrote:
> > > >
> > >
> >
>
https://codereview.chromium.org/2014803002/diff/280001/chrome/browser/android...
> > > > File chrome/browser/android/download/download_controller.cc (right):
> > > > 
> > > >
> > >
> >
>
https://codereview.chromium.org/2014803002/diff/280001/chrome/browser/android...
> > > > chrome/browser/android/download/download_controller.cc:262:
> > > > base::Unretained(this), info, callback, request));
> > > > is it safe to pass request instead of global_id? Request can get deleted
> > when
> > > > the async call runs.
> > > 
> > > Well now that you say that, isn't the code racy and flaky then as is?
> > > 
> > > InterceptDownloadResourceThrottle::::ProcessDownloadRequest() calls
> > > controller()->Cancel() which seems to initiate the async destruction of
the
> > > request.
> > > So doesn't it race with PrepareDownloadInfo()?
> > > 
> > > Seems like we want to pull out all the information we can right when we
> > > intercept on the IO thread somehow.
> > 
> > Actually sorry, it does happen on the right thread I think. It just confused
> us
> > both?
> > Doesn't look like there is a task being posted before we reach into the
> > |request|.
> > 
> > Maybe it would be simpler to follow and harder to break if the code to
> retrieve
> > all that move up to InterceptDownloadResourceThrottle.
> 
> Or we can pass a callback to cancel the request to
> download_controller_android_impl, after it finishes all the cookie stuff, it
> will call controller()->cancel() on the right thread

So that should fix a race in the existing code, and maybe that's good enough for
this patch.

I'd file a bug though and put a TODO since it seems a bit flaky. For example
since we don't set |defer| somebody else could intercept the request and cancel
it.

I'I think this code can be rewritten to be a single posted task by using
GetCookieListWithOptionsAsync() instead (without needing to hold on to
|request|).
In fact it looks like the cookie API we are using is deprecated.

The CanGetCookies() business will have to be split up to first see if access is
allowed while we have the context, and then when the |cookie_list| is loaded it
can do the TabSpecificContentSettings::CookiesRead() business.

[Jinsuk Kim, 2016-06-10 01:50:27]

Ended up moving cookie-related logic up to the caller rather than passing cancel
callback, since DownloadController::CreateGETDownload() is already carrying
callbacks - adding another here would make it harder to read. PTAL.

https://codereview.chromium.org/2014803002/diff/280001/chrome/browser/android...
File chrome/browser/android/download/download_controller.cc (right):

https://codereview.chromium.org/2014803002/diff/280001/chrome/browser/android...
chrome/browser/android/download/download_controller.cc:262:
base::Unretained(this), info, callback, request));
On 2016/06/09 17:07:44, qinmin wrote:
> is it safe to pass request instead of global_id? Request can get deleted when
> the async call runs.

Pulled all the info in advance and passed it to CreateGETDownload().

[Jinsuk Kim, 2016-06-10 04:27:51]

Patchset #12 (id:310001) has been deleted

[Jinsuk Kim, 2016-06-10 04:28:03]

Patchset #11 (id:290001) has been deleted

[no sievers, 2016-06-10 19:40:07]

https://codereview.chromium.org/2014803002/diff/330001/android_webview/browse...
File
android_webview/browser/renderer_host/aw_resource_dispatcher_host_delegate.h
(right):

https://codereview.chromium.org/2014803002/diff/330001/android_webview/browse...
android_webview/browser/renderer_host/aw_resource_dispatcher_host_delegate.h:1: 
nit: remove newline

https://codereview.chromium.org/2014803002/diff/330001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/download/DownloadInfo.java
(right):

https://codereview.chromium.org/2014803002/diff/330001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/download/DownloadInfo.java:10:
public final class DownloadInfo {
nit: can you drop 'public' now since it's only used within the package?

https://codereview.chromium.org/2014803002/diff/330001/chrome/browser/android...
File chrome/browser/android/intercept_download_resource_throttle.cc (right):

https://codereview.chromium.org/2014803002/diff/330001/chrome/browser/android...
chrome/browser/android/intercept_download_resource_throttle.cc:129:
cookie_store->GetAllCookiesForURLAsync(
Can't we use GetCookieListWithOptionsAsync() in the first place to pass in
|options| and avoid the need for the second task?
Looks like this method we are using here is marked as deprecated even.

https://codereview.chromium.org/2014803002/diff/330001/chrome/browser/android...
chrome/browser/android/intercept_download_resource_throttle.cc:132:
base::Unretained(this), info, callback));
|this| might go away...

https://codereview.chromium.org/2014803002/diff/330001/chrome/browser/android...
chrome/browser/android/intercept_download_resource_throttle.cc:142: if
(request_->context()->network_delegate()->CanGetCookies(*request_,
And even if |this| is still valid, |request| might have gone away here.

Other implementations of ResourceThrottle seem to assume that |request| is valid
at least until WillProcessResponse(). 

There is a second complication: To fix the race in the current code with
canceling the request we really need to defer the download until our async tasks
for the cookies complete (well, just to be able to keep |this| and the |request|
alive really). Currently we cancel the request too early which initiates
destruction of |this|. But if we don't cancel it from WillProcessResponse() it
will start the download internally. So I think we should set |defer| to |true|
in WillProcessResponse(). 

I think you'd still want to use a WeakPtr for any task we post to |this|. (Even
if we defer the request which probably keeps it alive, it's not totally clear
that the whole stack might not tear down for another reason.)

There is an alternative way also to rewrite this code a bit further so we don't
need to access |request| after the cookie retrieval completes. Let me know in
case you think that's better. (You basically use GetCookieListWithOptionsAsync()
in the first place and also see if reading cookies is allowed right away which
doesn't need the cookie_list. What needs the list is the recording of this event
i.e. TabSpecificContentSettings::CookiesRead() which can happen asynchronously
though.)

@Min: 
On a related note, this makes me wonder how ResourceThrottles like Safe Browsing
currently work since they are added *after* InterceptDownloadResourceThrottle
which cancels the request, see
ChromeResourceDispatcherHostDelegate::DownloadStarting() and
AppendStandardResourceThrottles(). If we push the download to the system manager
we are ignoring these throttles that run afterwards right?

Doesn't the intercepting throttle that delegates to the system download manager
always have to be last?

[Jinsuk Kim, 2016-06-13 03:21:12]

https://codereview.chromium.org/2014803002/diff/330001/android_webview/browse...
File
android_webview/browser/renderer_host/aw_resource_dispatcher_host_delegate.h
(right):

https://codereview.chromium.org/2014803002/diff/330001/android_webview/browse...
android_webview/browser/renderer_host/aw_resource_dispatcher_host_delegate.h:1: 
On 2016/06/10 19:40:06, sievers wrote:
> nit: remove newline

Done.

https://codereview.chromium.org/2014803002/diff/330001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/download/DownloadInfo.java
(right):

https://codereview.chromium.org/2014803002/diff/330001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/download/DownloadInfo.java:10:
public final class DownloadInfo {
On 2016/06/10 19:40:06, sievers wrote:
> nit: can you drop 'public' now since it's only used within the package?

Done.

https://codereview.chromium.org/2014803002/diff/330001/chrome/browser/android...
File chrome/browser/android/intercept_download_resource_throttle.cc (right):

https://codereview.chromium.org/2014803002/diff/330001/chrome/browser/android...
chrome/browser/android/intercept_download_resource_throttle.cc:129:
cookie_store->GetAllCookiesForURLAsync(
On 2016/06/10 19:40:06, sievers wrote:
> Can't we use GetCookieListWithOptionsAsync() in the first place to pass in
> |options| and avoid the need for the second task?
> Looks like this method we are using here is marked as deprecated even.

Done. Sorry I should have done this in the previous patch.

https://codereview.chromium.org/2014803002/diff/330001/chrome/browser/android...
chrome/browser/android/intercept_download_resource_throttle.cc:132:
base::Unretained(this), info, callback));
On 2016/06/10 19:40:06, sievers wrote:
> |this| might go away...

Set |defer| to true to make sure |this| will stay.

https://codereview.chromium.org/2014803002/diff/330001/chrome/browser/android...
chrome/browser/android/intercept_download_resource_throttle.cc:142: if
(request_->context()->network_delegate()->CanGetCookies(*request_,
On 2016/06/10 19:40:06, sievers wrote:
> And even if |this| is still valid, |request| might have gone away here.
> 
> Other implementations of ResourceThrottle seem to assume that |request| is
valid
> at least until WillProcessResponse(). 
> 
> There is a second complication: To fix the race in the current code with
> canceling the request we really need to defer the download until our async
tasks
> for the cookies complete (well, just to be able to keep |this| and the
|request|
> alive really). Currently we cancel the request too early which initiates
> destruction of |this|. But if we don't cancel it from WillProcessResponse() it
> will start the download internally. So I think we should set |defer| to |true|
> in WillProcessResponse(). 
> 
> I think you'd still want to use a WeakPtr for any task we post to |this|.
(Even
> if we defer the request which probably keeps it alive, it's not totally clear
> that the whole stack might not tear down for another reason.)
> 
> There is an alternative way also to rewrite this code a bit further so we
don't
> need to access |request| after the cookie retrieval completes. Let me know in
> case you think that's better. (You basically use
GetCookieListWithOptionsAsync()
> in the first place and also see if reading cookies is allowed right away which
> doesn't need the cookie_list. What needs the list is the recording of this
event
> i.e. TabSpecificContentSettings::CookiesRead() which can happen asynchronously
> though.)
> 
> @Min: 
> On a related note, this makes me wonder how ResourceThrottles like Safe
Browsing
> currently work since they are added *after* InterceptDownloadResourceThrottle
> which cancels the request, see
> ChromeResourceDispatcherHostDelegate::DownloadStarting() and
> AppendStandardResourceThrottles(). If we push the download to the system
manager
> we are ignoring these throttles that run afterwards right?
> 
> Doesn't the intercepting throttle that delegates to the system download
manager
> always have to be last?

Set |defer| to true to resume the process later when the cookies complete.

[Jinsuk Kim, 2016-06-13 05:34:50]

https://codereview.chromium.org/2014803002/diff/330001/chrome/browser/android...
File chrome/browser/android/intercept_download_resource_throttle.cc (right):

https://codereview.chromium.org/2014803002/diff/330001/chrome/browser/android...
chrome/browser/android/intercept_download_resource_throttle.cc:142: if
(request_->context()->network_delegate()->CanGetCookies(*request_,
On 2016/06/10 19:40:06, sievers wrote:
> And even if |this| is still valid, |request| might have gone away here.
> 
> Other implementations of ResourceThrottle seem to assume that |request| is
valid
> at least until WillProcessResponse(). 
> 
> There is a second complication: To fix the race in the current code with
> canceling the request we really need to defer the download until our async
tasks
> for the cookies complete (well, just to be able to keep |this| and the
|request|
> alive really). Currently we cancel the request too early which initiates
> destruction of |this|. But if we don't cancel it from WillProcessResponse() it
> will start the download internally. So I think we should set |defer| to |true|
> in WillProcessResponse(). 
> 
> I think you'd still want to use a WeakPtr for any task we post to |this|.
(Even
> if we defer the request which probably keeps it alive, it's not totally clear
> that the whole stack might not tear down for another reason.)
> 
> There is an alternative way also to rewrite this code a bit further so we
don't
> need to access |request| after the cookie retrieval completes. Let me know in
> case you think that's better. (You basically use
GetCookieListWithOptionsAsync()
> in the first place and also see if reading cookies is allowed right away which
> doesn't need the cookie_list. What needs the list is the recording of this
event
> i.e. TabSpecificContentSettings::CookiesRead() which can happen asynchronously
> though.)

Didn't split CanGetCookies() because it doesn't look straightforward to get
cookie_setting which is required to check if reading cookies is allowed without
actually calling the method.

I think it acceptable to keep the method as it doesn't seem to make the flow
complicated too much.

> 
> @Min: 
> On a related note, this makes me wonder how ResourceThrottles like Safe
Browsing
> currently work since they are added *after* InterceptDownloadResourceThrottle
> which cancels the request, see
> ChromeResourceDispatcherHostDelegate::DownloadStarting() and
> AppendStandardResourceThrottles(). If we push the download to the system
manager
> we are ignoring these throttles that run afterwards right?
> 
> Doesn't the intercepting throttle that delegates to the system download
manager
> always have to be last?

[qinmin, 2016-06-13 18:31:44]

https://codereview.chromium.org/2014803002/diff/350001/chrome/browser/android...
File chrome/browser/android/download/chrome_download_delegate.h (right):

https://codereview.chromium.org/2014803002/diff/350001/chrome/browser/android...
chrome/browser/android/download/chrome_download_delegate.h:13: : public
content::WebContentsUserData<ChromeDownloadDelegate> {
nit: fix the intent

https://codereview.chromium.org/2014803002/diff/350001/chrome/browser/android...
File chrome/browser/android/download/download_controller.cc (right):

https://codereview.chromium.org/2014803002/diff/350001/chrome/browser/android...
chrome/browser/android/download/download_controller.cc:132: jweak obj;
nit: obj_

https://codereview.chromium.org/2014803002/diff/350001/chrome/browser/android...
File chrome/browser/android/download/download_controller_base.cc (right):

https://codereview.chromium.org/2014803002/diff/350001/chrome/browser/android...
chrome/browser/android/download/download_controller_base.cc:16: :
has_user_gesture(false) {
nit: fix indent

https://codereview.chromium.org/2014803002/diff/350001/chrome/browser/android...
chrome/browser/android/download/download_controller_base.cc:20:
request->response_headers()->GetMimeType(&original_mime_type);
ditto

https://codereview.chromium.org/2014803002/diff/350001/chrome/browser/android...
chrome/browser/android/download/download_controller_base.cc:25: user_agent =
GetUserAgent();
ditto here and below

https://codereview.chromium.org/2014803002/diff/350001/chrome/browser/android...
File chrome/browser/android/intercept_download_resource_throttle.cc (right):

https://codereview.chromium.org/2014803002/diff/350001/chrome/browser/android...
chrome/browser/android/intercept_download_resource_throttle.cc:134:
base::Bind(&InterceptDownloadResourceThrottle::CheckCookiePolicy,
could the throttle go away while this is posted?

[no sievers, 2016-06-13 18:32:47]

lgtm if Min is happy too.

Thanks!

https://codereview.chromium.org/2014803002/diff/350001/chrome/browser/android...
File chrome/browser/android/intercept_download_resource_throttle.cc (right):

https://codereview.chromium.org/2014803002/diff/350001/chrome/browser/android...
chrome/browser/android/intercept_download_resource_throttle.cc:126: if
(cookie_store && !cookie_completed_) {
nit: Can you put a comment that deferring will cause this function to be called
again after we Resume()?

https://codereview.chromium.org/2014803002/diff/350001/chrome/browser/android...
chrome/browser/android/intercept_download_resource_throttle.cc:162:
controller()->Resume();
maybe even mention it here too, since that resource loading code is a bit hard
to follow

[no sievers, 2016-06-13 18:35:51]

https://codereview.chromium.org/2014803002/diff/350001/chrome/browser/android...
File chrome/browser/android/intercept_download_resource_throttle.cc (right):

https://codereview.chromium.org/2014803002/diff/350001/chrome/browser/android...
chrome/browser/android/intercept_download_resource_throttle.cc:134:
base::Bind(&InterceptDownloadResourceThrottle::CheckCookiePolicy,
On 2016/06/13 18:31:44, qinmin wrote:
> could the throttle go away while this is posted?

Actually good catch. This should still use a WeakPtr or CancelableClosure.
Everything here happens on the IO thread right?

https://codereview.chromium.org/2014803002/diff/350001/chrome/browser/android...
chrome/browser/android/intercept_download_resource_throttle.cc:156:
base::Bind(&InterceptDownloadResourceThrottle::ResumeDeferredRequest,
same here

[Jinsuk Kim, 2016-06-14 00:55:58]

https://codereview.chromium.org/2014803002/diff/350001/chrome/browser/android...
File chrome/browser/android/download/chrome_download_delegate.h (right):

https://codereview.chromium.org/2014803002/diff/350001/chrome/browser/android...
chrome/browser/android/download/chrome_download_delegate.h:13: : public
content::WebContentsUserData<ChromeDownloadDelegate> {
On 2016/06/13 18:31:44, qinmin wrote:
> nit: fix the intent

Done.

https://codereview.chromium.org/2014803002/diff/350001/chrome/browser/android...
File chrome/browser/android/download/download_controller.cc (right):

https://codereview.chromium.org/2014803002/diff/350001/chrome/browser/android...
chrome/browser/android/download/download_controller.cc:132: jweak obj;
On 2016/06/13 18:31:44, qinmin wrote:
> nit: obj_

Done.

https://codereview.chromium.org/2014803002/diff/350001/chrome/browser/android...
File chrome/browser/android/download/download_controller_base.cc (right):

https://codereview.chromium.org/2014803002/diff/350001/chrome/browser/android...
chrome/browser/android/download/download_controller_base.cc:16: :
has_user_gesture(false) {
On 2016/06/13 18:31:44, qinmin wrote:
> nit: fix indent

Done.

https://codereview.chromium.org/2014803002/diff/350001/chrome/browser/android...
chrome/browser/android/download/download_controller_base.cc:20:
request->response_headers()->GetMimeType(&original_mime_type);
On 2016/06/13 18:31:44, qinmin wrote:
> ditto

Done.

https://codereview.chromium.org/2014803002/diff/350001/chrome/browser/android...
chrome/browser/android/download/download_controller_base.cc:25: user_agent =
GetUserAgent();
On 2016/06/13 18:31:44, qinmin wrote:
> ditto here and below

Done.

https://codereview.chromium.org/2014803002/diff/350001/chrome/browser/android...
File chrome/browser/android/intercept_download_resource_throttle.cc (right):

https://codereview.chromium.org/2014803002/diff/350001/chrome/browser/android...
chrome/browser/android/intercept_download_resource_throttle.cc:126: if
(cookie_store && !cookie_completed_) {
On 2016/06/13 18:32:47, sievers wrote:
> nit: Can you put a comment that deferring will cause this function to be
called
> again after we Resume()?

Done.

https://codereview.chromium.org/2014803002/diff/350001/chrome/browser/android...
chrome/browser/android/intercept_download_resource_throttle.cc:134:
base::Bind(&InterceptDownloadResourceThrottle::CheckCookiePolicy,
On 2016/06/13 18:35:51, sievers wrote:
> On 2016/06/13 18:31:44, qinmin wrote:
> > could the throttle go away while this is posted?
> 
> Actually good catch. This should still use a WeakPtr or CancelableClosure.
> Everything here happens on the IO thread right?

Yes it does. Used WeakPtr.

https://codereview.chromium.org/2014803002/diff/350001/chrome/browser/android...
chrome/browser/android/intercept_download_resource_throttle.cc:134:
base::Bind(&InterceptDownloadResourceThrottle::CheckCookiePolicy,
On 2016/06/13 18:31:44, qinmin wrote:
> could the throttle go away while this is posted?

Thanks. Done.

https://codereview.chromium.org/2014803002/diff/350001/chrome/browser/android...
chrome/browser/android/intercept_download_resource_throttle.cc:156:
base::Bind(&InterceptDownloadResourceThrottle::ResumeDeferredRequest,
On 2016/06/13 18:35:51, sievers wrote:
> same here

Done.

https://codereview.chromium.org/2014803002/diff/350001/chrome/browser/android...
chrome/browser/android/intercept_download_resource_throttle.cc:162:
controller()->Resume();
On 2016/06/13 18:32:47, sievers wrote:
> maybe even mention it here too, since that resource loading code is a bit hard
> to follow

Done.

[qinmin, 2016-06-14 16:21:40]

lgtm

[Jinsuk Kim, 2016-06-14 21:50:50]

On 2016/06/14 16:21:40, qinmin wrote:
> lgtm

Min, are the changes in this patch guarded by instrumentation/unit tests?

[qinmin, 2016-06-14 21:54:05]

On 2016/06/14 21:50:50, Jinsuk wrote:
> On 2016/06/14 16:21:40, qinmin wrote:
> > lgtm
> 
> Min, are the changes in this patch guarded by instrumentation/unit tests?

I think there is DownloadTest that uses the Android DownloadManager

[qinmin, 2016-06-14 21:54:11]

On 2016/06/14 21:50:50, Jinsuk wrote:
> On 2016/06/14 16:21:40, qinmin wrote:
> > lgtm
> 
> Min, are the changes in this patch guarded by instrumentation/unit tests?

I think there is some DownloadTest that uses the Android DownloadManager

[Jinsuk Kim, 2016-06-14 21:59:52]

Description was changed from

==========
Move DownloadControllerAndroid from content/ to chrome/

This change lets embedder to decide how to handle downloads,
and makes content layer more self-contained. Accompanying changes are:

- Now WebContents holds the reference to download delegate via
WebContentsUserData.

- ContentViewDownloadDelegate interface was removed
       
BUG=598880,617313, 617361
==========

to

==========
Move DownloadControllerAndroid from content/ to chrome/

This change lets embedder to decide how to handle downloads,
and makes content layer more self-contained. Accompanying changes are:

- Now WebContents holds the reference to download delegate via
WebContentsUserData.
- ContentViewDownloadDelegate interface was removed
- DownloadControlerAndroid -> DownloadControllerBase
- DownloadControlerAndroidImpl -> DownloadController to
  have the name matched with the Java class  
     
BUG=598880,617313, 617361
==========

[Jinsuk Kim, 2016-06-14 22:02:48]

jinsukkim@chromium.org changed reviewers:
+ boliu@chromium.org, sky@chromium.org, tedchoc@chromium.org

[Jinsuk Kim, 2016-06-14 22:02:49]

boliu@chromium.org: Please review changes in android_webview

tedchoc@chromium.org: Please review changes in chrome/browser/android

sky@chromium.org: Please review changes in chrome/browser (mostly refactoring)

Thanks,

[boliu, 2016-06-14 23:56:45]

a_w rs lgtm

[Ted C, 2016-06-15 17:44:11]

https://codereview.chromium.org/2014803002/diff/370001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/download/ChromeDownloadDelegate.java
(right):

https://codereview.chromium.org/2014803002/diff/370001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/download/ChromeDownloadDelegate.java:155:
public void requestHttpGetDownload(String url, String userAgent, String
contentDisposition,
should this be private now?

https://codereview.chromium.org/2014803002/diff/370001/chrome/browser/android...
File chrome/browser/android/download/chrome_download_delegate.cc (right):

https://codereview.chromium.org/2014803002/diff/370001/chrome/browser/android...
chrome/browser/android/download/chrome_download_delegate.cc:133:
content::WebContents* web_contents) {}
why take web_contents if unused?

https://codereview.chromium.org/2014803002/diff/370001/chrome/browser/android...
chrome/browser/android/download/chrome_download_delegate.cc:138: weak_java_ref_
= JavaObjectWeakGlobalRef(env, jobj);
why a weak ref?

And if we have to use it, we should ensure the object doesn't go away before any
of all calls below.  The fact we aren't checking makes me think this should be a
regular global ref.

https://codereview.chromium.org/2014803002/diff/370001/chrome/browser/android...
File chrome/browser/android/download/chrome_download_delegate.h (right):

https://codereview.chromium.org/2014803002/diff/370001/chrome/browser/android...
chrome/browser/android/download/chrome_download_delegate.h:13: : public
content::WebContentsUserData<ChromeDownloadDelegate> {
is this indented 3?

https://codereview.chromium.org/2014803002/diff/370001/chrome/browser/android...
File chrome/browser/android/download/download_controller.cc (right):

https://codereview.chromium.org/2014803002/diff/370001/chrome/browser/android...
chrome/browser/android/download/download_controller.cc:161: : java_object_(NULL)
{
why don't we just initialize the java object here?

We already have GetInstance exposed on the java side, so we can rely on that
instead of it being passed in via the Init function.

The double Init functions really threw me off for a while as well, so anything
to clear that up is good.  And if we do it here, it seems like we can delete
Init being called from java entirely.

https://codereview.chromium.org/2014803002/diff/370001/chrome/browser/android...
chrome/browser/android/download/download_controller.cc:369: if (!java_object_) {
this pattern is really quite odd to me.

What is the control flow/construction ordering of this class?  Do we expect
native to be creating the java side or the java side to create the native?  Or
do we not now?  If the last, then we should fix that.  If the former, why not
just call getInstance in the constructor of this class to avoid this weird
pattern?

Are both of these singletons?  If so, they never get destroyed right?  If that
is the case, why the jweak again?  Keep a strong ref since we'll never need to
worry about this.

If not, why not a scoped weak global ref?  I'm not understanding the need for
JavaObject as a struct and it seems to be completely different from anything
else we do, which is a good indicator that we're doing some wonky.

https://codereview.chromium.org/2014803002/diff/370001/chrome/browser/android...
File chrome/browser/android/download/download_controller.h (right):

https://codereview.chromium.org/2014803002/diff/370001/chrome/browser/android...
chrome/browser/android/download/download_controller.h:1: // Copyright (c) 2012
The Chromium Authors. All rights reserved.
update these w/ the new dates

https://codereview.chromium.org/2014803002/diff/370001/chrome/browser/android...
chrome/browser/android/download/download_controller.h:38: class
DownloadController : public DownloadControllerBase {
the naming scheme I'm more used to is the pure virtual is DownloadController,
this would be DownloadControllerImpl

Granted, we use both in chrome, so I can't say this is wrong, but it does seem
to be the less common of the two

find . -name *_base.h | wc -l = 226
find . -name *_impl.h | wc -l = 684

[no sievers, 2016-06-15 18:17:28]

https://codereview.chromium.org/2014803002/diff/370001/chrome/browser/android...
File chrome/browser/android/download/chrome_download_delegate.cc (right):

https://codereview.chromium.org/2014803002/diff/370001/chrome/browser/android...
chrome/browser/android/download/chrome_download_delegate.cc:138: weak_java_ref_
= JavaObjectWeakGlobalRef(env, jobj);
On 2016/06/15 17:44:11, Ted C wrote:
> why a weak ref?
> 
> And if we have to use it, we should ensure the object doesn't go away before
any
> of all calls below.  The fact we aren't checking makes me think this should be
a
> regular global ref.

I was thinking that it's a normal pattern for a Delegate implementation (i.e.
delegatee) to outlive the source of the calls, which seemed conceptually similar
here except that it also involves JNI.
He put a comment in the header next to the weak_java_ref_ to that extent.

But yea this relies on the implementation to do the right thing i.e. Tab to init
and tear down the delegate correctly.

https://codereview.chromium.org/2014803002/diff/370001/chrome/browser/android...
File chrome/browser/android/download/download_controller.cc (right):

https://codereview.chromium.org/2014803002/diff/370001/chrome/browser/android...
chrome/browser/android/download/download_controller.cc:369: if (!java_object_) {
On 2016/06/15 17:44:11, Ted C wrote:
> this pattern is really quite odd to me.
> 
> What is the control flow/construction ordering of this class?  Do we expect
> native to be creating the java side or the java side to create the native?  Or
> do we not now?  If the last, then we should fix that.  If the former, why not
> just call getInstance in the constructor of this class to avoid this weird
> pattern?
> 
> Are both of these singletons?  If so, they never get destroyed right?  If that
> is the case, why the jweak again?  Keep a strong ref since we'll never need to
> worry about this.
> 
> If not, why not a scoped weak global ref?  I'm not understanding the need for
> JavaObject as a struct and it seems to be completely different from anything
> else we do, which is a good indicator that we're doing some wonky.

This is just moved from
content/browser/android/download_controller_android_impl.cc in this patch
though.
Maybe sort out separately?

[no sievers, 2016-06-15 18:34:06]

https://codereview.chromium.org/2014803002/diff/370001/chrome/browser/android...
File chrome/browser/android/download/chrome_download_delegate.cc (right):

https://codereview.chromium.org/2014803002/diff/370001/chrome/browser/android...
chrome/browser/android/download/chrome_download_delegate.cc:138: weak_java_ref_
= JavaObjectWeakGlobalRef(env, jobj);
On 2016/06/15 18:17:28, sievers wrote:
> On 2016/06/15 17:44:11, Ted C wrote:
> > why a weak ref?
> > 
> > And if we have to use it, we should ensure the object doesn't go away before
> any
> > of all calls below.  The fact we aren't checking makes me think this should
be
> a
> > regular global ref.
> 
> I was thinking that it's a normal pattern for a Delegate implementation (i.e.
> delegatee) to outlive the source of the calls, which seemed conceptually
similar
> here except that it also involves JNI.
> He put a comment in the header next to the weak_java_ref_ to that extent.
> 
> But yea this relies on the implementation to do the right thing i.e. Tab to
init
> and tear down the delegate correctly.

Oh and this was my other thinking:
This is really just code split out of Tab (so it can be unit-tested and
mocked?).
And the pattern for Java-object-owns-native-instance (with explicit native
init/destroy) seems to be using a weak ref back to Java also. 

But I don't know a good way to enforce that this is not used elsewhere outside
of tab_android.cc in the wrong manner (besides comments.. well we could DCHECK()
I guess but unfortunately it would only trigger in the rare case where a
potential race is lost and it would crash anyhow).

[Jinsuk Kim, 2016-06-15 21:05:30]

https://codereview.chromium.org/2014803002/diff/370001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/download/ChromeDownloadDelegate.java
(right):

https://codereview.chromium.org/2014803002/diff/370001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/download/ChromeDownloadDelegate.java:155:
public void requestHttpGetDownload(String url, String userAgent, String
contentDisposition,
On 2016/06/15 17:44:11, Ted C wrote:
> should this be private now?

Done.

https://codereview.chromium.org/2014803002/diff/370001/chrome/browser/android...
File chrome/browser/android/download/chrome_download_delegate.cc (right):

https://codereview.chromium.org/2014803002/diff/370001/chrome/browser/android...
chrome/browser/android/download/chrome_download_delegate.cc:133:
content::WebContents* web_contents) {}
On 2016/06/15 17:44:11, Ted C wrote:
> why take web_contents if unused?

This follows the pattern required for the classes inheriting
WebContentsUserData.
https://cs-staging.chromium.org/chromium/src/content/public/browser/web_conte...
WebContentsUserData::CreateForWebContents() expects the constructor to have the
parameter.

https://codereview.chromium.org/2014803002/diff/370001/chrome/browser/android...
chrome/browser/android/download/chrome_download_delegate.cc:138: weak_java_ref_
= JavaObjectWeakGlobalRef(env, jobj);
On 2016/06/15 17:44:11, Ted C wrote:
> why a weak ref?
> 
> And if we have to use it, we should ensure the object doesn't go away before
any
> of all calls below.  The fact we aren't checking makes me think this should be
a
> regular global ref.

Referenced other cases (can't remember what it was right now...) where the
native needs to invoke the Java object which is owned by other Java class (Tab
in this case). I *think* it's safe to expect the Java object will be available
as Tab and the object will outlive underlying (native) WebContents and the
download delegate owned by it. But feel free to let me know if there can be a
chance that this can cause a problem. I'd be happy to change it as suggested.

https://codereview.chromium.org/2014803002/diff/370001/chrome/browser/android...
File chrome/browser/android/download/chrome_download_delegate.h (right):

https://codereview.chromium.org/2014803002/diff/370001/chrome/browser/android...
chrome/browser/android/download/chrome_download_delegate.h:13: : public
content::WebContentsUserData<ChromeDownloadDelegate> {
On 2016/06/15 17:44:11, Ted C wrote:
> is this indented 3?

Fixed to 4

https://codereview.chromium.org/2014803002/diff/370001/chrome/browser/android...
File chrome/browser/android/download/download_controller.cc (right):

https://codereview.chromium.org/2014803002/diff/370001/chrome/browser/android...
chrome/browser/android/download/download_controller.cc:161: : java_object_(NULL)
{
On 2016/06/15 17:44:11, Ted C wrote:
> why don't we just initialize the java object here?
> 
> We already have GetInstance exposed on the java side, so we can rely on that
> instead of it being passed in via the Init function.
> 
> The double Init functions really threw me off for a while as well, so anything
> to clear that up is good.  And if we do it here, it seems like we can delete
> Init being called from java entirely.

Honestly I didn't give it much thought as it was simply copied over to here.
Maybe comments in the original patch here
https://codereview.chromium.org/10449017/diff/10001/content/browser/android/d...
can give us a clue.

https://codereview.chromium.org/2014803002/diff/370001/chrome/browser/android...
File chrome/browser/android/download/download_controller.h (right):

https://codereview.chromium.org/2014803002/diff/370001/chrome/browser/android...
chrome/browser/android/download/download_controller.h:1: // Copyright (c) 2012
The Chromium Authors. All rights reserved.
On 2016/06/15 17:44:11, Ted C wrote:
> update these w/ the new dates

Done. Didn't update it since this was an already present file just renamed while
refactoring. Will update the dates for the other renamed files.

https://codereview.chromium.org/2014803002/diff/370001/chrome/browser/android...
chrome/browser/android/download/download_controller.h:38: class
DownloadController : public DownloadControllerBase {
On 2016/06/15 17:44:11, Ted C wrote:
> the naming scheme I'm more used to is the pure virtual is DownloadController,
> this would be DownloadControllerImpl
> 
> Granted, we use both in chrome, so I can't say this is wrong, but it does seem
> to be the less common of the two
> 
> find . -name *_base.h | wc -l = 226
> find . -name *_impl.h | wc -l = 684

The reason it was named as such is to have the name matched to its Java
counterpart, which I think is better. I'd like to keep it if both are
acceptable.

[Ted C, 2016-06-15 23:56:38]

https://codereview.chromium.org/2014803002/diff/370001/chrome/browser/android...
File chrome/browser/android/download/chrome_download_delegate.cc (right):

https://codereview.chromium.org/2014803002/diff/370001/chrome/browser/android...
chrome/browser/android/download/chrome_download_delegate.cc:133:
content::WebContents* web_contents) {}
On 2016/06/15 21:05:30, Jinsuk wrote:
> On 2016/06/15 17:44:11, Ted C wrote:
> > why take web_contents if unused?
> 
> This follows the pattern required for the classes inheriting
> WebContentsUserData.
>
https://cs-staging.chromium.org/chromium/src/content/public/browser/web_conte...
> WebContentsUserData::CreateForWebContents() expects the constructor to have
the
> parameter.

Acknowledged.

https://codereview.chromium.org/2014803002/diff/370001/chrome/browser/android...
chrome/browser/android/download/chrome_download_delegate.cc:138: weak_java_ref_
= JavaObjectWeakGlobalRef(env, jobj);
On 2016/06/15 21:05:30, Jinsuk wrote:
> On 2016/06/15 17:44:11, Ted C wrote:
> > why a weak ref?
> > 
> > And if we have to use it, we should ensure the object doesn't go away before
> any
> > of all calls below.  The fact we aren't checking makes me think this should
be
> a
> > regular global ref.
> 
> Referenced other cases (can't remember what it was right now...) where the
> native needs to invoke the Java object which is owned by other Java class (Tab
> in this case). I *think* it's safe to expect the Java object will be available
> as Tab and the object will outlive underlying (native) WebContents and the
> download delegate owned by it. But feel free to let me know if there can be a
> chance that this can cause a problem. I'd be happy to change it as suggested.

In general, the cases I've seen for weak refs in java are:

1.) Webview to handle the case that destroy is not required by WebView
    embedders on Android, so they have to handle the case where their
    java objects are GCed but never tell native.
2.) Programmer error.

In the chrome layer, we should always have clear destruction paths.
There are some async cases where you can argue it, but if the java
object has a the ability to destroy the native side then it should
always be a strong ref (or the lifecycle is such that it is never
possible to have one w/o the other).

If we have weakrefs, then we should be null checking them each time they
are used.  tab_android looks like a mistake and something we should undo.

Also, this is an odd flow for initializing the java <-> native stuff.
It should not involve Tab to create the connection.

I think it should either be:

1.) Java ChromeDownloadDelegate constructor calls a new nativeInit (static
    Init on the c++ side) that takes the web contents and constructs the
    native side.
2.) C++ constructor does the reverse with a tab ref or something and whatever
    else it needs.

Ideally, these are self contained units and this isn't yet.

https://codereview.chromium.org/2014803002/diff/370001/chrome/browser/android...
File chrome/browser/android/download/download_controller.cc (right):

https://codereview.chromium.org/2014803002/diff/370001/chrome/browser/android...
chrome/browser/android/download/download_controller.cc:161: : java_object_(NULL)
{
On 2016/06/15 21:05:30, Jinsuk wrote:
> On 2016/06/15 17:44:11, Ted C wrote:
> > why don't we just initialize the java object here?
> > 
> > We already have GetInstance exposed on the java side, so we can rely on that
> > instead of it being passed in via the Init function.
> > 
> > The double Init functions really threw me off for a while as well, so
anything
> > to clear that up is good.  And if we do it here, it seems like we can delete
> > Init being called from java entirely.
> 
> Honestly I didn't give it much thought as it was simply copied over to here.
> Maybe comments in the original patch here
>
https://codereview.chromium.org/10449017/diff/10001/content/browser/android/d...
> can give us a clue.

That change is very, very old, so I wouldn't take it as a good indicator
for the proper way we should be doing things now.

Since this was copied over I'll drop my comments on this class, but I
do think it needs some serious TLC to clean up some wonkiness here.

https://codereview.chromium.org/2014803002/diff/370001/chrome/browser/android...
chrome/browser/android/download/download_controller.cc:369: if (!java_object_) {
On 2016/06/15 18:17:28, sievers wrote:
> On 2016/06/15 17:44:11, Ted C wrote:
> > this pattern is really quite odd to me.
> > 
> > What is the control flow/construction ordering of this class?  Do we expect
> > native to be creating the java side or the java side to create the native? 
Or
> > do we not now?  If the last, then we should fix that.  If the former, why
not
> > just call getInstance in the constructor of this class to avoid this weird
> > pattern?
> > 
> > Are both of these singletons?  If so, they never get destroyed right?  If
that
> > is the case, why the jweak again?  Keep a strong ref since we'll never need
to
> > worry about this.
> > 
> > If not, why not a scoped weak global ref?  I'm not understanding the need
for
> > JavaObject as a struct and it seems to be completely different from anything
> > else we do, which is a good indicator that we're doing some wonky.
> 
> This is just moved from
> content/browser/android/download_controller_android_impl.cc in this patch
> though.
> Maybe sort out separately?

yeah

https://codereview.chromium.org/2014803002/diff/370001/chrome/browser/android...
File chrome/browser/android/download/download_controller.h (right):

https://codereview.chromium.org/2014803002/diff/370001/chrome/browser/android...
chrome/browser/android/download/download_controller.h:38: class
DownloadController : public DownloadControllerBase {
On 2016/06/15 21:05:30, Jinsuk wrote:
> On 2016/06/15 17:44:11, Ted C wrote:
> > the naming scheme I'm more used to is the pure virtual is
DownloadController,
> > this would be DownloadControllerImpl
> > 
> > Granted, we use both in chrome, so I can't say this is wrong, but it does
seem
> > to be the less common of the two
> > 
> > find . -name *_base.h | wc -l = 226
> > find . -name *_impl.h | wc -l = 684
> 
> The reason it was named as such is to have the name matched to its Java
> counterpart, which I think is better. I'd like to keep it if both are
> acceptable.

Acknowledged.

https://codereview.chromium.org/2014803002/diff/390001/chrome/browser/android...
File chrome/browser/android/download/download_controller.h (right):

https://codereview.chromium.org/2014803002/diff/390001/chrome/browser/android...
chrome/browser/android/download/download_controller.h:1: // Copyright (c) 2016
The Chromium Authors. All rights reserved.
we no longer put (c) in these :-P

[Jinsuk Kim, 2016-06-16 05:32:54]

Patchset #15 (id:410001) has been deleted

[Jinsuk Kim, 2016-06-16 05:34:13]

PTAL

https://codereview.chromium.org/2014803002/diff/370001/chrome/browser/android...
File chrome/browser/android/download/chrome_download_delegate.cc (right):

https://codereview.chromium.org/2014803002/diff/370001/chrome/browser/android...
chrome/browser/android/download/chrome_download_delegate.cc:138: weak_java_ref_
= JavaObjectWeakGlobalRef(env, jobj);
On 2016/06/15 23:56:37, Ted C wrote:
> On 2016/06/15 21:05:30, Jinsuk wrote:
> > On 2016/06/15 17:44:11, Ted C wrote:
> > > why a weak ref?
> > > 
> > > And if we have to use it, we should ensure the object doesn't go away
before
> > any
> > > of all calls below.  The fact we aren't checking makes me think this
should
> be
> > a
> > > regular global ref.
> > 
> > Referenced other cases (can't remember what it was right now...) where the
> > native needs to invoke the Java object which is owned by other Java class
(Tab
> > in this case). I *think* it's safe to expect the Java object will be
available
> > as Tab and the object will outlive underlying (native) WebContents and the
> > download delegate owned by it. But feel free to let me know if there can be
a
> > chance that this can cause a problem. I'd be happy to change it as
suggested.
> 
> In general, the cases I've seen for weak refs in java are:
> 
> 1.) Webview to handle the case that destroy is not required by WebView
>     embedders on Android, so they have to handle the case where their
>     java objects are GCed but never tell native.
> 2.) Programmer error.
> 
> In the chrome layer, we should always have clear destruction paths.
> There are some async cases where you can argue it, but if the java
> object has a the ability to destroy the native side then it should
> always be a strong ref (or the lifecycle is such that it is never
> possible to have one w/o the other).
> 
> If we have weakrefs, then we should be null checking them each time they
> are used.  tab_android looks like a mistake and something we should undo.
> 
> Also, this is an odd flow for initializing the java <-> native stuff.
> It should not involve Tab to create the connection.
> 
> I think it should either be:
> 
> 1.) Java ChromeDownloadDelegate constructor calls a new nativeInit (static
>     Init on the c++ side) that takes the web contents and constructs the
>     native side.
> 2.) C++ constructor does the reverse with a tab ref or something and whatever
>     else it needs.
> 
> Ideally, these are self contained units and this isn't yet.

Done. Did 1)

https://codereview.chromium.org/2014803002/diff/370001/chrome/browser/android...
File chrome/browser/android/download/download_controller.cc (right):

https://codereview.chromium.org/2014803002/diff/370001/chrome/browser/android...
chrome/browser/android/download/download_controller.cc:161: : java_object_(NULL)
{
On 2016/06/15 23:56:37, Ted C wrote:
> On 2016/06/15 21:05:30, Jinsuk wrote:
> > On 2016/06/15 17:44:11, Ted C wrote:
> > > why don't we just initialize the java object here?
> > > 
> > > We already have GetInstance exposed on the java side, so we can rely on
that
> > > instead of it being passed in via the Init function.
> > > 
> > > The double Init functions really threw me off for a while as well, so
> anything
> > > to clear that up is good.  And if we do it here, it seems like we can
delete
> > > Init being called from java entirely.
> > 
> > Honestly I didn't give it much thought as it was simply copied over to here.
> > Maybe comments in the original patch here
> >
>
https://codereview.chromium.org/10449017/diff/10001/content/browser/android/d...
> > can give us a clue.
> 
> That change is very, very old, so I wouldn't take it as a good indicator
> for the proper way we should be doing things now.
> 
> Since this was copied over I'll drop my comments on this class, but I
> do think it needs some serious TLC to clean up some wonkiness here.

Acknowledged.

https://codereview.chromium.org/2014803002/diff/390001/chrome/browser/android...
File chrome/browser/android/download/download_controller.h (right):

https://codereview.chromium.org/2014803002/diff/390001/chrome/browser/android...
chrome/browser/android/download/download_controller.h:1: // Copyright (c) 2016
The Chromium Authors. All rights reserved.
On 2016/06/15 23:56:38, Ted C wrote:
> we no longer put (c) in these :-P

Done.

[Ted C, 2016-06-17 20:29:37]

lgtm w/ a couple smaller comments

https://codereview.chromium.org/2014803002/diff/430001/chrome/browser/android...
File chrome/browser/android/download/chrome_download_delegate.cc (right):

https://codereview.chromium.org/2014803002/diff/430001/chrome/browser/android...
chrome/browser/android/download/chrome_download_delegate.cc:140: void
ChromeDownloadDelegate::Init(JNIEnv* env, jobject jobj) {
let's name this something different to avoid the name conflicts w/ the JNI
method.

SetJavaRef or something

https://codereview.chromium.org/2014803002/diff/430001/chrome/browser/android...
chrome/browser/android/download/chrome_download_delegate.cc:206: void
Init(JNIEnv* env,
this is a static right?  the chrome convention would be to add a comment above
// static

https://codereview.chromium.org/2014803002/diff/430001/chrome/browser/android...
File chrome/browser/android/download/chrome_download_delegate.h (right):

https://codereview.chromium.org/2014803002/diff/430001/chrome/browser/android...
chrome/browser/android/download/chrome_download_delegate.h:20: void
Init(JNIEnv*, jobject obj);
does this need to be public?

[Jinsuk Kim, 2016-06-20 02:00:53]

Hi sky@, would you take a look at the files at chrome/browser/ ?

https://codereview.chromium.org/2014803002/diff/430001/chrome/browser/android...
File chrome/browser/android/download/chrome_download_delegate.cc (right):

https://codereview.chromium.org/2014803002/diff/430001/chrome/browser/android...
chrome/browser/android/download/chrome_download_delegate.cc:140: void
ChromeDownloadDelegate::Init(JNIEnv* env, jobject jobj) {
On 2016/06/17 20:29:37, Ted C wrote:
> let's name this something different to avoid the name conflicts w/ the JNI
> method.
> 
> SetJavaRef or something

Done.

https://codereview.chromium.org/2014803002/diff/430001/chrome/browser/android...
chrome/browser/android/download/chrome_download_delegate.cc:206: void
Init(JNIEnv* env,
On 2016/06/17 20:29:37, Ted C wrote:
> this is a static right?  the chrome convention would be to add a comment above
> // static

It's meant to be non-static. nativeInit() comes down with the instantiated Java
object.

https://codereview.chromium.org/2014803002/diff/430001/chrome/browser/android...
File chrome/browser/android/download/chrome_download_delegate.h (right):

https://codereview.chromium.org/2014803002/diff/430001/chrome/browser/android...
chrome/browser/android/download/chrome_download_delegate.h:20: void
Init(JNIEnv*, jobject obj);
On 2016/06/17 20:29:37, Ted C wrote:
> does this need to be public?

Renamed to SetJavaRef. Yes it needs to be called from outside the class
(::Init())

[sky, 2016-06-20 15:05:53]

LGTM

[Jinsuk Kim, 2016-06-21 06:22:03]

Thanks for reviewing!

After lgtm'ed, fixed a bug in InterceptDownloadResourceThrottle caused by the
misunderstanding on how ThrottlingResourceHandler works. Resume() does not cause
the throttle to be invoked again - it simply holds off calling of the next
throttle. So the download processing should be completed within 
InterceptDownloadResourceThrottle::ProcessDownloadRequest(). |defer| should
still be set to true to ensure the throttle instance keeps alive while async
call is made. Resume() needs not calling since we already have Cancel(). 

ChromeDownloadDelegate instantiation now involves native side init where
|WebContents| object is required. And it is assumed to be created in UI thread
in normal circumstance. Fixed  ChromeDownloadDelegateTest
#testShouldInterceptContextMenuDownload so that it will run in the right thread,
and also provided with a Tab instance that has |WebContents| object available
for it.

[no sievers, 2016-06-21 18:32:09]

On 2016/06/21 06:22:03, Jinsuk wrote:
> Thanks for reviewing!
> 
> After lgtm'ed, fixed a bug in InterceptDownloadResourceThrottle caused by the
> misunderstanding on how ThrottlingResourceHandler works. Resume() does not
cause
> the throttle to be invoked again - it simply holds off calling of the next
> throttle. So the download processing should be completed within 
> InterceptDownloadResourceThrottle::ProcessDownloadRequest(). |defer| should
> still be set to true to ensure the throttle instance keeps alive while async
> call is made. Resume() needs not calling since we already have Cancel(). 
> 


Hmm really?

After WillProcessResponse() is called the first time and we set |defer| it will
set |deferred_stage_| to |DEFERRED_RESPONSE| in
ThrottlingResourceHandler::OnResponseStarted().

In ThrottlingResourceHandler::Resume() it would therefore call ResumeResponse()
which is the same function as before that triggers WillProcessResponse().

For comparison also see DownloadResourceThrottle::ContinueDownload() which calls
either Resume() or Cancel().

[no sievers, 2016-06-21 18:36:38]

On 2016/06/21 18:32:09, sievers wrote:
> On 2016/06/21 06:22:03, Jinsuk wrote:
> > Thanks for reviewing!
> > 
> > After lgtm'ed, fixed a bug in InterceptDownloadResourceThrottle caused by
the
> > misunderstanding on how ThrottlingResourceHandler works. Resume() does not
> cause
> > the throttle to be invoked again - it simply holds off calling of the next
> > throttle. So the download processing should be completed within 
> > InterceptDownloadResourceThrottle::ProcessDownloadRequest(). |defer| should
> > still be set to true to ensure the throttle instance keeps alive while async
> > call is made. Resume() needs not calling since we already have Cancel(). 
> > 
> 
> 
> Hmm really?
> 
> After WillProcessResponse() is called the first time and we set |defer| it
will
> set |deferred_stage_| to |DEFERRED_RESPONSE| in
> ThrottlingResourceHandler::OnResponseStarted().
> 
> In ThrottlingResourceHandler::Resume() it would therefore call
ResumeResponse()
> which is the same function as before that triggers WillProcessResponse().
> 
> For comparison also see DownloadResourceThrottle::ContinueDownload() which
calls
> either Resume() or Cancel().

Ah, you are right I missed this:
   if (*defer) {
     ...
     return true;  // Do not cancel.
    }
  }

  next_index_ = 0;  // Reset for next time.

So it continues with the next Throttle when deferred, which in our case is not
good enough since we actually need to take over the download.

lgtm!

[Jinsuk Kim, 2016-06-21 22:23:16]

On 2016/06/21 18:36:38, sievers wrote:
> On 2016/06/21 18:32:09, sievers wrote:
> > On 2016/06/21 06:22:03, Jinsuk wrote:
> > > Thanks for reviewing!
> > > 
> > > After lgtm'ed, fixed a bug in InterceptDownloadResourceThrottle caused by
> the
> > > misunderstanding on how ThrottlingResourceHandler works. Resume() does not
> > cause
> > > the throttle to be invoked again - it simply holds off calling of the next
> > > throttle. So the download processing should be completed within 
> > > InterceptDownloadResourceThrottle::ProcessDownloadRequest(). |defer|
should
> > > still be set to true to ensure the throttle instance keeps alive while
async
> > > call is made. Resume() needs not calling since we already have Cancel(). 
> > > 
> > 
> > 
> > Hmm really?
> > 
> > After WillProcessResponse() is called the first time and we set |defer| it
> will
> > set |deferred_stage_| to |DEFERRED_RESPONSE| in
> > ThrottlingResourceHandler::OnResponseStarted().
> > 
> > In ThrottlingResourceHandler::Resume() it would therefore call
> ResumeResponse()
> > which is the same function as before that triggers WillProcessResponse().
> > 
> > For comparison also see DownloadResourceThrottle::ContinueDownload() which
> calls
> > either Resume() or Cancel().
> 
> Ah, you are right I missed this:
>    if (*defer) {
>      ...
>      return true;  // Do not cancel.
>     }
>   }
> 
>   next_index_ = 0;  // Reset for next time.
> 
> So it continues with the next Throttle when deferred, which in our case is not
> good enough since we actually need to take over the download.
> 
> lgtm!

Thanks for taking another look!

[Jinsuk Kim, 2016-06-21 22:23:31]

The CQ bit was checked by jinsukkim@chromium.org

[Jinsuk Kim, 2016-06-21 22:23:32]

The patchset sent to the CQ was uploaded after l-g-t-m from qinmin@chromium.org,
boliu@chromium.org, tedchoc@chromium.org, sky@chromium.org
Link to the patchset: https://codereview.chromium.org/2014803002/#ps470001
(title: "fix failing tests/bugs")

[commit-bot: I haz the power, 2016-06-21 22:24:05]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2014803002/470001

[commit-bot: I haz the power, 2016-06-21 22:35:04]

Description was changed from

==========
Move DownloadControllerAndroid from content/ to chrome/

This change lets embedder to decide how to handle downloads,
and makes content layer more self-contained. Accompanying changes are:

- Now WebContents holds the reference to download delegate via
WebContentsUserData.
- ContentViewDownloadDelegate interface was removed
- DownloadControlerAndroid -> DownloadControllerBase
- DownloadControlerAndroidImpl -> DownloadController to
  have the name matched with the Java class  
     
BUG=598880,617313, 617361
==========

to

==========
Move DownloadControllerAndroid from content/ to chrome/

This change lets embedder to decide how to handle downloads,
and makes content layer more self-contained. Accompanying changes are:

- Now WebContents holds the reference to download delegate via
WebContentsUserData.
- ContentViewDownloadDelegate interface was removed
- DownloadControlerAndroid -> DownloadControllerBase
- DownloadControlerAndroidImpl -> DownloadController to
  have the name matched with the Java class  
     
BUG=598880,617313, 617361
==========

[commit-bot: I haz the power, 2016-06-21 22:35:06]

Committed patchset #17 (id:470001)

[Jinsuk Kim, 2016-06-21 22:36:19]

Patchset #1 (id:1) has been deleted

[commit-bot: I haz the power, 2016-06-21 22:36:48]

Description was changed from

==========
Move DownloadControllerAndroid from content/ to chrome/

This change lets embedder to decide how to handle downloads,
and makes content layer more self-contained. Accompanying changes are:

- Now WebContents holds the reference to download delegate via
WebContentsUserData.
- ContentViewDownloadDelegate interface was removed
- DownloadControlerAndroid -> DownloadControllerBase
- DownloadControlerAndroidImpl -> DownloadController to
  have the name matched with the Java class  
     
BUG=598880,617313, 617361
==========

to

==========
Move DownloadControllerAndroid from content/ to chrome/

This change lets embedder to decide how to handle downloads,
and makes content layer more self-contained. Accompanying changes are:

- Now WebContents holds the reference to download delegate via
WebContentsUserData.
- ContentViewDownloadDelegate interface was removed
- DownloadControlerAndroid -> DownloadControllerBase
- DownloadControlerAndroidImpl -> DownloadController to
  have the name matched with the Java class

BUG=598880,617313, 617361

Committed: https://crrev.com/3da95ee0a9da46764cc912e3b36f29dd171eab40
Cr-Commit-Position: refs/heads/master@{#401127}
==========

[Jinsuk Kim, 2016-06-21 22:36:49]

Patchset #1 (id:20001) has been deleted

[commit-bot: I haz the power, 2016-06-21 22:36:51]

Patchset 16 (id:??) landed as
https://crrev.com/3da95ee0a9da46764cc912e3b36f29dd171eab40
Cr-Commit-Position: refs/heads/master@{#401127}

[Jinsuk Kim, 2016-06-21 22:37:13]

Patchset #1 (id:80001) has been deleted

[Jinsuk Kim, 2016-06-21 22:38:14]

Patchset #2 (id:120001) has been deleted