Fix a crash in the translate info bar.

Fix a crash in the translate info bar.

A language from the accepted list was converted before being validated, but the unconverted version was returned.

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

[MAD, 2011-08-05 21:55:44]

A subtle crash was introduced by the new feature of looking up all accepted
languages... If we found one that needed to be converted to remove the country
code, then we return the unconverted version and so it wasn't recognized
correctly...

Also added a new test for this case which crashes without the fix...

Thanks!

BYE
MAD

[MAD, 2011-08-08 17:55:25]

Ping!

This is a Beta crasher that will need to be merged up...

It's a very simple fix that should be quick to review...

Thanks!

BYE
MAD

On Fri, Aug 5, 2011 at 5:55 PM, <mad@chromium.org> wrote:

> Reviewers: dfilimon, bstell1, Jungshik Shin,
>
> Message:
> A subtle crash was introduced by the new feature of looking up all accepted
> languages... If we found one that needed to be converted to remove the
> country
> code, then we return the unconverted version and so it wasn't recognized
> correctly...
>
> Also added a new test for this case which crashes without the fix...
>
> Thanks!
>
> BYE
> MAD
>
>
> Description:
> Fix a crash in the translate info bar.
>
> A language from the accepted list was converted before being validated, but
> the
> unconverted version was returned.
>
> BUG=90106
>
> Please review this at
http://codereview.chromium.**org/7589002/<http://codereview.chromium.org/7589...
>
> SVN Base:
svn://svn.chromium.org/chrome/**trunk/src/<http://svn.chromium.org/chrome/trunk/src/>
>
> Affected files:
>  M     chrome/browser/translate/**translate_manager.cc
>  M     chrome/browser/translate/**translate_manager_browsertest.**cc
>
>
> Index: chrome/browser/translate/**translate_manager.cc
> ==============================**==============================**=======
> --- chrome/browser/translate/**translate_manager.cc       (revision 95591)
> +++ chrome/browser/translate/**translate_manager.cc       (working copy)
> @@ -781,8 +781,9 @@
>   std::vector<std::string>::**iterator iter;
>   for (iter = accept_langs_list.begin();
>        iter != accept_langs_list.end(); ++iter) {
> -    if (IsSupportedLanguage(**GetLanguageCode(*iter)))
> -      return *iter;
> +    std::string lang_code = GetLanguageCode(*iter);
> +    if (IsSupportedLanguage(lang_**code))
> +      return lang_code;
>   }
>   return std::string();
>  }
> Index: chrome/browser/translate/**translate_manager_browsertest.**cc
> ==============================**==============================**=======
> --- chrome/browser/translate/**translate_manager_browsertest.**cc
> (revision 95591)
> +++ chrome/browser/translate/**translate_manager_browsertest.**cc
> (working copy)
> @@ -903,6 +903,16 @@
>
>   // Expect the infobar to pop up
>   EXPECT_TRUE(**GetTranslateInfoBar() != NULL);
> +
> +  // Set Qbz and English-US as the only accepted languages to test the
> country
> +  // code removal code which was causing a crash as filed in Issue 90106.
> +  prefs->SetString(prefs::**kAcceptLanguages, "qbz,en-us");
> +
> +  // Go to a German page
> +  SimulateNavigation(GURL("http:**//google.de <http://google.de>"), "de",
> true);
> +
> +  // Expect the infobar to pop up
> +  EXPECT_TRUE(**GetTranslateInfoBar() != NULL);
>  }
>
>  // Tests that the translate enabled preference is honored.
>
>
>

[dfilimon, 2011-08-08 18:17:34]

On 2011/08/08 17:55:25, MAD wrote:
> Ping!
> 
> This is a Beta crasher that will need to be merged up...
> 
> It's a very simple fix that should be quick to review...
> 
> Thanks!
> 
> BYE
> MAD
> 
> On Fri, Aug 5, 2011 at 5:55 PM, <mailto:mad@chromium.org> wrote:
> 
> > Reviewers: dfilimon, bstell1, Jungshik Shin,
> >
> > Message:
> > A subtle crash was introduced by the new feature of looking up all accepted
> > languages... If we found one that needed to be converted to remove the
> > country
> > code, then we return the unconverted version and so it wasn't recognized
> > correctly...
> >
> > Also added a new test for this case which crashes without the fix...
> >
> > Thanks!
> >
> > BYE
> > MAD
> >
> >
> > Description:
> > Fix a crash in the translate info bar.
> >
> > A language from the accepted list was converted before being validated, but
> > the
> > unconverted version was returned.
> >
> > BUG=90106
> >
> > Please review this at
>
http://codereview.chromium.**org/7589002/%3Chttp://codereview.chromium.org/75...>
> >
> > SVN Base:
>
svn://svn.chromium.org/chrome/**trunk/src/<http://svn.chromium.org/chrome/trunk/src/>
> >
> > Affected files:
> >  M     chrome/browser/translate/**translate_manager.cc
> >  M     chrome/browser/translate/**translate_manager_browsertest.**cc
> >
> >
> > Index: chrome/browser/translate/**translate_manager.cc
> > ==============================**==============================**=======
> > --- chrome/browser/translate/**translate_manager.cc       (revision 95591)
> > +++ chrome/browser/translate/**translate_manager.cc       (working copy)
> > @@ -781,8 +781,9 @@
> >   std::vector<std::string>::**iterator iter;
> >   for (iter = accept_langs_list.begin();
> >        iter != accept_langs_list.end(); ++iter) {
> > -    if (IsSupportedLanguage(**GetLanguageCode(*iter)))
> > -      return *iter;
> > +    std::string lang_code = GetLanguageCode(*iter);
> > +    if (IsSupportedLanguage(lang_**code))
> > +      return lang_code;
> >   }
> >   return std::string();
> >  }
> > Index: chrome/browser/translate/**translate_manager_browsertest.**cc
> > ==============================**==============================**=======
> > --- chrome/browser/translate/**translate_manager_browsertest.**cc
> > (revision 95591)
> > +++ chrome/browser/translate/**translate_manager_browsertest.**cc
> > (working copy)
> > @@ -903,6 +903,16 @@
> >
> >   // Expect the infobar to pop up
> >   EXPECT_TRUE(**GetTranslateInfoBar() != NULL);
> > +
> > +  // Set Qbz and English-US as the only accepted languages to test the
> > country
> > +  // code removal code which was causing a crash as filed in Issue 90106.
> > +  prefs->SetString(prefs::**kAcceptLanguages, "qbz,en-us");
> > +
> > +  // Go to a German page
> > +  SimulateNavigation(GURL("http:**//google.de <http://google.de%3E%22),
"de",
> > true);
> > +
> > +  // Expect the infobar to pop up
> > +  EXPECT_TRUE(**GetTranslateInfoBar() != NULL);
> >  }
> >
> >  // Tests that the translate enabled preference is honored.
> >
> >
> >

LGTM.

[commit-bot: I haz the power, 2011-08-08 18:30:48]

No LGTM from valid reviewers yet.

[bstell1, 2011-08-08 18:32:25]

LGTM

http://codereview.chromium.org/7589002/diff/2001/chrome/browser/translate/tra...
File chrome/browser/translate/translate_manager_browsertest.cc (right):

http://codereview.chromium.org/7589002/diff/2001/chrome/browser/translate/tra...
chrome/browser/translate/translate_manager_browsertest.cc:908: // code removal
code which was causing a crash as filed in Issue 90106.
please add a very short description of the issue

[MAD, 2011-08-08 18:42:15]

Thanks!

BYE
MAD

http://codereview.chromium.org/7589002/diff/2001/chrome/browser/translate/tra...
File chrome/browser/translate/translate_manager_browsertest.cc (right):

http://codereview.chromium.org/7589002/diff/2001/chrome/browser/translate/tra...
chrome/browser/translate/translate_manager_browsertest.cc:908: // code removal
code which was causing a crash as filed in Issue 90106.
On 2011/08/08 18:32:25, bstell1 wrote:
> please add a very short description of the issue

Done.

[bstell1, 2011-08-08 19:34:50]

Thank you for fixing this :-D

On Mon, Aug 8, 2011 at 11:42 AM, <mad@chromium.org> wrote:

> Thanks!
>
> BYE
> MAD
>
>
>
> http://codereview.chromium.**org/7589002/diff/2001/chrome/**
>
browser/translate/translate_**manager_browsertest.cc<http://codereview.chromium.org/7589002/diff/2001/chrome/browser/translate/translate_manager_browsertest.cc>
> File chrome/browser/translate/**translate_manager_browsertest.**cc
> (right):
>
> http://codereview.chromium.**org/7589002/diff/2001/chrome/**
>
browser/translate/translate_**manager_browsertest.cc#**newcode908<http://codereview.chromium.org/7589002/diff/2001/chrome/browser/translate/translate_manager_browsertest.cc#newcode908>
> chrome/browser/translate/**translate_manager_browsertest.**cc:908: // code
> removal code which was causing a crash as filed in Issue 90106.
> On 2011/08/08 18:32:25, bstell1 wrote:
>
>> please add a very short description of the issue
>>
>
> Done.
>
>
>
http://codereview.chromium.**org/7589002/<http://codereview.chromium.org/7589...
>

[MAD, 2011-08-08 20:24:16]

Added some NULL checks as suggested by Peter...

[Peter Kasting, 2011-08-08 20:28:49]

http://codereview.chromium.org/7589002/diff/6002/chrome/browser/translate/tra...
File chrome/browser/translate/translate_manager.cc (right):

http://codereview.chromium.org/7589002/diff/6002/chrome/browser/translate/tra...
chrome/browser/translate/translate_manager.cc:518: NOTREACHED();
Don't do this.  This is effectively handling a DCHECK failure which our style
guide bans.

If we want the API call to be able to fail we need to really handle it here and
we need to rename the API to indicate that.  I think we don't want it to be able
to fail, in which case the function should be changed to simply CHECK or DCHECK
its preconditions and this should be removed.

[MAD, 2011-08-08 21:55:27]

How about this then? (I forgot to add the delegate code previously, sorry about
that, it's there now)...

But I would feel more comfortable submitting only the minimal fix for the beta
merge... Agreed?

Thanks!

BYE
MAD

[Peter Kasting, 2011-08-08 22:09:49]

Just merging the minimal fix is OK, although I think merging the complete fix
here shouldn't cause any problem.

http://codereview.chromium.org/7589002/diff/7003/chrome/browser/translate/tra...
File chrome/browser/translate/translate_infobar_delegate.cc (right):

http://codereview.chromium.org/7589002/diff/7003/chrome/browser/translate/tra...
chrome/browser/translate/translate_infobar_delegate.cc:38: // This must be
validated by our callers.
Nit: How about this for comments and checks:

In header:

// Factory method to create a non-error translate infobar.  |original_language|
// and |target_language| must be ASCII language codes (e.g. "en", "fr", etc.)
// for languages the TranslateManager supports translating.  The lone exception
// is when the user initiates translation from the context menu, in which case
// it's legal to call this with |type| == TRANSLATING and
// |originalLanguage| == kUnknownLanguageCode.
... (declaration) ...;

In .cc file:

... (declaration) ... {
  DCHECK_NE(TRANSLATION_ERROR, type);
  DCHECK(TranslateManager::IsSupportedLanguage(original_language) ||
      ((type == TRANSLATING) &&
       (original_language == chrome::kUnknownLanguageCode)));
  DCHECK(TranslateManager::IsSupportedLanguage(target_language));
  ... (function body) ...

http://codereview.chromium.org/7589002/diff/7003/chrome/browser/translate/tra...
File chrome/browser/translate/translate_manager.cc (right):

http://codereview.chromium.org/7589002/diff/7003/chrome/browser/translate/tra...
chrome/browser/translate/translate_manager.cc:745: DCHECK(infobar != NULL);
Nit: Move this to top of function ("precondition")

[MAD, 2011-08-09 14:19:41]

Thanks!

BYE
MAD

http://codereview.chromium.org/7589002/diff/7003/chrome/browser/translate/tra...
File chrome/browser/translate/translate_infobar_delegate.cc (right):

http://codereview.chromium.org/7589002/diff/7003/chrome/browser/translate/tra...
chrome/browser/translate/translate_infobar_delegate.cc:38: // This must be
validated by our callers.
On 2011/08/08 22:09:49, Peter Kasting wrote:
> Nit: How about this for comments and checks:
> 
> In header:
> 
> // Factory method to create a non-error translate infobar. 
|original_language|
> // and |target_language| must be ASCII language codes (e.g. "en", "fr", etc.)
> // for languages the TranslateManager supports translating.  The lone
exception
> // is when the user initiates translation from the context menu, in which case
> // it's legal to call this with |type| == TRANSLATING and
> // |originalLanguage| == kUnknownLanguageCode.
> ... (declaration) ...;
> 
> In .cc file:
> 
> ... (declaration) ... {
>   DCHECK_NE(TRANSLATION_ERROR, type);
>   DCHECK(TranslateManager::IsSupportedLanguage(original_language) ||
>       ((type == TRANSLATING) &&
>        (original_language == chrome::kUnknownLanguageCode)));
>   DCHECK(TranslateManager::IsSupportedLanguage(target_language));
>   ... (function body) ...

Done.

http://codereview.chromium.org/7589002/diff/7003/chrome/browser/translate/tra...
File chrome/browser/translate/translate_manager.cc (right):

http://codereview.chromium.org/7589002/diff/7003/chrome/browser/translate/tra...
chrome/browser/translate/translate_manager.cc:745: DCHECK(infobar != NULL);
On 2011/08/08 22:09:49, Peter Kasting wrote:
> Nit: Move this to top of function ("precondition")

Done.

[jungshik at Google, 2011-08-09 20:41:42]

thank you for the fix. LGTM

[MAD, 2011-08-09 20:59:11]

Waiting for win try bots to go green before I hit the commit box...

Thanks!

On Tue, Aug 9, 2011 at 4:41 PM, <jshin@chromium.org> wrote:

> thank you for the fix. LGTM
>
>
>
http://codereview.chromium.**org/7589002/<http://codereview.chromium.org/7589...
>

[commit-bot: I haz the power, 2011-08-10 04:09:19]

Change committed as 96125