Remove most content-level dependencies from TranslateManager.

Remove most content-level dependencies from TranslateManager.

This CL introduces new TranslateDriver APIs to abstract calls to WebContents,
NavigationController, and NavigationEntry.

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

[blundell, 2014-04-08 11:37:01]

Dependent on https://codereview.chromium.org/225263017/

[droger, 2014-04-08 12:01:28]

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

https://codereview.chromium.org/228483003/diff/20001/chrome/browser/translate...
chrome/browser/translate/translate_manager.cc:279:
translate_driver_->GetActiveURL().spec());
I'm sure you know this is related to http://crbug.com/360020.
We'll probably change this code in the near future, but I guess there is no harm
in doing this simple change here.

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

https://codereview.chromium.org/228483003/diff/20001/components/translate/con...
components/translate/content/browser/content_translate_driver.cc:107:
content::NavigationEntry* entry = navigation_controller_->GetActiveEntry();
Not sure if we can do something about it, but GetActiveEntry() is deprecated.
GetVisibleEntry() should be used instead.

I am also not quite sure what is the difference between 
navigation_controller_->GetActiveEntry()->GetURL() and
navigation_controller_->GetWebContents()->GetVisibleURL() (which you use a
couple lines below).

Another source of confusion is that it looks like your API mirrors the
NavigationController API, but the implementation actually does not (one of them
goes through webcontents and not the other one).

That said I don't have any solution to propose unfortunately.

[blundell, 2014-04-08 12:46:02]

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

https://codereview.chromium.org/228483003/diff/20001/chrome/browser/translate...
chrome/browser/translate/translate_manager.cc:279:
translate_driver_->GetActiveURL().spec());
I was planning on leaving this code in TranslateManager, having the
TranslateManager and fully construct the report error URL and then pass it to
the client to present to the user. Does that make sense?

On 2014/04/08 12:01:28, droger wrote:
> I'm sure you know this is related to http://crbug.com/360020.
> We'll probably change this code in the near future, but I guess there is no
harm
> in doing this simple change here.

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

https://codereview.chromium.org/228483003/diff/20001/components/translate/con...
components/translate/content/browser/content_translate_driver.cc:107:
content::NavigationEntry* entry = navigation_controller_->GetActiveEntry();
The problem is that GetActiveEntry() and GetVisibleEntry() don't have the same
semantics; I'd prefer to avoid changing semantics as part of these refactorings.

The latter call is the same as
navigation_controller_->GetVisibleEntry()->GetURL() just wrapped up to return an
empty GURL if there's no visible entry, which maybe answers your question about
the parallelism to the NavigationController API.

On 2014/04/08 12:01:28, droger wrote:
> Not sure if we can do something about it, but GetActiveEntry() is deprecated.
> GetVisibleEntry() should be used instead.
> 
> I am also not quite sure what is the difference between 
> navigation_controller_->GetActiveEntry()->GetURL() and
> navigation_controller_->GetWebContents()->GetVisibleURL() (which you use a
> couple lines below).
> 
> Another source of confusion is that it looks like your API mirrors the
> NavigationController API, but the implementation actually does not (one of
them
> goes through webcontents and not the other one).
> 
> That said I don't have any solution to propose unfortunately.

[droger, 2014-04-08 13:01:21]

lgtm

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

https://codereview.chromium.org/228483003/diff/20001/chrome/browser/translate...
chrome/browser/translate/translate_manager.cc:279:
translate_driver_->GetActiveURL().spec());
On 2014/04/08 12:46:02, blundell wrote:
> I was planning on leaving this code in TranslateManager, having the
> TranslateManager and fully construct the report error URL and then pass it to
> the client to present to the user. Does that make sense?
> 

I think this is fine, yes. Initially I had another idea (outlined in the bug),
but it's actually maybe not very good because we'd hit the problem again when
componentizing translate infobars.

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

https://codereview.chromium.org/228483003/diff/20001/components/translate/con...
components/translate/content/browser/content_translate_driver.cc:107:
content::NavigationEntry* entry = navigation_controller_->GetActiveEntry();
On 2014/04/08 12:46:02, blundell wrote:
> The latter call is the same as
> navigation_controller_->GetVisibleEntry()->GetURL() just wrapped up to return
an
> empty GURL if there's no visible entry, which maybe answers your question
about
> the parallelism to the NavigationController API.
> 

Yes it does answer my question. Thanks.

[blundell, 2014-04-08 18:40:52]

The CQ bit was checked by blundell@chromium.org

[commit-bot: I haz the power, 2014-04-08 18:41:08]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/blundell@chromium.org/228483003/20001

[commit-bot: I haz the power, 2014-04-08 22:45:05]

Change committed as 262528