Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
10980002:
Mac Web Intents Part 1: Show extension download progress (Closed)
|
Created:
8 years, 3 months ago by sail Modified:
8 years, 2 months ago Reviewers:
asanka, smckay, Avi (use Gerrit), miket_OOO, sky, groby-ooo-7-16, Randy Smith (Not in Mondays), Steve McKay, Mihai Parparita -not on Chrome, Nico, Greg Billock, Robert Sesek CC:
chromium-reviews, mihaip-chromium-reviews_chromium.org, Aaron Boodman, groby+watch_chromium.org, rdsmith+dwatch_chromium.org, gbillock+watch_chromium.org, smckay+watch_chromium.org, rouslan+watch_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionMac Web Intents Part 1: Show extension download progress
As per spec we want to show the extension download UI through the Web Intents Picker dialog.
This CL also pipes download progress to the WebIntentPickerModel.
I'll send out a separate CL to update the Mac UI to show the download progress.
I also have a separate CL out for review to hide the download from the browser download shelf: https://codereview.chromium.org/11016022
BUG=152010
TEST=Go to http://webintents.org. Click share. Click "Add to Chrome". Verify that the download shelf is not shown.
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=159836
Patch Set 1 #
Total comments: 44
Patch Set 2 : a #Patch Set 3 : address review comments #
Total comments: 4
Patch Set 4 : address review comments #Patch Set 5 : added test #
Total comments: 6
Patch Set 6 : address review comments, fix test #Patch Set 7 : remove downloads code #Messages
Total messages: 38 (0 generated)
Looks pretty good. http://codereview.chromium.org/10980002/diff/1/chrome/browser/download/downlo... File chrome/browser/download/download_util.cc (right): http://codereview.chromium.org/10980002/diff/1/chrome/browser/download/downlo... chrome/browser/download/download_util.cc:509: item->SetUserData(kShowInShelfKey, new ShowInShelfData(should_show)); Can't you just add a bool to DownloadItem? http://codereview.chromium.org/10980002/diff/1/chrome/browser/download/downlo... File chrome/browser/download/download_util.h (right): http://codereview.chromium.org/10980002/diff/1/chrome/browser/download/downlo... chrome/browser/download/download_util.h:192: bool GetShouldShowInShelf(content::DownloadItem* item); nit: getshould sounds a bit awkward. just shouldshowinshelf. http://codereview.chromium.org/10980002/diff/1/chrome/browser/extensions/api/... File chrome/browser/extensions/api/webstore_private/webstore_private_api.h (right): http://codereview.chromium.org/10980002/diff/1/chrome/browser/extensions/api/... chrome/browser/extensions/api/webstore_private/webstore_private_api.h:162: bool cancelled) OVERRIDE; prefer enums over bool parameters http://codereview.chromium.org/10980002/diff/1/chrome/browser/extensions/webs... File chrome/browser/extensions/webstore_installer.cc (right): http://codereview.chromium.org/10980002/diff/1/chrome/browser/extensions/webs... chrome/browser/extensions/webstore_installer.cc:366: delegate_->OnExtensionInstallFailure(id_, error, cancelled); nit: It looks like cancelled is true iff error == kInstallCanceledError -- is that right? can you check for that instead? (not sure if that's better) http://codereview.chromium.org/10980002/diff/1/chrome/browser/ui/browser.cc File chrome/browser/ui/browser.cc (right): http://codereview.chromium.org/10980002/diff/1/chrome/browser/ui/browser.cc#n... chrome/browser/ui/browser.cc:1425: return; test http://codereview.chromium.org/10980002/diff/1/chrome/browser/ui/intents/web_... File chrome/browser/ui/intents/web_intent_picker_controller.cc (right): http://codereview.chromium.org/10980002/diff/1/chrome/browser/ui/intents/web_... chrome/browser/ui/intents/web_intent_picker_controller.cc:545: download_util::SetShouldShowInShelf(item, false); From what I understand, this relies on this notification running before the "download started" notification, so that this flag is set before the shelf decides if it needs to do anything. Is that right? Is that enforced somehow? http://codereview.chromium.org/10980002/diff/1/chrome/browser/ui/intents/web_... chrome/browser/ui/intents/web_intent_picker_controller.cc:603: // If the user cancelled the install then don't show an error message. nit: canceled
http://codereview.chromium.org/10980002/diff/1/chrome/browser/download/downlo... File chrome/browser/download/download_util.cc (right): http://codereview.chromium.org/10980002/diff/1/chrome/browser/download/downlo... chrome/browser/download/download_util.cc:504: static_cast<ShowInShelfData*>(item->GetUserData(kShowInShelfKey)); Do we not have a type token wrapper for getting data out of one of these by type? http://codereview.chromium.org/10980002/diff/1/chrome/browser/ui/intents/web_... File chrome/browser/ui/intents/web_intent_picker_controller.cc (right): http://codereview.chromium.org/10980002/diff/1/chrome/browser/ui/intents/web_... chrome/browser/ui/intents/web_intent_picker_controller.cc:475: picker_model_->SetPendingInstallExtensionId(id); Needed? I don't see where this is used.
[Adding Asanka as reviewer--I'd prefer this not go in without his ok, as he's Mr. Downloads UI. Adding Aaron on the cc line; he may want to take a look at this as it has to do with extension install. ] Greg, a couple of comments/questions: * Could you give me a pointer to the spec you're referring to? The bug doesn't mention it. * Does this affect all extension downloads, or just some subset? If all, that's good news, as it means we can fold this work in with work the webstore install folks wanted us to do to so that webstore initiated downloads didn't show up in the download shelf at all. * I agree with Nico (sort of :-}). Situations where SupportsUserData are used to encode a boolean have a code smell to me; the reason I made DownloadItem have that functionality was to allow pulling apart of the download system into subsystems, where each (presumably meaty) subsystem would have their own download associated structure of data. In this particular case, I think the "yes/no" for the download shelf belongs in the UI subsystem, which Asanka is beginning to look at pulling out. For now, I think we should have it as a boolean on the download item, preferably removing the temporary boolean at the same time (thought that'll require a bit of work). But at a high level this looks like a lot of work to get around getting the download subsystem to provide you a bit of functionality that it should provide anyway.
On 2012/09/25 13:51:52, rdsmith wrote: > [Adding Asanka as reviewer--I'd prefer this not go in without his ok, as he's > Mr. Downloads UI. Adding Aaron on the cc line; he may want to take a look at > this as it has to do with extension install. ] > > Greg, a couple of comments/questions: Sorry, this was seeing what I expected to see. Replace "Greg" with "@sail:" :-}. > > * Could you give me a pointer to the spec you're referring to? The bug doesn't > mention it. > > * Does this affect all extension downloads, or just some subset? If all, that's > good news, as it means we can fold this work in with work the webstore install > folks wanted us to do to so that webstore initiated downloads didn't show up in > the download shelf at all. > > * I agree with Nico (sort of :-}). Situations where SupportsUserData are used > to encode a boolean have a code smell to me; the reason I made DownloadItem have > that functionality was to allow pulling apart of the download system into > subsystems, where each (presumably meaty) subsystem would have their own > download associated structure of data. In this particular case, I think the > "yes/no" for the download shelf belongs in the UI subsystem, which Asanka is > beginning to look at pulling out. For now, I think we should have it as a > boolean on the download item, preferably removing the temporary boolean at the > same time (thought that'll require a bit of work). But at a high level this > looks like a lot of work to get around getting the download subsystem to provide > you a bit of functionality that it should provide anyway.
I agree with Randy that the 'should show in downloads UI' flag is better off in the DownloadItem for now. http://codereview.chromium.org/10980002/diff/1/chrome/browser/ui/intents/web_... File chrome/browser/ui/intents/web_intent_picker_controller.cc (right): http://codereview.chromium.org/10980002/diff/1/chrome/browser/ui/intents/web_... chrome/browser/ui/intents/web_intent_picker_controller.cc:545: download_util::SetShouldShowInShelf(item, false); On 2012/09/25 03:07:18, Nico wrote: > From what I understand, this relies on this notification running before the > "download started" notification, so that this flag is set before the shelf > decides if it needs to do anything. Is that right? Is that enforced somehow? The ordering is: 1. DownloadManager::OnDownloadCreated(). Called when the DownloadItem is created. 2. OnStartedCallback for the download request. This function (OnExtensionDownloadStarted) is running as part of this step. ShowShouldInShelf flag is set here. 3. <download filename determination> 4. WebContentsDelegate::OnStartDownload(). Browser::OnStartDownload() checks the ShouldShowInShelf flag here. http://codereview.chromium.org/10980002/diff/1/chrome/browser/ui/intents/web_... chrome/browser/ui/intents/web_intent_picker_controller.cc:602: CancelDownload(); Why call CancelDownload() here?
http://codereview.chromium.org/10980002/diff/1/chrome/browser/download/downlo... File chrome/browser/download/download_util.cc (right): http://codereview.chromium.org/10980002/diff/1/chrome/browser/download/downlo... chrome/browser/download/download_util.cc:509: item->SetUserData(kShowInShelfKey, new ShowInShelfData(should_show)); On 2012/09/25 03:07:18, Nico wrote: > Can't you just add a bool to DownloadItem? That'd probably work if you generalize the property to something NOT "shelf" related. See my comment in the header file. Then you could simply revert these changes and inspect the property directly. http://codereview.chromium.org/10980002/diff/1/chrome/browser/download/downlo... File chrome/browser/download/download_util.h (right): http://codereview.chromium.org/10980002/diff/1/chrome/browser/download/downlo... chrome/browser/download/download_util.h:192: bool GetShouldShowInShelf(content::DownloadItem* item); On 2012/09/25 03:07:18, Nico wrote: > nit: getshould sounds a bit awkward. just shouldshowinshelf. A thought about naming. Whether the shelf should show or not is a behavior we want based on a property of this download item. So instead of naming this WRT the behavior we want, we could name it according to the kind of property we need to make the decision. This avoids situations in the future where callers need access to the same property for non-shelf reasons. e.g. We could tag the download with "inprocess", "hidden", "system", or slightly more specific "intent-data". HasHiddenProperty(downloaditem) SetHiddenProperty(downloaditem, bool) or HasInProcessProperty(downloaditem) SetInProcessProperty(downloaditem, bool) http://codereview.chromium.org/10980002/diff/1/chrome/browser/extensions/api/... File chrome/browser/extensions/api/webstore_private/webstore_private_api.h (right): http://codereview.chromium.org/10980002/diff/1/chrome/browser/extensions/api/... chrome/browser/extensions/api/webstore_private/webstore_private_api.h:162: bool cancelled) OVERRIDE; On 2012/09/25 03:07:18, Nico wrote: > prefer enums over bool parameters +1, much more flexible and can aide readability too. http://codereview.chromium.org/10980002/diff/1/chrome/browser/extensions/webs... File chrome/browser/extensions/webstore_installer.cc (right): http://codereview.chromium.org/10980002/diff/1/chrome/browser/extensions/webs... chrome/browser/extensions/webstore_installer.cc:227: ReportFailure(kInstallCanceledError, true); Add a method name ReportCancelled, call that instead. http://codereview.chromium.org/10980002/diff/1/chrome/browser/ui/intents/web_... File chrome/browser/ui/intents/web_intent_picker_controller.cc (right): http://codereview.chromium.org/10980002/diff/1/chrome/browser/ui/intents/web_... chrome/browser/ui/intents/web_intent_picker_controller.cc:545: download_util::SetShouldShowInShelf(item, false); On 2012/09/25 03:07:18, Nico wrote: > From what I understand, this relies on this notification running before the > "download started" notification, so that this flag is set before the shelf > decides if it needs to do anything. Is that right? Is that enforced somehow? What if we flipped the perspective a bit and started all downloads as "system" download, then promote them to "user" downloads once we know the download is actually a user "download" and not the system loading the data for some other purpose. http://codereview.chromium.org/10980002/diff/1/chrome/browser/ui/intents/web_... chrome/browser/ui/intents/web_intent_picker_controller.cc:1100: void WebIntentPickerController::UpdateDownloadState( Feels like this entire method should be pushed into the picker model...and named UpdateExtensionDownloadState. http://codereview.chromium.org/10980002/diff/1/chrome/browser/ui/intents/web_... chrome/browser/ui/intents/web_intent_picker_controller.cc:1108: void WebIntentPickerController::CancelDownload() { CancelExtensionDownload Does the picker model need to be updated too? http://codereview.chromium.org/10980002/diff/1/chrome/browser/ui/intents/web_... File chrome/browser/ui/intents/web_intent_picker_model.cc (right): http://codereview.chromium.org/10980002/diff/1/chrome/browser/ui/intents/web_... chrome/browser/ui/intents/web_intent_picker_model.cc:28: pending_install_download_percent_(0) { pending_install_download_percent_ > pending_install_download_progress_, then just document legal values as 00-99 where the field is declared. http://codereview.chromium.org/10980002/diff/1/chrome/browser/ui/intents/web_... chrome/browser/ui/intents/web_intent_picker_model.cc:66: ClearPendingInstall(); There are a lot of things at play in the picker. I think you should qualify all Extension specific fields/methods with the work "Extension". ClearPendingExtensionInstall.
http://codereview.chromium.org/10980002/diff/1/chrome/browser/ui/intents/web_... File chrome/browser/ui/intents/web_intent_picker_controller.cc (right): http://codereview.chromium.org/10980002/diff/1/chrome/browser/ui/intents/web_... chrome/browser/ui/intents/web_intent_picker_controller.cc:575: if (picker_) nit: This should never be NULL. Can we just DCHECK(picker_)? http://codereview.chromium.org/10980002/diff/1/chrome/browser/ui/intents/web_... File chrome/browser/ui/intents/web_intent_picker_model.cc (right): http://codereview.chromium.org/10980002/diff/1/chrome/browser/ui/intents/web_... chrome/browser/ui/intents/web_intent_picker_model.cc:174: observer_->OnModelChanged(this); Pre your refactor, OnModelChanged used to trigger a performLayout, which used to be a bit extensive. (I.e. visual artifacts). Is that fixed?
FYI, I'm not in the extensions OWNERS file anymore, so you'll need to get one of them to take a look too.
Hi Steve, sorry for the slow response. I should have most of this addressed later today. Thanks On Mon, Oct 1, 2012 at 1:24 PM, â…” Steve McKay <smckay@google.com> wrote: > Any word on my comments Sailesh? > > Steve McKay | Engineering | smckay@google.com | 310-359-8331 > > > > > On Tue, Sep 25, 2012 at 11:56 AM, <smckay@chromium.org> wrote: > >> >> http://codereview.chromium.**org/10980002/diff/1/chrome/** >> browser/download/download_**util.cc<http://codereview.chromium.org/10980002/diff/1/chrome/browser/download/download_util.cc> >> File chrome/browser/download/**download_util.cc (right): >> >> http://codereview.chromium.**org/10980002/diff/1/chrome/** >> browser/download/download_**util.cc#newcode509<http://codereview.chromium.org/10980002/diff/1/chrome/browser/download/download_util.cc#newcode509> >> chrome/browser/download/**download_util.cc:509: >> item->SetUserData(**kShowInShelfKey, new ShowInShelfData(should_show)); >> On 2012/09/25 03:07:18, Nico wrote: >> >>> Can't you just add a bool to DownloadItem? >>> >> >> That'd probably work if you generalize the property to something NOT >> "shelf" related. See my comment in the header file. >> >> Then you could simply revert these changes and inspect the property >> directly. >> >> >> http://codereview.chromium.**org/10980002/diff/1/chrome/** >> browser/download/download_**util.h<http://codereview.chromium.org/10980002/diff/1/chrome/browser/download/download_util.h> >> File chrome/browser/download/**download_util.h (right): >> >> http://codereview.chromium.**org/10980002/diff/1/chrome/** >> browser/download/download_**util.h#newcode192<http://codereview.chromium.org/10980002/diff/1/chrome/browser/download/download_util.h#newcode192> >> chrome/browser/download/**download_util.h:192: bool >> GetShouldShowInShelf(content::**DownloadItem* item); >> On 2012/09/25 03:07:18, Nico wrote: >> >>> nit: getshould sounds a bit awkward. just shouldshowinshelf. >>> >> >> A thought about naming. Whether the shelf should show or not is a >> behavior we want based on a property of this download item. So instead >> of naming this WRT the behavior we want, we could name it according to >> the kind of property we need to make the decision. This avoids >> situations in the future where callers need access to the same property >> for non-shelf reasons. >> >> e.g. We could tag the download with "inprocess", "hidden", "system", or >> slightly more specific "intent-data". >> >> HasHiddenProperty(**downloaditem) >> SetHiddenProperty(**downloaditem, bool) >> >> or >> >> HasInProcessProperty(**downloaditem) >> SetInProcessProperty(**downloaditem, bool) >> >> >> http://codereview.chromium.**org/10980002/diff/1/chrome/** >> browser/extensions/api/**webstore_private/webstore_**private_api.h<http://codereview.chromium.org/10980002/diff/1/chrome/browser/extensions/api/webstore_private/webstore_private_api.h> >> File >> chrome/browser/extensions/api/**webstore_private/webstore_**private_api.h >> (right): >> >> http://codereview.chromium.**org/10980002/diff/1/chrome/** >> browser/extensions/api/**webstore_private/webstore_** >> private_api.h#newcode162<http://codereview.chromium.org/10980002/diff/1/chrome/browser/extensions/api/webstore_private/webstore_private_api.h#newcode162> >> chrome/browser/extensions/api/**webstore_private/webstore_** >> private_api.h:162: >> bool cancelled) OVERRIDE; >> On 2012/09/25 03:07:18, Nico wrote: >> >>> prefer enums over bool parameters >>> >> >> +1, much more flexible and can aide readability too. >> >> >> http://codereview.chromium.**org/10980002/diff/1/chrome/** >> browser/extensions/webstore_**installer.cc<http://codereview.chromium.org/10980002/diff/1/chrome/browser/extensions/webstore_installer.cc> >> File chrome/browser/extensions/**webstore_installer.cc (right): >> >> http://codereview.chromium.**org/10980002/diff/1/chrome/** >> browser/extensions/webstore_**installer.cc#newcode227<http://codereview.chromium.org/10980002/diff/1/chrome/browser/extensions/webstore_installer.cc#newcode227> >> chrome/browser/extensions/**webstore_installer.cc:227: >> ReportFailure(**kInstallCanceledError, true); >> Add a method name ReportCancelled, call that instead. >> >> >> http://codereview.chromium.**org/10980002/diff/1/chrome/** >> browser/ui/intents/web_intent_**picker_controller.cc<http://codereview.chromium.org/10980002/diff/1/chrome/browser/ui/intents/web_intent_picker_controller.cc> >> File chrome/browser/ui/intents/web_**intent_picker_controller.cc (right): >> >> http://codereview.chromium.**org/10980002/diff/1/chrome/** >> browser/ui/intents/web_intent_**picker_controller.cc#**newcode545<http://codereview.chromium.org/10980002/diff/1/chrome/browser/ui/intents/web_intent_picker_controller.cc#newcode545> >> chrome/browser/ui/intents/web_**intent_picker_controller.cc:**545: >> download_util::**SetShouldShowInShelf(item, false); >> On 2012/09/25 03:07:18, Nico wrote: >> >>> From what I understand, this relies on this notification running >>> >> before the >> >>> "download started" notification, so that this flag is set before the >>> >> shelf >> >>> decides if it needs to do anything. Is that right? Is that enforced >>> >> somehow? >> >> What if we flipped the perspective a bit and started all downloads as >> "system" download, then promote them to "user" downloads once we know >> the download is actually a user "download" and not the system loading >> the data for some other purpose. >> >> http://codereview.chromium.**org/10980002/diff/1/chrome/** >> browser/ui/intents/web_intent_**picker_controller.cc#**newcode1100<http://codereview.chromium.org/10980002/diff/1/chrome/browser/ui/intents/web_intent_picker_controller.cc#newcode1100> >> chrome/browser/ui/intents/web_**intent_picker_controller.cc:**1100: void >> WebIntentPickerController::**UpdateDownloadState( >> Feels like this entire method should be pushed into the picker >> model...and named UpdateExtensionDownloadState. >> >> http://codereview.chromium.**org/10980002/diff/1/chrome/** >> browser/ui/intents/web_intent_**picker_controller.cc#**newcode1108<http://codereview.chromium.org/10980002/diff/1/chrome/browser/ui/intents/web_intent_picker_controller.cc#newcode1108> >> chrome/browser/ui/intents/web_**intent_picker_controller.cc:**1108: void >> WebIntentPickerController::**CancelDownload() { >> CancelExtensionDownload >> >> Does the picker model need to be updated too? >> >> http://codereview.chromium.**org/10980002/diff/1/chrome/** >> browser/ui/intents/web_intent_**picker_model.cc<http://codereview.chromium.org/10980002/diff/1/chrome/browser/ui/intents/web_intent_picker_model.cc> >> File chrome/browser/ui/intents/web_**intent_picker_model.cc (right): >> >> http://codereview.chromium.**org/10980002/diff/1/chrome/** >> browser/ui/intents/web_intent_**picker_model.cc#newcode28<http://codereview.chromium.org/10980002/diff/1/chrome/browser/ui/intents/web_intent_picker_model.cc#newcode28> >> chrome/browser/ui/intents/web_**intent_picker_model.cc:28: >> pending_install_download_**percent_(0) { >> pending_install_download_**percent_ > pending_install_download_** >> progress_, >> then just document legal values as 00-99 where the field is declared. >> >> http://codereview.chromium.**org/10980002/diff/1/chrome/** >> browser/ui/intents/web_intent_**picker_model.cc#newcode66<http://codereview.chromium.org/10980002/diff/1/chrome/browser/ui/intents/web_intent_picker_model.cc#newcode66> >> chrome/browser/ui/intents/web_**intent_picker_model.cc:66: >> ClearPendingInstall(); >> There are a lot of things at play in the picker. I think you should >> qualify all Extension specific fields/methods with the work "Extension". >> >> ClearPendingExtensionInstall. >> >> http://codereview.chromium.**org/10980002/<http://codereview.chromium.org/109... >> > >
Any word on my comments Sailesh? Steve McKay | Engineering | smckay@google.com | 310-359-8331 On Tue, Sep 25, 2012 at 11:56 AM, <smckay@chromium.org> wrote: > > http://codereview.chromium.**org/10980002/diff/1/chrome/** > browser/download/download_**util.cc<http://codereview.chromium.org/10980002/diff/1/chrome/browser/download/download_util.cc> > File chrome/browser/download/**download_util.cc (right): > > http://codereview.chromium.**org/10980002/diff/1/chrome/** > browser/download/download_**util.cc#newcode509<http://codereview.chromium.org/10980002/diff/1/chrome/browser/download/download_util.cc#newcode509> > chrome/browser/download/**download_util.cc:509: > item->SetUserData(**kShowInShelfKey, new ShowInShelfData(should_show)); > On 2012/09/25 03:07:18, Nico wrote: > >> Can't you just add a bool to DownloadItem? >> > > That'd probably work if you generalize the property to something NOT > "shelf" related. See my comment in the header file. > > Then you could simply revert these changes and inspect the property > directly. > > > http://codereview.chromium.**org/10980002/diff/1/chrome/** > browser/download/download_**util.h<http://codereview.chromium.org/10980002/diff/1/chrome/browser/download/download_util.h> > File chrome/browser/download/**download_util.h (right): > > http://codereview.chromium.**org/10980002/diff/1/chrome/** > browser/download/download_**util.h#newcode192<http://codereview.chromium.org/10980002/diff/1/chrome/browser/download/download_util.h#newcode192> > chrome/browser/download/**download_util.h:192: bool > GetShouldShowInShelf(content::**DownloadItem* item); > On 2012/09/25 03:07:18, Nico wrote: > >> nit: getshould sounds a bit awkward. just shouldshowinshelf. >> > > A thought about naming. Whether the shelf should show or not is a > behavior we want based on a property of this download item. So instead > of naming this WRT the behavior we want, we could name it according to > the kind of property we need to make the decision. This avoids > situations in the future where callers need access to the same property > for non-shelf reasons. > > e.g. We could tag the download with "inprocess", "hidden", "system", or > slightly more specific "intent-data". > > HasHiddenProperty(**downloaditem) > SetHiddenProperty(**downloaditem, bool) > > or > > HasInProcessProperty(**downloaditem) > SetInProcessProperty(**downloaditem, bool) > > > http://codereview.chromium.**org/10980002/diff/1/chrome/** > browser/extensions/api/**webstore_private/webstore_**private_api.h<http://codereview.chromium.org/10980002/diff/1/chrome/browser/extensions/api/webstore_private/webstore_private_api.h> > File > chrome/browser/extensions/api/**webstore_private/webstore_**private_api.h > (right): > > http://codereview.chromium.**org/10980002/diff/1/chrome/** > browser/extensions/api/**webstore_private/webstore_** > private_api.h#newcode162<http://codereview.chromium.org/10980002/diff/1/chrome/browser/extensions/api/webstore_private/webstore_private_api.h#newcode162> > chrome/browser/extensions/api/**webstore_private/webstore_** > private_api.h:162: > bool cancelled) OVERRIDE; > On 2012/09/25 03:07:18, Nico wrote: > >> prefer enums over bool parameters >> > > +1, much more flexible and can aide readability too. > > > http://codereview.chromium.**org/10980002/diff/1/chrome/** > browser/extensions/webstore_**installer.cc<http://codereview.chromium.org/10980002/diff/1/chrome/browser/extensions/webstore_installer.cc> > File chrome/browser/extensions/**webstore_installer.cc (right): > > http://codereview.chromium.**org/10980002/diff/1/chrome/** > browser/extensions/webstore_**installer.cc#newcode227<http://codereview.chromium.org/10980002/diff/1/chrome/browser/extensions/webstore_installer.cc#newcode227> > chrome/browser/extensions/**webstore_installer.cc:227: > ReportFailure(**kInstallCanceledError, true); > Add a method name ReportCancelled, call that instead. > > > http://codereview.chromium.**org/10980002/diff/1/chrome/** > browser/ui/intents/web_intent_**picker_controller.cc<http://codereview.chromium.org/10980002/diff/1/chrome/browser/ui/intents/web_intent_picker_controller.cc> > File chrome/browser/ui/intents/web_**intent_picker_controller.cc (right): > > http://codereview.chromium.**org/10980002/diff/1/chrome/** > browser/ui/intents/web_intent_**picker_controller.cc#**newcode545<http://codereview.chromium.org/10980002/diff/1/chrome/browser/ui/intents/web_intent_picker_controller.cc#newcode545> > chrome/browser/ui/intents/web_**intent_picker_controller.cc:**545: > download_util::**SetShouldShowInShelf(item, false); > On 2012/09/25 03:07:18, Nico wrote: > >> From what I understand, this relies on this notification running >> > before the > >> "download started" notification, so that this flag is set before the >> > shelf > >> decides if it needs to do anything. Is that right? Is that enforced >> > somehow? > > What if we flipped the perspective a bit and started all downloads as > "system" download, then promote them to "user" downloads once we know > the download is actually a user "download" and not the system loading > the data for some other purpose. > > http://codereview.chromium.**org/10980002/diff/1/chrome/** > browser/ui/intents/web_intent_**picker_controller.cc#**newcode1100<http://codereview.chromium.org/10980002/diff/1/chrome/browser/ui/intents/web_intent_picker_controller.cc#newcode1100> > chrome/browser/ui/intents/web_**intent_picker_controller.cc:**1100: void > WebIntentPickerController::**UpdateDownloadState( > Feels like this entire method should be pushed into the picker > model...and named UpdateExtensionDownloadState. > > http://codereview.chromium.**org/10980002/diff/1/chrome/** > browser/ui/intents/web_intent_**picker_controller.cc#**newcode1108<http://codereview.chromium.org/10980002/diff/1/chrome/browser/ui/intents/web_intent_picker_controller.cc#newcode1108> > chrome/browser/ui/intents/web_**intent_picker_controller.cc:**1108: void > WebIntentPickerController::**CancelDownload() { > CancelExtensionDownload > > Does the picker model need to be updated too? > > http://codereview.chromium.**org/10980002/diff/1/chrome/** > browser/ui/intents/web_intent_**picker_model.cc<http://codereview.chromium.org/10980002/diff/1/chrome/browser/ui/intents/web_intent_picker_model.cc> > File chrome/browser/ui/intents/web_**intent_picker_model.cc (right): > > http://codereview.chromium.**org/10980002/diff/1/chrome/** > browser/ui/intents/web_intent_**picker_model.cc#newcode28<http://codereview.chromium.org/10980002/diff/1/chrome/browser/ui/intents/web_intent_picker_model.cc#newcode28> > chrome/browser/ui/intents/web_**intent_picker_model.cc:28: > pending_install_download_**percent_(0) { > pending_install_download_**percent_ > pending_install_download_** > progress_, > then just document legal values as 00-99 where the field is declared. > > http://codereview.chromium.**org/10980002/diff/1/chrome/** > browser/ui/intents/web_intent_**picker_model.cc#newcode66<http://codereview.chromium.org/10980002/diff/1/chrome/browser/ui/intents/web_intent_picker_model.cc#newcode66> > chrome/browser/ui/intents/web_**intent_picker_model.cc:66: > ClearPendingInstall(); > There are a lot of things at play in the picker. I think you should > qualify all Extension specific fields/methods with the work "Extension". > > ClearPendingExtensionInstall. > > http://codereview.chromium.**org/10980002/<http://codereview.chromium.org/109... >
http://codereview.chromium.org/10980002/diff/1/chrome/browser/ui/intents/web_... File chrome/browser/ui/intents/web_intent_picker_controller.cc (right): http://codereview.chromium.org/10980002/diff/1/chrome/browser/ui/intents/web_... chrome/browser/ui/intents/web_intent_picker_controller.cc:475: picker_model_->SetPendingInstallExtensionId(id); On 2012/09/25 07:15:46, Greg Billock wrote: > Needed? I don't see where this is used. Now used?
On 2012/10/01 20:29:55, smckay wrote: > Any word on my comments Sailesh? (I don't know how you commented, but your comments don't show up on http://codereview.chromium.org/10980002/ as far as I can tell) > > Steve McKay | Engineering | mailto:smckay@google.com | 310-359-8331 > > > > > On Tue, Sep 25, 2012 at 11:56 AM, <mailto:smckay@chromium.org> wrote: > > > > > http://codereview.chromium.**org/10980002/diff/1/chrome/** > > > browser/download/download_**util.cc<http://codereview.chromium.org/10980002/diff/1/chrome/browser/download/download_util.cc> > > File chrome/browser/download/**download_util.cc (right): > > > > http://codereview.chromium.**org/10980002/diff/1/chrome/** > > > browser/download/download_**util.cc#newcode509<http://codereview.chromium.org/10980002/diff/1/chrome/browser/download/download_util.cc#newcode509> > > chrome/browser/download/**download_util.cc:509: > > item->SetUserData(**kShowInShelfKey, new ShowInShelfData(should_show)); > > On 2012/09/25 03:07:18, Nico wrote: > > > >> Can't you just add a bool to DownloadItem? > >> > > > > That'd probably work if you generalize the property to something NOT > > "shelf" related. See my comment in the header file. > > > > Then you could simply revert these changes and inspect the property > > directly. > > > > > > http://codereview.chromium.**org/10980002/diff/1/chrome/** > > > browser/download/download_**util.h<http://codereview.chromium.org/10980002/diff/1/chrome/browser/download/download_util.h> > > File chrome/browser/download/**download_util.h (right): > > > > http://codereview.chromium.**org/10980002/diff/1/chrome/** > > > browser/download/download_**util.h#newcode192<http://codereview.chromium.org/10980002/diff/1/chrome/browser/download/download_util.h#newcode192> > > chrome/browser/download/**download_util.h:192: bool > > GetShouldShowInShelf(content::**DownloadItem* item); > > On 2012/09/25 03:07:18, Nico wrote: > > > >> nit: getshould sounds a bit awkward. just shouldshowinshelf. > >> > > > > A thought about naming. Whether the shelf should show or not is a > > behavior we want based on a property of this download item. So instead > > of naming this WRT the behavior we want, we could name it according to > > the kind of property we need to make the decision. This avoids > > situations in the future where callers need access to the same property > > for non-shelf reasons. > > > > e.g. We could tag the download with "inprocess", "hidden", "system", or > > slightly more specific "intent-data". > > > > HasHiddenProperty(**downloaditem) > > SetHiddenProperty(**downloaditem, bool) > > > > or > > > > HasInProcessProperty(**downloaditem) > > SetInProcessProperty(**downloaditem, bool) > > > > > > http://codereview.chromium.**org/10980002/diff/1/chrome/** > > > browser/extensions/api/**webstore_private/webstore_**private_api.h<http://codereview.chromium.org/10980002/diff/1/chrome/browser/extensions/api/webstore_private/webstore_private_api.h> > > File > > chrome/browser/extensions/api/**webstore_private/webstore_**private_api.h > > (right): > > > > http://codereview.chromium.**org/10980002/diff/1/chrome/** > > browser/extensions/api/**webstore_private/webstore_** > > > private_api.h#newcode162<http://codereview.chromium.org/10980002/diff/1/chrome/browser/extensions/api/webstore_private/webstore_private_api.h#newcode162> > > chrome/browser/extensions/api/**webstore_private/webstore_** > > private_api.h:162: > > bool cancelled) OVERRIDE; > > On 2012/09/25 03:07:18, Nico wrote: > > > >> prefer enums over bool parameters > >> > > > > +1, much more flexible and can aide readability too. > > > > > > http://codereview.chromium.**org/10980002/diff/1/chrome/** > > > browser/extensions/webstore_**installer.cc<http://codereview.chromium.org/10980002/diff/1/chrome/browser/extensions/webstore_installer.cc> > > File chrome/browser/extensions/**webstore_installer.cc (right): > > > > http://codereview.chromium.**org/10980002/diff/1/chrome/** > > > browser/extensions/webstore_**installer.cc#newcode227<http://codereview.chromium.org/10980002/diff/1/chrome/browser/extensions/webstore_installer.cc#newcode227> > > chrome/browser/extensions/**webstore_installer.cc:227: > > ReportFailure(**kInstallCanceledError, true); > > Add a method name ReportCancelled, call that instead. > > > > > > http://codereview.chromium.**org/10980002/diff/1/chrome/** > > > browser/ui/intents/web_intent_**picker_controller.cc<http://codereview.chromium.org/10980002/diff/1/chrome/browser/ui/intents/web_intent_picker_controller.cc> > > File chrome/browser/ui/intents/web_**intent_picker_controller.cc (right): > > > > http://codereview.chromium.**org/10980002/diff/1/chrome/** > > > browser/ui/intents/web_intent_**picker_controller.cc#**newcode545<http://codereview.chromium.org/10980002/diff/1/chrome/browser/ui/intents/web_intent_picker_controller.cc#newcode545> > > chrome/browser/ui/intents/web_**intent_picker_controller.cc:**545: > > download_util::**SetShouldShowInShelf(item, false); > > On 2012/09/25 03:07:18, Nico wrote: > > > >> From what I understand, this relies on this notification running > >> > > before the > > > >> "download started" notification, so that this flag is set before the > >> > > shelf > > > >> decides if it needs to do anything. Is that right? Is that enforced > >> > > somehow? > > > > What if we flipped the perspective a bit and started all downloads as > > "system" download, then promote them to "user" downloads once we know > > the download is actually a user "download" and not the system loading > > the data for some other purpose. > > > > http://codereview.chromium.**org/10980002/diff/1/chrome/** > > > browser/ui/intents/web_intent_**picker_controller.cc#**newcode1100<http://codereview.chromium.org/10980002/diff/1/chrome/browser/ui/intents/web_intent_picker_controller.cc#newcode1100> > > chrome/browser/ui/intents/web_**intent_picker_controller.cc:**1100: void > > WebIntentPickerController::**UpdateDownloadState( > > Feels like this entire method should be pushed into the picker > > model...and named UpdateExtensionDownloadState. > > > > http://codereview.chromium.**org/10980002/diff/1/chrome/** > > > browser/ui/intents/web_intent_**picker_controller.cc#**newcode1108<http://codereview.chromium.org/10980002/diff/1/chrome/browser/ui/intents/web_intent_picker_controller.cc#newcode1108> > > chrome/browser/ui/intents/web_**intent_picker_controller.cc:**1108: void > > WebIntentPickerController::**CancelDownload() { > > CancelExtensionDownload > > > > Does the picker model need to be updated too? > > > > http://codereview.chromium.**org/10980002/diff/1/chrome/** > > > browser/ui/intents/web_intent_**picker_model.cc<http://codereview.chromium.org/10980002/diff/1/chrome/browser/ui/intents/web_intent_picker_model.cc> > > File chrome/browser/ui/intents/web_**intent_picker_model.cc (right): > > > > http://codereview.chromium.**org/10980002/diff/1/chrome/** > > > browser/ui/intents/web_intent_**picker_model.cc#newcode28<http://codereview.chromium.org/10980002/diff/1/chrome/browser/ui/intents/web_intent_picker_model.cc#newcode28> > > chrome/browser/ui/intents/web_**intent_picker_model.cc:28: > > pending_install_download_**percent_(0) { > > pending_install_download_**percent_ > pending_install_download_** > > progress_, > > then just document legal values as 00-99 where the field is declared. > > > > http://codereview.chromium.**org/10980002/diff/1/chrome/** > > > browser/ui/intents/web_intent_**picker_model.cc#newcode66<http://codereview.chromium.org/10980002/diff/1/chrome/browser/ui/intents/web_intent_picker_model.cc#newcode66> > > chrome/browser/ui/intents/web_**intent_picker_model.cc:66: > > ClearPendingInstall(); > > There are a lot of things at play in the picker. I think you should > > qualify all Extension specific fields/methods with the work "Extension". > > > > ClearPendingExtensionInstall. > > > > > http://codereview.chromium.**org/10980002/%3Chttp://codereview.chromium.org/1...> > >
(I don't know how you commented, but your comments don't show up on http://codereview.chromium.**org/10980002/<http://codereview.chromium.org/109... as far as I can tell) That's really weird. I received a message from codereview with my comments. It was CC'd to everyone, but I don't see them either in the list of comments on the review :| Should be in your email history too. I can send along a copy if needed. Steve McKay | Engineering | smckay@google.com | 310-359-8331 On Mon, Oct 1, 2012 at 8:29 PM, <thakis@chromium.org> wrote: > On 2012/10/01 20:29:55, smckay wrote: > >> Any word on my comments Sailesh? >> > > (I don't know how you commented, but your comments don't show up on > http://codereview.chromium.**org/10980002/<http://codereview.chromium.org/109... far as I can tell) > > > Steve McKay | Engineering | mailto:smckay@google.com | 310-359-8331 >> > > > > > On Tue, Sep 25, 2012 at 11:56 AM, <mailto:smckay@chromium.org> wrote: >> > > > >> > http://codereview.chromium.****org/10980002/diff/1/chrome/** >> > >> > > browser/download/download_****util.cc<http://codereview.** > chromium.org/10980002/diff/1/**chrome/browser/download/**download_util.cc<http://codereview.chromium.org/10980002/diff/1/chrome/browser/download/download_util.cc> > > > >> > File chrome/browser/download/****download_util.cc (right): >> > >> > http://codereview.chromium.****org/10980002/diff/1/chrome/** >> > >> > > browser/download/download_****util.cc#newcode509<http://** > codereview.chromium.org/**10980002/diff/1/chrome/** > browser/download/download_**util.cc#newcode509<http://codereview.chromium.org/10980002/diff/1/chrome/browser/download/download_util.cc#newcode509> > > > >> > chrome/browser/download/****download_util.cc:509: >> > item->SetUserData(****kShowInShelfKey, new >> ShowInShelfData(should_show)); >> >> > On 2012/09/25 03:07:18, Nico wrote: >> > >> >> Can't you just add a bool to DownloadItem? >> >> >> > >> > That'd probably work if you generalize the property to something NOT >> > "shelf" related. See my comment in the header file. >> > >> > Then you could simply revert these changes and inspect the property >> > directly. >> > >> > >> > http://codereview.chromium.****org/10980002/diff/1/chrome/** >> > >> > > browser/download/download_****util.h<http://codereview.** > chromium.org/10980002/diff/1/**chrome/browser/download/**download_util.h<http://codereview.chromium.org/10980002/diff/1/chrome/browser/download/download_util.h> > > > >> > File chrome/browser/download/****download_util.h (right): >> > >> > http://codereview.chromium.****org/10980002/diff/1/chrome/** >> > >> > > browser/download/download_****util.h#newcode192<http://** > codereview.chromium.org/**10980002/diff/1/chrome/** > browser/download/download_**util.h#newcode192<http://codereview.chromium.org/10980002/diff/1/chrome/browser/download/download_util.h#newcode192> > > > >> > chrome/browser/download/****download_util.h:192: bool >> > GetShouldShowInShelf(content::****DownloadItem* item); >> >> > On 2012/09/25 03:07:18, Nico wrote: >> > >> >> nit: getshould sounds a bit awkward. just shouldshowinshelf. >> >> >> > >> > A thought about naming. Whether the shelf should show or not is a >> > behavior we want based on a property of this download item. So instead >> > of naming this WRT the behavior we want, we could name it according to >> > the kind of property we need to make the decision. This avoids >> > situations in the future where callers need access to the same property >> > for non-shelf reasons. >> > >> > e.g. We could tag the download with "inprocess", "hidden", "system", or >> > slightly more specific "intent-data". >> > >> > HasHiddenProperty(****downloaditem) >> > SetHiddenProperty(****downloaditem, bool) >> > >> > or >> > >> > HasInProcessProperty(****downloaditem) >> > SetInProcessProperty(****downloaditem, bool) >> > >> > >> > http://codereview.chromium.****org/10980002/diff/1/chrome/** >> > >> > > browser/extensions/api/****webstore_private/webstore_****private_api.h< > http://**codereview.chromium.org/**10980002/diff/1/chrome/** > browser/extensions/api/**webstore_private/webstore_**private_api.h<http://codereview.chromium.org/10980002/diff/1/chrome/browser/extensions/api/webstore_private/webstore_private_api.h> > > > >> > File >> > chrome/browser/extensions/api/****webstore_private/webstore_**** >> private_api.h >> > (right): >> > >> > http://codereview.chromium.****org/10980002/diff/1/chrome/** >> > browser/extensions/api/****webstore_private/webstore_** >> > >> > > private_api.h#newcode162<http:**//codereview.chromium.org/** > 10980002/diff/1/chrome/**browser/extensions/api/** > webstore_private/webstore_**private_api.h#newcode162<http://codereview.chromium.org/10980002/diff/1/chrome/browser/extensions/api/webstore_private/webstore_private_api.h#newcode162> > > > >> > chrome/browser/extensions/api/****webstore_private/webstore_** >> >> > private_api.h:162: >> > bool cancelled) OVERRIDE; >> > On 2012/09/25 03:07:18, Nico wrote: >> > >> >> prefer enums over bool parameters >> >> >> > >> > +1, much more flexible and can aide readability too. >> > >> > >> > http://codereview.chromium.****org/10980002/diff/1/chrome/** >> > >> > > browser/extensions/webstore_****installer.cc<http://** > codereview.chromium.org/**10980002/diff/1/chrome/** > browser/extensions/webstore_**installer.cc<http://codereview.chromium.org/10980002/diff/1/chrome/browser/extensions/webstore_installer.cc> > > > >> > File chrome/browser/extensions/****webstore_installer.cc (right): >> > >> > http://codereview.chromium.****org/10980002/diff/1/chrome/** >> > >> > > browser/extensions/webstore_****installer.cc#newcode227<http:/** > /codereview.chromium.org/**10980002/diff/1/chrome/** > browser/extensions/webstore_**installer.cc#newcode227<http://codereview.chromium.org/10980002/diff/1/chrome/browser/extensions/webstore_installer.cc#newcode227> > > > >> > chrome/browser/extensions/****webstore_installer.cc:227: >> > ReportFailure(****kInstallCanceledError, true); >> >> > Add a method name ReportCancelled, call that instead. >> > >> > >> > http://codereview.chromium.****org/10980002/diff/1/chrome/** >> > >> > > browser/ui/intents/web_intent_****picker_controller.cc<http://** > codereview.chromium.org/**10980002/diff/1/chrome/** > browser/ui/intents/web_intent_**picker_controller.cc<http://codereview.chromium.org/10980002/diff/1/chrome/browser/ui/intents/web_intent_picker_controller.cc> > > > >> > File chrome/browser/ui/intents/web_****intent_picker_controller.cc >> (right): >> > >> > http://codereview.chromium.****org/10980002/diff/1/chrome/** >> > >> > > browser/ui/intents/web_intent_****picker_controller.cc#****newcode545< > http://codereview.**chromium.org/10980002/diff/1/** > chrome/browser/ui/intents/web_**intent_picker_controller.cc#**newcode545<http://codereview.chromium.org/10980002/diff/1/chrome/browser/ui/intents/web_intent_picker_controller.cc#newcode545> > > > >> > chrome/browser/ui/intents/web_****intent_picker_controller.cc:****545: >> > download_util::****SetShouldShowInShelf(item, false); >> >> > On 2012/09/25 03:07:18, Nico wrote: >> > >> >> From what I understand, this relies on this notification running >> >> >> > before the >> > >> >> "download started" notification, so that this flag is set before the >> >> >> > shelf >> > >> >> decides if it needs to do anything. Is that right? Is that enforced >> >> >> > somehow? >> > >> > What if we flipped the perspective a bit and started all downloads as >> > "system" download, then promote them to "user" downloads once we know >> > the download is actually a user "download" and not the system loading >> > the data for some other purpose. >> > >> > http://codereview.chromium.****org/10980002/diff/1/chrome/** >> > >> > > browser/ui/intents/web_intent_****picker_controller.cc#****newcode1100< > http://codereview.**chromium.org/10980002/diff/1/** > chrome/browser/ui/intents/web_**intent_picker_controller.cc#**newcode1100<http://codereview.chromium.org/10980002/diff/1/chrome/browser/ui/intents/web_intent_picker_controller.cc#newcode1100> > > > >> > chrome/browser/ui/intents/web_****intent_picker_controller.cc:****1100: >> void >> > WebIntentPickerController::****UpdateDownloadState( >> >> > Feels like this entire method should be pushed into the picker >> > model...and named UpdateExtensionDownloadState. >> > >> > http://codereview.chromium.****org/10980002/diff/1/chrome/** >> > >> > > browser/ui/intents/web_intent_****picker_controller.cc#****newcode1108< > http://codereview.**chromium.org/10980002/diff/1/** > chrome/browser/ui/intents/web_**intent_picker_controller.cc#**newcode1108<http://codereview.chromium.org/10980002/diff/1/chrome/browser/ui/intents/web_intent_picker_controller.cc#newcode1108> > > > >> > chrome/browser/ui/intents/web_****intent_picker_controller.cc:****1108: >> void >> > WebIntentPickerController::****CancelDownload() { >> >> > CancelExtensionDownload >> > >> > Does the picker model need to be updated too? >> > >> > http://codereview.chromium.****org/10980002/diff/1/chrome/** >> > >> > > browser/ui/intents/web_intent_****picker_model.cc<http://** > codereview.chromium.org/**10980002/diff/1/chrome/** > browser/ui/intents/web_intent_**picker_model.cc<http://codereview.chromium.org/10980002/diff/1/chrome/browser/ui/intents/web_intent_picker_model.cc> > > > >> > File chrome/browser/ui/intents/web_****intent_picker_model.cc (right): >> > >> > http://codereview.chromium.****org/10980002/diff/1/chrome/** >> > >> > > browser/ui/intents/web_intent_****picker_model.cc#newcode28<ht** > tp://codereview.chromium.org/**10980002/diff/1/chrome/** > browser/ui/intents/web_intent_**picker_model.cc#newcode28<http://codereview.chromium.org/10980002/diff/1/chrome/browser/ui/intents/web_intent_picker_model.cc#newcode28> > > > >> > chrome/browser/ui/intents/web_****intent_picker_model.cc:28: >> > pending_install_download_****percent_(0) { >> > pending_install_download_****percent_ > pending_install_download_** >> >> > progress_, >> > then just document legal values as 00-99 where the field is declared. >> > >> > http://codereview.chromium.****org/10980002/diff/1/chrome/** >> > >> > > browser/ui/intents/web_intent_****picker_model.cc#newcode66<ht** > tp://codereview.chromium.org/**10980002/diff/1/chrome/** > browser/ui/intents/web_intent_**picker_model.cc#newcode66<http://codereview.chromium.org/10980002/diff/1/chrome/browser/ui/intents/web_intent_picker_model.cc#newcode66> > > > >> > chrome/browser/ui/intents/web_****intent_picker_model.cc:66: >> >> > ClearPendingInstall(); >> > There are a lot of things at play in the picker. I think you should >> > qualify all Extension specific fields/methods with the work "Extension". >> > >> > ClearPendingExtensionInstall. >> > >> > >> > > http://codereview.chromium.****org/10980002/%3Chttp://coderev** > iew.chromium.org/10980002/ <http://codereview.chromium.org/10980002/>> > >> > >> > > > http://codereview.chromium.**org/10980002/<http://codereview.chromium.org/109... >
Addressed all review comments. The only thing missing is a browser test for the new DownloadItem field. I'm working on adding that right now. https://codereview.chromium.org/10980002/diff/1/chrome/browser/download/downl... File chrome/browser/download/download_util.cc (right): https://codereview.chromium.org/10980002/diff/1/chrome/browser/download/downl... chrome/browser/download/download_util.cc:504: static_cast<ShowInShelfData*>(item->GetUserData(kShowInShelfKey)); On 2012/09/25 07:15:46, Greg Billock wrote: > Do we not have a type token wrapper for getting data out of one of these by > type? Done. Removed code. https://codereview.chromium.org/10980002/diff/1/chrome/browser/download/downl... chrome/browser/download/download_util.cc:509: item->SetUserData(kShowInShelfKey, new ShowInShelfData(should_show)); On 2012/09/25 03:07:18, Nico wrote: > Can't you just add a bool to DownloadItem? Done. Added DownloadItem::ShouldShowInDownloadsUI() https://codereview.chromium.org/10980002/diff/1/chrome/browser/download/downl... File chrome/browser/download/download_util.h (right): https://codereview.chromium.org/10980002/diff/1/chrome/browser/download/downl... chrome/browser/download/download_util.h:192: bool GetShouldShowInShelf(content::DownloadItem* item); On 2012/09/25 03:07:18, Nico wrote: > nit: getshould sounds a bit awkward. just shouldshowinshelf. Done. Removed code. https://codereview.chromium.org/10980002/diff/1/chrome/browser/download/downl... chrome/browser/download/download_util.h:192: bool GetShouldShowInShelf(content::DownloadItem* item); On 2012/09/25 18:56:04, Steve McKay wrote: > On 2012/09/25 03:07:18, Nico wrote: > > nit: getshould sounds a bit awkward. just shouldshowinshelf. > > A thought about naming. Whether the shelf should show or not is a behavior we > want based on a property of this download item. So instead of naming this WRT > the behavior we want, we could name it according to the kind of property we need > to make the decision. This avoids situations in the future where callers need > access to the same property for non-shelf reasons. > > e.g. We could tag the download with "inprocess", "hidden", "system", or slightly > more specific "intent-data". > > HasHiddenProperty(downloaditem) > SetHiddenProperty(downloaditem, bool) > > or > > HasInProcessProperty(downloaditem) > SetInProcessProperty(downloaditem, bool) I added a DownloadItem::ShouldShowInDownloadsUI() field. It sounds like there's more refactoring coming soon so maybe it's not worth adding extra properties here. Let me know if you think I should change this. Thanks https://codereview.chromium.org/10980002/diff/1/chrome/browser/extensions/api... File chrome/browser/extensions/api/webstore_private/webstore_private_api.h (right): https://codereview.chromium.org/10980002/diff/1/chrome/browser/extensions/api... chrome/browser/extensions/api/webstore_private/webstore_private_api.h:162: bool cancelled) OVERRIDE; On 2012/09/25 03:07:18, Nico wrote: > prefer enums over bool parameters Done. Changed to WebstoreInstaller::FailureReason https://codereview.chromium.org/10980002/diff/1/chrome/browser/extensions/web... File chrome/browser/extensions/webstore_installer.cc (right): https://codereview.chromium.org/10980002/diff/1/chrome/browser/extensions/web... chrome/browser/extensions/webstore_installer.cc:227: ReportFailure(kInstallCanceledError, true); On 2012/09/25 18:56:04, Steve McKay wrote: > Add a method name ReportCancelled, call that instead. I changed ReportFailure to take a enum. Do you still want me to add a ReportCancelled function? https://codereview.chromium.org/10980002/diff/1/chrome/browser/extensions/web... chrome/browser/extensions/webstore_installer.cc:366: delegate_->OnExtensionInstallFailure(id_, error, cancelled); On 2012/09/25 03:07:18, Nico wrote: > nit: It looks like cancelled is true iff error == kInstallCanceledError -- is > that right? can you check for that instead? (not sure if that's better) I changed this to take an enum. error is a string that can come from other places too (net::Error, etc...). https://codereview.chromium.org/10980002/diff/1/chrome/browser/ui/intents/web... File chrome/browser/ui/intents/web_intent_picker_controller.cc (right): https://codereview.chromium.org/10980002/diff/1/chrome/browser/ui/intents/web... chrome/browser/ui/intents/web_intent_picker_controller.cc:475: picker_model_->SetPendingInstallExtensionId(id); On 2012/10/02 03:02:29, Greg Billock wrote: > On 2012/09/25 07:15:46, Greg Billock wrote: > > Needed? I don't see where this is used. > > Now used? Yea, used here: http://codereview.chromium.org/11009017/diff/1015/chrome/browser/ui/cocoa/int... https://codereview.chromium.org/10980002/diff/1/chrome/browser/ui/intents/web... chrome/browser/ui/intents/web_intent_picker_controller.cc:575: if (picker_) On 2012/09/25 22:20:56, groby wrote: > nit: This should never be NULL. Can we just DCHECK(picker_)? Unfortunately there's this function is called asynchronously and the picker my be set to NULL in the meantime. https://codereview.chromium.org/10980002/diff/1/chrome/browser/ui/intents/web... chrome/browser/ui/intents/web_intent_picker_controller.cc:602: CancelDownload(); On 2012/09/25 16:29:30, asanka wrote: > Why call CancelDownload() here? For example, if we weren't able to unpack the extension this would be called but the download would be still marked as pending. We need to explicitly cancel here. https://codereview.chromium.org/10980002/diff/1/chrome/browser/ui/intents/web... chrome/browser/ui/intents/web_intent_picker_controller.cc:603: // If the user cancelled the install then don't show an error message. On 2012/09/25 03:07:18, Nico wrote: > nit: canceled Weird, it looks like we use both in chrome/*: dhcp-172-19-0-37:chrome sail$ grep -ris canceled * | wc -l 583 dhcp-172-19-0-37:chrome sail$ grep -ris cancelled * | wc -l 896 intents/* only uses cancelled so keeping that. https://codereview.chromium.org/10980002/diff/1/chrome/browser/ui/intents/web... chrome/browser/ui/intents/web_intent_picker_controller.cc:1100: void WebIntentPickerController::UpdateDownloadState( On 2012/09/25 18:56:04, Steve McKay wrote: > Feels like this entire method should be pushed into the picker model...and named > UpdateExtensionDownloadState. Done. https://codereview.chromium.org/10980002/diff/1/chrome/browser/ui/intents/web... chrome/browser/ui/intents/web_intent_picker_controller.cc:1108: void WebIntentPickerController::CancelDownload() { On 2012/09/25 18:56:04, Steve McKay wrote: > CancelExtensionDownload > > Does the picker model need to be updated too? Done. This used to be in the caller code. Moved to here. https://codereview.chromium.org/10980002/diff/1/chrome/browser/ui/intents/web... File chrome/browser/ui/intents/web_intent_picker_model.cc (right): https://codereview.chromium.org/10980002/diff/1/chrome/browser/ui/intents/web... chrome/browser/ui/intents/web_intent_picker_model.cc:28: pending_install_download_percent_(0) { On 2012/09/25 18:56:04, Steve McKay wrote: > pending_install_download_percent_ > pending_install_download_progress_, then > just document legal values as 00-99 where the field is declared. Done. https://codereview.chromium.org/10980002/diff/1/chrome/browser/ui/intents/web... chrome/browser/ui/intents/web_intent_picker_model.cc:66: ClearPendingInstall(); On 2012/09/25 18:56:04, Steve McKay wrote: > There are a lot of things at play in the picker. I think you should qualify all > Extension specific fields/methods with the work "Extension". > > ClearPendingExtensionInstall. Done. Updated all the other fields as well. https://codereview.chromium.org/10980002/diff/1/chrome/browser/ui/intents/web... chrome/browser/ui/intents/web_intent_picker_model.cc:174: observer_->OnModelChanged(this); On 2012/09/25 22:20:56, groby wrote: > Pre your refactor, OnModelChanged used to trigger a performLayout, which used to > be a bit extensive. (I.e. visual artifacts). Is that fixed? Yea, it's really cheap now and I don't see any visual artifacts. On my MacBook Pro extra layouts (that don't change the content) take almost no time (rounded to 0 miliseconds).
On 2012/10/02 18:57:01, sail wrote: > Addressed all review comments. > The only thing missing is a browser test for the new DownloadItem field. I'm > working on adding that right now. > > https://codereview.chromium.org/10980002/diff/1/chrome/browser/download/downl... > File chrome/browser/download/download_util.cc (right): > > https://codereview.chromium.org/10980002/diff/1/chrome/browser/download/downl... > chrome/browser/download/download_util.cc:504: > static_cast<ShowInShelfData*>(item->GetUserData(kShowInShelfKey)); > On 2012/09/25 07:15:46, Greg Billock wrote: > > Do we not have a type token wrapper for getting data out of one of these by > > type? > > Done. > Removed code. > > https://codereview.chromium.org/10980002/diff/1/chrome/browser/download/downl... > chrome/browser/download/download_util.cc:509: item->SetUserData(kShowInShelfKey, > new ShowInShelfData(should_show)); > On 2012/09/25 03:07:18, Nico wrote: > > Can't you just add a bool to DownloadItem? > > Done. > Added DownloadItem::ShouldShowInDownloadsUI() > > https://codereview.chromium.org/10980002/diff/1/chrome/browser/download/downl... > File chrome/browser/download/download_util.h (right): > > https://codereview.chromium.org/10980002/diff/1/chrome/browser/download/downl... > chrome/browser/download/download_util.h:192: bool > GetShouldShowInShelf(content::DownloadItem* item); > On 2012/09/25 03:07:18, Nico wrote: > > nit: getshould sounds a bit awkward. just shouldshowinshelf. > > Done. > Removed code. > > https://codereview.chromium.org/10980002/diff/1/chrome/browser/download/downl... > chrome/browser/download/download_util.h:192: bool > GetShouldShowInShelf(content::DownloadItem* item); > On 2012/09/25 18:56:04, Steve McKay wrote: > > On 2012/09/25 03:07:18, Nico wrote: > > > nit: getshould sounds a bit awkward. just shouldshowinshelf. > > > > A thought about naming. Whether the shelf should show or not is a behavior we > > want based on a property of this download item. So instead of naming this WRT > > the behavior we want, we could name it according to the kind of property we > need > > to make the decision. This avoids situations in the future where callers need > > access to the same property for non-shelf reasons. > > > > e.g. We could tag the download with "inprocess", "hidden", "system", or > slightly > > more specific "intent-data". > > > > HasHiddenProperty(downloaditem) > > SetHiddenProperty(downloaditem, bool) > > > > or > > > > HasInProcessProperty(downloaditem) > > SetInProcessProperty(downloaditem, bool) > > I added a DownloadItem::ShouldShowInDownloadsUI() field. It sounds like there's > more refactoring coming soon so maybe it's not worth adding extra properties > here. Let me know if you think I should change this. Thanks > > https://codereview.chromium.org/10980002/diff/1/chrome/browser/extensions/api... > File chrome/browser/extensions/api/webstore_private/webstore_private_api.h > (right): > > https://codereview.chromium.org/10980002/diff/1/chrome/browser/extensions/api... > chrome/browser/extensions/api/webstore_private/webstore_private_api.h:162: bool > cancelled) OVERRIDE; > On 2012/09/25 03:07:18, Nico wrote: > > prefer enums over bool parameters > > Done. > Changed to WebstoreInstaller::FailureReason > > https://codereview.chromium.org/10980002/diff/1/chrome/browser/extensions/web... > File chrome/browser/extensions/webstore_installer.cc (right): > > https://codereview.chromium.org/10980002/diff/1/chrome/browser/extensions/web... > chrome/browser/extensions/webstore_installer.cc:227: > ReportFailure(kInstallCanceledError, true); > On 2012/09/25 18:56:04, Steve McKay wrote: > > Add a method name ReportCancelled, call that instead. > > I changed ReportFailure to take a enum. Do you still want me to add a > ReportCancelled function? > > https://codereview.chromium.org/10980002/diff/1/chrome/browser/extensions/web... > chrome/browser/extensions/webstore_installer.cc:366: > delegate_->OnExtensionInstallFailure(id_, error, cancelled); > On 2012/09/25 03:07:18, Nico wrote: > > nit: It looks like cancelled is true iff error == kInstallCanceledError -- is > > that right? can you check for that instead? (not sure if that's better) > > I changed this to take an enum. > > error is a string that can come from other places too (net::Error, etc...). > > https://codereview.chromium.org/10980002/diff/1/chrome/browser/ui/intents/web... > File chrome/browser/ui/intents/web_intent_picker_controller.cc (right): > > https://codereview.chromium.org/10980002/diff/1/chrome/browser/ui/intents/web... > chrome/browser/ui/intents/web_intent_picker_controller.cc:475: > picker_model_->SetPendingInstallExtensionId(id); > On 2012/10/02 03:02:29, Greg Billock wrote: > > On 2012/09/25 07:15:46, Greg Billock wrote: > > > Needed? I don't see where this is used. > > > > Now used? > > Yea, used here: > http://codereview.chromium.org/11009017/diff/1015/chrome/browser/ui/cocoa/int... > > https://codereview.chromium.org/10980002/diff/1/chrome/browser/ui/intents/web... > chrome/browser/ui/intents/web_intent_picker_controller.cc:575: if (picker_) > On 2012/09/25 22:20:56, groby wrote: > > nit: This should never be NULL. Can we just DCHECK(picker_)? > > Unfortunately there's this function is called asynchronously and the picker my > be set to NULL in the meantime. > > https://codereview.chromium.org/10980002/diff/1/chrome/browser/ui/intents/web... > chrome/browser/ui/intents/web_intent_picker_controller.cc:602: CancelDownload(); > On 2012/09/25 16:29:30, asanka wrote: > > Why call CancelDownload() here? > > For example, if we weren't able to unpack the extension this would be called but > the download would be still marked as pending. We need to explicitly cancel > here. > > https://codereview.chromium.org/10980002/diff/1/chrome/browser/ui/intents/web... > chrome/browser/ui/intents/web_intent_picker_controller.cc:603: // If the user > cancelled the install then don't show an error message. > On 2012/09/25 03:07:18, Nico wrote: > > nit: canceled > > Weird, it looks like we use both in chrome/*: > dhcp-172-19-0-37:chrome sail$ grep -ris canceled * | wc -l > 583 > dhcp-172-19-0-37:chrome sail$ grep -ris cancelled * | wc -l > 896 > > intents/* only uses cancelled so keeping that. > > https://codereview.chromium.org/10980002/diff/1/chrome/browser/ui/intents/web... > chrome/browser/ui/intents/web_intent_picker_controller.cc:1100: void > WebIntentPickerController::UpdateDownloadState( > On 2012/09/25 18:56:04, Steve McKay wrote: > > Feels like this entire method should be pushed into the picker model...and > named > > UpdateExtensionDownloadState. > > Done. > > https://codereview.chromium.org/10980002/diff/1/chrome/browser/ui/intents/web... > chrome/browser/ui/intents/web_intent_picker_controller.cc:1108: void > WebIntentPickerController::CancelDownload() { > On 2012/09/25 18:56:04, Steve McKay wrote: > > CancelExtensionDownload > > > > Does the picker model need to be updated too? > > Done. > This used to be in the caller code. Moved to here. > > https://codereview.chromium.org/10980002/diff/1/chrome/browser/ui/intents/web... > File chrome/browser/ui/intents/web_intent_picker_model.cc (right): > > https://codereview.chromium.org/10980002/diff/1/chrome/browser/ui/intents/web... > chrome/browser/ui/intents/web_intent_picker_model.cc:28: > pending_install_download_percent_(0) { > On 2012/09/25 18:56:04, Steve McKay wrote: > > pending_install_download_percent_ > pending_install_download_progress_, then > > just document legal values as 00-99 where the field is declared. > > Done. > > https://codereview.chromium.org/10980002/diff/1/chrome/browser/ui/intents/web... > chrome/browser/ui/intents/web_intent_picker_model.cc:66: ClearPendingInstall(); > On 2012/09/25 18:56:04, Steve McKay wrote: > > There are a lot of things at play in the picker. I think you should qualify > all > > Extension specific fields/methods with the work "Extension". > > > > ClearPendingExtensionInstall. > > Done. > Updated all the other fields as well. > > https://codereview.chromium.org/10980002/diff/1/chrome/browser/ui/intents/web... > chrome/browser/ui/intents/web_intent_picker_model.cc:174: > observer_->OnModelChanged(this); > On 2012/09/25 22:20:56, groby wrote: > > Pre your refactor, OnModelChanged used to trigger a performLayout, which used > to > > be a bit extensive. (I.e. visual artifacts). Is that fixed? > > Yea, it's really cheap now and I don't see any visual artifacts. On my MacBook > Pro extra layouts (that don't change the content) take almost no time (rounded > to 0 miliseconds). +mihaip. Mihai, could you take a look for extensions/OWNERS? Thanks!
+sky. Scott, can you take a look for content/public/test/OWNERS? Thanks!
http://codereview.chromium.org/10980002/diff/20001/chrome/browser/ui/browser.cc File chrome/browser/ui/browser.cc (right): http://codereview.chromium.org/10980002/diff/20001/chrome/browser/ui/browser.... chrome/browser/ui/browser.cc:1433: fprintf(stderr, "%s skipping\n", __PRETTY_FUNCTION__); Take out printfs. http://codereview.chromium.org/10980002/diff/20001/chrome/browser/ui/intents/... File chrome/browser/ui/intents/web_intent_picker_model.cc (right): http://codereview.chromium.org/10980002/diff/20001/chrome/browser/ui/intents/... chrome/browser/ui/intents/web_intent_picker_model.cc:177: pending_extension_install_status_string_ = download_model.GetStatusText(); Need to call OnModelChanged.
http://codereview.chromium.org/10980002/diff/20001/chrome/browser/ui/browser.cc File chrome/browser/ui/browser.cc (right): http://codereview.chromium.org/10980002/diff/20001/chrome/browser/ui/browser.... chrome/browser/ui/browser.cc:1433: fprintf(stderr, "%s skipping\n", __PRETTY_FUNCTION__); On 2012/10/02 19:12:20, Greg Billock wrote: > Take out printfs. Done. Oops, removed. http://codereview.chromium.org/10980002/diff/20001/chrome/browser/ui/intents/... File chrome/browser/ui/intents/web_intent_picker_model.cc (right): http://codereview.chromium.org/10980002/diff/20001/chrome/browser/ui/intents/... chrome/browser/ui/intents/web_intent_picker_model.cc:177: pending_extension_install_status_string_ = download_model.GetStatusText(); On 2012/10/02 19:12:20, Greg Billock wrote: > Need to call OnModelChanged. Done.
FYI https://codereview.chromium.org/10980002/diff/1/chrome/browser/download/downl... File chrome/browser/download/download_util.h (right): https://codereview.chromium.org/10980002/diff/1/chrome/browser/download/downl... chrome/browser/download/download_util.h:192: bool GetShouldShowInShelf(content::DownloadItem* item); On 2012/10/02 18:57:01, sail wrote: > On 2012/09/25 18:56:04, Steve McKay wrote: > > On 2012/09/25 03:07:18, Nico wrote: > > > nit: getshould sounds a bit awkward. just shouldshowinshelf. > > > > A thought about naming. Whether the shelf should show or not is a behavior we > > want based on a property of this download item. So instead of naming this WRT > > the behavior we want, we could name it according to the kind of property we > need > > to make the decision. This avoids situations in the future where callers need > > access to the same property for non-shelf reasons. > > > > e.g. We could tag the download with "inprocess", "hidden", "system", or > slightly > > more specific "intent-data". > > > > HasHiddenProperty(downloaditem) > > SetHiddenProperty(downloaditem, bool) > > > > or > > > > HasInProcessProperty(downloaditem) > > SetInProcessProperty(downloaditem, bool) > > I added a DownloadItem::ShouldShowInDownloadsUI() field. It sounds like there's > more refactoring coming soon so maybe it's not worth adding extra properties > here. Let me know if you think I should change this. Thanks You missed my point about finding the intrinsic property behind the immediate need. If you name it "ShouldShowInDownloadsUI" other callers won't be able to depend on the method...without it being a hack. If you name these according to the intrinsic property ("IsHiddenDownload"), it's instantly shareable across multiple uses. Think API, not "make it work for what I need". Does that make sense?
Some of the comment threads are long here. It is my belief that all comments are addressed at this point. http://codereview.chromium.org/10980002/diff/1/chrome/browser/extensions/webs... File chrome/browser/extensions/webstore_installer.cc (right): http://codereview.chromium.org/10980002/diff/1/chrome/browser/extensions/webs... chrome/browser/extensions/webstore_installer.cc:227: ReportFailure(kInstallCanceledError, true); This is fine. On 2012/10/02 18:57:01, sail wrote: > On 2012/09/25 18:56:04, Steve McKay wrote: > > Add a method name ReportCancelled, call that instead. > > I changed ReportFailure to take a enum. Do you still want me to add a > ReportCancelled function? http://codereview.chromium.org/10980002/diff/1/chrome/browser/ui/intents/web_... File chrome/browser/ui/intents/web_intent_picker_controller.cc (right): http://codereview.chromium.org/10980002/diff/1/chrome/browser/ui/intents/web_... chrome/browser/ui/intents/web_intent_picker_controller.cc:545: download_util::SetShouldShowInShelf(item, false); When we use a separate RequestHandler for intents, we'll basically have that. That'll likely be able to make use of this machinery, though (the show-in-shelf flag is great). Thanks, asanka, for the clarification. On 2012/09/25 18:56:04, Steve McKay wrote: > On 2012/09/25 03:07:18, Nico wrote: > > From what I understand, this relies on this notification running before the > > "download started" notification, so that this flag is set before the shelf > > decides if it needs to do anything. Is that right? Is that enforced somehow? > > What if we flipped the perspective a bit and started all downloads as "system" > download, then promote them to "user" downloads once we know the download is > actually a user "download" and not the system loading the data for some other > purpose.
lgtm still need a browser test that setting the downloads ui flag gets honored in the browser and doesn't show. Sailesh is adding.
LGTM
Added a test. https://codereview.chromium.org/10980002/diff/1/chrome/browser/download/downl... File chrome/browser/download/download_util.h (right): https://codereview.chromium.org/10980002/diff/1/chrome/browser/download/downl... chrome/browser/download/download_util.h:192: bool GetShouldShowInShelf(content::DownloadItem* item); On 2012/10/02 19:19:22, Steve McKay wrote: > On 2012/10/02 18:57:01, sail wrote: > > On 2012/09/25 18:56:04, Steve McKay wrote: > > > On 2012/09/25 03:07:18, Nico wrote: > > > > nit: getshould sounds a bit awkward. just shouldshowinshelf. > > > > > > A thought about naming. Whether the shelf should show or not is a behavior > we > > > want based on a property of this download item. So instead of naming this > WRT > > > the behavior we want, we could name it according to the kind of property we > > need > > > to make the decision. This avoids situations in the future where callers > need > > > access to the same property for non-shelf reasons. > > > > > > e.g. We could tag the download with "inprocess", "hidden", "system", or > > slightly > > > more specific "intent-data". > > > > > > HasHiddenProperty(downloaditem) > > > SetHiddenProperty(downloaditem, bool) > > > > > > or > > > > > > HasInProcessProperty(downloaditem) > > > SetInProcessProperty(downloaditem, bool) > > > > I added a DownloadItem::ShouldShowInDownloadsUI() field. It sounds like > there's > > more refactoring coming soon so maybe it's not worth adding extra properties > > here. Let me know if you think I should change this. Thanks > > You missed my point about finding the intrinsic property behind the immediate > need. If you name it "ShouldShowInDownloadsUI" other callers won't be able to > depend on the method...without it being a hack. If you name these according to > the intrinsic property ("IsHiddenDownload"), it's instantly shareable across > multiple uses. Think API, not "make it work for what I need". > > Does that make sense? Sounds good. Renamed to DownloadItem::SetIsHiddenDownload(). https://codereview.chromium.org/10980002/diff/1/chrome/browser/ui/browser.cc File chrome/browser/ui/browser.cc (right): https://codereview.chromium.org/10980002/diff/1/chrome/browser/ui/browser.cc#... chrome/browser/ui/browser.cc:1425: return; On 2012/09/25 03:07:18, Nico wrote: > test Done. Added a DownloadTest.HiddenDownload test.
LGTM. Thanks.
test lgtm
http://codereview.chromium.org/10980002/diff/23003/chrome/browser/download/do... File chrome/browser/download/download_browsertest.cc (right): http://codereview.chromium.org/10980002/diff/23003/chrome/browser/download/do... chrome/browser/download/download_browsertest.cc:2268: // Download and set ShouldShowInDownloadsUI to false. Nit: Comment out of date. http://codereview.chromium.org/10980002/diff/23003/chrome/browser/ui/intents/... File chrome/browser/ui/intents/web_intent_picker_controller.cc (right): http://codereview.chromium.org/10980002/diff/23003/chrome/browser/ui/intents/... chrome/browser/ui/intents/web_intent_picker_controller.cc:601: CancelDownload(error, reason); If the extension install failed due to a reason other than the download being interrupted or cancelled, then wouldn't calling CancelDownload() here cause a re-entry to OnExtensionInstallFailure()?
chrome/browser/extensions LGTM with one comment (inline virtuals). http://codereview.chromium.org/10980002/diff/23003/chrome/browser/extensions/... File chrome/browser/extensions/webstore_installer.h (right): http://codereview.chromium.org/10980002/diff/23003/chrome/browser/extensions/... chrome/browser/extensions/webstore_installer.h:57: content::DownloadItem* item) {} Don't inline these (sorry). http://dev.chromium.org/developers/coding-style/cpp-dos-and-donts#TOC-Stop-in....
http://codereview.chromium.org/10980002/diff/23003/chrome/browser/download/do... File chrome/browser/download/download_browsertest.cc (right): http://codereview.chromium.org/10980002/diff/23003/chrome/browser/download/do... chrome/browser/download/download_browsertest.cc:2268: // Download and set ShouldShowInDownloadsUI to false. On 2012/10/02 21:08:37, asanka wrote: > Nit: Comment out of date. Done. http://codereview.chromium.org/10980002/diff/23003/chrome/browser/extensions/... File chrome/browser/extensions/webstore_installer.h (right): http://codereview.chromium.org/10980002/diff/23003/chrome/browser/extensions/... chrome/browser/extensions/webstore_installer.h:57: content::DownloadItem* item) {} On 2012/10/02 21:09:52, miket wrote: > Don't inline these (sorry). > > http://dev.chromium.org/developers/coding-style/cpp-dos-and-donts#TOC-Stop-in.... Done. http://codereview.chromium.org/10980002/diff/23003/chrome/browser/ui/intents/... File chrome/browser/ui/intents/web_intent_picker_controller.cc (right): http://codereview.chromium.org/10980002/diff/23003/chrome/browser/ui/intents/... chrome/browser/ui/intents/web_intent_picker_controller.cc:601: CancelDownload(error, reason); On 2012/10/02 21:08:37, asanka wrote: > If the extension install failed due to a reason other than the download being > interrupted or cancelled, then wouldn't calling CancelDownload() here cause a > re-entry to OnExtensionInstallFailure()? Done. I see what you mean, removed. (I'll retest the extension unpack failure. If it's a problem I can fix it in separate CL).
LGTM
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/sail@chromium.org/10980002/26004
Presubmit check for 10980002-26004 failed and returned exit status 1.
Running presubmit commit checks ...
** Presubmit ERRORS **
Missing LGTM from an OWNER for files in these directories:
content/public/browser
Presubmit checks took 2.5s to calculate.
Wat? No. Why are you adding a boolean flag to the download item that's only used in chrome/? Where are the LGs from the downloads people? DownloadItem is a base::SupportsUserData. You can tag a DownloadItem with random booleans with no help from content/.
On 2012/10/02 22:07:21, Avi wrote: > Wat? No. > > Why are you adding a boolean flag to the download item that's only used in > chrome/? Where are the LGs from the downloads people? > > DownloadItem is a base::SupportsUserData. You can tag a DownloadItem with random > booleans with no help from content/. See comments #4, #5, #6. Sailesh originally coded it that way, and changed it to this implementation upon request from downloads folks.
On 2012/10/02 22:07:21, Avi wrote: > Wat? No. > > Why are you adding a boolean flag to the download item that's only used in > chrome/? Where are the LGs from the downloads people? > > DownloadItem is a base::SupportsUserData. You can tag a DownloadItem with random > booleans with no help from content/. See comments #4, #5, #6. Sailesh originally coded it that way, and changed it to this implementation upon request from downloads folks.
Hi all, I've moved the download specific changes to a separate CL: https://codereview.chromium.org/11016022/ This change no longer hides the download. Instead it just pipes the download progress to the picker model.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/sail@chromium.org/10980002/35002
Change committed as 159836 |
