Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
9688004:
Sample extensions page does not feature chrome.ttsEngine. (Closed)
|
Created:
8 years, 9 months ago by mitchellwrosen Modified:
8 years, 7 months ago Base URL:
http://git.chromium.org/chromium/src.git@master Visibility:
Public. |
DescriptionSample extensions page does not feature chrome.ttsEngine.
Wrote a small sample TTS Engine that prints text to a small window on the bottom
of the screen rather than synthesizing speech.
BUG=117480
TEST=Manual
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=138047
Patch Set 1 #
Total comments: 15
Patch Set 2 : Minor changes, silent -> console #
Total comments: 1
Patch Set 3 : Small change #Patch Set 4 : License header, move into ttsEngine/ #Patch Set 5 : Trying again... #
Messages
Total messages: 22 (0 generated)
lgtm
I'm sorry, somehow this completely escaped my notice. I didn't try to run this, and someone else should review the code, but in theory it looks good. Except for that one line in the manifest.
https://chromiumcodereview.appspot.com/9688004/diff/1/chrome/common/extension... File chrome/common/extensions/docs/examples/api/silent_tts_engine/manifest.json (right): https://chromiumcodereview.appspot.com/9688004/diff/1/chrome/common/extension... chrome/common/extensions/docs/examples/api/silent_tts_engine/manifest.json:6: "permisions": ["ASS"], Is this just here to make sure we looked? :)
On 2012/05/14 20:40:04, kathyw wrote: > https://chromiumcodereview.appspot.com/9688004/diff/1/chrome/common/extension... > File chrome/common/extensions/docs/examples/api/silent_tts_engine/manifest.json > (right): > > https://chromiumcodereview.appspot.com/9688004/diff/1/chrome/common/extension... > chrome/common/extensions/docs/examples/api/silent_tts_engine/manifest.json:6: > "permisions": ["ASS"], > > Is this just here to make sure we looked? :) Yeah... that was in there just to make sure you looked... *hides face in shame*. Who should I add as a reviewer to get an LGTM?
Hi! Nice work on this extension! Just some minor issues to make it a bit more readable and match the Chrome style. Have you considered putting this in the Chrome Web Store? I think it'd be useful for developers creating talking apps. https://chromiumcodereview.appspot.com/9688004/diff/1/chrome/common/extension... File chrome/common/extensions/docs/examples/api/silent_tts_engine/background.html (right): https://chromiumcodereview.appspot.com/9688004/diff/1/chrome/common/extension... chrome/common/extensions/docs/examples/api/silent_tts_engine/background.html:2: <script type="text/javascript" src="silent_tts_engine.js"></script> It's no longer necessary to have a background.html page - the new format is: "background": { "scripts": ["background.js"] }, You'll need to specify manifest version 2 to do this: http://code.google.com/chrome/extensions/manifest.html#manifest_version https://chromiumcodereview.appspot.com/9688004/diff/1/chrome/common/extension... File chrome/common/extensions/docs/examples/api/silent_tts_engine/manifest.json (right): https://chromiumcodereview.appspot.com/9688004/diff/1/chrome/common/extension... chrome/common/extensions/docs/examples/api/silent_tts_engine/manifest.json:10: "voice_name": "Console", This should match the extension name. I actually like "Console TTS Engine" a little better than "Silent", which implies it might do nothing. Another name could be "Subtitle", since it's kind of like a subtitle display. https://chromiumcodereview.appspot.com/9688004/diff/1/chrome/common/extension... File chrome/common/extensions/docs/examples/api/silent_tts_engine/silent_tts_engine.js (right): https://chromiumcodereview.appspot.com/9688004/diff/1/chrome/common/extension... chrome/common/extensions/docs/examples/api/silent_tts_engine/silent_tts_engine.js:1: var t; To be more clear, call this timeoutId or something like that https://chromiumcodereview.appspot.com/9688004/diff/1/chrome/common/extension... chrome/common/extensions/docs/examples/api/silent_tts_engine/silent_tts_engine.js:3: var tts_id = -1; Would prefer consistent variable naming - the Chrome style is ttsId rather than tts_id (but consistency within the same file is most important) https://chromiumcodereview.appspot.com/9688004/diff/1/chrome/common/extension... chrome/common/extensions/docs/examples/api/silent_tts_engine/silent_tts_engine.js:23: tts_window.document.getElementById("text").innerHTML = Put tts_window.document in a global like tts_doc. Alternatively make a utility function getTtsElement that calls tts_window.document.getElementById so all you need is: getTtsElement("text").innerHTML = ... https://chromiumcodereview.appspot.com/9688004/diff/1/chrome/common/extension... chrome/common/extensions/docs/examples/api/silent_tts_engine/silent_tts_engine.js:24: tts_window.document.getElementById("text").innerHTML + text; Use the += operator https://chromiumcodereview.appspot.com/9688004/diff/1/chrome/common/extension... chrome/common/extensions/docs/examples/api/silent_tts_engine/silent_tts_engine.js:54: t = setTimeout(function(){log_utterance(utterance, ++index, sendTtsEvent)}, Indent it just like any other function, like this: t = setTimeout(function() { log_utterance(utterance, ++index, sendTtsEvent) }, milliseconds);
https://chromiumcodereview.appspot.com/9688004/diff/1/chrome/common/extension... File chrome/common/extensions/docs/examples/api/silent_tts_engine/background.html (right): https://chromiumcodereview.appspot.com/9688004/diff/1/chrome/common/extension... chrome/common/extensions/docs/examples/api/silent_tts_engine/background.html:2: <script type="text/javascript" src="silent_tts_engine.js"></script> On 2012/05/16 05:49:58, Dominic Mazzoni wrote: > It's no longer necessary to have a background.html page - the new format is: > > "background": { > "scripts": ["background.js"] > }, > > You'll need to specify manifest version 2 to do this: > > http://code.google.com/chrome/extensions/manifest.html#manifest_version > Done. https://chromiumcodereview.appspot.com/9688004/diff/1/chrome/common/extension... File chrome/common/extensions/docs/examples/api/silent_tts_engine/manifest.json (right): https://chromiumcodereview.appspot.com/9688004/diff/1/chrome/common/extension... chrome/common/extensions/docs/examples/api/silent_tts_engine/manifest.json:10: "voice_name": "Console", On 2012/05/16 05:49:58, Dominic Mazzoni wrote: > This should match the extension name. I actually like "Console TTS Engine" a > little better than "Silent", which implies it might do nothing. > > Another name could be "Subtitle", since it's kind of like a subtitle display. Done. https://chromiumcodereview.appspot.com/9688004/diff/1/chrome/common/extension... File chrome/common/extensions/docs/examples/api/silent_tts_engine/silent_tts_engine.js (right): https://chromiumcodereview.appspot.com/9688004/diff/1/chrome/common/extension... chrome/common/extensions/docs/examples/api/silent_tts_engine/silent_tts_engine.js:1: var t; On 2012/05/16 05:49:58, Dominic Mazzoni wrote: > To be more clear, call this timeoutId or something like that Done. https://chromiumcodereview.appspot.com/9688004/diff/1/chrome/common/extension... chrome/common/extensions/docs/examples/api/silent_tts_engine/silent_tts_engine.js:3: var tts_id = -1; On 2012/05/16 05:49:58, Dominic Mazzoni wrote: > Would prefer consistent variable naming - the Chrome style is ttsId rather than > tts_id (but consistency within the same file is most important) Done. https://chromiumcodereview.appspot.com/9688004/diff/1/chrome/common/extension... chrome/common/extensions/docs/examples/api/silent_tts_engine/silent_tts_engine.js:23: tts_window.document.getElementById("text").innerHTML = On 2012/05/16 05:49:58, Dominic Mazzoni wrote: > Put tts_window.document in a global like tts_doc. > > Alternatively make a utility function getTtsElement that calls > tts_window.document.getElementById so all you need is: > > getTtsElement("text").innerHTML = ... > Done. https://chromiumcodereview.appspot.com/9688004/diff/1/chrome/common/extension... chrome/common/extensions/docs/examples/api/silent_tts_engine/silent_tts_engine.js:24: tts_window.document.getElementById("text").innerHTML + text; On 2012/05/16 05:49:58, Dominic Mazzoni wrote: > Use the += operator Done. https://chromiumcodereview.appspot.com/9688004/diff/1/chrome/common/extension... chrome/common/extensions/docs/examples/api/silent_tts_engine/silent_tts_engine.js:54: t = setTimeout(function(){log_utterance(utterance, ++index, sendTtsEvent)}, On 2012/05/16 05:49:58, Dominic Mazzoni wrote: > Indent it just like any other function, like this: > > t = setTimeout(function() { > log_utterance(utterance, ++index, sendTtsEvent) > }, milliseconds); Done.
lgtm https://chromiumcodereview.appspot.com/9688004/diff/7003/chrome/common/extens... File chrome/common/extensions/docs/examples/api/console_tts_engine/console_tts_engine.html (right): https://chromiumcodereview.appspot.com/9688004/diff/7003/chrome/common/extens... chrome/common/extensions/docs/examples/api/console_tts_engine/console_tts_engine.html:3: <title>Silent TTS Engine</title> Silent -> Console
mitchellwrosen: have you signed the individual contributors license agreement and added yourself to the OWNERS file? http://dev.chromium.org/developers/contributing-code Confirm that and upload the final version and I'll land click on the commit queue button.
On 2012/05/16 21:35:23, Dominic Mazzoni wrote: > mitchellwrosen: have you signed the individual contributors license agreement > and added yourself to the OWNERS file? > > http://dev.chromium.org/developers/contributing-code > > Confirm that and upload the final version and I'll land click on the commit > queue button. I did sign the contributors license. Which OWNERS file should I add my name to? The closest one to my extension is chrome/common/extensions. Should I maybe make a new one inside chrome/common/extensions/docs/examples/api/console_tts_engine?
On 2012/05/18 21:38:13, mitchellwrosen wrote: > On 2012/05/16 21:35:23, Dominic Mazzoni wrote: > > mitchellwrosen: have you signed the individual contributors license agreement > > and added yourself to the OWNERS file? > > > > http://dev.chromium.org/developers/contributing-code > > > > Confirm that and upload the final version and I'll land click on the commit > > queue button. > > I did sign the contributors license. Which OWNERS file should I add my name to? > The closest one to my extension is chrome/common/extensions. Should I maybe make > a new one inside chrome/common/extensions/docs/examples/api/console_tts_engine? Or perhaps you meant src/AUTHORS?
> Or perhaps you meant src/AUTHORS? Sorry, you're right - I meant src/AUTHORS. I think it's okay to do that in this same changelist.
I have an @chromium email address, though :) I'm a student at Cal Poly, working for Software Inventions, contracted by Google to make a Chrome performance monitor + more. Been working on Chromium for a couple months now.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mitchellwrosen@chromium.org/9688004/4006
Presubmit check for 9688004-4006 failed and returned exit status 1. Running presubmit commit checks ... ** Presubmit Warnings ** License must match: .*? Copyright \(c\) 2012 The Chromium Authors\. All rights reserved\.\n.*? Use of this source code is governed by a BSD-style license that can be\n.*? found in the LICENSE file\.(?: \*/)?\n Found a bad license header in these files: chrome/common/extensions/docs/examples/api/console_tts_engine/console_tts_engine.js Presubmit checks took 1.9s to calculate.
Whoops... didn't notice the presubmit checks the first time around. Sorry.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mitchellwrosen@chromium.org/9688004/18002
Try job failure for 9688004-18002 (retry) on linux_rel for step "browser_tests". It's a second try, previously, step "browser_tests" failed. http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mitchellwrosen@chromium.org/9688004/6005
Change committed as 138047 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
