TranslateManager is no longer a singleton

TranslateManager is no longer a singleton

There is now a TranslateManager per tab.
TranslateManager is owned by TranslateTabHelper.

This changes simplifies all the tracking of WebContents objects,
since there is now one and only one WebContents per TranslateManager.
This allows to get rid of all the |web_contents| parameters as well as the
PendingRequest type.

Users of the TranslateManager now get a manager from a WebContents
object, through the associated TranslateTabHelper.

BUG=332736
TBR=brettw

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

[droger, 2014-02-17 13:18:15]

TranslateManager is owned directly by TranslateTabHelper (aka
ChromeTranslateClient), and not by the driver as we discussed.

The reason is that TranslateManager still depends on Chrome, and the driver is
only in the component.

[blundell, 2014-02-17 15:26:28]

TranslateTabHelper::GetWebContents() can return NULL (when the WebContents is
going away), so you either need to be checking for NULL or convincing yourself
that your methods can never be called when the WebContents is going away and
DCHECKING that the WebContents isn't NULL.

The changes in TranslateManager seem quite subtle...is there any way to break
this CL up into smaller pieces, or is that not possible?

https://codereview.chromium.org/166963002/diff/80001/chrome/browser/translate...
File chrome/browser/translate/translate_manager.cc (left):

https://codereview.chromium.org/166963002/diff/80001/chrome/browser/translate...
chrome/browser/translate/translate_manager.cc:498: if (!translate_tab_helper)
I think you should keep this check, checking against web_contents at the top of
the method.

https://codereview.chromium.org/166963002/diff/80001/chrome/browser/translate...
chrome/browser/translate/translate_manager.cc:518: DCHECK(translate_tab_helper);
You can change this to DCHECK(translate_tab_helper_->GetWebContents())

https://codereview.chromium.org/166963002/diff/80001/chrome/browser/translate...
File chrome/browser/translate/translate_manager.cc (right):

https://codereview.chromium.org/166963002/diff/80001/chrome/browser/translate...
chrome/browser/translate/translate_manager.cc:496: WebContents* web_contents =
translate_tab_helper_->GetWebContents();
This can return NULL, so you need to keep the check that was there before.

https://codereview.chromium.org/166963002/diff/80001/chrome/browser/translate...
File chrome/browser/translate/translate_manager.h (right):

https://codereview.chromium.org/166963002/diff/80001/chrome/browser/translate...
chrome/browser/translate/translate_manager.h:32: // It is a singleton.
Kill

https://codereview.chromium.org/166963002/diff/80001/chrome/browser/translate...
File chrome/browser/translate/translate_tab_helper.cc (right):

https://codereview.chromium.org/166963002/diff/80001/chrome/browser/translate...
chrome/browser/translate/translate_tab_helper.cc:61: return
FromWebContents(web_contents)->GetTranslateManager();
This should short-circuit out if the TranslateTabHelper is NULL.

https://codereview.chromium.org/166963002/diff/80001/chrome/browser/translate...
chrome/browser/translate/translate_tab_helper.cc:69: return
content::WebContentsObserver::web_contents();
Can't this body just be web_contents()?

[droger, 2014-02-17 16:52:48]

I applied your comments, and as discussed offline added back the existing checks
where applicable.
I'll do a followup CL to remove the unnecessary checks.

I also now delete the TranslateManager when WebContentsDestroyed is called.

https://codereview.chromium.org/166963002/diff/80001/chrome/browser/translate...
File chrome/browser/translate/translate_manager.cc (left):

https://codereview.chromium.org/166963002/diff/80001/chrome/browser/translate...
chrome/browser/translate/translate_manager.cc:498: if (!translate_tab_helper)
On 2014/02/17 15:26:28, blundell wrote:
> I think you should keep this check, checking against web_contents at the top
of
> the method.

Done.

https://codereview.chromium.org/166963002/diff/80001/chrome/browser/translate...
chrome/browser/translate/translate_manager.cc:518: DCHECK(translate_tab_helper);
On 2014/02/17 15:26:28, blundell wrote:
> You can change this to DCHECK(translate_tab_helper_->GetWebContents())

Done.

https://codereview.chromium.org/166963002/diff/80001/chrome/browser/translate...
File chrome/browser/translate/translate_manager.cc (right):

https://codereview.chromium.org/166963002/diff/80001/chrome/browser/translate...
chrome/browser/translate/translate_manager.cc:496: WebContents* web_contents =
translate_tab_helper_->GetWebContents();
On 2014/02/17 15:26:28, blundell wrote:
> This can return NULL, so you need to keep the check that was there before.

I changed TranslateTabHelper so that in theory this can never be NULL
(translate_manager_ is deleted when OnWebContentsDestroyed is called).
I nevertheless added the check as you suggested, and we'll maybe remove it
later.

https://codereview.chromium.org/166963002/diff/80001/chrome/browser/translate...
File chrome/browser/translate/translate_tab_helper.cc (right):

https://codereview.chromium.org/166963002/diff/80001/chrome/browser/translate...
chrome/browser/translate/translate_tab_helper.cc:61: return
FromWebContents(web_contents)->GetTranslateManager();
On 2014/02/17 15:26:28, blundell wrote:
> This should short-circuit out if the TranslateTabHelper is NULL.

Done.

https://codereview.chromium.org/166963002/diff/80001/chrome/browser/translate...
chrome/browser/translate/translate_tab_helper.cc:69: return
content::WebContentsObserver::web_contents();
On 2014/02/17 15:26:28, blundell wrote:
> Can't this body just be web_contents()?

Done.

[blundell, 2014-02-18 09:21:46]

LGTM with a bunch of nits. Thanks!

https://codereview.chromium.org/166963002/diff/320001/chrome/browser/translat...
File chrome/browser/translate/translate_infobar_delegate.cc (right):

https://codereview.chromium.org/166963002/diff/320001/chrome/browser/translat...
chrome/browser/translate/translate_infobar_delegate.cc:134:
TranslateTabHelper::GetManagerFromWebContents(web_contents())
You should either DCHECK the manager here or short-circuit out if it's NULL - I
don't know which makes sense offhand.

https://codereview.chromium.org/166963002/diff/320001/chrome/browser/translat...
chrome/browser/translate/translate_infobar_delegate.cc:253:
TranslateTabHelper::GetManagerFromWebContents(web_contents())
Same question as above.

https://codereview.chromium.org/166963002/diff/320001/chrome/browser/translat...
File chrome/browser/translate/translate_manager.cc (right):

https://codereview.chromium.org/166963002/diff/320001/chrome/browser/translat...
chrome/browser/translate/translate_manager.cc:177: // We may get this
notifications multiple times.  Make sure to translate
Orthogonal to your CL, but this comment seems misplaced: isn't it about the
logic below this check?

https://codereview.chromium.org/166963002/diff/320001/chrome/browser/translat...
chrome/browser/translate/translate_manager.cc:365: if
((web_contents->IsLoading()) && attempt < kMaxTranslateLoadCheckAttempts) {
I see, this is why you changed the comment on the header. It seems that
something like "Initiates translation once the page is finished loading" would
make sense?

https://codereview.chromium.org/166963002/diff/320001/chrome/browser/translat...
chrome/browser/translate/translate_manager.cc:476: if (!web_contents)
This should be above the check that starts on 470.

https://codereview.chromium.org/166963002/diff/320001/chrome/browser/translat...
File chrome/browser/translate/translate_manager.h (right):

https://codereview.chromium.org/166963002/diff/320001/chrome/browser/translat...
chrome/browser/translate/translate_manager.h:60: // Reverts the contents of the
page to its original
nit: Can fit on one line.

https://codereview.chromium.org/166963002/diff/320001/chrome/browser/translat...
chrome/browser/translate/translate_manager.h:93: // If the tab is loading, this
does nothing, otherwise it calls
Is this the right comment change? It seems to be saying something different from
the old comment.

https://codereview.chromium.org/166963002/diff/320001/chrome/browser/translat...
chrome/browser/translate/translate_manager.h:97: // Sends a translation request.
This comment is not very clear at this point.

https://codereview.chromium.org/166963002/diff/320001/chrome/browser/translat...
chrome/browser/translate/translate_manager.h:105: void
OnTranslateScriptFetchComplete(int page_id,
Could you add a comment to this method?

https://codereview.chromium.org/166963002/diff/320001/chrome/browser/translat...
File chrome/browser/translate/translate_manager_browsertest.cc (right):

https://codereview.chromium.org/166963002/diff/320001/chrome/browser/translat...
chrome/browser/translate/translate_manager_browsertest.cc:234: // TODO: update
comment.
Address this :).

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

https://codereview.chromium.org/166963002/diff/320001/chrome/browser/translat...
chrome/browser/translate/translate_tab_helper.cc:80: if (error_type !=
TranslateErrors::NONE)
Add a DCHECK that web_contents() isn't NULL at the top

https://codereview.chromium.org/166963002/diff/320001/chrome/browser/translat...
chrome/browser/translate/translate_tab_helper.cc:129:
translate_manager_.reset();
Add a comment on why you're destroying the TranslateManager here.

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

https://codereview.chromium.org/166963002/diff/320001/chrome/browser/translat...
chrome/browser/translate/translate_tab_helper.h:50: // |web_contents|.
, or NULL if there is no such associated instance.

https://codereview.chromium.org/166963002/diff/320001/chrome/browser/translat...
chrome/browser/translate/translate_tab_helper.h:57: // Gets the associated
WebContents.
Returns NULL if the WebContents is being destroyed.

https://codereview.chromium.org/166963002/diff/320001/chrome/browser/translat...
File chrome/browser/translate/translate_ui_delegate.cc (right):

https://codereview.chromium.org/166963002/diff/320001/chrome/browser/translat...
chrome/browser/translate/translate_ui_delegate.cc:161:
TranslateTabHelper::GetManagerFromWebContents(web_contents());
DCHECK(manager) here and below

[droger, 2014-02-18 10:16:51]

Thanks for the review.

https://codereview.chromium.org/166963002/diff/320001/chrome/browser/translat...
File chrome/browser/translate/translate_infobar_delegate.cc (right):

https://codereview.chromium.org/166963002/diff/320001/chrome/browser/translat...
chrome/browser/translate/translate_infobar_delegate.cc:134:
TranslateTabHelper::GetManagerFromWebContents(web_contents())
On 2014/02/18 09:21:46, blundell wrote:
> You should either DCHECK the manager here or short-circuit out if it's NULL -
I
> don't know which makes sense offhand.

I'm doing a short-circuit here (because ReportLanguageDetectionError() has some
internal checks that seem to imply that it can be called when the tab is
destroyed).

https://codereview.chromium.org/166963002/diff/320001/chrome/browser/translat...
chrome/browser/translate/translate_infobar_delegate.cc:253:
TranslateTabHelper::GetManagerFromWebContents(web_contents())
On 2014/02/18 09:21:46, blundell wrote:
> Same question as above.

I'm DCHECKing here, because TranslatePage() assumes that the WebContents are not
NULL. This should imply that the TranslateManager should always exist here.

https://codereview.chromium.org/166963002/diff/320001/chrome/browser/translat...
File chrome/browser/translate/translate_manager.cc (right):

https://codereview.chromium.org/166963002/diff/320001/chrome/browser/translat...
chrome/browser/translate/translate_manager.cc:177: // We may get this
notifications multiple times.  Make sure to translate
On 2014/02/18 09:21:46, blundell wrote:
> Orthogonal to your CL, but this comment seems misplaced: isn't it about the
> logic below this check?

Done.

https://codereview.chromium.org/166963002/diff/320001/chrome/browser/translat...
chrome/browser/translate/translate_manager.cc:365: if
((web_contents->IsLoading()) && attempt < kMaxTranslateLoadCheckAttempts) {
On 2014/02/18 09:21:46, blundell wrote:
> I see, this is why you changed the comment on the header. It seems that
> something like "Initiates translation once the page is finished loading" would
> make sense?

Yes, Done.

https://codereview.chromium.org/166963002/diff/320001/chrome/browser/translat...
File chrome/browser/translate/translate_manager_browsertest.cc (right):

https://codereview.chromium.org/166963002/diff/320001/chrome/browser/translat...
chrome/browser/translate/translate_manager_browsertest.cc:234: // TODO: update
comment.
On 2014/02/18 09:21:46, blundell wrote:
> Address this :).

Ooops. Done.

[droger, 2014-02-18 10:54:17]

+mad

[MAD, 2014-02-18 20:29:12]

Très cool... Very well done! Thanks again...

A few minor comments/questions/nits...

BYE
MAD

https://codereview.chromium.org/166963002/diff/430002/chrome/browser/translat...
File chrome/browser/translate/translate_infobar_delegate.cc (right):

https://codereview.chromium.org/166963002/diff/430002/chrome/browser/translat...
chrome/browser/translate/translate_infobar_delegate.cc:136: if (!manager)
Why an if here and DCHECK elsewhere?

https://codereview.chromium.org/166963002/diff/430002/chrome/browser/translat...
File chrome/browser/translate/translate_manager.h (right):

https://codereview.chromium.org/166963002/diff/430002/chrome/browser/translat...
chrome/browser/translate/translate_manager.h:117: TranslateTabHelper*
translate_tab_helper_;  // Weak
Please add a '.' at the end of the comment.

https://codereview.chromium.org/166963002/diff/430002/chrome/browser/translat...
File chrome/browser/translate/translate_service.cc (right):

https://codereview.chromium.org/166963002/diff/430002/chrome/browser/translat...
chrome/browser/translate/translate_service.cc:9: #include
"chrome/browser/translate/translate_manager.h"
Do we still need this include?

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

https://codereview.chromium.org/166963002/diff/430002/chrome/browser/translat...
chrome/browser/translate/translate_tab_helper.cc:131: // TODO(droger):
Destroying the TranslateManager now guarantees that it never
This comment doesn't sound like a TODO... What is there left to be done?

[droger, 2014-02-19 09:17:28]

https://codereview.chromium.org/166963002/diff/430002/chrome/browser/translat...
File chrome/browser/translate/translate_infobar_delegate.cc (right):

https://codereview.chromium.org/166963002/diff/430002/chrome/browser/translat...
chrome/browser/translate/translate_infobar_delegate.cc:136: if (!manager)
On 2014/02/18 20:29:12, MAD wrote:
> Why an if here and DCHECK elsewhere?

The logic I used is the following:

- if the function assumes web_contents cannot be NULL (like TranslatePage()
which would crash in this case), then I assume that the page necessarily still
exists and I that I can always get the TranslateManager from it. I put a DCHECK
in that case.

- For ReportLanguageDetectionError, I did differently because the function
gracefully handles the case where
chrome::FindBrowserWithWebContents(web_contents) returns NULL. I think that
might mean that web_contents can be NULL here, and manager would be NULL in this
case. That's why I explicitly handle this case.

https://codereview.chromium.org/166963002/diff/430002/chrome/browser/translat...
File chrome/browser/translate/translate_manager.h (right):

https://codereview.chromium.org/166963002/diff/430002/chrome/browser/translat...
chrome/browser/translate/translate_manager.h:117: TranslateTabHelper*
translate_tab_helper_;  // Weak
On 2014/02/18 20:29:12, MAD wrote:
> Please add a '.' at the end of the comment.

Done.

https://codereview.chromium.org/166963002/diff/430002/chrome/browser/translat...
File chrome/browser/translate/translate_service.cc (right):

https://codereview.chromium.org/166963002/diff/430002/chrome/browser/translat...
chrome/browser/translate/translate_service.cc:9: #include
"chrome/browser/translate/translate_manager.h"
On 2014/02/18 20:29:12, MAD wrote:
> Do we still need this include?

Removed.

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

https://codereview.chromium.org/166963002/diff/430002/chrome/browser/translat...
chrome/browser/translate/translate_tab_helper.cc:131: // TODO(droger):
Destroying the TranslateManager now guarantees that it never
On 2014/02/18 20:29:12, MAD wrote:
> This comment doesn't sound like a TODO... What is there left to be done?

I plan to do a small follow up CL to remove a few of cases in TranslateManager
where we test if web_contents() is NULL. These tests should be unnecessary,
since the TranslateManager should be destroyed as soon as web_contents() become
NULL. I prefer doing this in a follow-up CL though rather than here.

[MAD, 2014-02-19 16:22:07]

OK, LGTM, conditional to updating the TODO comment text.

Thanks!

BYE
MAD

https://codereview.chromium.org/166963002/diff/430002/chrome/browser/translat...
File chrome/browser/translate/translate_infobar_delegate.cc (right):

https://codereview.chromium.org/166963002/diff/430002/chrome/browser/translat...
chrome/browser/translate/translate_infobar_delegate.cc:136: if (!manager)
OK, good point... I wonder if we should add a comment explaining why manager
could be NULL here?

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

https://codereview.chromium.org/166963002/diff/430002/chrome/browser/translat...
chrome/browser/translate/translate_tab_helper.cc:131: // TODO(droger):
Destroying the TranslateManager now guarantees that it never
Still, the TODO text should be worded in a way that it explains what will need
to be done, not what is currently being done... 

If someone would like to fix this TODO for you, they wouldn't know what... to
do... ;-)

[droger, 2014-02-19 16:36:45]

TBR brettw as OWNER of chrome/browser/tab_contents/render_view_context_menu.cc

[droger, 2014-02-19 16:38:19]

The CQ bit was checked by droger@chromium.org

[droger, 2014-02-19 16:40:08]

https://codereview.chromium.org/166963002/diff/430002/chrome/browser/translat...
File chrome/browser/translate/translate_infobar_delegate.cc (right):

https://codereview.chromium.org/166963002/diff/430002/chrome/browser/translat...
chrome/browser/translate/translate_infobar_delegate.cc:136: if (!manager)
On 2014/02/19 16:22:07, MAD wrote:
> OK, good point... I wonder if we should add a comment explaining why manager
> could be NULL here?

I don't know if this actually happen.
I'll make a try job with some DCHECK here and see if it is hit.

[commit-bot: I haz the power, 2014-02-19 16:41:05]

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

[commit-bot: I haz the power, 2014-02-19 21:18:42]

Change committed as 252098