Move download stuff to download tab helper.

Move download stuff to download tab helper.

BUG=71097
TEST=zero visible change in how the download system works

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=85486

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=85504

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=85762

[Avi (use Gerrit), 2011-05-13 00:16:34]

To the download expert for review, John as FYI.

[jam, 2011-05-13 06:22:02]

http://codereview.chromium.org/6973035/diff/3001/chrome/browser/tab_contents/...
File chrome/browser/tab_contents/tab_util.cc (right):

http://codereview.chromium.org/6973035/diff/3001/chrome/browser/tab_contents/...
chrome/browser/tab_contents/tab_util.cc:22: TabContentsWrapper*
tab_util::GetTabContentsWrapperByID(
note: we want to get rid of GetAsTabContents() since it's a big hack.  So we
should really avoid adding GetTabContentsWrapperByID since it's really code
that'll have to be removed in the future (since GetTabContentsByID itself has to
be removed)

[Avi (use Gerrit), 2011-05-13 13:56:20]

On 2011/05/13 06:22:02, John Abd-El-Malek wrote:
> note: we want to get rid of GetAsTabContents() since it's a big hack.  So we
> should really avoid adding GetTabContentsWrapperByID

I'm not aware of the architectural issue with GetAsTabContents :( I'm trying to
avoid doing the simultaneous fixing while moving stuff from TC to TCW. Is this
something that we need to fix first before doing this CL?

[jam, 2011-05-13 18:23:03]

On 2011/05/13 13:56:20, Avi wrote:
> On 2011/05/13 06:22:02, John Abd-El-Malek wrote:
> > note: we want to get rid of GetAsTabContents() since it's a big hack.  So we
> > should really avoid adding GetTabContentsWrapperByID
> 
> I'm not aware of the architectural issue with GetAsTabContents :( I'm trying
to
> avoid doing the simultaneous fixing while moving stuff from TC to TCW. Is this
> something that we need to fix first before doing this CL?

I would really prefer not adding this function, since by adding it we can be
sure that people will use it.

but it doesn't look like it's really necessary.  Can't you use the TabContents
from the DownloadProcessHandle and then do the static TCW function to get a TC? 
I realize that DPH's function uses GetAsTabContents indirectly, but at least
we're not adding another public function that uses it.

[Avi (use Gerrit), 2011-05-13 18:39:58]

Of course. (New version up; ptal.) When you have a chance I'd like to talk. I'm
not familiar with the whole situation here and would like to be filled in.

[jam, 2011-05-13 18:47:56]

thanks, content lgtm

[Randy Smith (Not in Mondays), 2011-05-13 18:49:59]

On 2011/05/13 00:16:34, Avi wrote:
> To the download expert for review, John as FYI.

Avi: Just a quick heads up that I'm struggling to review a lot of this stuff, so
I'm not sure I'm the right person.  The only file here I consider myself an
expert on is download_manager.cc, and what you're doing there is pretty minimal.
 If John has a good overview of the rest of the system, great, but if you're
counting on me for detailed review you should probably find someone else.

I am planning to review this--I just don't know that I'm going to give you the
coverage you want.

[Avi (use Gerrit), 2011-05-13 18:52:17]

The tricky stuff seems to be the plumbing, who's calling what. Is there someone
else you'd recommend? I tried to keep this as straightforward as I could, for my
own sake :)

[Randy Smith (Not in Mondays), 2011-05-13 18:54:41]

On 2011/05/13 18:52:17, Avi wrote:
> The tricky stuff seems to be the plumbing, who's calling what. Is there
someone
> else you'd recommend? I tried to keep this as straightforward as I could, for
my
> own sake :)

Yeah, I agree, but very little of that is inside the download system proper, and
I don't know the main UI system really at all.  Weren't you the one who created
DownloadTabHelper?

[Randy Smith (Not in Mondays), 2011-05-13 19:01:29]

LGTM, but that's said without confidence that I've done a solid review.  The
changes in chrome/browser/download are fine, and the changes in
chrome/browser/ui/download/ look fine, but I don't feel on top enough of what's
trying to be done to evaluate the architecture of the delegate creation.  I saw
no problems in the other files, but I'm not an expert on them and you shouldn't
take my LGTM as applying to those files.

[Avi (use Gerrit), 2011-05-13 19:12:28]

Yes, I created the DownloadTabHelper, but just as a collection of "everything
involving downloads" from the TabContents. The whole delegate situation is why I
wanted another look; it's already kinda tricky and AFAIK I've recreated it
exactly with the new code.

[Avi (use Gerrit), 2011-05-16 17:48:46]

PTAL: now I have the delegate set properly for the CF.

[jam, 2011-05-16 18:27:32]

SLGTM