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

Issue 7708025: Adds extension APIs of events on changing volume. (Closed)

Created:
9 years, 4 months ago by yoshiki
Modified:
9 years, 3 months ago
CC:
chromium-reviews, derat+watch_chromium.org, hashimoto+watch_chromium.org, aboxhall+watch_chromium.org, Erik does not do reviews, yoshiki+watch_chromium.org, mihaip+watch_chromium.org, yuzo+watch_chromium.org, davidbarr+watch_chromium.org, Aaron Boodman, dmazzoni+watch_chromium.org, dtseng+watch_chromium.org, ctguil+watch_chromium.org, davemoore+watch_chromium.org, zork+watch_chromium.org, Zachary Kuznia, dmazzoni, Daniel Erat
Visibility:
Public.

Description

Adds an extension API of the event on changing volume. - experimental.accessibility.onVolumeChange I run chrome/common/extensions/docs/build/build.py, but no documents about a11y was changed. So this CL doesn't include any changes of documents. BUG=chromium-os:16592 TEST=call the APIs manually on chromium-os Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=98948

Patch Set 1 #

Total comments: 34

Patch Set 2 : Combine the notifications into a single notification: onVolumeChanged. #

Total comments: 2

Patch Set 3 : Add AccessibilityVolumeInfo to the types section. #

Total comments: 10

Patch Set 4 : review fix (to be commited) #

Total comments: 4

Patch Set 5 : review fix (to be commited) #

Unified diffs Side-by-side diffs Delta from patch set Stats (+131 lines, -14 lines) Patch
M chrome/browser/accessibility_events.h View 1 2 3 4 3 chunks +40 lines, -10 lines 0 comments Download
M chrome/browser/accessibility_events.cc View 1 2 3 2 chunks +28 lines, -3 lines 0 comments Download
M chrome/browser/chromeos/system_key_event_listener.cc View 1 4 chunks +16 lines, -0 lines 0 comments Download
M chrome/browser/extensions/extension_accessibility_api.h View 1 1 chunk +1 line, -0 lines 0 comments Download
M chrome/browser/extensions/extension_accessibility_api.cc View 1 4 chunks +13 lines, -1 line 0 comments Download
M chrome/browser/extensions/extension_accessibility_api_constants.h View 1 3 chunks +4 lines, -0 lines 0 comments Download
M chrome/browser/extensions/extension_accessibility_api_constants.cc View 1 2 3 2 chunks +3 lines, -0 lines 0 comments Download
M chrome/common/chrome_notification_types.h View 1 2 3 4 1 chunk +5 lines, -0 lines 0 comments Download
M chrome/common/extensions/api/extension_api.json View 1 2 3 4 2 chunks +21 lines, -0 lines 0 comments Download

Messages

Total messages: 14 (0 generated)
yoshiki
Please take a look!
9 years, 4 months ago (2011-08-24 08:21:15 UTC) #1
dmazzoni
Please add Erik Kay or someone else on the extensions team after addressing comments from ...
9 years, 4 months ago (2011-08-24 14:49:01 UTC) #2
Daniel Erat
I had more comments but Dominic beat me to it. :-P http://codereview.chromium.org/7708025/diff/1/chrome/browser/chromeos/system_key_event_listener.cc File chrome/browser/chromeos/system_key_event_listener.cc (right): ...
9 years, 4 months ago (2011-08-24 14:52:57 UTC) #3
Zachary Kuznia
http://codereview.chromium.org/7708025/diff/1/chrome/browser/extensions/extension_accessibility_api_constants.cc File chrome/browser/extensions/extension_accessibility_api_constants.cc (right): http://codereview.chromium.org/7708025/diff/1/chrome/browser/extensions/extension_accessibility_api_constants.cc#newcode48 chrome/browser/extensions/extension_accessibility_api_constants.cc:48: const char kTypeVolume[] = "volume"; nit: volume should come ...
9 years, 4 months ago (2011-08-25 06:14:07 UTC) #4
yoshiki
Thank you for reviews. I replaced 3 notifications (onVolumeUp|-Down|-Muted) with a single one (onVolumeChanged) taking ...
9 years, 3 months ago (2011-08-29 09:16:16 UTC) #5
Zachary Kuznia
http://codereview.chromium.org/7708025/diff/14001/chrome/common/extensions/api/extension_api.json File chrome/common/extensions/api/extension_api.json (right): http://codereview.chromium.org/7708025/diff/14001/chrome/common/extensions/api/extension_api.json#newcode513 chrome/common/extensions/api/extension_api.json:513: "$ref": "AccessibilityVolumeInfo", I think you need to add AccessibilityVolumeInfo ...
9 years, 3 months ago (2011-08-29 09:36:28 UTC) #6
yoshiki
http://codereview.chromium.org/7708025/diff/14001/chrome/common/extensions/api/extension_api.json File chrome/common/extensions/api/extension_api.json (right): http://codereview.chromium.org/7708025/diff/14001/chrome/common/extensions/api/extension_api.json#newcode513 chrome/common/extensions/api/extension_api.json:513: "$ref": "AccessibilityVolumeInfo", On 2011/08/29 09:36:29, Zachary Kuznia wrote: > ...
9 years, 3 months ago (2011-08-29 11:50:43 UTC) #7
Daniel Erat
Thanks! Looks fine to me with a few nits, but I'll let someone who has ...
9 years, 3 months ago (2011-08-29 15:05:35 UTC) #8
dmazzoni
LGTM with just a couple more issues. http://codereview.chromium.org/7708025/diff/14003/chrome/browser/extensions/extension_accessibility_api_constants.cc File chrome/browser/extensions/extension_accessibility_api_constants.cc (right): http://codereview.chromium.org/7708025/diff/14003/chrome/browser/extensions/extension_accessibility_api_constants.cc#newcode21 chrome/browser/extensions/extension_accessibility_api_constants.cc:21: const char ...
9 years, 3 months ago (2011-08-29 22:24:06 UTC) #9
Zachary Kuznia
LGTM http://codereview.chromium.org/7708025/diff/14003/chrome/common/extensions/api/extension_api.json File chrome/common/extensions/api/extension_api.json (right): http://codereview.chromium.org/7708025/diff/14003/chrome/common/extensions/api/extension_api.json#newcode391 chrome/common/extensions/api/extension_api.json:391: "volume": {"type": "double", "description": "The value of the ...
9 years, 3 months ago (2011-08-30 01:58:05 UTC) #10
yoshiki
Thanks a lot! http://codereview.chromium.org/7708025/diff/14003/chrome/browser/accessibility_events.cc File chrome/browser/accessibility_events.cc (right): http://codereview.chromium.org/7708025/diff/14003/chrome/browser/accessibility_events.cc#newcode30 chrome/browser/accessibility_events.cc:30: Profile *profile = ProfileManager::GetDefaultProfile(); On 2011/08/29 ...
9 years, 3 months ago (2011-08-30 06:43:30 UTC) #11
yoshiki
Hi Even, I'd like to change extension_api.json, but owner's lgtm is needed. Do you take ...
9 years, 3 months ago (2011-08-30 10:40:37 UTC) #12
Ben Olmstead
LGTM for extensions system changes. A few nits. http://codereview.chromium.org/7708025/diff/22001/chrome/browser/accessibility_events.h File chrome/browser/accessibility_events.h (right): http://codereview.chromium.org/7708025/diff/22001/chrome/browser/accessibility_events.h#newcode19 chrome/browser/accessibility_events.h:19: // ...
9 years, 3 months ago (2011-08-30 18:12:18 UTC) #13
Daniel Erat
9 years, 3 months ago (2011-08-30 19:00:40 UTC) #14
On Tue, Aug 30, 2011 at 11:12,  <bolms@chromium.org> wrote:
> LGTM for extensions system changes.  A few nits.
>
>
>
http://codereview.chromium.org/7708025/diff/22001/chrome/browser/accessibilit...
> File chrome/browser/accessibility_events.h (right):
>
>
http://codereview.chromium.org/7708025/diff/22001/chrome/browser/accessibilit...
> chrome/browser/accessibility_events.h:19: // notification type with
> AccessibilityControlInfo details to any
> AccessibilityControlInfo -> AccessibilityEventInfo
>
>
http://codereview.chromium.org/7708025/diff/22001/chrome/browser/accessibilit...
> chrome/browser/accessibility_events.h:53: void
> SerializeToDict(base::DictionaryValue* dict) const;
> OVERRIDE
>
>
http://codereview.chromium.org/7708025/diff/22001/chrome/common/extensions/ap...
> File chrome/common/extensions/api/extension_api.json (right):
>
>
http://codereview.chromium.org/7708025/diff/22001/chrome/common/extensions/ap...
> chrome/common/extensions/api/extension_api.json:391: "volume": {"type":
> "double", "description": "The value of the volume percent. This must be
> between 0.0 and 100.0."},
> Is the value log scale or linear scale?

Not defined.  (The current mapping between percent and decibels isn't
very good and out to be changed at some point.)

>
http://codereview.chromium.org/7708025/diff/22001/chrome/common/extensions/ap...
> chrome/common/extensions/api/extension_api.json:524: "description":
> "Information about the current volume level and the volume being muted."
> "The current state of the system volume control, including whether it is
> muted." ?
>
> http://codereview.chromium.org/7708025/
>

Powered by Google App Engine
This is Rietveld 408576698