Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2269383002:
Magnifier border is now more visible on light backgrounds. (Closed)
|
Created:
4 years, 4 months ago by sammiequon Modified:
4 years, 3 months ago CC:
chromium-reviews, kalyank, sadrul, oshima+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@lkgr Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Descriptionchromeos: Magnifier border is now more visible on light backgrounds.
Previously the partial magnifier border was difficult to see on white colored backgrounds. I add a textured layer instead of a solid color, so the border is outlined in black, so that it is visible on all backgrounds.
BUG=638996
TEST=none
https://screenshot.googleplex.com/htWDLX1Bxdp
Committed: https://crrev.com/da331ee46dfead28a5b2af2379f9fad320509957
Committed: https://crrev.com/904192168450735ec94121000bf86d645a47b857
Cr-Original-Commit-Position: refs/heads/master@{#418785}
Cr-Commit-Position: refs/heads/master@{#419199}
Patch Set 1 #
Total comments: 2
Patch Set 2 : Rebased. #Patch Set 3 : Fixed patch set 1 errors. #
Total comments: 8
Patch Set 4 : Fixed patch set 3 errors. #
Total comments: 12
Patch Set 5 : Fixed patch set 4 errors. #
Total comments: 2
Patch Set 6 : Nit. #
Total comments: 14
Patch Set 7 : Fixed patch set 6 errors. #
Total comments: 4
Patch Set 8 : Nits. #
Total comments: 2
Patch Set 9 : Fixed memory leak. #
Total comments: 4
Patch Set 10 : Nits. #
Dependent Patchsets: Messages
Total messages: 45 (16 generated)
Patchset #1 (id:1) has been deleted
The edited files are: ash/magnifier/partial_magnification_controller.h - line 51 ash/magnifier/partial_magnification_controller.cc - lines 26-29,112-156,296,298
Description was changed from ========== Magnifier border is now more visible on light backgrounds. BUG=638996 ========== to ========== Magnifier border is now more visible on light backgrounds. BUG=638996 ==========
sammiequon@chromium.org changed reviewers: + jdufault@chromium.org
On 2016/08/24 00:57:58, sammiequon wrote: > The edited files are: > > ash/magnifier/partial_magnification_controller.h - line 51 > ash/magnifier/partial_magnification_controller.cc - lines 26-29,112-156,296,298 jdufault@- Please take a look. Thanks!
The partial magnifier has landed so if you set your upstream to origin/master the CL should should show up with only your changes. $ git reparent-branch origin/master https://codereview.chromium.org/2269383002/diff/20001/ash/magnifier/partial_m... File ash/magnifier/partial_magnification_controller.cc (right): https://codereview.chromium.org/2269383002/diff/20001/ash/magnifier/partial_m... ash/magnifier/partial_magnification_controller.cc:138: recorder.canvas()->DrawCircle( Add comment saying which circle is the inner outline and which one is the outer outline.
https://codereview.chromium.org/2269383002/diff/20001/ash/magnifier/partial_m... File ash/magnifier/partial_magnification_controller.cc (right): https://codereview.chromium.org/2269383002/diff/20001/ash/magnifier/partial_m... ash/magnifier/partial_magnification_controller.cc:138: recorder.canvas()->DrawCircle( On 2016/08/24 21:15:39, jdufault wrote: > Add comment saying which circle is the inner outline and which one is the outer > outline. Done.
https://codereview.chromium.org/2269383002/diff/60001/ash/magnifier/partial_m... File ash/magnifier/partial_magnification_controller.cc (right): https://codereview.chromium.org/2269383002/diff/60001/ash/magnifier/partial_m... ash/magnifier/partial_magnification_controller.cc:117: BorderRenderer(const gfx::Rect& bounds) { bounds_ = bounds; } use ctor list : bounds_(bounds) https://codereview.chromium.org/2269383002/diff/60001/ash/magnifier/partial_m... ash/magnifier/partial_magnification_controller.cc:122: // Overridden from LayerDelegate. // ui::LayerDelegate: https://codereview.chromium.org/2269383002/diff/60001/ash/magnifier/partial_m... ash/magnifier/partial_magnification_controller.cc:135: // Draw the border outlines. Remove comment? https://codereview.chromium.org/2269383002/diff/60001/ash/magnifier/partial_m... ash/magnifier/partial_magnification_controller.cc:155: gfx::Rect bounds_; newline above here
https://codereview.chromium.org/2269383002/diff/60001/ash/magnifier/partial_m... File ash/magnifier/partial_magnification_controller.cc (right): https://codereview.chromium.org/2269383002/diff/60001/ash/magnifier/partial_m... ash/magnifier/partial_magnification_controller.cc:117: BorderRenderer(const gfx::Rect& bounds) { bounds_ = bounds; } On 2016/08/25 19:53:25, jdufault wrote: > use ctor list > > : bounds_(bounds) Done. https://codereview.chromium.org/2269383002/diff/60001/ash/magnifier/partial_m... ash/magnifier/partial_magnification_controller.cc:122: // Overridden from LayerDelegate. On 2016/08/25 19:53:25, jdufault wrote: > // ui::LayerDelegate: Done. https://codereview.chromium.org/2269383002/diff/60001/ash/magnifier/partial_m... ash/magnifier/partial_magnification_controller.cc:135: // Draw the border outlines. On 2016/08/25 19:53:25, jdufault wrote: > Remove comment? Done. https://codereview.chromium.org/2269383002/diff/60001/ash/magnifier/partial_m... ash/magnifier/partial_magnification_controller.cc:155: gfx::Rect bounds_; On 2016/08/25 19:53:25, jdufault wrote: > newline above here Done.
https://codereview.chromium.org/2269383002/diff/80001/ash/magnifier/partial_m... File ash/magnifier/partial_magnification_controller.cc (right): https://codereview.chromium.org/2269383002/diff/80001/ash/magnifier/partial_m... ash/magnifier/partial_magnification_controller.cc:26: // Thickness of the outline around magnifiying glass border. specify units (DIP) https://codereview.chromium.org/2269383002/diff/80001/ash/magnifier/partial_m... ash/magnifier/partial_magnification_controller.cc:115: : public ui::LayerDelegate { Is this from git cl format? Can you run it just in case? https://codereview.chromium.org/2269383002/diff/80001/ash/magnifier/partial_m... ash/magnifier/partial_magnification_controller.cc:126: // Draw the border. What about adding inner to make it more clear that this is only part of the border? // Draw the inner border. https://codereview.chromium.org/2269383002/diff/80001/ash/magnifier/partial_m... ash/magnifier/partial_magnification_controller.cc:130: paint.setStyle(SkPaint::kStroke_Style); Move all of the common paint config options away from the comment so it is clear that these settings are global / shared among every paint operation. SkPaint paint; paint.setAntiAlias(true); paint.setStylue(SkPaint::kStroke_Style); // Draw the inner border. ... https://codereview.chromium.org/2269383002/diff/80001/ash/magnifier/partial_m... ash/magnifier/partial_magnification_controller.cc:137: // Draw border outer outline. What about moving these comments above setStrokeWidth and collapsing them to // Draw the border outer outline and then draw the border inner outline. paint.setStrokeWidth(...) paint.setColor(...) ...DrawCircle(...) ...DrawCircle(...) https://codereview.chromium.org/2269383002/diff/80001/ash/magnifier/partial_m... File ash/magnifier/partial_magnification_controller.h (right): https://codereview.chromium.org/2269383002/diff/80001/ash/magnifier/partial_m... ash/magnifier/partial_magnification_controller.h:51: class BorderRenderer; Remove forward decl.
Patchset #5 (id:100001) has been deleted
https://codereview.chromium.org/2269383002/diff/80001/ash/magnifier/partial_m... File ash/magnifier/partial_magnification_controller.cc (right): https://codereview.chromium.org/2269383002/diff/80001/ash/magnifier/partial_m... ash/magnifier/partial_magnification_controller.cc:26: // Thickness of the outline around magnifiying glass border. On 2016/08/26 22:56:46, jdufault wrote: > specify units (DIP) Done. https://codereview.chromium.org/2269383002/diff/80001/ash/magnifier/partial_m... ash/magnifier/partial_magnification_controller.cc:115: : public ui::LayerDelegate { On 2016/08/26 22:56:46, jdufault wrote: > Is this from git cl format? Can you run it just in case? Yes, ran it again. It is 81 long if one line. https://codereview.chromium.org/2269383002/diff/80001/ash/magnifier/partial_m... ash/magnifier/partial_magnification_controller.cc:126: // Draw the border. On 2016/08/26 22:56:46, jdufault wrote: > What about adding inner to make it more clear that this is only part of the > border? > > // Draw the inner border. Done. https://codereview.chromium.org/2269383002/diff/80001/ash/magnifier/partial_m... ash/magnifier/partial_magnification_controller.cc:130: paint.setStyle(SkPaint::kStroke_Style); On 2016/08/26 22:56:46, jdufault wrote: > Move all of the common paint config options away from the comment so it is clear > that these settings are global / shared among every paint operation. > > > SkPaint paint; > paint.setAntiAlias(true); > paint.setStylue(SkPaint::kStroke_Style); > > // Draw the inner border. > ... > Done. https://codereview.chromium.org/2269383002/diff/80001/ash/magnifier/partial_m... ash/magnifier/partial_magnification_controller.cc:137: // Draw border outer outline. On 2016/08/26 22:56:46, jdufault wrote: > What about moving these comments above setStrokeWidth and collapsing them to > > // Draw the border outer outline and then draw the border inner outline. > paint.setStrokeWidth(...) > paint.setColor(...) > ...DrawCircle(...) > ...DrawCircle(...) Done. https://codereview.chromium.org/2269383002/diff/80001/ash/magnifier/partial_m... File ash/magnifier/partial_magnification_controller.h (right): https://codereview.chromium.org/2269383002/diff/80001/ash/magnifier/partial_m... ash/magnifier/partial_magnification_controller.h:51: class BorderRenderer; On 2016/08/26 22:56:46, jdufault wrote: > Remove forward decl. Done.
lgtm https://codereview.chromium.org/2269383002/diff/120001/ash/magnifier/partial_... File ash/magnifier/partial_magnification_controller.cc (right): https://codereview.chromium.org/2269383002/diff/120001/ash/magnifier/partial_... ash/magnifier/partial_magnification_controller.cc:117: BorderRenderer(const gfx::Rect& bounds) : bounds_(bounds) {} explicit
https://codereview.chromium.org/2269383002/diff/120001/ash/magnifier/partial_... File ash/magnifier/partial_magnification_controller.cc (right): https://codereview.chromium.org/2269383002/diff/120001/ash/magnifier/partial_... ash/magnifier/partial_magnification_controller.cc:117: BorderRenderer(const gfx::Rect& bounds) : bounds_(bounds) {} On 2016/09/13 18:27:37, jdufault wrote: > explicit Done.
sammiequon@chromium.org changed reviewers: + jamescook@chromium.org
jamescook@ - Please take a look. Thanks!
I may not get to these CLs before tomorrow. In the meantime, please add a more detailed CL description. IMHO all CL descriptions should include: * What platform does this impact (start description with "ash:" or "chromeos:" or put in summary) * State why you have to make the change * Summarize what you did * Add TEST= line, even if that line is =none It's really helpful when you trying to do an emergency revert and you're reading ~100 CLs to figure out which one is the likely culprit.
Description was changed from ========== Magnifier border is now more visible on light backgrounds. BUG=638996 ========== to ========== chromeos: Magnifier border is now more visible on light backgrounds. Previously the partial magnifier border was difficult to see on white colored backgrounds. I add a textured layer instead of a solid color, so the border is outlined in black, so that it is visible on all backgrounds. BUG=638996 TEST= ==========
Please attach a screenshot to the bug. This really helps me when reviewing CLs that have visual output. Also, use "TEST=none" in the CL description. Code generally looks fine, just nits. https://codereview.chromium.org/2269383002/diff/140001/ash/magnifier/partial_... File ash/magnifier/partial_magnification_controller.cc (right): https://codereview.chromium.org/2269383002/diff/140001/ash/magnifier/partial_... ash/magnifier/partial_magnification_controller.cc:30: // Thickness of the outline around magnifiying glass border in DIP. nit: magnifying https://codereview.chromium.org/2269383002/diff/140001/ash/magnifier/partial_... ash/magnifier/partial_magnification_controller.cc:32: const SkColor kBorderColor = SK_ColorWHITE; nit: document what this looks like (is it black / white / black)? https://codereview.chromium.org/2269383002/diff/140001/ash/magnifier/partial_... ash/magnifier/partial_magnification_controller.cc:126: // The border render draws the border as well as outline on both the outer and nit: render -> renderer https://codereview.chromium.org/2269383002/diff/140001/ash/magnifier/partial_... ash/magnifier/partial_magnification_controller.cc:128: class PartialMagnificationController::BorderRenderer I like how you put this in an inner class. https://codereview.chromium.org/2269383002/diff/140001/ash/magnifier/partial_... ash/magnifier/partial_magnification_controller.cc:136: // ui::LayerDelegate. super nit: We usually end with a colon, like "ui::LayerDelegate:" also above https://codereview.chromium.org/2269383002/diff/140001/ash/magnifier/partial_... ash/magnifier/partial_magnification_controller.cc:148: bounds_.width() / 2 - kBorderSize / 2, paint); optional: I wonder if caching "const int radius = bounds_.width() / 2;" would make this more or less readable. https://codereview.chromium.org/2269383002/diff/140001/ash/magnifier/partial_... ash/magnifier/partial_magnification_controller.cc:169: gfx::Rect bounds_; document: bounds of what? or rename to something like |window_bounds_| or |magnifier_bounds_|
Description was changed from ========== chromeos: Magnifier border is now more visible on light backgrounds. Previously the partial magnifier border was difficult to see on white colored backgrounds. I add a textured layer instead of a solid color, so the border is outlined in black, so that it is visible on all backgrounds. BUG=638996 TEST= ========== to ========== chromeos: Magnifier border is now more visible on light backgrounds. Previously the partial magnifier border was difficult to see on white colored backgrounds. I add a textured layer instead of a solid color, so the border is outlined in black, so that it is visible on all backgrounds. BUG=638996 TEST=none https://screenshot.googleplex.com/htWDLX1Bxdp ==========
https://codereview.chromium.org/2269383002/diff/140001/ash/magnifier/partial_... File ash/magnifier/partial_magnification_controller.cc (right): https://codereview.chromium.org/2269383002/diff/140001/ash/magnifier/partial_... ash/magnifier/partial_magnification_controller.cc:30: // Thickness of the outline around magnifiying glass border in DIP. On 2016/09/14 03:04:58, James Cook wrote: > nit: magnifying Done. https://codereview.chromium.org/2269383002/diff/140001/ash/magnifier/partial_... ash/magnifier/partial_magnification_controller.cc:32: const SkColor kBorderColor = SK_ColorWHITE; On 2016/09/14 03:04:59, James Cook wrote: > nit: document what this looks like (is it black / white / black)? Done. https://codereview.chromium.org/2269383002/diff/140001/ash/magnifier/partial_... ash/magnifier/partial_magnification_controller.cc:126: // The border render draws the border as well as outline on both the outer and On 2016/09/14 03:04:58, James Cook wrote: > nit: render -> renderer Done. https://codereview.chromium.org/2269383002/diff/140001/ash/magnifier/partial_... ash/magnifier/partial_magnification_controller.cc:128: class PartialMagnificationController::BorderRenderer On 2016/09/14 03:04:59, James Cook wrote: > I like how you put this in an inner class. :) https://codereview.chromium.org/2269383002/diff/140001/ash/magnifier/partial_... ash/magnifier/partial_magnification_controller.cc:136: // ui::LayerDelegate. On 2016/09/14 03:04:59, James Cook wrote: > super nit: We usually end with a colon, like "ui::LayerDelegate:" > > also above Done. https://codereview.chromium.org/2269383002/diff/140001/ash/magnifier/partial_... ash/magnifier/partial_magnification_controller.cc:148: bounds_.width() / 2 - kBorderSize / 2, paint); On 2016/09/14 03:04:59, James Cook wrote: > optional: I wonder if caching "const int radius = bounds_.width() / 2;" would > make this more or less readable. Done. https://codereview.chromium.org/2269383002/diff/140001/ash/magnifier/partial_... ash/magnifier/partial_magnification_controller.cc:169: gfx::Rect bounds_; On 2016/09/14 03:04:59, James Cook wrote: > document: bounds of what? or rename to something like |window_bounds_| or > |magnifier_bounds_| Done.
Code LGTM with nits In the screenshot the black parts of the outline seem a little thick. I wonder if they would look better if they were 1 px thinner. You might ask Tom or your UX person about it. https://codereview.chromium.org/2269383002/diff/160001/ash/magnifier/partial_... File ash/magnifier/partial_magnification_controller.cc (right): https://codereview.chromium.org/2269383002/diff/160001/ash/magnifier/partial_... ash/magnifier/partial_magnification_controller.cc:32: // The color of the border an its outlines. The border has an outline on both nit: an -> and https://codereview.chromium.org/2269383002/diff/160001/ash/magnifier/partial_... ash/magnifier/partial_magnification_controller.cc:133: explicit BorderRenderer(const gfx::Rect& bounds) nit: |bounds| -> |magnifier_bounds|
On 2016/09/14 20:54:00, James Cook wrote: > Code LGTM with nits > > In the screenshot the black parts of the outline seem a little thick. I wonder > if they would look better if they were 1 px thinner. You might ask Tom or your > UX person about it. > > https://codereview.chromium.org/2269383002/diff/160001/ash/magnifier/partial_... > File ash/magnifier/partial_magnification_controller.cc (right): > > https://codereview.chromium.org/2269383002/diff/160001/ash/magnifier/partial_... > ash/magnifier/partial_magnification_controller.cc:32: // The color of the border > an its outlines. The border has an outline on both > nit: an -> and > > https://codereview.chromium.org/2269383002/diff/160001/ash/magnifier/partial_... > ash/magnifier/partial_magnification_controller.cc:133: explicit > BorderRenderer(const gfx::Rect& bounds) > nit: |bounds| -> |magnifier_bounds| Yep, we have mocks from UI and the thickness is address there (going to be in the next CL).
https://codereview.chromium.org/2269383002/diff/160001/ash/magnifier/partial_... File ash/magnifier/partial_magnification_controller.cc (right): https://codereview.chromium.org/2269383002/diff/160001/ash/magnifier/partial_... ash/magnifier/partial_magnification_controller.cc:32: // The color of the border an its outlines. The border has an outline on both On 2016/09/14 20:54:00, James Cook wrote: > nit: an -> and Done. https://codereview.chromium.org/2269383002/diff/160001/ash/magnifier/partial_... ash/magnifier/partial_magnification_controller.cc:133: explicit BorderRenderer(const gfx::Rect& bounds) On 2016/09/14 20:54:00, James Cook wrote: > nit: |bounds| -> |magnifier_bounds| Done.
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, jamescook@chromium.org Link to the patchset: https://codereview.chromium.org/2269383002/#ps180001 (title: "Nits.")
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 ========== chromeos: Magnifier border is now more visible on light backgrounds. Previously the partial magnifier border was difficult to see on white colored backgrounds. I add a textured layer instead of a solid color, so the border is outlined in black, so that it is visible on all backgrounds. BUG=638996 TEST=none https://screenshot.googleplex.com/htWDLX1Bxdp ========== to ========== chromeos: Magnifier border is now more visible on light backgrounds. Previously the partial magnifier border was difficult to see on white colored backgrounds. I add a textured layer instead of a solid color, so the border is outlined in black, so that it is visible on all backgrounds. BUG=638996 TEST=none https://screenshot.googleplex.com/htWDLX1Bxdp ==========
Message was sent while issue was closed.
Committed patchset #8 (id:180001)
Message was sent while issue was closed.
Description was changed from ========== chromeos: Magnifier border is now more visible on light backgrounds. Previously the partial magnifier border was difficult to see on white colored backgrounds. I add a textured layer instead of a solid color, so the border is outlined in black, so that it is visible on all backgrounds. BUG=638996 TEST=none https://screenshot.googleplex.com/htWDLX1Bxdp ========== to ========== chromeos: Magnifier border is now more visible on light backgrounds. Previously the partial magnifier border was difficult to see on white colored backgrounds. I add a textured layer instead of a solid color, so the border is outlined in black, so that it is visible on all backgrounds. BUG=638996 TEST=none https://screenshot.googleplex.com/htWDLX1Bxdp Committed: https://crrev.com/da331ee46dfead28a5b2af2379f9fad320509957 Cr-Commit-Position: refs/heads/master@{#418785} ==========
Message was sent while issue was closed.
Patchset 8 (id:??) landed as https://crrev.com/da331ee46dfead28a5b2af2379f9fad320509957 Cr-Commit-Position: refs/heads/master@{#418785}
Message was sent while issue was closed.
A revert of this CL (patchset #8 id:180001) has been created in https://codereview.chromium.org/2344843002/ by yutak@chromium.org. The reason for reverting is: Caused leaks in unit tests. https://build.chromium.org/p/chromium.memory/builders/Linux%20Chromium%20OS%2... ================================================================= ==1214==ERROR: LeakSanitizer: detected memory leaks Direct leak of 24 byte(s) in 1 object(s) allocated from: #0 0x647f4b in operator new(unsigned long) (/b/swarm_slave/w/irbU0iLK/out/Release/ash_unittests+0x647f4b) #1 0x18e142f in ash::PartialMagnificationController::CreateMagnifierWindow(aura::Window*) ash/magnifier/partial_magnification_controller.cc:323:31 #2 0x18e1fd4 in SetActive ash/magnifier/partial_magnification_controller.cc:236:5 #3 0x18e1fd4 in ash::PartialMagnificationController::OnLocatedEvent(ui::LocatedEvent*, ui::PointerDetails const&) ash/magnifier/partial_magnification_controller.cc:259 #4 0x26cab42 in DispatchEvent ui/events/event_dispatcher.cc:191:12 #5 0x26cab42 in ui::EventDispatcher::DispatchEventToEventHandlers(std::vector<ui::EventHandler*, std::allocator<ui::EventHandler*> >*, ui::Event*) ui/events/event_dispatcher.cc:170 <snip>.
Message was sent while issue was closed.
https://codereview.chromium.org/2269383002/diff/180001/ash/magnifier/partial_... File ash/magnifier/partial_magnification_controller.cc (right): https://codereview.chromium.org/2269383002/diff/180001/ash/magnifier/partial_... ash/magnifier/partial_magnification_controller.cc:323: border_layer_->set_delegate(new BorderRenderer(gfx::Rect(GetWindowSize()))); This might be the problem. I don't think set_delegate() takes ownership.
Message was sent while issue was closed.
Description was changed from ========== chromeos: Magnifier border is now more visible on light backgrounds. Previously the partial magnifier border was difficult to see on white colored backgrounds. I add a textured layer instead of a solid color, so the border is outlined in black, so that it is visible on all backgrounds. BUG=638996 TEST=none https://screenshot.googleplex.com/htWDLX1Bxdp Committed: https://crrev.com/da331ee46dfead28a5b2af2379f9fad320509957 Cr-Commit-Position: refs/heads/master@{#418785} ========== to ========== chromeos: Magnifier border is now more visible on light backgrounds. Previously the partial magnifier border was difficult to see on white colored backgrounds. I add a textured layer instead of a solid color, so the border is outlined in black, so that it is visible on all backgrounds. BUG=638996 TEST=none https://screenshot.googleplex.com/htWDLX1Bxdp Committed: https://crrev.com/da331ee46dfead28a5b2af2379f9fad320509957 Cr-Commit-Position: refs/heads/master@{#418785} ==========
jamescook@ - Please take a look. Thanks! https://codereview.chromium.org/2269383002/diff/180001/ash/magnifier/partial_... File ash/magnifier/partial_magnification_controller.cc (right): https://codereview.chromium.org/2269383002/diff/180001/ash/magnifier/partial_... ash/magnifier/partial_magnification_controller.cc:323: border_layer_->set_delegate(new BorderRenderer(gfx::Rect(GetWindowSize()))); On 2016/09/15 16:05:43, James Cook wrote: > This might be the problem. I don't think set_delegate() takes ownership. Thanks for the help! Done.
LGTM https://codereview.chromium.org/2269383002/diff/200001/ash/magnifier/partial_... File ash/magnifier/partial_magnification_controller.cc (right): https://codereview.chromium.org/2269383002/diff/200001/ash/magnifier/partial_... ash/magnifier/partial_magnification_controller.cc:322: border_renderer_.reset(new BorderRenderer(gfx::Rect(GetWindowSize()))); super nit: I would put this line and the set_delegate() line next to each other. https://codereview.chromium.org/2269383002/diff/200001/ash/magnifier/partial_... File ash/magnifier/partial_magnification_controller.h (right): https://codereview.chromium.org/2269383002/diff/200001/ash/magnifier/partial_... ash/magnifier/partial_magnification_controller.h:89: std::unique_ptr<BorderRenderer> border_renderer_; optional: Consider documenting that this must be destroyed before |border_layer_|. (Otherwise the layer will have a pointer to a deleted delegate.) Things are destroyed in the correct order here due to the member order, but it might be nice to point out that that isn't an accident.
https://codereview.chromium.org/2269383002/diff/200001/ash/magnifier/partial_... File ash/magnifier/partial_magnification_controller.cc (right): https://codereview.chromium.org/2269383002/diff/200001/ash/magnifier/partial_... ash/magnifier/partial_magnification_controller.cc:322: border_renderer_.reset(new BorderRenderer(gfx::Rect(GetWindowSize()))); On 2016/09/16 01:48:52, James Cook wrote: > super nit: I would put this line and the set_delegate() line next to each other. Done. https://codereview.chromium.org/2269383002/diff/200001/ash/magnifier/partial_... File ash/magnifier/partial_magnification_controller.h (right): https://codereview.chromium.org/2269383002/diff/200001/ash/magnifier/partial_... ash/magnifier/partial_magnification_controller.h:89: std::unique_ptr<BorderRenderer> border_renderer_; On 2016/09/16 01:48:52, James Cook wrote: > optional: Consider documenting that this must be destroyed before > |border_layer_|. (Otherwise the layer will have a pointer to a deleted > delegate.) Things are destroyed in the correct order here due to the member > order, but it might be nice to point out that that isn't an accident. Done.
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, jamescook@chromium.org Link to the patchset: https://codereview.chromium.org/2269383002/#ps220001 (title: "Nits.")
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 ========== chromeos: Magnifier border is now more visible on light backgrounds. Previously the partial magnifier border was difficult to see on white colored backgrounds. I add a textured layer instead of a solid color, so the border is outlined in black, so that it is visible on all backgrounds. BUG=638996 TEST=none https://screenshot.googleplex.com/htWDLX1Bxdp Committed: https://crrev.com/da331ee46dfead28a5b2af2379f9fad320509957 Cr-Commit-Position: refs/heads/master@{#418785} ========== to ========== chromeos: Magnifier border is now more visible on light backgrounds. Previously the partial magnifier border was difficult to see on white colored backgrounds. I add a textured layer instead of a solid color, so the border is outlined in black, so that it is visible on all backgrounds. BUG=638996 TEST=none https://screenshot.googleplex.com/htWDLX1Bxdp Committed: https://crrev.com/da331ee46dfead28a5b2af2379f9fad320509957 Cr-Commit-Position: refs/heads/master@{#418785} ==========
Message was sent while issue was closed.
Committed patchset #10 (id:220001)
Message was sent while issue was closed.
Description was changed from ========== chromeos: Magnifier border is now more visible on light backgrounds. Previously the partial magnifier border was difficult to see on white colored backgrounds. I add a textured layer instead of a solid color, so the border is outlined in black, so that it is visible on all backgrounds. BUG=638996 TEST=none https://screenshot.googleplex.com/htWDLX1Bxdp Committed: https://crrev.com/da331ee46dfead28a5b2af2379f9fad320509957 Cr-Commit-Position: refs/heads/master@{#418785} ========== to ========== chromeos: Magnifier border is now more visible on light backgrounds. Previously the partial magnifier border was difficult to see on white colored backgrounds. I add a textured layer instead of a solid color, so the border is outlined in black, so that it is visible on all backgrounds. BUG=638996 TEST=none https://screenshot.googleplex.com/htWDLX1Bxdp Committed: https://crrev.com/da331ee46dfead28a5b2af2379f9fad320509957 Committed: https://crrev.com/904192168450735ec94121000bf86d645a47b857 Cr-Original-Commit-Position: refs/heads/master@{#418785} Cr-Commit-Position: refs/heads/master@{#419199} ==========
Message was sent while issue was closed.
Patchset 10 (id:??) landed as https://crrev.com/904192168450735ec94121000bf86d645a47b857 Cr-Commit-Position: refs/heads/master@{#419199} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
