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

Issue 10382065: Fix TTS code to be compatible with new extension API parameter serialization. (Closed)

Created:
8 years, 7 months ago by asargent_no_longer_on_chrome
Modified:
8 years, 7 months ago
Reviewers:
dmazzoni
CC:
chromium-reviews, Aaron Boodman, Satish, mihaip-chromium-reviews_chromium.org
Visibility:
Public.

Description

Fix TTS code to be compatible with new extension API parameter serialization. Some TTS tests broke when I landed codereview.chromium.org/10161038 because it turned out the tts.speak method takes the incoming options dictionary argument and re-purposes it to send an event it dispatches back to extension code. With the above reverenced CL changes to avoid JSON.stringify, instead using V8ValueConverter, it turns out one subtle difference between the two is that JSON.stringify will ignore key/value pairs where the value is not a stringifiable type (in this case, a function), whereas V8ValueConverter leaves the key in and inserts a null value. This was causing the schema parameter validation to fail for the event dispatch. BUG=122675 TEST=Existing tests should pass, and re-landing the CL referenced above should work with the TTS API browser tests. Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=135933

Patch Set 1 #

Unified diffs Side-by-side diffs Delta from patch set Stats (+4 lines, -0 lines) Patch
M chrome/browser/speech/extension_api/tts_engine_extension_api.cc View 1 chunk +2 lines, -0 lines 0 comments Download
M chrome/browser/speech/extension_api/tts_extension_api_constants.h View 1 chunk +1 line, -0 lines 0 comments Download
M chrome/browser/speech/extension_api/tts_extension_api_constants.cc View 1 chunk +1 line, -0 lines 0 comments Download

Messages

Total messages: 6 (0 generated)
asargent_no_longer_on_chrome
8 years, 7 months ago (2012-05-08 18:38:03 UTC) #1
dmazzoni
lgtm
8 years, 7 months ago (2012-05-08 19:13:15 UTC) #2
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/asargent@chromium.org/10382065/1
8 years, 7 months ago (2012-05-08 19:47:28 UTC) #3
commit-bot: I haz the power
Try job failure for 10382065-1 on win_rel for step "update". http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&number=27070 Step "update" is always ...
8 years, 7 months ago (2012-05-08 20:08:31 UTC) #4
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/asargent@chromium.org/10382065/1
8 years, 7 months ago (2012-05-08 20:28:25 UTC) #5
commit-bot: I haz the power
8 years, 7 months ago (2012-05-08 20:48:35 UTC) #6
Try job failure for 10382065-1 on win_rel for step "update".
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

Step "update" is always a major failure.
Look at the try server FAQ for more details.

Powered by Google App Engine
This is Rietveld 408576698