Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(30)

Issue 2694933002: Fix appearance of task manager border at fractional scale factors. (Closed)

Created:
3 years, 10 months ago by Evan Stade
Modified:
3 years, 10 months ago
CC:
chromium-reviews, tfarina
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

Fix appearance of task manager border at fractional scale factors. BUG=688507 Review-Url: https://codereview.chromium.org/2694933002 Cr-Commit-Position: refs/heads/master@{#452215} Committed: https://chromium.googlesource.com/chromium/src/+/194bb15b9740165e8ca0ec4a4ee898887876946a

Patch Set 1 #

Patch Set 2 : more better #

Patch Set 3 : inline #

Total comments: 3

Patch Set 4 : handle snaptopixelbounds case #

Total comments: 2

Patch Set 5 : update tests #

Patch Set 6 : more self documenting? #

Total comments: 4

Patch Set 7 : tests #

Total comments: 6
Unified diffs Side-by-side diffs Delta from patch set Stats (+60 lines, -40 lines) Patch
M ui/views/border.cc View 1 2 3 4 5 2 chunks +22 lines, -12 lines 0 comments Download
M ui/views/border_unittest.cc View 1 2 3 4 5 6 4 chunks +38 lines, -28 lines 6 comments Download

Messages

Total messages: 56 (24 generated)
Evan Stade
slightly worried this will break something somewhere, but I poked around and couldn't find that ...
3 years, 10 months ago (2017-02-17 02:07:48 UTC) #2
Peter Kasting
LGTM https://codereview.chromium.org/2694933002/diff/40001/ui/views/border.cc File ui/views/border.cc (right): https://codereview.chromium.org/2694933002/diff/40001/ui/views/border.cc#newcode52 ui/views/border.cc:52: gfx::RectF bounds(view.GetLocalBounds()); Nit: Prefer = to () for ...
3 years, 10 months ago (2017-02-17 02:40:15 UTC) #6
Evan Stade
+msw for owners. I'll fix the (weird) border tests tomorrow. https://codereview.chromium.org/2694933002/diff/40001/ui/views/border.cc File ui/views/border.cc (right): https://codereview.chromium.org/2694933002/diff/40001/ui/views/border.cc#newcode52 ...
3 years, 10 months ago (2017-02-17 06:19:55 UTC) #10
Peter Kasting
https://codereview.chromium.org/2694933002/diff/40001/ui/views/border.cc File ui/views/border.cc (right): https://codereview.chromium.org/2694933002/diff/40001/ui/views/border.cc#newcode52 ui/views/border.cc:52: gfx::RectF bounds(view.GetLocalBounds()); On 2017/02/17 06:19:55, Evan Stade wrote: > ...
3 years, 10 months ago (2017-02-17 07:13:20 UTC) #11
Evan Stade
Mike: regarding the now failing border unit tests, do you have an idea how I ...
3 years, 10 months ago (2017-02-17 20:42:23 UTC) #12
msw
On 2017/02/17 20:42:23, Evan Stade wrote: > Mike: regarding the now failing border unit tests, ...
3 years, 10 months ago (2017-02-17 21:00:30 UTC) #14
ericrk
Glad to see this is being added in! I was trying this change out to ...
3 years, 10 months ago (2017-02-17 22:00:45 UTC) #16
Peter Kasting
On 2017/02/17 22:00:45, ericrk wrote: > Glad to see this is being added in! > ...
3 years, 10 months ago (2017-02-17 22:27:54 UTC) #17
Evan Stade
On 2017/02/17 22:00:45, ericrk wrote: > Glad to see this is being added in! > ...
3 years, 10 months ago (2017-02-17 23:52:57 UTC) #18
Peter Kasting
https://codereview.chromium.org/2694933002/diff/60001/ui/views/border.cc File ui/views/border.cc (right): https://codereview.chromium.org/2694933002/diff/60001/ui/views/border.cc#newcode60 ui/views/border.cc:60: bounds = gfx::RectF(gfx::ToEnclosingRect(bounds)); Why ToEnclosingRect() and not ToEnclosedRect()? The ...
3 years, 10 months ago (2017-02-18 00:08:28 UTC) #19
Evan Stade
On 2017/02/18 00:08:28, Peter Kasting wrote: > https://codereview.chromium.org/2694933002/diff/60001/ui/views/border.cc > File ui/views/border.cc (right): > > https://codereview.chromium.org/2694933002/diff/60001/ui/views/border.cc#newcode60 ...
3 years, 10 months ago (2017-02-18 00:19:20 UTC) #20
Peter Kasting
On 2017/02/18 00:19:20, Evan Stade wrote: > On 2017/02/18 00:08:28, Peter Kasting wrote: > > ...
3 years, 10 months ago (2017-02-18 00:25:48 UTC) #21
ericrk
https://codereview.chromium.org/2694933002/diff/60001/ui/views/border.cc File ui/views/border.cc (right): https://codereview.chromium.org/2694933002/diff/60001/ui/views/border.cc#newcode60 ui/views/border.cc:60: bounds = gfx::RectF(gfx::ToEnclosingRect(bounds)); On 2017/02/18 00:08:28, Peter Kasting wrote: ...
3 years, 10 months ago (2017-02-18 00:37:26 UTC) #22
ericrk
On 2017/02/18 00:25:48, Peter Kasting wrote: > On 2017/02/18 00:19:20, Evan Stade wrote: > > ...
3 years, 10 months ago (2017-02-18 00:43:56 UTC) #23
Evan Stade
On 2017/02/18 00:43:56, ericrk wrote: > On 2017/02/18 00:25:48, Peter Kasting wrote: > > On ...
3 years, 10 months ago (2017-02-18 01:32:27 UTC) #24
Evan Stade
On 2017/02/18 00:43:56, ericrk wrote: > On 2017/02/18 00:25:48, Peter Kasting wrote: > > On ...
3 years, 10 months ago (2017-02-18 01:32:30 UTC) #25
Peter Kasting
On 2017/02/18 01:32:30, Evan Stade wrote: > On 2017/02/18 00:43:56, ericrk wrote: > > On ...
3 years, 10 months ago (2017-02-18 01:40:03 UTC) #26
Matt Giuca
On 2017/02/17 21:00:30, msw wrote: > On 2017/02/17 20:42:23, Evan Stade wrote: > > Mike: ...
3 years, 10 months ago (2017-02-19 23:38:54 UTC) #27
Evan Stade
On 2017/02/19 23:38:54, Matt Giuca wrote: > Hi, > > It looks like you've run ...
3 years, 10 months ago (2017-02-21 17:53:36 UTC) #28
Evan Stade
On 2017/02/18 01:40:03, Peter Kasting wrote: > > I believe this patch to be correct ...
3 years, 10 months ago (2017-02-21 19:56:58 UTC) #31
Evan Stade
On 2017/02/21 19:56:58, Evan Stade wrote: > On 2017/02/18 01:40:03, Peter Kasting wrote: > > ...
3 years, 10 months ago (2017-02-21 20:06:41 UTC) #34
ericrk
On 2017/02/21 20:06:41, Evan Stade wrote: > On 2017/02/21 19:56:58, Evan Stade wrote: > > ...
3 years, 10 months ago (2017-02-21 22:04:51 UTC) #37
ericrk
On 2017/02/21 22:04:51, ericrk wrote: > On 2017/02/21 20:06:41, Evan Stade wrote: > > On ...
3 years, 10 months ago (2017-02-21 23:05:08 UTC) #38
Matt Giuca
https://codereview.chromium.org/2694933002/diff/100001/ui/views/border_unittest.cc File ui/views/border_unittest.cc (right): https://codereview.chromium.org/2694933002/diff/100001/ui/views/border_unittest.cc#newcode88 ui/views/border_unittest.cc:88: void onDrawPaint(const SkPaint& paint) override { ++draw_paint_call_count_; } Can ...
3 years, 10 months ago (2017-02-21 23:18:51 UTC) #39
Evan Stade
https://codereview.chromium.org/2694933002/diff/100001/ui/views/border_unittest.cc File ui/views/border_unittest.cc (right): https://codereview.chromium.org/2694933002/diff/100001/ui/views/border_unittest.cc#newcode88 ui/views/border_unittest.cc:88: void onDrawPaint(const SkPaint& paint) override { ++draw_paint_call_count_; } On ...
3 years, 10 months ago (2017-02-22 01:34:55 UTC) #40
Matt Giuca
lgtm (with a few queries). Thanks for dealing with the tests. https://codereview.chromium.org/2694933002/diff/120001/ui/views/border_unittest.cc File ui/views/border_unittest.cc (right): ...
3 years, 10 months ago (2017-02-22 03:29:09 UTC) #43
Evan Stade
+sky as msw may be ooo https://codereview.chromium.org/2694933002/diff/120001/ui/views/border_unittest.cc File ui/views/border_unittest.cc (right): https://codereview.chromium.org/2694933002/diff/120001/ui/views/border_unittest.cc#newcode105 ui/views/border_unittest.cc:105: // Stores the ...
3 years, 10 months ago (2017-02-22 16:59:23 UTC) #47
sky
LGTM
3 years, 10 months ago (2017-02-22 17:40:30 UTC) #48
Peter Kasting
LGTM https://codereview.chromium.org/2694933002/diff/120001/ui/views/border_unittest.cc File ui/views/border_unittest.cc (right): https://codereview.chromium.org/2694933002/diff/120001/ui/views/border_unittest.cc#newcode178 ui/views/border_unittest.cc:178: const SkColor kBorderColor = SK_ColorMAGENTA; On 2017/02/22 16:59:23, ...
3 years, 10 months ago (2017-02-22 19:07:12 UTC) #49
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2694933002/120001
3 years, 10 months ago (2017-02-22 20:06:08 UTC) #52
commit-bot: I haz the power
Committed patchset #7 (id:120001) as https://chromium.googlesource.com/chromium/src/+/194bb15b9740165e8ca0ec4a4ee898887876946a
3 years, 10 months ago (2017-02-22 21:39:29 UTC) #55
Matt Giuca
3 years, 10 months ago (2017-02-22 22:36:13 UTC) #56
Message was sent while issue was closed.
https://codereview.chromium.org/2694933002/diff/120001/ui/views/border_unitte...
File ui/views/border_unittest.cc (right):

https://codereview.chromium.org/2694933002/diff/120001/ui/views/border_unitte...
ui/views/border_unittest.cc:178: const SkColor kBorderColor = SK_ColorMAGENTA;
On 2017/02/22 19:07:12, Peter Kasting (backlogged) wrote:
> On 2017/02/22 16:59:23, Evan Stade wrote:
> > On 2017/02/22 03:29:09, Matt Giuca wrote:
> > > Any reason to change this to magenta?
> > > 
> > > (Perhaps just to have different tests drawing different colours to test
that
> > it
> > > isn't just hard-coded to BLUE?)
> > 
> > no real reason. I guess I thought it was less likely to be used in any real
> life
> > scenario.
> 
> Choose border color using random number generator?  :)

And choose the expected border color with a 98% chance to be the same as the set
one. A little puzzle for the sheriffs :)

Powered by Google App Engine
This is Rietveld 408576698