Support dragging a virtual file out of the browser.

Support dragging a virtual file out of the browser.

BUG=none
TEST=none


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

[darin (slow to review), 2009-11-04 17:07:39]

http://codereview.chromium.org/351029/diff/1/5
File app/os_exchange_data_provider_win.cc (right):

http://codereview.chromium.org/351029/diff/1/5#newcode246
Line 246: return static_cast<const OSExchangeDataProviderWin&>(data.provider()).
nit: please static_cast using pointers instead of reference types.  casting a
reference type can result in constructors being called in some cases, so it is
usually best to only cast pointers.

  return static_cast<const OSExchangeDataProviderWin*>(&data.provider())->
      async_operation();

http://codereview.chromium.org/351029/diff/1/5#newcode666
Line 666: bool is_left_button_down = (GetAsyncKeyState(VK_LBUTTON) & 0x8000) !=
0;
do you really need the key state right now, or is it enough to just know the key
state for the last input event?  if the system is bogged down then
GetAsyncKeyState might not do what you want.  what you probably want is to know
the key state (GetKeyState) when the current event, that trigged the
drag-n-drop, was being processed.

http://codereview.chromium.org/351029/diff/1/5#newcode678
Line 678: // If the left button is up and the target has not request the data
nit: "...has not request[ed] the data..."

http://codereview.chromium.org/351029/diff/1/5#newcode701
Line 701: MessageLoop::current()->Run();
what does the call stack look like leading up to DataObjectImpl::GetData.  are
you sure that everything on the call stack is OK being possibly re-entered? 
(nested message loops are very scary!)

http://codereview.chromium.org/351029/diff/1/5#newcode924
Line 924: wchar_t* data = reinterpret_cast<wchar_t*>((BYTE*)(drop_files) +
drop_size);
please use reinterpret_cast<BYTE*> instead.

http://codereview.chromium.org/351029/diff/1/6
File app/os_exchange_data_provider_win.h (right):

http://codereview.chromium.org/351029/diff/1/6#newcode91
Line 91: for (size_t i = 0; i < downloads.size(); ++i)
nit: please use {}'s on this for loop since its body is multi-line.

http://codereview.chromium.org/351029/diff/1/30
File chrome/browser/download/download_manager.cc (right):

http://codereview.chromium.org/351029/diff/1/30#newcode1131
Line 1131: path,
nit: can you just pass FilePath() as the parameter?

http://codereview.chromium.org/351029/diff/1/20
File chrome/browser/drag_download_file.cc (right):

http://codereview.chromium.org/351029/diff/1/20#newcode30
Line 30: file_util::MoveAfterReboot(dir_path_, to_path);
nit: just pass FilePath() as the second arg.

http://codereview.chromium.org/351029/diff/1/19
File chrome/browser/drag_download_stream_win.cc (right):

http://codereview.chromium.org/351029/diff/1/19#newcode66
Line 66: file_util::Delete(dir_path_, false);
hmm... what thread does this destructor run on?  you should definitely avoid
doing any blocking file i/o on either the UI thread or the IO thread.

perhaps you should ensure that this class is deleted on the FILE thread.  see
jam's recent post to chromium-dev about how best to do this.

http://codereview.chromium.org/351029/diff/1/19#newcode90
Line 90: IStream* DragDownloadFileStream::GetStream() {
please add assertions on each method to ensure that they get called on the
proper threads.

for example:
DCHECK(ChromeThread::CurrentlyOn(ChromeThread::UI));

http://codereview.chromium.org/351029/diff/1/19#newcode108
Line 108: request_= new URLRequest(url_, this);
it looks like you are creating URLRequest on the same thread that you are using
TabContents and Profile on.  that can't be right.  URLRequest should only be
used on the IO thread, and TabContents and Profile should only be used on the UI
thread.

http://codereview.chromium.org/351029/diff/1/19#newcode148
Line 148: void DragDownloadFileStream::OnAuthRequired(URLRequest* request,
i'm not sure why you need to override the default OnAuthRequired or
OnSSLCertificateError.  the default behavior should cause OnResponseStarted to
be triggered where you should observe an unsuccessful request.

http://codereview.chromium.org/351029/diff/1/19#newcode262
Line 262: MessageLoop::current()->Run();
more nested message loops :(

can we avoid these somehow?  can we try to not begin the drag-n-drop process
until we have downloaded the response headers?

my concern is that this could stuff up the chrome UI for a long time if the
network request is slow.

http://codereview.chromium.org/351029/diff/1/18
File chrome/browser/drag_download_stream_win.h (right):

http://codereview.chromium.org/351029/diff/1/18#newcode9
Line 9: #include <objidl.h>
nit: insert a new line below here.

http://codereview.chromium.org/351029/diff/1/18#newcode21
Line 21: FilePath GetZoneDataStreamName(const std::wstring& file_name);
it would be nice to put these utility functions in a class:

class ZoneData {
 public:
  ... GetStreamName ...
  ... GetStreamContent ...
  ... GetStreamSize ...
};

http://codereview.chromium.org/351029/diff/1/17
File chrome/browser/views/tab_contents/tab_contents_view_win.cc (right):

http://codereview.chromium.org/351029/diff/1/17#newcode134
Line 134: if (drop_data.need_to_download_url) {
this function is now way too big.  please break it up into sub-functions.

http://codereview.chromium.org/351029/diff/1/9
File webkit/api/public/WebDragData.h (right):

http://codereview.chromium.org/351029/diff/1/9#newcode70
Line 70: WEBKIT_API bool needToDownloadUrl() const;
nit: needToDownloadURL, setNeedToDownloadURL

see the webkit style guide regarding names with acronyms.

http://codereview.chromium.org/351029/diff/1/8
File webkit/api/src/WebDragData.cpp (right):

http://codereview.chromium.org/351029/diff/1/8#newcode94
Line 94: return m_private->needToDownloadUrl;
looks like ChromiumDataObject also has its member mis-named.

http://codereview.chromium.org/351029/diff/1/10
File webkit/glue/webdropdata.h (right):

http://codereview.chromium.org/351029/diff/1/10#newcode40
Line 40: bool need_to_download_url;
does this really belong in a section titled "User is dragging a link into the
webview"?

this feature is about dragging items (files) out of the webview, right?

[jianli, 2009-11-06 02:47:06]

http://codereview.chromium.org/351029/diff/1/5
File app/os_exchange_data_provider_win.cc (right):

http://codereview.chromium.org/351029/diff/1/5#newcode246
Line 246: return static_cast<const OSExchangeDataProviderWin&>(data.provider()).
On 2009/11/04 17:07:40, darin wrote:
> nit: please static_cast using pointers instead of reference types.  casting a
> reference type can result in constructors being called in some cases, so it is
> usually best to only cast pointers.
> 
>   return static_cast<const OSExchangeDataProviderWin*>(&data.provider())->
>       async_operation();

Never thought about this when I copied and pasted. Good to know that. Fixed this
function plus the original one: OSExchangeDataProviderWin::GetIDataObject.

http://codereview.chromium.org/351029/diff/1/5#newcode666
Line 666: bool is_left_button_down = (GetAsyncKeyState(VK_LBUTTON) & 0x8000) !=
0;
On 2009/11/04 17:07:40, darin wrote:
> do you really need the key state right now, or is it enough to just know the
key
> state for the last input event?  if the system is bogged down then
> GetAsyncKeyState might not do what you want.  what you probably want is to
know
> the key state (GetKeyState) when the current event, that trigged the
> drag-n-drop, was being processed.

Changed to use GetKeyState.

http://codereview.chromium.org/351029/diff/1/5#newcode678
Line 678: // If the left button is up and the target has not request the data
On 2009/11/04 17:07:40, darin wrote:
> nit: "...has not request[ed] the data..."

Done.

http://codereview.chromium.org/351029/diff/1/5#newcode701
Line 701: MessageLoop::current()->Run();
On 2009/11/04 17:07:40, darin wrote:
> what does the call stack look like leading up to DataObjectImpl::GetData.  are
> you sure that everything on the call stack is OK being possibly re-entered? 
> (nested message loops are very scary!)

I do not like nested message loop either. However, I do not find other good way
to avoid this:
 * in streaming mode, we need to wait till the response header is fetched.
 * in non-streaming mode, we need to wait till the whole file is downloaded.

We do not want to download the data before the drop happens because the
drag-and-drop can be canceled or the drop target might be invalid. I do not see
a way to tell OLE that the drop data is ready.

If we have to use nested message loop, we need to do our best to make it less
scary. I found out and fixed the problem that the application could not be
terminated if we close the window when drag download was still in progress due
to slow network request. I introduced InnerMessageLoopHelper in message_loop.*
to encapsulate the logic to run and quit message loop.

The call stack is like the following:
  DataObjectImpl::GetData
  ole32.dll!...
  ...
  user32.dll!...
  AcceleratorHandler::Dispatch
  MessagePumpForUI::ProcessMessageHelper
  MessagePumpForUI::ProcessPumpReplacementMessage
  MessagePumpForUI::ProcessNextWindowsMessage
  MessagePumpForUI::DoRunLoop
  ...

http://codereview.chromium.org/351029/diff/1/5#newcode924
Line 924: wchar_t* data = reinterpret_cast<wchar_t*>((BYTE*)(drop_files) +
drop_size);
On 2009/11/04 17:07:40, darin wrote:
> please use reinterpret_cast<BYTE*> instead.

Done.

http://codereview.chromium.org/351029/diff/1/6
File app/os_exchange_data_provider_win.h (right):

http://codereview.chromium.org/351029/diff/1/6#newcode91
Line 91: for (size_t i = 0; i < downloads.size(); ++i)
On 2009/11/04 17:07:40, darin wrote:
> nit: please use {}'s on this for loop since its body is multi-line.

Done.

http://codereview.chromium.org/351029/diff/1/30
File chrome/browser/download/download_manager.cc (right):

http://codereview.chromium.org/351029/diff/1/30#newcode1131
Line 1131: path,
On 2009/11/04 17:07:40, darin wrote:
> nit: can you just pass FilePath() as the parameter?

Done.

http://codereview.chromium.org/351029/diff/1/20
File chrome/browser/drag_download_file.cc (right):

http://codereview.chromium.org/351029/diff/1/20#newcode30
Line 30: file_util::MoveAfterReboot(dir_path_, to_path);
On 2009/11/04 17:07:40, darin wrote:
> nit: just pass FilePath() as the second arg.

Done.

http://codereview.chromium.org/351029/diff/1/19
File chrome/browser/drag_download_stream_win.cc (right):

http://codereview.chromium.org/351029/diff/1/19#newcode66
Line 66: file_util::Delete(dir_path_, false);
On 2009/11/04 17:07:40, darin wrote:
> hmm... what thread does this destructor run on?  you should definitely avoid
> doing any blocking file i/o on either the UI thread or the IO thread.
> 
> perhaps you should ensure that this class is deleted on the FILE thread.  see
> jam's recent post to chromium-dev about how best to do this.

The destructor is called on UI thread.

This is the left over I forgot to get rid of. Remove it since it is not used any
more.

http://codereview.chromium.org/351029/diff/1/19#newcode90
Line 90: IStream* DragDownloadFileStream::GetStream() {
On 2009/11/04 17:07:40, darin wrote:
> please add assertions on each method to ensure that they get called on the
> proper threads.
> 
> for example:
> DCHECK(ChromeThread::CurrentlyOn(ChromeThread::UI));

Done.

http://codereview.chromium.org/351029/diff/1/19#newcode108
Line 108: request_= new URLRequest(url_, this);
On 2009/11/04 17:07:40, darin wrote:
> it looks like you are creating URLRequest on the same thread that you are
using
> TabContents and Profile on.  that can't be right.  URLRequest should only be
> used on the IO thread, and TabContents and Profile should only be used on the
UI
> thread.

OnInitiateDownload is called on IO thread (see DragDownloadFileStream::Start). I
changed not to use TabContents in OnInitiateDownload.

http://codereview.chromium.org/351029/diff/1/19#newcode148
Line 148: void DragDownloadFileStream::OnAuthRequired(URLRequest* request,
On 2009/11/04 17:07:40, darin wrote:
> i'm not sure why you need to override the default OnAuthRequired or
> OnSSLCertificateError.  the default behavior should cause OnResponseStarted to
> be triggered where you should observe an unsuccessful request.

Done.

http://codereview.chromium.org/351029/diff/1/19#newcode262
Line 262: MessageLoop::current()->Run();
On 2009/11/04 17:07:40, darin wrote:
> more nested message loops :(
> 
> can we avoid these somehow?  can we try to not begin the drag-n-drop process
> until we have downloaded the response headers?
> 
> my concern is that this could stuff up the chrome UI for a long time if the
> network request is slow.

No. I do not think we can avoid this nested message loop. Here we are waiting to
get more data from the sever and then fill to the streaming read buffer and
repeat it until the buffer is full.

This is ugly. But I do not find out a good way to do this. I have run the test
to simulate when network request is slow, Chrome UI is still well responding and
we can do all other things without any problem, including initiating any
drag-out download.

http://codereview.chromium.org/351029/diff/1/18
File chrome/browser/drag_download_stream_win.h (right):

http://codereview.chromium.org/351029/diff/1/18#newcode9
Line 9: #include <objidl.h>
On 2009/11/04 17:07:40, darin wrote:
> nit: insert a new line below here.

Done.

http://codereview.chromium.org/351029/diff/1/18#newcode21
Line 21: FilePath GetZoneDataStreamName(const std::wstring& file_name);
On 2009/11/04 17:07:40, darin wrote:
> it would be nice to put these utility functions in a class:
> 
> class ZoneData {
>  public:
>   ... GetStreamName ...
>   ... GetStreamContent ...
>   ... GetStreamSize ...
> };

Done.

http://codereview.chromium.org/351029/diff/1/17
File chrome/browser/views/tab_contents/tab_contents_view_win.cc (right):

http://codereview.chromium.org/351029/diff/1/17#newcode134
Line 134: if (drop_data.need_to_download_url) {
On 2009/11/04 17:07:40, darin wrote:
> this function is now way too big.  please break it up into sub-functions.

Done.

http://codereview.chromium.org/351029/diff/1/9
File webkit/api/public/WebDragData.h (right):

http://codereview.chromium.org/351029/diff/1/9#newcode70
Line 70: WEBKIT_API bool needToDownloadUrl() const;
On 2009/11/04 17:07:40, darin wrote:
> nit: needToDownloadURL, setNeedToDownloadURL
> 
> see the webkit style guide regarding names with acronyms.

Done. Also changed the name to downloadURL and setDownloadURL and use the type
WebURL. This is to address the feedback from the corresponding upstream patch.

http://codereview.chromium.org/351029/diff/1/8
File webkit/api/src/WebDragData.cpp (right):

http://codereview.chromium.org/351029/diff/1/8#newcode94
Line 94: return m_private->needToDownloadUrl;
On 2009/11/04 17:07:40, darin wrote:
> looks like ChromiumDataObject also has its member mis-named.

Done.

http://codereview.chromium.org/351029/diff/1/10
File webkit/glue/webdropdata.h (right):

http://codereview.chromium.org/351029/diff/1/10#newcode40
Line 40: bool need_to_download_url;
On 2009/11/04 17:07:40, darin wrote:
> does this really belong in a section titled "User is dragging a link into the
> webview"?
> 
> this feature is about dragging items (files) out of the webview, right?

Done.

[jianli, 2009-12-11 00:10:57]

Changed to use background thread to do drag-and-drop as we discussed. Move all
drag-related codes in TabContentViewWin to a new class TabContentsDragWin.
Please review again. Thanks.

[darin (slow to review), 2009-12-15 01:00:33]

Here's some review comments for changes in base/.  I'm sorry for the delay in
getting feedback to you.  I need a large block of time to review such a large
and complex CL.  In the future, please break up your CLs into smaller more
digestible chunks.

http://codereview.chromium.org/351029/diff/7001/7014
File base/message_loop.h (right):

http://codereview.chromium.org/351029/diff/7001/7014#newcode520
base/message_loop.h:520: class InnerMessageLoopHelper {
this class makes me very nervous.  why does it need to be a friend of
MessageLoop?  you had similar code before that didn't need to be a friend.

http://codereview.chromium.org/351029/diff/11001/12009
File base/base_drop_target.h (right):

http://codereview.chromium.org/351029/diff/11001/12009#newcode34
base/base_drop_target.h:34: bool suspend() const { return suspend_; }
"suspend" is an awkward name for this attribute.  it sounds like a command. 
"suspended" would be better, but that of course means that set_suspend would
need to be changed to set_suspended.  and suspend_ would need to be changed to
suspended_, etc.

http://codereview.chromium.org/351029/diff/11001/12011
File base/file_util.h (right):

http://codereview.chromium.org/351029/diff/11001/12011#newcode139
base/file_util.h:139: // the operating system is restarted. Pass empty to_path
to mean the deletion.
nit: "to mean deletion" ... drop the "the"

http://codereview.chromium.org/351029/diff/11001/12011#newcode140
base/file_util.h:140: bool MoveAfterReboot(const FilePath& from_path, const
FilePath& to_path);
it seems like you only require "delete after reboot" functionality.  perhaps you
should only implement DeleteAfterReboot(const FilePath&) for now.  we can add
MoveAfterReboot later if we need that.

http://codereview.chromium.org/351029/diff/11001/12010
File base/file_util_win.cc (right):

http://codereview.chromium.org/351029/diff/11001/12010#newcode131
base/file_util_win.cc:131: return MoveFileEx(from_path.value().c_str(),
to_path_str,
do we know that we will only reach here for file paths that live on the local
filesystem?

From MSDN: "If dwFlags specifies MOVEFILE_DELAY_UNTIL_REBOOT, the file cannot
exist on a remote share"

http://codereview.chromium.org/351029/diff/11001/12013
File base/message_pump_win.cc (right):

http://codereview.chromium.org/351029/diff/11001/12013#newcode336
base/message_pump_win.cc:336: if (!CallMsgFilter(&msg, kMessageFilterCode))
ProcessNextWindowsMessage should return true if any work was done.  this code
makes it sometimes return false, which seems wrong to me.

it seems to me that CallMsgFilter should be called inside ProcessMessageHelper
just before the call to WillProcessMessage.

also, it seems that if CallMsgFilter returns non-zero, that ProcessMessageHelper
should return true.  work was done.

(the work done signal is important to the functioning of the message pump.)

would it be possible to implement a unit test for this feature?

http://codereview.chromium.org/351029/diff/11001/12020
File chrome/browser/drag_download_file.cc (right):

http://codereview.chromium.org/351029/diff/11001/12020#newcode32
chrome/browser/drag_download_file.cc:32: file_util::MoveAfterReboot(dir_path_,
FilePath());
This method only works for ordinary files or empty directories according to
MSDN.

[jianli, 2009-12-15 19:37:48]

http://codereview.chromium.org/351029/diff/7001/7014
File base/message_loop.h (right):

http://codereview.chromium.org/351029/diff/7001/7014#newcode520
base/message_loop.h:520: class InnerMessageLoopHelper {
On 2009/12/15 01:00:33, darin wrote:
> this class makes me very nervous.  why does it need to be a friend of
> MessageLoop?  you had similar code before that didn't need to be a friend.

This was in the old patch and it has been removed since we do not need something
like this any more.

http://codereview.chromium.org/351029/diff/11001/12009
File base/base_drop_target.h (right):

http://codereview.chromium.org/351029/diff/11001/12009#newcode34
base/base_drop_target.h:34: bool suspend() const { return suspend_; }
On 2009/12/15 01:00:33, darin wrote:
> "suspend" is an awkward name for this attribute.  it sounds like a command. 
> "suspended" would be better, but that of course means that set_suspend would
> need to be changed to set_suspended.  and suspend_ would need to be changed to
> suspended_, etc.

Done.

http://codereview.chromium.org/351029/diff/11001/12011
File base/file_util.h (right):

http://codereview.chromium.org/351029/diff/11001/12011#newcode139
base/file_util.h:139: // the operating system is restarted. Pass empty to_path
to mean the deletion.
On 2009/12/15 01:00:33, darin wrote:
> nit: "to mean deletion" ... drop the "the"

Done.

http://codereview.chromium.org/351029/diff/11001/12011#newcode140
base/file_util.h:140: bool MoveAfterReboot(const FilePath& from_path, const
FilePath& to_path);
On 2009/12/15 01:00:33, darin wrote:
> it seems like you only require "delete after reboot" functionality.  perhaps
you
> should only implement DeleteAfterReboot(const FilePath&) for now.  we can add
> MoveAfterReboot later if we need that.

Done.

http://codereview.chromium.org/351029/diff/11001/12010
File base/file_util_win.cc (right):

http://codereview.chromium.org/351029/diff/11001/12010#newcode131
base/file_util_win.cc:131: return MoveFileEx(from_path.value().c_str(),
to_path_str,
On 2009/12/15 01:00:33, darin wrote:
> do we know that we will only reach here for file paths that live on the local
> filesystem?
> 
> From MSDN: "If dwFlags specifies MOVEFILE_DELAY_UNTIL_REBOOT, the file cannot
> exist on a remote share"

We do not know. I added the comment in the function declaration for this.

http://codereview.chromium.org/351029/diff/11001/12013
File base/message_pump_win.cc (right):

http://codereview.chromium.org/351029/diff/11001/12013#newcode336
base/message_pump_win.cc:336: if (!CallMsgFilter(&msg, kMessageFilterCode))
On 2009/12/15 01:00:33, darin wrote:
> ProcessNextWindowsMessage should return true if any work was done.  this code
> makes it sometimes return false, which seems wrong to me.
> 
> it seems to me that CallMsgFilter should be called inside ProcessMessageHelper
> just before the call to WillProcessMessage.
> 
> also, it seems that if CallMsgFilter returns non-zero, that
ProcessMessageHelper
> should return true.  work was done.
> 
> (the work done signal is important to the functioning of the message pump.)
> 
> would it be possible to implement a unit test for this feature?

Fixed. Added a unit test DispatcherWithMessageHook for it.

Also fixed a flakiness in RunTest_Dispatcher that is caused by unexpected
WM_TIMER message during the dispatch count.

http://codereview.chromium.org/351029/diff/11001/12020
File chrome/browser/drag_download_file.cc (right):

http://codereview.chromium.org/351029/diff/11001/12020#newcode32
chrome/browser/drag_download_file.cc:32: file_util::MoveAfterReboot(dir_path_,
FilePath());
On 2009/12/15 01:00:33, darin wrote:
> This method only works for ordinary files or empty directories according to
> MSDN.

Thanks for noticing this. Changed to remove the file first and then the
directory.

[darin (slow to review), 2009-12-15 21:29:21]

I made it down to chrome/browser/drag_download_file.h.  I'll pick up from there
next.

http://codereview.chromium.org/351029/diff/16001/17006
File app/os_exchange_data.h (right):

http://codereview.chromium.org/351029/diff/16001/17006#newcode65
app/os_exchange_data.h:65: struct DownloadFileInfo;
nit: I think this forward declaration of DownloadFileInfo should be above the
comment since the comment refers to DownloadFileObserver.

http://codereview.chromium.org/351029/diff/16001/17006#newcode70
app/os_exchange_data.h:70: const std::vector<OSExchangeData::DownloadFileInfo*>&
downloads) = 0;
nit: no need for OSExchangeData:: prefix

http://codereview.chromium.org/351029/diff/16001/17006#newcode138
app/os_exchange_data.h:138: virtual void SetDownloadFilename(DownloadFileInfo*
download) = 0;
nit: *FileName instead of *Filename

http://codereview.chromium.org/351029/diff/16001/17004
File app/os_exchange_data_provider_win.cc (right):

http://codereview.chromium.org/351029/diff/16001/17004#newcode33
app/os_exchange_data_provider_win.cc:33: static STGMEDIUM*
GetStorageForFilename(const std::wstring& full_path);
nit: *FileName instead of *Filename

http://codereview.chromium.org/351029/diff/16001/17004#newcode930
app/os_exchange_data_provider_win.cc:930: const size_t drop_size =
sizeof(DROPFILES);
nit: kDropSize

http://codereview.chromium.org/351029/diff/16001/17005
File app/os_exchange_data_provider_win.h (right):

http://codereview.chromium.org/351029/diff/16001/17005#newcode20
app/os_exchange_data_provider_win.h:20: public:
please add a virtual destructor.  if possible, make it protected.

http://codereview.chromium.org/351029/diff/16001/17011
File base/file_util.h (right):

http://codereview.chromium.org/351029/diff/16001/17011#newcode136
base/file_util.h:136: // 2) The directory to be deleted must be empty.
To address issue #1, you might say that this function should only be used to
delete files in the temp folder since that folder is likely on the local
filesystem.

http://codereview.chromium.org/351029/diff/16001/17010
File base/file_util_win.cc (right):

http://codereview.chromium.org/351029/diff/16001/17010#newcode105
base/file_util_win.cc:105: return MoveFileEx(path.value().c_str(), L"",
I think the second parameter here should be NULL.

From MSDN:
"If dwFlags specifies MOVEFILE_DELAY_UNTIL_REBOOT and lpNewFileName is NULL,
MoveFileEx registers the lpExistingFileName file to be deleted when the system
restarts."

http://codereview.chromium.org/351029/diff/16001/17010#newcode107
base/file_util_win.cc:107: MOVEFILE_REPLACE_EXISTING) != 0;
nit: The return type is BOOL, so you should compare against FALSE instead of 0.

http://codereview.chromium.org/351029/diff/16001/17044
File chrome/browser/download/download_exe.cc (right):

http://codereview.chromium.org/351029/diff/16001/17044#newcode163
chrome/browser/download/download_exe.cc:163: if (extension == g_executables[i])
Perhaps this code should use LowerCaseEqualsASCII?  It would be a shame if we
had bugs where "EXE" failed to be recognized as an executable extension.

http://codereview.chromium.org/351029/diff/16001/17030
File chrome/browser/download/download_manager.cc (right):

http://codereview.chromium.org/351029/diff/16001/17030#newcode1260
chrome/browser/download/download_manager.cc:1260: generated_name);
nit: indentation off by one

http://codereview.chromium.org/351029/diff/16001/17030#newcode1449
chrome/browser/download/download_manager.cc:1449:
StringToLowerASCII(&ascii_extension);
maybe we could just eliminate this call if IsExecutableExtension uses
LowerCaseEqualsASCII.  that way if we have future callers of
IsExecutableExtension, they won't need to also remember to lowercase the input
string.

http://codereview.chromium.org/351029/diff/16001/17031
File chrome/browser/download/download_manager.h (right):

http://codereview.chromium.org/351029/diff/16001/17031#newcode486
chrome/browser/download/download_manager.h:486: static void
GenerateSafeFilename(const std::string& mime_type,
nit: this should also be *FileName instead of *Filename

http://codereview.chromium.org/351029/diff/16001/17031#newcode499
chrome/browser/download/download_manager.h:499: static void
GenerateFilename(const GURL& url,
ditto

[jianli, 2009-12-15 22:50:21]

http://codereview.chromium.org/351029/diff/16001/17006
File app/os_exchange_data.h (right):

http://codereview.chromium.org/351029/diff/16001/17006#newcode65
app/os_exchange_data.h:65: struct DownloadFileInfo;
On 2009/12/15 21:29:22, darin wrote:
> nit: I think this forward declaration of DownloadFileInfo should be above the
> comment since the comment refers to DownloadFileObserver.

Done.

http://codereview.chromium.org/351029/diff/16001/17006#newcode70
app/os_exchange_data.h:70: const std::vector<OSExchangeData::DownloadFileInfo*>&
downloads) = 0;
On 2009/12/15 21:29:22, darin wrote:
> nit: no need for OSExchangeData:: prefix

Done.

http://codereview.chromium.org/351029/diff/16001/17006#newcode138
app/os_exchange_data.h:138: virtual void SetDownloadFilename(DownloadFileInfo*
download) = 0;
On 2009/12/15 21:29:22, darin wrote:
> nit: *FileName instead of *Filename

Done.

http://codereview.chromium.org/351029/diff/16001/17004
File app/os_exchange_data_provider_win.cc (right):

http://codereview.chromium.org/351029/diff/16001/17004#newcode930
app/os_exchange_data_provider_win.cc:930: const size_t drop_size =
sizeof(DROPFILES);
On 2009/12/15 21:29:22, darin wrote:
> nit: kDropSize

Done.

http://codereview.chromium.org/351029/diff/16001/17005
File app/os_exchange_data_provider_win.h (right):

http://codereview.chromium.org/351029/diff/16001/17005#newcode20
app/os_exchange_data_provider_win.h:20: public:
On 2009/12/15 21:29:22, darin wrote:
> please add a virtual destructor.  if possible, make it protected.

Done.

http://codereview.chromium.org/351029/diff/16001/17011
File base/file_util.h (right):

http://codereview.chromium.org/351029/diff/16001/17011#newcode136
base/file_util.h:136: // 2) The directory to be deleted must be empty.
On 2009/12/15 21:29:22, darin wrote:
> To address issue #1, you might say that this function should only be used to
> delete files in the temp folder since that folder is likely on the local
> filesystem.

Done.

http://codereview.chromium.org/351029/diff/16001/17010
File base/file_util_win.cc (right):

http://codereview.chromium.org/351029/diff/16001/17010#newcode105
base/file_util_win.cc:105: return MoveFileEx(path.value().c_str(), L"",
On 2009/12/15 21:29:22, darin wrote:
> I think the second parameter here should be NULL.
> 
> From MSDN:
> "If dwFlags specifies MOVEFILE_DELAY_UNTIL_REBOOT and lpNewFileName is NULL,
> MoveFileEx registers the lpExistingFileName file to be deleted when the system
> restarts."

Done.

http://codereview.chromium.org/351029/diff/16001/17010#newcode107
base/file_util_win.cc:107: MOVEFILE_REPLACE_EXISTING) != 0;
On 2009/12/15 21:29:22, darin wrote:
> nit: The return type is BOOL, so you should compare against FALSE instead of
0.

Done.

http://codereview.chromium.org/351029/diff/16001/17044
File chrome/browser/download/download_exe.cc (right):

http://codereview.chromium.org/351029/diff/16001/17044#newcode163
chrome/browser/download/download_exe.cc:163: if (extension == g_executables[i])
On 2009/12/15 21:29:22, darin wrote:
> Perhaps this code should use LowerCaseEqualsASCII?  It would be a shame if we
> had bugs where "EXE" failed to be recognized as an executable extension.

Done.

http://codereview.chromium.org/351029/diff/16001/17030
File chrome/browser/download/download_manager.cc (right):

http://codereview.chromium.org/351029/diff/16001/17030#newcode1260
chrome/browser/download/download_manager.cc:1260: generated_name);
On 2009/12/15 21:29:22, darin wrote:
> nit: indentation off by one

Done.

http://codereview.chromium.org/351029/diff/16001/17030#newcode1449
chrome/browser/download/download_manager.cc:1449:
StringToLowerASCII(&ascii_extension);
On 2009/12/15 21:29:22, darin wrote:
> maybe we could just eliminate this call if IsExecutableExtension uses
> LowerCaseEqualsASCII.  that way if we have future callers of
> IsExecutableExtension, they won't need to also remember to lowercase the input
> string.

Good thought. Done.

http://codereview.chromium.org/351029/diff/16001/17031
File chrome/browser/download/download_manager.h (right):

http://codereview.chromium.org/351029/diff/16001/17031#newcode486
chrome/browser/download/download_manager.h:486: static void
GenerateSafeFilename(const std::string& mime_type,
On 2009/12/15 21:29:22, darin wrote:
> nit: this should also be *FileName instead of *Filename

It seems that we have mixed used of FileName and Filename across our code base.
In addition to fixing this one, I also fixed the one in
SavePackage::GenerateFilename.

http://codereview.chromium.org/351029/diff/16001/17031#newcode499
chrome/browser/download/download_manager.h:499: static void
GenerateFilename(const GURL& url,
On 2009/12/15 21:29:22, darin wrote:
> ditto

Done.

[jianli, 2009-12-16 02:10:20]

Forgot to include 2 new files tab_contents_drag_win.* in my last patch. Updated
the patch.

[darin (slow to review), 2009-12-17 01:03:44]

OK, I reviewed up until chrome/browser/tab_contents/web_drag_source_win.cc.

http://codereview.chromium.org/351029/diff/18022/20091
File base/file_util_win.cc (right):

http://codereview.chromium.org/351029/diff/18022/20091#newcode107
base/file_util_win.cc:107: MOVEFILE_REPLACE_EXISTING) == TRUE;
nit: comparing to TRUE can be problematic since any non-zero value can be
considered "true"... x != FALSE is usually better.

http://codereview.chromium.org/351029/diff/18022/20111
File chrome/browser/download/download_manager.cc (right):

http://codereview.chromium.org/351029/diff/18022/20111#newcode1254
chrome/browser/download/download_manager.cc:1254: void
DownloadManager::GenerateFileName(DownloadCreateInfo* info,
the google style guide says to not overload method names.  maybe you could name
this one GenerateFileNameFromInfo.

http://codereview.chromium.org/351029/diff/18022/20102
File chrome/browser/drag_download_file.cc (right):

http://codereview.chromium.org/351029/diff/18022/20102#newcode27
chrome/browser/drag_download_file.cc:27: DragDownloadFile::~DragDownloadFile() {
assert that this is called on the DnD thread.

http://codereview.chromium.org/351029/diff/18022/20102#newcode43
chrome/browser/drag_download_file.cc:43: if (is_started_)
assert that this is called on the DnD thread.

http://codereview.chromium.org/351029/diff/18022/20102#newcode65
chrome/browser/drag_download_file.cc:65: // Start the nested message loop to
wait untill the whole file is downloaded.
nit: untill -> until

http://codereview.chromium.org/351029/diff/18022/20102#newcode82
chrome/browser/drag_download_file.cc:82: tab_contents_->GetURL(),
are there any concerns if the URL of the tab contents has changed?  what if a
page navigation is occurring?  maybe some of these parameters (referrer,
encoding) should be saved when we assign url_.

http://codereview.chromium.org/351029/diff/18022/20102#newcode101
chrome/browser/drag_download_file.cc:101: void
DragDownloadFile::SetDownloads(std::vector<DownloadItem*>& downloads) {
in google style, any reference parameter should be const.  in this case, it
looks like you only read from downloads, so better make it a "const
std::vector<T>&"

http://codereview.chromium.org/351029/diff/18022/20102#newcode163
chrome/browser/drag_download_file.cc:163: observer_->OnDataReady(format_,
downloads);
Q: who is responsible for freeing downloads[0]?

if OnDataReady does not take ownership of downloads[0], then memory will be
leaked.  perhaps it would help to use linked_ptr<T>, so you'd instead have:

std::vector< linked_ptr<OSExchangeData::DownloadFileInfo> > downloads;

A typedef might be nice for that if you go this route.  (Alternatively, you
could also make DownloadFileInfo reference counted.)

http://codereview.chromium.org/351029/diff/18022/20097
File chrome/browser/drag_download_file.h (right):

http://codereview.chromium.org/351029/diff/18022/20097#newcode16
chrome/browser/drag_download_file.h:16: : public
OSExchangeData::DownloadFileProvider,
nit: no line break for the first base class since it is not necessary.

class DragDownloadFile : public OSExchangeData::DownloadFileProvider,
                         public Foo,
                         public Bar {

http://codereview.chromium.org/351029/diff/18022/20101
File chrome/browser/drag_download_stream_win.cc (right):

http://codereview.chromium.org/351029/diff/18022/20101#newcode73
chrome/browser/drag_download_stream_win.cc:73: DCHECK(!request_);
please assert that this is called on the DnD thread.

http://codereview.chromium.org/351029/diff/18022/20101#newcode76
chrome/browser/drag_download_stream_win.cc:76: bool
DragDownloadFileStream::Start(
this function looks a lot like DragDownloadFile::Start.  can you avoid code
duplication?

http://codereview.chromium.org/351029/diff/18022/20101#newcode182
chrome/browser/drag_download_stream_win.cc:182: // Determine if we need to sniff
the content to overwrite the mime type.
this looks a lot like BufferedResourceHandler::DelayResponse.  can you avoid
duplicating this code?

http://codereview.chromium.org/351029/diff/18022/20101#newcode246
chrome/browser/drag_download_stream_win.cc:246: observer_->OnDataReady(format_,
downloads);
same comment as before regarding OnDataReady.  are you sure that it always takes
ownership of the given downloads?  are you sure we don't leak memory in some
cases?

http://codereview.chromium.org/351029/diff/18022/20101#newcode259
chrome/browser/drag_download_stream_win.cc:259: // If we have the unread data
resulted from the content sniff leftover, serve
it seems really unfortunate to have to duplicate delicate content sniffing
logic.  we shouldn't have to do that.  some refactoring should be done to avoid
it.

http://codereview.chromium.org/351029/diff/18022/20101#newcode398
chrome/browser/drag_download_stream_win.cc:398: // This is needed on Vista
because otherwise Vistal Shell will abort the
nit: Vista1 -> Vista

http://codereview.chromium.org/351029/diff/18022/20101#newcode490
chrome/browser/drag_download_stream_win.cc:490: // in the response header.
oh, this seems like a big deal.  i don't think we should enable this feature
until we have a solution to this.  or, we need to disable the CHECK to avoid
crashing.

http://codereview.chromium.org/351029/diff/18022/20100
File chrome/browser/drag_download_stream_win.h (right):

http://codereview.chromium.org/351029/diff/18022/20100#newcode6
chrome/browser/drag_download_stream_win.h:6: #define
CHROME_BROWSER_DRAG_DOWNLOAD_STREAM_WIN_H_
typically, you should have one class per header file.  how about breaking these
up into several files?  at that point though we are creating a lot of files.  it
seems like it would be good to introduce another subdirectory of browser, or
perhaps these files can just go into browser/download since they have something
to do with downloading.

http://codereview.chromium.org/351029/diff/18022/20100#newcode21
chrome/browser/drag_download_stream_win.h:21: class ZoneData {
this class needs some documentation.

http://codereview.chromium.org/351029/diff/18022/20100#newcode25
chrome/browser/drag_download_stream_win.h:25: static int GetStreamSize();
this should have a DISALLOW_IMPLICIT_CONSTRUCTORS

http://codereview.chromium.org/351029/diff/18022/20100#newcode28
chrome/browser/drag_download_stream_win.h:28: class DragDownloadFileStream
class DragDownloadFileStream : public IStream,
                               public OSExchangeData::DownloadFileProvider,
                               ...

http://codereview.chromium.org/351029/diff/18022/20100#newcode32
chrome/browser/drag_download_stream_win.h:32: static const int kBufferSize =
32768;
please move these down into the private section.

http://codereview.chromium.org/351029/diff/18022/20100#newcode84
chrome/browser/drag_download_stream_win.h:84: // Accessors.
do we care what thread this is called on?  based on the comment below for
filename_, i'd assume that this may only be called on the I/O thread.  we should
probably assert that.

http://codereview.chromium.org/351029/diff/18022/20100#newcode109
chrome/browser/drag_download_stream_win.h:109: URLRequestContextGetter*
request_context_getter_;
i think you need to keep an owning reference to the URLRequestContextGetter. 
use a scoped_refptr.  or do you have a guarantee that this will remain alive
longer than a DragDownloadFileStream instance?

http://codereview.chromium.org/351029/diff/18022/20100#newcode147
chrome/browser/drag_download_stream_win.h:147: class DragDownloadDataStream
class DragDownloadDataStream : public OSExchangeData::DownloadFileProvider,
                               public IStream {

http://codereview.chromium.org/351029/diff/18022/20100#newcode147
chrome/browser/drag_download_stream_win.h:147: class DragDownloadDataStream
any rules about what thread this gets accessed on?  by the way, it would be good
to write a comment that provides an overview of the DnD architecture for
Windows.

http://codereview.chromium.org/351029/diff/18022/20117
File chrome/browser/tab_contents/web_drag_source_win.cc (right):

http://codereview.chromium.org/351029/diff/18022/20117#newcode45
chrome/browser/tab_contents/web_drag_source_win.cc:45: if
(!ChromeThread::IsWellKnownThread(ChromeThread::UI)) {
I don't understand the IsWellKnownThread check here.  Given ChromeThread::UI,
IsWellKnownThread will only return false if the UI thread has been destroyed. 
that should basically only be true late during shutdown, but by that time all of
the other child threads (except for DNS query threads) should be gone.

(I haven't reviewed enough of this CL yet to understand the lifetime of the DnD
thread.)

[darin (slow to review), 2009-12-17 21:43:21]

OK, done reviewing all files now.

http://codereview.chromium.org/351029/diff/18022/20129
File chrome/browser/views/tab_contents/tab_contents_drag_win.cc (right):

http://codereview.chromium.org/351029/diff/18022/20129#newcode29
chrome/browser/views/tab_contents/tab_contents_drag_win.cc:29: namespace {
nit: add a new line below here.

http://codereview.chromium.org/351029/diff/18022/20129#newcode39
chrome/browser/views/tab_contents/tab_contents_drag_win.cc:39: msg->message ==
WM_KEYDOWN || msg->message == WM_KEYUP) {
what about WM_SYSKEY{DOWN,UP}?  can we ignore those?

http://codereview.chromium.org/351029/diff/18022/20129#newcode56
chrome/browser/views/tab_contents/tab_contents_drag_win.cc:56: }  // namespace
nit: add a new line above here.

http://codereview.chromium.org/351029/diff/18022/20129#newcode60
chrome/browser/views/tab_contents/tab_contents_drag_win.cc:60: explicit
DragDropThread(TabContentsDragWin* drag_handler) :
nit: the colon should go on the next line:

  explicit DragDropThread(TabContentsDragWin* drag_handler)
      : base::Thread("Chrome_DragDropThread"),
        drag_handler_(drag_handler) {

http://codereview.chromium.org/351029/diff/18022/20129#newcode63
chrome/browser/views/tab_contents/tab_contents_drag_win.cc:63: }
you need to define a destructor that calls the Thread::Stop() method. 
otherwise, your CleanUp function will not be called.

(it is a major flaw in the design of the Thread class that such a boo-boo is so
easy to make.)

http://codereview.chromium.org/351029/diff/18022/20129#newcode77
chrome/browser/views/tab_contents/tab_contents_drag_win.cc:77: // Hold a
reference count to TabContentsDragWin to make sure that it is always
do you care what thread the TabContentsDragWin dies on?  maybe it would be good
for it to always be deleted on the UI thread?

http://codereview.chromium.org/351029/diff/18022/20130
File chrome/browser/views/tab_contents/tab_contents_drag_win.h (right):

http://codereview.chromium.org/351029/diff/18022/20130#newcode20
chrome/browser/views/tab_contents/tab_contents_drag_win.h:20: public
base::RefCountedThreadSafe<TabContentsDragWin> {
can you add comments like you did for other classes to annotate which methods
are called on which threads?

http://codereview.chromium.org/351029/diff/18022/20130#newcode29
chrome/browser/views/tab_contents/tab_contents_drag_win.h:29: //
DataObjectImple::Observer implementation.
nit: DataObjectImple -> DataObjectImpl

[jianli, 2009-12-17 23:27:40]

http://codereview.chromium.org/351029/diff/18022/20091
File base/file_util_win.cc (right):

http://codereview.chromium.org/351029/diff/18022/20091#newcode107
base/file_util_win.cc:107: MOVEFILE_REPLACE_EXISTING) == TRUE;
On 2009/12/17 01:03:44, darin wrote:
> nit: comparing to TRUE can be problematic since any non-zero value can be
> considered "true"... x != FALSE is usually better.

Done.

http://codereview.chromium.org/351029/diff/18022/20111
File chrome/browser/download/download_manager.cc (right):

http://codereview.chromium.org/351029/diff/18022/20111#newcode1254
chrome/browser/download/download_manager.cc:1254: void
DownloadManager::GenerateFileName(DownloadCreateInfo* info,
On 2009/12/17 01:03:44, darin wrote:
> the google style guide says to not overload method names.  maybe you could
name
> this one GenerateFileNameFromInfo.

Done.

http://codereview.chromium.org/351029/diff/18022/20102
File chrome/browser/drag_download_file.cc (right):

http://codereview.chromium.org/351029/diff/18022/20102#newcode27
chrome/browser/drag_download_file.cc:27: DragDownloadFile::~DragDownloadFile() {
On 2009/12/17 01:03:44, darin wrote:
> assert that this is called on the DnD thread.

Done.

http://codereview.chromium.org/351029/diff/18022/20102#newcode43
chrome/browser/drag_download_file.cc:43: if (is_started_)
On 2009/12/17 01:03:44, darin wrote:
> assert that this is called on the DnD thread.

Done.

http://codereview.chromium.org/351029/diff/18022/20102#newcode65
chrome/browser/drag_download_file.cc:65: // Start the nested message loop to
wait untill the whole file is downloaded.
On 2009/12/17 01:03:44, darin wrote:
> nit: untill -> until

Done.

http://codereview.chromium.org/351029/diff/18022/20102#newcode82
chrome/browser/drag_download_file.cc:82: tab_contents_->GetURL(),
On 2009/12/17 01:03:44, darin wrote:
> are there any concerns if the URL of the tab contents has changed?  what if a
> page navigation is occurring?  maybe some of these parameters (referrer,
> encoding) should be saved when we assign url_.

Done.

http://codereview.chromium.org/351029/diff/18022/20102#newcode101
chrome/browser/drag_download_file.cc:101: void
DragDownloadFile::SetDownloads(std::vector<DownloadItem*>& downloads) {
On 2009/12/17 01:03:44, darin wrote:
> in google style, any reference parameter should be const.  in this case, it
> looks like you only read from downloads, so better make it a "const
> std::vector<T>&"

This is because DownloadManager::Observer defined SetDownloads in such way.  I
can not add const modifier to the parameter because one implementation
DownloadsDOMHandler::SetDownloads(std::vector<DownloadItem*>& downloads) uses
something like the following:
   download_items_.swap(downloads);

Do you want me to fix all of these to use pointer:
  virtual void SetDownloads(std::vector<DownloadItem*>* downloads) = 0;

http://codereview.chromium.org/351029/diff/18022/20102#newcode163
chrome/browser/drag_download_file.cc:163: observer_->OnDataReady(format_,
downloads);
On 2009/12/17 01:03:44, darin wrote:
> Q: who is responsible for freeing downloads[0]?
> 
> if OnDataReady does not take ownership of downloads[0], then memory will be
> leaked.  perhaps it would help to use linked_ptr<T>, so you'd instead have:
> 
> std::vector< linked_ptr<OSExchangeData::DownloadFileInfo> > downloads;
> 
> A typedef might be nice for that if you go this route.  (Alternatively, you
> could also make DownloadFileInfo reference counted.)

DataObjectImpl::OnDataReady will take the ownership. It is destroyed on
DataObjectImpl::StopDownloads in the inner for loop:
  delete (*iter)->downloads[i];

http://codereview.chromium.org/351029/diff/18022/20097
File chrome/browser/drag_download_file.h (right):

http://codereview.chromium.org/351029/diff/18022/20097#newcode16
chrome/browser/drag_download_file.h:16: : public
OSExchangeData::DownloadFileProvider,
On 2009/12/17 01:03:44, darin wrote:
> nit: no line break for the first base class since it is not necessary.
> 
> class DragDownloadFile : public OSExchangeData::DownloadFileProvider,
>                          public Foo,
>                          public Bar {

Done.

http://codereview.chromium.org/351029/diff/18022/20101
File chrome/browser/drag_download_stream_win.cc (right):

http://codereview.chromium.org/351029/diff/18022/20101#newcode73
chrome/browser/drag_download_stream_win.cc:73: DCHECK(!request_);
On 2009/12/17 01:03:44, darin wrote:
> please assert that this is called on the DnD thread.

The destructor could be called on either drag-and-drop thread or I/O thread
depending on how the last reference count is released. The RunnableMethod
cleanup from either thread might be the last one. I added the comment for this.

http://codereview.chromium.org/351029/diff/18022/20101#newcode76
chrome/browser/drag_download_stream_win.cc:76: bool
DragDownloadFileStream::Start(
On 2009/12/17 01:03:44, darin wrote:
> this function looks a lot like DragDownloadFile::Start.  can you avoid code
> duplication?

Refactor some duplicate logic in DragDownloadFile and DragDownloadFileStream
into a base class DragDownloadFileBase.

http://codereview.chromium.org/351029/diff/18022/20101#newcode182
chrome/browser/drag_download_stream_win.cc:182: // Determine if we need to sniff
the content to overwrite the mime type.
On 2009/12/17 01:03:44, darin wrote:
> this looks a lot like BufferedResourceHandler::DelayResponse.  can you avoid
> duplicating this code?

Yes, I borrowed the logic from BufferedResourceHandler::DelayResponse since the
logic there cannot be used directly without a lot refactoring. I agree we should
avoid duplicating the code. So I am going to refactor the code there. Since the
current patch is already very complicated, could we do the refactoring not in
this patch? I put a TODO here and will get to this with my next patch.

http://codereview.chromium.org/351029/diff/18022/20101#newcode246
chrome/browser/drag_download_stream_win.cc:246: observer_->OnDataReady(format_,
downloads);
On 2009/12/17 01:03:44, darin wrote:
> same comment as before regarding OnDataReady.  are you sure that it always
takes
> ownership of the given downloads?  are you sure we don't leak memory in some
> cases?

Yes. It is deleted on DataObjectImpl::StopDownloads. StopDownloads() is called
when download is cancelled or DataObjectImpl is destructed.

http://codereview.chromium.org/351029/diff/18022/20101#newcode259
chrome/browser/drag_download_stream_win.cc:259: // If we have the unread data
resulted from the content sniff leftover, serve
On 2009/12/17 01:03:44, darin wrote:
> it seems really unfortunate to have to duplicate delicate content sniffing
> logic.  we shouldn't have to do that.  some refactoring should be done to
avoid
> it.

I will do refactoring to use the logic from
BufferedResourceHandler::DelayResponse. See my comment above.

http://codereview.chromium.org/351029/diff/18022/20101#newcode398
chrome/browser/drag_download_stream_win.cc:398: // This is needed on Vista
because otherwise Vistal Shell will abort the
On 2009/12/17 01:03:44, darin wrote:
> nit: Vista1 -> Vista

Done.

http://codereview.chromium.org/351029/diff/18022/20101#newcode490
chrome/browser/drag_download_stream_win.cc:490: // in the response header.
On 2009/12/17 01:03:44, darin wrote:
> oh, this seems like a big deal.  i don't think we should enable this feature
> until we have a solution to this.  or, we need to disable the CHECK to avoid
> crashing.

I think normally we should expect Content-Length in the response header. If it
is not there, we treat it as error.

http://codereview.chromium.org/351029/diff/18022/20100
File chrome/browser/drag_download_stream_win.h (right):

http://codereview.chromium.org/351029/diff/18022/20100#newcode6
chrome/browser/drag_download_stream_win.h:6: #define
CHROME_BROWSER_DRAG_DOWNLOAD_STREAM_WIN_H_
On 2009/12/17 01:03:44, darin wrote:
> typically, you should have one class per header file.  how about breaking
these
> up into several files?  at that point though we are creating a lot of files. 
it
> seems like it would be good to introduce another subdirectory of browser, or
> perhaps these files can just go into browser/download since they have
something
> to do with downloading.

Break into multiple files and move all drag_download* files to browser/download.
The ZoneData class is renamed as ZoneIdentifierADS and move into
win_safe_util.*.

http://codereview.chromium.org/351029/diff/18022/20100#newcode21
chrome/browser/drag_download_stream_win.h:21: class ZoneData {
On 2009/12/17 01:03:44, darin wrote:
> this class needs some documentation.

Done.

http://codereview.chromium.org/351029/diff/18022/20100#newcode25
chrome/browser/drag_download_stream_win.h:25: static int GetStreamSize();
On 2009/12/17 01:03:44, darin wrote:
> this should have a DISALLOW_IMPLICIT_CONSTRUCTORS

Done.

http://codereview.chromium.org/351029/diff/18022/20100#newcode28
chrome/browser/drag_download_stream_win.h:28: class DragDownloadFileStream
On 2009/12/17 01:03:44, darin wrote:
> class DragDownloadFileStream : public IStream,
>                                public OSExchangeData::DownloadFileProvider,
>                                ...
> 

Done.

http://codereview.chromium.org/351029/diff/18022/20100#newcode32
chrome/browser/drag_download_stream_win.h:32: static const int kBufferSize =
32768;
On 2009/12/17 01:03:44, darin wrote:
> please move these down into the private section.

Done.

http://codereview.chromium.org/351029/diff/18022/20100#newcode84
chrome/browser/drag_download_stream_win.h:84: // Accessors.
On 2009/12/17 01:03:44, darin wrote:
> do we care what thread this is called on?  based on the comment below for
> filename_, i'd assume that this may only be called on the I/O thread.  we
should
> probably assert that.

Removed since it is not used any more.

http://codereview.chromium.org/351029/diff/18022/20100#newcode109
chrome/browser/drag_download_stream_win.h:109: URLRequestContextGetter*
request_context_getter_;
On 2009/12/17 01:03:44, darin wrote:
> i think you need to keep an owning reference to the URLRequestContextGetter. 
> use a scoped_refptr.  or do you have a guarantee that this will remain alive
> longer than a DragDownloadFileStream instance?

Changed to use scoped_refptr.

http://codereview.chromium.org/351029/diff/18022/20100#newcode147
chrome/browser/drag_download_stream_win.h:147: class DragDownloadDataStream
On 2009/12/17 01:03:44, darin wrote:
> class DragDownloadDataStream : public OSExchangeData::DownloadFileProvider,
>                                public IStream {

Done.

http://codereview.chromium.org/351029/diff/18022/20100#newcode147
chrome/browser/drag_download_stream_win.h:147: class DragDownloadDataStream
On 2009/12/17 01:03:44, darin wrote:
> any rules about what thread this gets accessed on?  by the way, it would be
good
> to write a comment that provides an overview of the DnD architecture for
> Windows.

This class is always used on DnD thread since it is used to provide the static
data only and I added the comment for this.

I've added the detailed comment in TabContentsDragWin::PrepareDragForDownload
(tab_content_drag_win.cc).

http://codereview.chromium.org/351029/diff/18022/20117
File chrome/browser/tab_contents/web_drag_source_win.cc (right):

http://codereview.chromium.org/351029/diff/18022/20117#newcode45
chrome/browser/tab_contents/web_drag_source_win.cc:45: if
(!ChromeThread::IsWellKnownThread(ChromeThread::UI)) {
On 2009/12/17 01:03:44, darin wrote:
> I don't understand the IsWellKnownThread check here.  Given ChromeThread::UI,
> IsWellKnownThread will only return false if the UI thread has been destroyed. 
> that should basically only be true late during shutdown, but by that time all
of
> the other child threads (except for DNS query threads) should be gone.
> 
> (I haven't reviewed enough of this CL yet to understand the lifetime of the
DnD
> thread.)

My bad. It should be CurrentlyOn.

http://codereview.chromium.org/351029/diff/18022/20129
File chrome/browser/views/tab_contents/tab_contents_drag_win.cc (right):

http://codereview.chromium.org/351029/diff/18022/20129#newcode29
chrome/browser/views/tab_contents/tab_contents_drag_win.cc:29: namespace {
On 2009/12/17 21:43:21, darin wrote:
> nit: add a new line below here.

Done.

http://codereview.chromium.org/351029/diff/18022/20129#newcode39
chrome/browser/views/tab_contents/tab_contents_drag_win.cc:39: msg->message ==
WM_KEYDOWN || msg->message == WM_KEYUP) {
On 2009/12/17 21:43:21, darin wrote:
> what about WM_SYSKEY{DOWN,UP}?  can we ignore those?

I think we do not care about WM_SYSKEY* because when ALT is pressed down while
holding the left button, it is meant to create a link.

http://codereview.chromium.org/351029/diff/18022/20129#newcode56
chrome/browser/views/tab_contents/tab_contents_drag_win.cc:56: }  // namespace
On 2009/12/17 21:43:21, darin wrote:
> nit: add a new line above here.

Done.

http://codereview.chromium.org/351029/diff/18022/20129#newcode60
chrome/browser/views/tab_contents/tab_contents_drag_win.cc:60: explicit
DragDropThread(TabContentsDragWin* drag_handler) :
On 2009/12/17 21:43:21, darin wrote:
> nit: the colon should go on the next line:
> 
>   explicit DragDropThread(TabContentsDragWin* drag_handler)
>       : base::Thread("Chrome_DragDropThread"),
>         drag_handler_(drag_handler) {

Done.

http://codereview.chromium.org/351029/diff/18022/20129#newcode63
chrome/browser/views/tab_contents/tab_contents_drag_win.cc:63: }
On 2009/12/17 21:43:21, darin wrote:
> you need to define a destructor that calls the Thread::Stop() method. 
> otherwise, your CleanUp function will not be called.
> 
> (it is a major flaw in the design of the Thread class that such a boo-boo is
so
> easy to make.)

Done.

http://codereview.chromium.org/351029/diff/18022/20129#newcode77
chrome/browser/views/tab_contents/tab_contents_drag_win.cc:77: // Hold a
reference count to TabContentsDragWin to make sure that it is always
On 2009/12/17 21:43:21, darin wrote:
> do you care what thread the TabContentsDragWin dies on?  maybe it would be
good
> for it to always be deleted on the UI thread?

TabContentsDragWin is always destructed on UI thread. Added the DCHECK for it.

http://codereview.chromium.org/351029/diff/18022/20130
File chrome/browser/views/tab_contents/tab_contents_drag_win.h (right):

http://codereview.chromium.org/351029/diff/18022/20130#newcode20
chrome/browser/views/tab_contents/tab_contents_drag_win.h:20: public
base::RefCountedThreadSafe<TabContentsDragWin> {
On 2009/12/17 21:43:21, darin wrote:
> can you add comments like you did for other classes to annotate which methods
> are called on which threads?

Done.

http://codereview.chromium.org/351029/diff/18022/20130#newcode29
chrome/browser/views/tab_contents/tab_contents_drag_win.h:29: //
DataObjectImple::Observer implementation.
On 2009/12/17 21:43:21, darin wrote:
> nit: DataObjectImple -> DataObjectImpl

Done.

[darin (slow to review), 2009-12-18 18:38:15]

On Thu, Dec 17, 2009 at 3:27 PM, <jianli@chromium.org> wrote:
...

> http://codereview.chromium.org/351029/diff/18022/20102#newcode101
> chrome/browser/drag_download_file.cc:101: void
> DragDownloadFile::SetDownloads(std::vector<DownloadItem*>& downloads) {
> On 2009/12/17 01:03:44, darin wrote:
>
>> in google style, any reference parameter should be const.  in this
>>
> case, it
>
>> looks like you only read from downloads, so better make it a "const
>> std::vector<T>&"
>>
>
> This is because DownloadManager::Observer defined SetDownloads in such
> way.  I can not add const modifier to the parameter because one
> implementation
> DownloadsDOMHandler::SetDownloads(std::vector<DownloadItem*>& downloads)
> uses something like the following:
>   download_items_.swap(downloads);
>

It seems like an unfortunate design.  I bet switching to
std::vector<linked_ptr<T> > would be all around superior.




>
> Do you want me to fix all of these to use pointer:
>  virtual void SetDownloads(std::vector<DownloadItem*>* downloads) = 0;
>
>
This can be a follow-up CL.




>
> http://codereview.chromium.org/351029/diff/18022/20102#newcode163
> chrome/browser/drag_download_file.cc:163:
> observer_->OnDataReady(format_, downloads);
> On 2009/12/17 01:03:44, darin wrote:
>
>> Q: who is responsible for freeing downloads[0]?
>>
>
>  if OnDataReady does not take ownership of downloads[0], then memory
>>
> will be
>
>> leaked.  perhaps it would help to use linked_ptr<T>, so you'd instead
>>
> have:
>
>  std::vector< linked_ptr<OSExchangeData::DownloadFileInfo> > downloads;
>>
>
>  A typedef might be nice for that if you go this route.
>>
> (Alternatively, you
>
>> could also make DownloadFileInfo reference counted.)
>>
>
> DataObjectImpl::OnDataReady will take the ownership. It is destroyed on
> DataObjectImpl::StopDownloads in the inner for loop:
>  delete (*iter)->downloads[i];


I see that too, but what if the inner loop code isn't reached?  Please just
make
sure memory isn't being leaked in unusual cases.




> http://codereview.chromium.org/351029/diff/18022/20101
> File chrome/browser/drag_download_stream_win.cc (right):
>
> http://codereview.chromium.org/351029/diff/18022/20101#newcode73
> chrome/browser/drag_download_stream_win.cc:73: DCHECK(!request_);
> On 2009/12/17 01:03:44, darin wrote:
>
>> please assert that this is called on the DnD thread.
>>
>
> The destructor could be called on either drag-and-drop thread or I/O
> thread depending on how the last reference count is released. The
> RunnableMethod cleanup from either thread might be the last one. I added
> the comment for this.
>
>
OK



>
> http://codereview.chromium.org/351029/diff/18022/20101#newcode76
> chrome/browser/drag_download_stream_win.cc:76: bool
> DragDownloadFileStream::Start(
> On 2009/12/17 01:03:44, darin wrote:
>
>> this function looks a lot like DragDownloadFile::Start.  can you avoid
>>
> code
>
>> duplication?
>>
>
> Refactor some duplicate logic in DragDownloadFile and
> DragDownloadFileStream into a base class DragDownloadFileBase.
>
>
Thanks!


>
> http://codereview.chromium.org/351029/diff/18022/20101#newcode182
> chrome/browser/drag_download_stream_win.cc:182: // Determine if we need
> to sniff the content to overwrite the mime type.
> On 2009/12/17 01:03:44, darin wrote:
>
>> this looks a lot like BufferedResourceHandler::DelayResponse.  can you
>>
> avoid
>
>> duplicating this code?
>>
>
> Yes, I borrowed the logic from BufferedResourceHandler::DelayResponse
> since the logic there cannot be used directly without a lot refactoring.
> I agree we should avoid duplicating the code. So I am going to refactor
> the code there. Since the current patch is already very complicated,
> could we do the refactoring not in this patch? I put a TODO here and
> will get to this with my next patch.
>
>
OK



>
> http://codereview.chromium.org/351029/diff/18022/20101#newcode246
> chrome/browser/drag_download_stream_win.cc:246:
> observer_->OnDataReady(format_, downloads);
> On 2009/12/17 01:03:44, darin wrote:
>
>> same comment as before regarding OnDataReady.  are you sure that it
>>
> always takes
>
>> ownership of the given downloads?  are you sure we don't leak memory
>>
> in some
>
>> cases?
>>
>
> Yes. It is deleted on DataObjectImpl::StopDownloads. StopDownloads() is
> called when download is cancelled or DataObjectImpl is destructed.
>
>
OK



>
> http://codereview.chromium.org/351029/diff/18022/20101#newcode259
> chrome/browser/drag_download_stream_win.cc:259: // If we have the unread
> data resulted from the content sniff leftover, serve
> On 2009/12/17 01:03:44, darin wrote:
>
>> it seems really unfortunate to have to duplicate delicate content
>>
> sniffing
>
>> logic.  we shouldn't have to do that.  some refactoring should be done
>>
> to avoid
>
>> it.
>>
>
> I will do refactoring to use the logic from
> BufferedResourceHandler::DelayResponse. See my comment above.
>
>
OK



> http://codereview.chromium.org/351029/diff/18022/20101#newcode490
> chrome/browser/drag_download_stream_win.cc:490: // in the response
> header.
> On 2009/12/17 01:03:44, darin wrote:
>
>> oh, this seems like a big deal.  i don't think we should enable this
>>
> feature
>
>> until we have a solution to this.  or, we need to disable the CHECK to
>>
> avoid
>
>> crashing.
>>
>
> I think normally we should expect Content-Length in the response header.
> If it is not there, we treat it as error.
>
>
OK.... it seems like files that are generated server-side typically do not
have a C-L.  They instead use chunked encoding so that the server doesn't
have to generate the complete file before sending it.  I'm surprised that we
wouldn't need to worry about supporting chunked encoding use cases.




>
> http://codereview.chromium.org/351029/diff/18022/20100
> File chrome/browser/drag_download_stream_win.h (right):
>
> http://codereview.chromium.org/351029/diff/18022/20100#newcode6
> chrome/browser/drag_download_stream_win.h:6: #define
> CHROME_BROWSER_DRAG_DOWNLOAD_STREAM_WIN_H_
> On 2009/12/17 01:03:44, darin wrote:
>
>> typically, you should have one class per header file.  how about
>>
> breaking these
>
>> up into several files?  at that point though we are creating a lot of
>>
> files.  it
>
>> seems like it would be good to introduce another subdirectory of
>>
> browser, or
>
>> perhaps these files can just go into browser/download since they have
>>
> something
>
>> to do with downloading.
>>
>
> Break into multiple files and move all drag_download* files to
> browser/download. The ZoneData class is renamed as ZoneIdentifierADS and
> move into win_safe_util.*.


OK



>
>
> http://codereview.chromium.org/351029/diff/18022/20100#newcode21
> chrome/browser/drag_download_stream_win.h:21: class ZoneData {
> On 2009/12/17 01:03:44, darin wrote:
>
>> this class needs some documentation.
>>
>
> Done.
>
>
> http://codereview.chromium.org/351029/diff/18022/20100#newcode25
> chrome/browser/drag_download_stream_win.h:25: static int
> GetStreamSize();
> On 2009/12/17 01:03:44, darin wrote:
>
>> this should have a DISALLOW_IMPLICIT_CONSTRUCTORS
>>
>
> Done.
>
>
> http://codereview.chromium.org/351029/diff/18022/20100#newcode28
> chrome/browser/drag_download_stream_win.h:28: class
> DragDownloadFileStream
> On 2009/12/17 01:03:44, darin wrote:
>
>> class DragDownloadFileStream : public IStream,
>>                                public
>>
> OSExchangeData::DownloadFileProvider,
>
>>                                ...
>>
>
>
> Done.
>
>
> http://codereview.chromium.org/351029/diff/18022/20100#newcode32
> chrome/browser/drag_download_stream_win.h:32: static const int
> kBufferSize = 32768;
> On 2009/12/17 01:03:44, darin wrote:
>
>> please move these down into the private section.
>>
>
> Done.
>
>
> http://codereview.chromium.org/351029/diff/18022/20100#newcode84
> chrome/browser/drag_download_stream_win.h:84: // Accessors.
> On 2009/12/17 01:03:44, darin wrote:
>
>> do we care what thread this is called on?  based on the comment below
>>
> for
>
>> filename_, i'd assume that this may only be called on the I/O thread.
>>
> we should
>
>> probably assert that.
>>
>
> Removed since it is not used any more.
>
>
OK, /me likes code removal :)




>
> http://codereview.chromium.org/351029/diff/18022/20100#newcode109
> chrome/browser/drag_download_stream_win.h:109: URLRequestContextGetter*
> request_context_getter_;
> On 2009/12/17 01:03:44, darin wrote:
>
>> i think you need to keep an owning reference to the
>>
> URLRequestContextGetter.
>
>> use a scoped_refptr.  or do you have a guarantee that this will remain
>>
> alive
>
>> longer than a DragDownloadFileStream instance?
>>
>
> Changed to use scoped_refptr.
>
>
OK



> http://codereview.chromium.org/351029/diff/18022/20100#newcode147
> chrome/browser/drag_download_stream_win.h:147: class
> DragDownloadDataStream
> On 2009/12/17 01:03:44, darin wrote:
>
>> any rules about what thread this gets accessed on?  by the way, it
>>
> would be good
>
>> to write a comment that provides an overview of the DnD architecture
>>
> for
>
>> Windows.
>>
>
> This class is always used on DnD thread since it is used to provide the
> static data only and I added the comment for this.
>
>
Thanks



> I've added the detailed comment in
> TabContentsDragWin::PrepareDragForDownload (tab_content_drag_win.cc).
>
>
Thanks




> http://codereview.chromium.org/351029/diff/18022/20129#newcode39
> chrome/browser/views/tab_contents/tab_contents_drag_win.cc:39:
> msg->message == WM_KEYDOWN || msg->message == WM_KEYUP) {
> On 2009/12/17 21:43:21, darin wrote:
>
>> what about WM_SYSKEY{DOWN,UP}?  can we ignore those?
>>
>
> I think we do not care about WM_SYSKEY* because when ALT is pressed down
> while holding the left button, it is meant to create a link.
>
>
OK... please add a comment explaining this.



/18022/20129#newcode77<http://codereview.chromium.org/351029/diff/18022/20129#newcode77>

> chrome/browser/views/tab_contents/tab_contents_drag_win.cc:77: // Hold a
> reference count to TabContentsDragWin to make sure that it is always
> On 2009/12/17 21:43:21, darin wrote:
>
>> do you care what thread the TabContentsDragWin dies on?  maybe it
>>
> would be good
>
>> for it to always be deleted on the UI thread?
>>
>
> TabContentsDragWin is always destructed on UI thread. Added the DCHECK
> for it.
>
>
Thanks



http://codereview.chromium.org/351029
>

[jianli, 2009-12-18 22:21:01]

Thanks a lot.

On Fri, Dec 18, 2009 at 10:37 AM, Darin Fisher <darin@chromium.org> wrote:

>
>
> On Thu, Dec 17, 2009 at 3:27 PM, <jianli@chromium.org> wrote:
> ...
>
>> http://codereview.chromium.org/351029/diff/18022/20102#newcode101
>> chrome/browser/drag_download_file.cc:101: void
>> DragDownloadFile::SetDownloads(std::vector<DownloadItem*>& downloads) {
>> On 2009/12/17 01:03:44, darin wrote:
>>
>>> in google style, any reference parameter should be const.  in this
>>>
>> case, it
>>
>>> looks like you only read from downloads, so better make it a "const
>>> std::vector<T>&"
>>>
>>
>> This is because DownloadManager::Observer defined SetDownloads in such
>> way.  I can not add const modifier to the parameter because one
>> implementation
>> DownloadsDOMHandler::SetDownloads(std::vector<DownloadItem*>& downloads)
>> uses something like the following:
>>   download_items_.swap(downloads);
>>
>
> It seems like an unfortunate design.  I bet switching to
> std::vector<linked_ptr<T> > would be all around superior.
>
>
>
>
>>
>> Do you want me to fix all of these to use pointer:
>>  virtual void SetDownloads(std::vector<DownloadItem*>* downloads) = 0;
>>
>>
> This can be a follow-up CL.
>
>
> Will do.

>
>
>>
>> http://codereview.chromium.org/351029/diff/18022/20102#newcode163
>> chrome/browser/drag_download_file.cc:163:
>> observer_->OnDataReady(format_, downloads);
>> On 2009/12/17 01:03:44, darin wrote:
>>
>>> Q: who is responsible for freeing downloads[0]?
>>>
>>
>>  if OnDataReady does not take ownership of downloads[0], then memory
>>>
>> will be
>>
>>> leaked.  perhaps it would help to use linked_ptr<T>, so you'd instead
>>>
>> have:
>>
>>  std::vector< linked_ptr<OSExchangeData::DownloadFileInfo> > downloads;
>>>
>>
>>  A typedef might be nice for that if you go this route.
>>>
>> (Alternatively, you
>>
>>> could also make DownloadFileInfo reference counted.)
>>>
>>
>> DataObjectImpl::OnDataReady will take the ownership. It is destroyed on
>> DataObjectImpl::StopDownloads in the inner for loop:
>>  delete (*iter)->downloads[i];
>
>
> I see that too, but what if the inner loop code isn't reached?  Please just
> make
> sure memory isn't being leaked in unusual cases.
>
>
> The destructor will always be called by OLE when it succeeds or fails the
drag-and-drop.

The only bad thing is that the user closes the Chrome application while
drag-out is still in progress. I will try to figure out how to address this
issue.

>
>
>> http://codereview.chromium.org/351029/diff/18022/20101
>> File chrome/browser/drag_download_stream_win.cc (right):
>>
>> http://codereview.chromium.org/351029/diff/18022/20101#newcode73
>> chrome/browser/drag_download_stream_win.cc:73: DCHECK(!request_);
>> On 2009/12/17 01:03:44, darin wrote:
>>
>>> please assert that this is called on the DnD thread.
>>>
>>
>> The destructor could be called on either drag-and-drop thread or I/O
>> thread depending on how the last reference count is released. The
>> RunnableMethod cleanup from either thread might be the last one. I added
>> the comment for this.
>>
>>
> OK
>
>
>
>>
>> http://codereview.chromium.org/351029/diff/18022/20101#newcode76
>> chrome/browser/drag_download_stream_win.cc:76: bool
>> DragDownloadFileStream::Start(
>> On 2009/12/17 01:03:44, darin wrote:
>>
>>> this function looks a lot like DragDownloadFile::Start.  can you avoid
>>>
>> code
>>
>>> duplication?
>>>
>>
>> Refactor some duplicate logic in DragDownloadFile and
>> DragDownloadFileStream into a base class DragDownloadFileBase.
>>
>>
> Thanks!
>
>
>>
>> http://codereview.chromium.org/351029/diff/18022/20101#newcode182
>> chrome/browser/drag_download_stream_win.cc:182: // Determine if we need
>> to sniff the content to overwrite the mime type.
>> On 2009/12/17 01:03:44, darin wrote:
>>
>>> this looks a lot like BufferedResourceHandler::DelayResponse.  can you
>>>
>> avoid
>>
>>> duplicating this code?
>>>
>>
>> Yes, I borrowed the logic from BufferedResourceHandler::DelayResponse
>> since the logic there cannot be used directly without a lot refactoring.
>> I agree we should avoid duplicating the code. So I am going to refactor
>> the code there. Since the current patch is already very complicated,
>> could we do the refactoring not in this patch? I put a TODO here and
>> will get to this with my next patch.
>>
>>
> OK
>
>
>
>>
>> http://codereview.chromium.org/351029/diff/18022/20101#newcode246
>> chrome/browser/drag_download_stream_win.cc:246:
>> observer_->OnDataReady(format_, downloads);
>> On 2009/12/17 01:03:44, darin wrote:
>>
>>> same comment as before regarding OnDataReady.  are you sure that it
>>>
>> always takes
>>
>>> ownership of the given downloads?  are you sure we don't leak memory
>>>
>> in some
>>
>>> cases?
>>>
>>
>> Yes. It is deleted on DataObjectImpl::StopDownloads. StopDownloads() is
>> called when download is cancelled or DataObjectImpl is destructed.
>>
>>
> OK
>
>
>
>>
>> http://codereview.chromium.org/351029/diff/18022/20101#newcode259
>> chrome/browser/drag_download_stream_win.cc:259: // If we have the unread
>> data resulted from the content sniff leftover, serve
>> On 2009/12/17 01:03:44, darin wrote:
>>
>>> it seems really unfortunate to have to duplicate delicate content
>>>
>> sniffing
>>
>>> logic.  we shouldn't have to do that.  some refactoring should be done
>>>
>> to avoid
>>
>>> it.
>>>
>>
>> I will do refactoring to use the logic from
>> BufferedResourceHandler::DelayResponse. See my comment above.
>>
>>
> OK
>
>
>
>>  http://codereview.chromium.org/351029/diff/18022/20101#newcode490
>> chrome/browser/drag_download_stream_win.cc:490: // in the response
>> header.
>> On 2009/12/17 01:03:44, darin wrote:
>>
>>> oh, this seems like a big deal.  i don't think we should enable this
>>>
>> feature
>>
>>> until we have a solution to this.  or, we need to disable the CHECK to
>>>
>> avoid
>>
>>> crashing.
>>>
>>
>> I think normally we should expect Content-Length in the response header.
>> If it is not there, we treat it as error.
>>
>>
> OK.... it seems like files that are generated server-side typically do not
> have a C-L.  They instead use chunked encoding so that the server doesn't
> have to generate the complete file before sending it.  I'm surprised that
> we
> wouldn't need to worry about supporting chunked encoding use cases.
>
>
> Thank you for catching this. I do not find a way to make Shell work without
providing the file length in advance. There is a couple
solutions/workarounds:

   - We can always provide the drag-out download in non-streaming way. That
   is, we will use our DownloadManager to download the file first and then
   serve it in CF_HDROP format. One disadvantage of this approach is that we
   have to buffer the whole temporary download file.
   - We can modify the drag-out API to provide more information when the web
   application calls SetData. Currently we propose to use
   "SetData('DownloadURL', 'http://...'). Could we consider adding more
   metadata information into SetData? Maybe the web application needs to call
   "SetData('DownloadURLSize', SomeSize)" if the server could serve the data in
   chunked encoding.



>
>
>>
>> http://codereview.chromium.org/351029/diff/18022/20100
>> File chrome/browser/drag_download_stream_win.h (right):
>>
>> http://codereview.chromium.org/351029/diff/18022/20100#newcode6
>> chrome/browser/drag_download_stream_win.h:6: #define
>> CHROME_BROWSER_DRAG_DOWNLOAD_STREAM_WIN_H_
>> On 2009/12/17 01:03:44, darin wrote:
>>
>>> typically, you should have one class per header file.  how about
>>>
>> breaking these
>>
>>> up into several files?  at that point though we are creating a lot of
>>>
>> files.  it
>>
>>> seems like it would be good to introduce another subdirectory of
>>>
>> browser, or
>>
>>> perhaps these files can just go into browser/download since they have
>>>
>> something
>>
>>> to do with downloading.
>>>
>>
>> Break into multiple files and move all drag_download* files to
>> browser/download. The ZoneData class is renamed as ZoneIdentifierADS and
>> move into win_safe_util.*.
>
>
> OK
>
>
>
>>
>>
>> http://codereview.chromium.org/351029/diff/18022/20100#newcode21
>> chrome/browser/drag_download_stream_win.h:21: class ZoneData {
>> On 2009/12/17 01:03:44, darin wrote:
>>
>>> this class needs some documentation.
>>>
>>
>> Done.
>>
>>
>> http://codereview.chromium.org/351029/diff/18022/20100#newcode25
>> chrome/browser/drag_download_stream_win.h:25: static int
>> GetStreamSize();
>> On 2009/12/17 01:03:44, darin wrote:
>>
>>> this should have a DISALLOW_IMPLICIT_CONSTRUCTORS
>>>
>>
>> Done.
>>
>>
>> http://codereview.chromium.org/351029/diff/18022/20100#newcode28
>> chrome/browser/drag_download_stream_win.h:28: class
>> DragDownloadFileStream
>> On 2009/12/17 01:03:44, darin wrote:
>>
>>> class DragDownloadFileStream : public IStream,
>>>                                public
>>>
>> OSExchangeData::DownloadFileProvider,
>>
>>>                                ...
>>>
>>
>>
>> Done.
>>
>>
>> http://codereview.chromium.org/351029/diff/18022/20100#newcode32
>> chrome/browser/drag_download_stream_win.h:32: static const int
>> kBufferSize = 32768;
>> On 2009/12/17 01:03:44, darin wrote:
>>
>>> please move these down into the private section.
>>>
>>
>> Done.
>>
>>
>> http://codereview.chromium.org/351029/diff/18022/20100#newcode84
>> chrome/browser/drag_download_stream_win.h:84: // Accessors.
>> On 2009/12/17 01:03:44, darin wrote:
>>
>>> do we care what thread this is called on?  based on the comment below
>>>
>> for
>>
>>> filename_, i'd assume that this may only be called on the I/O thread.
>>>
>> we should
>>
>>> probably assert that.
>>>
>>
>> Removed since it is not used any more.
>>
>>
> OK, /me likes code removal :)
>
>
>
>
>>
>> http://codereview.chromium.org/351029/diff/18022/20100#newcode109
>> chrome/browser/drag_download_stream_win.h:109: URLRequestContextGetter*
>> request_context_getter_;
>> On 2009/12/17 01:03:44, darin wrote:
>>
>>> i think you need to keep an owning reference to the
>>>
>> URLRequestContextGetter.
>>
>>> use a scoped_refptr.  or do you have a guarantee that this will remain
>>>
>> alive
>>
>>> longer than a DragDownloadFileStream instance?
>>>
>>
>> Changed to use scoped_refptr.
>>
>>
> OK
>
>
>
>>  http://codereview.chromium.org/351029/diff/18022/20100#newcode147
>> chrome/browser/drag_download_stream_win.h:147: class
>> DragDownloadDataStream
>> On 2009/12/17 01:03:44, darin wrote:
>>
>>> any rules about what thread this gets accessed on?  by the way, it
>>>
>> would be good
>>
>>> to write a comment that provides an overview of the DnD architecture
>>>
>> for
>>
>>> Windows.
>>>
>>
>> This class is always used on DnD thread since it is used to provide the
>> static data only and I added the comment for this.
>>
>>
> Thanks
>
>
>
>> I've added the detailed comment in
>> TabContentsDragWin::PrepareDragForDownload (tab_content_drag_win.cc).
>>
>>
> Thanks
>
>
>
>
>>  http://codereview.chromium.org/351029/diff/18022/20129#newcode39
>> chrome/browser/views/tab_contents/tab_contents_drag_win.cc:39:
>> msg->message == WM_KEYDOWN || msg->message == WM_KEYUP) {
>> On 2009/12/17 21:43:21, darin wrote:
>>
>>> what about WM_SYSKEY{DOWN,UP}?  can we ignore those?
>>>
>>
>> I think we do not care about WM_SYSKEY* because when ALT is pressed down
>> while holding the left button, it is meant to create a link.
>>
>>
> OK... please add a comment explaining this.
>

Done.

>
>
> 
/18022/20129#newcode77<http://codereview.chromium.org/351029/diff/18022/20129#newcode77>
>
>> chrome/browser/views/tab_contents/tab_contents_drag_win.cc:77: // Hold a
>> reference count to TabContentsDragWin to make sure that it is always
>> On 2009/12/17 21:43:21, darin wrote:
>>
>>> do you care what thread the TabContentsDragWin dies on?  maybe it
>>>
>> would be good
>>
>>> for it to always be deleted on the UI thread?
>>>
>>
>> TabContentsDragWin is always destructed on UI thread. Added the DCHECK
>> for it.
>>
>>
> Thanks
>
>
>
>  http://codereview.chromium.org/351029
>>
>

[darin (slow to review), 2009-12-21 04:59:55]

On Fri, Dec 18, 2009 at 2:20 PM, Jian Li <jianli@chromium.org> wrote:

> Thanks a lot.
>
> On Fri, Dec 18, 2009 at 10:37 AM, Darin Fisher <darin@chromium.org> wrote:
>
>>
>>
>> On Thu, Dec 17, 2009 at 3:27 PM, <jianli@chromium.org> wrote:
>> ...
>>
>>> http://codereview.chromium.org/351029/diff/18022/20102#newcode101
>>> chrome/browser/drag_download_file.cc:101: void
>>> DragDownloadFile::SetDownloads(std::vector<DownloadItem*>& downloads) {
>>> On 2009/12/17 01:03:44, darin wrote:
>>>
>>>> in google style, any reference parameter should be const.  in this
>>>>
>>> case, it
>>>
>>>> looks like you only read from downloads, so better make it a "const
>>>> std::vector<T>&"
>>>>
>>>
>>> This is because DownloadManager::Observer defined SetDownloads in such
>>> way.  I can not add const modifier to the parameter because one
>>> implementation
>>> DownloadsDOMHandler::SetDownloads(std::vector<DownloadItem*>& downloads)
>>> uses something like the following:
>>>   download_items_.swap(downloads);
>>>
>>
>> It seems like an unfortunate design.  I bet switching to
>> std::vector<linked_ptr<T> > would be all around superior.
>>
>>
>>
>>
>>>
>>> Do you want me to fix all of these to use pointer:
>>>  virtual void SetDownloads(std::vector<DownloadItem*>* downloads) = 0;
>>>
>>>
>> This can be a follow-up CL.
>>
>>
>> Will do.
>
>>
>>
>>>
>>> http://codereview.chromium.org/351029/diff/18022/20102#newcode163
>>> chrome/browser/drag_download_file.cc:163:
>>> observer_->OnDataReady(format_, downloads);
>>> On 2009/12/17 01:03:44, darin wrote:
>>>
>>>> Q: who is responsible for freeing downloads[0]?
>>>>
>>>
>>>  if OnDataReady does not take ownership of downloads[0], then memory
>>>>
>>> will be
>>>
>>>> leaked.  perhaps it would help to use linked_ptr<T>, so you'd instead
>>>>
>>> have:
>>>
>>>  std::vector< linked_ptr<OSExchangeData::DownloadFileInfo> > downloads;
>>>>
>>>
>>>  A typedef might be nice for that if you go this route.
>>>>
>>> (Alternatively, you
>>>
>>>> could also make DownloadFileInfo reference counted.)
>>>>
>>>
>>> DataObjectImpl::OnDataReady will take the ownership. It is destroyed on
>>> DataObjectImpl::StopDownloads in the inner for loop:
>>>  delete (*iter)->downloads[i];
>>
>>
>> I see that too, but what if the inner loop code isn't reached?  Please
>> just make
>> sure memory isn't being leaked in unusual cases.
>>
>>
>> The destructor will always be called by OLE when it succeeds or fails the
> drag-and-drop.
>
> The only bad thing is that the user closes the Chrome application while
> drag-out is still in progress. I will try to figure out how to address this
> issue.
>
>>
>>
>>>  http://codereview.chromium.org/351029/diff/18022/20101
>>> File chrome/browser/drag_download_stream_win.cc (right):
>>>
>>> http://codereview.chromium.org/351029/diff/18022/20101#newcode73
>>> chrome/browser/drag_download_stream_win.cc:73: DCHECK(!request_);
>>> On 2009/12/17 01:03:44, darin wrote:
>>>
>>>> please assert that this is called on the DnD thread.
>>>>
>>>
>>> The destructor could be called on either drag-and-drop thread or I/O
>>> thread depending on how the last reference count is released. The
>>> RunnableMethod cleanup from either thread might be the last one. I added
>>> the comment for this.
>>>
>>>
>> OK
>>
>>
>>
>>>
>>> http://codereview.chromium.org/351029/diff/18022/20101#newcode76
>>> chrome/browser/drag_download_stream_win.cc:76: bool
>>> DragDownloadFileStream::Start(
>>> On 2009/12/17 01:03:44, darin wrote:
>>>
>>>> this function looks a lot like DragDownloadFile::Start.  can you avoid
>>>>
>>> code
>>>
>>>> duplication?
>>>>
>>>
>>> Refactor some duplicate logic in DragDownloadFile and
>>> DragDownloadFileStream into a base class DragDownloadFileBase.
>>>
>>>
>> Thanks!
>>
>>
>>>
>>> http://codereview.chromium.org/351029/diff/18022/20101#newcode182
>>> chrome/browser/drag_download_stream_win.cc:182: // Determine if we need
>>> to sniff the content to overwrite the mime type.
>>> On 2009/12/17 01:03:44, darin wrote:
>>>
>>>> this looks a lot like BufferedResourceHandler::DelayResponse.  can you
>>>>
>>> avoid
>>>
>>>> duplicating this code?
>>>>
>>>
>>> Yes, I borrowed the logic from BufferedResourceHandler::DelayResponse
>>> since the logic there cannot be used directly without a lot refactoring.
>>> I agree we should avoid duplicating the code. So I am going to refactor
>>> the code there. Since the current patch is already very complicated,
>>> could we do the refactoring not in this patch? I put a TODO here and
>>> will get to this with my next patch.
>>>
>>>
>> OK
>>
>>
>>
>>>
>>> http://codereview.chromium.org/351029/diff/18022/20101#newcode246
>>> chrome/browser/drag_download_stream_win.cc:246:
>>> observer_->OnDataReady(format_, downloads);
>>> On 2009/12/17 01:03:44, darin wrote:
>>>
>>>> same comment as before regarding OnDataReady.  are you sure that it
>>>>
>>> always takes
>>>
>>>> ownership of the given downloads?  are you sure we don't leak memory
>>>>
>>> in some
>>>
>>>> cases?
>>>>
>>>
>>> Yes. It is deleted on DataObjectImpl::StopDownloads. StopDownloads() is
>>> called when download is cancelled or DataObjectImpl is destructed.
>>>
>>>
>> OK
>>
>>
>>
>>>
>>> http://codereview.chromium.org/351029/diff/18022/20101#newcode259
>>> chrome/browser/drag_download_stream_win.cc:259: // If we have the unread
>>> data resulted from the content sniff leftover, serve
>>> On 2009/12/17 01:03:44, darin wrote:
>>>
>>>> it seems really unfortunate to have to duplicate delicate content
>>>>
>>> sniffing
>>>
>>>> logic.  we shouldn't have to do that.  some refactoring should be done
>>>>
>>> to avoid
>>>
>>>> it.
>>>>
>>>
>>> I will do refactoring to use the logic from
>>> BufferedResourceHandler::DelayResponse. See my comment above.
>>>
>>>
>> OK
>>
>>
>>
>>>  http://codereview.chromium.org/351029/diff/18022/20101#newcode490
>>> chrome/browser/drag_download_stream_win.cc:490: // in the response
>>> header.
>>> On 2009/12/17 01:03:44, darin wrote:
>>>
>>>> oh, this seems like a big deal.  i don't think we should enable this
>>>>
>>> feature
>>>
>>>> until we have a solution to this.  or, we need to disable the CHECK to
>>>>
>>> avoid
>>>
>>>> crashing.
>>>>
>>>
>>> I think normally we should expect Content-Length in the response header.
>>> If it is not there, we treat it as error.
>>>
>>>
>> OK.... it seems like files that are generated server-side typically do not
>> have a C-L.  They instead use chunked encoding so that the server doesn't
>> have to generate the complete file before sending it.  I'm surprised that
>> we
>> wouldn't need to worry about supporting chunked encoding use cases.
>>
>>
>> Thank you for catching this. I do not find a way to make Shell work
> without providing the file length in advance. There is a couple
> solutions/workarounds:
>
>    - We can always provide the drag-out download in non-streaming way.
>    That is, we will use our DownloadManager to download the file first and
then
>    serve it in CF_HDROP format. One disadvantage of this approach is that we
>    have to buffer the whole temporary download file.
>    - We can modify the drag-out API to provide more information when the
>    web application calls SetData. Currently we propose to use
>    "SetData('DownloadURL', 'http://...'). Could we consider adding more
>    metadata information into SetData? Maybe the web application needs to call
>    "SetData('DownloadURLSize', SomeSize)" if the server could serve the data
in
>    chunked encoding.
>
>
>

The first option seems better to me.  I don't think we should extend the API
to add another way to specify end-of-file.

-Darin



>
>>
>>>
>>> http://codereview.chromium.org/351029/diff/18022/20100
>>> File chrome/browser/drag_download_stream_win.h (right):
>>>
>>> http://codereview.chromium.org/351029/diff/18022/20100#newcode6
>>> chrome/browser/drag_download_stream_win.h:6: #define
>>> CHROME_BROWSER_DRAG_DOWNLOAD_STREAM_WIN_H_
>>> On 2009/12/17 01:03:44, darin wrote:
>>>
>>>> typically, you should have one class per header file.  how about
>>>>
>>> breaking these
>>>
>>>> up into several files?  at that point though we are creating a lot of
>>>>
>>> files.  it
>>>
>>>> seems like it would be good to introduce another subdirectory of
>>>>
>>> browser, or
>>>
>>>> perhaps these files can just go into browser/download since they have
>>>>
>>> something
>>>
>>>> to do with downloading.
>>>>
>>>
>>> Break into multiple files and move all drag_download* files to
>>> browser/download. The ZoneData class is renamed as ZoneIdentifierADS and
>>> move into win_safe_util.*.
>>
>>
>> OK
>>
>>
>>
>>>
>>>
>>> http://codereview.chromium.org/351029/diff/18022/20100#newcode21
>>> chrome/browser/drag_download_stream_win.h:21: class ZoneData {
>>> On 2009/12/17 01:03:44, darin wrote:
>>>
>>>> this class needs some documentation.
>>>>
>>>
>>> Done.
>>>
>>>
>>> http://codereview.chromium.org/351029/diff/18022/20100#newcode25
>>> chrome/browser/drag_download_stream_win.h:25: static int
>>> GetStreamSize();
>>> On 2009/12/17 01:03:44, darin wrote:
>>>
>>>> this should have a DISALLOW_IMPLICIT_CONSTRUCTORS
>>>>
>>>
>>> Done.
>>>
>>>
>>> http://codereview.chromium.org/351029/diff/18022/20100#newcode28
>>> chrome/browser/drag_download_stream_win.h:28: class
>>> DragDownloadFileStream
>>> On 2009/12/17 01:03:44, darin wrote:
>>>
>>>> class DragDownloadFileStream : public IStream,
>>>>                                public
>>>>
>>> OSExchangeData::DownloadFileProvider,
>>>
>>>>                                ...
>>>>
>>>
>>>
>>> Done.
>>>
>>>
>>> http://codereview.chromium.org/351029/diff/18022/20100#newcode32
>>> chrome/browser/drag_download_stream_win.h:32: static const int
>>> kBufferSize = 32768;
>>> On 2009/12/17 01:03:44, darin wrote:
>>>
>>>> please move these down into the private section.
>>>>
>>>
>>> Done.
>>>
>>>
>>> http://codereview.chromium.org/351029/diff/18022/20100#newcode84
>>> chrome/browser/drag_download_stream_win.h:84: // Accessors.
>>> On 2009/12/17 01:03:44, darin wrote:
>>>
>>>> do we care what thread this is called on?  based on the comment below
>>>>
>>> for
>>>
>>>> filename_, i'd assume that this may only be called on the I/O thread.
>>>>
>>> we should
>>>
>>>> probably assert that.
>>>>
>>>
>>> Removed since it is not used any more.
>>>
>>>
>> OK, /me likes code removal :)
>>
>>
>>
>>
>>>
>>> http://codereview.chromium.org/351029/diff/18022/20100#newcode109
>>> chrome/browser/drag_download_stream_win.h:109: URLRequestContextGetter*
>>> request_context_getter_;
>>> On 2009/12/17 01:03:44, darin wrote:
>>>
>>>> i think you need to keep an owning reference to the
>>>>
>>> URLRequestContextGetter.
>>>
>>>> use a scoped_refptr.  or do you have a guarantee that this will remain
>>>>
>>> alive
>>>
>>>> longer than a DragDownloadFileStream instance?
>>>>
>>>
>>> Changed to use scoped_refptr.
>>>
>>>
>> OK
>>
>>
>>
>>>  http://codereview.chromium.org/351029/diff/18022/20100#newcode147
>>> chrome/browser/drag_download_stream_win.h:147: class
>>> DragDownloadDataStream
>>> On 2009/12/17 01:03:44, darin wrote:
>>>
>>>> any rules about what thread this gets accessed on?  by the way, it
>>>>
>>> would be good
>>>
>>>> to write a comment that provides an overview of the DnD architecture
>>>>
>>> for
>>>
>>>> Windows.
>>>>
>>>
>>> This class is always used on DnD thread since it is used to provide the
>>> static data only and I added the comment for this.
>>>
>>>
>> Thanks
>>
>>
>>
>>> I've added the detailed comment in
>>> TabContentsDragWin::PrepareDragForDownload (tab_content_drag_win.cc).
>>>
>>>
>> Thanks
>>
>>
>>
>>
>>>  http://codereview.chromium.org/351029/diff/18022/20129#newcode39
>>> chrome/browser/views/tab_contents/tab_contents_drag_win.cc:39:
>>> msg->message == WM_KEYDOWN || msg->message == WM_KEYUP) {
>>> On 2009/12/17 21:43:21, darin wrote:
>>>
>>>> what about WM_SYSKEY{DOWN,UP}?  can we ignore those?
>>>>
>>>
>>> I think we do not care about WM_SYSKEY* because when ALT is pressed down
>>> while holding the left button, it is meant to create a link.
>>>
>>>
>> OK... please add a comment explaining this.
>>
>
> Done.
>
>>
>>
>> 
/18022/20129#newcode77<http://codereview.chromium.org/351029/diff/18022/20129#newcode77>
>>
>>> chrome/browser/views/tab_contents/tab_contents_drag_win.cc:77: // Hold a
>>> reference count to TabContentsDragWin to make sure that it is always
>>> On 2009/12/17 21:43:21, darin wrote:
>>>
>>>> do you care what thread the TabContentsDragWin dies on?  maybe it
>>>>
>>> would be good
>>>
>>>> for it to always be deleted on the UI thread?
>>>>
>>>
>>> TabContentsDragWin is always destructed on UI thread. Added the DCHECK
>>> for it.
>>>
>>>
>> Thanks
>>
>>
>>
>>  http://codereview.chromium.org/351029
>>>
>>
>

[jianli, 2009-12-21 20:21:15]

Changed to always download the file first. Removes all new files and changed
related to doing download in streaming way.

When the Chrome is closed and download is still in progress, the following
steps will be performed.

   - The download item will be cancelled.
   - DragDownloadFile::OnDownloadUpdated will get notified with the status
   set to DownloadItem::CANCELLED.
   - DragDownloadFile::DownloadCancelled will be called and then the nested
   message loop will be exited.
   - DragDownloadFile::Start will return with the false result to indicate
   the failure.
   - DataObjectImpl::GetData will return DV_E_FORMATETC to OLE.
   - OLE will perform a series of cleanup and finally causes the last
   reference count to DataObjectImpl to be released.

As the result, I do not see any problem here for this scenario.

Could you please review again? Thanks.


On Sun, Dec 20, 2009 at 8:59 PM, Darin Fisher <darin@chromium.org> wrote:

> On Fri, Dec 18, 2009 at 2:20 PM, Jian Li <jianli@chromium.org> wrote:
>
>> Thanks a lot.
>>
>> On Fri, Dec 18, 2009 at 10:37 AM, Darin Fisher <darin@chromium.org>wrote:
>>
>>>
>>>
>>> On Thu, Dec 17, 2009 at 3:27 PM, <jianli@chromium.org> wrote:
>>> ...
>>>
>>>> http://codereview.chromium.org/351029/diff/18022/20102#newcode101
>>>> chrome/browser/drag_download_file.cc:101: void
>>>> DragDownloadFile::SetDownloads(std::vector<DownloadItem*>& downloads) {
>>>> On 2009/12/17 01:03:44, darin wrote:
>>>>
>>>>> in google style, any reference parameter should be const.  in this
>>>>>
>>>> case, it
>>>>
>>>>> looks like you only read from downloads, so better make it a "const
>>>>> std::vector<T>&"
>>>>>
>>>>
>>>> This is because DownloadManager::Observer defined SetDownloads in such
>>>> way.  I can not add const modifier to the parameter because one
>>>> implementation
>>>> DownloadsDOMHandler::SetDownloads(std::vector<DownloadItem*>& downloads)
>>>> uses something like the following:
>>>>   download_items_.swap(downloads);
>>>>
>>>
>>> It seems like an unfortunate design.  I bet switching to
>>> std::vector<linked_ptr<T> > would be all around superior.
>>>
>>>
>>>
>>>
>>>>
>>>> Do you want me to fix all of these to use pointer:
>>>>  virtual void SetDownloads(std::vector<DownloadItem*>* downloads) = 0;
>>>>
>>>>
>>> This can be a follow-up CL.
>>>
>>>
>>> Will do.
>>
>>>
>>>
>>>>
>>>> http://codereview.chromium.org/351029/diff/18022/20102#newcode163
>>>> chrome/browser/drag_download_file.cc:163:
>>>> observer_->OnDataReady(format_, downloads);
>>>> On 2009/12/17 01:03:44, darin wrote:
>>>>
>>>>> Q: who is responsible for freeing downloads[0]?
>>>>>
>>>>
>>>>  if OnDataReady does not take ownership of downloads[0], then memory
>>>>>
>>>> will be
>>>>
>>>>> leaked.  perhaps it would help to use linked_ptr<T>, so you'd instead
>>>>>
>>>> have:
>>>>
>>>>  std::vector< linked_ptr<OSExchangeData::DownloadFileInfo> > downloads;
>>>>>
>>>>
>>>>  A typedef might be nice for that if you go this route.
>>>>>
>>>> (Alternatively, you
>>>>
>>>>> could also make DownloadFileInfo reference counted.)
>>>>>
>>>>
>>>> DataObjectImpl::OnDataReady will take the ownership. It is destroyed on
>>>> DataObjectImpl::StopDownloads in the inner for loop:
>>>>  delete (*iter)->downloads[i];
>>>
>>>
>>> I see that too, but what if the inner loop code isn't reached?  Please
>>> just make
>>> sure memory isn't being leaked in unusual cases.
>>>
>>>
>>> The destructor will always be called by OLE when it succeeds or fails the
>> drag-and-drop.
>>
>> The only bad thing is that the user closes the Chrome application while
>> drag-out is still in progress. I will try to figure out how to address this
>> issue.
>>
>>>
>>>
>>>>  http://codereview.chromium.org/351029/diff/18022/20101
>>>> File chrome/browser/drag_download_stream_win.cc (right):
>>>>
>>>> http://codereview.chromium.org/351029/diff/18022/20101#newcode73
>>>> chrome/browser/drag_download_stream_win.cc:73: DCHECK(!request_);
>>>> On 2009/12/17 01:03:44, darin wrote:
>>>>
>>>>> please assert that this is called on the DnD thread.
>>>>>
>>>>
>>>> The destructor could be called on either drag-and-drop thread or I/O
>>>> thread depending on how the last reference count is released. The
>>>> RunnableMethod cleanup from either thread might be the last one. I added
>>>> the comment for this.
>>>>
>>>>
>>> OK
>>>
>>>
>>>
>>>>
>>>> http://codereview.chromium.org/351029/diff/18022/20101#newcode76
>>>> chrome/browser/drag_download_stream_win.cc:76: bool
>>>> DragDownloadFileStream::Start(
>>>> On 2009/12/17 01:03:44, darin wrote:
>>>>
>>>>> this function looks a lot like DragDownloadFile::Start.  can you avoid
>>>>>
>>>> code
>>>>
>>>>> duplication?
>>>>>
>>>>
>>>> Refactor some duplicate logic in DragDownloadFile and
>>>> DragDownloadFileStream into a base class DragDownloadFileBase.
>>>>
>>>>
>>> Thanks!
>>>
>>>
>>>>
>>>> http://codereview.chromium.org/351029/diff/18022/20101#newcode182
>>>> chrome/browser/drag_download_stream_win.cc:182: // Determine if we need
>>>> to sniff the content to overwrite the mime type.
>>>> On 2009/12/17 01:03:44, darin wrote:
>>>>
>>>>> this looks a lot like BufferedResourceHandler::DelayResponse.  can you
>>>>>
>>>> avoid
>>>>
>>>>> duplicating this code?
>>>>>
>>>>
>>>> Yes, I borrowed the logic from BufferedResourceHandler::DelayResponse
>>>> since the logic there cannot be used directly without a lot refactoring.
>>>> I agree we should avoid duplicating the code. So I am going to refactor
>>>> the code there. Since the current patch is already very complicated,
>>>> could we do the refactoring not in this patch? I put a TODO here and
>>>> will get to this with my next patch.
>>>>
>>>>
>>> OK
>>>
>>>
>>>
>>>>
>>>> http://codereview.chromium.org/351029/diff/18022/20101#newcode246
>>>> chrome/browser/drag_download_stream_win.cc:246:
>>>> observer_->OnDataReady(format_, downloads);
>>>> On 2009/12/17 01:03:44, darin wrote:
>>>>
>>>>> same comment as before regarding OnDataReady.  are you sure that it
>>>>>
>>>> always takes
>>>>
>>>>> ownership of the given downloads?  are you sure we don't leak memory
>>>>>
>>>> in some
>>>>
>>>>> cases?
>>>>>
>>>>
>>>> Yes. It is deleted on DataObjectImpl::StopDownloads. StopDownloads() is
>>>> called when download is cancelled or DataObjectImpl is destructed.
>>>>
>>>>
>>> OK
>>>
>>>
>>>
>>>>
>>>> http://codereview.chromium.org/351029/diff/18022/20101#newcode259
>>>> chrome/browser/drag_download_stream_win.cc:259: // If we have the unread
>>>> data resulted from the content sniff leftover, serve
>>>> On 2009/12/17 01:03:44, darin wrote:
>>>>
>>>>> it seems really unfortunate to have to duplicate delicate content
>>>>>
>>>> sniffing
>>>>
>>>>> logic.  we shouldn't have to do that.  some refactoring should be done
>>>>>
>>>> to avoid
>>>>
>>>>> it.
>>>>>
>>>>
>>>> I will do refactoring to use the logic from
>>>> BufferedResourceHandler::DelayResponse. See my comment above.
>>>>
>>>>
>>> OK
>>>
>>>
>>>
>>>>  http://codereview.chromium.org/351029/diff/18022/20101#newcode490
>>>> chrome/browser/drag_download_stream_win.cc:490: // in the response
>>>> header.
>>>> On 2009/12/17 01:03:44, darin wrote:
>>>>
>>>>> oh, this seems like a big deal.  i don't think we should enable this
>>>>>
>>>> feature
>>>>
>>>>> until we have a solution to this.  or, we need to disable the CHECK to
>>>>>
>>>> avoid
>>>>
>>>>> crashing.
>>>>>
>>>>
>>>> I think normally we should expect Content-Length in the response header.
>>>> If it is not there, we treat it as error.
>>>>
>>>>
>>> OK.... it seems like files that are generated server-side typically do
>>> not
>>> have a C-L.  They instead use chunked encoding so that the server doesn't
>>> have to generate the complete file before sending it.  I'm surprised that
>>> we
>>> wouldn't need to worry about supporting chunked encoding use cases.
>>>
>>>
>>> Thank you for catching this. I do not find a way to make Shell work
>> without providing the file length in advance. There is a couple
>> solutions/workarounds:
>>
>>    - We can always provide the drag-out download in non-streaming way.
>>    That is, we will use our DownloadManager to download the file first and
then
>>    serve it in CF_HDROP format. One disadvantage of this approach is that we
>>    have to buffer the whole temporary download file.
>>    - We can modify the drag-out API to provide more information when the
>>    web application calls SetData. Currently we propose to use
>>    "SetData('DownloadURL', 'http://...'). Could we consider adding more
>>    metadata information into SetData? Maybe the web application needs to call
>>    "SetData('DownloadURLSize', SomeSize)" if the server could serve the data
in
>>    chunked encoding.
>>
>>
>>
>
> The first option seems better to me.  I don't think we should extend the
> API to add another way to specify end-of-file.
>
> -Darin
>
>
>
>>
>>>
>>>>
>>>> http://codereview.chromium.org/351029/diff/18022/20100
>>>> File chrome/browser/drag_download_stream_win.h (right):
>>>>
>>>> http://codereview.chromium.org/351029/diff/18022/20100#newcode6
>>>> chrome/browser/drag_download_stream_win.h:6: #define
>>>> CHROME_BROWSER_DRAG_DOWNLOAD_STREAM_WIN_H_
>>>> On 2009/12/17 01:03:44, darin wrote:
>>>>
>>>>> typically, you should have one class per header file.  how about
>>>>>
>>>> breaking these
>>>>
>>>>> up into several files?  at that point though we are creating a lot of
>>>>>
>>>> files.  it
>>>>
>>>>> seems like it would be good to introduce another subdirectory of
>>>>>
>>>> browser, or
>>>>
>>>>> perhaps these files can just go into browser/download since they have
>>>>>
>>>> something
>>>>
>>>>> to do with downloading.
>>>>>
>>>>
>>>> Break into multiple files and move all drag_download* files to
>>>> browser/download. The ZoneData class is renamed as ZoneIdentifierADS and
>>>> move into win_safe_util.*.
>>>
>>>
>>> OK
>>>
>>>
>>>
>>>>
>>>>
>>>> http://codereview.chromium.org/351029/diff/18022/20100#newcode21
>>>> chrome/browser/drag_download_stream_win.h:21: class ZoneData {
>>>> On 2009/12/17 01:03:44, darin wrote:
>>>>
>>>>> this class needs some documentation.
>>>>>
>>>>
>>>> Done.
>>>>
>>>>
>>>> http://codereview.chromium.org/351029/diff/18022/20100#newcode25
>>>> chrome/browser/drag_download_stream_win.h:25: static int
>>>> GetStreamSize();
>>>> On 2009/12/17 01:03:44, darin wrote:
>>>>
>>>>> this should have a DISALLOW_IMPLICIT_CONSTRUCTORS
>>>>>
>>>>
>>>> Done.
>>>>
>>>>
>>>> http://codereview.chromium.org/351029/diff/18022/20100#newcode28
>>>> chrome/browser/drag_download_stream_win.h:28: class
>>>> DragDownloadFileStream
>>>> On 2009/12/17 01:03:44, darin wrote:
>>>>
>>>>> class DragDownloadFileStream : public IStream,
>>>>>                                public
>>>>>
>>>> OSExchangeData::DownloadFileProvider,
>>>>
>>>>>                                ...
>>>>>
>>>>
>>>>
>>>> Done.
>>>>
>>>>
>>>> http://codereview.chromium.org/351029/diff/18022/20100#newcode32
>>>> chrome/browser/drag_download_stream_win.h:32: static const int
>>>> kBufferSize = 32768;
>>>> On 2009/12/17 01:03:44, darin wrote:
>>>>
>>>>> please move these down into the private section.
>>>>>
>>>>
>>>> Done.
>>>>
>>>>
>>>> http://codereview.chromium.org/351029/diff/18022/20100#newcode84
>>>> chrome/browser/drag_download_stream_win.h:84: // Accessors.
>>>> On 2009/12/17 01:03:44, darin wrote:
>>>>
>>>>> do we care what thread this is called on?  based on the comment below
>>>>>
>>>> for
>>>>
>>>>> filename_, i'd assume that this may only be called on the I/O thread.
>>>>>
>>>> we should
>>>>
>>>>> probably assert that.
>>>>>
>>>>
>>>> Removed since it is not used any more.
>>>>
>>>>
>>> OK, /me likes code removal :)
>>>
>>>
>>>
>>>
>>>>
>>>> http://codereview.chromium.org/351029/diff/18022/20100#newcode109
>>>> chrome/browser/drag_download_stream_win.h:109: URLRequestContextGetter*
>>>> request_context_getter_;
>>>> On 2009/12/17 01:03:44, darin wrote:
>>>>
>>>>> i think you need to keep an owning reference to the
>>>>>
>>>> URLRequestContextGetter.
>>>>
>>>>> use a scoped_refptr.  or do you have a guarantee that this will remain
>>>>>
>>>> alive
>>>>
>>>>> longer than a DragDownloadFileStream instance?
>>>>>
>>>>
>>>> Changed to use scoped_refptr.
>>>>
>>>>
>>> OK
>>>
>>>
>>>
>>>>  http://codereview.chromium.org/351029/diff/18022/20100#newcode147
>>>> chrome/browser/drag_download_stream_win.h:147: class
>>>> DragDownloadDataStream
>>>> On 2009/12/17 01:03:44, darin wrote:
>>>>
>>>>> any rules about what thread this gets accessed on?  by the way, it
>>>>>
>>>> would be good
>>>>
>>>>> to write a comment that provides an overview of the DnD architecture
>>>>>
>>>> for
>>>>
>>>>> Windows.
>>>>>
>>>>
>>>> This class is always used on DnD thread since it is used to provide the
>>>> static data only and I added the comment for this.
>>>>
>>>>
>>> Thanks
>>>
>>>
>>>
>>>> I've added the detailed comment in
>>>> TabContentsDragWin::PrepareDragForDownload (tab_content_drag_win.cc).
>>>>
>>>>
>>> Thanks
>>>
>>>
>>>
>>>
>>>>  http://codereview.chromium.org/351029/diff/18022/20129#newcode39
>>>> chrome/browser/views/tab_contents/tab_contents_drag_win.cc:39:
>>>> msg->message == WM_KEYDOWN || msg->message == WM_KEYUP) {
>>>> On 2009/12/17 21:43:21, darin wrote:
>>>>
>>>>> what about WM_SYSKEY{DOWN,UP}?  can we ignore those?
>>>>>
>>>>
>>>> I think we do not care about WM_SYSKEY* because when ALT is pressed down
>>>> while holding the left button, it is meant to create a link.
>>>>
>>>>
>>> OK... please add a comment explaining this.
>>>
>>
>> Done.
>>
>>>
>>>
>>> 
/18022/20129#newcode77<http://codereview.chromium.org/351029/diff/18022/20129#newcode77>
>>>
>>>> chrome/browser/views/tab_contents/tab_contents_drag_win.cc:77: // Hold a
>>>> reference count to TabContentsDragWin to make sure that it is always
>>>> On 2009/12/17 21:43:21, darin wrote:
>>>>
>>>>> do you care what thread the TabContentsDragWin dies on?  maybe it
>>>>>
>>>> would be good
>>>>
>>>>> for it to always be deleted on the UI thread?
>>>>>
>>>>
>>>> TabContentsDragWin is always destructed on UI thread. Added the DCHECK
>>>> for it.
>>>>
>>>>
>>> Thanks
>>>
>>>
>>>
>>>  http://codereview.chromium.org/351029
>>>>
>>>
>>
>

[darin (slow to review), 2010-01-04 19:35:11]

I like the overall simplification.

http://codereview.chromium.org/351029/diff/23024/23028
File app/os_exchange_data.h (right):

http://codereview.chromium.org/351029/diff/23024/23028#newcode71
app/os_exchange_data.h:71: const std::vector<DownloadFileInfo*>& downloads) = 0;
please document that ownership of the DownloadFileInfo objects is being passed
to OnDataReady.  this is a very fragile way of memory management, so it needs
clear documentation.

http://codereview.chromium.org/351029/diff/23024/23026
File app/os_exchange_data_provider_win.cc (right):

http://codereview.chromium.org/351029/diff/23024/23026#newcode457
app/os_exchange_data_provider_win.cc:457: void
OSExchangeDataProviderWin::SetDownloadFileName(
the name of this method seems pretty poor.  it is doing more than just naming a
file.  there are side effects here.  the parameter is being stored in a vector. 
maybe AddDownloadFileInfo would be a better name?  i'm not sure.

http://codereview.chromium.org/351029/diff/23024/23026#newcode583
app/os_exchange_data_provider_win.cc:583: // Update the downloads.
it seems like you should DCHECK that we reach this code.  otherwise, we might
miss some memory leaks of the DownloadFileInfo objects.

also, you should DCHECK that downloads.size() == (*iter)->downloads.size()

http://codereview.chromium.org/351029/diff/23024/23027
File app/os_exchange_data_provider_win.h (right):

http://codereview.chromium.org/351029/diff/23024/23027#newcode80
app/os_exchange_data_provider_win.h:80:
std::vector<OSExchangeData::DownloadFileInfo*> downloads;
who owns the pointers held by this vector?  maybe you should be using
linked_ptr.

http://codereview.chromium.org/351029/diff/23024/23027#newcode109
app/os_exchange_data_provider_win.h:109: downloads.clear();
does this leak memory?  how do the members of the downloads array get freed?

http://codereview.chromium.org/351029/diff/23024/23049
File chrome/browser/download/download_manager.h (right):

http://codereview.chromium.org/351029/diff/23024/23049#newcode555
chrome/browser/download/download_manager.h:555: FilePath* generated_name);
please indent this line so that the parameters are in a column

void Foo(A* a,
         B* b);

http://codereview.chromium.org/351029/diff/23024/23068
File chrome/browser/download/drag_download_file_win.h (right):

http://codereview.chromium.org/351029/diff/23024/23068#newcode68
chrome/browser/download/drag_download_file_win.h:68: // Acessed on drag-and-drop
thread.
nit: Acessed -> Accessed

http://codereview.chromium.org/351029/diff/23024/23054
File chrome/browser/tab_contents/web_drag_source_win.cc (right):

http://codereview.chromium.org/351029/diff/23024/23054#newcode60
chrome/browser/tab_contents/web_drag_source_win.cc:60: WebDragSource* source_;
how do you ensure that this WebDragSource pointer remains valid between the time
that the WebDragSourceUIThreadTask is allocated and Run is called?

[jianli, 2010-01-05 01:50:04]

All fixed. Thanks.

http://codereview.chromium.org/351029/diff/23024/23028
File app/os_exchange_data.h (right):

http://codereview.chromium.org/351029/diff/23024/23028#newcode71
app/os_exchange_data.h:71: const std::vector<DownloadFileInfo*>& downloads) = 0;
On 2010/01/04 19:35:12, darin wrote:
> please document that ownership of the DownloadFileInfo objects is being passed
> to OnDataReady.  this is a very fragile way of memory management, so it needs
> clear documentation.

Done.

http://codereview.chromium.org/351029/diff/23024/23026
File app/os_exchange_data_provider_win.cc (right):

http://codereview.chromium.org/351029/diff/23024/23026#newcode457
app/os_exchange_data_provider_win.cc:457: void
OSExchangeDataProviderWin::SetDownloadFileName(
On 2010/01/04 19:35:12, darin wrote:
> the name of this method seems pretty poor.  it is doing more than just naming
a
> file.  there are side effects here.  the parameter is being stored in a
vector. 
> maybe AddDownloadFileInfo would be a better name?  i'm not sure.

Renamed to SetDownloadFileInfo. The reason I am naming it as Set*** instead of
Add* is to be consistent with other methods like SetFileContents and SetHtml.
Both of them also store the information in a vector.

http://codereview.chromium.org/351029/diff/23024/23026#newcode583
app/os_exchange_data_provider_win.cc:583: // Update the downloads.
On 2010/01/04 19:35:12, darin wrote:
> it seems like you should DCHECK that we reach this code.  otherwise, we might
> miss some memory leaks of the DownloadFileInfo objects.
> 
> also, you should DCHECK that downloads.size() == (*iter)->downloads.size()

Done.

http://codereview.chromium.org/351029/diff/23024/23027
File app/os_exchange_data_provider_win.h (right):

http://codereview.chromium.org/351029/diff/23024/23027#newcode80
app/os_exchange_data_provider_win.h:80:
std::vector<OSExchangeData::DownloadFileInfo*> downloads;
On 2010/01/04 19:35:12, darin wrote:
> who owns the pointers held by this vector?  maybe you should be using
> linked_ptr.

DataObjectImpl owns it. DataObjectImpl::StopDownloads will destroy the pointers
in this vector:
  delete (*iter)->downloads[i];

http://codereview.chromium.org/351029/diff/23024/23027#newcode109
app/os_exchange_data_provider_win.h:109: downloads.clear();
On 2010/01/04 19:35:12, darin wrote:
> does this leak memory?  how do the members of the downloads array get freed?

No, it does not leak memory. When DataObjectImpl is destructed,
STLDeleteContainerPointers is called to free StoredDataInfo objects.
  STLDeleteContainerPointers(contents_.begin(), contents_.end());

http://codereview.chromium.org/351029/diff/23024/23049
File chrome/browser/download/download_manager.h (right):

http://codereview.chromium.org/351029/diff/23024/23049#newcode555
chrome/browser/download/download_manager.h:555: FilePath* generated_name);
On 2010/01/04 19:35:12, darin wrote:
> please indent this line so that the parameters are in a column
> 
> void Foo(A* a,
>          B* b);

Done.

http://codereview.chromium.org/351029/diff/23024/23068
File chrome/browser/download/drag_download_file_win.h (right):

http://codereview.chromium.org/351029/diff/23024/23068#newcode68
chrome/browser/download/drag_download_file_win.h:68: // Acessed on drag-and-drop
thread.
On 2010/01/04 19:35:12, darin wrote:
> nit: Acessed -> Accessed

Done.

http://codereview.chromium.org/351029/diff/23024/23054
File chrome/browser/tab_contents/web_drag_source_win.cc (right):

http://codereview.chromium.org/351029/diff/23024/23054#newcode60
chrome/browser/tab_contents/web_drag_source_win.cc:60: WebDragSource* source_;
On 2010/01/04 19:35:12, darin wrote:
> how do you ensure that this WebDragSource pointer remains valid between the
time
> that the WebDragSourceUIThreadTask is allocated and Run is called?

Good catch. Changed to add base::RefCountedThreadSafe to BaseDragSource so that
we can post task to itself.

[darin (slow to review), 2010-01-13 07:06:31]

OK, LGTM