Locale access from the IO thread for null language tags in speech.

In the current speech input implementation, the application locale is used as
language in case that no language attribute is provided. For this, the
l10n_util::GetApplicationLocale was used from the IO thread. However this is
not valid since that function requires file access.

After considering different options this patch now uses the first of the accepted languages, defaulting to "en-US" in case that this list is empty.

BUG=53598
TEST=SpeechRecognitionRequestTest

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

[Leandro Graciá Gil, 2010-11-12 18:00:33]

Currently considering possible alternatives after talking with jabdelmalek.
Please ignore this review for the moment.

[Leandro Graciá Gil, 2010-11-15 16:52:11]

After talking with Satish we have decided to use the 1st accepted language
instead of the application locale for the no specified language attribute case.
This should be safe to perform from the IO thread and avoids the threading
problems related to accessing the locale from g_browser_process or the IOThread
object.

Since this is no longer a threading problem, please Evan and John feel free to
review if you want but do not feel obliged to do so. Both of you have already
been quite helpful :)

[Leandro Graciá Gil, 2010-11-15 16:57:06]

http://codereview.chromium.org/4896001/diff/4001/chrome/browser/speech/speech...
File chrome/browser/speech/speech_recognition_request.cc (right):

http://codereview.chromium.org/4896001/diff/4001/chrome/browser/speech/speech...
chrome/browser/speech/speech_recognition_request.cc:132: // If no language is
provided then we use the 1st from the accepted language list.
Just noticed the line length. I'll fix this.

[Leandro Graciá Gil, 2010-11-16 12:00:01]

The unittest should be fixed now.

[Satish, 2010-11-16 14:23:41]

TEST= should not be left empty. You could either specify 'none' if no relevant
tests are required or in this case if there are existing unittests/browsertests,
specify them here.

http://codereview.chromium.org/4896001/diff/9001/chrome/browser/speech/speech...
File chrome/browser/speech/speech_recognition_request.cc (right):

http://codereview.chromium.org/4896001/diff/9001/chrome/browser/speech/speech...
chrome/browser/speech/speech_recognition_request.cc:145: if (language.empty())
language = "en-US";
Having a local var with the same name 'language' might be confusing to readers.
How about:

std::string lang_param(language);
if (lang_param.empty() && url_context_) {
  ... new code above this line ...
}
if (lang_param.empty()) lang_param = "en-US";
parts.push_back(...)

[Leandro Graciá Gil, 2010-11-16 17:03:00]

http://codereview.chromium.org/4896001/diff/9001/chrome/browser/speech/speech...
File chrome/browser/speech/speech_recognition_request.cc (right):

http://codereview.chromium.org/4896001/diff/9001/chrome/browser/speech/speech...
chrome/browser/speech/speech_recognition_request.cc:145: if (language.empty())
language = "en-US";
On 2010/11/16 14:23:42, Satish wrote:
> Having a local var with the same name 'language' might be confusing to
readers.
> How about:
> 
> std::string lang_param(language);
> if (lang_param.empty() && url_context_) {
>   ... new code above this line ...
> }
> if (lang_param.empty()) lang_param = "en-US";
> parts.push_back(...)

Done.

[Satish, 2010-11-17 10:45:34]

LGTM

Please check with the other reviewers as well before submitting.

[jam, 2010-11-17 17:58:59]

lgtm, thanks for fixing this

http://codereview.chromium.org/4896001/diff/14001/chrome/browser/speech/speec...
File chrome/browser/speech/speech_recognition_request.cc (right):

http://codereview.chromium.org/4896001/diff/14001/chrome/browser/speech/speec...
chrome/browser/speech/speech_recognition_request.cc:143: if (lang_param.empty())
lang_param = "en-US";
nit: while this isn't against the style guide, it makes debugging harder because
it's not possible to set breakpoints.  two lines would be nicer.