Remove dependency of TranslateInfobarDelegate on chrome/ and content/

Remove dependency of TranslateInfobarDelegate on chrome/ and content/

BUG=371845
TBR=rohitrao

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

[blundell, 2014-05-19 12:38:42]

https://codereview.chromium.org/292513002/diff/1/chrome/browser/translate/opt...
File chrome/browser/translate/options_menu_model.cc (left):

https://codereview.chromium.org/292513002/diff/1/chrome/browser/translate/opt...
chrome/browser/translate/options_menu_model.cc:124: WebContents* web_contents =
translate_infobar_delegate_->GetWebContents();
It seems like it would also be OK for this code to assume that it is an
InfoBarService that owns the delegate, unless this code is also being targeted
for removal from //chrome? That would avoid needing to add the OpenUrlInNewTab()
API.

https://codereview.chromium.org/292513002/diff/1/chrome/browser/translate/opt...
File chrome/browser/translate/options_menu_model.cc (right):

https://codereview.chromium.org/292513002/diff/1/chrome/browser/translate/opt...
chrome/browser/translate/options_menu_model.cc:18: const char
kAboutGoogleTranslateURL[] =
Why did you move this here, out of curiosity?

[droger, 2014-05-19 12:51:03]

On 2014/05/19 12:38:42, blundell wrote:
>
https://codereview.chromium.org/292513002/diff/1/chrome/browser/translate/opt...
> File chrome/browser/translate/options_menu_model.cc (left):
> 
>
https://codereview.chromium.org/292513002/diff/1/chrome/browser/translate/opt...
> chrome/browser/translate/options_menu_model.cc:124: WebContents* web_contents
=
> translate_infobar_delegate_->GetWebContents();
> It seems like it would also be OK for this code to assume that it is an
> InfoBarService that owns the delegate, unless this code is also being targeted
> for removal from //chrome? That would avoid needing to add the
OpenUrlInNewTab()
> API.
> 
>
https://codereview.chromium.org/292513002/diff/1/chrome/browser/translate/opt...
> File chrome/browser/translate/options_menu_model.cc (right):
> 
>
https://codereview.chromium.org/292513002/diff/1/chrome/browser/translate/opt...
> chrome/browser/translate/options_menu_model.cc:18: const char
> kAboutGoogleTranslateURL[] =
> Why did you move this here, out of curiosity?

Initially I thought we'd move OptionsMenuModel to the component, and thus I
tried to remove content/ and chrome/ dependencies.
It may not make sense to move this to the component though, as this file is
mostly creating browser commands, which may never move to the component.
Note that the mechanism for commands seems to be in ui/, which is usable by
components.

[droger, 2014-05-19 14:01:32]

As per offline discussion, I removed the driver method.
Please take a look.

[droger, 2014-05-19 15:49:51]

Sorry about the confusion around browser commands.
I checked that the commands are only usedby OptionsMenuModel internally.

I reverted to the first approach, which prepares for the componentization of
OptionsMenuModel.

[blundell, 2014-05-20 11:56:56]

LGTM

https://codereview.chromium.org/292513002/diff/60001/chrome/browser/translate...
File chrome/browser/translate/options_menu_model.cc (left):

https://codereview.chromium.org/292513002/diff/60001/chrome/browser/translate...
chrome/browser/translate/options_menu_model.cc:25: // |translate_delegate| must
already be owned.
Does it make sense to DCHECK GetTranslateClient() here?

https://codereview.chromium.org/292513002/diff/60001/chrome/browser/translate...
File chrome/browser/translate/options_menu_model.cc (right):

https://codereview.chromium.org/292513002/diff/60001/chrome/browser/translate...
chrome/browser/translate/options_menu_model.cc:128:
translate_infobar_delegate_->GetTranslateClient();
nit: I think it would be cleaner to add
TranslateInfoBarDelegate::GetTranslateDriver().

[droger, 2014-05-20 12:30:23]

https://codereview.chromium.org/292513002/diff/60001/chrome/browser/translate...
File chrome/browser/translate/options_menu_model.cc (left):

https://codereview.chromium.org/292513002/diff/60001/chrome/browser/translate...
chrome/browser/translate/options_menu_model.cc:25: // |translate_delegate| must
already be owned.
On 2014/05/20 11:56:56, blundell wrote:
> Does it make sense to DCHECK GetTranslateClient() here?

I don't know why this test was here.
I guess It does not hurt to keep the DCHECK.
Done.

https://codereview.chromium.org/292513002/diff/60001/chrome/browser/translate...
File chrome/browser/translate/options_menu_model.cc (right):

https://codereview.chromium.org/292513002/diff/60001/chrome/browser/translate...
chrome/browser/translate/options_menu_model.cc:128:
translate_infobar_delegate_->GetTranslateClient();
On 2014/05/20 11:56:56, blundell wrote:
> nit: I think it would be cleaner to add
> TranslateInfoBarDelegate::GetTranslateDriver().

Done.

[droger, 2014-05-20 12:31:38]

TBR=rohitrao for chrome/browser/ui/cocoa/infobars/translate_infobar_unittest.mm

[droger, 2014-05-20 12:31:47]

The CQ bit was checked by droger@chromium.org

[commit-bot: I haz the power, 2014-05-20 12:32:24]

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

[rohitrao (ping after 24h), 2014-05-20 12:35:30]

cocoa/ changes LGTM

[commit-bot: I haz the power, 2014-05-20 17:26:33]

Change committed as 271685