Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
7212017:
Add save package download items to history. (Closed)
|
Created:
9 years, 6 months ago by achuithb Modified:
9 years, 5 months ago CC:
chromium-reviews, rdsmith+dwatch_chromium.org Base URL:
svn://chrome-svn/chrome/trunk/src/ Visibility:
Public. |
DescriptionAdd save package download items to transient history on cros.
BUG=chromium-os:16431
TEST=Use right click 'Save As' to save an html file on cros. It should appear in the downloads panel.
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=91232
Patch Set 1 #
Total comments: 4
Patch Set 2 : '' #
Total comments: 4
Patch Set 3 : '' #Patch Set 4 : '' #Patch Set 5 : '' #Patch Set 6 : '' #
Messages
Total messages: 18 (0 generated)
http://codereview.chromium.org/7212017/diff/1/chrome/browser/download/downloa... File chrome/browser/download/download_manager.cc (right): http://codereview.chromium.org/7212017/diff/1/chrome/browser/download/downloa... chrome/browser/download/download_manager.cc:527: extension.erase(extension.begin()); // drop the . fix lint error. http://codereview.chromium.org/7212017/diff/1/chrome/browser/download/downloa... chrome/browser/download/download_manager.cc:975: void DownloadManager::SavePageAsDownloadStarted(DownloadItem* download) { rename for consistency. http://codereview.chromium.org/7212017/diff/1/chrome/browser/download/downloa... chrome/browser/download/download_manager.cc:1230: // The 'contents' may no longer exist if the user closed the tab before we fix lint error http://codereview.chromium.org/7212017/diff/1/chrome/browser/download/downloa... File chrome/browser/download/download_manager.h (right): http://codereview.chromium.org/7212017/diff/1/chrome/browser/download/downloa... chrome/browser/download/download_manager.h:31: #include <hash_map> fix lint error
Drive-by with download comments (chrome/browser/download/OWNERS, you need approval before landing). http://codereview.chromium.org/7212017/diff/4001/chrome/browser/download/down... File chrome/browser/download/download_manager.cc (right): http://codereview.chromium.org/7212017/diff/4001/chrome/browser/download/down... chrome/browser/download/download_manager.cc:981: // Add to history and notify the cros download panel. Why is it ChromeOS-specific? Those differences make the code really harder to maintain. http://codereview.chromium.org/7212017/diff/4001/chrome/browser/download/down... File chrome/browser/download/download_manager.h (right): http://codereview.chromium.org/7212017/diff/4001/chrome/browser/download/down... chrome/browser/download/download_manager.h:31: #include <hash_map> Shouldn't this be base/hash_tables? But why is this change necessary here?
There was a previous CL: http://codereview.chromium.org/6312027 attempting to fix http://crbug.com/4823 That CL was reverted due to crashiness issues. I've taken a pretty close look at the CL and it seems fairly innocuous, so I've got no idea what went wrong there. I believe in the long term we want to merge save_package into download_manager, but that's a significant undertaking. There's also some work towards implementing mhtml in webkit, and should that land, it may make save_package obsolete. Anyhow, my goal with this CL was to make the most non-intrusive change possible to fix the cros-specific issue (http://crosbug.com/16431) I'm happy to take out the cros ifdef. The side-effect of that will be that the downloads page on all platforms will temporary display html files (they aren't persisted in the history db, so when you restart chrome, these entries will be gone). Thanks! http://codereview.chromium.org/7212017/diff/4001/chrome/browser/download/down... File chrome/browser/download/download_manager.h (right): http://codereview.chromium.org/7212017/diff/4001/chrome/browser/download/down... chrome/browser/download/download_manager.h:31: #include <hash_map> On 2011/06/21 08:05:30, Paweł Hajdan Jr. wrote: > Shouldn't this be base/hash_tables? But why is this change necessary here? It is base/hash_tables.h. I've made that change, but I can pull it out if you think it's unnecessary. I was fixing some lint errors (include_what_you_use in this case) since I touched these files. There's a bug in the linter since it's not satisfied with the inclusion of base/hash_tables.h. Remove or keep hash_tables.h include?
I'd really prefer the ChromeOS #ifdef to be removed. And maybe we should persist those downloads in the history... why don't we? Also, could you write an automated test for this change? http://codereview.chromium.org/7212017/diff/4001/chrome/browser/download/down... File chrome/browser/download/download_manager.h (right): http://codereview.chromium.org/7212017/diff/4001/chrome/browser/download/down... chrome/browser/download/download_manager.h:31: #include <hash_map> On 2011/06/21 22:51:37, achuith.bhandarkar wrote: > On 2011/06/21 08:05:30, Paweł Hajdan Jr. wrote: > > Shouldn't this be base/hash_tables? But why is this change necessary here? > > It is base/hash_tables.h. I've made that change, but I can pull it out if you > think it's unnecessary. I was fixing some lint errors (include_what_you_use in > this case) since I touched these files. There's a bug in the linter since it's > not satisfied with the inclusion of base/hash_tables.h. Remove or keep > hash_tables.h include? I'd say keep it, and report the linter bug to the authors.
FYI, the crash the resulted in the earlier revert was http://crbug.com/76963; see my comment # 6 for context. I'm really worried that doing this without unifying the SavePackage and Downloads systems will be stepping into a tar pit. I'll comment more later today, hopefully with more content than just my gut feeling :-}, but I wanted to give you guys the context for the earlier revert. I agree with Pawel that I don't want the ChromeOs ifdef to go in unless it's very necessary, and I'm worried about doing this little to fix this problem (e.g. if you call NotifyModelChanged without ShowDownloadInBrowser, you've got a disparity between the state the download shelf shows and what downloads.html will show). I'd rather try and resurrect the earlier patch and fix the problems that the above issue surfaced.
Ok, more contentful musings:
* I think this might be ok as a CrOS-only change. But I'd rather not do that,
both because in general I think it's a slippery slope of putting in CrOS ifdefs
everywhere, and this is a bug on all platforms, and in specific I'm concerned
about yet more divergence and complexity in the Downloads/SavePackage interface.
We've already got plenty :-{. If we do go in the direction of a CrOS-only
change, I'd like to understand what the motivation/priority is for it from the
CrOS side, and I'll want to really grok the CrOS downloads interface in order to
suggest the right suite of tests (which, arguably, I should do anyway).
* If we try to make this a general change, I think we need to address all the
issues on the previous patch (making sure the various actions from the downloads
page and download shelf work properly in this new context), as well as the issue
raised by the crash (finding the downloads manager after tab close), as well as
the issue I raise in my patch (making sure the download item is destroyed when
the download manager is destroyed). This last may not be an issue (I don't
think the download manager can be destroyed if the tab is still around) but I'd
like testing in that area. And the simplest way to deal with my discomfort
would be to fix 58183 as well.
Magnus (original patch committer) might be willing to pick this back up--I don't
know if he'd still be interested in this work (he expressed continuing interest
when I reverted his patch, but we decided to hold off since we expected, and
still expect, to unify SavePackage and downloads at some point--current target
Q4). Or you could do the iterations yourself, Achuith. I'm not sure what makes
sense.
Thanks for the link to the bug. I think that's very fixable. Here's another suggestion - I create 2 patches. One with the 2 line fix in SavePageAsDownloadStarted, either Cros-specific or not. Another patch which resurrects Magnus CL and fixes the crash (I'm happy to see it through the iterations). The first patch would be low risk and targeted towards crosbug.com/16431, and the second one more comprehensive (and higher risk), and targeted towards http://crbug.com/4823, http://crbug.com/76963 and http://crbug.com/58183. The idea is that if the second patch needs to be reverted, I can still have a fix for the Cros bug. What do both of you think? This bug is quite annoying on Cros - you 'Save As' a web page, and we popup an empty downloads panel. The downloads panel cannot query download_manager about the newly downloaded file and does not receive notifications about Save As downloads. About calling NotifyModelChanged without ShowDownloadInBrowser - we already have a disparity between what shows in the DownloadShelf and downloads.html. SavePackage::Init directly calls OnStartDownload which I believe ends up calling Browser::OnStartDownload which launches the DownloadShelf, but the file never appears in downloads.html.
I'm tentatively ok with doing this as a two-phased approach if you commit to pushing the second phase through, depending on how clean and limited we can make the first phase. Let's try to make the first phase be non-CrOS specific, and fall back to that if it turns out to be too hard. Could you (on both download panel and the downloads page) confirm that all functionality advertised by the page actually works after this change for one of these items? I don't know what the CrOS downloads panel advertised for functionality (does it do anything other than display downloads?) but for the downloads.html page what I'd like to be sure is: * Show in finder, and remove from list work. * The functionality displayed during the download makes sense. * Either the file name isn't a link or clicking on it does something equivalent to "opening" the download. * During the save page download, either "cancel" works (which is complicated so you'll probably want to do it as part of the longer fix) or the "cancel" button/link is not enabled? If you actually enter the download into the history, could you make sure that the correspondence between presentation and actual functionality on downloads.html remains after you stop and restart the browser?
On 2011/06/22 16:28:13, achuith.bhandarkar wrote: > Here's another suggestion - I create 2 patches. One with the 2 line fix in > SavePageAsDownloadStarted, either Cros-specific or not. I'm fine with a split, just please make sure each patch will be covered by a test. > Another patch which > resurrects Magnus CL and fixes the crash (I'm happy to see it through the > iterations). The first patch would be low risk and targeted towards > crosbug.com/16431, and the second one more comprehensive (and higher risk), and > targeted towards http://crbug.com/4823, http://crbug.com/76963 and > http://crbug.com/58183. The idea is that if the second patch needs to be > reverted, I can still have a fix for the Cros bug. Planning for landing safety sounds good.
On 2011/06/22 18:00:52, rdsmith wrote: > I'm tentatively ok with doing this as a two-phased approach if you commit to > pushing the second phase through, depending on how clean and limited we can make > the first phase. Let's try to make the first phase be non-CrOS specific, and > fall back to that if it turns out to be too hard. Non-cros seems to work fine. > Could you (on both download panel and the downloads page) confirm that all > functionality advertised by the page actually works after this change for one of > these items? I don't know what the CrOS downloads panel advertised for > functionality (does it do anything other than display downloads?) but for the > downloads.html page what I'd like to be sure is: > * Show in finder, and remove from list work. Works as you'd expect. > * The functionality displayed during the download makes sense. There are a couple of legacy bugs which I would want to fix with the next patch instead. The progress is incorrect (we display the progress of the parent page without taking into account the size of the linked assets). Pause/Resume/Cancel also don't work, but we're tracking that with http://crosbug.com/58183. > * Either the file name isn't a link or clicking on it does something equivalent > to "opening" the download. This works - it goes to a file:// url for the downloaded file and everything works as you would expect. > * During the save page download, either "cancel" works (which is complicated so > you'll probably want to do it as part of the longer fix) or the "cancel" > button/link is not enabled? Both of these are not easy to do :/ For disabling the link I have to mark the download item as a save_page item and have the UI hide it in javascript. It's non-trivial enough that I'd probably want to fix it the right way instead if possible. > If you actually enter the download into the history, could you make sure that > the correspondence between presentation and actual functionality on > downloads.html remains after you stop and restart the browser? Not going to enter the download into history with this CL. Next patch.
On 2011/06/27 22:30:40, achuith.bhandarkar wrote: > > * During the save page download, either "cancel" works (which is complicated > so > > you'll probably want to do it as part of the longer fix) or the "cancel" > > button/link is not enabled? > > Both of these are not easy to do :/ For disabling the link I have to mark the > download item as a save_page item and have the UI hide it in javascript. It's > non-trivial enough that I'd probably want to fix it the right way instead if > possible. It sounds like you're focussing on the webui and I was focussing on the download shelf; between us we'll actually cover all the bases! For the DownloadShelf, see DownloadShelfContextMenu::IsCommendIdEnabled. Given this this currently doesn't work (http://crbug.com/58183) I'm ok with you fixing it in your second patch. Are you ok with fixing it in that patch? Either by disabling the option, or by making cancel actually work? (Warning: The second may be tricky.) I think I'll say LGTM, given your testing and presuming you're willing to fix the cancel problem in one fashion or another in your next patch. I would recommend you watch the canaries for crashes for a few days after you commit.
I'm going to check this in so I can get started on the next round of fixes. I'll look at having a patch that fixes the permanent history issue and the cancellation issue with save_package, but if the code for those issues turns out to be orthogonal I'll split them up into 2 patches for ease of review and ease of revertability.
I'd rather you wait for Pawel's LGTM, if you could.
Oops, ok!
Pawel - ping?
Randy - I believe Pawel is on a bit of a break. Mind if I go ahead and land this CL?
Nope, go ahead. Sorry to slow you down.
Change committed as 91232 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
