Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2526863004:
harmony: fix collected cookies dialog spacing and button insets (Closed)
|
Created:
4 years ago by Elly Fong-Jones Modified:
4 years ago Reviewers:
sky CC:
chromium-reviews, tfarina Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Descriptionharmony: fix collected cookies dialog spacing and button insets
This change:
1) Swaps the block/allow/remove button spacing over to RELATED_BUTTON
instead of RELATED_CONTROL (-2px margin in non-Harmony mode)
2) Changes DialogClientView to use the ViewsDelegate's dialog button
insets more. Only Harmony has a non-zero top inset for the buttons,
so behavior in non-Harmony mode is unaffected.
BUG=610428
Committed: https://crrev.com/b0c6c3336fd015d6ffa3c7efff3a7b264eaf5d48
Cr-Commit-Position: refs/heads/master@{#435278}
Patch Set 1 #
Total comments: 4
Messages
Total messages: 17 (6 generated)
Description was changed from ========== harmony: fix collected cookies dialog spacing and button insets This change: 1) Swaps the block/allow/remove button spacing over to RELATED_BUTTON instead of RELATED_CONTROL (-2px margin in non-Harmony mode) 2) Changes DialogClientView to use the ViewsDelegate's dialog button insets more. Only Harmony has a non-zero top inset for the buttons, so behavior in non-Harmony mode is unaffected. BUG=610428 ========== to ========== harmony: fix collected cookies dialog spacing and button insets This change: 1) Swaps the block/allow/remove button spacing over to RELATED_BUTTON instead of RELATED_CONTROL (-2px margin in non-Harmony mode) 2) Changes DialogClientView to use the ViewsDelegate's dialog button insets more. Only Harmony has a non-zero top inset for the buttons, so behavior in non-Harmony mode is unaffected. BUG=610428 ==========
ellyjones@chromium.org changed reviewers: + sky@chromium.org
sky: ptal? :)
https://codereview.chromium.org/2526863004/diff/1/ui/views/window/dialog_clie... File ui/views/window/dialog_client_view.cc (right): https://codereview.chromium.org/2526863004/diff/1/ui/views/window/dialog_clie... ui/views/window/dialog_client_view.cc:224: // If the ViewsDelegate supplies a non-zero top inset, use that; Might the ViewsDelegate supply 0?
https://codereview.chromium.org/2526863004/diff/1/ui/views/window/dialog_clie... File ui/views/window/dialog_client_view.cc (right): https://codereview.chromium.org/2526863004/diff/1/ui/views/window/dialog_clie... ui/views/window/dialog_client_view.cc:224: // If the ViewsDelegate supplies a non-zero top inset, use that; On 2016/11/23 20:28:37, sky wrote: > Might the ViewsDelegate supply 0? Yes - in non-Harmony mode, ChromeViewsDelegate::GetDialogButtonInsets() returns a zero top inset, since that was the default before. I don't know why it was the default, but I fear to change it :)
https://codereview.chromium.org/2526863004/diff/1/ui/views/window/dialog_clie... File ui/views/window/dialog_client_view.cc (right): https://codereview.chromium.org/2526863004/diff/1/ui/views/window/dialog_clie... ui/views/window/dialog_client_view.cc:224: // If the ViewsDelegate supplies a non-zero top inset, use that; On 2016/11/28 17:02:49, Elly Fong-Jones wrote: > On 2016/11/23 20:28:37, sky wrote: > > Might the ViewsDelegate supply 0? > > Yes - in non-Harmony mode, ChromeViewsDelegate::GetDialogButtonInsets() returns > a zero top inset, since that was the default before. I don't know why it was the > default, but I fear to change it :) If it returns 0 then you fallback to using the kRelatedControlVerticalSpacing. Is that expected?
On 2016/11/28 17:44:35, sky wrote: > https://codereview.chromium.org/2526863004/diff/1/ui/views/window/dialog_clie... > File ui/views/window/dialog_client_view.cc (right): > > https://codereview.chromium.org/2526863004/diff/1/ui/views/window/dialog_clie... > ui/views/window/dialog_client_view.cc:224: // If the ViewsDelegate supplies a > non-zero top inset, use that; > On 2016/11/28 17:02:49, Elly Fong-Jones wrote: > > On 2016/11/23 20:28:37, sky wrote: > > > Might the ViewsDelegate supply 0? > > > > Yes - in non-Harmony mode, ChromeViewsDelegate::GetDialogButtonInsets() > returns > > a zero top inset, since that was the default before. I don't know why it was > the > > default, but I fear to change it :) > > If it returns 0 then you fallback to using the kRelatedControlVerticalSpacing. > Is that expected? Yes, it is. This function used to always use kRelatedControlVerticalSpacing.
https://codereview.chromium.org/2526863004/diff/1/ui/views/window/dialog_clie... File ui/views/window/dialog_client_view.cc (right): https://codereview.chromium.org/2526863004/diff/1/ui/views/window/dialog_clie... ui/views/window/dialog_client_view.cc:224: // If the ViewsDelegate supplies a non-zero top inset, use that; On 2016/11/28 17:44:35, sky wrote: > On 2016/11/28 17:02:49, Elly Fong-Jones wrote: > > On 2016/11/23 20:28:37, sky wrote: > > > Might the ViewsDelegate supply 0? > > > > Yes - in non-Harmony mode, ChromeViewsDelegate::GetDialogButtonInsets() > returns > > a zero top inset, since that was the default before. I don't know why it was > the > > default, but I fear to change it :) > > If it returns 0 then you fallback to using the kRelatedControlVerticalSpacing. > Is that expected? I don't think I'm being clear. Let me try again. If you implement the way you have it now even if the delegate returns 0, it's is forced to kRelatedControlVerticalSpacing. Are you sure you don't want: int spacing = ViewsDelegate::GetInstance() ? ViewsDelegate::GetInstance()->Get.... : kRelatedControlVerticalSpacing; That way the delegate can return 0 and really get 0.
On 2016/11/28 19:05:06, sky wrote: > https://codereview.chromium.org/2526863004/diff/1/ui/views/window/dialog_clie... > File ui/views/window/dialog_client_view.cc (right): > > https://codereview.chromium.org/2526863004/diff/1/ui/views/window/dialog_clie... > ui/views/window/dialog_client_view.cc:224: // If the ViewsDelegate supplies a > non-zero top inset, use that; > On 2016/11/28 17:44:35, sky wrote: > > On 2016/11/28 17:02:49, Elly Fong-Jones wrote: > > > On 2016/11/23 20:28:37, sky wrote: > > > > Might the ViewsDelegate supply 0? > > > > > > Yes - in non-Harmony mode, ChromeViewsDelegate::GetDialogButtonInsets() > > returns > > > a zero top inset, since that was the default before. I don't know why it was > > the > > > default, but I fear to change it :) > > > > If it returns 0 then you fallback to using the kRelatedControlVerticalSpacing. > > Is that expected? > > I don't think I'm being clear. Let me try again. > If you implement the way you have it now even if the delegate returns 0, it's is > forced to kRelatedControlVerticalSpacing. Are you sure you don't want: > > int spacing = ViewsDelegate::GetInstance() ? > ViewsDelegate::GetInstance()->Get.... : kRelatedControlVerticalSpacing; > > That way the delegate can return 0 and really get 0. I could introduce a separate GetDialogButtonTopInset() or something. The issue is that the existing GetDialogButtonInsets() returns (0, x, x, x) and if I change it to simply return (x, x, x, x) dialogs will end up with extra padding in them that shouldn't be there. Do you think I should introduce a separate ViewsDelegate method for this?
It sounds like you don't need the flexibility of what I'm outlining, so go with this (it's simpler). LGTM
The CQ bit was checked by ellyjones@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": 1, "attempt_start_ts": 1480519617194500, "parent_rev":
"28f6cd1d80aeb8e950f593b1517b8b9685d888d0", "commit_rev":
"a304bdb8a1a447d9c8804efd1ecc5f1e10d6e09e"}
Message was sent while issue was closed.
Description was changed from ========== harmony: fix collected cookies dialog spacing and button insets This change: 1) Swaps the block/allow/remove button spacing over to RELATED_BUTTON instead of RELATED_CONTROL (-2px margin in non-Harmony mode) 2) Changes DialogClientView to use the ViewsDelegate's dialog button insets more. Only Harmony has a non-zero top inset for the buttons, so behavior in non-Harmony mode is unaffected. BUG=610428 ========== to ========== harmony: fix collected cookies dialog spacing and button insets This change: 1) Swaps the block/allow/remove button spacing over to RELATED_BUTTON instead of RELATED_CONTROL (-2px margin in non-Harmony mode) 2) Changes DialogClientView to use the ViewsDelegate's dialog button insets more. Only Harmony has a non-zero top inset for the buttons, so behavior in non-Harmony mode is unaffected. BUG=610428 ==========
Message was sent while issue was closed.
Committed patchset #1 (id:1)
Message was sent while issue was closed.
Description was changed from ========== harmony: fix collected cookies dialog spacing and button insets This change: 1) Swaps the block/allow/remove button spacing over to RELATED_BUTTON instead of RELATED_CONTROL (-2px margin in non-Harmony mode) 2) Changes DialogClientView to use the ViewsDelegate's dialog button insets more. Only Harmony has a non-zero top inset for the buttons, so behavior in non-Harmony mode is unaffected. BUG=610428 ========== to ========== harmony: fix collected cookies dialog spacing and button insets This change: 1) Swaps the block/allow/remove button spacing over to RELATED_BUTTON instead of RELATED_CONTROL (-2px margin in non-Harmony mode) 2) Changes DialogClientView to use the ViewsDelegate's dialog button insets more. Only Harmony has a non-zero top inset for the buttons, so behavior in non-Harmony mode is unaffected. BUG=610428 Committed: https://crrev.com/b0c6c3336fd015d6ffa3c7efff3a7b264eaf5d48 Cr-Commit-Position: refs/heads/master@{#435278} ==========
Message was sent while issue was closed.
Patchset 1 (id:??) landed as https://crrev.com/b0c6c3336fd015d6ffa3c7efff3a7b264eaf5d48 Cr-Commit-Position: refs/heads/master@{#435278} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
