Translate: New Bubble UX (for the view toolkit)

Translate: New Bubble UX (for the view toolkit)

This CL enables the new Translate bubble UX behind the flag --enable-translate-new-ux. As for the screenshots, please see the issue crbug.com/276181.

BUG=276181
TEST=browser_tests --gtest_filter=TranslateManagerBrowserTests.*

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

[hajimehoshi, 2013-10-08 02:26:39]

Can you take a look?  Thank you in advance.

toyoshim: please see chrome/browser/translate/*. As you know, 
translate_ui_delegate is now being committed in the other CL. Please ignore
this.
sky: please see the other files.

[hajimehoshi, 2013-10-08 02:49:33]

> toyoshim: please see chrome/browser/translate/*. As you know, 
> translate_ui_delegate is now being committed in the other CL. Please ignore
> this.

Now that was committed and I rebased this CL.

[Takashi Toyoshima, 2013-10-08 14:54:09]

This change is not small, but has no tests. Do you have a plan how to test these
components? If you have no idea, I suggest to split them to small changes that
contains unit tests for each.

https://codereview.chromium.org/25373009/diff/33001/chrome/browser/tab_conten...
File chrome/browser/tab_contents/language_state.h (right):

https://codereview.chromium.org/25373009/diff/33001/chrome/browser/tab_conten...
chrome/browser/tab_contents/language_state.h:15: class WebContents;
sort these names (line 13:14 were wrong originally)

https://codereview.chromium.org/25373009/diff/33001/chrome/browser/tab_conten...
chrome/browser/tab_contents/language_state.h:84: bool is_translate_suggested()
const { return is_translate_suggested_; }
To be consistent, translation_suggested() and SetTranslationSuggested() look
better.

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

https://codereview.chromium.org/25373009/diff/33001/chrome/browser/translate/...
chrome/browser/translate/translate_manager.cc:82: bool IsEnabledTranslateNewUX()
{
IsTranslateNewUXEnabled() sounds better.

Just an idea.
By the way, TranslateManager is a singleton. So, I guess we don't have to check
a flag for each. Can we check all flags TranslateManager need when the singleton
is created, and cache them as member variables?
It's ok to forget this idea in this CL.

https://codereview.chromium.org/25373009/diff/33001/chrome/browser/translate/...
chrome/browser/translate/translate_manager.cc:88: // TODO(hajimehoshi): Show
bubble only when needed.
What do you mean by this TODO?

https://codereview.chromium.org/25373009/diff/33001/chrome/browser/translate/...
chrome/browser/translate/translate_manager.cc:90: DCHECK(browser != NULL);
Do you check if these DCHECK are always correct?
Some classes have an implementation for browser being NULL. I'm afraid it might
happen here too.

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

https://codereview.chromium.org/25373009/diff/33001/chrome/browser/translate/...
chrome/browser/translate/translate_ui_delegate.cc:175:
translate_tab_helper->language_state().SetIsTranslateSuggested(false);
Here, you hide bubble UX immediately.
If a user mistakenly decide to block a language, they don't have a chance to
revert it, right? I'm not sure this is a good experience. Also these code
require Observer to change a status dynamically. WDYT?

https://codereview.chromium.org/25373009/diff/33001/chrome/browser/ui/browser...
File chrome/browser/ui/browser_commands.cc (right):

https://codereview.chromium.org/25373009/diff/33001/chrome/browser/ui/browser...
chrome/browser/ui/browser_commands.cc:684: void Translate(Browser* browser) {
We have similar implementation in render_view_context_menu.
You may want to make it work with new UX, and implementation will be shared
between here and there.

https://codereview.chromium.org/25373009/diff/33001/chrome/browser/ui/browser...
File chrome/browser/ui/browser_window.h (right):

https://codereview.chromium.org/25373009/diff/33001/chrome/browser/ui/browser...
chrome/browser/ui/browser_window.h:227: // TODO(hajimehoshi): Make this pure
virtual.
Yeah, this TODO means we don't implement this method in any child class for
testing. At least, file a bug for this, and add an URL link for that here. And
of course, writing a test is better than that.

https://codereview.chromium.org/25373009/diff/33001/chrome/browser/ui/transla...
File chrome/browser/ui/translate/translate_bubble.h (right):

https://codereview.chromium.org/25373009/diff/33001/chrome/browser/ui/transla...
chrome/browser/ui/translate/translate_bubble.h:10: class TranslateBubble {
namespace is enough.

https://codereview.chromium.org/25373009/diff/33001/chrome/browser/ui/views/f...
File chrome/browser/ui/views/frame/browser_view.cc (right):

https://codereview.chromium.org/25373009/diff/33001/chrome/browser/ui/views/f...
chrome/browser/ui/views/frame/browser_view.cc:1168:
language_state.SetIsTranslateSuggested(true);
Do you really need to set language state here?
Also how about naming this function UpdateTranslateBubble or something. The
language state is always set before calling ShowTranslateBubble. It looks
redundant.

https://codereview.chromium.org/25373009/diff/33001/chrome/browser/ui/views/t...
File chrome/browser/ui/views/toolbar_view.cc (right):

https://codereview.chromium.org/25373009/diff/33001/chrome/browser/ui/views/t...
chrome/browser/ui/views/toolbar_view.cc:289: DCHECK(translate_icon_view);
Did you check if this DCHECK is correct?

https://codereview.chromium.org/25373009/diff/33001/chrome/browser/ui/views/t...
File chrome/browser/ui/views/translate/translate_bubble_view.cc (right):

https://codereview.chromium.org/25373009/diff/33001/chrome/browser/ui/views/t...
chrome/browser/ui/views/translate/translate_bubble_view.cc:88: // component
instead of Combobox.
Have a crbug for this?

https://codereview.chromium.org/25373009/diff/33001/chrome/browser/ui/views/t...
File chrome/browser/ui/views/translate/translate_bubble_view.h (right):

https://codereview.chromium.org/25373009/diff/33001/chrome/browser/ui/views/t...
chrome/browser/ui/views/translate/translate_bubble_view.h:84: // Create the
'before translate' view. Caller takes ownership of the returned
Creates

ditto on following a series of 'Create'

[hajimehoshi, 2013-10-09 11:06:24]

Thanks. I'll separate this CL into a CL for the UI component and the other CL
for the usage.

[hajimehoshi, 2013-10-10 08:58:58]

I'll close this.

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

I reopened this to show the below comments. Please review
https://codereview.chromium.org/26775007/.

https://codereview.chromium.org/25373009/diff/33001/chrome/browser/tab_conten...
File chrome/browser/tab_contents/language_state.h (right):

https://codereview.chromium.org/25373009/diff/33001/chrome/browser/tab_conten...
chrome/browser/tab_contents/language_state.h:15: class WebContents;
On 2013/10/08 14:54:09, Takashi Toyoshima (chromium) wrote:
> sort these names (line 13:14 were wrong originally)

Done.

https://codereview.chromium.org/25373009/diff/33001/chrome/browser/tab_conten...
chrome/browser/tab_contents/language_state.h:84: bool is_translate_suggested()
const { return is_translate_suggested_; }
On 2013/10/08 14:54:09, Takashi Toyoshima (chromium) wrote:
> To be consistent, translation_suggested() and SetTranslationSuggested() look
> better.

Done.

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

https://codereview.chromium.org/25373009/diff/33001/chrome/browser/translate/...
chrome/browser/translate/translate_manager.cc:88: // TODO(hajimehoshi): Show
bubble only when needed.
This comment means nothing. I'll remove this.

https://codereview.chromium.org/25373009/diff/33001/chrome/browser/translate/...
chrome/browser/translate/translate_manager.cc:90: DCHECK(browser != NULL);
I found these assertion failed on TranslateManagerBrowserTest. I'll remove them.

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

https://codereview.chromium.org/25373009/diff/33001/chrome/browser/translate/...
chrome/browser/translate/translate_ui_delegate.cc:175:
translate_tab_helper->language_state().SetIsTranslateSuggested(false);
Hiding the icon is, if anything, Kenji's idea. We agreed to that it's OK because
the user can retry from the context menu.

https://codereview.chromium.org/25373009/diff/33001/chrome/browser/ui/browser...
File chrome/browser/ui/browser_commands.cc (right):

https://codereview.chromium.org/25373009/diff/33001/chrome/browser/ui/browser...
chrome/browser/ui/browser_commands.cc:684: void Translate(Browser* browser) {
This is slightly different from render_view_context_menu, because this method
only shows the bubble according to the current state, while
IDC_CONTENT_CONTEXT_TRANSLATE command tries to translate the page actually.

https://codereview.chromium.org/25373009/diff/33001/chrome/browser/ui/browser...
File chrome/browser/ui/browser_window.h (right):

https://codereview.chromium.org/25373009/diff/33001/chrome/browser/ui/browser...
chrome/browser/ui/browser_window.h:227: // TODO(hajimehoshi): Make this pure
virtual.
OK, I'll write the test.

https://codereview.chromium.org/25373009/diff/33001/chrome/browser/ui/transla...
File chrome/browser/ui/translate/translate_bubble.h (right):

https://codereview.chromium.org/25373009/diff/33001/chrome/browser/ui/transla...
chrome/browser/ui/translate/translate_bubble.h:10: class TranslateBubble {
On 2013/10/08 14:54:09, Takashi Toyoshima (chromium) wrote:
> namespace is enough.

Done.

https://codereview.chromium.org/25373009/diff/33001/chrome/browser/ui/views/f...
File chrome/browser/ui/views/frame/browser_view.cc (right):

https://codereview.chromium.org/25373009/diff/33001/chrome/browser/ui/views/f...
chrome/browser/ui/views/frame/browser_view.cc:1168:
language_state.SetIsTranslateSuggested(true);
As we discussed offline, this calling is needed to show the icon whenever the
bubble appears, and I'll rename this to SetIsTranslateEnabled.

https://codereview.chromium.org/25373009/diff/33001/chrome/browser/ui/views/t...
File chrome/browser/ui/views/toolbar_view.cc (right):

https://codereview.chromium.org/25373009/diff/33001/chrome/browser/ui/views/t...
chrome/browser/ui/views/toolbar_view.cc:289: DCHECK(translate_icon_view);
Ah, this could be called before LocationBarView's initializing. I'll do this in
the same way as GetBookmarkBubbleAnchor.

https://codereview.chromium.org/25373009/diff/33001/chrome/browser/ui/views/t...
File chrome/browser/ui/views/translate/translate_bubble_view.cc (right):

https://codereview.chromium.org/25373009/diff/33001/chrome/browser/ui/views/t...
chrome/browser/ui/views/translate/translate_bubble_view.cc:88: // component
instead of Combobox.
Created. Done.

https://codereview.chromium.org/25373009/diff/33001/chrome/browser/ui/views/t...
File chrome/browser/ui/views/translate/translate_bubble_view.h (right):

https://codereview.chromium.org/25373009/diff/33001/chrome/browser/ui/views/t...
chrome/browser/ui/views/translate/translate_bubble_view.h:84: // Create the
'before translate' view. Caller takes ownership of the returned
On 2013/10/08 14:54:09, Takashi Toyoshima (chromium) wrote:
> Creates
> 
> ditto on following a series of 'Create'

Done.

[hajimehoshi, 2013-10-18 11:12:37]

The CL https://codereview.chromium.org/26775007/ is now being committed (of
course, I'll rebase this).

Can you take a look this again?  Thank you in advance.
toyoshim: chrome/browser/translate/*
sky: the other files (if there exist files you can't review, please let me know
who to ask.)

[sky, 2013-10-18 14:31:10]

You have both patches here don't you? Make this patch relative to the other.

https://codereview.chromium.org/25373009/diff/103001/chrome/app/generated_res...
File chrome/app/generated_resources.grd (right):

https://codereview.chromium.org/25373009/diff/103001/chrome/app/generated_res...
chrome/app/generated_resources.grd:6823: +        Enable the new Translate UX.
Why the caps here and below (Translate)?

https://codereview.chromium.org/25373009/diff/103001/chrome/browser/tab_conte...
File chrome/browser/tab_contents/language_state.cc (right):

https://codereview.chromium.org/25373009/diff/103001/chrome/browser/tab_conte...
chrome/browser/tab_contents/language_state.cc:51: SetTranslateEnabled(false);
It's unusual that this property implicitly changes on navigation.

https://codereview.chromium.org/25373009/diff/103001/chrome/browser/tab_conte...
File chrome/browser/tab_contents/language_state.h (right):

https://codereview.chromium.org/25373009/diff/103001/chrome/browser/tab_conte...
chrome/browser/tab_contents/language_state.h:31: virtual void
OnTranslateEnabledChanged(content::WebContents* source) = 0;
Add description. Also, I would prefer this in its own header.

https://codereview.chromium.org/25373009/diff/103001/chrome/browser/tab_conte...
chrome/browser/tab_contents/language_state.h:88: bool IsLanguageChanged() const;
HasLanguageChanged

https://codereview.chromium.org/25373009/diff/103001/chrome/browser/translate...
File chrome/browser/translate/translate_bubble_browsertest.cc (right):

https://codereview.chromium.org/25373009/diff/103001/chrome/browser/translate...
chrome/browser/translate/translate_bubble_browsertest.cc:130:
base::MessageLoop::current()->RunUntilIdle();
How do you know this is good enough for the translate manager to catch up?

https://codereview.chromium.org/25373009/diff/103001/chrome/browser/translate...
chrome/browser/translate/translate_bubble_browsertest.cc:135: };
DISALLOW...

https://codereview.chromium.org/25373009/diff/103001/chrome/browser/translate...
chrome/browser/translate/translate_bubble_browsertest.cc:139:
IN_PROC_BROWSER_TEST_F(TranslateBubbleBrowserTest, TranslateSucceeded) {
Is it possible to write these as unit tests?

https://codereview.chromium.org/25373009/diff/103001/chrome/browser/translate...
chrome/browser/translate/translate_bubble_browsertest.cc:165:
PageTranslatedDetails>
nit: indent 2 more.

https://codereview.chromium.org/25373009/diff/103001/chrome/browser/ui/browser.h
File chrome/browser/ui/browser.h (right):

https://codereview.chromium.org/25373009/diff/103001/chrome/browser/ui/browse...
chrome/browser/ui/browser.h:103: public LanguageState::Observer,
Having Browser implement tons of observers doesn't really scale. Move this into
BrowserLanguageStateObserver and have Browser own BrowserLanguageStateObserver.

https://codereview.chromium.org/25373009/diff/103001/chrome/browser/ui/browse...
File chrome/browser/ui/browser_window.h (right):

https://codereview.chromium.org/25373009/diff/103001/chrome/browser/ui/browse...
chrome/browser/ui/browser_window.h:224: virtual void ShowTranslateBubble(
Why does this and GetTranslateBUbbleModel need to be on BrowserWindow? I think
I'm ok with ShowTranslateBubble, but GetTranslateBubbleModel? That shouldn't be
on browserwindow.

[hajimehoshi, 2013-10-18 15:19:59]

https://codereview.chromium.org/25373009/diff/103001/chrome/browser/translate...
File chrome/browser/translate/translate_bubble_browsertest.cc (right):

https://codereview.chromium.org/25373009/diff/103001/chrome/browser/translate...
chrome/browser/translate/translate_bubble_browsertest.cc:139:
IN_PROC_BROWSER_TEST_F(TranslateBubbleBrowserTest, TranslateSucceeded) {
I tried to write a unit test, but I think this is impossible because the bubble
view belongs to the browser, not the web content.

[sky, 2013-10-18 18:20:58]

There are stubs for various ui/views things that work in unit tests.

On Fri, Oct 18, 2013 at 8:19 AM,  <hajimehoshi@chromium.org> wrote:
>
>
https://codereview.chromium.org/25373009/diff/103001/chrome/browser/translate...
> File chrome/browser/translate/translate_bubble_browsertest.cc (right):
>
>
https://codereview.chromium.org/25373009/diff/103001/chrome/browser/translate...
> chrome/browser/translate/translate_bubble_browsertest.cc:139:
> IN_PROC_BROWSER_TEST_F(TranslateBubbleBrowserTest, TranslateSucceeded) {
> I tried to write a unit test, but I think this is impossible because the
> bubble view belongs to the browser, not the web content.
>
> https://codereview.chromium.org/25373009/

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

[hajimehoshi, 2013-10-21 10:29:12]

Thank you! It's not completed, and I'll do the rest tomorrow.

https://codereview.chromium.org/25373009/diff/103001/chrome/app/generated_res...
File chrome/app/generated_resources.grd (right):

https://codereview.chromium.org/25373009/diff/103001/chrome/app/generated_res...
chrome/app/generated_resources.grd:6823: +        Enable the new Translate UX.
On 2013/10/18 14:31:10, sky wrote:
> Why the caps here and below (Translate)?

Done.

https://codereview.chromium.org/25373009/diff/103001/chrome/browser/tab_conte...
File chrome/browser/tab_contents/language_state.h (right):

https://codereview.chromium.org/25373009/diff/103001/chrome/browser/tab_conte...
chrome/browser/tab_contents/language_state.h:31: virtual void
OnTranslateEnabledChanged(content::WebContents* source) = 0;
On 2013/10/18 14:31:10, sky wrote:
> Add description. Also, I would prefer this in its own header.

Done.

https://codereview.chromium.org/25373009/diff/103001/chrome/browser/tab_conte...
chrome/browser/tab_contents/language_state.h:88: bool IsLanguageChanged() const;
On 2013/10/18 14:31:10, sky wrote:
> HasLanguageChanged

Done.

https://codereview.chromium.org/25373009/diff/103001/chrome/browser/translate...
File chrome/browser/translate/translate_bubble_browsertest.cc (right):

https://codereview.chromium.org/25373009/diff/103001/chrome/browser/translate...
chrome/browser/translate/translate_bubble_browsertest.cc:130:
base::MessageLoop::current()->RunUntilIdle();
From TranslateManagerBrowserTest. I'll add a comment.

https://codereview.chromium.org/25373009/diff/103001/chrome/browser/translate...
chrome/browser/translate/translate_bubble_browsertest.cc:135: };
On 2013/10/18 14:31:10, sky wrote:
> DISALLOW...

Done.

https://codereview.chromium.org/25373009/diff/103001/chrome/browser/translate...
chrome/browser/translate/translate_bubble_browsertest.cc:165:
PageTranslatedDetails>
On 2013/10/18 14:31:10, sky wrote:
> nit: indent 2 more.

Done. (by template 'using')

https://codereview.chromium.org/25373009/diff/103001/chrome/browser/ui/browser.h
File chrome/browser/ui/browser.h (right):

https://codereview.chromium.org/25373009/diff/103001/chrome/browser/ui/browse...
chrome/browser/ui/browser.h:103: public LanguageState::Observer,
On 2013/10/18 14:31:10, sky wrote:
> Having Browser implement tons of observers doesn't really scale. Move this
into
> BrowserLanguageStateObserver and have Browser own
BrowserLanguageStateObserver.

Done.

[hajimehoshi, 2013-10-22 10:30:24]

All done.

https://codereview.chromium.org/25373009/diff/103001/chrome/browser/tab_conte...
File chrome/browser/tab_contents/language_state.cc (right):

https://codereview.chromium.org/25373009/diff/103001/chrome/browser/tab_conte...
chrome/browser/tab_contents/language_state.cc:51: SetTranslateEnabled(false);
This is necessary to hide the translate icon on the Omnibox when the page is
navigated. I don't think it is better to move calling SetTranslateEnabled to the
caller of DidNavigate.

https://codereview.chromium.org/25373009/diff/103001/chrome/browser/translate...
File chrome/browser/translate/translate_bubble_browsertest.cc (right):

https://codereview.chromium.org/25373009/diff/103001/chrome/browser/translate...
chrome/browser/translate/translate_bubble_browsertest.cc:139:
IN_PROC_BROWSER_TEST_F(TranslateBubbleBrowserTest, TranslateSucceeded) {
I moved these tests to translate_manager_browsertest.cc as unit tests.

https://codereview.chromium.org/25373009/diff/103001/chrome/browser/ui/browse...
File chrome/browser/ui/browser_window.h (right):

https://codereview.chromium.org/25373009/diff/103001/chrome/browser/ui/browse...
chrome/browser/ui/browser_window.h:224: virtual void ShowTranslateBubble(
Removed GetTranslateBubbleModel.

[sky, 2013-10-22 16:09:22]

https://codereview.chromium.org/25373009/diff/353001/chrome/browser/tab_conte...
File chrome/browser/tab_contents/language_state.h (right):

https://codereview.chromium.org/25373009/diff/353001/chrome/browser/tab_conte...
chrome/browser/tab_contents/language_state.h:78: void SetTranslateEnabled(bool
value);
Document the behavior of this. In particular that it resets on every navigation.

https://codereview.chromium.org/25373009/diff/353001/chrome/browser/translate...
File chrome/browser/translate/translate_bubble_showable.h (right):

https://codereview.chromium.org/25373009/diff/353001/chrome/browser/translate...
chrome/browser/translate/translate_bubble_showable.h:20: virtual void
Show(content::WebContents* web_contents,
How about putting this in browser_dialogs and naming it ShowTranslateBubble? If
it needs a browser or webcontents, make it take those. The function should take
everything it needs and not have to search through the browserlist. The
implementation should reside in the approriate platforms, eg c/b/u/views.

If you went this route to make it mockable, then the following would be better:

class TranslateBubbleFactory {
 public:
  static void Show(XXX)  // internally uses factory, if no factory then calls
ShowImplementation().
  static void SetFactory(...);

 protected:
  virtual void ShowImplementation();

};

Platforms then implement TranslateBubbleFactory::ShowImplementation

Also, this should be in the chrome/browser/ui.

https://codereview.chromium.org/25373009/diff/353001/chrome/browser/ui/browse...
File chrome/browser/ui/browser.cc (right):

https://codereview.chromium.org/25373009/diff/353001/chrome/browser/ui/browse...
chrome/browser/ui/browser.cc:1765: /*void
Browser::OnTranslateEnabledChanged(content::WebContents* source) {
?

https://codereview.chromium.org/25373009/diff/353001/chrome/browser/ui/views/...
File chrome/browser/ui/views/location_bar/translate_icon_view.cc (right):

https://codereview.chromium.org/25373009/diff/353001/chrome/browser/ui/views/...
chrome/browser/ui/views/location_bar/translate_icon_view.cc:19:
TranslateIconView::TranslateIconView(
This is nearly a duplicate of StarView. Refactor the two so they share code.
Maybe have the class take the id of the command an a delegate that can determine
if the corresponding bubble is showing as well as notification before executing
the command (for the histograms used by starview).

[sky, 2013-10-22 16:16:00]

https://codereview.chromium.org/25373009/diff/353001/chrome/browser/translate...
File chrome/browser/translate/translate_bubble_showable.h (right):

https://codereview.chromium.org/25373009/diff/353001/chrome/browser/translate...
chrome/browser/translate/translate_bubble_showable.h:20: virtual void
Show(content::WebContents* web_contents,
On 2013/10/22 16:09:22, sky wrote:
> How about putting this in browser_dialogs and naming it ShowTranslateBubble?
If
> it needs a browser or webcontents, make it take those. The function should
take
> everything it needs and not have to search through the browserlist. The
> implementation should reside in the approriate platforms, eg c/b/u/views.
> 
> If you went this route to make it mockable, then the following would be
better:
> 
> class TranslateBubbleFactory {
>  public:
>   static void Show(XXX)  // internally uses factory, if no factory then calls
> ShowImplementation().
>   static void SetFactory(...);
> 
>  protected:
>   virtual void ShowImplementation();
> 
> };
> 
> Platforms then implement TranslateBubbleFactory::ShowImplementation
> 
> Also, this should be in the chrome/browser/ui.

Also, you could keep the Show method in browser_dialogs like I outlined and have
TranslateBubbleFactory::Show() invoke it if no factory is set. That way
ShowImplementation() can remain pure virtual.

[hajimehoshi, 2013-10-23 10:45:09]

Thank you!

https://codereview.chromium.org/25373009/diff/353001/chrome/browser/tab_conte...
File chrome/browser/tab_contents/language_state.h (right):

https://codereview.chromium.org/25373009/diff/353001/chrome/browser/tab_conte...
chrome/browser/tab_contents/language_state.h:78: void SetTranslateEnabled(bool
value);
On 2013/10/22 16:09:22, sky wrote:
> Document the behavior of this. In particular that it resets on every
navigation.

Done.

https://codereview.chromium.org/25373009/diff/353001/chrome/browser/translate...
File chrome/browser/translate/translate_bubble_showable.h (right):

https://codereview.chromium.org/25373009/diff/353001/chrome/browser/translate...
chrome/browser/translate/translate_bubble_showable.h:20: virtual void
Show(content::WebContents* web_contents,
On 2013/10/22 16:16:00, sky wrote:
> On 2013/10/22 16:09:22, sky wrote:
> > How about putting this in browser_dialogs and naming it ShowTranslateBubble?
> If
> > it needs a browser or webcontents, make it take those. The function should
> take
> > everything it needs and not have to search through the browserlist. The
> > implementation should reside in the approriate platforms, eg c/b/u/views.
> > 
> > If you went this route to make it mockable, then the following would be
> better:
> > 
> > class TranslateBubbleFactory {
> >  public:
> >   static void Show(XXX)  // internally uses factory, if no factory then
calls
> > ShowImplementation().
> >   static void SetFactory(...);
> > 
> >  protected:
> >   virtual void ShowImplementation();
> > 
> > };
> > 
> > Platforms then implement TranslateBubbleFactory::ShowImplementation
> > 
> > Also, this should be in the chrome/browser/ui.
> 
> Also, you could keep the Show method in browser_dialogs like I outlined and
have
> TranslateBubbleFactory::Show() invoke it if no factory is set. That way
> ShowImplementation() can remain pure virtual.

Done. Showing the bubble via BrowserWindow is already platform-independent, so
just created TranslateBubbleFactory.

https://codereview.chromium.org/25373009/diff/353001/chrome/browser/ui/browse...
File chrome/browser/ui/browser.cc (right):

https://codereview.chromium.org/25373009/diff/353001/chrome/browser/ui/browse...
chrome/browser/ui/browser.cc:1765: /*void
Browser::OnTranslateEnabledChanged(content::WebContents* source) {
Sorry, done.

https://codereview.chromium.org/25373009/diff/353001/chrome/browser/ui/views/...
File chrome/browser/ui/views/location_bar/translate_icon_view.cc (right):

https://codereview.chromium.org/25373009/diff/353001/chrome/browser/ui/views/...
chrome/browser/ui/views/location_bar/translate_icon_view.cc:19:
TranslateIconView::TranslateIconView(
On 2013/10/22 16:09:22, sky wrote:
> This is nearly a duplicate of StarView. Refactor the two so they share code.
> Maybe have the class take the id of the command an a delegate that can
determine
> if the corresponding bubble is showing as well as notification before
executing
> the command (for the histograms used by starview).

Done.  Created BubbleIconView inheriting views::ImageView to share the
implementation.  I'm worried about using inheriting instead of composition
pattern.  Composition sounds better, but the Star and Translate icon share
implementation of views::ImageView, so it seems to be a little hard to use
composition.  In addition, I think BubbleIconView is not a good name.  I'd be
glad if you could have suggestion.

[sky, 2013-10-23 14:43:29]

https://codereview.chromium.org/25373009/diff/583001/chrome/browser/tab_conte...
File chrome/browser/tab_contents/language_state.h (right):

https://codereview.chromium.org/25373009/diff/583001/chrome/browser/tab_conte...
chrome/browser/tab_contents/language_state.h:77: // Whether the translate is
enabled. This value is supposed to be used for the
Remove second sentence, it isn't useful here.

https://codereview.chromium.org/25373009/diff/583001/chrome/browser/ui/browse...
File chrome/browser/ui/browser_language_state_observer.h (right):

https://codereview.chromium.org/25373009/diff/583001/chrome/browser/ui/browse...
chrome/browser/ui/browser_language_state_observer.h:15: // The implementation of
LanguageStateObserver for Browser. This observes the
'The implementation' -> 'Implementation of..'

https://codereview.chromium.org/25373009/diff/583001/chrome/browser/ui/browse...
chrome/browser/ui/browser_language_state_observer.h:16: // state if Translate is
enabled or not, and updates the toolbar.
Change second sentence to 'Updates toolbar in response to changes in state of
translate.'

https://codereview.chromium.org/25373009/diff/583001/chrome/browser/ui/transl...
File chrome/browser/ui/translate/translate_bubble_factory.cc (right):

https://codereview.chromium.org/25373009/diff/583001/chrome/browser/ui/transl...
chrome/browser/ui/translate/translate_bubble_factory.cc:40: current_factory_ =
factory;
Shouldn't you delete old? And document ownership.

https://codereview.chromium.org/25373009/diff/583001/chrome/browser/ui/views/...
File chrome/browser/ui/views/frame/browser_view.cc (right):

https://codereview.chromium.org/25373009/diff/583001/chrome/browser/ui/views/...
chrome/browser/ui/views/frame/browser_view.cc:1187: if (web_contents !=
browser_->tab_strip_model()->GetActiveWebContents())
Don't you have this check where ShowTranslateBubble is called from? Do we really
need it in both places?

https://codereview.chromium.org/25373009/diff/583001/chrome/browser/ui/views/...
File chrome/browser/ui/views/location_bar/bubble_icon_view.h (right):

https://codereview.chromium.org/25373009/diff/583001/chrome/browser/ui/views/...
chrome/browser/ui/views/location_bar/bubble_icon_view.h:13: // The icon on the
Omnibox which can show a bubble when the user click this.
Represents an icon on the omnibox that shows a bubble when clicked.

https://codereview.chromium.org/25373009/diff/583001/chrome/browser/ui/views/...
File chrome/browser/ui/views/location_bar/bubble_icon_view_delegate.h (right):

https://codereview.chromium.org/25373009/diff/583001/chrome/browser/ui/views/...
chrome/browser/ui/views/location_bar/bubble_icon_view_delegate.h:9: class
BubbleIconViewDelegate {
There is no point in both subclassing *and* having a delegate. If you can't get
the delegate to work, which it appears each needs to subclass for different
reasons, then nuke the delegate and have pure virtual methods.

https://codereview.chromium.org/25373009/diff/583001/chrome/browser/ui/views/...
chrome/browser/ui/views/location_bar/bubble_icon_view_delegate.h:17: virtual
void OnExecutingByMouse() = 0;
Add an enum and a single OnExecute(EXECUTE_SOURCE) method.

[hajimehoshi, 2013-10-24 11:31:14]

Thank you!

https://codereview.chromium.org/25373009/diff/583001/chrome/browser/tab_conte...
File chrome/browser/tab_contents/language_state.h (right):

https://codereview.chromium.org/25373009/diff/583001/chrome/browser/tab_conte...
chrome/browser/tab_contents/language_state.h:77: // Whether the translate is
enabled. This value is supposed to be used for the
On 2013/10/23 14:43:29, sky wrote:
> Remove second sentence, it isn't useful here.

Done.

https://codereview.chromium.org/25373009/diff/583001/chrome/browser/ui/browse...
File chrome/browser/ui/browser_language_state_observer.h (right):

https://codereview.chromium.org/25373009/diff/583001/chrome/browser/ui/browse...
chrome/browser/ui/browser_language_state_observer.h:15: // The implementation of
LanguageStateObserver for Browser. This observes the
On 2013/10/23 14:43:29, sky wrote:
> 'The implementation' -> 'Implementation of..'

Done.

https://codereview.chromium.org/25373009/diff/583001/chrome/browser/ui/browse...
chrome/browser/ui/browser_language_state_observer.h:16: // state if Translate is
enabled or not, and updates the toolbar.
On 2013/10/23 14:43:29, sky wrote:
> Change second sentence to 'Updates toolbar in response to changes in state of
> translate.'

Done.

https://codereview.chromium.org/25373009/diff/583001/chrome/browser/ui/transl...
File chrome/browser/ui/translate/translate_bubble_factory.cc (right):

https://codereview.chromium.org/25373009/diff/583001/chrome/browser/ui/transl...
chrome/browser/ui/translate/translate_bubble_factory.cc:40: current_factory_ =
factory;
On 2013/10/23 14:43:29, sky wrote:
> Shouldn't you delete old? And document ownership.

Done (added a comment to the header).  Is it better to have
TranslateBubbleFactory take the owner?  If so, maybe |current_factory_| should
be deleted by AtExitManager like what Singleton does?

https://codereview.chromium.org/25373009/diff/583001/chrome/browser/ui/views/...
File chrome/browser/ui/views/frame/browser_view.cc (right):

https://codereview.chromium.org/25373009/diff/583001/chrome/browser/ui/views/...
chrome/browser/ui/views/frame/browser_view.cc:1187: if (web_contents !=
browser_->tab_strip_model()->GetActiveWebContents())
On 2013/10/23 14:43:29, sky wrote:
> Don't you have this check where ShowTranslateBubble is called from? Do we
really
> need it in both places?

Done.

https://codereview.chromium.org/25373009/diff/583001/chrome/browser/ui/views/...
File chrome/browser/ui/views/location_bar/bubble_icon_view.h (right):

https://codereview.chromium.org/25373009/diff/583001/chrome/browser/ui/views/...
chrome/browser/ui/views/location_bar/bubble_icon_view.h:13: // The icon on the
Omnibox which can show a bubble when the user click this.
On 2013/10/23 14:43:29, sky wrote:
> Represents an icon on the omnibox that shows a bubble when clicked.

Done.

https://codereview.chromium.org/25373009/diff/583001/chrome/browser/ui/views/...
File chrome/browser/ui/views/location_bar/bubble_icon_view_delegate.h (right):

https://codereview.chromium.org/25373009/diff/583001/chrome/browser/ui/views/...
chrome/browser/ui/views/location_bar/bubble_icon_view_delegate.h:9: class
BubbleIconViewDelegate {
On 2013/10/23 14:43:29, sky wrote:
> There is no point in both subclassing *and* having a delegate. If you can't
get
> the delegate to work, which it appears each needs to subclass for different
> reasons, then nuke the delegate and have pure virtual methods.

Done.

https://codereview.chromium.org/25373009/diff/583001/chrome/browser/ui/views/...
chrome/browser/ui/views/location_bar/bubble_icon_view_delegate.h:17: virtual
void OnExecutingByMouse() = 0;
On 2013/10/23 14:43:29, sky wrote:
> Add an enum and a single OnExecute(EXECUTE_SOURCE) method.

Done.

[sky, 2013-10-24 15:49:07]

LGTM

https://codereview.chromium.org/25373009/diff/763001/chrome/browser/ui/views/...
File chrome/browser/ui/views/location_bar/bubble_icon_view.h (right):

https://codereview.chromium.org/25373009/diff/763001/chrome/browser/ui/views/...
chrome/browser/ui/views/location_bar/bubble_icon_view.h:24: // Returns true if a
related bubble is already shown.
'already shown' -> showing

https://codereview.chromium.org/25373009/diff/763001/chrome/browser/ui/views/...
chrome/browser/ui/views/location_bar/bubble_icon_view.h:27: // Handles when the
command is executed.
Invoked prior to executing the command.

[hajimehoshi, 2013-10-25 02:45:40]

Thank you!

https://codereview.chromium.org/25373009/diff/763001/chrome/browser/ui/views/...
File chrome/browser/ui/views/location_bar/bubble_icon_view.h (right):

https://codereview.chromium.org/25373009/diff/763001/chrome/browser/ui/views/...
chrome/browser/ui/views/location_bar/bubble_icon_view.h:24: // Returns true if a
related bubble is already shown.
On 2013/10/24 15:49:08, sky wrote:
> 'already shown' -> showing

Done.

https://codereview.chromium.org/25373009/diff/763001/chrome/browser/ui/views/...
chrome/browser/ui/views/location_bar/bubble_icon_view.h:27: // Handles when the
command is executed.
On 2013/10/24 15:49:08, sky wrote:
> Invoked prior to executing the command.

Done.

[Takashi Toyoshima, 2013-10-25 03:12:01]

thank you for great efforts.
chrome/*/translate/ lgtm.

[commit-bot: I haz the power, 2013-10-25 05:46:14]

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

[commit-bot: I haz the power, 2013-10-25 06:47:37]

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

[commit-bot: I haz the power, 2013-10-25 07:15:31]

Sorry for I got bad news for ya.
Compile failed with a clobber build on android_clang_dbg.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_cl...
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-25 07:20:55]

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

[commit-bot: I haz the power, 2013-10-25 10:39:45]

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

[commit-bot: I haz the power, 2013-10-25 10:47:57]

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

[commit-bot: I haz the power, 2013-10-25 13:18:30]

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

[commit-bot: I haz the power, 2013-10-28 03:32:59]

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

[commit-bot: I haz the power, 2013-10-28 04:58:46]

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

[commit-bot: I haz the power, 2013-10-28 05:07:27]

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=linux_clan...

[commit-bot: I haz the power, 2013-10-28 05:39:43]

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

[commit-bot: I haz the power, 2013-10-28 06:09:25]

Failed to trigger a try job on linux_rel
HTTP Error 400: Bad Request

[commit-bot: I haz the power, 2013-10-28 06:09:41]

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

[commit-bot: I haz the power, 2013-10-28 09:25:02]

Change committed as 231301