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

Issue 23513039: Replace animated tab audio indicator with static tab audio indicator. (Closed)

Created:
7 years, 3 months ago by miu
Modified:
7 years, 3 months ago
Reviewers:
sail, sky
CC:
chromium-reviews, tfarina, oshima+watch_chromium.org
Visibility:
Public.

Description

Replace animated tab audio indicator with static tab audio indicator. This implements the "short term" plan as described in http://crbug.com/178934#c46. The audio indicator is shown on the right-side of the tab (to the left of the close button) while a tab is playing non-silent audio. When the tab size becomes too small, the preference is to hide the favicon before the audio indicator. Exception: If there is a "recording/capture throbber" animating on the favicon, then the favicon takes precedence as user privacy awareness is the primary concern. NOTE: For now, this feature remains disabled by default, behind the --enable-audible-notifications flag. Indicator Icon: To closely match the mock, I re-used the existing ChromeOS audio file type graphics, making it grayscale and semi-transparent. This approach works very well with a number of light- and dark-colored themes. Added comprehensive unit testing of the layout/visibility of the components in the tab to confirm correctness for all tab widths and combinations of: Selected vs. non-selected, mini/pinned mode vs. normal mode, audio playing vs. silence, and recording/capturing vs. not. UX Testing: Manual testing for look-and-feel on: Aura and Mac, different DPI factors, and various themes. BUG=178934 Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=224214

Patch Set 1 #

Total comments: 11

Patch Set 2 : Addressed review comments from sail@. #

Total comments: 9

Patch Set 3 : Addressed review comments from sky@. Also, rebased. #

Unified diffs Side-by-side diffs Delta from patch set Stats (+572 lines, -601 lines) Patch
D chrome/app/theme/default_100_percent/common/audio_equalizer_column.png View Binary file 0 comments Download
A chrome/app/theme/default_100_percent/common/tab_audio_indicator.png View Binary file 0 comments Download
D chrome/app/theme/default_200_percent/common/audio_equalizer_column.png View Binary file 0 comments Download
A chrome/app/theme/default_200_percent/common/tab_audio_indicator.png View Binary file 0 comments Download
M chrome/app/theme/theme_resources.grd View 1 2 2 chunks +1 line, -1 line 0 comments Download
D chrome/browser/ui/cocoa/tabs/tab_audio_indicator_view_mac.h View 1 2 1 chunk +0 lines, -38 lines 0 comments Download
D chrome/browser/ui/cocoa/tabs/tab_audio_indicator_view_mac.mm View 1 2 1 chunk +0 lines, -66 lines 0 comments Download
D chrome/browser/ui/cocoa/tabs/tab_audio_indicator_view_mac_unittest.mm View 1 2 1 chunk +0 lines, -37 lines 0 comments Download
M chrome/browser/ui/cocoa/tabs/tab_controller.h View 3 chunks +3 lines, -0 lines 0 comments Download
M chrome/browser/ui/cocoa/tabs/tab_controller.mm View 1 4 chunks +76 lines, -8 lines 0 comments Download
M chrome/browser/ui/cocoa/tabs/tab_controller_unittest.mm View 1 3 chunks +198 lines, -0 lines 0 comments Download
M chrome/browser/ui/cocoa/tabs/tab_strip_controller.mm View 1 2 4 chunks +14 lines, -18 lines 0 comments Download
D chrome/browser/ui/tabs/tab_audio_indicator.h View 1 2 1 chunk +0 lines, -83 lines 0 comments Download
D chrome/browser/ui/tabs/tab_audio_indicator.cc View 1 2 1 chunk +0 lines, -165 lines 0 comments Download
D chrome/browser/ui/tabs/tab_audio_indicator_unittest.cc View 1 2 1 chunk +0 lines, -74 lines 0 comments Download
M chrome/browser/ui/views/tabs/tab.h View 1 2 9 chunks +15 lines, -9 lines 0 comments Download
M chrome/browser/ui/views/tabs/tab.cc View 1 2 14 chunks +92 lines, -48 lines 0 comments Download
M chrome/browser/ui/views/tabs/tab_unittest.cc View 7 chunks +173 lines, -48 lines 0 comments Download
M chrome/chrome_browser_ui.gypi View 1 2 2 chunks +0 lines, -4 lines 0 comments Download
M chrome/chrome_tests_unit.gypi View 1 2 2 chunks +0 lines, -2 lines 0 comments Download

Messages

Total messages: 12 (0 generated)
miu
Sailesh, Please review. I felt sad pulling out all the existing animation code (for the ...
7 years, 3 months ago (2013-09-11 04:37:06 UTC) #1
sail
Hi Yuri, thanks for taking this on. This change looks good. You'll want to get ...
7 years, 3 months ago (2013-09-11 19:18:37 UTC) #2
miu
Thanks for the quick turnaround, Sailesh. Took action on all your comments. PTAL. https://codereview.chromium.org/23513039/diff/1/chrome/browser/ui/cocoa/tabs/tab_controller.mm File ...
7 years, 3 months ago (2013-09-11 21:35:06 UTC) #3
miu
pkasting: Would you please review the non-Mac UI changes: chrome/browser/ui/views/* Thanks, Yuri
7 years, 3 months ago (2013-09-11 21:36:30 UTC) #4
sail
cocoa/* lgtm https://codereview.chromium.org/23513039/diff/12001/chrome/browser/ui/cocoa/tabs/tab_strip_controller.mm File chrome/browser/ui/cocoa/tabs/tab_strip_controller.mm (right): https://codereview.chromium.org/23513039/diff/12001/chrome/browser/ui/cocoa/tabs/tab_strip_controller.mm#newcode1665 chrome/browser/ui/cocoa/tabs/tab_strip_controller.mm:1665: NSRect frame; initialize to NSZeroRect
7 years, 3 months ago (2013-09-11 22:21:54 UTC) #5
Peter Kasting
I've never reviewed code in ui/views/tabs; try asking sky for review.
7 years, 3 months ago (2013-09-16 21:50:23 UTC) #6
miu
sky: Could you take a look at chrome/browser/ui/views/tabs/* Thanks, Yuri
7 years, 3 months ago (2013-09-16 22:00:04 UTC) #7
sky
https://codereview.chromium.org/23513039/diff/12001/chrome/browser/ui/views/tabs/tab.cc File chrome/browser/ui/views/tabs/tab.cc (right): https://codereview.chromium.org/23513039/diff/12001/chrome/browser/ui/views/tabs/tab.cc#newcode814 chrome/browser/ui/views/tabs/tab.cc:814: const int right = showing_close_button_ ? Does this work ...
7 years, 3 months ago (2013-09-16 23:47:19 UTC) #8
miu
https://codereview.chromium.org/23513039/diff/12001/chrome/browser/ui/views/tabs/tab.cc File chrome/browser/ui/views/tabs/tab.cc (right): https://codereview.chromium.org/23513039/diff/12001/chrome/browser/ui/views/tabs/tab.cc#newcode814 chrome/browser/ui/views/tabs/tab.cc:814: const int right = showing_close_button_ ? On 2013/09/16 23:47:19, ...
7 years, 3 months ago (2013-09-19 02:37:11 UTC) #9
sky
LGTM
7 years, 3 months ago (2013-09-19 15:48:17 UTC) #10
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/miu@chromium.org/23513039/28001
7 years, 3 months ago (2013-09-19 18:08:13 UTC) #11
commit-bot: I haz the power
7 years, 3 months ago (2013-09-19 21:57:24 UTC) #12
Message was sent while issue was closed.
Change committed as 224214

Powered by Google App Engine
This is Rietveld 408576698