Move LanguageState to the translate component

Move LanguageState to the translate component

This CL introduces the TranslateDriver and a content implementation of it
(ContentTranslateDriver), and moves LanguageState to the translate component.
LanguageState gets information from the content driver through the
TranslateDriver API.

BUG=335092
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=246023

[blundell, 2014-01-17 15:09:38]

Mostly nits

The new interface/impl should be TranslateDriver and ContentTranslateDriver. The
Client naming is for the embedder interfaces (assuming no one opposes it on
chromium-dev).

https://codereview.chromium.org/133273029/diff/30003/components/translate/con...
File components/translate/content/browser/DEPS (right):

https://codereview.chromium.org/133273029/diff/30003/components/translate/con...
components/translate/content/browser/DEPS:2: "+content/public/browser",
There should be a DEPS file in ../ aldding content/public/common.

https://codereview.chromium.org/133273029/diff/30003/components/translate/cor...
File components/translate/core/browser/language_state.h (right):

https://codereview.chromium.org/133273029/diff/30003/components/translate/cor...
components/translate/core/browser/language_state.h:15: // There is one
LanguageState instance per WebContents.
s/per WebContents/per tab

https://codereview.chromium.org/133273029/diff/30003/components/translate/cor...
components/translate/core/browser/language_state.h:102: // Whether a translation
is currently pending (WebContents waiting for the
Please rework to avoid WebContents, notfications, renderer, etc. references.

https://codereview.chromium.org/133273029/diff/30003/components/translate/cor...
File components/translate/core/browser/translate_client.h (right):

https://codereview.chromium.org/133273029/diff/30003/components/translate/cor...
components/translate/core/browser/translate_client.h:10: class TranslateClient {
This needs a class-level comment.

https://codereview.chromium.org/133273029/diff/30003/components/translate/cor...
components/translate/core/browser/translate_client.h:19: LanguageState
language_state_;
This should just be in the subclasses I think.

https://codereview.chromium.org/133273029/diff/30003/components/translate/cor...
components/translate/core/browser/translate_client.h:22: // Methods called by
LanguageState.
Why would these methods be private?

[droger, 2014-01-17 16:14:20]

Thanks, I applied the comments.

https://codereview.chromium.org/133273029/diff/30003/components/translate/con...
File components/translate/content/browser/DEPS (right):

https://codereview.chromium.org/133273029/diff/30003/components/translate/con...
components/translate/content/browser/DEPS:2: "+content/public/browser",
On 2014/01/17 15:09:39, blundell wrote:
> There should be a DEPS file in ../ aldding content/public/common.

Done.

https://codereview.chromium.org/133273029/diff/30003/components/translate/cor...
File components/translate/core/browser/language_state.h (right):

https://codereview.chromium.org/133273029/diff/30003/components/translate/cor...
components/translate/core/browser/language_state.h:15: // There is one
LanguageState instance per WebContents.
On 2014/01/17 15:09:39, blundell wrote:
> s/per WebContents/per tab

Done.

https://codereview.chromium.org/133273029/diff/30003/components/translate/cor...
components/translate/core/browser/language_state.h:102: // Whether a translation
is currently pending (WebContents waiting for the
On 2014/01/17 15:09:39, blundell wrote:
> Please rework to avoid WebContents, notfications, renderer, etc. references.

Done.

https://codereview.chromium.org/133273029/diff/30003/components/translate/cor...
File components/translate/core/browser/translate_client.h (right):

https://codereview.chromium.org/133273029/diff/30003/components/translate/cor...
components/translate/core/browser/translate_client.h:10: class TranslateClient {
On 2014/01/17 15:09:39, blundell wrote:
> This needs a class-level comment.

Done.

https://codereview.chromium.org/133273029/diff/30003/components/translate/cor...
components/translate/core/browser/translate_client.h:19: LanguageState
language_state_;
On 2014/01/17 15:09:39, blundell wrote:
> This should just be in the subclasses I think.

Done.

https://codereview.chromium.org/133273029/diff/30003/components/translate/cor...
components/translate/core/browser/translate_client.h:22: // Methods called by
LanguageState.
On 2014/01/17 15:09:39, blundell wrote:
> Why would these methods be private?

As discussed offline, it was that nobody can call them, except LanguageState
which is a friend.
Everything is now public, as it's not a goal for components to have a private
and a public API.

[blundell, 2014-01-17 16:37:35]

I'm going to do one more pass on Monday, but adding mad@ now for his review as
well.

https://codereview.chromium.org/133273029/diff/150001/chrome/browser/translat...
File chrome/browser/translate/translate_tab_helper.h (right):

https://codereview.chromium.org/133273029/diff/150001/chrome/browser/translat...
chrome/browser/translate/translate_tab_helper.h:22: // Information about the
language the page is in and has been translated to.
This comment reads oddly.

https://codereview.chromium.org/133273029/diff/150001/chrome/browser/translat...
chrome/browser/translate/translate_tab_helper.h:26: ContentTranslateDriver*
GetTranslateDriver();
I assume that TranslateTabHelper will get merged with TranslateDelegateImpl in
the medium-term?

https://codereview.chromium.org/133273029/diff/150001/chrome/chrome_browser.gypi
File chrome/chrome_browser.gypi (right):

https://codereview.chromium.org/133273029/diff/150001/chrome/chrome_browser.g...
chrome/chrome_browser.gypi:36:
'../components/components.gyp:translate_content_browser',
This should be !iOS.

https://codereview.chromium.org/133273029/diff/150001/components/translate.gypi
File components/translate.gypi (right):

https://codereview.chromium.org/133273029/diff/150001/components/translate.gy...
components/translate.gypi:8: 'target_name': 'translate_content_browser',
This should be under a !iOS condition.

[droger, 2014-01-20 10:37:06]

https://codereview.chromium.org/133273029/diff/150001/chrome/browser/translat...
File chrome/browser/translate/translate_tab_helper.h (right):

https://codereview.chromium.org/133273029/diff/150001/chrome/browser/translat...
chrome/browser/translate/translate_tab_helper.h:22: // Information about the
language the page is in and has been translated to.
On 2014/01/17 16:37:36, blundell wrote:
> This comment reads oddly.

This line was in the original code (line 40 on the left).
Removed.

https://codereview.chromium.org/133273029/diff/150001/chrome/browser/translat...
chrome/browser/translate/translate_tab_helper.h:26: ContentTranslateDriver*
GetTranslateDriver();
On 2014/01/17 16:37:36, blundell wrote:
> I assume that TranslateTabHelper will get merged with TranslateDelegateImpl in
> the medium-term?

If all goes well yes.
I still need to sort out the ownership issues in TranslateManager before being
100% sure, and that's what I'll do next.

[blundell, 2014-01-20 14:22:53]

LGTM with naming decision deferred to MAD

https://codereview.chromium.org/133273029/diff/300004/chrome/browser/translat...
File chrome/browser/translate/translate_tab_helper.cc (right):

https://codereview.chromium.org/133273029/diff/300004/chrome/browser/translat...
chrome/browser/translate/translate_tab_helper.cc:30: ContentTranslateDriver*
TranslateTabHelper::GetTranslateDriver() {
Why not return this as a reference?

https://codereview.chromium.org/133273029/diff/300004/chrome/browser/translat...
File chrome/browser/translate/translate_tab_helper.h (right):

https://codereview.chromium.org/133273029/diff/300004/chrome/browser/translat...
chrome/browser/translate/translate_tab_helper.h:24: // Returns the content
driver for translate.
// Returns the ContentTranslateDriver instance associated with this WebContents

https://codereview.chromium.org/133273029/diff/300004/components/translate/co...
File components/translate/content/browser/language_state_observer.h (right):

https://codereview.chromium.org/133273029/diff/300004/components/translate/co...
components/translate/content/browser/language_state_observer.h:13: class
LanguageStateObserver {
I think that I would have just have this class be
ContentTranslateDriver::Observer. I'll leave it to MAD to make the final
decision.

https://codereview.chromium.org/133273029/diff/300004/components/translate/co...
File components/translate/core/browser/language_state.h (right):

https://codereview.chromium.org/133273029/diff/300004/components/translate/co...
components/translate/core/browser/language_state.h:69: // Whether the translate
is enabled. This value is supposed to be used for the
The second sentence of this comment should go now I think.

https://codereview.chromium.org/133273029/diff/300004/components/translate/co...
components/translate/core/browser/language_state.h:95: TranslateDriver*
translate_driver_;
Should have a comment, including lifetime expectation.

[MAD, 2014-01-20 16:12:24]

I agree with the observer name/location change... And I have a few extra nits...

Thanks!

BYE
MAD

https://codereview.chromium.org/133273029/diff/300004/components/translate/co...
File components/translate/content/browser/content_translate_driver.cc (right):

https://codereview.chromium.org/133273029/diff/300004/components/translate/co...
components/translate/content/browser/content_translate_driver.cc:29:
language_state_observer_ = observer;
Since this is just a simple mutator, it could also be inlined and use
hacker_style_name() with a set_ prefix.

https://codereview.chromium.org/133273029/diff/300004/components/translate/co...
File components/translate/content/browser/content_translate_driver.h (right):

https://codereview.chromium.org/133273029/diff/300004/components/translate/co...
components/translate/content/browser/content_translate_driver.h:26:
LanguageState& GetLanguageState();
As per Chromium Coding Style:
Simple accessors should generally be the only inline functions. These should be
named unix_hacker_style().

https://codereview.chromium.org/133273029/diff/300004/components/translate/co...
File components/translate/content/browser/language_state_observer.h (right):

https://codereview.chromium.org/133273029/diff/300004/components/translate/co...
components/translate/content/browser/language_state_observer.h:13: class
LanguageStateObserver {
On 2014/01/20 14:22:53, blundell wrote:
> I think that I would have just have this class be
> ContentTranslateDriver::Observer. I'll leave it to MAD to make the final
> decision.

Yes, I agree, since we now set this observer on the driver and not the language
state directly, we might as well move it there.

https://codereview.chromium.org/133273029/diff/300004/components/translate/co...
File components/translate/core/browser/language_state.h (right):

https://codereview.chromium.org/133273029/diff/300004/components/translate/co...
components/translate/core/browser/language_state.h:25:
LanguageState(TranslateDriver* driver);
Why did you remove the "explicit"?

[droger, 2014-01-20 17:05:04]

Thanks for the review, I applied the comments.

https://codereview.chromium.org/133273029/diff/300004/components/translate/co...
File components/translate/core/browser/language_state.h (right):

https://codereview.chromium.org/133273029/diff/300004/components/translate/co...
components/translate/core/browser/language_state.h:25:
LanguageState(TranslateDriver* driver);
On 2014/01/20 16:12:25, MAD wrote:
> Why did you remove the "explicit"?
This was a mistake, explicit is back.

[MAD, 2014-01-20 21:47:35]

LGTM with one final nit...

Thanks again!

BYE
MAD

https://codereview.chromium.org/133273029/diff/540002/components/translate/co...
File components/translate/content/browser/content_translate_driver.h (right):

https://codereview.chromium.org/133273029/diff/540002/components/translate/co...
components/translate/content/browser/content_translate_driver.h:41: // Sets the
LanguageStateObserver. Calling this method is optional.
LanguageStateObserver -> Observer

[droger, 2014-01-21 09:56:51]

+joi as OWNER for:
- DEPS for content additions in
    -- components/translate/content/browser/DEPS
    -- components/translate/content/DEPS
- components/components_test.gyp


+ jochen as OWNER for:
(should be only "trivial" moving and renaming)
- chrome/browser/tab_contents
- chrome/browser/ui

[droger, 2014-01-21 10:01:06]

On 2014/01/21 09:56:51, droger wrote:
> + jochen as OWNER for:
> (should be only "trivial" moving and renaming)
> - chrome/browser/tab_contents
> - chrome/browser/ui

jochen: I missed these also:
- chrome/browser/policy/policy_browsertest.cc
- chrome/browser/extensions/api/tabs/tabs_api.cc

[Jói, 2014-01-21 10:16:38]

My bits LGTM.

[jochen (gone - plz use gerrit), 2014-01-21 10:20:17]

lgtm

[commit-bot: I haz the power, 2014-01-21 10:21:40]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/droger@chromium.org/133273029/1010001

[commit-bot: I haz the power, 2014-01-21 12:38:30]

Change committed as 246023