Move TranslateInfoBarDelegate and OptionsMenuModel to the Translate component.

Move TranslateInfoBarDelegate and OptionsMenuModel to the Translate component.

To solve the dependency on chrome_command_ids, this CL removes the
IDC_OPTIONS_TRANSLATE_* constants and instead defines the command IDs
locally in the file, which is an approach already used at several other places in
the code.

To solve the dependency on themes, the icon ID for the translate infobar is no
longer hardcoded, but fetched from the translate client instead.

BUG=371845
TBR=estade, joaodasilva

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

[blundell, 2014-05-21 15:04:57]

LGTM!

[droger, 2014-05-21 15:41:14]

+pkasting as OWNER of chrome/browser/ui and also for the DEPS change
(components/translate now depends on components/infobars).

+cpu as OWNER of chrome/app/chrome_command_ids.h

[Peter Kasting, 2014-05-21 19:31:09]

The whole translate infobar UI is going away in favor of the new bubble UI, at
least on desktop -- it's already off by default on views, and Mac is in
progress.

I wonder if it really makes sense to try and componentize this, or if it would
be better to try and make the bubble UI work.  Have you already considered this
and talked to the folks who own the bubble UI?

[droger, 2014-05-21 19:41:43]

On 2014/05/21 19:31:09, Peter Kasting wrote:
> The whole translate infobar UI is going away in favor of the new bubble UI, at
> least on desktop -- it's already off by default on views, and Mac is in
> progress.
> 
> I wonder if it really makes sense to try and componentize this, or if it would
> be better to try and make the bubble UI work.  Have you already considered
this
> and talked to the folks who own the bubble UI?

There are no plans for mobile to use bubbles as far as I know.

[Peter Kasting, 2014-05-21 20:38:23]

LGTM

https://codereview.chromium.org/284313008/diff/60001/components/translate/cor...
File components/translate/core/browser/options_menu_model.cc (right):

https://codereview.chromium.org/284313008/diff/60001/components/translate/cor...
components/translate/core/browser/options_menu_model.cc:28: const int
OptionsMenuModel::kOptionsReportBadDetection = 4;
Hmm, looks like maybe these should be an enum rather than separate ints.

https://codereview.chromium.org/284313008/diff/60001/components/translate/cor...
File components/translate/core/browser/options_menu_model.h (right):

https://codereview.chromium.org/284313008/diff/60001/components/translate/cor...
components/translate/core/browser/options_menu_model.h:29: static const int
kOptionsAboutTranslate;
Nit: Style guide says static consts go above all functions (right below typedefs
and enums).

Is it necessary to have "Options" in front of all these constants, when they're
already scoped to the "OptionsMenuModel" class?

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

https://codereview.chromium.org/284313008/diff/60001/components/translate/cor...
components/translate/core/browser/translate_client.h:55: // Returns the resource
ID of the icon to be shown for the Translate infobars.
Nit: I wonder if this should be listed higher up, e.g. by the other Get
functions?  I'm struggling to think of any logical order for these functions :/

https://codereview.chromium.org/284313008/diff/60001/components/translate/cor...
File components/translate/core/browser/translate_infobar_delegate.h (right):

https://codereview.chromium.org/284313008/diff/60001/components/translate/cor...
components/translate/core/browser/translate_infobar_delegate.h:198:
TranslateClient* GetTranslateClient() const;
Can't return a non-const pointer from a const function.  Both should be const,
or neither.

Why is this actually needed, anyway?  For the test?  It's only used once in
translate_infobar_delegate.cc, and there we don't NULL-check, so we might as
well just access translate_manager_->translate_client() directly.

[cpu_(ooo_6.6-7.5), 2014-05-22 00:27:11]

lgtm for chrome_command_ids.h

[droger, 2014-05-22 12:33:36]

Thanks, I applied the comments.

TBR=estade for autofill
TBR=joaodasilva for policy

https://codereview.chromium.org/284313008/diff/60001/components/translate/cor...
File components/translate/core/browser/options_menu_model.cc (right):

https://codereview.chromium.org/284313008/diff/60001/components/translate/cor...
components/translate/core/browser/options_menu_model.cc:28: const int
OptionsMenuModel::kOptionsReportBadDetection = 4;
On 2014/05/21 20:38:24, Peter Kasting wrote:
> Hmm, looks like maybe these should be an enum rather than separate ints.

I agree, the enum was also my first idea.
I used these ints because the other examples I found in the code base were using
ints.
Anyway I'll convert this to an enum, since it seems better.
I'll revert to ints in a follow up if you think that it's better to remain
consistent with the others:

https://code.google.com/p/chromium/codesearch#chromium/src/ui/message_center/...
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ui/...
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ui/...

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

https://codereview.chromium.org/284313008/diff/60001/components/translate/cor...
components/translate/core/browser/translate_client.h:55: // Returns the resource
ID of the icon to be shown for the Translate infobars.
On 2014/05/21 20:38:24, Peter Kasting wrote:
> Nit: I wonder if this should be listed higher up, e.g. by the other Get
> functions?  I'm struggling to think of any logical order for these functions
:/

Done.

https://codereview.chromium.org/284313008/diff/60001/components/translate/cor...
File components/translate/core/browser/translate_infobar_delegate.h (right):

https://codereview.chromium.org/284313008/diff/60001/components/translate/cor...
components/translate/core/browser/translate_infobar_delegate.h:198:
TranslateClient* GetTranslateClient() const;
On 2014/05/21 20:38:24, Peter Kasting wrote:
> Can't return a non-const pointer from a const function.  Both should be const,
> or neither.
> 
> Why is this actually needed, anyway?  For the test?  It's only used once in
> translate_infobar_delegate.cc, and there we don't NULL-check, so we might as
> well just access translate_manager_->translate_client() directly.

Done.

[droger, 2014-05-22 12:33:50]

The CQ bit was checked by droger@chromium.org

[commit-bot: I haz the power, 2014-05-22 12:34:33]

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

[Joao da Silva, 2014-05-22 13:01:51]

policy lgtm

[Peter Kasting, 2014-05-22 17:37:23]

https://codereview.chromium.org/284313008/diff/20002/chrome/browser/ui/androi...
File chrome/browser/ui/android/infobars/infobar_android.cc (left):

https://codereview.chromium.org/284313008/diff/20002/chrome/browser/ui/androi...
chrome/browser/ui/android/infobars/infobar_android.cc:16: 
Nit: Leave two blank lines up here

https://codereview.chromium.org/284313008/diff/20002/components/translate/cor...
File components/translate/core/browser/translate_infobar_delegate.cc (right):

https://codereview.chromium.org/284313008/diff/20002/components/translate/cor...
components/translate/core/browser/translate_infobar_delegate.cc:340:
TranslateClient* TranslateInfoBarDelegate::GetTranslateClient() {
I'm still not clear on where this is actually called.  Is it dead?  Can it be
removed?

[droger, 2014-05-26 12:16:04]

blundell, pkasting: I had to make non-trivial changes to the CL after your
LGTMs, so I'd like another pass before landing this.

There was a compile failure on Android, that I initially overlooked but that was
actually a real architectural issue.

1) the static ints (like kDefaultBarTargetHeight) in infobar.h are declared in
the infobar component but implemented in chrome (in the platform specific code).
This prevents the infobar component from linking. This issue was pre-existing
but didn't manifest until now.

2) TranslateInfoBarDelegate::CreateInfoBar() is similar: it is declared in
TranslateInfoBarDelegate but implemented in platform specific code. This issue
is new in this CL.

To fix 1) I went with a workaround and moved the constants in the component for
android (in infobar_android.cc). This is not a clean solution, but since it is
somewhat unrelated to this CL I didn't want to include more logic here. A clean
fix may be to change these constants to be either non-const globals, or to be
non-static and passed to the infobar as parameters.

To fix 2) I moved the CreateInfoBar() method to the TranslateClient, so that the
interface and the implementation both stay in Chrome.

I can split these changes in another CL if you prefer, as this CL is now quite
large.

https://codereview.chromium.org/284313008/diff/20002/components/translate/cor...
File components/translate/core/browser/translate_infobar_delegate.cc (right):

https://codereview.chromium.org/284313008/diff/20002/components/translate/cor...
components/translate/core/browser/translate_infobar_delegate.cc:340:
TranslateClient* TranslateInfoBarDelegate::GetTranslateClient() {
On 2014/05/22 17:37:24, Peter Kasting wrote:
> I'm still not clear on where this is actually called.  Is it dead?  Can it be
> removed?

It was only called in one place, I removed it.

[blundell, 2014-05-26 12:32:50]

lgtm

[Peter Kasting, 2014-05-28 02:17:27]

I will try to review this tomorrow.

[Peter Kasting, 2014-05-28 19:25:55]

LGTM

https://codereview.chromium.org/284313008/diff/180001/components/translate/co...
File components/translate/core/browser/translate_client.h (right):

https://codereview.chromium.org/284313008/diff/180001/components/translate/co...
components/translate/core/browser/translate_client.h:50:
scoped_ptr<TranslateInfoBarDelegate> delegate) = 0;
Does this really need to be virtual?  It seems like maybe it could be static, if
we're allowed to declare a static function here that we define in chrome/ (maybe
we can't).

If this can't be static, it seems like at least it could be const?

[droger, 2014-06-02 11:30:14]

https://codereview.chromium.org/284313008/diff/180001/components/translate/co...
File components/translate/core/browser/translate_client.h (right):

https://codereview.chromium.org/284313008/diff/180001/components/translate/co...
components/translate/core/browser/translate_client.h:50:
scoped_ptr<TranslateInfoBarDelegate> delegate) = 0;
On 2014/05/28 19:25:55, Peter Kasting wrote:
> Does this really need to be virtual?  It seems like maybe it could be static,
if
> we're allowed to declare a static function here that we define in chrome/
(maybe
> we can't).
> 
> If this can't be static, it seems like at least it could be const?

I made it const.

The component should be standalone, and cannot rely on chrome/ for its
implementation.
It would not link.
Maybe we could work around these issues to make it link -- I'm not sure -- , but
I don't think it is something we want.

[droger, 2014-06-02 11:30:23]

The CQ bit was checked by droger@chromium.org

[commit-bot: I haz the power, 2014-06-02 11:31:16]

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

[blundell, 2014-06-02 11:42:54]

On 2014/06/02 11:30:14, droger wrote:
>
https://codereview.chromium.org/284313008/diff/180001/components/translate/co...
> File components/translate/core/browser/translate_client.h (right):
> 
>
https://codereview.chromium.org/284313008/diff/180001/components/translate/co...
> components/translate/core/browser/translate_client.h:50:
> scoped_ptr<TranslateInfoBarDelegate> delegate) = 0;
> On 2014/05/28 19:25:55, Peter Kasting wrote:
> > Does this really need to be virtual?  It seems like maybe it could be
static,
> if
> > we're allowed to declare a static function here that we define in chrome/
> (maybe
> > we can't).
> > 
> > If this can't be static, it seems like at least it could be const?
> 
> I made it const.
> 
> The component should be standalone, and cannot rely on chrome/ for its
> implementation.
> It would not link.
> Maybe we could work around these issues to make it link -- I'm not sure -- ,
but
> I don't think it is something we want.

Yes, we avoid putting static functions in the component that are intended to be
implemented by the embedder.

[commit-bot: I haz the power, 2014-06-02 13:28:10]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  linux_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_rel/bu...)

[droger, 2014-06-02 14:52:17]

The CQ bit was unchecked by droger@chromium.org

[droger, 2014-06-02 14:52:19]

The CQ bit was checked by droger@chromium.org

[commit-bot: I haz the power, 2014-06-02 14:53:51]

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

[commit-bot: I haz the power, 2014-06-02 16:01:33]

Change committed as 274252