Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
147593002:
Implement new dangerous download reporting dialog for UNCOMMON_DOWNLOAD, in Views (Closed)
|
Created:
6 years, 11 months ago by felt Modified:
6 years, 10 months ago CC:
msw, chromium-reviews, tfarina, benjhayden+dwatch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Visibility:
Public. |
DescriptionImplement new dangerous download reporting dialog for UNCOMMON_DOWNLOAD, in Views
Adds a new dialog, DownloadFeedbackDialogView, to ask people whether they want to participate in the upload program. Currently only hooked up to UNCOMMON_DOWNLOAD.
Still TODO:
- need to add dialog to DANGEROUS_HOST as well
- need to add new histogram for dialog once DANGEROUS_HOST is in place
--------------
How to test [Win & CrOS only]:
1. Visit http://testsafebrowsing.appspot.com/, click on #5, then click discard, then click any choice in the dialog
2. If you repeat #1, you shouldn't see the dialog again unless you delete the preferences file in the User Data directory
3. If you do #1 in two separate windows, only one of them should trigger the dialog
BUG=312533
TBR=asanka@chromium.org
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=249796
Patch Set 1 : [Initial patchset for testing] #Patch Set 2 : Added prefs constraints #Patch Set 3 : Moved clickjacking.discard_download up #Patch Set 4 : Remove now-unneeded entry in context menu #Patch Set 5 : Fixed similarity issue [now ready for review] #
Total comments: 5
Patch Set 6 : changes for mattm's comments #Patch Set 7 : fix typo #
Total comments: 57
Patch Set 8 : fixes for pkasting's comments #Patch Set 9 : More fixes for comments #Patch Set 10 : MODAL_TYPE_WINDOW #
Total comments: 4
Patch Set 11 : Tweaks for Window issue #Patch Set 12 : Update out-of-date comment #Patch Set 13 : Now using SetUserData #Patch Set 14 : Testing bugfix #
Total comments: 6
Patch Set 15 : Improved DialogStatusData #
Total comments: 4
Patch Set 16 : set_currently_shown #Patch Set 17 : Exclude OffTheRecord profiles #Patch Set 18 : Tweak to userdata code #Patch Set 19 : Added logging statements to test on Win #Patch Set 20 : Remove logging statement #
Total comments: 3
Patch Set 21 : Move the UserData check into the dialog itself #
Total comments: 8
Patch Set 22 : Header file switcharoos #
Total comments: 2
Patch Set 23 : Include fix #Messages
Total messages: 56 (0 generated)
Asanka, Matt -- cc'ing you as an FYI, in case you want to look over this as well.
Hi Mike, Can you please review this CL, which adds a new dangerous download reporting dialog? You can see both the original mocks & a screenshot of the implementation on the bug. A note: download_feedback_dialog_view.h is a new file, but code review is acting like it's an altered version of download_in_progress_dialog_view.h. They are similar, but I haven't actually altered the file download_in_progress_dialog_view.h. Do you have any idea how to fix this? Thanks, Adrienne
Felt, please try adjusting the similarity passed to git cl upload. I think a higher will do the trick. Say 80, 90. $ git cl upload -t" " --similarity=90
On 2014/01/26 16:41:08, tfarina wrote: > Felt, please try adjusting the similarity passed to git cl upload. I think a > higher will do the trick. Say 80, 90. > > $ git cl upload -t" " --similarity=90 thanks tfarina, that fixed it.
https://codereview.chromium.org/147593002/diff/200001/chrome/browser/ui/views... File chrome/browser/ui/views/download/download_feedback_dialog_view.cc (right): https://codereview.chromium.org/147593002/diff/200001/chrome/browser/ui/views... chrome/browser/ui/views/download/download_feedback_dialog_view.cc:21: // been shown before. Uses USER_DISABLED as a safe default. So it's intentional that if the browser is closed without selecting, it'll never come up again? https://codereview.chromium.org/147593002/diff/200001/chrome/browser/ui/views... File chrome/browser/ui/views/download/download_item_view.cc (right): https://codereview.chromium.org/147593002/diff/200001/chrome/browser/ui/views... chrome/browser/ui/views/download/download_item_view.cc:576: case DownloadFeedbackDialogView::kMaxValue: { default:
https://codereview.chromium.org/147593002/diff/200001/chrome/browser/ui/views... File chrome/browser/ui/views/download/download_feedback_dialog_view.cc (right): https://codereview.chromium.org/147593002/diff/200001/chrome/browser/ui/views... chrome/browser/ui/views/download/download_feedback_dialog_view.cc:21: // been shown before. Uses USER_DISABLED as a safe default. On 2014/01/27 08:03:33, mattm wrote: > So it's intentional that if the browser is closed without selecting, it'll never > come up again? hmmm. I guess that would be bad. my concern was that closing the dialog using the "x" or closing the window need a safe default. i'll test those cases out. https://codereview.chromium.org/147593002/diff/200001/chrome/browser/ui/views... File chrome/browser/ui/views/download/download_item_view.cc (right): https://codereview.chromium.org/147593002/diff/200001/chrome/browser/ui/views... chrome/browser/ui/views/download/download_item_view.cc:576: case DownloadFeedbackDialogView::kMaxValue: { On 2014/01/27 08:03:33, mattm wrote: > default: do you need default for enums? won't you get a compiler error if you miss a case w/ an enum? added anyway
https://codereview.chromium.org/147593002/diff/200001/chrome/browser/ui/views... File chrome/browser/ui/views/download/download_item_view.cc (right): https://codereview.chromium.org/147593002/diff/200001/chrome/browser/ui/views... chrome/browser/ui/views/download/download_item_view.cc:576: case DownloadFeedbackDialogView::kMaxValue: { On 2014/01/27 20:06:59, felt wrote: > On 2014/01/27 08:03:33, mattm wrote: > > default: > > do you need default for enums? won't you get a compiler error if you miss a case > w/ an enum? added anyway Yeah, that's true. I was meaning that default: may be cleaner than specifying case MaxValue, but you don't need both. Feel free to ignore that comment if you want.
Mike, could you please take a look at this before the ski trip Thurs/Fri? Thanks, Adrienne
Hi Peter, could you please review this new dialog for Safe Browsing download checks?
On 2014/02/03 04:41:46, felt wrote: > Hi Peter, could you please review this new dialog for Safe Browsing download > checks? Could you restate that request? I'm not sure (a) which files I'm reviewing or (b) what, precisely I'm looking at in those files Are you wanting a full review of everything? If so, what did Matt review?
On 2014/02/03 22:37:11, Peter Kasting wrote: > On 2014/02/03 04:41:46, felt wrote: > > Hi Peter, could you please review this new dialog for Safe Browsing download > > checks? > > Could you restate that request? I'm not sure > (a) which files I'm reviewing or > (b) what, precisely I'm looking at in those files > > Are you wanting a full review of everything? If so, what did Matt review? Basically the entire CL falls in a directory that you are the OWNER of. So yes, I'd like a full review of everything please. I cc'ed Matt as an FYI, I get the impression that he skimmed it but did not do a full review.
https://codereview.chromium.org/147593002/diff/270001/chrome/app/generated_re... File chrome/app/generated_resources.grd (right): https://codereview.chromium.org/147593002/diff/270001/chrome/app/generated_re... chrome/app/generated_resources.grd:2158: + <message name="IDS_FEEDBACK_SERVICE_DIALOG_OK_BUTTON_LABEL" desc="Button text for opting in"> Nit: How about: "Button that elects to upload suspected malicious files for analysis" (and similarly but with "that elects not to upload" for the other string below) https://codereview.chromium.org/147593002/diff/270001/chrome/browser/download... File chrome/browser/download/download_shelf_context_menu.cc (left): https://codereview.chromium.org/147593002/diff/270001/chrome/browser/download... chrome/browser/download/download_shelf_context_menu.cc:354: maybe_malicious_download_menu_model_->AddItemWithStringId( How come we removed this? https://codereview.chromium.org/147593002/diff/270001/chrome/browser/ui/views... File chrome/browser/ui/views/download/download_feedback_dialog_view.cc (right): https://codereview.chromium.org/147593002/diff/270001/chrome/browser/ui/views... chrome/browser/ui/views/download/download_feedback_dialog_view.cc:20: // Enforce the constraint that this dialog should only be shown if it hasn't Nit: DCHECKs don't really "enforce" anything, so I'd drop the "Enforce the constraint that". https://codereview.chromium.org/147593002/diff/270001/chrome/browser/ui/views... chrome/browser/ui/views/download/download_feedback_dialog_view.cc:24: DownloadFeedbackDialogView* window = new DownloadFeedbackDialogView( Nit: Breaking after '=' instead of '(' seems a little nicer https://codereview.chromium.org/147593002/diff/270001/chrome/browser/ui/views... chrome/browser/ui/views/download/download_feedback_dialog_view.cc:35: title_text_ = l10n_util::GetStringUTF16(IDS_FEEDBACK_SERVICE_DIALOG_TITLE); Nit: All these strings can be set in the initializer list. https://codereview.chromium.org/147593002/diff/270001/chrome/browser/ui/views... chrome/browser/ui/views/download/download_feedback_dialog_view.cc:36: base::string16 explanation_text = l10n_util::GetStringUTF16( Nit: You can just inline this into the next statement; doing this will allow you to set |explanation_box_view_| in the initializer list. https://codereview.chromium.org/147593002/diff/270001/chrome/browser/ui/views... chrome/browser/ui/views/download/download_feedback_dialog_view.cc:73: return ui::MODAL_TYPE_SYSTEM; Why does this need to be system-modal? https://codereview.chromium.org/147593002/diff/270001/chrome/browser/ui/views... File chrome/browser/ui/views/download/download_feedback_dialog_view.h (right): https://codereview.chromium.org/147593002/diff/270001/chrome/browser/ui/views... chrome/browser/ui/views/download/download_feedback_dialog_view.h:22: // or UNCOMMON_DOWNLOAD. User should only see this dialog once! Nit: User -> The user I would probably avoid the "!" at the end too. https://codereview.chromium.org/147593002/diff/270001/chrome/browser/ui/views... chrome/browser/ui/views/download/download_feedback_dialog_view.h:25: // Used to set prefs::kSafeBrowsingDownloadReportingEnabled. Nit: "Possible values for the kSafeBrowsingDownloadReportingEnabled pref."? Also, maybe the pref should be called "kSafeBrowsingDownloadReportingStatus"? https://codereview.chromium.org/147593002/diff/270001/chrome/browser/ui/views... chrome/browser/ui/views/download/download_feedback_dialog_view.h:26: enum Response { Nit: Since not all of these are responses, what about calling this DownloadReportingStatus, or DownloadReportingPrefValues? https://codereview.chromium.org/147593002/diff/270001/chrome/browser/ui/views... chrome/browser/ui/views/download/download_feedback_dialog_view.h:27: kNeverShown, Nit: kDialogNotYetShown? https://codereview.chromium.org/147593002/diff/270001/chrome/browser/ui/views... chrome/browser/ui/views/download/download_feedback_dialog_view.h:28: kUserDisabled, // Set by Show() as a default. Nit: kDownloadReportingEnabled? (And similarly with disabled) https://codereview.chromium.org/147593002/diff/270001/chrome/browser/ui/views... chrome/browser/ui/views/download/download_feedback_dialog_view.h:49: // views::WidgetDelegate: Nit: You don't directly subclass WidgetDelegate. Put these overrides with the others, in the appropriate order. (e.g. if DialogDelegate is a subclass of WidgetDelegate and does not override WidgetDelegate::Foo(), you should list Foo() before any of the DialogDelegate overrides, in the order in which it appears in WidgetDelegate. If DialogDelegate does override it, place it with other DialogDelegate functions relative to where it is listed in the DialogDelegate header.) https://codereview.chromium.org/147593002/diff/270001/chrome/browser/ui/views... File chrome/browser/ui/views/download/download_item_view.cc (right): https://codereview.chromium.org/147593002/diff/270001/chrome/browser/ui/views... chrome/browser/ui/views/download/download_item_view.cc:560: case DownloadFeedbackDialogView::kNeverShown: { Nit: {} not necessary on any of these cases. https://codereview.chromium.org/147593002/diff/270001/chrome/browser/ui/views... chrome/browser/ui/views/download/download_item_view.cc:576: case DownloadFeedbackDialogView::kMaxValue: Having this value buys you nothing over just having the "default:" case below. Eliminate this line and eliminate the value from the original enum. https://codereview.chromium.org/147593002/diff/270001/chrome/browser/ui/views... chrome/browser/ui/views/download/download_item_view.cc:920: void DownloadItemView::BeginDownloadFeedbackWrapper(bool user_enabled) { Nit: If you make the argument to this be the enum type instead of a bool, then not only is the meaning of the argument slightly clearer, but you can slightly simplify the switch statement above by doing: case DownloadFeedbackDialogView::kUserEnabled: case DownloadFeedbackDialogView::kUserDisabled: BeginDownloadFeedbackWrapper(pref_value); break; https://codereview.chromium.org/147593002/diff/270001/chrome/browser/ui/views... chrome/browser/ui/views/download/download_item_view.cc:924: // WARNING: 'this' is deleted at this point. Don't access 'this'. Nit: Shorter: if (!user_enabled || !BeginDownloadFeedback()) download()->Remove(); // WARNING: |this| is now deleted! (This is also slightly clearer that BeginDownloadFeedback() will have deleted |this| if it returns true.) https://codereview.chromium.org/147593002/diff/270001/chrome/browser/ui/views... File chrome/browser/ui/views/download/download_item_view.h (right): https://codereview.chromium.org/147593002/diff/270001/chrome/browser/ui/views... chrome/browser/ui/views/download/download_item_view.h:151: // false is returned, nothing has changed. Nit: While here, maybe change to: Submits the downloaded file to the safebrowsing download feedback service. Returns whether submission was successful. On successful submission, |this| and the DownloadItem will have been deleted. https://codereview.chromium.org/147593002/diff/270001/chrome/browser/ui/views... chrome/browser/ui/views/download/download_item_view.h:152: bool BeginDownloadFeedback(); Nit: This function name and the one below seem somewhat confusing. What about: bool SubmitDownloadToFeedbackService(); and void PossiblySubmitDownloadToFeedbackService(); ? https://codereview.chromium.org/147593002/diff/270001/chrome/browser/ui/views... chrome/browser/ui/views/download/download_item_view.h:156: void BeginDownloadFeedbackWrapper(bool user_enabled); Nit: Header declaration order does not mach .cc definition order https://codereview.chromium.org/147593002/diff/270001/chrome/browser/ui/views... File chrome/browser/ui/views/download/download_shelf_view.h (right): https://codereview.chromium.org/147593002/diff/270001/chrome/browser/ui/views... chrome/browser/ui/views/download/download_shelf_view.h:57: content::PageNavigator* GetNavigator(); Nit: While here: There's no need for this function. All callers can just call browser() instead, which is also public. Nuke this. Note: If this comment and/or the last one in this file would result in large diffs, feel free to do them as separate CLs. https://codereview.chromium.org/147593002/diff/270001/chrome/browser/ui/views... chrome/browser/ui/views/download/download_shelf_view.h:60: BrowserView* GetParentWindowView(); Nit: This function should be inlined and named like: BrowserView* get_parent() { return parent_; } https://codereview.chromium.org/147593002/diff/270001/chrome/browser/ui/views... chrome/browser/ui/views/download/download_shelf_view.h:85: virtual Browser* browser() const OVERRIDE; Nit: While here: Virtual functions must never be named unix_hacker()-style. Capitalize this (and in the remainder of the class hierarchy).
https://codereview.chromium.org/147593002/diff/270001/chrome/app/generated_re... File chrome/app/generated_resources.grd (right): https://codereview.chromium.org/147593002/diff/270001/chrome/app/generated_re... chrome/app/generated_resources.grd:2158: + <message name="IDS_FEEDBACK_SERVICE_DIALOG_OK_BUTTON_LABEL" desc="Button text for opting in"> On 2014/02/04 21:56:33, Peter Kasting wrote: > Nit: How about: "Button that elects to upload suspected malicious files for > analysis" (and similarly but with "that elects not to upload" for the other > string below) Done. https://codereview.chromium.org/147593002/diff/270001/chrome/browser/download... File chrome/browser/download/download_shelf_context_menu.cc (left): https://codereview.chromium.org/147593002/diff/270001/chrome/browser/download... chrome/browser/download/download_shelf_context_menu.cc:354: maybe_malicious_download_menu_model_->AddItemWithStringId( On 2014/02/04 21:56:33, Peter Kasting wrote: > How come we removed this? Previously, the main button said "Report & Discard" and the context menu had the alternative option "Discard." Now the main button just says "Discard" and the reporting question happens in the dialog. It would be redundant to have "Discard" as the main button with "Discard" also in the context menu. https://codereview.chromium.org/147593002/diff/270001/chrome/browser/ui/views... File chrome/browser/ui/views/download/download_feedback_dialog_view.cc (right): https://codereview.chromium.org/147593002/diff/270001/chrome/browser/ui/views... chrome/browser/ui/views/download/download_feedback_dialog_view.cc:20: // Enforce the constraint that this dialog should only be shown if it hasn't On 2014/02/04 21:56:33, Peter Kasting wrote: > Nit: DCHECKs don't really "enforce" anything, so I'd drop the "Enforce the > constraint that". Done. https://codereview.chromium.org/147593002/diff/270001/chrome/browser/ui/views... chrome/browser/ui/views/download/download_feedback_dialog_view.cc:24: DownloadFeedbackDialogView* window = new DownloadFeedbackDialogView( On 2014/02/04 21:56:33, Peter Kasting wrote: > Nit: Breaking after '=' instead of '(' seems a little nicer Done. https://codereview.chromium.org/147593002/diff/270001/chrome/browser/ui/views... chrome/browser/ui/views/download/download_feedback_dialog_view.cc:35: title_text_ = l10n_util::GetStringUTF16(IDS_FEEDBACK_SERVICE_DIALOG_TITLE); On 2014/02/04 21:56:33, Peter Kasting wrote: > Nit: All these strings can be set in the initializer list. Done. https://codereview.chromium.org/147593002/diff/270001/chrome/browser/ui/views... chrome/browser/ui/views/download/download_feedback_dialog_view.cc:36: base::string16 explanation_text = l10n_util::GetStringUTF16( On 2014/02/04 21:56:33, Peter Kasting wrote: > Nit: You can just inline this into the next statement; doing this will allow you > to set |explanation_box_view_| in the initializer list. Done. https://codereview.chromium.org/147593002/diff/270001/chrome/browser/ui/views... chrome/browser/ui/views/download/download_feedback_dialog_view.cc:73: return ui::MODAL_TYPE_SYSTEM; On 2014/02/04 21:56:33, Peter Kasting wrote: > Why does this need to be system-modal? I was copying how similar dialogs seem to work. I admit to not entirely understanding the types of modality. What is your recommendation? https://codereview.chromium.org/147593002/diff/270001/chrome/browser/ui/views... File chrome/browser/ui/views/download/download_feedback_dialog_view.h (right): https://codereview.chromium.org/147593002/diff/270001/chrome/browser/ui/views... chrome/browser/ui/views/download/download_feedback_dialog_view.h:22: // or UNCOMMON_DOWNLOAD. User should only see this dialog once! On 2014/02/04 21:56:33, Peter Kasting wrote: > Nit: User -> The user > > I would probably avoid the "!" at the end too. Done. https://codereview.chromium.org/147593002/diff/270001/chrome/browser/ui/views... chrome/browser/ui/views/download/download_feedback_dialog_view.h:25: // Used to set prefs::kSafeBrowsingDownloadReportingEnabled. On 2014/02/04 21:56:33, Peter Kasting wrote: > Nit: "Possible values for the kSafeBrowsingDownloadReportingEnabled pref."? Done. > > Also, maybe the pref should be called "kSafeBrowsingDownloadReportingStatus"? I don't mind renaming the pref, but I'd rather rename the pref in another CL. https://codereview.chromium.org/147593002/diff/270001/chrome/browser/ui/views... chrome/browser/ui/views/download/download_feedback_dialog_view.h:26: enum Response { On 2014/02/04 21:56:33, Peter Kasting wrote: > Nit: Since not all of these are responses, what about calling this > DownloadReportingStatus, or DownloadReportingPrefValues? Done. https://codereview.chromium.org/147593002/diff/270001/chrome/browser/ui/views... chrome/browser/ui/views/download/download_feedback_dialog_view.h:27: kNeverShown, On 2014/02/04 21:56:33, Peter Kasting wrote: > Nit: kDialogNotYetShown? Done. https://codereview.chromium.org/147593002/diff/270001/chrome/browser/ui/views... chrome/browser/ui/views/download/download_feedback_dialog_view.h:28: kUserDisabled, // Set by Show() as a default. On 2014/02/04 21:56:33, Peter Kasting wrote: > Nit: kDownloadReportingEnabled? (And similarly with disabled) Done. https://codereview.chromium.org/147593002/diff/270001/chrome/browser/ui/views... chrome/browser/ui/views/download/download_feedback_dialog_view.h:49: // views::WidgetDelegate: On 2014/02/04 21:56:33, Peter Kasting wrote: > Nit: You don't directly subclass WidgetDelegate. Put these overrides with the > others, in the appropriate order. (e.g. if DialogDelegate is a subclass of > WidgetDelegate and does not override WidgetDelegate::Foo(), you should list > Foo() before any of the DialogDelegate overrides, in the order in which it > appears in WidgetDelegate. If DialogDelegate does override it, place it with > other DialogDelegate functions relative to where it is listed in the > DialogDelegate header.) Done. https://codereview.chromium.org/147593002/diff/270001/chrome/browser/ui/views... File chrome/browser/ui/views/download/download_item_view.cc (right): https://codereview.chromium.org/147593002/diff/270001/chrome/browser/ui/views... chrome/browser/ui/views/download/download_item_view.cc:560: case DownloadFeedbackDialogView::kNeverShown: { On 2014/02/04 21:56:33, Peter Kasting wrote: > Nit: {} not necessary on any of these cases. Done, but doesn't the style guide say either with or without {} is fine? https://codereview.chromium.org/147593002/diff/270001/chrome/browser/ui/views... chrome/browser/ui/views/download/download_item_view.cc:576: case DownloadFeedbackDialogView::kMaxValue: On 2014/02/04 21:56:33, Peter Kasting wrote: > Having this value buys you nothing over just having the "default:" case below. > Eliminate this line and eliminate the value from the original enum. Switching to just "default", but I need kMaxValue so that it can be histogrammed in a subsequent CL. https://codereview.chromium.org/147593002/diff/270001/chrome/browser/ui/views... chrome/browser/ui/views/download/download_item_view.cc:924: // WARNING: 'this' is deleted at this point. Don't access 'this'. On 2014/02/04 21:56:33, Peter Kasting wrote: > Nit: Shorter: > > if (!user_enabled || !BeginDownloadFeedback()) > download()->Remove(); > // WARNING: |this| is now deleted! > > (This is also slightly clearer that BeginDownloadFeedback() will have deleted > |this| if it returns true.) Done. https://codereview.chromium.org/147593002/diff/270001/chrome/browser/ui/views... File chrome/browser/ui/views/download/download_item_view.h (right): https://codereview.chromium.org/147593002/diff/270001/chrome/browser/ui/views... chrome/browser/ui/views/download/download_item_view.h:151: // false is returned, nothing has changed. On 2014/02/04 21:56:33, Peter Kasting wrote: > Nit: While here, maybe change to: > > Submits the downloaded file to the safebrowsing download feedback service. > Returns whether submission was successful. On successful submission, |this| and > the DownloadItem will have been deleted. Done. https://codereview.chromium.org/147593002/diff/270001/chrome/browser/ui/views... chrome/browser/ui/views/download/download_item_view.h:152: bool BeginDownloadFeedback(); On 2014/02/04 21:56:33, Peter Kasting wrote: > Nit: This function name and the one below seem somewhat confusing. What about: > > bool SubmitDownloadToFeedbackService(); > > and > > void PossiblySubmitDownloadToFeedbackService(); > > ? Done. https://codereview.chromium.org/147593002/diff/270001/chrome/browser/ui/views... chrome/browser/ui/views/download/download_item_view.h:156: void BeginDownloadFeedbackWrapper(bool user_enabled); On 2014/02/04 21:56:33, Peter Kasting wrote: > Nit: Header declaration order does not mach .cc definition order Done. https://codereview.chromium.org/147593002/diff/270001/chrome/browser/ui/views... File chrome/browser/ui/views/download/download_shelf_view.h (right): https://codereview.chromium.org/147593002/diff/270001/chrome/browser/ui/views... chrome/browser/ui/views/download/download_shelf_view.h:57: content::PageNavigator* GetNavigator(); On 2014/02/04 21:56:33, Peter Kasting wrote: > Nit: While here: There's no need for this function. All callers can just call > browser() instead, which is also public. Nuke this. > > Note: If this comment and/or the last one in this file would result in large > diffs, feel free to do them as separate CLs. I'll send a follow-up CL for this. https://codereview.chromium.org/147593002/diff/270001/chrome/browser/ui/views... chrome/browser/ui/views/download/download_shelf_view.h:60: BrowserView* GetParentWindowView(); On 2014/02/04 21:56:33, Peter Kasting wrote: > Nit: This function should be inlined and named like: > > BrowserView* get_parent() { return parent_; } Done. https://codereview.chromium.org/147593002/diff/270001/chrome/browser/ui/views... chrome/browser/ui/views/download/download_shelf_view.h:85: virtual Browser* browser() const OVERRIDE; On 2014/02/04 21:56:33, Peter Kasting wrote: > Nit: While here: Virtual functions must never be named unix_hacker()-style. > Capitalize this (and in the remainder of the class hierarchy). I'll send a follow-up CL for this.
https://codereview.chromium.org/147593002/diff/270001/chrome/browser/ui/views... File chrome/browser/ui/views/download/download_feedback_dialog_view.cc (right): https://codereview.chromium.org/147593002/diff/270001/chrome/browser/ui/views... chrome/browser/ui/views/download/download_feedback_dialog_view.cc:73: return ui::MODAL_TYPE_SYSTEM; On 2014/02/04 23:37:22, felt wrote: > On 2014/02/04 21:56:33, Peter Kasting wrote: > > Why does this need to be system-modal? > > I was copying how similar dialogs seem to work. I admit to not entirely > understanding the types of modality. What is your recommendation? In general, we want to be as non-modal as possible. There are several possibilities: (1) If the dialog is totally optional, we could make it dismiss on focus loss, in which case it could be entirely non-modal. If we did this, we'd presumably want to show the dialog again in the future since the user didn't actually choose an answer. (2) If the dialog needs to be modal to the current page, we could just do that without making it system-modal. In this case, we'd still need to worry about what should happen if a user gets the dialog in one tab, then while that dialog is still open also has a dangerous download happen in a second tab. Perhaps we should show the dialog on both tabs but dismiss both when the user answers either. (3) If we make the dialog system-modal we avoid these sorts of complications, but we block the user from doing anything else, which isn't very user-friendly. It can feel frustrating to have Chrome demanding an answer to this question before I can do even unrelated tasks. https://codereview.chromium.org/147593002/diff/270001/chrome/browser/ui/views... File chrome/browser/ui/views/download/download_item_view.cc (right): https://codereview.chromium.org/147593002/diff/270001/chrome/browser/ui/views... chrome/browser/ui/views/download/download_item_view.cc:560: case DownloadFeedbackDialogView::kNeverShown: { On 2014/02/04 23:37:22, felt wrote: > On 2014/02/04 21:56:33, Peter Kasting wrote: > > Nit: {} not necessary on any of these cases. > > Done, but doesn't the style guide say either with or without {} is fine? Yes, either way is acceptable by the style guide. That's why I said "not necessary" rather than "banned, do not use". https://codereview.chromium.org/147593002/diff/270001/chrome/browser/ui/views... chrome/browser/ui/views/download/download_item_view.cc:576: case DownloadFeedbackDialogView::kMaxValue: On 2014/02/04 23:37:22, felt wrote: > On 2014/02/04 21:56:33, Peter Kasting wrote: > > Having this value buys you nothing over just having the "default:" case below. > > > Eliminate this line and eliminate the value from the original enum. > > Switching to just "default", but I need kMaxValue so that it can be histogrammed > in a subsequent CL. In that case, I would eliminate "default" and only have the kMaxValue case. Then the compiler should hopefully complain if someone introduces another value without updating this switch statement.
https://codereview.chromium.org/147593002/diff/270001/chrome/browser/ui/views... File chrome/browser/ui/views/download/download_feedback_dialog_view.cc (right): https://codereview.chromium.org/147593002/diff/270001/chrome/browser/ui/views... chrome/browser/ui/views/download/download_feedback_dialog_view.cc:73: return ui::MODAL_TYPE_SYSTEM; On 2014/02/05 00:07:36, Peter Kasting wrote: > On 2014/02/04 23:37:22, felt wrote: > > On 2014/02/04 21:56:33, Peter Kasting wrote: > > > Why does this need to be system-modal? > > > > I was copying how similar dialogs seem to work. I admit to not entirely > > understanding the types of modality. What is your recommendation? > > In general, we want to be as non-modal as possible. There are several > possibilities: > (1) If the dialog is totally optional, we could make it dismiss on focus loss, > in which case it could be entirely non-modal. If we did this, we'd presumably > want to show the dialog again in the future since the user didn't actually > choose an answer. > (2) If the dialog needs to be modal to the current page, we could just do that > without making it system-modal. In this case, we'd still need to worry about > what should happen if a user gets the dialog in one tab, then while that dialog > is still open also has a dangerous download happen in a second tab. Perhaps we > should show the dialog on both tabs but dismiss both when the user answers > either. > (3) If we make the dialog system-modal we avoid these sorts of complications, > but we block the user from doing anything else, which isn't very user-friendly. > It can feel frustrating to have Chrome demanding an answer to this question > before I can do even unrelated tasks. This dialog doesn't pertain to any specific tab. It is related to the window that has the download shelf on it. So, I think that would mean it would have to be modal to that window (but not necessarily the entire browser?). Is that #3 (system-modal), or some other modality? https://codereview.chromium.org/147593002/diff/270001/chrome/browser/ui/views... File chrome/browser/ui/views/download/download_item_view.cc (right): https://codereview.chromium.org/147593002/diff/270001/chrome/browser/ui/views... chrome/browser/ui/views/download/download_item_view.cc:576: case DownloadFeedbackDialogView::kMaxValue: On 2014/02/05 00:07:36, Peter Kasting wrote: > On 2014/02/04 23:37:22, felt wrote: > > On 2014/02/04 21:56:33, Peter Kasting wrote: > > > Having this value buys you nothing over just having the "default:" case > below. > > > > > Eliminate this line and eliminate the value from the original enum. > > > > Switching to just "default", but I need kMaxValue so that it can be > histogrammed > > in a subsequent CL. > > In that case, I would eliminate "default" and only have the kMaxValue case. > Then the compiler should hopefully complain if someone introduces another value > without updating this switch statement. Done. https://codereview.chromium.org/147593002/diff/270001/chrome/browser/ui/views... chrome/browser/ui/views/download/download_item_view.cc:920: void DownloadItemView::BeginDownloadFeedbackWrapper(bool user_enabled) { On 2014/02/04 21:56:33, Peter Kasting wrote: > Nit: If you make the argument to this be the enum type instead of a bool, then > not only is the meaning of the argument slightly clearer, but you can slightly > simplify the switch statement above by doing: > > case DownloadFeedbackDialogView::kUserEnabled: > case DownloadFeedbackDialogView::kUserDisabled: > BeginDownloadFeedbackWrapper(pref_value); > break; Done.
https://codereview.chromium.org/147593002/diff/270001/chrome/browser/ui/views... File chrome/browser/ui/views/download/download_feedback_dialog_view.cc (right): https://codereview.chromium.org/147593002/diff/270001/chrome/browser/ui/views... chrome/browser/ui/views/download/download_feedback_dialog_view.cc:73: return ui::MODAL_TYPE_SYSTEM; On 2014/02/05 02:08:53, felt wrote: > On 2014/02/05 00:07:36, Peter Kasting wrote: > > On 2014/02/04 23:37:22, felt wrote: > > > On 2014/02/04 21:56:33, Peter Kasting wrote: > > > > Why does this need to be system-modal? > > > > > > I was copying how similar dialogs seem to work. I admit to not entirely > > > understanding the types of modality. What is your recommendation? > > > > In general, we want to be as non-modal as possible. There are several > > possibilities: > > (1) If the dialog is totally optional, we could make it dismiss on focus loss, > > in which case it could be entirely non-modal. If we did this, we'd presumably > > want to show the dialog again in the future since the user didn't actually > > choose an answer. > > (2) If the dialog needs to be modal to the current page, we could just do that > > without making it system-modal. In this case, we'd still need to worry about > > what should happen if a user gets the dialog in one tab, then while that > dialog > > is still open also has a dangerous download happen in a second tab. Perhaps > we > > should show the dialog on both tabs but dismiss both when the user answers > > either. > > (3) If we make the dialog system-modal we avoid these sorts of complications, > > but we block the user from doing anything else, which isn't very > user-friendly. > > It can feel frustrating to have Chrome demanding an answer to this question > > before I can do even unrelated tasks. > > This dialog doesn't pertain to any specific tab. It is related to the window > that has the download shelf on it. So, I think that would mean it would have to > be modal to that window (but not necessarily the entire browser?). Is that #3 > (system-modal), or some other modality? I think that's MODAL_TYPE_WINDOW? sky might know for sure.
https://codereview.chromium.org/147593002/diff/270001/chrome/browser/ui/views... File chrome/browser/ui/views/download/download_feedback_dialog_view.cc (right): https://codereview.chromium.org/147593002/diff/270001/chrome/browser/ui/views... chrome/browser/ui/views/download/download_feedback_dialog_view.cc:73: return ui::MODAL_TYPE_SYSTEM; On 2014/02/05 02:10:54, Peter Kasting wrote: > On 2014/02/05 02:08:53, felt wrote: > > On 2014/02/05 00:07:36, Peter Kasting wrote: > > > On 2014/02/04 23:37:22, felt wrote: > > > > On 2014/02/04 21:56:33, Peter Kasting wrote: > > > > > Why does this need to be system-modal? > > > > > > > > I was copying how similar dialogs seem to work. I admit to not entirely > > > > understanding the types of modality. What is your recommendation? > > > > > > In general, we want to be as non-modal as possible. There are several > > > possibilities: > > > (1) If the dialog is totally optional, we could make it dismiss on focus > loss, > > > in which case it could be entirely non-modal. If we did this, we'd > presumably > > > want to show the dialog again in the future since the user didn't actually > > > choose an answer. > > > (2) If the dialog needs to be modal to the current page, we could just do > that > > > without making it system-modal. In this case, we'd still need to worry > about > > > what should happen if a user gets the dialog in one tab, then while that > > dialog > > > is still open also has a dangerous download happen in a second tab. Perhaps > > we > > > should show the dialog on both tabs but dismiss both when the user answers > > > either. > > > (3) If we make the dialog system-modal we avoid these sorts of > complications, > > > but we block the user from doing anything else, which isn't very > > user-friendly. > > > It can feel frustrating to have Chrome demanding an answer to this question > > > before I can do even unrelated tasks. > > > > This dialog doesn't pertain to any specific tab. It is related to the window > > that has the download shelf on it. So, I think that would mean it would have > to > > be modal to that window (but not necessarily the entire browser?). Is that #3 > > (system-modal), or some other modality? > > I think that's MODAL_TYPE_WINDOW? sky might know for sure. sky agrees it should be MODAL_TYPE_WINDOW, so done.
On 2014/02/05 18:41:31, felt wrote: > sky agrees it should be MODAL_TYPE_WINDOW, so done. OK, so that leaves us in the scenario where you could have multiple windows, each showing this dialog. I don't think we can sanely prevent that from occurring, but I do wonder if we should at least make it so replying to the dialog in one window should make all other windows' dialogs immediately close? Otherwise, it's basically "whatever I replied to the most recent dialog with".
Other than the question I raised about window-modal desired behavior, LGTM. https://codereview.chromium.org/147593002/diff/270001/chrome/browser/ui/views... File chrome/browser/ui/views/download/download_feedback_dialog_view.h (right): https://codereview.chromium.org/147593002/diff/270001/chrome/browser/ui/views... chrome/browser/ui/views/download/download_feedback_dialog_view.h:22: // or UNCOMMON_DOWNLOAD. User should only see this dialog once! On 2014/02/04 23:37:22, felt wrote: > On 2014/02/04 21:56:33, Peter Kasting wrote: > > Nit: User -> The user > > > > I would probably avoid the "!" at the end too. > > Done. Tiny nit: It still just says "User". https://codereview.chromium.org/147593002/diff/270001/chrome/browser/ui/views... chrome/browser/ui/views/download/download_feedback_dialog_view.h:25: // Used to set prefs::kSafeBrowsingDownloadReportingEnabled. On 2014/02/04 23:37:22, felt wrote: > On 2014/02/04 21:56:33, Peter Kasting wrote: > > Nit: "Possible values for the kSafeBrowsingDownloadReportingEnabled pref."? > > Done. Not really... there's now just an added "pref" at the end. > > Also, maybe the pref should be called "kSafeBrowsingDownloadReportingStatus"? > > I don't mind renaming the pref, but I'd rather rename the pref in another CL. Fine with me. https://codereview.chromium.org/147593002/diff/420001/chrome/browser/ui/views... File chrome/browser/ui/views/download/download_item_view.cc (right): https://codereview.chromium.org/147593002/diff/420001/chrome/browser/ui/views... chrome/browser/ui/views/download/download_item_view.cc:570: case DownloadFeedbackDialogView::kDownloadReportingEnabled: Nit: I might put a newline above this line, as well as above the kMaxValue case line below, just to make the logical blocks more visually distinct. https://codereview.chromium.org/147593002/diff/420001/chrome/browser/ui/views... chrome/browser/ui/views/download/download_item_view.cc:938: !SubmitDownloadToFeedbackService()) { Nit: {} optional
Here's what I did for the WINDOW issue: - Added a new enum value, kDialogCurrentlyDisplayed - Set the pref to kDialogCurrentlyDisplayed as soon as the dialog is displayed - If download_item_view.cc sees kDialogCurrentlyDisplayed, it will conservatively ditch that download item https://codereview.chromium.org/147593002/diff/270001/chrome/browser/ui/views... File chrome/browser/ui/views/download/download_feedback_dialog_view.h (right): https://codereview.chromium.org/147593002/diff/270001/chrome/browser/ui/views... chrome/browser/ui/views/download/download_feedback_dialog_view.h:22: // or UNCOMMON_DOWNLOAD. User should only see this dialog once! On 2014/02/05 19:47:08, Peter Kasting wrote: > On 2014/02/04 23:37:22, felt wrote: > > On 2014/02/04 21:56:33, Peter Kasting wrote: > > > Nit: User -> The user > > > > > > I would probably avoid the "!" at the end too. > > > > Done. > > Tiny nit: It still just says "User". Done. https://codereview.chromium.org/147593002/diff/270001/chrome/browser/ui/views... chrome/browser/ui/views/download/download_feedback_dialog_view.h:25: // Used to set prefs::kSafeBrowsingDownloadReportingEnabled. On 2014/02/05 19:47:08, Peter Kasting wrote: > On 2014/02/04 23:37:22, felt wrote: > > On 2014/02/04 21:56:33, Peter Kasting wrote: > > > Nit: "Possible values for the kSafeBrowsingDownloadReportingEnabled pref."? > > > > Done. > > Not really... there's now just an added "pref" at the end. Done. > > > > Also, maybe the pref should be called > "kSafeBrowsingDownloadReportingStatus"? > > > > I don't mind renaming the pref, but I'd rather rename the pref in another CL. > > Fine with me. https://codereview.chromium.org/147593002/diff/420001/chrome/browser/ui/views... File chrome/browser/ui/views/download/download_item_view.cc (right): https://codereview.chromium.org/147593002/diff/420001/chrome/browser/ui/views... chrome/browser/ui/views/download/download_item_view.cc:570: case DownloadFeedbackDialogView::kDownloadReportingEnabled: On 2014/02/05 19:47:08, Peter Kasting wrote: > Nit: I might put a newline above this line, as well as above the kMaxValue case > line below, just to make the logical blocks more visually distinct. Done. https://codereview.chromium.org/147593002/diff/420001/chrome/browser/ui/views... chrome/browser/ui/views/download/download_item_view.cc:938: !SubmitDownloadToFeedbackService()) { On 2014/02/05 19:47:08, Peter Kasting wrote: > Nit: {} optional I like brackets when the condition is split across lines, so keeping {}.
asanka, this involves a trivial (one-line) change to chrome/browser/download/download_shelf_context_menu.cc; TBRing you for OWNERS approval
On 2014/02/05 19:57:25, felt wrote: > Here's what I did for the WINDOW issue: > > - Added a new enum value, kDialogCurrentlyDisplayed > - Set the pref to kDialogCurrentlyDisplayed as soon as the dialog is displayed > - If download_item_view.cc sees kDialogCurrentlyDisplayed, it will > conservatively ditch that download item OK, I guess that works. Does closing the dialog via the X button (instead of picking one of the choices) act the same as choosing "No thanks"? If so that makes me more comfortable with this proposed state.
On 2014/02/05 21:05:12, Peter Kasting wrote: > On 2014/02/05 19:57:25, felt wrote: > > Here's what I did for the WINDOW issue: > > > > - Added a new enum value, kDialogCurrentlyDisplayed > > - Set the pref to kDialogCurrentlyDisplayed as soon as the dialog is displayed > > - If download_item_view.cc sees kDialogCurrentlyDisplayed, it will > > conservatively ditch that download item > > OK, I guess that works. > > Does closing the dialog via the X button (instead of picking one of the choices) > act the same as choosing "No thanks"? If so that makes me more comfortable with > this proposed state. Yes, closing via the X button is the same as choosing Cancel().
The CQ bit was checked by felt@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/felt@chromium.org/147593002/620001
It seems that kDialogCurrentlyDisplayed is no different than kDownloadReportingDisabled? If somehow no selection is made (crash, or whatever if there are other corner cases), then it will stay as kDialogCurrentlyDisplayed forever, and will always be interpreted as kDownloadReportingDisabled. If that's the intention, it could just be removed and go back to using kDownloadReportingDisabled. If not, there should be some way for it to act differently. Did I miss something?
The CQ bit was unchecked by mattm@chromium.org
The CQ bit was unchecked by felt@chromium.org
On 2014/02/06 01:16:18, mattm wrote: > It seems that kDialogCurrentlyDisplayed is no different than > kDownloadReportingDisabled? If somehow no selection is made (crash, or whatever > if there are other corner cases), then it will stay as kDialogCurrentlyDisplayed > forever, and will always be interpreted as kDownloadReportingDisabled. If that's > the intention, it could just be removed and go back to using > kDownloadReportingDisabled. If not, there should be some way for it to act > differently. Did I miss something? That's my intention, except we can distinguish between the two cases if we ever need to. I'm hoping that we don't get crashes here often enough for it to really negatively influence the number of users who opt in. It would be nice to be able to reset it to never shown when the browser is starting up, but I'm not sure how to do that. I could look into that in a separate CL.
On 2014/02/06 01:19:14, felt wrote: > It would be nice to be able to reset it to never shown when the browser is > starting up, but I'm not sure how to do that. I could look into that in a > separate CL. Don't save the "currently displayed" value into the pref. Instead keep it as a separate bool in memory. This way, if you crash, on restart the pref value is still "not shown".
On 2014/02/06 01:21:11, Peter Kasting wrote: > On 2014/02/06 01:19:14, felt wrote: > > It would be nice to be able to reset it to never shown when the browser is > > starting up, but I'm not sure how to do that. I could look into that in a > > separate CL. > > Don't save the "currently displayed" value into the pref. Instead keep it as a > separate bool in memory. > > This way, if you crash, on restart the pref value is still "not shown". Thoughts on where to save it in memory? Can't save it in a DownloadItem, DownloadItem, the new dialog, or the parent window. Maybe... the browser()?
On 2014/02/06 01:23:25, felt wrote: > On 2014/02/06 01:21:11, Peter Kasting wrote: > > On 2014/02/06 01:19:14, felt wrote: > > > It would be nice to be able to reset it to never shown when the browser is > > > starting up, but I'm not sure how to do that. I could look into that in a > > > separate CL. > > > > Don't save the "currently displayed" value into the pref. Instead keep it as > a > > separate bool in memory. > > > > This way, if you crash, on restart the pref value is still "not shown". > > Thoughts on where to save it in memory? Can't save it in a DownloadItem, > DownloadItem, the new dialog, or the parent window. Maybe... the browser()? Why can't it be a static member of DownloadFeedbackDialogView, since you already have a static Show() function there that could check it?
On 2014/02/06 01:25:07, Peter Kasting wrote: > On 2014/02/06 01:23:25, felt wrote: > > On 2014/02/06 01:21:11, Peter Kasting wrote: > > > On 2014/02/06 01:19:14, felt wrote: > > > > It would be nice to be able to reset it to never shown when the browser is > > > > starting up, but I'm not sure how to do that. I could look into that in a > > > > separate CL. > > > > > > Don't save the "currently displayed" value into the pref. Instead keep it > as > > a > > > separate bool in memory. > > > > > > This way, if you crash, on restart the pref value is still "not shown". > > > > Thoughts on where to save it in memory? Can't save it in a DownloadItem, > > DownloadItem, the new dialog, or the parent window. Maybe... the browser()? > > Why can't it be a static member of DownloadFeedbackDialogView, since you already > have a static Show() function there that could check it? Wouldn't it then be shared across profiles? (I could be wrong about this.)
On 2014/02/06 01:27:12, felt wrote: > On 2014/02/06 01:25:07, Peter Kasting wrote: > > On 2014/02/06 01:23:25, felt wrote: > > > On 2014/02/06 01:21:11, Peter Kasting wrote: > > > > On 2014/02/06 01:19:14, felt wrote: > > > > > It would be nice to be able to reset it to never shown when the browser > is > > > > > starting up, but I'm not sure how to do that. I could look into that in > a > > > > > separate CL. > > > > > > > > Don't save the "currently displayed" value into the pref. Instead keep it > > as > > > a > > > > separate bool in memory. > > > > > > > > This way, if you crash, on restart the pref value is still "not shown". > > > > > > Thoughts on where to save it in memory? Can't save it in a DownloadItem, > > > DownloadItem, the new dialog, or the parent window. Maybe... the browser()? > > > > Why can't it be a static member of DownloadFeedbackDialogView, since you > already > > have a static Show() function there that could check it? > > Wouldn't it then be shared across profiles? (I could be wrong about this.) You could use SetUserData on the profile. (Profile is a content::BrowserContext which is a base::SupportsUserData)
On 2014/02/06 01:31:08, mattm wrote: > On 2014/02/06 01:27:12, felt wrote: > > On 2014/02/06 01:25:07, Peter Kasting wrote: > > > On 2014/02/06 01:23:25, felt wrote: > > > > On 2014/02/06 01:21:11, Peter Kasting wrote: > > > > > On 2014/02/06 01:19:14, felt wrote: > > > > > > It would be nice to be able to reset it to never shown when the > browser > > is > > > > > > starting up, but I'm not sure how to do that. I could look into that > in > > a > > > > > > separate CL. > > > > > > > > > > Don't save the "currently displayed" value into the pref. Instead keep > it > > > as > > > > a > > > > > separate bool in memory. > > > > > > > > > > This way, if you crash, on restart the pref value is still "not shown". > > > > > > > > Thoughts on where to save it in memory? Can't save it in a DownloadItem, > > > > DownloadItem, the new dialog, or the parent window. Maybe... the > browser()? > > > > > > Why can't it be a static member of DownloadFeedbackDialogView, since you > > already > > > have a static Show() function there that could check it? > > > > Wouldn't it then be shared across profiles? (I could be wrong about this.) > > You could use SetUserData on the profile. > (Profile is a content::BrowserContext which is a base::SupportsUserData) Ah! Good idea, I've never used SetUserData before. Done.
https://codereview.chromium.org/147593002/diff/930001/chrome/browser/ui/views... File chrome/browser/ui/views/download/download_item_view.cc (right): https://codereview.chromium.org/147593002/diff/930001/chrome/browser/ui/views... chrome/browser/ui/views/download/download_item_view.cc:94: const char kDialogStatusKey[] = "DownloadItemView DialogStatusKey"; The key doesn't actually need to be a string, you can avoid creating string data that isn't used. The common idiom is: const void* kDialogStatusKey = &kDialogStatusKey; https://codereview.chromium.org/147593002/diff/930001/chrome/browser/ui/views... chrome/browser/ui/views/download/download_item_view.cc:98: DialogStatusData() : currently_shown_(false) { } should have a virtual destructor https://codereview.chromium.org/147593002/diff/930001/chrome/browser/ui/views... chrome/browser/ui/views/download/download_item_view.cc:99: bool currently_shown_; style nit:Should be private with accessors, unless you change this to a struct. In that case it should follow the struct member naming rules (no trailing _). (A struct inheriting from a class seems a little weird, but the style guide doesn't really forbid it.. I see there are a few places in chrome doing that with SupportsUserData::Data already.)
https://codereview.chromium.org/147593002/diff/930001/chrome/browser/ui/views... File chrome/browser/ui/views/download/download_item_view.cc (right): https://codereview.chromium.org/147593002/diff/930001/chrome/browser/ui/views... chrome/browser/ui/views/download/download_item_view.cc:94: const char kDialogStatusKey[] = "DownloadItemView DialogStatusKey"; On 2014/02/06 03:13:10, mattm wrote: > The key doesn't actually need to be a string, you can avoid creating string data > that isn't used. The common idiom is: > const void* kDialogStatusKey = &kDialogStatusKey; Done. https://codereview.chromium.org/147593002/diff/930001/chrome/browser/ui/views... chrome/browser/ui/views/download/download_item_view.cc:98: DialogStatusData() : currently_shown_(false) { } On 2014/02/06 03:13:10, mattm wrote: > should have a virtual destructor Done. https://codereview.chromium.org/147593002/diff/930001/chrome/browser/ui/views... chrome/browser/ui/views/download/download_item_view.cc:99: bool currently_shown_; On 2014/02/06 03:13:10, mattm wrote: > style nit:Should be private with accessors, unless you change this to a struct. > In that case it should follow the struct member naming rules (no trailing _). > > (A struct inheriting from a class seems a little weird, but the style guide > doesn't really forbid it.. I see there are a few places in chrome doing that > with SupportsUserData::Data already.) Done.
lgtm with nits https://codereview.chromium.org/147593002/diff/990002/chrome/browser/ui/views... File chrome/browser/ui/views/download/download_item_view.cc (right): https://codereview.chromium.org/147593002/diff/990002/chrome/browser/ui/views... chrome/browser/ui/views/download/download_item_view.cc:100: bool currently_shown() { return currently_shown_; } should be const method https://codereview.chromium.org/147593002/diff/990002/chrome/browser/ui/views... chrome/browser/ui/views/download/download_item_view.cc:101: void currently_shown(bool shown) { currently_shown_ = shown; } set_currently_shown
https://codereview.chromium.org/147593002/diff/990002/chrome/browser/ui/views... File chrome/browser/ui/views/download/download_item_view.cc (right): https://codereview.chromium.org/147593002/diff/990002/chrome/browser/ui/views... chrome/browser/ui/views/download/download_item_view.cc:100: bool currently_shown() { return currently_shown_; } On 2014/02/06 03:35:22, mattm wrote: > should be const method Done. https://codereview.chromium.org/147593002/diff/990002/chrome/browser/ui/views... chrome/browser/ui/views/download/download_item_view.cc:101: void currently_shown(bool shown) { currently_shown_ = shown; } On 2014/02/06 03:35:22, mattm wrote: > set_currently_shown Done.
The CQ bit was checked by felt@chromium.org
https://codereview.chromium.org/147593002/diff/1320001/chrome/browser/ui/view... File chrome/browser/ui/views/download/download_feedback_dialog_view.h (right): https://codereview.chromium.org/147593002/diff/1320001/chrome/browser/ui/view... chrome/browser/ui/views/download/download_feedback_dialog_view.h:30: kDialogDecisionPending, // When a dialog is open in another window. A little odd since the pref is never actually set to this value. Could just note that in the comment, but it seems like it would be cleaner to modify the way PossiblySubmitDownloadToFeedbackService handles this to avoid it. Actually I wonder if it would be cleaner to move all that logic into download_feedback_dialog_view, and replace the Show method with MaybeShow ? WDYT?
The CQ bit was unchecked by felt@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/felt@chromium.org/147593002/1320001
The CQ bit was unchecked by commit-bot@chromium.org
Failed to apply patch for chrome/browser/ui/views/download/download_item_view.h: While running patch -p1 --forward --force --no-backup-if-mismatch; patching file chrome/browser/ui/views/download/download_item_view.h Hunk #1 FAILED at 26. 1 out of 2 hunks FAILED -- saving rejects to file chrome/browser/ui/views/download/download_item_view.h.rej Patch: chrome/browser/ui/views/download/download_item_view.h Index: chrome/browser/ui/views/download/download_item_view.h diff --git a/chrome/browser/ui/views/download/download_item_view.h b/chrome/browser/ui/views/download/download_item_view.h index a1c34b18c59882fa9c0858890a865112c773c97f..cefccc4f55eb647120eb819b876645a8532f159a 100644 --- a/chrome/browser/ui/views/download/download_item_view.h +++ b/chrome/browser/ui/views/download/download_item_view.h @@ -26,6 +26,7 @@ #include "base/timer/timer.h" #include "chrome/browser/download/download_item_model.h" #include "chrome/browser/icon_manager.h" +#include "chrome/browser/ui/views/download/download_feedback_dialog_view.h" #include "chrome/common/cancelable_task_tracker.h" #include "content/public/browser/download_item.h" #include "content/public/browser/download_manager.h" @@ -146,10 +147,15 @@ class DownloadItemView : public views::ButtonListener, void OpenDownload(); - // Submit the downloaded file to the safebrowsing download feedback service. - // If true is returned, the DownloadItem and |this| have been deleted. If - // false is returned, nothing has changed. - bool BeginDownloadFeedback(); + // Submits the downloaded file to the safebrowsing download feedback service. + // Returns whether submission was successful. On successful submission, + // |this| and the DownloadItem will have been deleted. + bool SubmitDownloadToFeedbackService(); + + // If the user has enabled uploading, calls SubmitDownloadToFeedbackService. + // Otherwise, it simply removes the DownloadItem without uploading. + void PossiblySubmitDownloadToFeedbackService( + DownloadFeedbackDialogView::DownloadReportingStatus status); void LoadIcon(); void LoadIconIfItemPathChanged();
https://codereview.chromium.org/147593002/diff/1320001/chrome/browser/ui/view... File chrome/browser/ui/views/download/download_feedback_dialog_view.h (right): https://codereview.chromium.org/147593002/diff/1320001/chrome/browser/ui/view... chrome/browser/ui/views/download/download_feedback_dialog_view.h:30: kDialogDecisionPending, // When a dialog is open in another window. On 2014/02/06 23:27:26, mattm wrote: > A little odd since the pref is never actually set to this value. Could just note > that in the comment, but it seems like it would be cleaner to modify the way > PossiblySubmitDownloadToFeedbackService handles this to avoid it. > > Actually I wonder if it would be cleaner to move all that logic into > download_feedback_dialog_view, and replace the Show method with MaybeShow ? > WDYT? There are a few ways to do this: 1. Have the different enum states to differentiate 2. Add a boolean to PossiblySubmitDownloadToFeedbackService 3. Refactor the logic into the dialog's Show method They all seem equivalent to me. Is it busywork to change it, or is #3 really that preferable?
https://codereview.chromium.org/147593002/diff/1320001/chrome/browser/ui/view... File chrome/browser/ui/views/download/download_feedback_dialog_view.h (right): https://codereview.chromium.org/147593002/diff/1320001/chrome/browser/ui/view... chrome/browser/ui/views/download/download_feedback_dialog_view.h:30: kDialogDecisionPending, // When a dialog is open in another window. On 2014/02/06 23:31:57, felt wrote: > On 2014/02/06 23:27:26, mattm wrote: > > A little odd since the pref is never actually set to this value. Could just > note > > that in the comment, but it seems like it would be cleaner to modify the way > > PossiblySubmitDownloadToFeedbackService handles this to avoid it. > > > > Actually I wonder if it would be cleaner to move all that logic into > > download_feedback_dialog_view, and replace the Show method with MaybeShow ? > > WDYT? > > There are a few ways to do this: > 1. Have the different enum states to differentiate > 2. Add a boolean to PossiblySubmitDownloadToFeedbackService > 3. Refactor the logic into the dialog's Show method > > They all seem equivalent to me. Is it busywork to change it, or is #3 really > that preferable? #3 does seem preferable to me from a code readability/maintainability perspective, as DownloadItemView is already a pretty big class, moving the complexity into DownloadFeedbackDialogView seems worthwhile. It also seems like a more logical place to have it.(Eg, DownloadItemView shouldn't have to care how the FeedbackView implements this, like if we decide to make it show the dialog in any window that this happens instead of only the first, if the logic was in the FeedbackView then DownloadItemView wouldn't even have to change.)
Matt, PTAL. Moved the UserData code into the dialog as requested.
Thanks! lgtm with a few nits. https://codereview.chromium.org/147593002/diff/1470001/chrome/browser/ui/view... File chrome/browser/ui/views/download/download_feedback_dialog_view.cc (right): https://codereview.chromium.org/147593002/diff/1470001/chrome/browser/ui/view... chrome/browser/ui/views/download/download_feedback_dialog_view.cc:26: }; these should go in an anonymous namespace https://codereview.chromium.org/147593002/diff/1470001/chrome/browser/ui/view... File chrome/browser/ui/views/download/download_feedback_dialog_view.h (right): https://codereview.chromium.org/147593002/diff/1470001/chrome/browser/ui/view... chrome/browser/ui/views/download/download_feedback_dialog_view.h:10: #include "chrome/browser/ui/browser.h" unused? https://codereview.chromium.org/147593002/diff/1470001/chrome/browser/ui/view... File chrome/browser/ui/views/download/download_item_view.cc (right): https://codereview.chromium.org/147593002/diff/1470001/chrome/browser/ui/view... chrome/browser/ui/views/download/download_item_view.cc:25: #include "chrome/browser/prefs/pref_service_syncable.h" Can these includes just be base/prefs/pref_service.h? https://codereview.chromium.org/147593002/diff/1470001/chrome/browser/ui/view... File chrome/browser/ui/views/download/download_shelf_view.h (right): https://codereview.chromium.org/147593002/diff/1470001/chrome/browser/ui/view... chrome/browser/ui/views/download/download_shelf_view.h:13: #include "chrome/browser/ui/views/frame/browser_view.h" Should go in download_item_view.cc?
https://codereview.chromium.org/147593002/diff/1470001/chrome/browser/ui/view... File chrome/browser/ui/views/download/download_feedback_dialog_view.cc (right): https://codereview.chromium.org/147593002/diff/1470001/chrome/browser/ui/view... chrome/browser/ui/views/download/download_feedback_dialog_view.cc:26: }; On 2014/02/07 02:03:36, mattm wrote: > these should go in an anonymous namespace Done. https://codereview.chromium.org/147593002/diff/1470001/chrome/browser/ui/view... File chrome/browser/ui/views/download/download_feedback_dialog_view.h (right): https://codereview.chromium.org/147593002/diff/1470001/chrome/browser/ui/view... chrome/browser/ui/views/download/download_feedback_dialog_view.h:10: #include "chrome/browser/ui/browser.h" On 2014/02/07 02:03:36, mattm wrote: > unused? Done. https://codereview.chromium.org/147593002/diff/1470001/chrome/browser/ui/view... File chrome/browser/ui/views/download/download_item_view.cc (right): https://codereview.chromium.org/147593002/diff/1470001/chrome/browser/ui/view... chrome/browser/ui/views/download/download_item_view.cc:25: #include "chrome/browser/prefs/pref_service_syncable.h" On 2014/02/07 02:03:36, mattm wrote: > Can these includes just be base/prefs/pref_service.h? Done. https://codereview.chromium.org/147593002/diff/1470001/chrome/browser/ui/view... File chrome/browser/ui/views/download/download_shelf_view.h (right): https://codereview.chromium.org/147593002/diff/1470001/chrome/browser/ui/view... chrome/browser/ui/views/download/download_shelf_view.h:13: #include "chrome/browser/ui/views/frame/browser_view.h" On 2014/02/07 02:03:36, mattm wrote: > Should go in download_item_view.cc? Done.
https://codereview.chromium.org/147593002/diff/1570001/chrome/browser/ui/view... File chrome/browser/ui/views/download/download_feedback_dialog_view.cc (right): https://codereview.chromium.org/147593002/diff/1570001/chrome/browser/ui/view... chrome/browser/ui/views/download/download_feedback_dialog_view.cc:8: #include "chrome/browser/prefs/pref_service_syncable.h" this one too?
https://codereview.chromium.org/147593002/diff/1570001/chrome/browser/ui/view... File chrome/browser/ui/views/download/download_feedback_dialog_view.cc (right): https://codereview.chromium.org/147593002/diff/1570001/chrome/browser/ui/view... chrome/browser/ui/views/download/download_feedback_dialog_view.cc:8: #include "chrome/browser/prefs/pref_service_syncable.h" On 2014/02/07 11:21:04, mattm wrote: > this one too? Done.
The CQ bit was checked by felt@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/felt@chromium.org/147593002/1760001
Message was sent while issue was closed.
Change committed as 249796 |
