Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
6312027:
Add files saved using 'Save page as' to the download history.... (Closed)
|
Created:
9 years, 10 months ago by magnus Modified:
9 years, 7 months ago CC:
chromium-reviews, rdsmith+dwatch_chromium.org, Paweł Hajdan Jr. Base URL:
http://src.chromium.org/svn/trunk/src/ Visibility:
Public. |
DescriptionAdd files saved using 'Save page as' to the download history.
Contributed by fuzzac@gmail.com.
BUG=4823
TEST=Open any web page. Save the page using 'Save page as'. Make sure the file
is listed in the downloads page.
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=76780
Patch Set 1 #
Total comments: 2
Patch Set 2 : '' #Patch Set 3 : '' #
Total comments: 20
Patch Set 4 : '' #
Total comments: 3
Patch Set 5 : '' #
Total comments: 2
Patch Set 6 : '' #Patch Set 7 : '' #
Messages
Total messages: 21 (0 generated)
Thank you for the patch. Could you also add a test? http://codereview.chromium.org/6312027/diff/1/chrome/browser/download/downloa... File chrome/browser/download/download_manager.cc (right): http://codereview.chromium.org/6312027/diff/1/chrome/browser/download/downloa... chrome/browser/download/download_manager.cc:839: download_item->set_db_handle(db_handle); nit: Could you put it inside the "if"? http://codereview.chromium.org/6312027/diff/1/chrome/browser/download/downloa... chrome/browser/download/download_manager.cc:841: history_downloads_[download_item->db_handle()] = download_item; nit: Should be sufficient to just use db_handle here.
Please also test carefully to make sure that the "Show in Folder" and "open" functionality from the downloads page works for downloads that show up there in that fashion, specifically if you have "saved complete web page" (with the with the subdir for extra files). The unique thing about DownloadItems that are created through the save page functionality is that they represent multiple files on disk, and I'm concerned that that attribute will mess up the functionality normally tied to a download item. If those tests pass, LGTM. On 2011/01/31 08:32:19, Paweł Hajdan Jr. wrote: > Thank you for the patch. Could you also add a test? > > http://codereview.chromium.org/6312027/diff/1/chrome/browser/download/downloa... > File chrome/browser/download/download_manager.cc (right): > > http://codereview.chromium.org/6312027/diff/1/chrome/browser/download/downloa... > chrome/browser/download/download_manager.cc:839: > download_item->set_db_handle(db_handle); > nit: Could you put it inside the "if"? > > http://codereview.chromium.org/6312027/diff/1/chrome/browser/download/downloa... > chrome/browser/download/download_manager.cc:841: > history_downloads_[download_item->db_handle()] = download_item; > nit: Should be sufficient to just use db_handle here.
Sorry, one other test request: Test the below items quitting and restarting the browser in between the save page as and the tests. You need to make sure that all the information needed for that functionality is saved in the history database. It probably is (my fear is more that the DownloadItem<-> file system correspondence is messed up than that we'll miss needed fields) but the test is still worth doing. On 2011/01/31 14:18:12, rdsmith wrote: > Please also test carefully to make sure that the "Show in Folder" and "open" > functionality from the downloads page works for downloads that show up there in > that fashion, specifically if you have "saved complete web page" (with the with > the subdir for extra files). The unique thing about DownloadItems that are > created through the save page functionality is that they represent multiple > files on disk, and I'm concerned that that attribute will mess up the > functionality normally tied to a download item. > > If those tests pass, LGTM. > > > On 2011/01/31 08:32:19, Paweł Hajdan Jr. wrote: > > Thank you for the patch. Could you also add a test? > > > > > http://codereview.chromium.org/6312027/diff/1/chrome/browser/download/downloa... > > File chrome/browser/download/download_manager.cc (right): > > > > > http://codereview.chromium.org/6312027/diff/1/chrome/browser/download/downloa... > > chrome/browser/download/download_manager.cc:839: > > download_item->set_db_handle(db_handle); > > nit: Could you put it inside the "if"? > > > > > http://codereview.chromium.org/6312027/diff/1/chrome/browser/download/downloa... > > chrome/browser/download/download_manager.cc:841: > > history_downloads_[download_item->db_handle()] = download_item; > > nit: Should be sufficient to just use db_handle here.
Great feedback. It does not save correctly to the history db (and the fact that I had to use a fake db handle should've alerted me to this). I'll look into a slightly more involved solution in SavePackage. On 2011/01/31 14:20:05, rdsmith wrote: > Sorry, one other test request: Test the below items quitting and restarting the > browser in between the save page as and the tests. You need to make sure that > all the information needed for that functionality is saved in the history > database. It probably is (my fear is more that the DownloadItem<-> file system > correspondence is messed up than that we'll miss needed fields) but the test is > still worth doing. > > On 2011/01/31 14:18:12, rdsmith wrote: > > Please also test carefully to make sure that the "Show in Folder" and "open" > > functionality from the downloads page works for downloads that show up there > in > > that fashion, specifically if you have "saved complete web page" (with the > with > > the subdir for extra files). The unique thing about DownloadItems that are > > created through the save page functionality is that they represent multiple > > files on disk, and I'm concerned that that attribute will mess up the > > functionality normally tied to a download item. > > > > If those tests pass, LGTM. > > > > > > On 2011/01/31 08:32:19, Paweł Hajdan Jr. wrote: > > > Thank you for the patch. Could you also add a test? > > > > > > > > > http://codereview.chromium.org/6312027/diff/1/chrome/browser/download/downloa... > > > File chrome/browser/download/download_manager.cc (right): > > > > > > > > > http://codereview.chromium.org/6312027/diff/1/chrome/browser/download/downloa... > > > chrome/browser/download/download_manager.cc:839: > > > download_item->set_db_handle(db_handle); > > > nit: Could you put it inside the "if"? > > > > > > > > > http://codereview.chromium.org/6312027/diff/1/chrome/browser/download/downloa... > > > chrome/browser/download/download_manager.cc:841: > > > history_downloads_[download_item->db_handle()] = download_item; > > > nit: Should be sufficient to just use db_handle here.
I uploaded another patch that correctly saves the information to the history database. "Show in Folder" and "open" also works as expected. I've moved some code related to creating and managing DownloadItems from save_package.cc to download_manager.cc. I also had to switch to the IO thread and back in order to allocate a unique download_id. On 2011/01/31 14:20:05, rdsmith wrote: > Sorry, one other test request: Test the below items quitting and restarting the > browser in between the save page as and the tests. You need to make sure that > all the information needed for that functionality is saved in the history > database. It probably is (my fear is more that the DownloadItem<-> file system > correspondence is messed up than that we'll miss needed fields) but the test is > still worth doing. > > On 2011/01/31 14:18:12, rdsmith wrote: > > Please also test carefully to make sure that the "Show in Folder" and "open" > > functionality from the downloads page works for downloads that show up there > in > > that fashion, specifically if you have "saved complete web page" (with the > with > > the subdir for extra files). The unique thing about DownloadItems that are > > created through the save page functionality is that they represent multiple > > files on disk, and I'm concerned that that attribute will mess up the > > functionality normally tied to a download item. > > > > If those tests pass, LGTM. > > > > > > On 2011/01/31 08:32:19, Paweł Hajdan Jr. wrote: > > > Thank you for the patch. Could you also add a test? > > > > > > > > > http://codereview.chromium.org/6312027/diff/1/chrome/browser/download/downloa... > > > File chrome/browser/download/download_manager.cc (right): > > > > > > > > > http://codereview.chromium.org/6312027/diff/1/chrome/browser/download/downloa... > > > chrome/browser/download/download_manager.cc:839: > > > download_item->set_db_handle(db_handle); > > > nit: Could you put it inside the "if"? > > > > > > > > > http://codereview.chromium.org/6312027/diff/1/chrome/browser/download/downloa... > > > chrome/browser/download/download_manager.cc:841: > > > history_downloads_[download_item->db_handle()] = download_item; > > > nit: Should be sufficient to just use db_handle here.
magnus: I apologize for asking this, but how important is it to you to get this
patch in/bug fixed in the short term? The problem is that the SavePackage and
Downloads systems are separate systems (I think SavePackage was copied from
downloads and modified :-{). In an ideal world, they'd be completely merged and
bugs like this would softly and silently vanish away. But until someone has the
bandwidth to do a good analysis of what they do differently and how the
downloads system should be expanded to encompass SavePackage, I'm reluctant to
see them get more intertwined--that kind of slow merging never ends well, and
could really end remarkably poorly. Something at the level of the original
quick hack you did would have been ok by me, but I think you're right that
putting the download item into the downloads page requires getting it cleanly
into the downloads history, which requires something like what you've done in
this CL. And I'd rather not go in that direction without a strong need.
If you felt the urge to take on the larger project of merging downloads and save
package, I'd be ecstatic, but I might fear for your sanity--it wouldn't be an
easy project. We're also in the middle of doing some serious refactoring within
the downloads system, so while we could start on design work for the merge, we'd
probably have to hold off on committing code for O(1 month). So my guess is
that that doesn't make sense.
I apologize for not getting to this point before now; I could have saved you
some time. But I'm left with my original question: How important is it to get
this functionality into Chrome?
On 2011/02/04 18:48:48, magnus wrote:
> I uploaded another patch that correctly saves the information to the history
> database. "Show in Folder" and "open" also works as expected. I've moved some
> code related to creating and managing DownloadItems from save_package.cc to
> download_manager.cc. I also had to switch to the IO thread and back in order
to
> allocate a unique download_id.
>
> On 2011/01/31 14:20:05, rdsmith wrote:
> > Sorry, one other test request: Test the below items quitting and restarting
> the
> > browser in between the save page as and the tests. You need to make sure
that
> > all the information needed for that functionality is saved in the history
> > database. It probably is (my fear is more that the DownloadItem<-> file
> system
> > correspondence is messed up than that we'll miss needed fields) but the test
> is
> > still worth doing.
> >
> > On 2011/01/31 14:18:12, rdsmith wrote:
> > > Please also test carefully to make sure that the "Show in Folder" and
"open"
> > > functionality from the downloads page works for downloads that show up
there
> > in
> > > that fashion, specifically if you have "saved complete web page" (with the
> > with
> > > the subdir for extra files). The unique thing about DownloadItems that
are
> > > created through the save page functionality is that they represent
multiple
> > > files on disk, and I'm concerned that that attribute will mess up the
> > > functionality normally tied to a download item.
> > >
> > > If those tests pass, LGTM.
> > >
> > >
> > > On 2011/01/31 08:32:19, Paweł Hajdan Jr. wrote:
> > > > Thank you for the patch. Could you also add a test?
> > > >
> > > >
> > >
> >
>
http://codereview.chromium.org/6312027/diff/1/chrome/browser/download/downloa...
> > > > File chrome/browser/download/download_manager.cc (right):
> > > >
> > > >
> > >
> >
>
http://codereview.chromium.org/6312027/diff/1/chrome/browser/download/downloa...
> > > > chrome/browser/download/download_manager.cc:839:
> > > > download_item->set_db_handle(db_handle);
> > > > nit: Could you put it inside the "if"?
> > > >
> > > >
> > >
> >
>
http://codereview.chromium.org/6312027/diff/1/chrome/browser/download/downloa...
> > > > chrome/browser/download/download_manager.cc:841:
> > > > history_downloads_[download_item->db_handle()] = download_item;
> > > > nit: Should be sufficient to just use db_handle here.
I completely see your point, SavePackage did seem a bit bolted on. This is one of those polish bugs - it seemed worth fixing but it's probably ok if it doesn't get fixed short term. That said: I could easily move the code back out to SavePackage to avoid polluting the download code and the messy bits would be kept all in one place. On Fri, Feb 4, 2011 at 11:38 AM, <rdsmith@chromium.org> wrote: > magnus: I apologize for asking this, but how important is it to you to get > this > patch in/bug fixed in the short term? The problem is that the SavePackage > and > Downloads systems are separate systems (I think SavePackage was copied from > downloads and modified :-{). In an ideal world, they'd be completely merged > and > bugs like this would softly and silently vanish away. But until someone has > the > bandwidth to do a good analysis of what they do differently and how the > downloads system should be expanded to encompass SavePackage, I'm reluctant > to > see them get more intertwined--that kind of slow merging never ends well, > and > could really end remarkably poorly. Something at the level of the original > quick hack you did would have been ok by me, but I think you're right that > putting the download item into the downloads page requires getting it > cleanly > into the downloads history, which requires something like what you've done > in > this CL. And I'd rather not go in that direction without a strong need. > > If you felt the urge to take on the larger project of merging downloads and > save > package, I'd be ecstatic, but I might fear for your sanity--it wouldn't be > an > easy project. We're also in the middle of doing some serious refactoring > within > the downloads system, so while we could start on design work for the merge, > we'd > probably have to hold off on committing code for O(1 month). So my guess is > that that doesn't make sense. > > I apologize for not getting to this point before now; I could have saved you > some time. But I'm left with my original question: How important is it to > get > this functionality into Chrome? > > > On 2011/02/04 18:48:48, magnus wrote: >> >> I uploaded another patch that correctly saves the information to the >> history >> database. "Show in Folder" and "open" also works as expected. I've moved >> some >> code related to creating and managing DownloadItems from save_package.cc >> to >> download_manager.cc. I also had to switch to the IO thread and back in >> order > > to >> >> allocate a unique download_id. > >> On 2011/01/31 14:20:05, rdsmith wrote: >> > Sorry, one other test request: Test the below items quitting and >> > restarting >> the >> > browser in between the save page as and the tests. You need to make >> > sure > > that >> >> > all the information needed for that functionality is saved in the >> > history >> > database. It probably is (my fear is more that the DownloadItem<-> file >> system >> > correspondence is messed up than that we'll miss needed fields) but the >> > test >> is >> > still worth doing. >> > >> > On 2011/01/31 14:18:12, rdsmith wrote: >> > > Please also test carefully to make sure that the "Show in Folder" and > > "open" >> >> > > functionality from the downloads page works for downloads that show up > > there >> >> > in >> > > that fashion, specifically if you have "saved complete web page" (with >> > > the >> > with >> > > the subdir for extra files). The unique thing about DownloadItems >> > > that > > are >> >> > > created through the save page functionality is that they represent > > multiple >> >> > > files on disk, and I'm concerned that that attribute will mess up the >> > > functionality normally tied to a download item. >> > > >> > > If those tests pass, LGTM. >> > > >> > > >> > > On 2011/01/31 08:32:19, Paweł Hajdan Jr. wrote: >> > > > Thank you for the patch. Could you also add a test? >> > > > >> > > > >> > > >> > > > http://codereview.chromium.org/6312027/diff/1/chrome/browser/download/downloa... >> >> > > > File chrome/browser/download/download_manager.cc (right): >> > > > >> > > > >> > > >> > > > http://codereview.chromium.org/6312027/diff/1/chrome/browser/download/downloa... >> >> > > > chrome/browser/download/download_manager.cc:839: >> > > > download_item->set_db_handle(db_handle); >> > > > nit: Could you put it inside the "if"? >> > > > >> > > > >> > > >> > > > http://codereview.chromium.org/6312027/diff/1/chrome/browser/download/downloa... >> >> > > > chrome/browser/download/download_manager.cc:841: >> > > > history_downloads_[download_item->db_handle()] = download_item; >> > > > nit: Should be sufficient to just use db_handle here. > > > > http://codereview.chromium.org/6312027/ >
On 2011/02/04 20:47:28, magnus wrote:
> I completely see your point, SavePackage did seem a bit bolted on.
>
> This is one of those polish bugs - it seemed worth fixing but it's
> probably ok if it doesn't get fixed short term.
Cool; thank you.
> That said: I could easily move the code back out to SavePackage to
> avoid polluting the download code and the messy bits would be kept all
> in one place.
Hmmm. If you can see a way to do that with minimal changes to the
DownloadManager class, I might be comfortable with it. I'm certainly happy to
take a look at that change if you want to do it. Unfortunately, I don't have a
good enough idea of what that would look like to say whether I'd be happier with
it or not :-} :-J.
>
> On Fri, Feb 4, 2011 at 11:38 AM, <mailto:rdsmith@chromium.org> wrote:
> > magnus: I apologize for asking this, but how important is it to you to get
> > this
> > patch in/bug fixed in the short term? The problem is that the
SavePackage
> > and
> > Downloads systems are separate systems (I think SavePackage was copied from
> > downloads and modified :-{). In an ideal world, they'd be completely
merged
> > and
> > bugs like this would softly and silently vanish away. But until
someone has
> > the
> > bandwidth to do a good analysis of what they do differently and how the
> > downloads system should be expanded to encompass SavePackage, I'm reluctant
> > to
> > see them get more intertwined--that kind of slow merging never ends well,
> > and
> > could really end remarkably poorly. Something at the level of the
original
> > quick hack you did would have been ok by me, but I think you're right that
> > putting the download item into the downloads page requires getting it
> > cleanly
> > into the downloads history, which requires something like what you've done
> > in
> > this CL. And I'd rather not go in that direction without a strong
need.
> >
> > If you felt the urge to take on the larger project of merging downloads and
> > save
> > package, I'd be ecstatic, but I might fear for your sanity--it wouldn't be
> > an
> > easy project. We're also in the middle of doing some serious
refactoring
> > within
> > the downloads system, so while we could start on design work for the merge,
> > we'd
> > probably have to hold off on committing code for O(1 month). So my
guess is
> > that that doesn't make sense.
> >
> > I apologize for not getting to this point before now; I could have saved you
> > some time. But I'm left with my original question: How important is it
to
> > get
> > this functionality into Chrome?
> >
> >
> > On 2011/02/04 18:48:48, magnus wrote:
> >>
> >> I uploaded another patch that correctly saves the information to the
> >> history
> >> database. "Show in Folder" and "open" also works as expected. I've moved
> >> some
> >> code related to creating and managing DownloadItems from save_package.cc
> >> to
> >> download_manager.cc. I also had to switch to the IO thread and back in
> >> order
> >
> > to
> >>
> >> allocate a unique download_id.
> >
> >> On 2011/01/31 14:20:05, rdsmith wrote:
> >> > Sorry, one other test request: Test the below items quitting and
> >> > restarting
> >> the
> >> > browser in between the save page as and the tests. You need to make
> >> > sure
> >
> > that
> >>
> >> > all the information needed for that functionality is saved in the
> >> > history
> >> > database. It probably is (my fear is more that the DownloadItem<->
file
> >> system
> >> > correspondence is messed up than that we'll miss needed fields) but the
> >> > test
> >> is
> >> > still worth doing.
> >> >
> >> > On 2011/01/31 14:18:12, rdsmith wrote:
> >> > > Please also test carefully to make sure that the "Show in Folder" and
> >
> > "open"
> >>
> >> > > functionality from the downloads page works for downloads that show up
> >
> > there
> >>
> >> > in
> >> > > that fashion, specifically if you have "saved complete web page" (with
> >> > > the
> >> > with
> >> > > the subdir for extra files). The unique thing about DownloadItems
> >> > > that
> >
> > are
> >>
> >> > > created through the save page functionality is that they represent
> >
> > multiple
> >>
> >> > > files on disk, and I'm concerned that that attribute will mess up the
> >> > > functionality normally tied to a download item.
> >> > >
> >> > > If those tests pass, LGTM.
> >> > >
> >> > >
> >> > > On 2011/01/31 08:32:19, Paweł Hajdan Jr. wrote:
> >> > > > Thank you for the patch. Could you also add a test?
> >> > > >
> >> > > >
> >> > >
> >> >
> >
> >
>
http://codereview.chromium.org/6312027/diff/1/chrome/browser/download/downloa...
> >>
> >> > > > File chrome/browser/download/download_manager.cc (right):
> >> > > >
> >> > > >
> >> > >
> >> >
> >
> >
>
http://codereview.chromium.org/6312027/diff/1/chrome/browser/download/downloa...
> >>
> >> > > > chrome/browser/download/download_manager.cc:839:
> >> > > > download_item->set_db_handle(db_handle);
> >> > > > nit: Could you put it inside the "if"?
> >> > > >
> >> > > >
> >> > >
> >> >
> >
> >
>
http://codereview.chromium.org/6312027/diff/1/chrome/browser/download/downloa...
> >>
> >> > > > chrome/browser/download/download_manager.cc:841:
> >> > > > history_downloads_[download_item->db_handle()] = download_item;
> >> > > > nit: Should be sufficient to just use db_handle here.
> >
> >
> >
> > http://codereview.chromium.org/6312027/
> >
> Hmmm. If you can see a way to do that with minimal changes to the > DownloadManager class, I might be comfortable with it. I'm certainly happy to > take a look at that change if you want to do it. Unfortunately, I don't have a > good enough idea of what that would look like to say whether I'd be happier with > it or not :-} :-J. I've uploaded a new patch with minimal changes to DownloadManager; no new code but I had to factor out a few lines into a function. The rest of it is contained in SavePackage.
I think I'm ok with this approach, though I'm curious about Pawel's opinion. Thank you for persevering. Obviously, do all the obvious tests (I'm think about making sure the download page behavior is good in this case for canceled and non-canceled versions). http://codereview.chromium.org/6312027/diff/10001/chrome/browser/download/sav... File chrome/browser/download/save_package.cc (right): http://codereview.chromium.org/6312027/diff/10001/chrome/browser/download/sav... chrome/browser/download/save_package.cc:697: download_manager->download_history()->RemoveEntry(download_); I'm ok with the difference in behavior, but this is a difference in behavior between downloads and save package activities. downloads are left in the history when cancelled (unless they're dangerous). See for instance DownloadManager::DownloadCancelled. http://codereview.chromium.org/6312027/diff/10001/chrome/browser/download/sav... chrome/browser/download/save_package.cc:750: DownloadItem* item = download_; What's the value of the local variable rather than using download_? http://codereview.chromium.org/6312027/diff/10001/chrome/browser/download/sav... File chrome/browser/download/save_package.h (right): http://codereview.chromium.org/6312027/diff/10001/chrome/browser/download/sav... chrome/browser/download/save_package.h:167: // Called when Save Page As entry commited to the history system nit: "is commited" http://codereview.chromium.org/6312027/diff/10001/chrome/browser/download/sav... chrome/browser/download/save_package.h:169: // Called when a Save Page As download is started. nit: I'd put a blank line above this comment.
I'm generally fine with the approach. There are possible issues with the behavior changes, but it seems Randy commented on that. Finally, please add an automated test. Without it it's extremely likely to break. http://codereview.chromium.org/6312027/diff/10001/chrome/browser/download/dow... File chrome/browser/download/download_manager.cc (right): http://codereview.chromium.org/6312027/diff/10001/chrome/browser/download/dow... chrome/browser/download/download_manager.cc:845: // Insert into our full map. nit: This comment doesn't add value, could you remove it? http://codereview.chromium.org/6312027/diff/10001/chrome/browser/download/sav... File chrome/browser/download/save_package.cc (right): http://codereview.chromium.org/6312027/diff/10001/chrome/browser/download/sav... chrome/browser/download/save_package.cc:397: DownloadItem* item = new DownloadItem(download_manager, path, url, is_otr); nit: If it's going to be stored on |item_| anyway, why have the temporary |item|? http://codereview.chromium.org/6312027/diff/10001/chrome/browser/download/sav... chrome/browser/download/save_package.cc:411: // Add entry to the history service nit: Dot at the end. Also, please add a blank line above each comment, including above. http://codereview.chromium.org/6312027/diff/10001/chrome/browser/download/sav... chrome/browser/download/save_package.cc:756: // Update the download history nit: Dot at the end. http://codereview.chromium.org/6312027/diff/10001/chrome/browser/download/sav... File chrome/browser/download/save_package.h (right): http://codereview.chromium.org/6312027/diff/10001/chrome/browser/download/sav... chrome/browser/download/save_package.h:19: #include "chrome/browser/history/download_create_info.h" nit: Is this include really needed? It should be possible to forward-declare DownloadCreateInfo. http://codereview.chromium.org/6312027/diff/10001/chrome/browser/download/sav... chrome/browser/download/save_package.h:168: void OnDownloadEntryAdded(DownloadCreateInfo info, int64 db_handle); nit: |info| should probably be passed as const DownloadCreateInfo& info.
Thanks for all the feedback. The behavior when canceling seems consistent with the other downloads (links back to original page). I've added checks in save_page_browsertest that verify that the download history got updated. http://codereview.chromium.org/6312027/diff/10001/chrome/browser/download/dow... File chrome/browser/download/download_manager.cc (right): http://codereview.chromium.org/6312027/diff/10001/chrome/browser/download/dow... chrome/browser/download/download_manager.cc:845: // Insert into our full map. On 2011/02/11 19:17:49, Paweł Hajdan Jr. wrote: > nit: This comment doesn't add value, could you remove it? Done. http://codereview.chromium.org/6312027/diff/10001/chrome/browser/download/sav... File chrome/browser/download/save_package.cc (right): http://codereview.chromium.org/6312027/diff/10001/chrome/browser/download/sav... chrome/browser/download/save_package.cc:397: DownloadItem* item = new DownloadItem(download_manager, path, url, is_otr); On 2011/02/11 19:17:49, Paweł Hajdan Jr. wrote: > nit: If it's going to be stored on |item_| anyway, why have the temporary > |item|? Done. http://codereview.chromium.org/6312027/diff/10001/chrome/browser/download/sav... chrome/browser/download/save_package.cc:411: // Add entry to the history service On 2011/02/11 19:17:49, Paweł Hajdan Jr. wrote: > nit: Dot at the end. Also, please add a blank line above each comment, including > above. Done. http://codereview.chromium.org/6312027/diff/10001/chrome/browser/download/sav... chrome/browser/download/save_package.cc:697: download_manager->download_history()->RemoveEntry(download_); On 2011/02/11 19:10:07, rdsmith wrote: > I'm ok with the difference in behavior, but this is a difference in behavior > between downloads and save package activities. downloads are left in the > history when cancelled (unless they're dangerous). See for instance > DownloadManager::DownloadCancelled. Changed it to keep the download in the history. http://codereview.chromium.org/6312027/diff/10001/chrome/browser/download/sav... chrome/browser/download/save_package.cc:750: DownloadItem* item = download_; On 2011/02/11 19:10:07, rdsmith wrote: > What's the value of the local variable rather than using download_? > Removed local variable. http://codereview.chromium.org/6312027/diff/10001/chrome/browser/download/sav... chrome/browser/download/save_package.cc:756: // Update the download history On 2011/02/11 19:17:49, Paweł Hajdan Jr. wrote: > nit: Dot at the end. Done. http://codereview.chromium.org/6312027/diff/10001/chrome/browser/download/sav... File chrome/browser/download/save_package.h (right): http://codereview.chromium.org/6312027/diff/10001/chrome/browser/download/sav... chrome/browser/download/save_package.h:19: #include "chrome/browser/history/download_create_info.h" On 2011/02/11 19:17:49, Paweł Hajdan Jr. wrote: > nit: Is this include really needed? It should be possible to forward-declare > DownloadCreateInfo. See comment below. http://codereview.chromium.org/6312027/diff/10001/chrome/browser/download/sav... chrome/browser/download/save_package.h:167: // Called when Save Page As entry commited to the history system On 2011/02/11 19:10:07, rdsmith wrote: > nit: "is commited" Done. http://codereview.chromium.org/6312027/diff/10001/chrome/browser/download/sav... chrome/browser/download/save_package.h:168: void OnDownloadEntryAdded(DownloadCreateInfo info, int64 db_handle); On 2011/02/11 19:17:49, Paweł Hajdan Jr. wrote: > nit: |info| should probably be passed as const DownloadCreateInfo& info. This is actually a callback function for the history system, which expect DownloadCreateInfo to be passed by value. http://codereview.chromium.org/6312027/diff/10001/chrome/browser/download/sav... chrome/browser/download/save_package.h:169: // Called when a Save Page As download is started. On 2011/02/11 19:10:07, rdsmith wrote: > nit: I'd put a blank line above this comment. Done.
http://codereview.chromium.org/6312027/diff/16001/chrome/browser/download/sav... File chrome/browser/download/save_page_browsertest.cc (right): http://codereview.chromium.org/6312027/diff/16001/chrome/browser/download/sav... chrome/browser/download/save_page_browsertest.cc:52: EXPECT_EQ(1u, download_manager->history_downloads_.size()); This is not enough. I'd like to query the history subsystem for the data. Also, I'm somewhat worried about hardcoding "1" here, and a rather vague method name. How about making this check inline in the tests (you can use FRIEND_TEST_ALL_PREFIXES if needed), and just having a DownloadManager* shortcut accessor in the test fixture?
LGTM if tests as previously specified pass (make sure all the behaviors you expect to work from the downloads list work after either completed or cancelled run). http://codereview.chromium.org/6312027/diff/16001/chrome/browser/download/dow... File chrome/browser/download/download_manager.cc (right): http://codereview.chromium.org/6312027/diff/16001/chrome/browser/download/dow... chrome/browser/download/download_manager.cc:835: // call this function with an invalid |db_handle|. For instance, this can nit: Two spaces after "."
I updated the tests to query the history system and inlined the checks in the tests. The downloads page works as expected with both completed and cancelled items. http://codereview.chromium.org/6312027/diff/16001/chrome/browser/download/dow... File chrome/browser/download/download_manager.cc (right): http://codereview.chromium.org/6312027/diff/16001/chrome/browser/download/dow... chrome/browser/download/download_manager.cc:835: // call this function with an invalid |db_handle|. For instance, this can On 2011/02/14 21:42:42, rdsmith wrote: > nit: Two spaces after "." Done.
LGTM with one comment. http://codereview.chromium.org/6312027/diff/24001/chrome/browser/download/sav... File chrome/browser/download/save_page_browsertest.cc (right): http://codereview.chromium.org/6312027/diff/24001/chrome/browser/download/sav... chrome/browser/download/save_page_browsertest.cc:68: // Make a copy of the URLs returned by the history system. This should clear history_urls_ to avoid possible interference with previous calls to QueryDownloadHistory.
Fixed. http://codereview.chromium.org/6312027/diff/24001/chrome/browser/download/sav... File chrome/browser/download/save_page_browsertest.cc (right): http://codereview.chromium.org/6312027/diff/24001/chrome/browser/download/sav... chrome/browser/download/save_page_browsertest.cc:68: // Make a copy of the URLs returned by the history system. On 2011/02/18 11:11:35, Paweł Hajdan Jr. wrote: > This should clear history_urls_ to avoid possible interference with previous > calls to QueryDownloadHistory. Done.
Magnus: Do you need a committer for this, or are you ok doing it on your own? On 2011/02/18 17:54:01, magnus wrote: > Fixed. > > http://codereview.chromium.org/6312027/diff/24001/chrome/browser/download/sav... > File chrome/browser/download/save_page_browsertest.cc (right): > > http://codereview.chromium.org/6312027/diff/24001/chrome/browser/download/sav... > chrome/browser/download/save_page_browsertest.cc:68: // Make a copy of the URLs > returned by the history system. > On 2011/02/18 11:11:35, Paweł Hajdan Jr. wrote: > > This should clear history_urls_ to avoid possible interference with previous > > calls to QueryDownloadHistory. > > Done.
On 2011/02/22 22:22:02, rdsmith wrote: > Magnus: Do you need a committer for this, or are you ok doing it on your own? > I don't yet have commit access, so I'm going to need someone to help. If either of you guys are willing to commit the patch, that would be much appreciated.
I'll try to commit the patch over the next couple of days. I'm build sheriff next week; that's a good time to watch the tree :-}. -- Randy On Tue, Feb 22, 2011 at 9:38 PM, <fuzzac@gmail.com> wrote: > On 2011/02/22 22:22:02, rdsmith wrote: >> >> Magnus: Do you need a committer for this, or are you ok doing it on your >> own? > > > I don't yet have commit access, so I'm going to need someone to help. If > either > of you guys are willing to commit the patch, that would be much appreciated. > > > http://codereview.chromium.org/6312027/ > |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
