[chromium-dev] Re: Tweaking DownloadManager

[Yuta Kitamura]

Hi,

2009/5/22 Adam Barth 

> In general, we prefer message passing to locking.  You might consider
> posting a task back to the original thread to notify the download item
> about whether the shell execute succeeded.


While I was looking at the design doc and source code about threading, I
realized that there is a way to avoid locking. I want to try it out.

Thanks,
Yuta

--~--~-~--~~~---~--~~
Chromium Developers mailing list: chromium-dev@googlegroups.com 
View archives, change email options, or unsubscribe: 
http://groups.google.com/group/chromium-dev
-~--~~~~--~~--~--~---

[chromium-dev] Re: Tweaking DownloadManager

[坊野 博典]

Hi Yuta,

I just provided another possible option that solves Issue 3551
"Download menu items should be disabled when downloaded file has been
deleted" and it is definitely up to you which option you take for
solving this issue.

Regards,

Hironori Bono
E-mail: hb...@chromium.org

2009/5/27 Yuta Kitamura :
> Hi Bono-san,
> Thanks for your response.
> I think your resolution has some problems:
> - There is a nasty timing issue; The user faces the same problem if the user
> opens the menu, somebody deletes the file after that, and then the user
> clicks the menu item.
> - There is no way to know the reason why the menu is disabled.
> - You can still open the item by clicking the button (when should we disable
> it?).
> - chrome://downloads/ still has the problem.
> I feel that we need to display some feedback of ShellOpen. Opening a file
> without checking sounds horrible to me.
> Thanks,
> Yuta
> 2009/05/22 20:40 Hironori Bono (坊野 博典) :
>>
>> Hi Yuta,
>>
>> I suspect it is simpler to change
>> DownloadShelfContextMenu::IsItemCommandEnabled() and enable its "open"
>> and "show in folder" items only when the downloaded file exists as
>> shown in my mock code . (Even
>> though this mock code is not tested thoroughly and may cause
>> side-effects, please feel free to copy and use it.)
>>
>> Regards,
>>
>> Hironori Bono
>> E-mail: hb...@chromium.org
>>
>> On Sat, May 23, 2009 at 10:47 AM, Yuta Kitamura  wrote:
>> > Hi all,
>> > I've been researching on DownloadManager for a day in order to to fix
>> > the
>> > issue 3551 . I want to hear some advice from
>> > people
>> > who know Chrome's design well.
>> > The problem of this issue is like the
>> > following: DownloadManager::OpenDownloadInShell() posts a task that
>> > opens a
>> > file using the shell (ShellExecute()) to the file thread. Because this
>> > task
>> > does not return any response (i.e. whether open was successful or not)
>> > back
>> > to the UI thread, DownloadItem cannot receive the result of open and the
>> > browser UI cannot display it either.
>> > So, I want to add the functionality of receiving the result of opening a
>> > file to DownloadItem. My current idea of implementation is:
>> > 1. Let DownloadItem be RefCountedThreadSafe, and rewrite use of
>> > "DownloadItem*" to "scoped_refptr".
>> > 2. Add the interface of receiving the result of OpenDownloadInShell to
>> > DownloadItem. Modify DownloadFileManager::OnOpenDownloadInShell to pass
>> > the
>> > result to it.
>> > 3. Modify UI (DownloadItemView) to reflect these changes.
>> > 4. Write tests.
>> > My question is:
>> > - How do you think about the whole idea? Am I going the right way?
>> > - Step 1: Is it okay to do this? Will this change give the negative
>> > effect
>> > of the browser's speed?
>> > - Step 3: How should the UI look like?
>> > - Step 4: How to do it? Is there a good reference in the existing tests?
>> > (Sorry but I'm not very experienced on testing...)
>> > How do you think? Any idea and suggestion is welcome!
>> > Thanks,
>> > Yuta
>> > >> >
>> >
>
>

--~--~-~--~~~---~--~~
Chromium Developers mailing list: chromium-dev@googlegroups.com 
View archives, change email options, or unsubscribe: 
http://groups.google.com/group/chromium-dev
-~--~~~~--~~--~--~---

[chromium-dev] Re: Tweaking DownloadManager

[Yuta Kitamura]

Hi Bono-san,
Thanks for your response.

I think your resolution has some problems:

- There is a nasty timing issue; The user faces the same problem if the user
opens the menu, somebody deletes the file after that, and then the user
clicks the menu item.
- There is no way to know the reason why the menu is disabled.
- You can still open the item by clicking the button (when should we disable
it?).
- chrome://downloads/ still has the problem.

I feel that we need to display some feedback of ShellOpen. Opening a file
without checking sounds horrible to me.

Thanks,
Yuta

2009/05/22 20:40 Hironori Bono (坊野 博典) :

> Hi Yuta,
>
> I suspect it is simpler to change
> DownloadShelfContextMenu::IsItemCommandEnabled() and enable its "open"
> and "show in folder" items only when the downloaded file exists as
> shown in my mock code . (Even
> though this mock code is not tested thoroughly and may cause
> side-effects, please feel free to copy and use it.)
>
> Regards,
>
> Hironori Bono
> E-mail: hb...@chromium.org
>
> On Sat, May 23, 2009 at 10:47 AM, Yuta Kitamura  wrote:
> > Hi all,
> > I've been researching on DownloadManager for a day in order to to fix the
> > issue 3551 . I want to hear some advice from
> people
> > who know Chrome's design well.
> > The problem of this issue is like the
> > following: DownloadManager::OpenDownloadInShell() posts a task that opens
> a
> > file using the shell (ShellExecute()) to the file thread. Because this
> task
> > does not return any response (i.e. whether open was successful or not)
> back
> > to the UI thread, DownloadItem cannot receive the result of open and the
> > browser UI cannot display it either.
> > So, I want to add the functionality of receiving the result of opening a
> > file to DownloadItem. My current idea of implementation is:
> > 1. Let DownloadItem be RefCountedThreadSafe, and rewrite use of
> > "DownloadItem*" to "scoped_refptr".
> > 2. Add the interface of receiving the result of OpenDownloadInShell to
> > DownloadItem. Modify DownloadFileManager::OnOpenDownloadInShell to pass
> the
> > result to it.
> > 3. Modify UI (DownloadItemView) to reflect these changes.
> > 4. Write tests.
> > My question is:
> > - How do you think about the whole idea? Am I going the right way?
> > - Step 1: Is it okay to do this? Will this change give the negative
> effect
> > of the browser's speed?
> > - Step 3: How should the UI look like?
> > - Step 4: How to do it? Is there a good reference in the existing tests?
> > (Sorry but I'm not very experienced on testing...)
> > How do you think? Any idea and suggestion is welcome!
> > Thanks,
> > Yuta
> > > >
> >
>

--~--~-~--~~~---~--~~
Chromium Developers mailing list: chromium-dev@googlegroups.com 
View archives, change email options, or unsubscribe: 
http://groups.google.com/group/chromium-dev
-~--~~~~--~~--~--~---

[chromium-dev] Re: Tweaking DownloadManager

[Adam Barth]

In general, we prefer message passing to locking.  You might consider
posting a task back to the original thread to notify the download item
about whether the shell execute succeeded.

Adam


On Fri, May 22, 2009 at 6:47 PM, Yuta Kitamura  wrote:
> Hi all,
> I've been researching on DownloadManager for a day in order to to fix the
> issue 3551 . I want to hear some advice from people
> who know Chrome's design well.
> The problem of this issue is like the
> following: DownloadManager::OpenDownloadInShell() posts a task that opens a
> file using the shell (ShellExecute()) to the file thread. Because this task
> does not return any response (i.e. whether open was successful or not) back
> to the UI thread, DownloadItem cannot receive the result of open and the
> browser UI cannot display it either.
> So, I want to add the functionality of receiving the result of opening a
> file to DownloadItem. My current idea of implementation is:
> 1. Let DownloadItem be RefCountedThreadSafe, and rewrite use of
> "DownloadItem*" to "scoped_refptr".
> 2. Add the interface of receiving the result of OpenDownloadInShell to
> DownloadItem. Modify DownloadFileManager::OnOpenDownloadInShell to pass the
> result to it.
> 3. Modify UI (DownloadItemView) to reflect these changes.
> 4. Write tests.
> My question is:
> - How do you think about the whole idea? Am I going the right way?
> - Step 1: Is it okay to do this? Will this change give the negative effect
> of the browser's speed?
> - Step 3: How should the UI look like?
> - Step 4: How to do it? Is there a good reference in the existing tests?
> (Sorry but I'm not very experienced on testing...)
> How do you think? Any idea and suggestion is welcome!
> Thanks,
> Yuta
> >
>

--~--~-~--~~~---~--~~
Chromium Developers mailing list: chromium-dev@googlegroups.com 
View archives, change email options, or unsubscribe: 
http://groups.google.com/group/chromium-dev
-~--~~~~--~~--~--~---

[chromium-dev] Re: Tweaking DownloadManager

[坊野 博典]

Hi Yuta,

I suspect it is simpler to change
DownloadShelfContextMenu::IsItemCommandEnabled() and enable its "open"
and "show in folder" items only when the downloaded file exists as
shown in my mock code . (Even
though this mock code is not tested thoroughly and may cause
side-effects, please feel free to copy and use it.)

Regards,

Hironori Bono
E-mail: hb...@chromium.org

On Sat, May 23, 2009 at 10:47 AM, Yuta Kitamura  wrote:
> Hi all,
> I've been researching on DownloadManager for a day in order to to fix the
> issue 3551 . I want to hear some advice from people
> who know Chrome's design well.
> The problem of this issue is like the
> following: DownloadManager::OpenDownloadInShell() posts a task that opens a
> file using the shell (ShellExecute()) to the file thread. Because this task
> does not return any response (i.e. whether open was successful or not) back
> to the UI thread, DownloadItem cannot receive the result of open and the
> browser UI cannot display it either.
> So, I want to add the functionality of receiving the result of opening a
> file to DownloadItem. My current idea of implementation is:
> 1. Let DownloadItem be RefCountedThreadSafe, and rewrite use of
> "DownloadItem*" to "scoped_refptr".
> 2. Add the interface of receiving the result of OpenDownloadInShell to
> DownloadItem. Modify DownloadFileManager::OnOpenDownloadInShell to pass the
> result to it.
> 3. Modify UI (DownloadItemView) to reflect these changes.
> 4. Write tests.
> My question is:
> - How do you think about the whole idea? Am I going the right way?
> - Step 1: Is it okay to do this? Will this change give the negative effect
> of the browser's speed?
> - Step 3: How should the UI look like?
> - Step 4: How to do it? Is there a good reference in the existing tests?
> (Sorry but I'm not very experienced on testing...)
> How do you think? Any idea and suggestion is welcome!
> Thanks,
> Yuta
> >
>

--~--~-~--~~~---~--~~
Chromium Developers mailing list: chromium-dev@googlegroups.com 
View archives, change email options, or unsubscribe: 
http://groups.google.com/group/chromium-dev
-~--~~~~--~~--~--~---