Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2793413002:
[subresource_filter] Update the desktop UI according to mocks (Closed)
|
Created:
3 years, 8 months ago by Charlie Harrison Modified:
3 years, 8 months ago CC:
chromium-reviews, tfarina Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
Description[subresource_filter] Update the desktop UI according to mocks
This patch adds logic to the content setting bubble to support using a
checkbox for policy decisions.
BUG=689992
Review-Url: https://codereview.chromium.org/2793413002
Cr-Commit-Position: refs/heads/master@{#462498}
Committed: https://chromium.googlesource.com/chromium/src/+/5edcdad617c78e16268090993b552065ee04694a
Patch Set 1 #Patch Set 2 : Not as part of the extra view #
Total comments: 20
Patch Set 3 : msw review #
Total comments: 12
Patch Set 4 : Remove string, update dialog #
Total comments: 4
Patch Set 5 : ordering #
Total comments: 2
Patch Set 6 : engedy review #
Messages
Total messages: 42 (29 generated)
The CQ bit was checked by csharrison@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
csharrison@chromium.org changed reviewers: + msw@chromium.org
msw: Are you a good reviewer for this change? Note that it is currently a WIP (please don't mind the bare "Reload" string). Please see the linked bug for mocks that I am trying to mimic. The main thing left to do here is somehow call ContentSettingImageModel::UpdateFromWebContents. I wonder if the best way to do that is via the ContentSettingBubbleModelDelegate?
I commented on the bug... not sure why we want this.
https://codereview.chromium.org/2793413002/diff/20001/chrome/browser/ui/conte... File chrome/browser/ui/content_settings/content_setting_bubble_model.cc (left): https://codereview.chromium.org/2793413002/diff/20001/chrome/browser/ui/conte... chrome/browser/ui/content_settings/content_setting_bubble_model.cc:1250: set_show_manage_text_as_button(true); If this function is no longer used, and won't be imminently used, it should be removed (along with the underlying functionality). https://codereview.chromium.org/2793413002/diff/20001/chrome/browser/ui/conte... File chrome/browser/ui/content_settings/content_setting_bubble_model.cc (right): https://codereview.chromium.org/2793413002/diff/20001/chrome/browser/ui/conte... chrome/browser/ui/content_settings/content_setting_bubble_model.cc:1259: set_done_text(is_checked ? base::UTF8ToUTF16("Reload") q: Does this need to trigger a layout/paint of the UI with the new string? Also, I know you asked me to ignore this placeholder string, but it'll need to be fixed before this CL can be landed. What's blocking that? https://codereview.chromium.org/2793413002/diff/20001/chrome/browser/ui/conte... chrome/browser/ui/content_settings/content_setting_bubble_model.cc:1569: show_manage_text_as_checkbox(false) {} optional nit: move these default values to the struct declaration https://codereview.chromium.org/2793413002/diff/20001/chrome/browser/ui/conte... File chrome/browser/ui/content_settings/content_setting_bubble_model.h (right): https://codereview.chromium.org/2793413002/diff/20001/chrome/browser/ui/conte... chrome/browser/ui/content_settings/content_setting_bubble_model.h:130: base::string16 done_text; nit: give this a better name like |done_button_text| or just |button_text|. https://codereview.chromium.org/2793413002/diff/20001/chrome/browser/ui/conte... chrome/browser/ui/content_settings/content_setting_bubble_model.h:159: virtual void OnChecked(bool is_checked) {} nit: OnManageCheckboxChecked? https://codereview.chromium.org/2793413002/diff/20001/chrome/browser/ui/conte... chrome/browser/ui/content_settings/content_setting_bubble_model.h:205: void set_done_text(const base::string16& done_text) { nit: match order to member declaration? https://codereview.chromium.org/2793413002/diff/20001/chrome/browser/ui/views... File chrome/browser/ui/views/content_setting_bubble_contents.cc (right): https://codereview.chromium.org/2793413002/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/content_setting_bubble_contents.cc:420: if (content_setting_bubble_model_->bubble_content() Use |bubble_content| here and below. https://codereview.chromium.org/2793413002/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/content_setting_bubble_contents.cc:442: if (content_setting_bubble_model_->bubble_content() optional nit: make and use a local |bubble_content| like above. (less important if you remove |show_manage_text_as_button|) https://codereview.chromium.org/2793413002/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/content_setting_bubble_contents.cc:448: } else if (content_setting_bubble_model_->bubble_content() No else after return here and below https://codereview.chromium.org/2793413002/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/content_setting_bubble_contents.cc:475: base::string16 done_text = const &
The CQ bit was checked by csharrison@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Thanks for the review, and for catching that subresource filter is the only consumer of the manage_text button. The patch still lacks the UI update step, as mentioned in a comment. https://codereview.chromium.org/2793413002/diff/20001/chrome/browser/ui/conte... File chrome/browser/ui/content_settings/content_setting_bubble_model.cc (left): https://codereview.chromium.org/2793413002/diff/20001/chrome/browser/ui/conte... chrome/browser/ui/content_settings/content_setting_bubble_model.cc:1250: set_show_manage_text_as_button(true); On 2017/04/05 22:08:33, msw wrote: > If this function is no longer used, and won't be imminently used, it should be > removed (along with the underlying functionality). Done. https://codereview.chromium.org/2793413002/diff/20001/chrome/browser/ui/conte... File chrome/browser/ui/content_settings/content_setting_bubble_model.cc (right): https://codereview.chromium.org/2793413002/diff/20001/chrome/browser/ui/conte... chrome/browser/ui/content_settings/content_setting_bubble_model.cc:1259: set_done_text(is_checked ? base::UTF8ToUTF16("Reload") On 2017/04/05 22:08:33, msw wrote: > q: Does this need to trigger a layout/paint of the UI with the new string? Also, > I know you asked me to ignore this placeholder string, but it'll need to be > fixed before this CL can be landed. What's blocking that? Yes, this needs to trigger a layout/paint of the UI with the new string. I'm not quite sure the best way to do that. In my original message I thought maybe going through the ContentSettingBubbleModelDelegate, but I wanted to ask your opinion on the general approach first. The string this needed just landed in the related Android patch (also linked from the bug). The current PS has an updated string. https://codereview.chromium.org/2793413002/diff/20001/chrome/browser/ui/conte... chrome/browser/ui/content_settings/content_setting_bubble_model.cc:1569: show_manage_text_as_checkbox(false) {} On 2017/04/05 22:08:34, msw wrote: > optional nit: move these default values to the struct declaration Done. https://codereview.chromium.org/2793413002/diff/20001/chrome/browser/ui/conte... File chrome/browser/ui/content_settings/content_setting_bubble_model.h (right): https://codereview.chromium.org/2793413002/diff/20001/chrome/browser/ui/conte... chrome/browser/ui/content_settings/content_setting_bubble_model.h:130: base::string16 done_text; On 2017/04/05 22:08:34, msw wrote: > nit: give this a better name like |done_button_text| or just |button_text|. Done. Changed to done_button_text. https://codereview.chromium.org/2793413002/diff/20001/chrome/browser/ui/conte... chrome/browser/ui/content_settings/content_setting_bubble_model.h:159: virtual void OnChecked(bool is_checked) {} On 2017/04/05 22:08:34, msw wrote: > nit: OnManageCheckboxChecked? Done. https://codereview.chromium.org/2793413002/diff/20001/chrome/browser/ui/conte... chrome/browser/ui/content_settings/content_setting_bubble_model.h:205: void set_done_text(const base::string16& done_text) { On 2017/04/05 22:08:34, msw wrote: > nit: match order to member declaration? Done. https://codereview.chromium.org/2793413002/diff/20001/chrome/browser/ui/views... File chrome/browser/ui/views/content_setting_bubble_contents.cc (right): https://codereview.chromium.org/2793413002/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/content_setting_bubble_contents.cc:420: if (content_setting_bubble_model_->bubble_content() On 2017/04/05 22:08:34, msw wrote: > Use |bubble_content| here and below. Done. https://codereview.chromium.org/2793413002/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/content_setting_bubble_contents.cc:442: if (content_setting_bubble_model_->bubble_content() On 2017/04/05 22:08:34, msw wrote: > optional nit: make and use a local |bubble_content| like above. > (less important if you remove |show_manage_text_as_button|) Done. https://codereview.chromium.org/2793413002/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/content_setting_bubble_contents.cc:448: } else if (content_setting_bubble_model_->bubble_content() On 2017/04/05 22:08:34, msw wrote: > No else after return here and below Done. https://codereview.chromium.org/2793413002/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/content_setting_bubble_contents.cc:475: base::string16 done_text = On 2017/04/05 22:08:34, msw wrote: > const & Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2793413002/diff/40001/chrome/browser/ui/conte... File chrome/browser/ui/content_settings/content_setting_bubble_model.cc (right): https://codereview.chromium.org/2793413002/diff/40001/chrome/browser/ui/conte... chrome/browser/ui/content_settings/content_setting_bubble_model.cc:1261: is_checked ? IDS_FILTERED_DECEPTIVE_CONTENT_RELOAD_ACTION : IDS_DONE; aside: It'd be nice if we could just re-use IDS_APP_MENU_RELOAD or another of the many existing reload strings, and not add another duplicate string. https://codereview.chromium.org/2793413002/diff/40001/chrome/browser/ui/conte... File chrome/browser/ui/content_settings/content_setting_bubble_model.h (right): https://codereview.chromium.org/2793413002/diff/40001/chrome/browser/ui/conte... chrome/browser/ui/content_settings/content_setting_bubble_model.h:231: void set_learn_more_link(const std::string& link) { optional nit: this and |set_done_button_text| probably belong below |add_media_menu| and |set_selected_device| https://codereview.chromium.org/2793413002/diff/40001/chrome/browser/ui/views... File chrome/browser/ui/views/content_setting_bubble_contents.cc (right): https://codereview.chromium.org/2793413002/diff/40001/chrome/browser/ui/views... chrome/browser/ui/views/content_setting_bubble_contents.cc:37: #include "ui/views/controls/button/md_text_button.h" nit: remove if no longer needed https://codereview.chromium.org/2793413002/diff/40001/chrome/browser/ui/views... chrome/browser/ui/views/content_setting_bubble_contents.cc:421: new views::Checkbox(base::UTF8ToUTF16(bubble_content.manage_text)); aside: BubbleContent::manage_text should probably be a string16. https://codereview.chromium.org/2793413002/diff/40001/chrome/browser/ui/views... chrome/browser/ui/views/content_setting_bubble_contents.cc:486: content_setting_bubble_model_->OnManageCheckboxChecked( This could probably call GetDialogClientView()->UpdateDialogButtons() to trigger the button update after calling OnManageCheckboxChecked, or perhaps something more general (view layout/paint). Either way, it should have a comment like "Toggling the check state may change the dialog button text."
The CQ bit was checked by csharrison@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
Thanks! https://codereview.chromium.org/2793413002/diff/40001/chrome/browser/ui/conte... File chrome/browser/ui/content_settings/content_setting_bubble_model.cc (right): https://codereview.chromium.org/2793413002/diff/40001/chrome/browser/ui/conte... chrome/browser/ui/content_settings/content_setting_bubble_model.cc:1261: is_checked ? IDS_FILTERED_DECEPTIVE_CONTENT_RELOAD_ACTION : IDS_DONE; On 2017/04/06 00:44:56, msw wrote: > aside: It'd be nice if we could just re-use IDS_APP_MENU_RELOAD or another of > the many existing reload strings, and not add another duplicate string. OK, I was not sure of the best practice here, considering there are many duplicate "Reload" strings already. I will remove it in this CL. https://codereview.chromium.org/2793413002/diff/40001/chrome/browser/ui/conte... File chrome/browser/ui/content_settings/content_setting_bubble_model.h (right): https://codereview.chromium.org/2793413002/diff/40001/chrome/browser/ui/conte... chrome/browser/ui/content_settings/content_setting_bubble_model.h:231: void set_learn_more_link(const std::string& link) { On 2017/04/06 00:44:56, msw wrote: > optional nit: this and |set_done_button_text| probably belong below > |add_media_menu| and |set_selected_device| Done. https://codereview.chromium.org/2793413002/diff/40001/chrome/browser/ui/views... File chrome/browser/ui/views/content_setting_bubble_contents.cc (right): https://codereview.chromium.org/2793413002/diff/40001/chrome/browser/ui/views... chrome/browser/ui/views/content_setting_bubble_contents.cc:37: #include "ui/views/controls/button/md_text_button.h" On 2017/04/06 00:44:56, msw wrote: > nit: remove if no longer needed Done. https://codereview.chromium.org/2793413002/diff/40001/chrome/browser/ui/views... chrome/browser/ui/views/content_setting_bubble_contents.cc:421: new views::Checkbox(base::UTF8ToUTF16(bubble_content.manage_text)); On 2017/04/06 00:44:56, msw wrote: > aside: BubbleContent::manage_text should probably be a string16. Ack. Let's address this in a followup? https://codereview.chromium.org/2793413002/diff/40001/chrome/browser/ui/views... chrome/browser/ui/views/content_setting_bubble_contents.cc:486: content_setting_bubble_model_->OnManageCheckboxChecked( On 2017/04/06 00:44:56, msw wrote: > This could probably call GetDialogClientView()->UpdateDialogButtons() to trigger > the button update after calling OnManageCheckboxChecked, or perhaps something > more general (view layout/paint). Either way, it should have a comment like > "Toggling the check state may change the dialog button text." This solution works for me, thanks for the tip.
The CQ bit was checked by csharrison@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...)
lgtm with accessor ordering matching struct member ordering. https://codereview.chromium.org/2793413002/diff/40001/chrome/browser/ui/conte... File chrome/browser/ui/content_settings/content_setting_bubble_model.cc (right): https://codereview.chromium.org/2793413002/diff/40001/chrome/browser/ui/conte... chrome/browser/ui/content_settings/content_setting_bubble_model.cc:1261: is_checked ? IDS_FILTERED_DECEPTIVE_CONTENT_RELOAD_ACTION : IDS_DONE; On 2017/04/06 02:04:58, Charlie Harrison wrote: > On 2017/04/06 00:44:56, msw wrote: > > aside: It'd be nice if we could just re-use IDS_APP_MENU_RELOAD or another of > > the many existing reload strings, and not add another duplicate string. > > OK, I was not sure of the best practice here, considering there are many > duplicate "Reload" strings already. I will remove it in this CL. Generally, it's good to reuse and consolidate string resources when they have the same content and semantic meaning. I'm surprised we don't have a cross-platform IDS_RELOAD = "Reload" already, the app menu string should suffice, and when the underlying string changes, usage should be audited. https://codereview.chromium.org/2793413002/diff/40001/chrome/browser/ui/views... File chrome/browser/ui/views/content_setting_bubble_contents.cc (right): https://codereview.chromium.org/2793413002/diff/40001/chrome/browser/ui/views... chrome/browser/ui/views/content_setting_bubble_contents.cc:421: new views::Checkbox(base::UTF8ToUTF16(bubble_content.manage_text)); On 2017/04/06 02:04:58, Charlie Harrison wrote: > On 2017/04/06 00:44:56, msw wrote: > > aside: BubbleContent::manage_text should probably be a string16. > > Ack. Let's address this in a followup? Acknowledged. https://codereview.chromium.org/2793413002/diff/60001/chrome/browser/ui/conte... File chrome/browser/ui/content_settings/content_setting_bubble_model.h (right): https://codereview.chromium.org/2793413002/diff/60001/chrome/browser/ui/conte... chrome/browser/ui/content_settings/content_setting_bubble_model.h:228: void set_done_button_text(const base::string16& done_button_text) { Move this one function below set_learn_more_link. (match accessor ordering to struct member ordering) https://codereview.chromium.org/2793413002/diff/60001/chrome/browser/ui/conte... chrome/browser/ui/content_settings/content_setting_bubble_model.h:237: void set_manage_text(const std::string& text) { Move this one function above set_show_manage_text_as_checkbox. (match accessor ordering to struct member ordering)
The CQ bit was checked by csharrison@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
csharrison@chromium.org changed reviewers: + engedy@chromium.org
Thanks, engedy could you take a quick pass? msw: One thing we had to do in the Android UI was size the button to be big enough for both the "Reload" and "Done" strings, so it doesn't resize. We may end up having to do that here in a followup. https://codereview.chromium.org/2793413002/diff/60001/chrome/browser/ui/conte... File chrome/browser/ui/content_settings/content_setting_bubble_model.h (right): https://codereview.chromium.org/2793413002/diff/60001/chrome/browser/ui/conte... chrome/browser/ui/content_settings/content_setting_bubble_model.h:228: void set_done_button_text(const base::string16& done_button_text) { On 2017/04/06 02:19:29, msw wrote: > Move this one function below set_learn_more_link. > (match accessor ordering to struct member ordering) Done. https://codereview.chromium.org/2793413002/diff/60001/chrome/browser/ui/conte... chrome/browser/ui/content_settings/content_setting_bubble_model.h:237: void set_manage_text(const std::string& text) { On 2017/04/06 02:19:29, msw wrote: > Move this one function above set_show_manage_text_as_checkbox. > (match accessor ordering to struct member ordering) Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2017/04/06 03:12:59, Charlie Harrison wrote: > msw: One thing we had to do in the Android UI was size the button to be big > enough for both the "Reload" and "Done" strings, so it doesn't resize. We may > end up having to do that here in a followup. That's a good point; for followup: see LabelButton::SetMinSize, or it should just increase width and maintain the bigger size needed if you set one string after the other to the button (that's how I wrote it, but I'm not sure it's still the case...).
Description was changed from ========== [subresource_filter] Update the desktop UI according to mocks This patch adds logic to the content setting bubble to support using a checkbox for policy decisions. Not yet implemented: Figure out a way to update the view from the ContentSettingSubresourceFilterBubbleModel. BUG=689992 ========== to ========== [subresource_filter] Update the desktop UI according to mocks This patch adds logic to the content setting bubble to support using a checkbox for policy decisions. BUG=689992 ==========
LGTM % nit. https://codereview.chromium.org/2793413002/diff/80001/chrome/browser/ui/conte... File chrome/browser/ui/content_settings/content_setting_bubble_model.cc (right): https://codereview.chromium.org/2793413002/diff/80001/chrome/browser/ui/conte... chrome/browser/ui/content_settings/content_setting_bubble_model.cc:1261: set_done_button_text(l10n_util::GetStringUTF16(string_id)); nit: Have you considered setting this to the empty string if |is_checked| is false to get whatever the bubble considers the default?
The CQ bit was checked by csharrison@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Thanks Balazs! https://codereview.chromium.org/2793413002/diff/80001/chrome/browser/ui/conte... File chrome/browser/ui/content_settings/content_setting_bubble_model.cc (right): https://codereview.chromium.org/2793413002/diff/80001/chrome/browser/ui/conte... chrome/browser/ui/content_settings/content_setting_bubble_model.cc:1261: set_done_button_text(l10n_util::GetStringUTF16(string_id)); On 2017/04/06 14:02:59, engedy wrote: > nit: Have you considered setting this to the empty string if |is_checked| is > false to get whatever the bubble considers the default? Good idea, let me do that.
The CQ bit was checked by csharrison@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from msw@chromium.org, engedy@chromium.org Link to the patchset: https://codereview.chromium.org/2793413002/#ps100001 (title: "engedy review")
The CQ bit was unchecked by csharrison@chromium.org
The CQ bit was checked by csharrison@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 100001, "attempt_start_ts": 1491493117678420,
"parent_rev": "2d4bf869bc8ba9245190e1a5e6e72328afd90ef0", "commit_rev":
"5edcdad617c78e16268090993b552065ee04694a"}
Message was sent while issue was closed.
Description was changed from ========== [subresource_filter] Update the desktop UI according to mocks This patch adds logic to the content setting bubble to support using a checkbox for policy decisions. BUG=689992 ========== to ========== [subresource_filter] Update the desktop UI according to mocks This patch adds logic to the content setting bubble to support using a checkbox for policy decisions. BUG=689992 Review-Url: https://codereview.chromium.org/2793413002 Cr-Commit-Position: refs/heads/master@{#462498} Committed: https://chromium.googlesource.com/chromium/src/+/5edcdad617c78e16268090993b55... ==========
Message was sent while issue was closed.
Committed patchset #6 (id:100001) as https://chromium.googlesource.com/chromium/src/+/5edcdad617c78e16268090993b55... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
