Chromium Code Reviews
Help | Chromium Project | Gerrit Changes | Sign in
(2)

Issue 12310022: More flexibility in BubbleBorder arrow rendering. (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
4 years, 4 months ago by dewittj
Modified:
4 years, 4 months ago
CC:
chromium-reviews, tfarina, alicet1, msw+watch_chromium.org, ben+watch_chromium.org, stevenjb
Base URL:
http://git.chromium.org/chromium/src.git@master
Visibility:
Public.

Description

More flexibility in BubbleBorder arrow rendering. Currently causing the arrow not to be painted leaves a margin of several pixels on the side the arrow would be painted, causing issues when laying out bubbles next to an anchor rectangle such as the edge of the screen. This patch changes the API so that you can choose whether the arrow is painted normally, there is space left for the arrow but it is not painted, or there is no space left for the arrow and it is not painted. r=msw@chromium.org,sadrul@chromium.org,ben@chromium.org BUG=NONE Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=184487

Patch Set 1 #

Patch Set 2 : Now with more user choice: bool -> enum. #

Total comments: 6

Patch Set 3 : Address msw style nits. #

Messages

Total messages: 13 (0 generated)
dewittj
Please take a look.
4 years, 4 months ago (2013-02-21 01:46:37 UTC) #1
msw - vacation june 8-27
LGTM with nits. I'll look again if you add tests like you mentioned offline. These ...
4 years, 4 months ago (2013-02-21 07:29:29 UTC) #2
dewittj
https://codereview.chromium.org/12310022/diff/2001/ash/system/tray/system_tray.cc File ash/system/tray/system_tray.cc (right): https://codereview.chromium.org/12310022/diff/2001/ash/system/tray/system_tray.cc#newcode379 ash/system/tray/system_tray.cc:379: if (items.size() == 1 && items[0]->ShouldHideArrow()) { On 2013/02/21 ...
4 years, 4 months ago (2013-02-21 19:13:57 UTC) #3
sadrul
LGTM It's weird that we need a mode that has "space left for the arrow ...
4 years, 4 months ago (2013-02-21 21:16:36 UTC) #4
dewittj
On 2013/02/21 21:16:36, sadrul wrote: > LGTM > > It's weird that we need a ...
4 years, 4 months ago (2013-02-22 22:50:50 UTC) #5
dewittj
sadrul: would you like me to hold off on committing this change to investigate modifying ...
4 years, 4 months ago (2013-02-23 01:44:11 UTC) #6
sadrul
On 2013/02/23 01:44:11, dewittj wrote: > sadrul: would you like me to hold off on ...
4 years, 4 months ago (2013-02-23 04:36:49 UTC) #7
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/dewittj@chromium.org/12310022/3009
4 years, 4 months ago (2013-02-23 15:49:40 UTC) #8
commit-bot: I haz the power
Presubmit check for 12310022-3009 failed and returned exit status 1. INFO:root:Found 7 file(s). Running presubmit ...
4 years, 4 months ago (2013-02-23 15:49:49 UTC) #9
dewittj
ben: Please could you review c/b/ui/views/message_center/notification_bubble_wrapper_win.cc
4 years, 4 months ago (2013-02-24 17:13:34 UTC) #10
Ben Goodger (Google)
lgtm
4 years, 4 months ago (2013-02-24 23:56:51 UTC) #11
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/dewittj@chromium.org/12310022/3009
4 years, 4 months ago (2013-02-25 18:31:26 UTC) #12
commit-bot: I haz the power
4 years, 4 months ago (2013-02-25 21:48:21 UTC) #13
Message was sent while issue was closed.
Change committed as 184487
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld cb946e318