Relanding the language detection.

Relanding the language detection code.

The code would crash if multiple PageContents notifications
were received rapidly.  The CLDHelper now notifies the
TabContents directly and the TabContents ensures only one
language detection can be performed at a time.
Added unit-tests to validate these cases in web_contents_unittest.cc.

Note that patch set 1 is the original patch, patch set 2 contains the new changes.

Original review:
http://codereview.chromium.org/492024/show

BUG=30662
TEST=Run the unit-tests.


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

[jcampan, 2009-12-18 22:34:00]

There are still bugs in the actual CLD toolbar library that were triggered by
the reliability tests. There are been worked on.
Could you review this in the mean-time so I can land as soon as the CLD library
is fixed?

[brettw, 2009-12-21 18:55:00]

http://codereview.chromium.org/504051/diff/2016/3013
File chrome/browser/cld_helper.cc (right):

http://codereview.chromium.org/504051/diff/2016/3013#newcode38
chrome/browser/cld_helper.cc:38: void CLDHelper::DoDetectLanguage() {
What do you think about checking for tab_contents_ == NULL here and returning
early? You would be able to save the work in some occasional cases. This should
be safe to do without locking or anything.

http://codereview.chromium.org/504051/diff/2016/3014
File chrome/browser/tab_contents/tab_contents.cc (right):

http://codereview.chromium.org/504051/diff/2016/3014#newcode646
chrome/browser/tab_contents/tab_contents.cc:646: if (entry && entry->page_id()
== cld_helper_->page_id())
I don't think this check is sufficient. The page ID is only unique within a
given RenderProcess. If we've had a cross-site transition or an interstitial, we
could have an active entry with the same page ID, but a different page. Can you
also store the child process ID and check it here?

http://codereview.chromium.org/504051/diff/2016/3015
File chrome/browser/tab_contents/tab_contents.h (right):

http://codereview.chromium.org/504051/diff/2016/3015#newcode293
chrome/browser/tab_contents/tab_contents.h:293: void PageLanguageDetected(const
std::string& language, int page_id);
This makes more sense to me with the arguments reversed, since its setting the
language for the given page id.

[jcampan, 2009-12-21 19:48:21]

http://codereview.chromium.org/504051/diff/2016/3013
File chrome/browser/cld_helper.cc (right):

http://codereview.chromium.org/504051/diff/2016/3013#newcode38
chrome/browser/cld_helper.cc:38: void CLDHelper::DoDetectLanguage() {
On 2009/12/21 18:55:00, brettw wrote:
> What do you think about checking for tab_contents_ == NULL here and returning
> early? You would be able to save the work in some occasional cases. This
should
> be safe to do without locking or anything.
Good idea. Done.

http://codereview.chromium.org/504051/diff/2016/3014
File chrome/browser/tab_contents/tab_contents.cc (right):

http://codereview.chromium.org/504051/diff/2016/3014#newcode646
chrome/browser/tab_contents/tab_contents.cc:646: if (entry && entry->page_id()
== cld_helper_->page_id())
On 2009/12/21 18:55:00, brettw wrote:
> I don't think this check is sufficient. The page ID is only unique within a
> given RenderProcess. If we've had a cross-site transition or an interstitial,
we
> could have an active entry with the same page ID, but a different page. Can
you
> also store the child process ID and check it here?
You are right! Done.

http://codereview.chromium.org/504051/diff/2016/3015
File chrome/browser/tab_contents/tab_contents.h (right):

http://codereview.chromium.org/504051/diff/2016/3015#newcode293
chrome/browser/tab_contents/tab_contents.h:293: void PageLanguageDetected(const
std::string& language, int page_id);
On 2009/12/21 18:55:00, brettw wrote:
> This makes more sense to me with the arguments reversed, since its setting the
> language for the given page id.

Done.

[brettw, 2009-12-22 23:42:43]

LGTM with minor change.

http://codereview.chromium.org/504051/diff/3040/3049
File chrome/browser/tab_contents/tab_contents.h (right):

http://codereview.chromium.org/504051/diff/3040/3049#newcode293
chrome/browser/tab_contents/tab_contents.h:293: void PageLanguageDetected(int
page_id, const std::string& language);
It doesn't look like you use the args anymore, and just ask the CLD helper for
the IDs and the language. I think you can just delete them. Don't forget to
update the comment above the decl in the header.