Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2583503002:
Alpha for overlay scrollbars decreased when hovered. (Closed)
DescriptionAlpha for overlay scrollbars decreased when hovered.
BUG=669682
Committed: https://crrev.com/0f321688bc82199ce1551766e185ac6d9267ede5
Cr-Commit-Position: refs/heads/master@{#439583}
Patch Set 1 #
Total comments: 1
Patch Set 2 : three constant values for stroke alpha #
Total comments: 4
Patch Set 3 : Moved constants to function scope. #
Total comments: 2
Patch Set 4 : stroke_alpha only used in overlay case. #Patch Set 5 : two switch cases #Messages
Total messages: 44 (30 generated)
The CQ bit was checked by sahel@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.
The CQ bit was checked by sahel@chromium.org
The CQ bit was unchecked by sahel@chromium.org
Description was changed from ========== Alpha for overlay scrollbars decreased when hovered. BUG=669682 ========== to ========== Alpha for overlay scrollbars decreased when hovered. BUG=669682 ==========
sahel@chromium.org changed reviewers: + bokan@chromium.org
lgtm % comment https://codereview.chromium.org/2583503002/diff/1/ui/native_theme/native_them... File ui/native_theme/native_theme_aura.cc (right): https://codereview.chromium.org/2583503002/diff/1/ui/native_theme/native_them... ui/native_theme/native_theme_aura.cc:229: : thumb_alpha; for consistency, replace thumb_alpha here with a new kOverlayStrokeAlphaNormal
The CQ bit was checked by sahel@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.
sahel@chromium.org changed reviewers: + estade@chromium.org
estade@chromium.org: Please review changes in
https://codereview.chromium.org/2583503002/diff/20001/ui/native_theme/native_... File ui/native_theme/native_theme_aura.cc (right): https://codereview.chromium.org/2583503002/diff/20001/ui/native_theme/native_... ui/native_theme/native_theme_aura.cc:38: constexpr SkAlpha kOverlayStrokeAlphaNormal = 0x4D; nit: the naming on these constants is a little inconsistent. It would be more symmetric if it were OverlayScrollbarFillAlpha vs OverlayScrollbarStrokeAlpha. That said, I would be in favor of inlining all these since they're only used once, and getting rid of the ones that don't differ between stroke and fill. https://codereview.chromium.org/2583503002/diff/20001/ui/native_theme/native_... ui/native_theme/native_theme_aura.cc:218: stroke_alpha = overlay ? kOverlayStrokeAlphaPressed : 0x80; stroke_alpha is unused when !overlay https://codereview.chromium.org/2583503002/diff/20001/ui/native_theme/native_... ui/native_theme/native_theme_aura.cc:244: thumb_rect.Inset(kStrokeWidth, kStrokeWidth); nit: you can do Inset(gfx::Insets(kStrokeWidth)); (also L240)
https://codereview.chromium.org/2583503002/diff/20001/ui/native_theme/native_... File ui/native_theme/native_theme_aura.cc (right): https://codereview.chromium.org/2583503002/diff/20001/ui/native_theme/native_... ui/native_theme/native_theme_aura.cc:38: constexpr SkAlpha kOverlayStrokeAlphaNormal = 0x4D; On 2016/12/15 22:02:49, Evan Stade wrote: > nit: the naming on these constants is a little inconsistent. It would be more > symmetric if it were OverlayScrollbarFillAlpha vs OverlayScrollbarStrokeAlpha. > That said, I would be in favor of inlining all these since they're only used > once, and getting rid of the ones that don't differ between stroke and fill. Agreed on naming. Personally, I like having named constants since it makes it easy to see where the style values are coming from and where to change but a middle ground might be to move them into the scope where they're used.
The CQ bit was checked by sahel@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.
I changed the names and moved constants to the function scope.
https://codereview.chromium.org/2583503002/diff/40001/ui/native_theme/native_... File ui/native_theme/native_theme_aura.cc (right): https://codereview.chromium.org/2583503002/diff/40001/ui/native_theme/native_... ui/native_theme/native_theme_aura.cc:220: stroke_alpha = overlay ? kOverlayScrollbarStrokeAlphaPressed : 0x80; this ternary still doesn't make sense to me because you never use it in the non overlay case.
The CQ bit was checked by sahel@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/2583503002/diff/40001/ui/native_theme/native_... File ui/native_theme/native_theme_aura.cc (right): https://codereview.chromium.org/2583503002/diff/40001/ui/native_theme/native_... ui/native_theme/native_theme_aura.cc:220: stroke_alpha = overlay ? kOverlayScrollbarStrokeAlphaPressed : 0x80; On 2016/12/19 16:06:56, Evan Stade wrote: > this ternary still doesn't make sense to me because you never use it in the non > overlay case. You are right, it's only used for overlay case. I wanted to do all the alpha assignments in one part of code with one switch-case. Done, moved it to overlay specific part of the function.
On 2016/12/19 17:57:26, sahel wrote: > https://codereview.chromium.org/2583503002/diff/40001/ui/native_theme/native_... > File ui/native_theme/native_theme_aura.cc (right): > > https://codereview.chromium.org/2583503002/diff/40001/ui/native_theme/native_... > ui/native_theme/native_theme_aura.cc:220: stroke_alpha = overlay ? > kOverlayScrollbarStrokeAlphaPressed : 0x80; > On 2016/12/19 16:06:56, Evan Stade wrote: > > this ternary still doesn't make sense to me because you never use it in the > non > > overlay case. > > You are right, it's only used for overlay case. I wanted to do all the alpha > assignments in one part of code with one switch-case. > Done, moved it to overlay specific part of the function. sorry for being unclear. All I really wanted you to do was to remove the ternary. But if you're going to have two switch statements, why not put one in each branch (overlay and !overlay)? Then you don't need any ternaries at all and you can move the constants into the scope where they're actually needed.
The CQ bit was checked by sahel@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.
Changed as recommended.
lgtm
The CQ bit was checked by sahel@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from bokan@chromium.org Link to the patchset: https://codereview.chromium.org/2583503002/#ps80001 (title: "two switch cases")
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": 80001, "attempt_start_ts": 1482184374986650,
"parent_rev": "f9097f9e3d45b55dedb0ccfc5c8f03ffa894937f", "commit_rev":
"378fa4c2a6e1ce13f958aa42497205bf9ff5c48b"}
Message was sent while issue was closed.
Description was changed from ========== Alpha for overlay scrollbars decreased when hovered. BUG=669682 ========== to ========== Alpha for overlay scrollbars decreased when hovered. BUG=669682 Review-Url: https://codereview.chromium.org/2583503002 ==========
Message was sent while issue was closed.
Committed patchset #5 (id:80001)
Message was sent while issue was closed.
Description was changed from ========== Alpha for overlay scrollbars decreased when hovered. BUG=669682 Review-Url: https://codereview.chromium.org/2583503002 ========== to ========== Alpha for overlay scrollbars decreased when hovered. BUG=669682 Committed: https://crrev.com/0f321688bc82199ce1551766e185ac6d9267ede5 Cr-Commit-Position: refs/heads/master@{#439583} ==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/0f321688bc82199ce1551766e185ac6d9267ede5 Cr-Commit-Position: refs/heads/master@{#439583} |
