Change URL back to the previous one if the typed URL causes file download.

Change URL back to the previous one if the typed URL causes file download.

This is the successor to http://codereview.chromium.org/115704

TESTED=manually

TEST=Type any URL that causes download, e.g.,
ftp://ftp.vim.org/pub/vim/pc/gvim72.zip into the omnibox.
Without this change the URL remains there. With this change,
the URL is changed back to the original one.

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

[Yuzo, 2009-06-17 10:25:56]

Hi, Brett,

Thank you for the review.

Question:
To send an IPC message (I'd call it ViewHostMsg_DownloadStarted) from
BufferedResourceHandler,
can/should I pass receiver_ from ResourceDispatcherHost
and call receiver->Send(new ViewHostMsg_DownloadStarted(...)) ?

In other words, what is the right way to send an IPC message
from BufferedResourceHandler?

Yuzo

[Yuzo, 2009-06-17 10:26:41]

Hi, Brett,

Thank you for the review.

Question:
To send an IPC message (I'd call it ViewHostMsg_DownloadStarted) from
BufferedResourceHandler,
can/should I pass receiver_ from ResourceDispatcherHost
and call receiver->Send(new ViewHostMsg_DownloadStarted(...)) ?

In other words, what is the right way to send an IPC message
from BufferedResourceHandler?

Yuzo

[brettw, 2009-06-17 17:07:07]

I believe receiver->Send actually sends to the renderer not to the UI thread.

I found something like what you want to do:
ResourceDispatcherHost::OnClosePageACK
You can see it calls a static function on RVH which then looks up the
appropriate object to call a non-static method on.

That non-static function would just forward to the RenderViewHostDelegate which
is implemented by TabContents.

[Yuzo, 2009-06-18 03:15:55]

Hi, Brett,

Sorry to say this after having had you spend much time on this,
but I noticed that this can be done much simpler: 3-line change.

Can you take a look?

Yuzo

On 2009/06/17 17:07:07, brettw wrote:
> I believe receiver->Send actually sends to the renderer not to the UI thread.
> 
> I found something like what you want to do:
> ResourceDispatcherHost::OnClosePageACK
> You can see it calls a static function on RVH which then looks up the
> appropriate object to call a non-static method on.
> 
> That non-static function would just forward to the RenderViewHostDelegate
which
> is implemented by TabContents.

[brettw, 2009-06-18 05:49:30]

Ah! We both missed the easy solution.

I noticed something when I was researching this:
chrome/browser/download/save_package.cc
calls the DidStartDownload function. It is currently not called on Mac. I think
that's wrong, since TabContents is cross-platform (probably this was leftover
from when Tabcontents wasn't ported).

Can you remove the ifdef in that file too?

http://codereview.chromium.org/126266/diff/2001/2002
File chrome/browser/tab_contents/tab_contents.cc (right):

http://codereview.chromium.org/126266/diff/2001/2002#newcode906
Line 906: if (delegate()) {
The rest of this file doesn't use {} around single-line statrments. Can you
remove them to match?

[Yuzo, 2009-06-18 06:02:04]

Brett,

Thank you for the review. I've uncommented the ifdef and
now running gcl try.

Yuzo

http://codereview.chromium.org/126266/diff/2001/2002
File chrome/browser/tab_contents/tab_contents.cc (right):

http://codereview.chromium.org/126266/diff/2001/2002#newcode906
Line 906: if (delegate()) {
On 2009/06/18 05:49:30, brettw wrote:
> The rest of this file doesn't use {} around single-line statrments. Can you
> remove them to match?

Sure, done.

[brettw, 2009-06-18 19:53:10]

LGTM. Do you need me to check this in?

[Yuzo, 2009-06-19 04:25:59]

Thank you. I've just checked this in myself.

On 2009/06/18 19:53:10, brettw wrote:
> LGTM. Do you need me to check this in?