Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
8757007:
Implement additional UI changes for dangerous download warnings. (Closed)
|
Created:
9 years ago by asanka Modified:
9 years ago CC:
chromium-reviews, rdsmith+dwatch_chromium.org, Paweł Hajdan Jr. Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionImplement additional UI changes for dangerous download warnings.
When a download is detected as malicious by the safe browsing service, the download shelf now displays a warning message along with a 'Discard' button and a drop down menu with additional actions.
BUG=102540
TEST=Initiating a download that is flagged as malicious by the safe browsing service causes the new malicious download warning to be displayed on the download shelf.
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=112954
Patch Set 1 #Patch Set 2 : " #
Total comments: 7
Patch Set 3 : Add DCHECKs #
Total comments: 29
Patch Set 4 : Address comments #
Total comments: 4
Patch Set 5 : Rebase only #Patch Set 6 : Address comments and update help center URL #
Total comments: 1
Patch Set 7 : Appease compiler and move 'learn more' URL to url_constants #Patch Set 8 : " #Messages
Total messages: 24 (0 generated)
http://codereview.chromium.org/8757007/diff/2001/chrome/app/generated_resourc... File chrome/app/generated_resources.grd (right): http://codereview.chromium.org/8757007/diff/2001/chrome/app/generated_resourc... chrome/app/generated_resources.grd:2286: http://www.google.com/support/chrome/bin/answer.py?answer=TODO&hl=[GRITLA... TODO: This will be fixed before commit once I have a URL to go here.
Screenshots are here: http://folder/asanka/dangerous-download-prompt/
On 2011/11/30 23:58:07, asanka wrote: > Screenshots are here: > > http://folder/asanka/dangerous-download-prompt/ YAY. Really nice work! I'll take a look at this CL now but I'm probably not the best reviewer.
lgtm all of my comments are questions to help me understand your change. they shouldn't be blocking you in any way. thanks again for doing this so quickly! noé. http://codereview.chromium.org/8757007/diff/2001/chrome/browser/download/down... File chrome/browser/download/download_shelf_context_menu.cc (right): http://codereview.chromium.org/8757007/diff/2001/chrome/browser/download/down... chrome/browser/download/download_shelf_context_menu.cc:98: download_item_->Cancel(true); Is that the same as calling CancelTask()? http://codereview.chromium.org/8757007/diff/2001/chrome/browser/download/down... chrome/browser/download/download_shelf_context_menu.cc:107: if (!browser) is that a failure case? should we dcheck here? http://codereview.chromium.org/8757007/diff/2001/chrome/browser/ui/views/down... File chrome/browser/ui/views/download/download_item_view.cc (right): http://codereview.chromium.org/8757007/diff/2001/chrome/browser/ui/views/down... chrome/browser/ui/views/download/download_item_view.cc:1071: gfx::Size size; does that get initialized to zero?
noelutz: Thanks for taking care of the strings. I'll update the CL with the new strings once we have a decision. dmazzoni: I'd like you to be the primary reviewer. http://codereview.chromium.org/8757007/diff/2001/chrome/browser/download/down... File chrome/browser/download/download_shelf_context_menu.cc (right): http://codereview.chromium.org/8757007/diff/2001/chrome/browser/download/down... chrome/browser/download/download_shelf_context_menu.cc:98: download_item_->Cancel(true); On 2011/12/01 00:48:27, noelutz wrote: > Is that the same as calling CancelTask()? download_model_->CancelTask() does different things depending on whether we are talking to a DownloadItem or a SavePackage. We are only dealing with DownloadItems here. http://codereview.chromium.org/8757007/diff/2001/chrome/browser/download/down... chrome/browser/download/download_shelf_context_menu.cc:107: if (!browser) On 2011/12/01 00:48:27, noelutz wrote: > is that a failure case? should we dcheck here? It shouldn't happen. Aside from not being NULL, we also assume that the last active browser is the one that was hosting the DownloadShelf. We don't want the help center article to open inside a panel browser or a popup browser, for example. I'll add DCHECKs. http://codereview.chromium.org/8757007/diff/2001/chrome/browser/ui/views/down... File chrome/browser/ui/views/download/download_item_view.cc (right): http://codereview.chromium.org/8757007/diff/2001/chrome/browser/ui/views/down... chrome/browser/ui/views/download/download_item_view.cc:1071: gfx::Size size; On 2011/12/01 00:48:27, noelutz wrote: > does that get initialized to zero? Yup. The default constructor sets (width,height) == (0,0).
Will take a look shortly, thanks... On Thu, Dec 1, 2011 at 10:45 AM, <asanka@chromium.org> wrote: > noelutz: Thanks for taking care of the strings. I'll update the CL with > the new > strings once we have a decision. > > dmazzoni: I'd like you to be the primary reviewer. > > > > > http://codereview.chromium.**org/8757007/diff/2001/chrome/** > browser/download/download_**shelf_context_menu.cc<http://codereview.chromium.org/8757007/diff/2001/chrome/browser/download/download_shelf_context_menu.cc> > File chrome/browser/download/**download_shelf_context_menu.cc (right): > > http://codereview.chromium.**org/8757007/diff/2001/chrome/** > browser/download/download_**shelf_context_menu.cc#**newcode98<http://codereview.chromium.org/8757007/diff/2001/chrome/browser/download/download_shelf_context_menu.cc#newcode98> > chrome/browser/download/**download_shelf_context_menu.**cc:98: > download_item_->Cancel(true); > On 2011/12/01 00:48:27, noelutz wrote: > >> Is that the same as calling CancelTask()? >> > > download_model_->CancelTask() does different things depending on whether > we are talking to a DownloadItem or a SavePackage. We are only dealing > with DownloadItems here. > > > http://codereview.chromium.**org/8757007/diff/2001/chrome/** > browser/download/download_**shelf_context_menu.cc#**newcode107<http://codereview.chromium.org/8757007/diff/2001/chrome/browser/download/download_shelf_context_menu.cc#newcode107> > chrome/browser/download/**download_shelf_context_menu.**cc:107: if > (!browser) > On 2011/12/01 00:48:27, noelutz wrote: > >> is that a failure case? should we dcheck here? >> > > It shouldn't happen. Aside from not being NULL, we also assume that the > last active browser is the one that was hosting the DownloadShelf. We > don't want the help center article to open inside a panel browser or a > popup browser, for example. > > I'll add DCHECKs. > > > http://codereview.chromium.**org/8757007/diff/2001/chrome/** > browser/ui/views/download/**download_item_view.cc<http://codereview.chromium.org/8757007/diff/2001/chrome/browser/ui/views/download/download_item_view.cc> > File chrome/browser/ui/views/**download/download_item_view.cc (right): > > http://codereview.chromium.**org/8757007/diff/2001/chrome/** > browser/ui/views/download/**download_item_view.cc#**newcode1071<http://codereview.chromium.org/8757007/diff/2001/chrome/browser/ui/views/download/download_item_view.cc#newcode1071> > chrome/browser/ui/views/**download/download_item_view.**cc:1071: gfx::Size > size; > On 2011/12/01 00:48:27, noelutz wrote: > >> does that get initialized to zero? >> > > Yup. The default constructor sets (width,height) == (0,0). > > http://codereview.chromium.**org/8757007/<http://codereview.chromium.org/8757... >
Just curious, are mac/gtk changes in separate changelists? http://codereview.chromium.org/8757007/diff/8001/chrome/app/generated_resourc... File chrome/app/generated_resources.grd (right): http://codereview.chromium.org/8757007/diff/8001/chrome/app/generated_resourc... chrome/app/generated_resources.grd:2492: <message name="IDS_DOWNLOAD_MENU_DISCARD" These look like duplicates, merge error? http://codereview.chromium.org/8757007/diff/8001/chrome/browser/download/down... File chrome/browser/download/download_shelf_context_menu.cc (right): http://codereview.chromium.org/8757007/diff/8001/chrome/browser/download/down... chrome/browser/download/download_shelf_context_menu.cc:28: if (download_item_->GetSafetyState() == DownloadItem::DANGEROUS && What does it mean if GetSafetyState() == DANGEROUS but GetDangerType() is not DANGEROUS_URL or DANGEROUS_CONTENT? I'd prefer something more like: if (download_item_->GetSafetyState() == DownloadItem::DANGEROUS) { if (...DANGEROUS_URL || ...DANGEROUS_CONTENT) { model = GetMaliciousMenuModel(): } else { NOTREACHED(); } } Or, alternatively, use a switch() inside so that new enum values would cause a compile error. http://codereview.chromium.org/8757007/diff/8001/chrome/browser/download/down... chrome/browser/download/download_shelf_context_menu.cc:98: download_item_->Cancel(true); Does Cancel take effect immediately, does it close all file handles before returning so that Delete will definitely succeed? It seems pretty important to delete malicious files, and I know that deleting files can be problematic on Windows, so I wonder if this should be a deferred delete that retries? http://codereview.chromium.org/8757007/diff/8001/chrome/browser/download/down... chrome/browser/download/download_shelf_context_menu.cc:100: // We have been deleted! Get rid of this comment http://codereview.chromium.org/8757007/diff/8001/chrome/browser/ui/views/down... File chrome/browser/ui/views/download/download_item_view.cc (left): http://codereview.chromium.org/8757007/diff/8001/chrome/browser/ui/views/down... chrome/browser/ui/views/download/download_item_view.cc:226: context_menu_->Stop(); Why are you deleting this? Was it wrong or is it no longer needed? http://codereview.chromium.org/8757007/diff/8001/chrome/browser/ui/views/down... chrome/browser/ui/views/download/download_item_view.cc:628: drop_down_image_set = NULL; // No drop-down in dangerous mode. Please preserve this comment somewhere. http://codereview.chromium.org/8757007/diff/8001/chrome/browser/ui/views/down... File chrome/browser/ui/views/download/download_item_view.cc (right): http://codereview.chromium.org/8757007/diff/8001/chrome/browser/ui/views/down... chrome/browser/ui/views/download/download_item_view.cc:196: malicious_mode_body_image_set_ = malicious_mode_body_image_set; Can you just say malicious_mode_body_image_set_ = dangerous_mode_body_image_set? They seem to be the same. If not, could you add a Clone() or a copy constructor so this can be easier? http://codereview.chromium.org/8757007/diff/8001/chrome/browser/ui/views/down... chrome/browser/ui/views/download/download_item_view.cc:390: if (mode_ == DANGEROUS_MODE) How about making this based on whether save_button is showing or not, like: if (save_button_) width += button_size.width() + kButtonPadding; ...since that's the real difference, right? http://codereview.chromium.org/8757007/diff/8001/chrome/browser/ui/views/down... chrome/browser/ui/views/download/download_item_view.cc:616: void DownloadItemView::OnPaint(gfx::Canvas* canvas) { This logic is getting a little more confusing to follow, please add a couple of very brief comments that help clarify: * Why do dangerous and malicious have a different body set, and why don't they distinguish between pushed or not? * Note that there's a drop-down in nromal and malicious mode, but in dangerous mode there's no drop-down http://codereview.chromium.org/8757007/diff/8001/chrome/browser/ui/views/down... chrome/browser/ui/views/download/download_item_view.cc:918: if ((IsShowingWarningDialog() || body_state_ == body_state) && I don't understand why IsShowingWarningDialog() is grouped with (body_state_ == body_state), can you add a comment? Could you split it into two separate if statements so the logic is more clear? http://codereview.chromium.org/8757007/diff/8001/chrome/browser/ui/views/down... chrome/browser/ui/views/download/download_item_view.cc:1178: new_name = dangerous_download_label_->GetText(); Does this need to be updated for dangerous vs malicious?
I'm not certain how useful my review is, as I had a hard time tracking the detailed logic changes within the UI. But the high level structure of the change LGTM> http://codereview.chromium.org/8757007/diff/8001/chrome/browser/download/down... File chrome/browser/download/download_shelf_context_menu.cc (right): http://codereview.chromium.org/8757007/diff/8001/chrome/browser/download/down... chrome/browser/download/download_shelf_context_menu.cc:99: download_item_->Delete(DownloadItem::DELETE_DUE_TO_USER_DISCARD); Remove (which Delete() calls) does a Cancel unilaterally, so it's not necessary here. I'm not sure whether it's desirable or not; that's up to you. http://codereview.chromium.org/8757007/diff/8001/chrome/browser/download/down... chrome/browser/download/download_shelf_context_menu.cc:100: // We have been deleted! Have you looked over the callers to this routine, and the ownership structure of this class and its containing classes? My memory is that the DownloadItemView deletes itself when the DownloadItem goes away, and I suspect that that cascades down to the DownloadShelfContextMenu. Are you certain that the stack unwinding all the way back up from here never assumes that the various "this" objects are valid? It would be worthwhile having comments on all such return values, so that no-one later makes the (not unreasonable) assumption that "this" will still be around in a method of the containing classes. http://codereview.chromium.org/8757007/diff/8001/chrome/browser/ui/views/down... File chrome/browser/ui/views/download/download_item_view.cc (right): http://codereview.chromium.org/8757007/diff/8001/chrome/browser/ui/views/down... chrome/browser/ui/views/download/download_item_view.cc:498: if (!IsShowingWarningDialog()) How can we be in dangerous mode and not showing a warning dialog?
dmazzoni: There's currently no equivalent CL in-flight for GTK/Mac. The CL for GTK (http://codereview.chromium.org/8588037/ ) for implementing support for dangerous download warnings is on hold until the UI is finalized. noelutz: I'm not sure there's consensus on the strings. :) We still need the help center article URL. http://codereview.chromium.org/8757007/diff/8001/chrome/app/generated_resourc... File chrome/app/generated_resources.grd (right): http://codereview.chromium.org/8757007/diff/8001/chrome/app/generated_resourc... chrome/app/generated_resources.grd:2492: <message name="IDS_DOWNLOAD_MENU_DISCARD" On 2011/12/01 19:49:22, Dominic Mazzoni wrote: > These look like duplicates, merge error? These are the use_titlecase and !use_titlecase cases. Only the "Learn more" string is different. http://codereview.chromium.org/8757007/diff/8001/chrome/browser/download/down... File chrome/browser/download/download_shelf_context_menu.cc (right): http://codereview.chromium.org/8757007/diff/8001/chrome/browser/download/down... chrome/browser/download/download_shelf_context_menu.cc:28: if (download_item_->GetSafetyState() == DownloadItem::DANGEROUS && On 2011/12/01 19:49:22, Dominic Mazzoni wrote: > What does it mean if GetSafetyState() == DANGEROUS but GetDangerType() is not > DANGEROUS_URL or DANGEROUS_CONTENT? > > I'd prefer something more like: > > if (download_item_->GetSafetyState() == DownloadItem::DANGEROUS) { > if (...DANGEROUS_URL || > ...DANGEROUS_CONTENT) { > model = GetMaliciousMenuModel(): > } else { > NOTREACHED(); > } > } > > Or, alternatively, use a switch() inside so that new enum values would cause a > compile error. Done. Went with your 'if' suggestion. http://codereview.chromium.org/8757007/diff/8001/chrome/browser/download/down... chrome/browser/download/download_shelf_context_menu.cc:98: download_item_->Cancel(true); On 2011/12/01 19:49:22, Dominic Mazzoni wrote: > Does Cancel take effect immediately, does it close all file handles before > returning so that Delete will definitely succeed? > > It seems pretty important to delete malicious files, and I know that deleting > files can be problematic on Windows, so I wonder if this should be a deferred > delete that retries? Calling Delete() results in a task posted to the FILE thread that deletes the actual file. The DownloadItem cancels and removes itself without waiting for the file to be deleted. The Cancel() call (called implicitly from Delete()), causes DownloadFileManager::CancelDownload() to be called, which also attempts to delete the file. http://codereview.chromium.org/8757007/diff/8001/chrome/browser/download/down... chrome/browser/download/download_shelf_context_menu.cc:99: download_item_->Delete(DownloadItem::DELETE_DUE_TO_USER_DISCARD); On 2011/12/01 19:50:46, rdsmith wrote: > Remove (which Delete() calls) does a Cancel unilaterally, so it's not necessary > here. I'm not sure whether it's desirable or not; that's up to you. Done. http://codereview.chromium.org/8757007/diff/8001/chrome/browser/download/down... chrome/browser/download/download_shelf_context_menu.cc:100: // We have been deleted! On 2011/12/01 19:49:22, Dominic Mazzoni wrote: > Get rid of this comment Done. http://codereview.chromium.org/8757007/diff/8001/chrome/browser/download/down... chrome/browser/download/download_shelf_context_menu.cc:100: // We have been deleted! On 2011/12/01 19:50:46, rdsmith wrote: > Have you looked over the callers to this routine, and the ownership structure of > this class and its containing classes? My memory is that the DownloadItemView > deletes itself when the DownloadItem goes away, and I suspect that that cascades > down to the DownloadShelfContextMenu. Are you certain that the stack unwinding > all the way back up from here never assumes that the various "this" objects are > valid? It would be worthwhile having comments on all such return values, so > that no-one later makes the (not unreasonable) assumption that "this" will still > be around in a method of the containing classes. Done. In general, any time we run a message loop we run the risk of some extension deleting the download item and causing DownloadItemView and everything within it to disappear. http://codereview.chromium.org/8757007/diff/8001/chrome/browser/ui/views/down... File chrome/browser/ui/views/download/download_item_view.cc (left): http://codereview.chromium.org/8757007/diff/8001/chrome/browser/ui/views/down... chrome/browser/ui/views/download/download_item_view.cc:226: context_menu_->Stop(); On 2011/12/01 19:49:22, Dominic Mazzoni wrote: > Why are you deleting this? Was it wrong or is it no longer needed? It was used as a signal that DownloadItemView (and everything down the hierarchy) was deleted. DownloadShelfContextMenu would set its download_item_ member to NULL in response. In DownloadItemView::ShowContextMenu(), we were calling context_menu_->Run() and afterwards checking (context_menu_->download_item()) to see if it was safe to access 'this' and if so change the pushed state of the drop down icon. I thought this was wrong because: - DownloadItemView::context_menu_ is a scoped_ptr<DownloadShelfContextMenu>, so the DownloadShelfContextMenu is deleted after Stop() is called. - The context_menu_->download() check accesses the potentially deleted *this, and the potentially deleted DownloadShelfContextMenu. The addition of the "Discard" option to the drop down menu made it so that we were reliably hitting this case. We now post a non-nestable task with a WeakPtr to the DownloadItemView to un-press the drop down button, which simply doesn't run if the DownloadItemView was deleted as a result of running the drop down menu. http://codereview.chromium.org/8757007/diff/8001/chrome/browser/ui/views/down... chrome/browser/ui/views/download/download_item_view.cc:628: drop_down_image_set = NULL; // No drop-down in dangerous mode. On 2011/12/01 19:49:22, Dominic Mazzoni wrote: > Please preserve this comment somewhere. Done. http://codereview.chromium.org/8757007/diff/8001/chrome/browser/ui/views/down... File chrome/browser/ui/views/download/download_item_view.cc (right): http://codereview.chromium.org/8757007/diff/8001/chrome/browser/ui/views/down... chrome/browser/ui/views/download/download_item_view.cc:196: malicious_mode_body_image_set_ = malicious_mode_body_image_set; On 2011/12/01 19:49:22, Dominic Mazzoni wrote: > Can you just say malicious_mode_body_image_set_ = dangerous_mode_body_image_set? > They seem to be the same. If not, could you add a Clone() or a copy constructor > so this can be easier? Done. I think you meant normal_body_image_set. Currently the body background looks the same in malicious and normal modes. http://codereview.chromium.org/8757007/diff/8001/chrome/browser/ui/views/down... chrome/browser/ui/views/download/download_item_view.cc:390: if (mode_ == DANGEROUS_MODE) On 2011/12/01 19:49:22, Dominic Mazzoni wrote: > How about making this based on whether save_button is showing or not, like: > > if (save_button_) > width += button_size.width() + kButtonPadding; > > ...since that's the real difference, right? Done. http://codereview.chromium.org/8757007/diff/8001/chrome/browser/ui/views/down... chrome/browser/ui/views/download/download_item_view.cc:498: if (!IsShowingWarningDialog()) On 2011/12/01 19:50:46, rdsmith wrote: > How can we be in dangerous mode and not showing a warning dialog? We get here when we aren't showing a warning dialog as well. We exit early only if we are in DANGEROUS_MODE because no part of the view responds to mouse hover in that mode. http://codereview.chromium.org/8757007/diff/8001/chrome/browser/ui/views/down... chrome/browser/ui/views/download/download_item_view.cc:616: void DownloadItemView::OnPaint(gfx::Canvas* canvas) { On 2011/12/01 19:49:22, Dominic Mazzoni wrote: > This logic is getting a little more confusing to follow, please add a couple of > very brief comments that help clarify: > > * Why do dangerous and malicious have a different body set, and why don't they > distinguish between pushed or not? > * Note that there's a drop-down in nromal and malicious mode, but in dangerous > mode there's no drop-down > I added a comment before OnPaint() that gives an overview of what we are going to paint, and also added a few comments within OnPaint(). Let me know if that is sufficient. http://codereview.chromium.org/8757007/diff/8001/chrome/browser/ui/views/down... chrome/browser/ui/views/download/download_item_view.cc:918: if ((IsShowingWarningDialog() || body_state_ == body_state) && On 2011/12/01 19:49:22, Dominic Mazzoni wrote: > I don't understand why IsShowingWarningDialog() is grouped with (body_state_ == > body_state), can you add a comment? Could you split it into two separate if > statements so the logic is more clear? If we are showing the warning dialog, we don't change the body state. So it's equivalent to the case where the new and the current body states are the same. Reorganized code to make this clearer. http://codereview.chromium.org/8757007/diff/8001/chrome/browser/ui/views/down... chrome/browser/ui/views/download/download_item_view.cc:1178: new_name = dangerous_download_label_->GetText(); On 2011/12/01 19:49:22, Dominic Mazzoni wrote: > Does this need to be updated for dangerous vs malicious? For both cases, the accessible text is going to be the text that was set in the dangerous_download_label_.
We don't have a definite answer on the string yet. Ian is talking to the lawyers. I'm also waiting on the help center URL. When do you need this by? Thanks, noe. On Thu, Dec 1, 2011 at 4:05 PM, <asanka@chromium.org> wrote: > dmazzoni: There's currently no equivalent CL in-flight for GTK/Mac. The CL > for > GTK (http://codereview.chromium.org/8588037/ ) for implementing support for > dangerous download warnings is on hold until the UI is finalized. > > noelutz: I'm not sure there's consensus on the strings. :) We still need the > help center article URL. > > > http://codereview.chromium.org/8757007/diff/8001/chrome/app/generated_resourc... > File chrome/app/generated_resources.grd (right): > > http://codereview.chromium.org/8757007/diff/8001/chrome/app/generated_resourc... > chrome/app/generated_resources.grd:2492: <message > name="IDS_DOWNLOAD_MENU_DISCARD" > On 2011/12/01 19:49:22, Dominic Mazzoni wrote: >> >> These look like duplicates, merge error? > > These are the use_titlecase and !use_titlecase cases. Only the "Learn > more" string is different. > > http://codereview.chromium.org/8757007/diff/8001/chrome/browser/download/down... > File chrome/browser/download/download_shelf_context_menu.cc (right): > > http://codereview.chromium.org/8757007/diff/8001/chrome/browser/download/down... > chrome/browser/download/download_shelf_context_menu.cc:28: if > (download_item_->GetSafetyState() == DownloadItem::DANGEROUS && > On 2011/12/01 19:49:22, Dominic Mazzoni wrote: >> >> What does it mean if GetSafetyState() == DANGEROUS but GetDangerType() > > is not >> >> DANGEROUS_URL or DANGEROUS_CONTENT? > >> I'd prefer something more like: > >> if (download_item_->GetSafetyState() == DownloadItem::DANGEROUS) { >> if (...DANGEROUS_URL || >> ...DANGEROUS_CONTENT) { >> model = GetMaliciousMenuModel(): >> } else { >> NOTREACHED(); >> } >> } > >> Or, alternatively, use a switch() inside so that new enum values would > > cause a >> >> compile error. > > Done. Went with your 'if' suggestion. > > http://codereview.chromium.org/8757007/diff/8001/chrome/browser/download/down... > chrome/browser/download/download_shelf_context_menu.cc:98: > download_item_->Cancel(true); > On 2011/12/01 19:49:22, Dominic Mazzoni wrote: >> >> Does Cancel take effect immediately, does it close all file handles > > before >> >> returning so that Delete will definitely succeed? > >> It seems pretty important to delete malicious files, and I know that > > deleting >> >> files can be problematic on Windows, so I wonder if this should be a > > deferred >> >> delete that retries? > > Calling Delete() results in a task posted to the FILE thread that > deletes the actual file. The DownloadItem cancels and removes itself > without waiting for the file to be deleted. > > The Cancel() call (called implicitly from Delete()), causes > DownloadFileManager::CancelDownload() to be called, which also attempts > to delete the file. > > http://codereview.chromium.org/8757007/diff/8001/chrome/browser/download/down... > chrome/browser/download/download_shelf_context_menu.cc:99: > download_item_->Delete(DownloadItem::DELETE_DUE_TO_USER_DISCARD); > On 2011/12/01 19:50:46, rdsmith wrote: >> >> Remove (which Delete() calls) does a Cancel unilaterally, so it's not > > necessary >> >> here. I'm not sure whether it's desirable or not; that's up to you. > > Done. > > http://codereview.chromium.org/8757007/diff/8001/chrome/browser/download/down... > chrome/browser/download/download_shelf_context_menu.cc:100: // We have > been deleted! > On 2011/12/01 19:49:22, Dominic Mazzoni wrote: >> >> Get rid of this comment > > Done. > > http://codereview.chromium.org/8757007/diff/8001/chrome/browser/download/down... > chrome/browser/download/download_shelf_context_menu.cc:100: // We have > been deleted! > On 2011/12/01 19:50:46, rdsmith wrote: >> >> Have you looked over the callers to this routine, and the ownership > > structure of >> >> this class and its containing classes? My memory is that the > > DownloadItemView >> >> deletes itself when the DownloadItem goes away, and I suspect that > > that cascades >> >> down to the DownloadShelfContextMenu. Are you certain that the stack > > unwinding >> >> all the way back up from here never assumes that the various "this" > > objects are >> >> valid? It would be worthwhile having comments on all such return > > values, so >> >> that no-one later makes the (not unreasonable) assumption that "this" > > will still >> >> be around in a method of the containing classes. > > Done. In general, any time we run a message loop we run the risk of some > extension deleting the download item and causing DownloadItemView and > everything within it to disappear. > > http://codereview.chromium.org/8757007/diff/8001/chrome/browser/ui/views/down... > File chrome/browser/ui/views/download/download_item_view.cc (left): > > http://codereview.chromium.org/8757007/diff/8001/chrome/browser/ui/views/down... > chrome/browser/ui/views/download/download_item_view.cc:226: > context_menu_->Stop(); > On 2011/12/01 19:49:22, Dominic Mazzoni wrote: >> >> Why are you deleting this? Was it wrong or is it no longer needed? > > It was used as a signal that DownloadItemView (and everything down the > hierarchy) was deleted. DownloadShelfContextMenu would set its > download_item_ member to NULL in response. > > In DownloadItemView::ShowContextMenu(), we were calling > context_menu_->Run() and afterwards checking > (context_menu_->download_item()) to see if it was safe to access 'this' > and if so change the pushed state of the drop down icon. > > I thought this was wrong because: > > - DownloadItemView::context_menu_ is a > scoped_ptr<DownloadShelfContextMenu>, so the DownloadShelfContextMenu is > deleted after Stop() is called. > > - The context_menu_->download() check accesses the potentially deleted > *this, and the potentially deleted DownloadShelfContextMenu. > > The addition of the "Discard" option to the drop down menu made it so > that we were reliably hitting this case. We now post a non-nestable > task with a WeakPtr to the DownloadItemView to un-press the drop down > button, which simply doesn't run if the DownloadItemView was deleted as > a result of running the drop down menu. > > http://codereview.chromium.org/8757007/diff/8001/chrome/browser/ui/views/down... > chrome/browser/ui/views/download/download_item_view.cc:628: > drop_down_image_set = NULL; // No drop-down in dangerous mode. > On 2011/12/01 19:49:22, Dominic Mazzoni wrote: >> >> Please preserve this comment somewhere. > > Done. > > http://codereview.chromium.org/8757007/diff/8001/chrome/browser/ui/views/down... > File chrome/browser/ui/views/download/download_item_view.cc (right): > > http://codereview.chromium.org/8757007/diff/8001/chrome/browser/ui/views/down... > chrome/browser/ui/views/download/download_item_view.cc:196: > malicious_mode_body_image_set_ = malicious_mode_body_image_set; > On 2011/12/01 19:49:22, Dominic Mazzoni wrote: >> >> Can you just say malicious_mode_body_image_set_ = > > dangerous_mode_body_image_set? >> >> They seem to be the same. If not, could you add a Clone() or a copy > > constructor >> >> so this can be easier? > > Done. I think you meant normal_body_image_set. Currently the body > background looks the same in malicious and normal modes. > > http://codereview.chromium.org/8757007/diff/8001/chrome/browser/ui/views/down... > chrome/browser/ui/views/download/download_item_view.cc:390: if (mode_ == > DANGEROUS_MODE) > On 2011/12/01 19:49:22, Dominic Mazzoni wrote: >> >> How about making this based on whether save_button is showing or not, > > like: > >> if (save_button_) >> width += button_size.width() + kButtonPadding; > >> ...since that's the real difference, right? > > Done. > > http://codereview.chromium.org/8757007/diff/8001/chrome/browser/ui/views/down... > chrome/browser/ui/views/download/download_item_view.cc:498: if > (!IsShowingWarningDialog()) > On 2011/12/01 19:50:46, rdsmith wrote: >> >> How can we be in dangerous mode and not showing a warning dialog? > > We get here when we aren't showing a warning dialog as well. We exit > early only if we are in DANGEROUS_MODE because no part of the view > responds to mouse hover in that mode. > > http://codereview.chromium.org/8757007/diff/8001/chrome/browser/ui/views/down... > chrome/browser/ui/views/download/download_item_view.cc:616: void > DownloadItemView::OnPaint(gfx::Canvas* canvas) { > On 2011/12/01 19:49:22, Dominic Mazzoni wrote: >> >> This logic is getting a little more confusing to follow, please add a > > couple of >> >> very brief comments that help clarify: > >> * Why do dangerous and malicious have a different body set, and why > > don't they >> >> distinguish between pushed or not? >> * Note that there's a drop-down in nromal and malicious mode, but in > > dangerous >> >> mode there's no drop-down > > > I added a comment before OnPaint() that gives an overview of what we are > going to paint, and also added a few comments within OnPaint(). Let me > know if that is sufficient. > > http://codereview.chromium.org/8757007/diff/8001/chrome/browser/ui/views/down... > chrome/browser/ui/views/download/download_item_view.cc:918: if > ((IsShowingWarningDialog() || body_state_ == body_state) && > On 2011/12/01 19:49:22, Dominic Mazzoni wrote: >> >> I don't understand why IsShowingWarningDialog() is grouped with > > (body_state_ == >> >> body_state), can you add a comment? Could you split it into two > > separate if >> >> statements so the logic is more clear? > > If we are showing the warning dialog, we don't change the body state. So > it's equivalent to the case where the new and the current body states > are the same. Reorganized code to make this clearer. > > http://codereview.chromium.org/8757007/diff/8001/chrome/browser/ui/views/down... > chrome/browser/ui/views/download/download_item_view.cc:1178: new_name = > dangerous_download_label_->GetText(); > On 2011/12/01 19:49:22, Dominic Mazzoni wrote: >> >> Does this need to be updated for dangerous vs malicious? > > For both cases, the accessible text is going to be the text that was set > in the dangerous_download_label_. > > http://codereview.chromium.org/8757007/ > -- -|||--
We have a winner: "[foo.exe|This file] appears to be malicious. [Discard][>]" Thanks, noe. On Thu, Dec 1, 2011 at 4:31 PM, Noé Lutz <noelutz@google.com> wrote: > We don't have a definite answer on the string yet. Ian is talking to > the lawyers. I'm also waiting on the help center URL. When do you > need this by? > > Thanks, > noe. > > On Thu, Dec 1, 2011 at 4:05 PM, <asanka@chromium.org> wrote: >> dmazzoni: There's currently no equivalent CL in-flight for GTK/Mac. The CL >> for >> GTK (http://codereview.chromium.org/8588037/ ) for implementing support for >> dangerous download warnings is on hold until the UI is finalized. >> >> noelutz: I'm not sure there's consensus on the strings. :) We still need the >> help center article URL. >> >> >> http://codereview.chromium.org/8757007/diff/8001/chrome/app/generated_resourc... >> File chrome/app/generated_resources.grd (right): >> >> http://codereview.chromium.org/8757007/diff/8001/chrome/app/generated_resourc... >> chrome/app/generated_resources.grd:2492: <message >> name="IDS_DOWNLOAD_MENU_DISCARD" >> On 2011/12/01 19:49:22, Dominic Mazzoni wrote: >>> >>> These look like duplicates, merge error? >> >> These are the use_titlecase and !use_titlecase cases. Only the "Learn >> more" string is different. >> >> http://codereview.chromium.org/8757007/diff/8001/chrome/browser/download/down... >> File chrome/browser/download/download_shelf_context_menu.cc (right): >> >> http://codereview.chromium.org/8757007/diff/8001/chrome/browser/download/down... >> chrome/browser/download/download_shelf_context_menu.cc:28: if >> (download_item_->GetSafetyState() == DownloadItem::DANGEROUS && >> On 2011/12/01 19:49:22, Dominic Mazzoni wrote: >>> >>> What does it mean if GetSafetyState() == DANGEROUS but GetDangerType() >> >> is not >>> >>> DANGEROUS_URL or DANGEROUS_CONTENT? >> >>> I'd prefer something more like: >> >>> if (download_item_->GetSafetyState() == DownloadItem::DANGEROUS) { >>> if (...DANGEROUS_URL || >>> ...DANGEROUS_CONTENT) { >>> model = GetMaliciousMenuModel(): >>> } else { >>> NOTREACHED(); >>> } >>> } >> >>> Or, alternatively, use a switch() inside so that new enum values would >> >> cause a >>> >>> compile error. >> >> Done. Went with your 'if' suggestion. >> >> http://codereview.chromium.org/8757007/diff/8001/chrome/browser/download/down... >> chrome/browser/download/download_shelf_context_menu.cc:98: >> download_item_->Cancel(true); >> On 2011/12/01 19:49:22, Dominic Mazzoni wrote: >>> >>> Does Cancel take effect immediately, does it close all file handles >> >> before >>> >>> returning so that Delete will definitely succeed? >> >>> It seems pretty important to delete malicious files, and I know that >> >> deleting >>> >>> files can be problematic on Windows, so I wonder if this should be a >> >> deferred >>> >>> delete that retries? >> >> Calling Delete() results in a task posted to the FILE thread that >> deletes the actual file. The DownloadItem cancels and removes itself >> without waiting for the file to be deleted. >> >> The Cancel() call (called implicitly from Delete()), causes >> DownloadFileManager::CancelDownload() to be called, which also attempts >> to delete the file. >> >> http://codereview.chromium.org/8757007/diff/8001/chrome/browser/download/down... >> chrome/browser/download/download_shelf_context_menu.cc:99: >> download_item_->Delete(DownloadItem::DELETE_DUE_TO_USER_DISCARD); >> On 2011/12/01 19:50:46, rdsmith wrote: >>> >>> Remove (which Delete() calls) does a Cancel unilaterally, so it's not >> >> necessary >>> >>> here. I'm not sure whether it's desirable or not; that's up to you. >> >> Done. >> >> http://codereview.chromium.org/8757007/diff/8001/chrome/browser/download/down... >> chrome/browser/download/download_shelf_context_menu.cc:100: // We have >> been deleted! >> On 2011/12/01 19:49:22, Dominic Mazzoni wrote: >>> >>> Get rid of this comment >> >> Done. >> >> http://codereview.chromium.org/8757007/diff/8001/chrome/browser/download/down... >> chrome/browser/download/download_shelf_context_menu.cc:100: // We have >> been deleted! >> On 2011/12/01 19:50:46, rdsmith wrote: >>> >>> Have you looked over the callers to this routine, and the ownership >> >> structure of >>> >>> this class and its containing classes? My memory is that the >> >> DownloadItemView >>> >>> deletes itself when the DownloadItem goes away, and I suspect that >> >> that cascades >>> >>> down to the DownloadShelfContextMenu. Are you certain that the stack >> >> unwinding >>> >>> all the way back up from here never assumes that the various "this" >> >> objects are >>> >>> valid? It would be worthwhile having comments on all such return >> >> values, so >>> >>> that no-one later makes the (not unreasonable) assumption that "this" >> >> will still >>> >>> be around in a method of the containing classes. >> >> Done. In general, any time we run a message loop we run the risk of some >> extension deleting the download item and causing DownloadItemView and >> everything within it to disappear. >> >> http://codereview.chromium.org/8757007/diff/8001/chrome/browser/ui/views/down... >> File chrome/browser/ui/views/download/download_item_view.cc (left): >> >> http://codereview.chromium.org/8757007/diff/8001/chrome/browser/ui/views/down... >> chrome/browser/ui/views/download/download_item_view.cc:226: >> context_menu_->Stop(); >> On 2011/12/01 19:49:22, Dominic Mazzoni wrote: >>> >>> Why are you deleting this? Was it wrong or is it no longer needed? >> >> It was used as a signal that DownloadItemView (and everything down the >> hierarchy) was deleted. DownloadShelfContextMenu would set its >> download_item_ member to NULL in response. >> >> In DownloadItemView::ShowContextMenu(), we were calling >> context_menu_->Run() and afterwards checking >> (context_menu_->download_item()) to see if it was safe to access 'this' >> and if so change the pushed state of the drop down icon. >> >> I thought this was wrong because: >> >> - DownloadItemView::context_menu_ is a >> scoped_ptr<DownloadShelfContextMenu>, so the DownloadShelfContextMenu is >> deleted after Stop() is called. >> >> - The context_menu_->download() check accesses the potentially deleted >> *this, and the potentially deleted DownloadShelfContextMenu. >> >> The addition of the "Discard" option to the drop down menu made it so >> that we were reliably hitting this case. We now post a non-nestable >> task with a WeakPtr to the DownloadItemView to un-press the drop down >> button, which simply doesn't run if the DownloadItemView was deleted as >> a result of running the drop down menu. >> >> http://codereview.chromium.org/8757007/diff/8001/chrome/browser/ui/views/down... >> chrome/browser/ui/views/download/download_item_view.cc:628: >> drop_down_image_set = NULL; // No drop-down in dangerous mode. >> On 2011/12/01 19:49:22, Dominic Mazzoni wrote: >>> >>> Please preserve this comment somewhere. >> >> Done. >> >> http://codereview.chromium.org/8757007/diff/8001/chrome/browser/ui/views/down... >> File chrome/browser/ui/views/download/download_item_view.cc (right): >> >> http://codereview.chromium.org/8757007/diff/8001/chrome/browser/ui/views/down... >> chrome/browser/ui/views/download/download_item_view.cc:196: >> malicious_mode_body_image_set_ = malicious_mode_body_image_set; >> On 2011/12/01 19:49:22, Dominic Mazzoni wrote: >>> >>> Can you just say malicious_mode_body_image_set_ = >> >> dangerous_mode_body_image_set? >>> >>> They seem to be the same. If not, could you add a Clone() or a copy >> >> constructor >>> >>> so this can be easier? >> >> Done. I think you meant normal_body_image_set. Currently the body >> background looks the same in malicious and normal modes. >> >> http://codereview.chromium.org/8757007/diff/8001/chrome/browser/ui/views/down... >> chrome/browser/ui/views/download/download_item_view.cc:390: if (mode_ == >> DANGEROUS_MODE) >> On 2011/12/01 19:49:22, Dominic Mazzoni wrote: >>> >>> How about making this based on whether save_button is showing or not, >> >> like: >> >>> if (save_button_) >>> width += button_size.width() + kButtonPadding; >> >>> ...since that's the real difference, right? >> >> Done. >> >> http://codereview.chromium.org/8757007/diff/8001/chrome/browser/ui/views/down... >> chrome/browser/ui/views/download/download_item_view.cc:498: if >> (!IsShowingWarningDialog()) >> On 2011/12/01 19:50:46, rdsmith wrote: >>> >>> How can we be in dangerous mode and not showing a warning dialog? >> >> We get here when we aren't showing a warning dialog as well. We exit >> early only if we are in DANGEROUS_MODE because no part of the view >> responds to mouse hover in that mode. >> >> http://codereview.chromium.org/8757007/diff/8001/chrome/browser/ui/views/down... >> chrome/browser/ui/views/download/download_item_view.cc:616: void >> DownloadItemView::OnPaint(gfx::Canvas* canvas) { >> On 2011/12/01 19:49:22, Dominic Mazzoni wrote: >>> >>> This logic is getting a little more confusing to follow, please add a >> >> couple of >>> >>> very brief comments that help clarify: >> >>> * Why do dangerous and malicious have a different body set, and why >> >> don't they >>> >>> distinguish between pushed or not? >>> * Note that there's a drop-down in nromal and malicious mode, but in >> >> dangerous >>> >>> mode there's no drop-down >> >> >> I added a comment before OnPaint() that gives an overview of what we are >> going to paint, and also added a few comments within OnPaint(). Let me >> know if that is sufficient. >> >> http://codereview.chromium.org/8757007/diff/8001/chrome/browser/ui/views/down... >> chrome/browser/ui/views/download/download_item_view.cc:918: if >> ((IsShowingWarningDialog() || body_state_ == body_state) && >> On 2011/12/01 19:49:22, Dominic Mazzoni wrote: >>> >>> I don't understand why IsShowingWarningDialog() is grouped with >> >> (body_state_ == >>> >>> body_state), can you add a comment? Could you split it into two >> >> separate if >>> >>> statements so the logic is more clear? >> >> If we are showing the warning dialog, we don't change the body state. So >> it's equivalent to the case where the new and the current body states >> are the same. Reorganized code to make this clearer. >> >> http://codereview.chromium.org/8757007/diff/8001/chrome/browser/ui/views/down... >> chrome/browser/ui/views/download/download_item_view.cc:1178: new_name = >> dangerous_download_label_->GetText(); >> On 2011/12/01 19:49:22, Dominic Mazzoni wrote: >>> >>> Does this need to be updated for dangerous vs malicious? >> >> For both cases, the accessible text is going to be the text that was set >> in the dangerous_download_label_. >> >> http://codereview.chromium.org/8757007/ >> > > > > -- > -|||-- > -- -|||--
Argh. no 'to be': "[foo.exe|This file] appears malicious. [Discard][>]" On Thu, Dec 1, 2011 at 5:02 PM, Noé Lutz <noelutz@google.com> wrote: > We have a winner: > > "[foo.exe|This file] appears to be malicious. [Discard][>]" > > Thanks, > noe. > > On Thu, Dec 1, 2011 at 4:31 PM, Noé Lutz <noelutz@google.com> wrote: >> We don't have a definite answer on the string yet. Ian is talking to >> the lawyers. I'm also waiting on the help center URL. When do you >> need this by? >> >> Thanks, >> noe. >> >> On Thu, Dec 1, 2011 at 4:05 PM, <asanka@chromium.org> wrote: >>> dmazzoni: There's currently no equivalent CL in-flight for GTK/Mac. The CL >>> for >>> GTK (http://codereview.chromium.org/8588037/ ) for implementing support for >>> dangerous download warnings is on hold until the UI is finalized. >>> >>> noelutz: I'm not sure there's consensus on the strings. :) We still need the >>> help center article URL. >>> >>> >>> http://codereview.chromium.org/8757007/diff/8001/chrome/app/generated_resourc... >>> File chrome/app/generated_resources.grd (right): >>> >>> http://codereview.chromium.org/8757007/diff/8001/chrome/app/generated_resourc... >>> chrome/app/generated_resources.grd:2492: <message >>> name="IDS_DOWNLOAD_MENU_DISCARD" >>> On 2011/12/01 19:49:22, Dominic Mazzoni wrote: >>>> >>>> These look like duplicates, merge error? >>> >>> These are the use_titlecase and !use_titlecase cases. Only the "Learn >>> more" string is different. >>> >>> http://codereview.chromium.org/8757007/diff/8001/chrome/browser/download/down... >>> File chrome/browser/download/download_shelf_context_menu.cc (right): >>> >>> http://codereview.chromium.org/8757007/diff/8001/chrome/browser/download/down... >>> chrome/browser/download/download_shelf_context_menu.cc:28: if >>> (download_item_->GetSafetyState() == DownloadItem::DANGEROUS && >>> On 2011/12/01 19:49:22, Dominic Mazzoni wrote: >>>> >>>> What does it mean if GetSafetyState() == DANGEROUS but GetDangerType() >>> >>> is not >>>> >>>> DANGEROUS_URL or DANGEROUS_CONTENT? >>> >>>> I'd prefer something more like: >>> >>>> if (download_item_->GetSafetyState() == DownloadItem::DANGEROUS) { >>>> if (...DANGEROUS_URL || >>>> ...DANGEROUS_CONTENT) { >>>> model = GetMaliciousMenuModel(): >>>> } else { >>>> NOTREACHED(); >>>> } >>>> } >>> >>>> Or, alternatively, use a switch() inside so that new enum values would >>> >>> cause a >>>> >>>> compile error. >>> >>> Done. Went with your 'if' suggestion. >>> >>> http://codereview.chromium.org/8757007/diff/8001/chrome/browser/download/down... >>> chrome/browser/download/download_shelf_context_menu.cc:98: >>> download_item_->Cancel(true); >>> On 2011/12/01 19:49:22, Dominic Mazzoni wrote: >>>> >>>> Does Cancel take effect immediately, does it close all file handles >>> >>> before >>>> >>>> returning so that Delete will definitely succeed? >>> >>>> It seems pretty important to delete malicious files, and I know that >>> >>> deleting >>>> >>>> files can be problematic on Windows, so I wonder if this should be a >>> >>> deferred >>>> >>>> delete that retries? >>> >>> Calling Delete() results in a task posted to the FILE thread that >>> deletes the actual file. The DownloadItem cancels and removes itself >>> without waiting for the file to be deleted. >>> >>> The Cancel() call (called implicitly from Delete()), causes >>> DownloadFileManager::CancelDownload() to be called, which also attempts >>> to delete the file. >>> >>> http://codereview.chromium.org/8757007/diff/8001/chrome/browser/download/down... >>> chrome/browser/download/download_shelf_context_menu.cc:99: >>> download_item_->Delete(DownloadItem::DELETE_DUE_TO_USER_DISCARD); >>> On 2011/12/01 19:50:46, rdsmith wrote: >>>> >>>> Remove (which Delete() calls) does a Cancel unilaterally, so it's not >>> >>> necessary >>>> >>>> here. I'm not sure whether it's desirable or not; that's up to you. >>> >>> Done. >>> >>> http://codereview.chromium.org/8757007/diff/8001/chrome/browser/download/down... >>> chrome/browser/download/download_shelf_context_menu.cc:100: // We have >>> been deleted! >>> On 2011/12/01 19:49:22, Dominic Mazzoni wrote: >>>> >>>> Get rid of this comment >>> >>> Done. >>> >>> http://codereview.chromium.org/8757007/diff/8001/chrome/browser/download/down... >>> chrome/browser/download/download_shelf_context_menu.cc:100: // We have >>> been deleted! >>> On 2011/12/01 19:50:46, rdsmith wrote: >>>> >>>> Have you looked over the callers to this routine, and the ownership >>> >>> structure of >>>> >>>> this class and its containing classes? My memory is that the >>> >>> DownloadItemView >>>> >>>> deletes itself when the DownloadItem goes away, and I suspect that >>> >>> that cascades >>>> >>>> down to the DownloadShelfContextMenu. Are you certain that the stack >>> >>> unwinding >>>> >>>> all the way back up from here never assumes that the various "this" >>> >>> objects are >>>> >>>> valid? It would be worthwhile having comments on all such return >>> >>> values, so >>>> >>>> that no-one later makes the (not unreasonable) assumption that "this" >>> >>> will still >>>> >>>> be around in a method of the containing classes. >>> >>> Done. In general, any time we run a message loop we run the risk of some >>> extension deleting the download item and causing DownloadItemView and >>> everything within it to disappear. >>> >>> http://codereview.chromium.org/8757007/diff/8001/chrome/browser/ui/views/down... >>> File chrome/browser/ui/views/download/download_item_view.cc (left): >>> >>> http://codereview.chromium.org/8757007/diff/8001/chrome/browser/ui/views/down... >>> chrome/browser/ui/views/download/download_item_view.cc:226: >>> context_menu_->Stop(); >>> On 2011/12/01 19:49:22, Dominic Mazzoni wrote: >>>> >>>> Why are you deleting this? Was it wrong or is it no longer needed? >>> >>> It was used as a signal that DownloadItemView (and everything down the >>> hierarchy) was deleted. DownloadShelfContextMenu would set its >>> download_item_ member to NULL in response. >>> >>> In DownloadItemView::ShowContextMenu(), we were calling >>> context_menu_->Run() and afterwards checking >>> (context_menu_->download_item()) to see if it was safe to access 'this' >>> and if so change the pushed state of the drop down icon. >>> >>> I thought this was wrong because: >>> >>> - DownloadItemView::context_menu_ is a >>> scoped_ptr<DownloadShelfContextMenu>, so the DownloadShelfContextMenu is >>> deleted after Stop() is called. >>> >>> - The context_menu_->download() check accesses the potentially deleted >>> *this, and the potentially deleted DownloadShelfContextMenu. >>> >>> The addition of the "Discard" option to the drop down menu made it so >>> that we were reliably hitting this case. We now post a non-nestable >>> task with a WeakPtr to the DownloadItemView to un-press the drop down >>> button, which simply doesn't run if the DownloadItemView was deleted as >>> a result of running the drop down menu. >>> >>> http://codereview.chromium.org/8757007/diff/8001/chrome/browser/ui/views/down... >>> chrome/browser/ui/views/download/download_item_view.cc:628: >>> drop_down_image_set = NULL; // No drop-down in dangerous mode. >>> On 2011/12/01 19:49:22, Dominic Mazzoni wrote: >>>> >>>> Please preserve this comment somewhere. >>> >>> Done. >>> >>> http://codereview.chromium.org/8757007/diff/8001/chrome/browser/ui/views/down... >>> File chrome/browser/ui/views/download/download_item_view.cc (right): >>> >>> http://codereview.chromium.org/8757007/diff/8001/chrome/browser/ui/views/down... >>> chrome/browser/ui/views/download/download_item_view.cc:196: >>> malicious_mode_body_image_set_ = malicious_mode_body_image_set; >>> On 2011/12/01 19:49:22, Dominic Mazzoni wrote: >>>> >>>> Can you just say malicious_mode_body_image_set_ = >>> >>> dangerous_mode_body_image_set? >>>> >>>> They seem to be the same. If not, could you add a Clone() or a copy >>> >>> constructor >>>> >>>> so this can be easier? >>> >>> Done. I think you meant normal_body_image_set. Currently the body >>> background looks the same in malicious and normal modes. >>> >>> http://codereview.chromium.org/8757007/diff/8001/chrome/browser/ui/views/down... >>> chrome/browser/ui/views/download/download_item_view.cc:390: if (mode_ == >>> DANGEROUS_MODE) >>> On 2011/12/01 19:49:22, Dominic Mazzoni wrote: >>>> >>>> How about making this based on whether save_button is showing or not, >>> >>> like: >>> >>>> if (save_button_) >>>> width += button_size.width() + kButtonPadding; >>> >>>> ...since that's the real difference, right? >>> >>> Done. >>> >>> http://codereview.chromium.org/8757007/diff/8001/chrome/browser/ui/views/down... >>> chrome/browser/ui/views/download/download_item_view.cc:498: if >>> (!IsShowingWarningDialog()) >>> On 2011/12/01 19:50:46, rdsmith wrote: >>>> >>>> How can we be in dangerous mode and not showing a warning dialog? >>> >>> We get here when we aren't showing a warning dialog as well. We exit >>> early only if we are in DANGEROUS_MODE because no part of the view >>> responds to mouse hover in that mode. >>> >>> http://codereview.chromium.org/8757007/diff/8001/chrome/browser/ui/views/down... >>> chrome/browser/ui/views/download/download_item_view.cc:616: void >>> DownloadItemView::OnPaint(gfx::Canvas* canvas) { >>> On 2011/12/01 19:49:22, Dominic Mazzoni wrote: >>>> >>>> This logic is getting a little more confusing to follow, please add a >>> >>> couple of >>>> >>>> very brief comments that help clarify: >>> >>>> * Why do dangerous and malicious have a different body set, and why >>> >>> don't they >>>> >>>> distinguish between pushed or not? >>>> * Note that there's a drop-down in nromal and malicious mode, but in >>> >>> dangerous >>>> >>>> mode there's no drop-down >>> >>> >>> I added a comment before OnPaint() that gives an overview of what we are >>> going to paint, and also added a few comments within OnPaint(). Let me >>> know if that is sufficient. >>> >>> http://codereview.chromium.org/8757007/diff/8001/chrome/browser/ui/views/down... >>> chrome/browser/ui/views/download/download_item_view.cc:918: if >>> ((IsShowingWarningDialog() || body_state_ == body_state) && >>> On 2011/12/01 19:49:22, Dominic Mazzoni wrote: >>>> >>>> I don't understand why IsShowingWarningDialog() is grouped with >>> >>> (body_state_ == >>>> >>>> body_state), can you add a comment? Could you split it into two >>> >>> separate if >>>> >>>> statements so the logic is more clear? >>> >>> If we are showing the warning dialog, we don't change the body state. So >>> it's equivalent to the case where the new and the current body states >>> are the same. Reorganized code to make this clearer. >>> >>> http://codereview.chromium.org/8757007/diff/8001/chrome/browser/ui/views/down... >>> chrome/browser/ui/views/download/download_item_view.cc:1178: new_name = >>> dangerous_download_label_->GetText(); >>> On 2011/12/01 19:49:22, Dominic Mazzoni wrote: >>>> >>>> Does this need to be updated for dangerous vs malicious? >>> >>> For both cases, the accessible text is going to be the text that was set >>> in the dangerous_download_label_. >>> >>> http://codereview.chromium.org/8757007/ >>> >> >> >> >> -- >> -|||-- >> > > > > -- > -|||-- > -- -|||--
On 2011/12/02 01:16:09, noelutz wrote: > Argh. no 'to be': > > "[foo.exe|This file] appears malicious. [Discard][>]" Awesome!
LGTM I'm happy with the changes to the downloads view and other UI-related changes. Thanks. Make sure that the non-UI-related changes to the downloads logic have been reviewed by someone who has sufficient expertise. http://codereview.chromium.org/8757007/diff/8001/chrome/app/generated_resourc... File chrome/app/generated_resources.grd (right): http://codereview.chromium.org/8757007/diff/8001/chrome/app/generated_resourc... chrome/app/generated_resources.grd:2492: <message name="IDS_DOWNLOAD_MENU_DISCARD" On 2011/12/02 00:05:04, asanka wrote: > On 2011/12/01 19:49:22, Dominic Mazzoni wrote: > > These look like duplicates, merge error? > > These are the use_titlecase and !use_titlecase cases. Only the "Learn more" > string is different. Ah, thanks - I didn't notice the <if>. http://codereview.chromium.org/8757007/diff/8002/chrome/browser/download/down... File chrome/browser/download/download_shelf_context_menu.cc (right): http://codereview.chromium.org/8757007/diff/8002/chrome/browser/download/down... chrome/browser/download/download_shelf_context_menu.cc:31: DownloadStateInfo::DANGEROUS_CONTENT) { Nit: Indent this 4 more spaces http://codereview.chromium.org/8757007/diff/8002/chrome/browser/ui/views/down... File chrome/browser/ui/views/download/download_item_view.cc (right): http://codereview.chromium.org/8757007/diff/8002/chrome/browser/ui/views/down... chrome/browser/ui/views/download/download_item_view.cc:607: // The DownloadItemView can be in three major modes (NORMAL_MODE, DANGEROUS_MODE Thanks, great comments. Love the ASCII art.
On 2011/12/02 20:15:45, Dominic Mazzoni wrote: > LGTM > > I'm happy with the changes to the downloads view and other UI-related changes. > Thanks. > > Make sure that the non-UI-related changes to the downloads logic have been > reviewed by someone who has sufficient expertise. Dominic: Just checking (since I'm almost certainly the "someone"): You're talking about changes outside of this CL, right? I think of this CL has being almost completely UI changes. > > http://codereview.chromium.org/8757007/diff/8001/chrome/app/generated_resourc... > File chrome/app/generated_resources.grd (right): > > http://codereview.chromium.org/8757007/diff/8001/chrome/app/generated_resourc... > chrome/app/generated_resources.grd:2492: <message > name="IDS_DOWNLOAD_MENU_DISCARD" > On 2011/12/02 00:05:04, asanka wrote: > > On 2011/12/01 19:49:22, Dominic Mazzoni wrote: > > > These look like duplicates, merge error? > > > > These are the use_titlecase and !use_titlecase cases. Only the "Learn more" > > string is different. > > Ah, thanks - I didn't notice the <if>. > > http://codereview.chromium.org/8757007/diff/8002/chrome/browser/download/down... > File chrome/browser/download/download_shelf_context_menu.cc (right): > > http://codereview.chromium.org/8757007/diff/8002/chrome/browser/download/down... > chrome/browser/download/download_shelf_context_menu.cc:31: > DownloadStateInfo::DANGEROUS_CONTENT) { > Nit: Indent this 4 more spaces > > http://codereview.chromium.org/8757007/diff/8002/chrome/browser/ui/views/down... > File chrome/browser/ui/views/download/download_item_view.cc (right): > > http://codereview.chromium.org/8757007/diff/8002/chrome/browser/ui/views/down... > chrome/browser/ui/views/download/download_item_view.cc:607: // The > DownloadItemView can be in three major modes (NORMAL_MODE, DANGEROUS_MODE > Thanks, great comments. Love the ASCII art.
On Fri, Dec 2, 2011 at 12:19 PM, <rdsmith@chromium.org> wrote: > Dominic: Just checking (since I'm almost certainly the "someone"): You're > talking about changes outside of this CL, right? I think of this CL has > being > almost completely UI changes. Yes, I'm happy with the UI changes. Really the only thing I don't feel like I fully understand is the file deletion scenario. The explanation made sense, but have you looked at it carefully? Specifically, if the user chooses to discard a malicious file, is it deleted properly, with no corner cases or race conditions that you can envision? - Dominic
On 2011/12/02 21:20:40, Dominic Mazzoni wrote:
> On Fri, Dec 2, 2011 at 12:19 PM, <mailto:rdsmith@chromium.org> wrote:
>
> > Dominic: Just checking (since I'm almost certainly the "someone"): You're
> > talking about changes outside of this CL, right? I think of this CL has
> > being
> > almost completely UI changes.
>
>
> Yes, I'm happy with the UI changes.
>
> Really the only thing I don't feel like I fully understand is the file
> deletion scenario. The explanation made sense, but have you looked at it
> carefully? Specifically, if the user chooses to discard a malicious file,
> is it deleted properly, with no corner cases or race conditions that you
> can envision?
We're the downloads system; race conditions are our middle names :-J. Having
said that, I'm not certain what kinds of race conditions you're worried about.
The download file is owned by the DownloadFile object (lives on the file thread)
and is given a not very useful name ("Unconfirmed %d.download") until it gets
the bill of health from the interactions on the UI thread. The rename to the
final name is completely under the UI threads control, and can't happen until
the download is validated. If for some reason we never get back to the
DownloadFile object, at DownloadFile destruction time the file is automatically
deleted (unless it's been specifically "released" by message from the UI
thread), and when the message Asanka refers to hits the file thread, the file is
deleted right then. If the user goes to the file system and copies or renames
that Unconfirmed %d.crdownload file before he cancels the dangerous download,
the user can indeed get at the data. But I have a hard time imagining that
that's the kind of race you're worried about.
Willing to flesh out your concern a bit more/give me an example? This may be a
case that I'm so mired in the trees I can't see the forest, but I think we've
thought about the races in the system a fair amount, and (more to the point) I
think that this CL doesn't affect them at all. Until the very end of the
download process, we don't commit the download (via rename to useful name); we
only do that once everything we're waiting on (specifically including dangerous
download validation) has happened.
>
> - Dominic
http://codereview.chromium.org/8757007/diff/8002/chrome/browser/download/down... File chrome/browser/download/download_shelf_context_menu.cc (right): http://codereview.chromium.org/8757007/diff/8002/chrome/browser/download/down... chrome/browser/download/download_shelf_context_menu.cc:31: DownloadStateInfo::DANGEROUS_CONTENT) { On 2011/12/02 20:15:45, Dominic Mazzoni wrote: > Nit: Indent this 4 more spaces Done. http://codereview.chromium.org/8757007/diff/8002/chrome/browser/ui/views/down... File chrome/browser/ui/views/download/download_item_view.cc (right): http://codereview.chromium.org/8757007/diff/8002/chrome/browser/ui/views/down... chrome/browser/ui/views/download/download_item_view.cc:607: // The DownloadItemView can be in three major modes (NORMAL_MODE, DANGEROUS_MODE On 2011/12/02 20:15:45, Dominic Mazzoni wrote: > Thanks, great comments. Love the ASCII art. Thanks! http://codereview.chromium.org/8757007/diff/18001/chrome/app/generated_resour... File chrome/app/generated_resources.grd (right): http://codereview.chromium.org/8757007/diff/18001/chrome/app/generated_resour... chrome/app/generated_resources.grd:2289: http://www.google.com/support/chrome/bin/answer.py?answer=99020&hl=[GRITL... This is a general safe browsing help center URL. It will be here until we have a final help center page for this feature.
LGTM (in case it wasn't clear) Thanks for the explanation! I trust that you guys have thought about all of the possible race conditions, etc. - the only thing that felt non-UI about was that the original code called Cancel() and then Delete(), and that wasn't obvious to me if that was safe. That's fixed now, so I don't see anything else that worries me like that. Thanks, - Dominic On Fri, Dec 2, 2011 at 1:36 PM, <rdsmith@chromium.org> wrote: > On 2011/12/02 21:20:40, Dominic Mazzoni wrote: > > On Fri, Dec 2, 2011 at 12:19 PM, <mailto:rdsmith@chromium.org> wrote: >> > > > Dominic: Just checking (since I'm almost certainly the "someone"): >> You're >> > talking about changes outside of this CL, right? I think of this CL has >> > being >> > almost completely UI changes. >> > > > Yes, I'm happy with the UI changes. >> > > Really the only thing I don't feel like I fully understand is the file >> deletion scenario. The explanation made sense, but have you looked at it >> carefully? Specifically, if the user chooses to discard a malicious file, >> is it deleted properly, with no corner cases or race conditions that you >> can envision? >> > > We're the downloads system; race conditions are our middle names :-J. > Having > said that, I'm not certain what kinds of race conditions you're worried > about. > The download file is owned by the DownloadFile object (lives on the file > thread) > and is given a not very useful name ("Unconfirmed %d.download") until it > gets > the bill of health from the interactions on the UI thread. The rename to > the > final name is completely under the UI threads control, and can't happen > until > the download is validated. If for some reason we never get back to the > DownloadFile object, at DownloadFile destruction time the file is > automatically > deleted (unless it's been specifically "released" by message from the UI > thread), and when the message Asanka refers to hits the file thread, the > file is > deleted right then. If the user goes to the file system and copies or > renames > that Unconfirmed %d.crdownload file before he cancels the dangerous > download, > the user can indeed get at the data. But I have a hard time imagining that > that's the kind of race you're worried about. > > Willing to flesh out your concern a bit more/give me an example? This may > be a > case that I'm so mired in the trees I can't see the forest, but I think > we've > thought about the races in the system a fair amount, and (more to the > point) I > think that this CL doesn't affect them at all. Until the very end of the > download process, we don't commit the download (via rename to useful > name); we > only do that once everything we're waiting on (specifically including > dangerous > download validation) has happened. > > > - Dominic >> > > > > http://codereview.chromium.**org/8757007/<http://codereview.chromium.org/8757... >
FYI: I moved the "learn more" URL from generated_resources to url_constants. The URL is non-translatable, and the norm seems to be to use google_util::AppendGoogleLocaleParam() for appending the locale parameter to help center URLs.
*Some* try runs aren't showing up for patchset 7. So adding build URLs manually: win: http://build.chromium.org/p/tryserver.chromium/builders/win/builds/6528 linux: http://build.chromium.org/p/tryserver.chromium/builders/linux/builds/6181 mac: http://build.chromium.org/p/tryserver.chromium/builders/mac/builds/6106 linux_view: http://build.chromium.org/p/tryserver.chromium/builders/linux_view/builds/977 linux_chromeos: http://build.chromium.org/p/tryserver.chromium/builders/linux_chromeos/builds...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/asanka@chromium.org/8757007/22001
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/asanka@chromium.org/8757007/25001
Change committed as 112954 |
