Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
6060008:
Adding active_downloads_ map. (Closed)
|
Created:
9 years, 12 months ago by ahendrickson Modified:
9 years, 7 months ago CC:
chromium-reviews, rdsmith+dwatch_chromium.org, Paweł Hajdan Jr. Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionAdding active_downloads_ map.
BUG=None
TEST=None
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=71052
Patch Set 1 #
Total comments: 5
Patch Set 2 : Extend lifetime of active_downloads_ until download is complete. #
Total comments: 8
Patch Set 3 : Cleanup based on feedback. #Patch Set 4 : Added comment. #
Total comments: 2
Patch Set 5 : Expanded comment. #
Total comments: 6
Patch Set 6 : Now using ContainsKey(). #Patch Set 7 : Merged with trunk. #Patch Set 8 : Renamed OnDownloadFileCompleted() to RemoveFromActiveList(). #Patch Set 9 : Don't update DownloadItem if it's not in progress. #
Total comments: 2
Patch Set 10 : Moved RemoveFromActiveList() declaration in the header. #
Messages
Total messages: 24 (0 generated)
http://codereview.chromium.org/6060008/diff/1/chrome/browser/download/downloa... File chrome/browser/download/download_item.h (right): http://codereview.chromium.org/6060008/diff/1/chrome/browser/download/downloa... chrome/browser/download/download_item.h:186: void set_full_path(const FilePath& full_path) { full_path_ = full_path; } I think this is a leftover from a previous CL; nuke? http://codereview.chromium.org/6060008/diff/1/chrome/browser/download/downloa... File chrome/browser/download/download_manager.cc (right): http://codereview.chromium.org/6060008/diff/1/chrome/browser/download/downloa... chrome/browser/download/download_manager.cc:444: // |in_progress_|, but we don't want to change the semantics yet. Just FYI: I think we'll be in a position to yank in_progress after my history deflake CL; that simplifies and makes clearer what's happening with in_progress. http://codereview.chromium.org/6060008/diff/1/chrome/browser/download/downloa... chrome/browser/download/download_manager.cc:551: active_downloads_.erase(download_id); It looks like the semantics for active_downloads_ is for the download to enter it when we create the download item, and to leave it whenever we'd normally remove from in_progress_. But in_progress_ is left when we've got all the data and have completed the UI stuff (soon to include the history insertion :-}), and the download isn't really done at that point. For non-dangerous downloads we go through and rename the file for the last time and then possibly auto-open the darn thing. For dangerous downloads we (may) wait until the user validates them and then do the same thing. My current preference is to keep the download in active_downloads_ until we are well and truly done with it--i.e. it's gotten it's final file name and it's been auto-opened if it's going to be. I'm not certain this is the right thing--I'm specifically worried it'll make our life harder when we go to remove in_progress_ around the downloads we show as visible in the DOM. And it doesn't make sense in terms of the map aspect of active_downloads_, as once the IO->FILE pipe is finished, we're never going to get that download_id referenced from a DownloadItem external source again. But if we keep the thing around, the active_downloads_ container can be defined as a container of all the downloads the download system is actively thinking about, which appeals to my simple categorization approach to these containers. If you actively disagree with my preference, I'm ok with it going in as is. But if you don't care, could you shift it over to being cleaned up in something like DownloadManager::DownloadFinished? We can always shift it back pretty easily if we decide that's what's needed.
Publish+Mail Comments
On 2011/01/01 06:27:37, paninvitee wrote:
http://codereview.chromium.org/6060008/diff/1/chrome/browser/download/downloa... File chrome/browser/download/download_item.h (right): http://codereview.chromium.org/6060008/diff/1/chrome/browser/download/downloa... chrome/browser/download/download_item.h:186: void set_full_path(const FilePath& full_path) { full_path_ = full_path; } On 2010/12/29 15:58:11, rdsmith wrote: > I think this is a leftover from a previous CL; nuke? You are correct. Fixed. http://codereview.chromium.org/6060008/diff/1/chrome/browser/download/downloa... File chrome/browser/download/download_manager.cc (right): http://codereview.chromium.org/6060008/diff/1/chrome/browser/download/downloa... chrome/browser/download/download_manager.cc:551: active_downloads_.erase(download_id); On 2010/12/29 15:58:11, rdsmith wrote: > It looks like the semantics for active_downloads_ is for the download to enter > it when we create the download item, and to leave it whenever we'd normally > remove from in_progress_. But in_progress_ is left when we've got all the data > and have completed the UI stuff (soon to include the history insertion :-}), and > the download isn't really done at that point. For non-dangerous downloads we go > through and rename the file for the last time and then possibly auto-open the > darn thing. For dangerous downloads we (may) wait until the user validates them > and then do the same thing. My current preference is to keep the download in > active_downloads_ until we are well and truly done with it--i.e. it's gotten > it's final file name and it's been auto-opened if it's going to be. I'm not > certain this is the right thing--I'm specifically worried it'll make our life > harder when we go to remove in_progress_ around the downloads we show as visible > in the DOM. And it doesn't make sense in terms of the map aspect of > active_downloads_, as once the IO->FILE pipe is finished, we're never going to > get that download_id referenced from a DownloadItem external source again. But > if we keep the thing around, the active_downloads_ container can be defined as a > container of all the downloads the download system is actively thinking about, > which appeals to my simple categorization approach to these containers. > > If you actively disagree with my preference, I'm ok with it going in as is. But > if you don't care, could you shift it over to being cleaned up in something like > DownloadManager::DownloadFinished? We can always shift it back pretty easily if > we decide that's what's needed. Done.
http://codereview.chromium.org/6060008/diff/5001/chrome/browser/download/down... File chrome/browser/download/download_item.cc (right): http://codereview.chromium.org/6060008/diff/5001/chrome/browser/download/down... chrome/browser/download/download_item.cc:203: id())); Oh, what a tangled web we're trying to unweave :-J. Several comments on this single statement, going from most detailed/low-level to most abstract/high-level: * The download item always runs on the UI thread (or if it doesn't we should have big warning signs in neon around the routines where it doesn't) so you can just call this method directly. * I think calling this method from NotifyObserversDownloadFileComplete breaks an implicit pattern--everywhere in the code that I've seen NotifyObservers<blah>, the *only* thing the routine has done is notify the observers. I'd rather not break that implicit expectation (and violate the principle of least astonishment) to save a single code duplication (NotifyObserversDownloadFileComplete is only called from two places). * If I poke around in the calling threads, it doesn't look to me as if NotifyObserversDownloadFileComplete will *really* be called from two places. Specifically, I think that the comment at the end of DownloadItem::Finished() is in error, and that if we have gotten to the end of DownloadItem::Finished(), then the name has been finalized. I could be wrong on this point (thus the tangled web comment above) but if I am, we should figure that out. And presuming the name is finalized by the end of DownloadItem::Finished() I think that's the right place for the call; that's the place at which the download system really lets go of the DownloadItem. http://codereview.chromium.org/6060008/diff/5001/chrome/browser/download/down... File chrome/browser/download/download_manager.cc (right): http://codereview.chromium.org/6060008/diff/5001/chrome/browser/download/down... chrome/browser/download/download_manager.cc:506: if (it != active_downloads_.end()) { This is a semantic change, i.e. that we'll keep updating the download observers and history after we've removed the download from in_progress_. Are you fairly comfortable that either this produces no user-visible change or that the user-visible change it produces is benign? http://codereview.chromium.org/6060008/diff/5001/chrome/browser/download/down... chrome/browser/download/download_manager.cc:1003: active_downloads_.erase(info.download_id); I don't think that this is correct, but I'm not certain what is. The COMPLETE state is reached when the last byte arrives, but there's a lot of download processing that occurs afterwards, so based on what I'd like to see active_downloads_ mean (== download there until no further processing in the downloads system) testing for moving the state out of IN_PROGRESS isn't right. But there isn't a state to test for (currently; one of my CLs is adding one), and this callback could occur arbitrarily long after the download actually compelete/is auto-opened. I think given all that, I'll suggest you leave this code the way it is, but put a TODO comment in that we need to fix it when we either have the final state to test against or the history insertion is done inline. Then whichever of my CLs I commit first I'll fix it in. http://codereview.chromium.org/6060008/diff/5001/chrome/browser/download/down... File chrome/browser/download/download_manager.h (right): http://codereview.chromium.org/6060008/diff/5001/chrome/browser/download/down... chrome/browser/download/download_manager.h:336: DownloadMap active_downloads_; Add a comment above parallel to the ones already there explaining when a download is and is not in the active_downloads_ map.
http://codereview.chromium.org/6060008/diff/5001/chrome/browser/download/down... File chrome/browser/download/download_item.cc (right): http://codereview.chromium.org/6060008/diff/5001/chrome/browser/download/down... chrome/browser/download/download_item.cc:203: id())); On 2011/01/03 22:02:22, rdsmith wrote: > Oh, what a tangled web we're trying to unweave :-J. Several comments on this > single statement, going from most detailed/low-level to most > abstract/high-level: > * The download item always runs on the UI thread (or if it doesn't we should > have big warning signs in neon around the routines where it doesn't) so you can > just call this method directly. Hmm, looking at the |DownloadItem| code, I don't see any indication that it runs only on the UI thread, except for some OS X specific ones in |OpenDownload()| and |ShowDownloadInShell()|. The only other reference to the UI thread that I see is for |Update()|, and that one implies that it can be called from another thread (although I only see it called from the UI thread). I think we at least need a comment in the header indicating that it should only be used on the UI thread. > > * I think calling this method from NotifyObserversDownloadFileComplete breaks an > implicit pattern--everywhere in the code that I've seen NotifyObservers<blah>, > the *only* thing the routine has done is notify the observers. I'd rather not > break that implicit expectation (and violate the principle of least > astonishment) to save a single code duplication > (NotifyObserversDownloadFileComplete is only called from two places). > > * If I poke around in the calling threads, it doesn't look to me as if > NotifyObserversDownloadFileComplete will *really* be called from two places. > Specifically, I think that the comment at the end of DownloadItem::Finished() is > in error, and that if we have gotten to the end of DownloadItem::Finished(), > then the name has been finalized. I could be wrong on this point (thus the > tangled web comment above) but if I am, we should figure that out. And > presuming the name is finalized by the end of DownloadItem::Finished() I think > that's the right place for the call; that's the place at which the download > system really lets go of the DownloadItem. OK, I've made it a straight call, and moved it to where |NotifyObserversDownloadFileCompleted()| gets called. I thought that comment was correct (the download happens in parallel with the renaming). I'm putting the call in there as well until we decide what we really want to do. http://codereview.chromium.org/6060008/diff/5001/chrome/browser/download/down... File chrome/browser/download/download_manager.cc (right): http://codereview.chromium.org/6060008/diff/5001/chrome/browser/download/down... chrome/browser/download/download_manager.cc:506: if (it != active_downloads_.end()) { On 2011/01/03 22:02:22, rdsmith wrote: > This is a semantic change, i.e. that we'll keep updating the download observers > and history after we've removed the download from in_progress_. Are you fairly > comfortable that either this produces no user-visible change or that the > user-visible change it produces is benign? My understanding is that nothing will be visible to the user until the download appears in the |in_progress_| map, so there should not be any user-visible change. http://codereview.chromium.org/6060008/diff/5001/chrome/browser/download/down... chrome/browser/download/download_manager.cc:1003: active_downloads_.erase(info.download_id); On 2011/01/03 22:02:22, rdsmith wrote: > I don't think that this is correct, but I'm not certain what is. The COMPLETE > state is reached when the last byte arrives, but there's a lot of download > processing that occurs afterwards, so based on what I'd like to see > active_downloads_ mean (== download there until no further processing in the > downloads system) testing for moving the state out of IN_PROGRESS isn't right. > But there isn't a state to test for (currently; one of my CLs is adding one), > and this callback could occur arbitrarily long after the download actually > compelete/is auto-opened. > > I think given all that, I'll suggest you leave this code the way it is, but put > a TODO comment in that we need to fix it when we either have the final state to > test against or the history insertion is done inline. Then whichever of my CLs > I commit first I'll fix it in. Added TODO. http://codereview.chromium.org/6060008/diff/5001/chrome/browser/download/down... File chrome/browser/download/download_manager.h (right): http://codereview.chromium.org/6060008/diff/5001/chrome/browser/download/down... chrome/browser/download/download_manager.h:336: DownloadMap active_downloads_; On 2011/01/03 22:02:22, rdsmith wrote: > Add a comment above parallel to the ones already there explaining when a > download is and is not in the active_downloads_ map. Done.
On Tue, Jan 4, 2011 at 11:51 AM, <ahendrickson@chromium.org> wrote: > Hmm, looking at the |DownloadItem| code, I don't see any indication that > it runs only on the UI thread, except for some OS X specific ones in > |OpenDownload()| and |ShowDownloadInShell()|. So the default assumption I have with any Chrome code is that it runs on only one thread, and I expect to see comments if that's not the case (unless it's pretty clearly a low-level utility function that can run on any thread). So the question isn't "Does this only run on one thread?" it's "What thread does this run on?". I think that's pretty clearly the UI thread. > The only other reference to the UI thread that I see is for |Update()|, > and that one implies that it can be called from another thread (although > I only see it called from the UI thread). > > I think we at least need a comment in the header indicating that it > should only be used on the UI thread. SGTM. > OK, I've made it a straight call, and moved it to where > |NotifyObserversDownloadFileCompleted()| gets called. > > I thought that comment was correct (the download happens in parallel > with the renaming). I'm putting the call in there as well until we > decide what we really want to do. That's fine with me, but if you have a potential pathway in min that could result in the call occurring down the non-DownloadItem::Finished() path, I'd love for you to sketch it out to me. >> This is a semantic change, i.e. that we'll keep updating the download observers >> and history after we've removed the download from in_progress_. Are you fairly >> comfortable that either this produces no user-visible change or that the >> user-visible change it produces is benign? > > My understanding is that nothing will be visible to the user until the > download appears in the |in_progress_| map, so there should not be any > user-visible change. See my comment above--I wasn't worried about semantic changes because of when the download entered |in_progress_|, I was concerned about semantic changes because of the difference between when it exits |in_progress_|/|active_downloads_|.
On 2011/01/04 17:59:18, rdsmith wrote: > On Tue, Jan 4, 2011 at 11:51 AM, <mailto:ahendrickson@chromium.org> wrote: > > Hmm, looking at the |DownloadItem| code, I don't see any indication that > > it runs only on the UI thread, except for some OS X specific ones in > > |OpenDownload()| and |ShowDownloadInShell()|. > > So the default assumption I have with any Chrome code is that it runs on only > one thread, and I expect to see comments if that's not the case (unless it's > pretty clearly a low-level utility function that can run on any thread). So the > question isn't "Does this only run on one thread?" it's "What thread does this > run on?". I think that's pretty clearly the UI thread. > > > The only other reference to the UI thread that I see is for |Update()|, > > and that one implies that it can be called from another thread (although > > I only see it called from the UI thread). > > > > I think we at least need a comment in the header indicating that it > > should only be used on the UI thread. > > SGTM. Added the comment to the |DownloadItem| declaration. > > > OK, I've made it a straight call, and moved it to where > > |NotifyObserversDownloadFileCompleted()| gets called. > > > > I thought that comment was correct (the download happens in parallel > > with the renaming). I'm putting the call in there as well until we > > decide what we really want to do. > > That's fine with me, but if you have a potential pathway in min that could > result in the call occurring down the non-DownloadItem::Finished() path, I'd > love for you to sketch it out to me. > > >> This is a semantic change, i.e. that we'll keep updating the download > observers > >> and history after we've removed the download from in_progress_. Are you > fairly > >> comfortable that either this produces no user-visible change or that the > >> user-visible change it produces is benign? > > > > My understanding is that nothing will be visible to the user until the > > download appears in the |in_progress_| map, so there should not be any > > user-visible change. > > See my comment above--I wasn't worried about semantic changes because of when > the download entered |in_progress_|, I was concerned about semantic changes > because of the difference between when it exits > |in_progress_|/|active_downloads_|. Ah, missed that. I don't believe this function should ever be called after the |DownloadItem| exits the |IN_PROGRESS| state. If it does, there will be a |NOTREACHED()| hit in |DownloadItem::Update()|. The only time |in_progress_.erase()| is called without a corresponding |active_downloads_.erase()| call is in |OnAllDataSaved()|. Later CLs will erase it in |OnCreateDownloadComplete()| if we're in the final state -- this may be a problem depending on how we decide to implement it.
Now ready for wider review.
With below nit, LGTM. http://codereview.chromium.org/6060008/diff/19001/chrome/browser/download/dow... File chrome/browser/download/download_manager.cc (right): http://codereview.chromium.org/6060008/diff/19001/chrome/browser/download/dow... chrome/browser/download/download_manager.cc:1004: // check for. I'd like a slightly more descriptive comment. Basically, I'd aim for a comment s.t. if we all got hit by a bus tomorrow, a new person taking over the code could, with some amount of work, figure out what the comment was saying needed to be done. Here something like "We don't actually know whether or not we can remove from the active_downloads_ map, as there is no state in DownloadItem::DownloadState to indicate that the downloads system is done with an item. Fix this when ..." would work.
http://codereview.chromium.org/6060008/diff/19001/chrome/browser/download/dow... File chrome/browser/download/download_manager.cc (right): http://codereview.chromium.org/6060008/diff/19001/chrome/browser/download/dow... chrome/browser/download/download_manager.cc:1004: // check for. On 2011/01/04 21:05:35, rdsmith wrote: > I'd like a slightly more descriptive comment. Basically, I'd aim for a comment > s.t. if we all got hit by a bus tomorrow, a new person taking over the code > could, with some amount of work, figure out what the comment was saying needed > to be done. Here something like "We don't actually know whether or not we can > remove from the active_downloads_ map, as there is no state in > DownloadItem::DownloadState to indicate that the downloads system is done with > an item. Fix this when ..." would work. Done.
Drive-by with some download comments. It seems to be a refactoring-in-progress, so I'm fine with difference between in_progress_ and active_downloads_ not being so obvious, but the issue with observers is a bit worrying. http://codereview.chromium.org/6060008/diff/25001/chrome/browser/download/dow... File chrome/browser/download/download_item.cc (right): http://codereview.chromium.org/6060008/diff/25001/chrome/browser/download/dow... chrome/browser/download/download_item.cc:334: NotifyObserversDownloadFileCompleted(); It is really weird that we have to notify *both* "regular" observers and the download_manager_. Can we somehow unify those two? The current solution is very error-prone, e.g. it's easy to forget to notify download_manager_. http://codereview.chromium.org/6060008/diff/25001/chrome/browser/download/dow... File chrome/browser/download/download_manager.cc (right): http://codereview.chromium.org/6060008/diff/25001/chrome/browser/download/dow... chrome/browser/download/download_manager.cc:449: DCHECK(downloads_.find(download) != downloads_.end()); nit: If possible, use ContainsKey. http://codereview.chromium.org/6060008/diff/25001/chrome/browser/download/dow... File chrome/browser/download/download_manager.h (right): http://codereview.chromium.org/6060008/diff/25001/chrome/browser/download/dow... chrome/browser/download/download_manager.h:315: // processed. The key is the ID assigned by the ResourceDispatcherHost, nit: "being processed" is extremely vague. Could you find a more precise description? How is it different from in_progress_, and why do we need/want to have yet another map here?
On 2011/01/05 08:11:16, Paweł Hajdan Jr. wrote: > http://codereview.chromium.org/6060008/diff/25001/chrome/browser/download/dow... > chrome/browser/download/download_manager.h:315: // processed. The key is the ID > assigned by the ResourceDispatcherHost, > nit: "being processed" is extremely vague. Could you find a more precise > description? How is it different from in_progress_, and why do we need/want to > have yet another map here? Pawel, just a quick note, as I'm responsible for the duplication, not Andy. The basic meaning of active_downloads_ and in_progress_ is identical, but there's enough subtlety around how in_progress_ is currently used that I didn't want to just replace them (or expand in_progress_) in one refactor. I intend to in_progress_ to go away, hopefully within a CL or two after the history inlining occurs.
On Wed, Jan 5, 2011 at 22:56, <rdsmith@chromium.org> wrote: > Pawel, just a quick note, as I'm responsible for the duplication, not Andy. > The > basic meaning of active_downloads_ and in_progress_ is identical, but > there's > enough subtlety around how in_progress_ is currently used that I didn't > want to > just replace them (or expand in_progress_) in one refactor. Sure, I don't mind making the change in a safe manner. My main comment is about the observers; a temporary duplication is fine.
http://codereview.chromium.org/6060008/diff/25001/chrome/browser/download/dow... File chrome/browser/download/download_manager.cc (right): http://codereview.chromium.org/6060008/diff/25001/chrome/browser/download/dow... chrome/browser/download/download_manager.cc:449: DCHECK(downloads_.find(download) != downloads_.end()); On 2011/01/05 08:11:16, Paweł Hajdan Jr. wrote: > nit: If possible, use ContainsKey. Thanks, I was looking for that! http://codereview.chromium.org/6060008/diff/25001/chrome/browser/download/dow... File chrome/browser/download/download_manager.h (right): http://codereview.chromium.org/6060008/diff/25001/chrome/browser/download/dow... chrome/browser/download/download_manager.h:315: // processed. The key is the ID assigned by the ResourceDispatcherHost, On 2011/01/05 08:11:16, Paweł Hajdan Jr. wrote: > nit: "being processed" is extremely vague. Could you find a more precise > description? How is it different from in_progress_, and why do we need/want to > have yet another map here? Is "active" any better? It will eventually replace |in_progress_|, and has a longer lifetime.
Note that the comment about observers in download_item.cc is still not addressed. :-/ http://codereview.chromium.org/6060008/diff/25001/chrome/browser/download/dow... File chrome/browser/download/download_manager.h (right): http://codereview.chromium.org/6060008/diff/25001/chrome/browser/download/dow... chrome/browser/download/download_manager.h:315: // processed. The key is the ID assigned by the ResourceDispatcherHost, On 2011/01/06 16:48:52, ahendrickson wrote: > On 2011/01/05 08:11:16, Paweł Hajdan Jr. wrote: > > nit: "being processed" is extremely vague. Could you find a more precise > > description? How is it different from in_progress_, and why do we need/want to > > have yet another map here? > > Is "active" any better? > > It will eventually replace |in_progress_|, and has a longer lifetime. Okay, ignore it for now. We can do it the right way in the CL which removes in_progress_, then it should be more obvious what to write there.
On 2011/01/05 08:11:16, Paweł Hajdan Jr. wrote: > http://codereview.chromium.org/6060008/diff/25001/chrome/browser/download/dow... > chrome/browser/download/download_item.cc:334: > NotifyObserversDownloadFileCompleted(); > It is really weird that we have to notify *both* "regular" observers and the > download_manager_. Can we somehow unify those two? The current solution is very > error-prone, e.g. it's easy to forget to notify download_manager_. Pawel: I think the key issue here (if you'll forgive me the excursion into philosophy) is whether the call into the DownloadManager is a notification or not. I think if it is a notification, the obvious thing to do is to make DownloadManager an observer for all of its DownloadItems and eliminate the second call that way. If it's not, we should probably change the name of the second call to reflect that. The reason I'm not sure if it is a notification or not is that currently, the DownloadItem and DownloadManager code are like Siamese twins. There's some code involving life-cycle management and download processing in each, and they call into each other. Long term, I want to change that and make DownloadItem as responsible for its own destiny as possible. But in the short term, I'm reluctant to hide those type of interactions; if there's some key life-cycle/management call from one class into the other, I'd like it directly visible in the code. Would you be cool with just changing the name of this routine to "RemoveFromActive" to be more explicit about what aspect of the lifecycle management is going on? I'd be reluctantly ok with just converting DownloadManager over to being a DownloadItem::Observer, but I'd rather do that when we get to the point in the refactor where we're severing the rest of the twin linkages :-}.
On 2011/01/04 19:00:50, ahendrickson wrote: > On 2011/01/04 17:59:18, rdsmith wrote: > > See my comment above--I wasn't worried about semantic changes because of when > > the download entered |in_progress_|, I was concerned about semantic changes > > because of the difference between when it exits > > |in_progress_|/|active_downloads_|. > > Ah, missed that. > > I don't believe this function should ever be called after the |DownloadItem| > exits the |IN_PROGRESS| state. If it does, there will be a |NOTREACHED()| hit > in |DownloadItem::Update()|. > > The only time |in_progress_.erase()| is called without a corresponding > |active_downloads_.erase()| call is in |OnAllDataSaved()|. > > Later CLs will erase it in |OnCreateDownloadComplete()| if we're in the final > state -- this may be a problem depending on how we decide to implement it. Andy: Based on issue 68881, I'd like to make my LGTM conditional on you moving this back to searching in_progress_ rather than active_downloads_. The race/window in 68881 is still there and still needs to be fixed, but I believe shifting to using active_downloads_ would make the window bigger (because we would no longer be gating whether or not DownloadItem::Update() was called based on the history callback completion). I'm not 100% sure I've analyzed this race correctly (actually, I'm at about 60% :-} :-J) but 60% is enough to make me ask you not to change the semantics in a call that might cause a wider window. In debug mode only, admittedly :-}.
See patch 9 title $-)
On Thu, Jan 6, 2011 at 22:08, <rdsmith@chromium.org> wrote: > Would you be cool with just changing the name of this routine to > "RemoveFromActive" to be more explicit about what aspect of the lifecycle > management is going on? I'd be reluctantly ok with just converting > DownloadManager over to being a DownloadItem::Observer, but I'd rather do > that > when we get to the point in the refactor where we're severing the rest of > the > twin linkages :-}. OK, sorry for delayed response.
Still LGTM :-}. On 2011/01/10 08:35:04, Paweł Hajdan Jr. wrote: > On Thu, Jan 6, 2011 at 22:08, <mailto:rdsmith@chromium.org> wrote: > > > Would you be cool with just changing the name of this routine to > > "RemoveFromActive" to be more explicit about what aspect of the lifecycle > > management is going on? I'd be reluctantly ok with just converting > > DownloadManager over to being a DownloadItem::Observer, but I'd rather do > > that > > when we get to the point in the refactor where we're severing the rest of > > the > > twin linkages :-}. > > > OK, sorry for delayed response.
Ping.
Sorry if you were waiting for me, with all the reviewers it gets confusing. I didn't totally internalize the race condition you're worried about, but I assume the other reviewers have checked all that. LGTM with these style nits. http://codereview.chromium.org/6060008/diff/42001/chrome/browser/download/dow... File chrome/browser/download/download_manager.cc (right): http://codereview.chromium.org/6060008/diff/42001/chrome/browser/download/dow... chrome/browser/download/download_manager.cc:577: I'd say just remove this blank line, it doesn't help when there are only two lines of code. http://codereview.chromium.org/6060008/diff/42001/chrome/browser/download/dow... File chrome/browser/download/download_manager.h (right): http://codereview.chromium.org/6060008/diff/42001/chrome/browser/download/dow... chrome/browser/download/download_manager.h:122: void RemoveFromActiveList(int32 download_id); It doesn't seem like this goes with the other functions in this block as a "notification." Probably there should be a blank line and a special comment just for this function (you can reference the active_downloads_ comment below).
LGTM |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
