Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2356323003:
cros: Change look of partial magnifier. (Closed)
|
Created:
4 years, 3 months ago by sammiequon Modified:
4 years, 2 months ago CC:
chromium-reviews, kalyank, sadrul Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Descriptioncros: Change look of partial magnifier.
I change the partial magnifier to match the mocks given by UI/UX. The new addition was shadow which I added to the border renderer, as well as tweaks to the sizing/color.
Previously: https://screenshot.googleplex.com/htWDLX1Bxdp
Current: https://screenshot.googleplex.com/870KD6fZ1A8
Mocks:
https://drive.google.com/corp/drive/folders/0B_2Uyb2Rhx2OOGRHM0RwamFoRm8
TEST=none
BUG=649194
Committed: https://crrev.com/8de2aed73a434f64d55b0d4fe39cd24a1cadedcf
Cr-Commit-Position: refs/heads/master@{#423307}
Patch Set 1 #
Total comments: 10
Patch Set 2 : Rebased. #Patch Set 3 : Fixed patch set 1 errors. #
Total comments: 6
Patch Set 4 : Fixed patch set 3 errors. #
Total comments: 6
Patch Set 5 : Fixed patch set 4 errors. #
Total comments: 4
Patch Set 6 : Rebased. #Patch Set 7 : Fixed patch set 5 errors. #
Total comments: 4
Patch Set 8 : Fixed patch set 7 errors. #
Messages
Total messages: 30 (13 generated)
Description was changed from ========== cros: Change look of partial magnifier. TEST=none BUG=649194 ========== to ========== cros: Change look of partial magnifier. I change the partial magnifier to match the mocks given by UI/UX. The new addition was shadow which I added to the border renderer, as well as tweaks to the sizing/color. Previously: https://screenshot.googleplex.com/htWDLX1Bxdp Current: https://screenshot.googleplex.com/qe98gwjSW6Z Mocks: https://drive.google.com/corp/drive/folders/0B_2Uyb2Rhx2OOGRHM0RwamFoRm8 TEST=none BUG=649194 ==========
sammiequon@chromium.org changed reviewers: + jdufault@chromium.org
On 2016/09/22 00:42:31, sammiequon wrote: > mailto:sammiequon@chromium.org changed reviewers: > + mailto:jdufault@chromium.org jdufault@ - Please take a look. Thanks!
https://codereview.chromium.org/2356323003/diff/1/ash/magnifier/partial_magni... File ash/magnifier/partial_magnification_controller.cc (right): https://codereview.chromium.org/2356323003/diff/1/ash/magnifier/partial_magni... ash/magnifier/partial_magnification_controller.cc:8: #include "third_party/skia/include/core/SkDrawLooper.h" Is this allowed? https://codereview.chromium.org/2356323003/diff/1/ash/magnifier/partial_magni... ash/magnifier/partial_magnification_controller.cc:282: if (pointer_details.pointer_type == ui::EventPointerType::POINTER_TYPE_PEN) Missed this again.
https://codereview.chromium.org/2356323003/diff/1/ash/magnifier/partial_magni... File ash/magnifier/partial_magnification_controller.cc (right): https://codereview.chromium.org/2356323003/diff/1/ash/magnifier/partial_magni... ash/magnifier/partial_magnification_controller.cc:8: #include "third_party/skia/include/core/SkDrawLooper.h" On 2016/09/22 00:51:46, sammiequon wrote: > Is this allowed? Yes, third_party/skia is explicitly added in ash/DEPS. https://codereview.chromium.org/2356323003/diff/1/ash/magnifier/partial_magni... ash/magnifier/partial_magnification_controller.cc:27: // Radius of the magnifying glass in DIP. This includes the thickness of the I think we should specify the radius of the magnified content without the border, the size of the border, and the size of the shadow separately without combining them together. Doing this will make it easy to tweak and understand the values. I think it will also simplify some of the computations below since it will make the data dependencies explicit. https://codereview.chromium.org/2356323003/diff/1/ash/magnifier/partial_magni... ash/magnifier/partial_magnification_controller.cc:37: const SkColor kBorderOutlineColor = SkColorSetARGB(51, 0, 0, 0); Can we use SK_ColorTRANSPARENT for all of the colors with an alpha of 0? https://codereview.chromium.org/2356323003/diff/1/ash/magnifier/partial_magni... ash/magnifier/partial_magnification_controller.cc:150: magnifier_shadows_.push_back(shadow1); Drop the temporaries and push_back the values directly.
https://codereview.chromium.org/2356323003/diff/1/ash/magnifier/partial_magni... File ash/magnifier/partial_magnification_controller.cc (right): https://codereview.chromium.org/2356323003/diff/1/ash/magnifier/partial_magni... ash/magnifier/partial_magnification_controller.cc:37: const SkColor kBorderOutlineColor = SkColorSetARGB(51, 0, 0, 0); On 2016/09/22 19:59:12, jdufault wrote: > Can we use SK_ColorTRANSPARENT for all of the colors with an alpha of 0? Sorry, ignore this comment. I was reading the colors as RGBA.
https://codereview.chromium.org/2356323003/diff/1/ash/magnifier/partial_magni... File ash/magnifier/partial_magnification_controller.cc (right): https://codereview.chromium.org/2356323003/diff/1/ash/magnifier/partial_magni... ash/magnifier/partial_magnification_controller.cc:8: #include "third_party/skia/include/core/SkDrawLooper.h" On 2016/09/22 19:59:12, jdufault wrote: > On 2016/09/22 00:51:46, sammiequon wrote: > > Is this allowed? > > Yes, third_party/skia is explicitly added in ash/DEPS. Oh right, forgot to check there. Thanks! https://codereview.chromium.org/2356323003/diff/1/ash/magnifier/partial_magni... ash/magnifier/partial_magnification_controller.cc:27: // Radius of the magnifying glass in DIP. This includes the thickness of the On 2016/09/22 19:59:12, jdufault wrote: > I think we should specify the radius of the magnified content without the > border, the size of the border, and the size of the shadow separately without > combining them together. > > Doing this will make it easy to tweak and understand the values. I think it will > also simplify some of the computations below since it will make the data > dependencies explicit. Makes sense, not sure why I didn't feel the need to do this the first time. Done. https://codereview.chromium.org/2356323003/diff/1/ash/magnifier/partial_magni... ash/magnifier/partial_magnification_controller.cc:150: magnifier_shadows_.push_back(shadow1); On 2016/09/22 19:59:12, jdufault wrote: > Drop the temporaries and push_back the values directly. Done.
The CQ bit was checked by sammiequon@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/2356323003/diff/40001/ash/magnifier/partial_m... File ash/magnifier/partial_magnification_controller.cc (right): https://codereview.chromium.org/2356323003/diff/40001/ash/magnifier/partial_m... ash/magnifier/partial_magnification_controller.cc:39: const int kShadowThickness = 24; move kShadowThickness to line 31 so it is next to all of the other sizes https://codereview.chromium.org/2356323003/diff/40001/ash/magnifier/partial_m... ash/magnifier/partial_magnification_controller.cc:109: rect.width() / 2 - (kShadowThickness + kBorderSize) / 2 - Line 109 looks like it is computing the value of kMagnifierRadius. Can we just use kMagnifierRadius directly? int clipping_radius = kMagnifierRadius; if (is_border_) clipping_radius += kShadowThickness + kBorderSize; I'm also a little confused why line on line 110 kShadowThickness + kBorderSize was getting divided by 2. https://codereview.chromium.org/2356323003/diff/40001/ash/magnifier/partial_m... ash/magnifier/partial_magnification_controller.cc:160: shadow_bounds.width() / 2 - kShadowThickness / 2, shadow_paint); Why is kShadowThickness divided in half?
https://codereview.chromium.org/2356323003/diff/40001/ash/magnifier/partial_m... File ash/magnifier/partial_magnification_controller.cc (right): https://codereview.chromium.org/2356323003/diff/40001/ash/magnifier/partial_m... ash/magnifier/partial_magnification_controller.cc:39: const int kShadowThickness = 24; On 2016/09/23 18:16:43, jdufault wrote: > move kShadowThickness to line 31 so it is next to all of the other sizes Done. https://codereview.chromium.org/2356323003/diff/40001/ash/magnifier/partial_m... ash/magnifier/partial_magnification_controller.cc:109: rect.width() / 2 - (kShadowThickness + kBorderSize) / 2 - On 2016/09/23 18:16:43, jdufault wrote: > Line 109 looks like it is computing the value of kMagnifierRadius. Can we just > use kMagnifierRadius directly? > > int clipping_radius = kMagnifierRadius; > if (is_border_) > clipping_radius += kShadowThickness + kBorderSize; > > I'm also a little confused why line on line 110 kShadowThickness + kBorderSize > was getting divided by 2. Done the suggestions. I add a comment(s) which hopefully explains this part better. https://codereview.chromium.org/2356323003/diff/40001/ash/magnifier/partial_m... ash/magnifier/partial_magnification_controller.cc:160: shadow_bounds.width() / 2 - kShadowThickness / 2, shadow_paint); On 2016/09/23 18:16:43, jdufault wrote: > Why is kShadowThickness divided in half? Done.
https://codereview.chromium.org/2356323003/diff/60001/ash/magnifier/partial_m... File ash/magnifier/partial_magnification_controller.cc (right): https://codereview.chromium.org/2356323003/diff/60001/ash/magnifier/partial_m... ash/magnifier/partial_magnification_controller.cc:33: // Offset of the shadow around the magnifying glass in DIP. Please describe what the offset is used for. https://codereview.chromium.org/2356323003/diff/60001/ash/magnifier/partial_m... ash/magnifier/partial_magnification_controller.cc:40: const SkColor kShadowOneColor = SkColorSetARGB(26, 0, 0, 0); Are there better names than kShadowOne, kShadowTwo? What about kTopShadow, kBottomShaddow? kSmallShadow, kBigShadow? https://codereview.chromium.org/2356323003/diff/60001/ash/magnifier/partial_m... ash/magnifier/partial_magnification_controller.cc:56: kShadowThickness * 2 + kShadowOffset * 2; Should the offset be multiplied by 2? Maybe group all of the terms together that need to be doubled, ie, (a + b + c) * 2 + d. Up to you.
Patchset #5 (id:80001) has been deleted
https://codereview.chromium.org/2356323003/diff/60001/ash/magnifier/partial_m... File ash/magnifier/partial_magnification_controller.cc (right): https://codereview.chromium.org/2356323003/diff/60001/ash/magnifier/partial_m... ash/magnifier/partial_magnification_controller.cc:33: // Offset of the shadow around the magnifying glass in DIP. On 2016/09/23 21:42:55, jdufault wrote: > Please describe what the offset is used for. Done. https://codereview.chromium.org/2356323003/diff/60001/ash/magnifier/partial_m... ash/magnifier/partial_magnification_controller.cc:40: const SkColor kShadowOneColor = SkColorSetARGB(26, 0, 0, 0); On 2016/09/23 21:42:55, jdufault wrote: > Are there better names than kShadowOne, kShadowTwo? What about kTopShadow, > kBottomShaddow? kSmallShadow, kBigShadow? Done. https://codereview.chromium.org/2356323003/diff/60001/ash/magnifier/partial_m... ash/magnifier/partial_magnification_controller.cc:56: kShadowThickness * 2 + kShadowOffset * 2; On 2016/09/23 21:42:54, jdufault wrote: > Should the offset be multiplied by 2? > > Maybe group all of the terms together that need to be doubled, ie, (a + b + c) * > 2 + d. Up to you. As discussed offline added a comment explaining this section.
lgtm https://codereview.chromium.org/2356323003/diff/100001/ash/magnifier/partial_... File ash/magnifier/partial_magnification_controller.cc (right): https://codereview.chromium.org/2356323003/diff/100001/ash/magnifier/partial_... ash/magnifier/partial_magnification_controller.cc:116: // Otherwise we want to clip the border, shadow and shadowoffset so we start shadowoffset => shadow offset https://codereview.chromium.org/2356323003/diff/100001/ash/magnifier/partial_... ash/magnifier/partial_magnification_controller.cc:177: const int magnifier_radius = Can we compute this value from the constants instead of using magnifier_window_bounds_.width()? Would it be const int magnifier_radius = kMagnifierRadius + kBorderSize;
Description was changed from ========== cros: Change look of partial magnifier. I change the partial magnifier to match the mocks given by UI/UX. The new addition was shadow which I added to the border renderer, as well as tweaks to the sizing/color. Previously: https://screenshot.googleplex.com/htWDLX1Bxdp Current: https://screenshot.googleplex.com/qe98gwjSW6Z Mocks: https://drive.google.com/corp/drive/folders/0B_2Uyb2Rhx2OOGRHM0RwamFoRm8 TEST=none BUG=649194 ========== to ========== cros: Change look of partial magnifier. I change the partial magnifier to match the mocks given by UI/UX. The new addition was shadow which I added to the border renderer, as well as tweaks to the sizing/color. Previously: https://screenshot.googleplex.com/htWDLX1Bxdp Current: https://screenshot.googleplex.com/870KD6fZ1A8 Mocks: https://drive.google.com/corp/drive/folders/0B_2Uyb2Rhx2OOGRHM0RwamFoRm8 TEST=none BUG=649194 ==========
sammiequon@chromium.org changed reviewers: + jamescook@chromium.org
jamescook@ - Please take a look. Thanks! https://codereview.chromium.org/2356323003/diff/100001/ash/magnifier/partial_... File ash/magnifier/partial_magnification_controller.cc (right): https://codereview.chromium.org/2356323003/diff/100001/ash/magnifier/partial_... ash/magnifier/partial_magnification_controller.cc:116: // Otherwise we want to clip the border, shadow and shadowoffset so we start On 2016/10/04 22:19:47, jdufault wrote: > shadowoffset => shadow offset Done. https://codereview.chromium.org/2356323003/diff/100001/ash/magnifier/partial_... ash/magnifier/partial_magnification_controller.cc:177: const int magnifier_radius = On 2016/10/04 22:19:47, jdufault wrote: > Can we compute this value from the constants instead of using > magnifier_window_bounds_.width()? > > Would it be > > const int magnifier_radius = kMagnifierRadius + kBorderSize; > > Done.
Thanks for including screenshots! https://codereview.chromium.org/2356323003/diff/140001/ash/magnifier/partial_... File ash/magnifier/partial_magnification_controller.cc (right): https://codereview.chromium.org/2356323003/diff/140001/ash/magnifier/partial_... ash/magnifier/partial_magnification_controller.cc:166: shadow_paint.setLooper(gfx::CreateShadowDrawLooper(magnifier_shadows_)); I am not familiar with CreateShadowDrawLooper() but according to https://cs.chromium.org/chromium/src/ui/gfx/skia_util.h?q=createshadowdrawloo... the function is deprecated. Do you want CreateShadowDrawLooperCorrectBlur instead? If not, leave a comment here about why you have to use the deprecated one. https://codereview.chromium.org/2356323003/diff/140001/ash/magnifier/partial_... ash/magnifier/partial_magnification_controller.cc:283: if (pointer_details.pointer_type == ui::EventPointerType::POINTER_TYPE_PEN) For my edification: What's going on here? It looks like you are ignoring stylus events, but then the code below seems to be processing stylus events. Are pen events being sent as mouse events or something? If so, why do you have to skip POINTER_TYPE_PEN?
https://codereview.chromium.org/2356323003/diff/140001/ash/magnifier/partial_... File ash/magnifier/partial_magnification_controller.cc (right): https://codereview.chromium.org/2356323003/diff/140001/ash/magnifier/partial_... ash/magnifier/partial_magnification_controller.cc:166: shadow_paint.setLooper(gfx::CreateShadowDrawLooper(magnifier_shadows_)); On 2016/10/05 19:42:35, James Cook wrote: > I am not familiar with CreateShadowDrawLooper() but according to > https://cs.chromium.org/chromium/src/ui/gfx/skia_util.h?q=createshadowdrawloo... > the function is deprecated. > > Do you want CreateShadowDrawLooperCorrectBlur instead? > > If not, leave a comment here about why you have to use the deprecated one. Done. https://codereview.chromium.org/2356323003/diff/140001/ash/magnifier/partial_... ash/magnifier/partial_magnification_controller.cc:283: if (pointer_details.pointer_type == ui::EventPointerType::POINTER_TYPE_PEN) On 2016/10/05 19:42:35, James Cook wrote: > For my edification: What's going on here? > > It looks like you are ignoring stylus events, but then the code below seems to > be processing stylus events. > > Are pen events being sent as mouse events or something? If so, why do you have > to skip POINTER_TYPE_PEN? My mistake I was testing the magnifier and i forgot to revert this. Thanks for catching this.
LGTM
The CQ bit was checked by sammiequon@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from jdufault@chromium.org Link to the patchset: https://codereview.chromium.org/2356323003/#ps160001 (title: "Fixed patch set 7 errors.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== cros: Change look of partial magnifier. I change the partial magnifier to match the mocks given by UI/UX. The new addition was shadow which I added to the border renderer, as well as tweaks to the sizing/color. Previously: https://screenshot.googleplex.com/htWDLX1Bxdp Current: https://screenshot.googleplex.com/870KD6fZ1A8 Mocks: https://drive.google.com/corp/drive/folders/0B_2Uyb2Rhx2OOGRHM0RwamFoRm8 TEST=none BUG=649194 ========== to ========== cros: Change look of partial magnifier. I change the partial magnifier to match the mocks given by UI/UX. The new addition was shadow which I added to the border renderer, as well as tweaks to the sizing/color. Previously: https://screenshot.googleplex.com/htWDLX1Bxdp Current: https://screenshot.googleplex.com/870KD6fZ1A8 Mocks: https://drive.google.com/corp/drive/folders/0B_2Uyb2Rhx2OOGRHM0RwamFoRm8 TEST=none BUG=649194 ==========
Message was sent while issue was closed.
Committed patchset #8 (id:160001)
Message was sent while issue was closed.
Description was changed from ========== cros: Change look of partial magnifier. I change the partial magnifier to match the mocks given by UI/UX. The new addition was shadow which I added to the border renderer, as well as tweaks to the sizing/color. Previously: https://screenshot.googleplex.com/htWDLX1Bxdp Current: https://screenshot.googleplex.com/870KD6fZ1A8 Mocks: https://drive.google.com/corp/drive/folders/0B_2Uyb2Rhx2OOGRHM0RwamFoRm8 TEST=none BUG=649194 ========== to ========== cros: Change look of partial magnifier. I change the partial magnifier to match the mocks given by UI/UX. The new addition was shadow which I added to the border renderer, as well as tweaks to the sizing/color. Previously: https://screenshot.googleplex.com/htWDLX1Bxdp Current: https://screenshot.googleplex.com/870KD6fZ1A8 Mocks: https://drive.google.com/corp/drive/folders/0B_2Uyb2Rhx2OOGRHM0RwamFoRm8 TEST=none BUG=649194 Committed: https://crrev.com/8de2aed73a434f64d55b0d4fe39cd24a1cadedcf Cr-Commit-Position: refs/heads/master@{#423307} ==========
Message was sent while issue was closed.
Patchset 8 (id:??) landed as https://crrev.com/8de2aed73a434f64d55b0d4fe39cd24a1cadedcf Cr-Commit-Position: refs/heads/master@{#423307} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
