Add code to support options for TTS speak.

Add code to support options for TTS speak.

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=56788

[Chaitanya, 2010-08-18 22:16:42]

I will update the description for this parameter in extension_api.json in a
separate CL?

On 2010/08/18 22:14:54, Chaitanya wrote:
>

[asargent_no_longer_on_chrome, 2010-08-18 22:52:35]

http://codereview.chromium.org/3116019/diff/6001/7001
File chrome/browser/extensions/extension_tts_api.cc (right):

http://codereview.chromium.org/3116019/diff/6001/7001#newcode15
chrome/browser/extensions/extension_tts_api.cc:15: #include <fstream>
nit: remove this (I assume it was for debugging)

http://codereview.chromium.org/3116019/diff/6001/7001#newcode24
chrome/browser/extensions/extension_tts_api.cc:24: int int_value;
nit - you could push the declaration of int_value here into the body of the test
for type integer

http://codereview.chromium.org/3116019/diff/6001/7001#newcode76
chrome/browser/extensions/extension_tts_api.cc:76:
base::DoubleToString(real_value * 5) + ";";
You should have named constants instead of literals here and above for '5' and
'2', and a comment explaining why you do the multiplication (if the names aren't
self-explanatory).

Also, several lines here seem to be doing the same pattern - look for key, read
it, append "keyname=value*factor;" to options. Think about whether you could
factor some of that out into a method. It's probably not a big deal but could
make the code read a little nicer.

http://codereview.chromium.org/3116019/diff/6001/7001#newcode80
chrome/browser/extensions/extension_tts_api.cc:80: if (options.compare("") != 0)
{
nit: it's probably clearer to write this as:

if (!options.empty()) {

[Chaitanya, 2010-08-18 23:47:56]

http://codereview.chromium.org/3116019/diff/6001/7001
File chrome/browser/extensions/extension_tts_api.cc (right):

http://codereview.chromium.org/3116019/diff/6001/7001#newcode15
chrome/browser/extensions/extension_tts_api.cc:15: #include <fstream>
On 2010/08/18 22:52:35, Antony Sargent wrote:
> nit: remove this (I assume it was for debugging)

Done.

http://codereview.chromium.org/3116019/diff/6001/7001#newcode24
chrome/browser/extensions/extension_tts_api.cc:24: int int_value;
On 2010/08/18 22:52:35, Antony Sargent wrote:
> nit - you could push the declaration of int_value here into the body of the
test
> for type integer

Done.

http://codereview.chromium.org/3116019/diff/6001/7001#newcode76
chrome/browser/extensions/extension_tts_api.cc:76:
base::DoubleToString(real_value * 5) + ";";
Tried to make this a tad bit cleaner. I couldn't collapse everything under one
function because there is a small mismatch with the key name for
kLanguageNameKey and kNameKey.

On 2010/08/18 22:52:35, Antony Sargent wrote:
> You should have named constants instead of literals here and above for '5' and
> '2', and a comment explaining why you do the multiplication (if the names
aren't
> self-explanatory).
> 
> Also, several lines here seem to be doing the same pattern - look for key,
read
> it, append "keyname=value*factor;" to options. Think about whether you could
> factor some of that out into a method. It's probably not a big deal but could
> make the code read a little nicer.
>

http://codereview.chromium.org/3116019/diff/6001/7001#newcode80
chrome/browser/extensions/extension_tts_api.cc:80: if (options.compare("") != 0)
{
On 2010/08/18 22:52:35, Antony Sargent wrote:
> nit: it's probably clearer to write this as:
> 
> if (!options.empty()) {

Done.

[asargent_no_longer_on_chrome, 2010-08-19 00:03:43]

lgtm w/ a few nits you should address before committing.

http://codereview.chromium.org/3116019/diff/10001/11001
File chrome/browser/extensions/extension_tts_api.cc (right):

http://codereview.chromium.org/3116019/diff/10001/11001#newcode46
chrome/browser/extensions/extension_tts_api.cc:46: void
AppendSpeakOptions(std::string key, std::string value,
Nit: It might be better to make this name singlar, ie "AppendSpeakOption", since
this only appends one option at at time.

http://codereview.chromium.org/3116019/diff/10001/11001#newcode47
chrome/browser/extensions/extension_tts_api.cc:47: std::string& options) {
Nit: we only allow pass-by-reference for const reference types. You should pass
options as std::string* instead of std::string&.

http://codereview.chromium.org/3116019/diff/10001/11001#newcode79
chrome/browser/extensions/extension_tts_api.cc:79:
base::DoubleToString(real_value * 5), options);
Nit: there was a recent thread on chromium-dev about function argument
formatting, where the conclusion was that we should have a preference for
aligning arguments with each other. So lines here should be intended like this:

SomeFunctionName(SomeParameter
                 SomeOtherParameter);

or

SomeFunctionName(
    SomeParameter
    SomeOtherParameter);


http://groups.google.com/a/chromium.org/group/chromium-
dev/browse_thread/thread/e84e1cbf130f772e/07a18dfd69e2b26e


Also, you could shorten this by adding

using base::DoubleToString;

at the top of the file, and then just calling DoubleToString various places
here.

[Chaitanya, 2010-08-19 00:24:24]

Fixed nits. Running try bots now.
Thanks!

http://codereview.chromium.org/3116019/diff/10001/11001
File chrome/browser/extensions/extension_tts_api.cc (right):

http://codereview.chromium.org/3116019/diff/10001/11001#newcode46
chrome/browser/extensions/extension_tts_api.cc:46: void
AppendSpeakOptions(std::string key, std::string value,
On 2010/08/19 00:03:43, Antony Sargent wrote:
> Nit: It might be better to make this name singlar, ie "AppendSpeakOption",
since
> this only appends one option at at time. 

Done.

http://codereview.chromium.org/3116019/diff/10001/11001#newcode47
chrome/browser/extensions/extension_tts_api.cc:47: std::string& options) {
On 2010/08/19 00:03:43, Antony Sargent wrote:
> Nit: we only allow pass-by-reference for const reference types. You should
pass
> options as std::string* instead of std::string&.
> 

Done.

http://codereview.chromium.org/3116019/diff/10001/11001#newcode79
chrome/browser/extensions/extension_tts_api.cc:79:
base::DoubleToString(real_value * 5), options);
On 2010/08/19 00:03:43, Antony Sargent wrote:
> Nit: there was a recent thread on chromium-dev about function argument
> formatting, where the conclusion was that we should have a preference for
> aligning arguments with each other. So lines here should be intended like
this:
> 
> SomeFunctionName(SomeParameter
>                  SomeOtherParameter);
> 
> or
> 
> SomeFunctionName(
>     SomeParameter
>     SomeOtherParameter);
> 
> 
> http://groups.google.com/a/chromium.org/group/chromium-
> dev/browse_thread/thread/e84e1cbf130f772e/07a18dfd69e2b26e
> 
> 
> Also, you could shorten this by adding
> 
> using base::DoubleToString;
> 
> at the top of the file, and then just calling DoubleToString various places
> here. 
> 
> 
> 

Done.