LanguageState should be owned by TranslateManager

LanguageState should be owned by TranslateManager

LanguageState is currently owned by ContentTranslateManager,
but it should be moved under TranslateManager

BUG=345690
TEST=components_unittests --gtest_filter=Translate*
TBR=sky,dconnelly,kalman

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

[Charlie Reis, 2014-05-16 22:26:03]

I'm not an owner for any of these files, and I'm about to disappear on leave. 
Perhaps you can find another reviewer?

[nshaik, 2014-05-16 22:57:42]

adding owners for review as suggested by git cl

[blundell, 2014-05-19 07:52:11]

mad@, blundell@ to CC. droger@ is the best reviewer for this CL.

[droger, 2014-05-19 10:02:32]

Thanks, this is great.

There is one problem though:
there should be no reference to content/ in components/translate/core/.
The dependency on content/ should stay in components/translate/content/.

The reason for this is that the code in components/translate/core/ is shared by
all platforms, but the iOS platform cannot use content/, and thus we put the
code depending on content/ in components/translate/content/ (which is not used
on iOS).
More details here:
http://www.chromium.org/developers/design-documents/layered-components-design

This means that you cannot move the DidNavigate() function, it has to stay in
the driver.

Looking at the code, it seems that the simplest for now would be to simply pass
a pointer TranslateManager* to ContentTranslateDriver (pass it in the
constructor for example), and then use it in DidNavigate() to get the language
state.

[droger, 2014-05-19 11:37:45]

I just noticed that actually DidNavigate() is only called from one place, which
is in 
TranslateTabHelper::DidNavigateAnyFrame().

Then another option, probably even simpler than what I proposed previously,
would be to simply remove TranslateDriver::DidNavigate() and inline it in
TranslateTabHelper::DidNavigateAnyFrame().

[nshaik, 2014-05-20 07:08:10]

Updated as per review comments, please review

[droger, 2014-05-20 07:43:07]

LGTM

Please fix the following before committing though:

https://codereview.chromium.org/290573013/diff/40001/chrome/browser/translate...
File chrome/browser/translate/translate_tab_helper.h (right):

https://codereview.chromium.org/290573013/diff/40001/chrome/browser/translate...
chrome/browser/translate/translate_tab_helper.h:34: class NavigationController;
Is this declaration necessary? I don't see where this is referenced.

https://codereview.chromium.org/290573013/diff/40001/components/translate/con...
File components/translate/content/browser/content_translate_driver.cc (right):

https://codereview.chromium.org/290573013/diff/40001/components/translate/con...
components/translate/content/browser/content_translate_driver.cc:11: #include
"content/public/browser/navigation_details.h"
Maybe you can remove this include.

https://codereview.chromium.org/290573013/diff/40001/components/translate/con...
File components/translate/content/browser/content_translate_driver.h (right):

https://codereview.chromium.org/290573013/diff/40001/components/translate/con...
components/translate/content/browser/content_translate_driver.h:10: #include
"components/translate/core/browser/language_state.h"
You can remove this.

https://codereview.chromium.org/290573013/diff/40001/components/translate/con...
components/translate/content/browser/content_translate_driver.h:13: struct
LoadCommittedDetails;
You can probably remove this.

https://codereview.chromium.org/290573013/diff/40001/components/translate/cor...
File components/translate/core/browser/translate_driver.h (right):

https://codereview.chromium.org/290573013/diff/40001/components/translate/cor...
components/translate/core/browser/translate_driver.h:11: class LanguageState;
You can remove this.

https://codereview.chromium.org/290573013/diff/40001/components/translate/cor...
File components/translate/core/browser/translate_manager.h (right):

https://codereview.chromium.org/290573013/diff/40001/components/translate/cor...
components/translate/core/browser/translate_manager.h:30: }
Remove all these declarations.

[nshaik, 2014-05-20 22:33:52]

https://codereview.chromium.org/290573013/diff/40001/chrome/browser/translate...
File chrome/browser/translate/translate_tab_helper.h (right):

https://codereview.chromium.org/290573013/diff/40001/chrome/browser/translate...
chrome/browser/translate/translate_tab_helper.h:34: class NavigationController;
On 2014/05/20 07:43:07, droger wrote:
> Is this declaration necessary? I don't see where this is referenced.

Done.

https://codereview.chromium.org/290573013/diff/40001/components/translate/con...
File components/translate/content/browser/content_translate_driver.cc (right):

https://codereview.chromium.org/290573013/diff/40001/components/translate/con...
components/translate/content/browser/content_translate_driver.cc:11: #include
"content/public/browser/navigation_details.h"
On 2014/05/20 07:43:07, droger wrote:
> Maybe you can remove this include.

Done.

https://codereview.chromium.org/290573013/diff/40001/components/translate/con...
File components/translate/content/browser/content_translate_driver.h (right):

https://codereview.chromium.org/290573013/diff/40001/components/translate/con...
components/translate/content/browser/content_translate_driver.h:10: #include
"components/translate/core/browser/language_state.h"
On 2014/05/20 07:43:07, droger wrote:
> You can remove this.

Done.

https://codereview.chromium.org/290573013/diff/40001/components/translate/con...
components/translate/content/browser/content_translate_driver.h:13: struct
LoadCommittedDetails;
On 2014/05/20 07:43:07, droger wrote:
> You can probably remove this.

Done.

https://codereview.chromium.org/290573013/diff/40001/components/translate/cor...
File components/translate/core/browser/translate_driver.h (right):

https://codereview.chromium.org/290573013/diff/40001/components/translate/cor...
components/translate/core/browser/translate_driver.h:11: class LanguageState;
On 2014/05/20 07:43:07, droger wrote:
> You can remove this.

Done.

https://codereview.chromium.org/290573013/diff/40001/components/translate/cor...
File components/translate/core/browser/translate_manager.h (right):

https://codereview.chromium.org/290573013/diff/40001/components/translate/cor...
components/translate/core/browser/translate_manager.h:30: }
On 2014/05/20 07:43:07, droger wrote:
> Remove all these declarations.

Done.

[droger, 2014-05-21 07:35:36]

The CQ bit was checked by droger@chromium.org

[commit-bot: I haz the power, 2014-05-21 07:35:57]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/naiem.shaik@gmail.com/290573013/80001

[droger, 2014-05-21 09:13:24]

Thanks!

I've tried checking the commit box, but it failed. You probably need to rebase
your change.

https://codereview.chromium.org/290573013/diff/80001/components/translate/cor...
File components/translate/core/browser/translate_manager.h (right):

https://codereview.chromium.org/290573013/diff/80001/components/translate/cor...
components/translate/core/browser/translate_manager.h:92: LanguageState&
GetLanguageState();
Can you add a comment for this method?

[nshaik, 2014-05-21 15:25:45]

The CQ bit was checked by naiem.shaik@gmail.com

[nshaik, 2014-05-21 15:26:15]

https://codereview.chromium.org/290573013/diff/80001/components/translate/cor...
File components/translate/core/browser/translate_manager.h (right):

https://codereview.chromium.org/290573013/diff/80001/components/translate/cor...
components/translate/core/browser/translate_manager.h:92: LanguageState&
GetLanguageState();
On 2014/05/21 09:13:25, droger wrote:
> Can you add a comment for this method?

Done.

[commit-bot: I haz the power, 2014-05-21 19:57:49]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/naiem.shaik@gmail.com/290573013/100001

[nshaik, 2014-05-22 01:03:54]

On 2014/05/21 19:57:49, I haz the power (commit-bot) wrote:
> CQ is trying da patch. Follow status at
>  https://chromium-status.appspot.com/cq/naiem.shaik@gmail.com/290573013/100001

I made a clean build and it went through fine on my machine.
not sure how this build failure sneaked in. I am trying it on my machine
again..will update the results in a bit

[nshaik, 2014-05-22 01:17:53]

On 2014/05/22 01:03:54, naiem.shaik wrote:
> On 2014/05/21 19:57:49, I haz the power (commit-bot) wrote:
> > CQ is trying da patch. Follow status at
> > 
https://chromium-status.appspot.com/cq/naiem.shaik@gmail.com/290573013/100001
> 
> I made a clean build and it went through fine on my machine.
> not sure how this build failure sneaked in. I am trying it on my machine
> again..will update the results in a bit

These files are not built/included in mac builds. will fix it on a linux machine
and upload the change

[nshaik, 2014-05-22 05:41:22]

The CQ bit was checked by naiem.shaik@gmail.com

[commit-bot: I haz the power, 2014-05-22 05:42:00]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/naiem.shaik@gmail.com/290573013/120001

[commit-bot: I haz the power, 2014-05-22 11:45:19]

FYI, CQ is re-trying this CL (attempt #1).
Please consider checking whether the failures are real,
and report flakes to chrome-troopers@google.com.
The failing builders are:
  win_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_rel/buil...)

[commit-bot: I haz the power, 2014-05-22 17:32:13]

Change committed as 272217

[tfarina, 2014-05-22 17:37:04]

please spell the test suite name in the CL description correctly in future
patches.

it is not unittests, the name of this test suite is components_unittests.

Thanks,

[sadrul, 2014-05-22 19:17:23]

Reverted in crrev.com/272264 because it caused redness in asan/lsan bots. The
revert CL includes some traces from the failure.

Links to the failed runs:

http://build.chromium.org/p/chromium.memory/builders/Linux%20ASan%20LSan%20Te...
http://build.chromium.org/p/chromium.memory/builders/Linux%20ASan%20LSan%20Te...
http://build.chromium.org/p/chromium.memory/builders/Linux%20ASan%20LSan%20Te...
http://build.chromium.org/p/chromium.memory/builders/Linux%20ASan%20Tests%20%...

[nshaik, 2014-05-22 21:17:09]

On 2014/05/22 19:17:23, sadrul wrote:
> Reverted in crrev.com/272264 because it caused redness in asan/lsan bots. The
> revert CL includes some traces from the failure.
> 
> Links to the failed runs:
> 
>
http://build.chromium.org/p/chromium.memory/builders/Linux%20ASan%20LSan%20Te...
>
http://build.chromium.org/p/chromium.memory/builders/Linux%20ASan%20LSan%20Te...
>
http://build.chromium.org/p/chromium.memory/builders/Linux%20ASan%20LSan%20Te...
>
http://build.chromium.org/p/chromium.memory/builders/Linux%20ASan%20Tests%20%...

I will start looking into the errors..

[nshaik, 2014-05-30 01:10:22]

On 2014/05/22 21:17:09, naiem.shaik wrote:
> On 2014/05/22 19:17:23, sadrul wrote:
> > Reverted in crrev.com/272264 because it caused redness in asan/lsan bots.
The
> > revert CL includes some traces from the failure.
> > 
> > Links to the failed runs:
> > 
> >
>
http://build.chromium.org/p/chromium.memory/builders/Linux%20ASan%20LSan%20Te...
> >
>
http://build.chromium.org/p/chromium.memory/builders/Linux%20ASan%20LSan%20Te...
> >
>
http://build.chromium.org/p/chromium.memory/builders/Linux%20ASan%20LSan%20Te...
> >
>
http://build.chromium.org/p/chromium.memory/builders/Linux%20ASan%20Tests%20%...
> 
> I will start looking into the errors..

The problem is, in TranslateTabHelper::WebContentsDestroyed(), TranslateManager
is reset.
Because, LanguageState is owned by the TranslateManager even LanguageState is
destroyed and then a call is made to  set_translation_declined.
In case of ContentTranslateDriver owning the LanguageState this issue does not
come up as it is still valid.

I was wondering if we have check for translate_manager_.get() before accessing
LanguageState is right fix for this.
The other fix would be not to call the reset on TranslateManager, what are the
other issues we need to deal with.

[blundell, 2014-05-30 09:25:38]

On 2014/05/30 01:10:22, naiem.shaik wrote:
> On 2014/05/22 21:17:09, naiem.shaik wrote:
> > On 2014/05/22 19:17:23, sadrul wrote:
> > > Reverted in crrev.com/272264 because it caused redness in asan/lsan bots.
> The
> > > revert CL includes some traces from the failure.
> > > 
> > > Links to the failed runs:
> > > 
> > >
> >
>
http://build.chromium.org/p/chromium.memory/builders/Linux%20ASan%20LSan%20Te...
> > >
> >
>
http://build.chromium.org/p/chromium.memory/builders/Linux%20ASan%20LSan%20Te...
> > >
> >
>
http://build.chromium.org/p/chromium.memory/builders/Linux%20ASan%20LSan%20Te...
> > >
> >
>
http://build.chromium.org/p/chromium.memory/builders/Linux%20ASan%20Tests%20%...
> > 
> > I will start looking into the errors..
> 
> The problem is, in TranslateTabHelper::WebContentsDestroyed(),
TranslateManager
> is reset.
> Because, LanguageState is owned by the TranslateManager even LanguageState is
> destroyed and then a call is made to  set_translation_declined.
> In case of ContentTranslateDriver owning the LanguageState this issue does not
> come up as it is still valid.
> 
> I was wondering if we have check for translate_manager_.get() before accessing
> LanguageState is right fix for this.
> The other fix would be not to call the reset on TranslateManager, what are the
> other issues we need to deal with.

Thanks for the investigation! David is the best person to answer this question.
He's OOO today but will be back on Monday.

[droger, 2014-06-02 08:51:28]

On 2014/05/30 01:10:22, naiem.shaik wrote:
> On 2014/05/22 21:17:09, naiem.shaik wrote:
> > On 2014/05/22 19:17:23, sadrul wrote:
> > > Reverted in crrev.com/272264 because it caused redness in asan/lsan bots.
> The
> > > revert CL includes some traces from the failure.
> > > 
> > > Links to the failed runs:
> > > 
> > >
> >
>
http://build.chromium.org/p/chromium.memory/builders/Linux%20ASan%20LSan%20Te...
> > >
> >
>
http://build.chromium.org/p/chromium.memory/builders/Linux%20ASan%20LSan%20Te...
> > >
> >
>
http://build.chromium.org/p/chromium.memory/builders/Linux%20ASan%20LSan%20Te...
> > >
> >
>
http://build.chromium.org/p/chromium.memory/builders/Linux%20ASan%20Tests%20%...
> > 
> > I will start looking into the errors..
> 
> The problem is, in TranslateTabHelper::WebContentsDestroyed(),
TranslateManager
> is reset.
> Because, LanguageState is owned by the TranslateManager even LanguageState is
> destroyed and then a call is made to  set_translation_declined.
> In case of ContentTranslateDriver owning the LanguageState this issue does not
> come up as it is still valid.
> 
> I was wondering if we have check for translate_manager_.get() before accessing
> LanguageState is right fix for this.
> The other fix would be not to call the reset on TranslateManager, what are the
> other issues we need to deal with.

I agree with your analysis. The language state is destroyed earlier than it was
previously, which causes the crash.

This part of the code is a bit tricky because the UI elements (such as
TranslateUIDelegate and TranslateInfobarDelegate) are owned by the infobar
service, and thus don't have the same lifetime as the TranslateManager.
You cannot simply check for translate_manager_ being NULL, since the pointer is
not currently reset when the translate manager is destroyed.

I think the best approach here is to make translate_manager_ a WeakPtr. That way
you can effectively check if it has been destroyed before calling it.
the declaration goes like this:
base::WeakPtr<TranslateManager> translate_manager_;
and then you can check if it is still valid:
if (!translate_manager_)
  // handle the case gracefully

blundell actually already spotted that there was a problem in
TranslateUIDelegate, see his comment here:
https://codereview.chromium.org/291503008/diff/40001/chrome/browser/translate...

I didn't follow up on this at that time, sorry.

[nshaik, 2014-06-05 06:56:16]

Made TranslateManager as WeakPtr, please review..

Thanks.

[droger, 2014-06-05 08:36:34]

Thanks. LGTM.

https://codereview.chromium.org/290573013/diff/150001/components/translate/co...
File components/translate/core/browser/translate_ui_delegate.cc (right):

https://codereview.chromium.org/290573013/diff/150001/components/translate/co...
components/translate/core/browser/translate_ui_delegate.cc:167: if
(translate_manager_) {
Note:
if translate_manager_ is NULL here, we will still update the preferences and the
UMA metrics.
I think this is fine actually, and in any case it should not happen in practice
since the previous code did not crash here.

Same comment for RevertTranslation() below.

https://codereview.chromium.org/290573013/diff/150001/components/translate/co...
File components/translate/core/browser/translate_ui_delegate.h (right):

https://codereview.chromium.org/290573013/diff/150001/components/translate/co...
components/translate/core/browser/translate_ui_delegate.h:29:
TranslateUIDelegate(TranslateClient* translate_client,
Optional cleanup while you are here:
1) remove the translate_client parameter, and get it from the manager (there is
a translate_client() method on the manager).
2) remove the translate_client_ member, it is not actually used.

[nshaik, 2014-06-05 18:45:31]

Please take a look..


Thanks

https://codereview.chromium.org/290573013/diff/150001/components/translate/co...
File components/translate/core/browser/translate_ui_delegate.cc (right):

https://codereview.chromium.org/290573013/diff/150001/components/translate/co...
components/translate/core/browser/translate_ui_delegate.cc:167: if
(translate_manager_) {
On 2014/06/05 08:36:35, droger wrote:
> Note:
> if translate_manager_ is NULL here, we will still update the preferences and
the
> UMA metrics.
> I think this is fine actually, and in any case it should not happen in
practice
> since the previous code did not crash here.
> 
> Same comment for RevertTranslation() below.

Done.

https://codereview.chromium.org/290573013/diff/150001/components/translate/co...
File components/translate/core/browser/translate_ui_delegate.h (right):

https://codereview.chromium.org/290573013/diff/150001/components/translate/co...
components/translate/core/browser/translate_ui_delegate.h:29:
TranslateUIDelegate(TranslateClient* translate_client,
On 2014/06/05 08:36:35, droger wrote:
> Optional cleanup while you are here:
> 1) remove the translate_client parameter, and get it from the manager (there
is
> a translate_client() method on the manager).
> 2) remove the translate_client_ member, it is not actually used.

Done.

[droger, 2014-06-05 20:55:43]

lgtm again

[nshaik, 2014-06-05 21:47:28]

The CQ bit was checked by naiem.shaik@gmail.com

[commit-bot: I haz the power, 2014-06-05 21:48:11]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/naiem.shaik@gmail.com/290573013/180001

[commit-bot: I haz the power, 2014-06-06 02:58:36]

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

[commit-bot: I haz the power, 2014-06-06 03:02:59]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-06-06 03:03:00]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/chromium_presubmit/bu...)

[nshaik, 2014-06-06 03:39:40]

"+ben@,mad@ for OWNERS"

[blundell, 2014-06-06 07:31:38]

On 2014/06/06 03:39:40, naiem.shaik wrote:
> "+ben@,mad@ for OWNERS"

Please specify what you're asking them to review. Thanks!

[nshaik, 2014-06-06 16:17:42]

On 2014/06/06 07:31:38, blundell wrote:
> On 2014/06/06 03:39:40, naiem.shaik wrote:
> > "+ben@,mad@ for OWNERS"
> 
> Please specify what you're asking them to review. Thanks!

Need OWNERS approval for these files:
    chrome/browser/extensions/api/tabs/tabs_api.cc
    chrome/browser/policy/policy_browsertest.cc
    chrome/browser/ui/browser_browsertest.cc
    chrome/browser/ui/browser_commands.cc
    chrome/browser/ui/cocoa/browser_window_cocoa.mm
    chrome/browser/ui/cocoa/browser_window_controller.mm
    chrome/browser/ui/views/frame/browser_view.cc
    chrome/browser/ui/views/location_bar/location_bar_view.cc
    chrome/browser/ui/views/translate/translate_bubble_view.cc
...
Can I ask some one to TBR instead of asking each file owner?

[tfarina, 2014-06-06 16:51:36]

On Fri, Jun 6, 2014 at 1:17 PM, <naiem.shaik@gmail.com> wrote:

> On 2014/06/06 07:31:38, blundell wrote:
>
>> On 2014/06/06 03:39:40, naiem.shaik wrote:
>> > "+ben@,mad@ for OWNERS"
>>
>
>  Please specify what you're asking them to review. Thanks!
>>
>
> Need OWNERS approval for these files:
>     chrome/browser/extensions/api/tabs/tabs_api.cc
>     chrome/browser/policy/policy_browsertest.cc
>     chrome/browser/ui/browser_browsertest.cc
>     chrome/browser/ui/browser_commands.cc
>     chrome/browser/ui/cocoa/browser_window_cocoa.mm
>     chrome/browser/ui/cocoa/browser_window_controller.mm
>     chrome/browser/ui/views/frame/browser_view.cc
>     chrome/browser/ui/views/location_bar/location_bar_view.cc
>     chrome/browser/ui/views/translate/translate_bubble_view.cc
>
If changes are non trivial you need to ask an OWNER of those directories to
review them. For example, for c/b/ui/views and specifically for those files
either Peter (pkasting) or Scott (sky) are good reviewers for them.

I didn't look at your changes though.


> ...
> Can I ask some one to TBR instead of asking each file owner?
>
> Use that with care, I'd let your reviewers (David or Colin) take that
decision, until you reach the commit bar.

Thanks,


> https://codereview.chromium.org/290573013/
>

-- 
Thiago Farina

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[droger, 2014-06-06 20:46:26]

This changes are fine to TBR: all the changes outside of translate are either
changes in #includes or, in one case, a minor update to a call site.

To TBR someone, the process is:
- update the CL description to add a line TBR=
- add the person the reviewer list (otherwise they won't get an email, and won't
know that they actually need to review it)
- post a comment in the CL explaining which file the TBR'ed person has to
review.

You can then commit the CL.

[nshaik, 2014-06-07 05:30:47]

Thanks Thiago,David.

@Sky, can you please TBR the changes.

[nshaik, 2014-06-07 17:30:43]

On 2014/06/07 05:30:47, naiem.shaik wrote:
> Thanks Thiago,David.
> 
> @Sky, can you please TBR the changes.

TBR=sky for //chrome/browser/ui

TBR=dconnelly for //chrome/browser/policy

TBR=kalman for chrome/browser/extensions/

[nshaik, 2014-06-10 01:14:43]

The CQ bit was checked by naiem.shaik@gmail.com

[nshaik, 2014-06-10 01:15:05]

The CQ bit was unchecked by naiem.shaik@gmail.com

[dconnelly, 2014-06-10 07:35:33]

lgtm

chrome/browser/policy lgtm

[blundell, 2014-06-10 08:10:58]

On 2014/06/07 17:30:43, naiem.shaik wrote:
> On 2014/06/07 05:30:47, naiem.shaik wrote:
> > Thanks Thiago,David.
> > 
> > @Sky, can you please TBR the changes.
> 
> TBR=sky for //chrome/browser/ui
> 
> TBR=dconnelly for //chrome/browser/policy
> 
> TBR=kalman for chrome/browser/extensions/

Note that you need to add dconnelly and kalman in the TBR= line (and you don't
need to add me or droger since you're not actually TBR'ing us). Thanks!

[nshaik, 2014-06-10 23:30:36]

The CQ bit was checked by naiem.shaik@gmail.com

[commit-bot: I haz the power, 2014-06-10 23:32:57]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/naiem.shaik@gmail.com/290573013/200001

[commit-bot: I haz the power, 2014-06-11 09:34:32]

Change committed as 276320