Remove dependency on language detection library.

Android: Remove dependency on language detection library.
    
Savings: 1.2MB in stripped so (in the .rodata section)
         750K in APK size


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

[Jay Civelli, 2012-11-02 01:01:25]

http://codereview.chromium.org/11361051/diff/2001/chrome/browser/chrome_brows...
File chrome/browser/chrome_browser_main.cc (right):

http://codereview.chromium.org/11361051/diff/2001/chrome/browser/chrome_brows...
chrome/browser/chrome_browser_main.cc:1352: #if !defined(OS_ANDROID)
Maybe add a comment explaining we don't have language detection enabled on
Android.

http://codereview.chromium.org/11361051/diff/2001/chrome/chrome_browser_ui.gypi
File chrome/chrome_browser_ui.gypi (left):

http://codereview.chromium.org/11361051/diff/2001/chrome/chrome_browser_ui.gy...
chrome/chrome_browser_ui.gypi:2096: '../third_party/cld/cld.gyp:cld',
Why the removal here?

[nilesh, 2012-11-02 01:40:41]

http://codereview.chromium.org/11361051/diff/2001/chrome/browser/chrome_brows...
File chrome/browser/chrome_browser_main.cc (right):

http://codereview.chromium.org/11361051/diff/2001/chrome/browser/chrome_brows...
chrome/browser/chrome_browser_main.cc:1352: #if !defined(OS_ANDROID)
On 2012/11/02 01:01:25, Jay Civelli wrote:
> Maybe add a comment explaining we don't have language detection enabled on
> Android.

Done.

http://codereview.chromium.org/11361051/diff/2001/chrome/chrome_browser_ui.gypi
File chrome/chrome_browser_ui.gypi (left):

http://codereview.chromium.org/11361051/diff/2001/chrome/chrome_browser_ui.gy...
chrome/chrome_browser_ui.gypi:2096: '../third_party/cld/cld.gyp:cld',
I dont think it is needed here. I could not find any code in borwser/ui/ which
uses third_party/cld

[Nico, 2012-11-02 15:23:26]

(+stuart, in case ios wants to do this too)

http://codereview.chromium.org/11361051/diff/9001/chrome/browser/chrome_brows...
File chrome/browser/chrome_browser_main.cc (right):

http://codereview.chromium.org/11361051/diff/9001/chrome/browser/chrome_brows...
chrome/browser/chrome_browser_main.cc:124: #if !defined(OS_ANDROID)
Drive-by: Instead of gating this on OS_ANDROID, could you use a
enable_language_detection feature flag instead? (see build/common.gypi, for
example enable_printing for how that's done.) Then other ports, say iOS, only
have to change common.gypi instead of adding OS_IOS to all the places where
you're adding OS_ANDROID.

[stuartmorgan, 2012-11-02 15:40:32]

On 2012/11/02 15:23:26, Nico wrote:
> (+stuart, in case ios wants to do this too)

We don't build any of these files (we had to do ChromeBrowerMain differently due
to the way message loops work, and we don't have renderer/), but from a
conceptual standpoint if there's going to be an enable_language_detection (+1
for that, btw), let's go ahead and put iOS in the list for "off" since we
certainly don't have it on.

[nilesh, 2012-11-02 17:37:31]

Thanks for the comments. Please take another look.

http://codereview.chromium.org/11361051/diff/9001/chrome/browser/chrome_brows...
File chrome/browser/chrome_browser_main.cc (right):

http://codereview.chromium.org/11361051/diff/9001/chrome/browser/chrome_brows...
chrome/browser/chrome_browser_main.cc:124: #if !defined(OS_ANDROID)
Done.

[Nico, 2012-11-02 17:43:53]

I'll stamp once Jay is happy.

http://codereview.chromium.org/11361051/diff/6002/chrome/renderer/translate_h...
File chrome/renderer/translate_helper.cc (right):

http://codereview.chromium.org/11361051/diff/6002/chrome/renderer/translate_h...
chrome/renderer/translate_helper.cc:113: bool
TranslateHelper::IsPageTranslatable(WebDocument* document) {
It feels like a lot more of this class could be disabled if you don't need
translation (medium-term maybe it should be an interface with an empty
implementation for non-translating platforms), but if Jay is happy with this, I
guess it's fine.

[Jay Civelli, 2012-11-02 17:52:36]

lgtm

http://codereview.chromium.org/11361051/diff/6002/chrome/renderer/translate_h...
File chrome/renderer/translate_helper.cc (right):

http://codereview.chromium.org/11361051/diff/6002/chrome/renderer/translate_h...
chrome/renderer/translate_helper.cc:113: bool
TranslateHelper::IsPageTranslatable(WebDocument* document) {
On 2012/11/02 17:43:54, Nico wrote:
> It feels like a lot more of this class could be disabled if you don't need
> translation (medium-term maybe it should be an interface with an empty
> implementation for non-translating platforms), but if Jay is happy with this,
I
> guess it's fine.

The idea is that we just want to exclude the language detection for now and
leave the translate stuff in. So if a page specifies its language, or if the
user triggers a translation (through a menu) the feature would work.
The problem with the CLD (language detection) is that it adds more than 1MB to
our binary.

[Nico, 2012-11-02 17:53:39]

On 2012/11/02 17:52:36, Jay Civelli wrote:
> lgtm

lgtm

> 
>
http://codereview.chromium.org/11361051/diff/6002/chrome/renderer/translate_h...
> File chrome/renderer/translate_helper.cc (right):
> 
>
http://codereview.chromium.org/11361051/diff/6002/chrome/renderer/translate_h...
> chrome/renderer/translate_helper.cc:113: bool
> TranslateHelper::IsPageTranslatable(WebDocument* document) {
> On 2012/11/02 17:43:54, Nico wrote:
> > It feels like a lot more of this class could be disabled if you don't need
> > translation (medium-term maybe it should be an interface with an empty
> > implementation for non-translating platforms), but if Jay is happy with
this,
> I
> > guess it's fine.
> 
> The idea is that we just want to exclude the language detection for now and
> leave the translate stuff in. So if a page specifies its language, or if the
> user triggers a translation (through a menu) the feature would work.
> The problem with the CLD (language detection) is that it adds more than 1MB to
> our binary.

Ah, that makes sense. Maybe the CL description could call that out more
explicitly (but it's now mentioned on the review, so either way is fine)

[Nico, 2012-11-02 17:54:12]

On Fri, Nov 2, 2012 at 10:53 AM,  <thakis@chromium.org> wrote:
> On 2012/11/02 17:52:36, Jay Civelli wrote:
>>
>> lgtm
>
>
> lgtm

(gmail collapsed it, but I too said lgtm up here)

>
>
>
>
http://codereview.chromium.org/11361051/diff/6002/chrome/renderer/translate_h...
>>
>> File chrome/renderer/translate_helper.cc (right):
>
>
>
>
http://codereview.chromium.org/11361051/diff/6002/chrome/renderer/translate_h...
>>
>> chrome/renderer/translate_helper.cc:113: bool
>> TranslateHelper::IsPageTranslatable(WebDocument* document) {
>> On 2012/11/02 17:43:54, Nico wrote:
>> > It feels like a lot more of this class could be disabled if you don't
>> > need
>> > translation (medium-term maybe it should be an interface with an empty
>> > implementation for non-translating platforms), but if Jay is happy with
>
> this,
>>
>> I
>> > guess it's fine.
>
>
>> The idea is that we just want to exclude the language detection for now
>> and
>> leave the translate stuff in. So if a page specifies its language, or if
>> the
>> user triggers a translation (through a menu) the feature would work.
>> The problem with the CLD (language detection) is that it adds more than
>> 1MB to
>> our binary.
>
>
> Ah, that makes sense. Maybe the CL description could call that out more
> explicitly (but it's now mentioned on the review, so either way is fine)
>
> http://codereview.chromium.org/11361051/

[commit-bot: I haz the power, 2012-11-02 18:27:19]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/nileshagrawal@chromium.org/11361051/6002

[commit-bot: I haz the power, 2012-11-02 18:33:35]

Step "update" is always a major failure.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2012-11-02 21:12:38]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/nileshagrawal@chromium.org/11361051/6002

[commit-bot: I haz the power, 2012-11-03 00:26:49]

Retried try job too often for step(s) content_browsertests

[commit-bot: I haz the power, 2012-11-05 16:12:01]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/nileshagrawal@chromium.org/11361051/6002

[commit-bot: I haz the power, 2012-11-05 16:12:07]

Failed to apply patch for chrome/chrome_browser.gypi:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file chrome/chrome_browser.gypi
  Hunk #1 FAILED at 51.
  Hunk #2 succeeded at 2564 (offset 28 lines).
  1 out of 2 hunks FAILED -- saving rejects to file
chrome/chrome_browser.gypi.rej

Patch:       chrome/chrome_browser.gypi
Index: chrome/chrome_browser.gypi
diff --git a/chrome/chrome_browser.gypi b/chrome/chrome_browser.gypi
index
b7b8d765d53d23639efbc88aaa6c7e49768eae33..2d3e9fe9093654a2a80eaf5a07b7a1eafd5558ed
100644
--- a/chrome/chrome_browser.gypi
+++ b/chrome/chrome_browser.gypi
@@ -51,7 +51,6 @@
         '../third_party/bzip2/bzip2.gyp:bzip2',
        
'../third_party/cacheinvalidation/cacheinvalidation.gyp:cacheinvalidation',
        
'../third_party/cacheinvalidation/cacheinvalidation.gyp:cacheinvalidation_proto_cpp',
-        '../third_party/cld/cld.gyp:cld',
         '../third_party/expat/expat.gyp:expat',
         '../third_party/hunspell/hunspell.gyp:hunspell',
         '../third_party/icu/icu.gyp:icui18n',
@@ -2536,6 +2535,16 @@
             ['exclude', '^browser/automation/'],
           ],
         }],
+        ['enable_language_detection==0', {
+          'sources!': [
+            'browser/language_usage_metrics.cc',
+            'browser/language_usage_metrics.h',
+          ],
+        }, {   # enable_language_detection==1
+          'dependencies': [
+            '../third_party/cld/cld.gyp:cld',
+          ],
+        }],
         ['enable_printing!=1', {
           'sources/': [
             ['exclude', '^browser/ui/webui/print_preview/'],

[commit-bot: I haz the power, 2012-11-05 17:47:31]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/nileshagrawal@chromium.org/11361051/10014

[commit-bot: I haz the power, 2012-11-05 20:40:32]

Change committed as 166024