thanks for the investigation, but try to write all this information in the same thread, this is really hard to follow since we need to look at 4 different threads and 1 bug to get all the info. You should also write all that to the bug report.
Nicolas On Sun, Sep 28, 2008 at 2:20 AM, HLS <[EMAIL PROTECTED]> wrote: > > Hi, > > I am really trying to figure out the properly fix to this problem > without revamping the DownloadManager and DownloadFileManager > interfaction. I'm surprise that the cog responsible has not provided > any feed back on this problem. > > Without attempting to be critical of the convoluted flow back and > forth between these two threads (which obviously can be simplified), > it may be a simple matter of not fully understanding the intentions > here. > > Tracing this, the file requested resource_dispatcher_host.cc class > finally issued a dispatch > > DownloadFileManager:DownloadFinished() > > and this is basically the sequence of events from this function: > > (note: --> means PostTask) > > T1 ---> DownloadFileManager::OnDownloadFinished() > T2 ---> DownloadManager::DownloadFinished() > T3 OpenDownloadInShell() > T4 ---> DownloadFileManager::OnOpenDownloadInShell() > T5 win_util::OpenItemViaShell() <=== error 2 > (file does not exist yet) > T6 ---> DownloadFileManager::OnFinalDownloadName() > T7 download->Rename(full_path) <=== final copy > is saved from temp > > So it looks like a synchronization issue at T5 and T7 as T7 has not > occured by the time the Shell is attempted at T5. > > I can see how this can be missed because if you were testing this with > the same file and it already exist, then that file can be used for the > shell. > > The is exactly what I am seeing, Chrome does not open the file the > first time, subsequent times it may IFF (if and only if) the same file > name is used. So the automatic sequencing appears to break the > "always open this type" logic as well. > > I was able to get around that by adding a test preference > "download.overwrite_existing: true" so that the sequencing is skipped > but it still fails the first time when the file doesn't exist. > > The complexity here that you may have to reorganize some code and/or > add the usage of kernel synchonization objects which I was surprise is > lacking in this code. > > The solution may also depend on whether the shellexec should, under > some mime_type condition, use the temporary file instead in T5. But > if its going to be based on the final download path storage, then T5 > (actually starting at T3) has to be syncrhronized with T7. > > I think if the design consideration is to allow for both cases (shell > the temp or the final storage), then in either case, the T3 may be > replaced with a OnStorageComplete logic which is dispatched by > DownloadFileManager::OnFinalDownloadName() after the T7 Rename takes > place. > > Then the OnStorageComplete() handler can determine what file to use. > > However, we have a chicken and egg here because at T7 the > DownloadFile instance is destroyed and the temporary file is > deleted. The probable fix is to get DownloadFile::Rename() and > overload bool option to not delete the temp file. > > Anyway, as you can see, its a bit convoluted. > > Comments? > > -- > HLS > > > On Sep 27, 9:31 am, HLS <[EMAIL PROTECTED]> wrote: > > Hi, I spent the night learning the code and trying to resolve this > > issue: > > > > http://code.google.com/p/chromium/issues/detail?id=2292 > > > > I think I find the problem but I am not smart enough yet to undestand > > the synchronization of the data flow, threads and/or events. > > > > The bottom line is that by the time this function in src\chrome\common > > \win_util.cc called, the file has yet to exist, thus yielding an > > error 2 (not found). > > > > bool OpenItemViaShell(const std::wstring& full_path, bool ask_for_app) > > { > > HINSTANCE h = ::ShellExecuteW( > > NULL, NULL, full_path.c_str(), NULL, > > file_util::GetDirectoryFromPath(full_path).c_str(), > > SW_SHOWNORMAL); > > > > LONG_PTR error = reinterpret_cast<LONG_PTR>(h); > > if (error > 32) > > return true; > > > > if ((error == SE_ERR_NOASSOC) && ask_for_app) > > return OpenItemWithExternalApp(full_path); > > > > return false; > > > > } > > > > I insolated this function into a testshell.cpp program and it works > > fine with the shell association. > > > > I fixed the download directory to something I can easily watch and the > > file simply does not exist by the time this function is called by, I > > believe, in > > > > src\chrome\browser\download\download_file.cc > > > > with the function void DownloadFileManager::DownloadFinished(). > > > > I tried to followed the logic with this, but there are two another > > problem associated with this that maybe can be resolved at the same > > time: > > > > 1) These files doe not have to be saved, they can remain in the > > temorary folder, but even if they remain in the download folder, then > > > > 2) We really need an option to stop the auto-sequential numbering of > > existing files. It is simply annoying and all it will do is accumulate > > files for no reason. > > > > If people prefer that I roll my sleeves and try to fix it, there is > > only one problem with that. I know how I am and I will totally revamp > > it. I prefer not to do this. > > > > Can someone on the team take control of this and try to address this? > > > > - Fix the timing issue, > > > > - Dictionary preference option to not sequence existing file > > > > Download.Overwrite_Existing: true <-- default? > > > > Time to get some sleep. > > > > Thanks > > > > -- > > HLS > > > --~--~---------~--~----~------------~-------~--~----~ You received this message because you are subscribed to the Google Groups "Chromium-dev" group. To post to this group, send email to chromium-dev@googlegroups.com To unsubscribe from this group, send email to [EMAIL PROTECTED] For more options, visit this group at http://groups.google.com/group/chromium-dev?hl=en -~----------~----~----~----~------~----~------~--~---
