Translate: Refactoring: Create TranslateUIDelegate

Translate: Refactoring: Create TranslateUIDelegate

The TranslateUIDelegate is a generic delegate for UI which offers Translate feature to the user. Currently this is used only by Translate infobar, and is going to be used by the new Translate UX we're implementing now (crbug/276181).

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

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

[hajimehoshi, 2013-09-10 09:03:21]

Can you take a look?  Thank you in advance.

[Takashi Toyoshima, 2013-09-10 09:33:16]

https://chromiumcodereview.appspot.com/23708029/diff/1/chrome/browser/transla...
File chrome/browser/translate/translate_ui_delegate.cc (right):

https://chromiumcodereview.appspot.com/23708029/diff/1/chrome/browser/transla...
chrome/browser/translate/translate_ui_delegate.cc:41: std::vector<std::string>
language_codes;
wrong indent

https://chromiumcodereview.appspot.com/23708029/diff/1/chrome/browser/transla...
chrome/browser/translate/translate_ui_delegate.cc:81: prefs_.reset(new
TranslatePrefs(profile->GetPrefs()));
Maybe we should write a comment at TranslateUIDelegate header file so that this
class assumes Profile object lives while this instance being alive.

https://chromiumcodereview.appspot.com/23708029/diff/1/chrome/browser/transla...
File chrome/browser/translate/translate_ui_delegate.h (right):

https://chromiumcodereview.appspot.com/23708029/diff/1/chrome/browser/transla...
chrome/browser/translate/translate_ui_delegate.h:39: std::string
language_code_at(size_t index) const {
I'm not sure, but having these function as inline functions sounds bad. Now is
the time to move these implementation into .cc file? WDYT?

[hajimehoshi, 2013-09-12 06:16:03]

Thanks.  Can you take a look again?

https://codereview.chromium.org/23708029/diff/1/chrome/browser/translate/tran...
File chrome/browser/translate/translate_ui_delegate.cc (right):

https://codereview.chromium.org/23708029/diff/1/chrome/browser/translate/tran...
chrome/browser/translate/translate_ui_delegate.cc:41: std::vector<std::string>
language_codes;
On 2013/09/10 09:33:16, Takashi Toyoshima (chromium) wrote:
> wrong indent

Done.

https://codereview.chromium.org/23708029/diff/1/chrome/browser/translate/tran...
chrome/browser/translate/translate_ui_delegate.cc:81: prefs_.reset(new
TranslatePrefs(profile->GetPrefs()));
On 2013/09/10 09:33:16, Takashi Toyoshima (chromium) wrote:
> Maybe we should write a comment at TranslateUIDelegate header file so that
this
> class assumes Profile object lives while this instance being alive.

Added a comment to explain the life of this object.

https://codereview.chromium.org/23708029/diff/1/chrome/browser/translate/tran...
File chrome/browser/translate/translate_ui_delegate.h (right):

https://codereview.chromium.org/23708029/diff/1/chrome/browser/translate/tran...
chrome/browser/translate/translate_ui_delegate.h:39: std::string
language_code_at(size_t index) const {
On 2013/09/10 09:33:16, Takashi Toyoshima (chromium) wrote:
> I'm not sure, but having these function as inline functions sounds bad. Now is
> the time to move these implementation into .cc file? WDYT?

I agree.  I moved the implementations of language_code_at, language_name_at,
original_language_index and target_language_index and renamed them according to
the naming rule.

[Takashi Toyoshima, 2013-09-12 07:08:40]

https://codereview.chromium.org/23708029/diff/15001/chrome/browser/translate/...
File chrome/browser/translate/translate_ui_delegate.h (right):

https://codereview.chromium.org/23708029/diff/15001/chrome/browser/translate/...
chrome/browser/translate/translate_ui_delegate.h:25: // while prefs object
lives.
You should not use abbreviation in comments. So prefs should be |prefs_| or
TranslatePrefs. Also the essential difficulty is not TranslatePrefs but Profile
and PrefService. Do you know how their lifetimes are managed?
So now I feel this kind of notice should be at translate_prefs.h.

https://codereview.chromium.org/23708029/diff/15001/chrome/browser/translate/...
chrome/browser/translate/translate_ui_delegate.h:41: void
set_original_language_index(size_t language_index) {
ditto.

https://codereview.chromium.org/23708029/diff/15001/chrome/browser/translate/...
chrome/browser/translate/translate_ui_delegate.h:47: void
set_target_language_index(size_t language_index) {
ditto.

[hajimehoshi, 2013-09-12 07:48:15]

Thanks!

https://codereview.chromium.org/23708029/diff/15001/chrome/browser/translate/...
File chrome/browser/translate/translate_ui_delegate.h (right):

https://codereview.chromium.org/23708029/diff/15001/chrome/browser/translate/...
chrome/browser/translate/translate_ui_delegate.h:25: // while prefs object
lives.
On 2013/09/12 07:08:40, Takashi Toyoshima (chromium) wrote:
> You should not use abbreviation in comments. So prefs should be |prefs_| or
> TranslatePrefs. Also the essential difficulty is not TranslatePrefs but
Profile
> and PrefService. Do you know how their lifetimes are managed?
> So now I feel this kind of notice should be at translate_prefs.h.

I added a comment about the life cycle of TranslatePrefs at translate_prefs.h. 
Thanks.

https://codereview.chromium.org/23708029/diff/15001/chrome/browser/translate/...
chrome/browser/translate/translate_ui_delegate.h:41: void
set_original_language_index(size_t language_index) {
On 2013/09/12 07:08:40, Takashi Toyoshima (chromium) wrote:
> ditto.

Done.

https://codereview.chromium.org/23708029/diff/15001/chrome/browser/translate/...
chrome/browser/translate/translate_ui_delegate.h:47: void
set_target_language_index(size_t language_index) {
On 2013/09/12 07:08:40, Takashi Toyoshima (chromium) wrote:
> ditto.

Done.

[hajimehoshi, 2013-09-13 08:48:48]

> Do you know how their lifetimes are managed?

As for the lifetimes of the objects, PrefService objects must be destroyed after
TranslateInfobarDelegate objects are destroyed.

A PrefService is associated with a Profile or with a local state [1]. A Profile
is owned by ProfileManager, which is owned by a BrowserProcessImpl. A local
state is also owned by a BrowserProcessImpl. ProfileManager is destroyed at
BrowserProcessImpl::StartTearDown.

A TranslateInfobarDelegate is owned by an InfobarService. An InfobarService is a
WebContentsUserData, therefore an InfobarServece's lifetime is same as a
WebContents, which represents one tab.

A Profile object is destroyed when (a) the user removes the profile or (b)
Chromium is shut down, and a local state is destroyed when (b) shutting down. In
the case of (a), BrowserList::CloseAllBrowsersWithProfile(profile) is called
ahead of deleting the profile data [2], therefore, the profile data should live
while the browser lives. In the case of (b), in application_lifetime.cc's
SessionEnding, CloseAllBrowsers is called ahead of
content::ImmediateShutdownAndExitProcess, which finally calls
BrowserProcessImpl::StartTearDown, therefore, the lives of objects are same as
(a).

[hajimehoshi, 2013-09-13 09:15:03]

[1] http://dev.chromium.org/developers/design-documents/preferences
[2] ProfileManager::FinishDeletingProfile.

[Takashi Toyoshima, 2013-10-01 10:58:24]

+miguelg FYI

Miguel:
Since this change is blocking our new feature implementation, we'll land this
change which will conflict with your fix for Android.
Can you share your plan how to fix delegate crash bug for Android? I'm fine if
you want to merge your workaround patch to branches. But for trunk, I still
think we should investigate why it can be NULL since potentially it will cause
another unexpected behavior.

Hajime:
LGTM.

[commit-bot: I haz the power, 2013-10-03 05:41:18]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/hajimehoshi@chromium.org/23708029/44001

[commit-bot: I haz the power, 2013-10-03 08:13:32]

Change committed as 226704

[tapted, 2013-10-03 10:03:11]

https://codereview.chromium.org/23708029/diff/44001/chrome/browser/translate/...
File chrome/browser/translate/translate_infobar_delegate.cc (right):

https://codereview.chromium.org/23708029/diff/44001/chrome/browser/translate/...
chrome/browser/translate/translate_infobar_delegate.cc:35: const size_t
TranslateInfoBarDelegate::kNoIndex = TranslateUIDelegate::kNoIndex;
This change (I think) has managed to add a static initializer, closing the tree.

at
http://build.chromium.org/p/chromium/builders/Linux/builds/43453/steps/sizes/...

  # thread_cache.cc strtoll@plt
> # translate_infobar_delegate.cc __i686.get_pc_thunk.cx
  # unicodetext.cc std::ios_base::Init::Init()@plt

sorry - I think I need to revert this.

[hajimehoshi, 2013-10-03 10:11:07]

https://codereview.chromium.org/23708029/diff/44001/chrome/browser/translate/...
File chrome/browser/translate/translate_infobar_delegate.cc (right):

https://codereview.chromium.org/23708029/diff/44001/chrome/browser/translate/...
chrome/browser/translate/translate_infobar_delegate.cc:35: const size_t
TranslateInfoBarDelegate::kNoIndex = TranslateUIDelegate::kNoIndex;
I guess the cause is that TranslateUIDelegate::kNoIndex is defined in *.cc.
Anyway, I reverted this CL once. Thank you.

On 2013/10/03 10:03:11, tapted wrote:
> This change (I think) has managed to add a static initializer, closing the
tree.
> 
> at
>
http://build.chromium.org/p/chromium/builders/Linux/builds/43453/steps/sizes/...
> 
>   # thread_cache.cc strtoll@plt
> > # translate_infobar_delegate.cc __i686.get_pc_thunk.cx
>   # unicodetext.cc std::ios_base::Init::Init()@plt
> 
> sorry - I think I need to revert this.

[Miguel Garcia, 2013-10-03 10:34:49]

Thanks for the heads up, we are working on a less hacky solution for all
infobars so hopefully I won't need this change but if I do I'll rework it
on the new code.

Out of curiosity, why do you need a delegate to then talk to another (ui_)
delegate that then talks to the translate manager ? Isn't that one
indirection too many?


On Tue, Oct 1, 2013 at 11:58 AM, <toyoshim@chromium.org> wrote:

> +miguelg FYI
>
> Miguel:
> Since this change is blocking our new feature implementation, we'll land
> this
> change which will conflict with your fix for Android.
> Can you share your plan how to fix delegate crash bug for Android? I'm
> fine if
> you want to merge your workaround patch to branches. But for trunk, I still
> think we should investigate why it can be NULL since potentially it will
> cause
> another unexpected behavior.
>
> Hajime:
> LGTM.
>
>
https://codereview.chromium.**org/23708029/<https://codereview.chromium.org/2...
>

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

[Takashi Toyoshima, 2013-10-03 10:40:58]

Hajime works on implementing new UI behind a flag.
There are some common methods which are called from new UI's delegate and old
UI's delegate.
So he factors them out to translate_ui_delegate.cc.

[hajimehoshi, 2013-10-03 12:04:39]

toyoshim@, can you take a look again?  Thanks.

[Takashi Toyoshima, 2013-10-03 12:31:32]

LGTM.

https://codereview.chromium.org/23708029/diff/44001/chrome/browser/translate/...
File chrome/browser/translate/translate_ui_delegate.cc (right):

https://codereview.chromium.org/23708029/diff/44001/chrome/browser/translate/...
chrome/browser/translate/translate_ui_delegate.cc:32: const size_t
TranslateUIDelegate::kNoIndex = static_cast<size_t>(-1);
For this file, alternative simple change is just replace this -1 with NO_INDEX.
But, your change is also OK.

[commit-bot: I haz the power, 2013-10-04 04:33:34]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/hajimehoshi@chromium.org/23708029/84001

[commit-bot: I haz the power, 2013-10-04 07:41:56]

Retried try job too often on win_rel for step(s) telemetry_unittests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[commit-bot: I haz the power, 2013-10-07 04:18:00]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/hajimehoshi@chromium.org/23708029/122001

[commit-bot: I haz the power, 2013-10-07 04:24:11]

Step "update" is always a major failure.
Look at the try server FAQ for more details.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_cl...

[commit-bot: I haz the power, 2013-10-07 04:46:53]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/hajimehoshi@chromium.org/23708029/122001

[commit-bot: I haz the power, 2013-10-07 05:12:49]

Sorry for I got bad news for ya.
Compile failed with a clobber build on win_x64_rel.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_x64_re...
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2013-10-07 06:34:12]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/hajimehoshi@chromium.org/23708029/122001

[commit-bot: I haz the power, 2013-10-07 06:58:09]

Sorry for I got bad news for ya.
Compile failed with a clobber build on win_x64_rel.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_x64_re...
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2013-10-07 07:39:56]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/hajimehoshi@chromium.org/23708029/122001

[commit-bot: I haz the power, 2013-10-07 08:05:23]

Sorry for I got bad news for ya.
Compile failed with a clobber build on win_x64_rel.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_x64_re...
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2013-10-07 08:54:55]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/hajimehoshi@chromium.org/23708029/122001

[commit-bot: I haz the power, 2013-10-07 09:19:44]

Sorry for I got bad news for ya.
Compile failed with a clobber build on win.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win&number...
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2013-10-08 00:28:17]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/hajimehoshi@chromium.org/23708029/122001

[commit-bot: I haz the power, 2013-10-08 02:17:43]

Change committed as 227430