Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2724403005:
Add a 1px border stroke to the shelf button active app indicator. (Closed)
|
Created:
3 years, 9 months ago by Evan Stade Modified:
3 years, 9 months ago Reviewers:
Daniel Erat CC:
chromium-reviews, kalyank, sadrul Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdd a 1px border stroke to the shelf button active app indicator.
BUG=670970
Review-Url: https://codereview.chromium.org/2724403005
Cr-Commit-Position: refs/heads/master@{#454741}
Committed: https://chromium.googlesource.com/chromium/src/+/7367db27e8ba876cd256abe839deda4078a32d9d
Patch Set 1 #
Total comments: 3
Patch Set 2 : adjust color and position #Messages
Total messages: 18 (12 generated)
estade@chromium.org changed reviewers: + derat@chromium.org
personally I think the stroke is too light, but we can easily change the opacity later.
The CQ bit was checked by estade@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...
https://codereview.chromium.org/2724403005/diff/1/ash/common/shelf/shelf_butt... File ash/common/shelf/shelf_button.cc (right): https://codereview.chromium.org/2724403005/diff/1/ash/common/shelf/shelf_butt... ash/common/shelf/shelf_button.cc:149: const float dsf = canvas->UndoDeviceScaleFactor(); i don't know much about device scale factors, but should you be wrapping all of this in Save/Restore calls to ensure that the original scale is restored? is it just safe to do here because this is at the end of the method?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by estade@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: This issue passed the CQ dry run.
https://codereview.chromium.org/2724403005/diff/1/ash/common/shelf/shelf_butt... File ash/common/shelf/shelf_button.cc (right): https://codereview.chromium.org/2724403005/diff/1/ash/common/shelf/shelf_butt... ash/common/shelf/shelf_button.cc:149: const float dsf = canvas->UndoDeviceScaleFactor(); On 2017/03/03 23:15:52, Daniel Erat wrote: > i don't know much about device scale factors, but should you be wrapping all of > this in Save/Restore calls to ensure that the original scale is restored? is it > just safe to do here because this is at the end of the method? the scopedcanvas above takes care of that. Technically I think it isn't even necessary because a new canvas is created for every View::OnPaint, but it's hard to keep track of when you need it and when not, so I just always add it.
lgtm https://codereview.chromium.org/2724403005/diff/1/ash/common/shelf/shelf_butt... File ash/common/shelf/shelf_button.cc (right): https://codereview.chromium.org/2724403005/diff/1/ash/common/shelf/shelf_butt... ash/common/shelf/shelf_button.cc:149: const float dsf = canvas->UndoDeviceScaleFactor(); On 2017/03/04 01:02:22, Evan Stade wrote: > On 2017/03/03 23:15:52, Daniel Erat wrote: > > i don't know much about device scale factors, but should you be wrapping all > of > > this in Save/Restore calls to ensure that the original scale is restored? is > it > > just safe to do here because this is at the end of the method? > > the scopedcanvas above takes care of that. Technically I think it isn't even > necessary because a new canvas is created for every View::OnPaint, but it's hard > to keep track of when you need it and when not, so I just always add it. ah, sorry. i missed both the fact that a ScopedCanvas class exists and that you were already using it here. carry on.
The CQ bit was checked by estade@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": 20001, "attempt_start_ts": 1488590772101520,
"parent_rev": "9f04f39d54f599a346c59670f91923f4af05e416", "commit_rev":
"7367db27e8ba876cd256abe839deda4078a32d9d"}
Message was sent while issue was closed.
Description was changed from ========== Add a 1px border stroke to the shelf button active app indicator. BUG=670970 ========== to ========== Add a 1px border stroke to the shelf button active app indicator. BUG=670970 Review-Url: https://codereview.chromium.org/2724403005 Cr-Commit-Position: refs/heads/master@{#454741} Committed: https://chromium.googlesource.com/chromium/src/+/7367db27e8ba876cd256abe839de... ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001) as https://chromium.googlesource.com/chromium/src/+/7367db27e8ba876cd256abe839de... |
