Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2422473002:
Fix wiring for Page Info buttons/links. (Closed)
DescriptionFix wiring for Page Info buttons/links.
This also includes a partial revert of
crrev.com/0af7d0edfddd038461850cbc8957f1fba1686df9 to force proper sizing of the "Re-enable warnings" styled label.
BUG=655872
Committed: https://crrev.com/633c166bf730a58a847a6705c46ec08989a74db0
Cr-Commit-Position: refs/heads/master@{#425401}
Patch Set 1 #Patch Set 2 : Fix wiring for Page Info buttons/links. #
Total comments: 11
Patch Set 3 : Fix wiring for Page Info buttons/links. #Messages
Total messages: 12 (4 generated)
lgarron@chromium.org changed reviewers: + msw@chromium.org
msw@, could you review? https://codereview.chromium.org/2422473002/diff/20001/chrome/browser/ui/views... File chrome/browser/ui/views/website_settings/website_settings_popup_view.cc (right): https://codereview.chromium.org/2422473002/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/website_settings/website_settings_popup_view.cc:328: reset_decisions_label_->SizeToFit(0); I removed this (and InvalidateLayout() below) in https://codereview.chromium.org/2412363002 to rely on the Views hierarchy. It seems we still need to force layout somehow, so I've brought this up and made a note in https://crbug.com/655880 to follow up. https://codereview.chromium.org/2422473002/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/website_settings/website_settings_popup_view.cc:544: DCHECK_EQ(BUTTON_CLOSE, button->id()); This is not strictly necessary, but I'd like to make sure to check all button/link/styled label IDs to prevent bugs like this in the future. (Do you have any advice on testing that the button with a given text does what you expect?)
https://codereview.chromium.org/2422473002/diff/20001/chrome/browser/ui/views... File chrome/browser/ui/views/website_settings/website_settings_popup_view.cc (right): https://codereview.chromium.org/2422473002/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/website_settings/website_settings_popup_view.cc:777: case LINK_COOKIE_DIALOG: Also note that `cookie_dialog_link_` is a member variable, so we could check it against `source` directly. However, I think it's easier to read and more future-proof to use Views IDs for every possible button/label/styled label in Page Info.
Bummer we missed these problems in code review. lgtm with nits. https://codereview.chromium.org/2422473002/diff/20001/chrome/browser/ui/views... File chrome/browser/ui/views/website_settings/website_settings_popup_view.cc (right): https://codereview.chromium.org/2422473002/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/website_settings/website_settings_popup_view.cc:328: reset_decisions_label_->SizeToFit(0); On 2016/10/14 01:18:40, lgarron wrote: > I removed this (and InvalidateLayout() below) in > https://codereview.chromium.org/2412363002 to rely on the Views hierarchy. > It seems we still need to force layout somehow, so I've brought this up and made > a note in https://crbug.com/655880 to follow up. Should this also restore details_label_->SizeToFit(0); after line 302? (and I presume the LEADING->FILL changes from that CL are still okay) https://codereview.chromium.org/2422473002/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/website_settings/website_settings_popup_view.cc:544: DCHECK_EQ(BUTTON_CLOSE, button->id()); On 2016/10/14 01:18:40, lgarron wrote: > This is not strictly necessary, but I'd like to make sure to check all > button/link/styled label IDs to prevent bugs like this in the future. > (Do you have any advice on testing that the button with a given text does what > you expect?) This is a good check. I'd encourage you to add automated tests for each button. https://codereview.chromium.org/2422473002/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/website_settings/website_settings_popup_view.cc:760: // Both switch cases require accessing web_contents(), so we check it here for optional nit: end comment after "here." for a one-liner https://codereview.chromium.org/2422473002/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/website_settings/website_settings_popup_view.cc:766: // TODO(crbug.com/655876): This opens the general Content Settings pane, Thanks for filing a bug and noting it here. https://codereview.chromium.org/2422473002/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/website_settings/website_settings_popup_view.cc:777: case LINK_COOKIE_DIALOG: On 2016/10/14 01:21:31, lgarron wrote: > Also note that `cookie_dialog_link_` is a member variable, so we could check it > against `source` directly. > However, I think it's easier to read and more future-proof to use Views IDs for > every possible button/label/styled label in Page Info. I agree, checking view ids is often nicer than checking member pointers.
The CQ bit was checked by lgarron@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from msw@chromium.org Link to the patchset: https://codereview.chromium.org/2422473002/#ps40001 (title: "Fix wiring for Page Info buttons/links.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
still lgtm, with a q https://codereview.chromium.org/2422473002/diff/20001/chrome/browser/ui/views... File chrome/browser/ui/views/website_settings/website_settings_popup_view.cc (right): https://codereview.chromium.org/2422473002/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/website_settings/website_settings_popup_view.cc:328: reset_decisions_label_->SizeToFit(0); On 2016/10/14 01:58:56, msw wrote: > On 2016/10/14 01:18:40, lgarron wrote: > > I removed this (and InvalidateLayout() below) in > > https://codereview.chromium.org/2412363002 to rely on the Views hierarchy. > > It seems we still need to force layout somehow, so I've brought this up and > made > > a note in https://crbug.com/655880 to follow up. > > Should this also restore details_label_->SizeToFit(0); after line 302? > (and I presume the LEADING->FILL changes from that CL are still okay) Please respond to questions.
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Description was changed from ========== Fix wiring for Page Info buttons/links. This also includes a partial revert of crrev.com/0af7d0edfddd038461850cbc8957f1fba1686df9 to force proper sizing of the "Re-enable warnings" styled label. BUG=655872 ========== to ========== Fix wiring for Page Info buttons/links. This also includes a partial revert of crrev.com/0af7d0edfddd038461850cbc8957f1fba1686df9 to force proper sizing of the "Re-enable warnings" styled label. BUG=655872 Committed: https://crrev.com/633c166bf730a58a847a6705c46ec08989a74db0 Cr-Commit-Position: refs/heads/master@{#425401} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/633c166bf730a58a847a6705c46ec08989a74db0 Cr-Commit-Position: refs/heads/master@{#425401}
Message was sent while issue was closed.
(Sorry, forgot to hit "publish" with "commit".) https://codereview.chromium.org/2422473002/diff/20001/chrome/browser/ui/views... File chrome/browser/ui/views/website_settings/website_settings_popup_view.cc (right): https://codereview.chromium.org/2422473002/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/website_settings/website_settings_popup_view.cc:328: reset_decisions_label_->SizeToFit(0); On 2016/10/14 at 01:58:56, msw wrote: > On 2016/10/14 01:18:40, lgarron wrote: > > I removed this (and InvalidateLayout() below) in > > https://codereview.chromium.org/2412363002 to rely on the Views hierarchy. > > It seems we still need to force layout somehow, so I've brought this up and made > > a note in https://crbug.com/655880 to follow up. > > Should this also restore details_label_->SizeToFit(0); after line 302? > (and I presume the LEADING->FILL changes from that CL are still okay) details_label_ is created at initialization time, so it sizes correctly. (It would have been obvious if details_label_ had needed this; unlike reset_decisions, it is always visible.) https://codereview.chromium.org/2422473002/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/website_settings/website_settings_popup_view.cc:760: // Both switch cases require accessing web_contents(), so we check it here for On 2016/10/14 at 01:58:56, msw wrote: > optional nit: end comment after "here." for a one-liner Definitely also reads better that way. Done. |
