Translate: Add TranslateBubbleView

Translate: Add TranslateBubbleView

This is used for Translate new UX. For screen shots, please see the crbug.com/276181

BUG=276181
TEST=unit_tests --gtest_filter=TranslateBubbleViewTest.*

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

[hajimehoshi, 2013-10-10 09:51:02]

Can you take a look? Thanks.

toyoshim: chrome/browser/translate/translate_ui_delegate.h (comment-change only)
sky: the other files

[Takashi Toyoshima, 2013-10-14 23:29:33]

Don't you set each crbug entries blocking the master bug?
This change seems almost good to me.

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

https://codereview.chromium.org/26775007/diff/42001/chrome/browser/ui/views/t...
chrome/browser/ui/views/translate/translate_bubble_view.cc:516: views::View*
TranslateBubbleView::CreateViewAfterTranslate() {
Don't you show source and target translation languages any more?

https://codereview.chromium.org/26775007/diff/42001/chrome/browser/ui/views/t...
chrome/browser/ui/views/translate/translate_bubble_view.cc:566: views::View*
TranslateBubbleView::CreateViewError() {
Currently, only one generic message is used for all errors.
You may want to revise this to show a specific message for each error.
Can you add TODO here?

[Takashi Toyoshima, 2013-10-14 23:31:24]

by the way, translate_ui_delegate.h is not included in the last patch set...

[hajimehoshi, 2013-10-15 04:12:40]

> by the way, translate_ui_delegate.h is not included in the last patch set...

Ah, sorry.  This file existed the patch set 1, but now the fix is not needed.

In terms of OWNERs, you don't have to review this, but I think it's better to
get your review.  Thanks.

[hajimehoshi, 2013-10-15 04:14:15]

Thank you!

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

https://codereview.chromium.org/26775007/diff/42001/chrome/browser/ui/views/t...
chrome/browser/ui/views/translate/translate_bubble_view.cc:516: views::View*
TranslateBubbleView::CreateViewAfterTranslate() {
No. I think it is intended because the user can see those on the 'Advanced'
panel. I'll double-check it with Kenji.

https://codereview.chromium.org/26775007/diff/42001/chrome/browser/ui/views/t...
chrome/browser/ui/views/translate/translate_bubble_view.cc:566: views::View*
TranslateBubbleView::CreateViewError() {
On 2013/10/14 23:29:34, Takashi Toyoshima (chromium) wrote:
> Currently, only one generic message is used for all errors.
> You may want to revise this to show a specific message for each error.
> Can you add TODO here?

Done.

[Takashi Toyoshima, 2013-10-15 04:30:19]

lgtm

[hajimehoshi, 2013-10-15 08:29:46]

I moved the implementation of the view-type transitions from TranslateBubbleView
to TranslateBubbleModel because it is needed to get the current view type from a
platform-independent object (in this case, TarnslateBubbleModel) when I add
browser tests for the bubble.

sky, can you take a look?

[sky, 2013-10-15 16:10:40]

https://codereview.chromium.org/26775007/diff/68001/chrome/browser/ui/transla...
File chrome/browser/ui/translate/language_combobox_model.h (right):

https://codereview.chromium.org/26775007/diff/68001/chrome/browser/ui/transla...
chrome/browser/ui/translate/language_combobox_model.h:16: class
LanguageComboboxModel : public ui::ComboboxModel {
Always add a description for classes.

https://codereview.chromium.org/26775007/diff/68001/chrome/browser/ui/transla...
chrome/browser/ui/translate/language_combobox_model.h:29: int default_index_;
const

https://codereview.chromium.org/26775007/diff/68001/chrome/browser/ui/transla...
File chrome/browser/ui/translate/translate_bubble_model.cc (right):

https://codereview.chromium.org/26775007/diff/68001/chrome/browser/ui/transla...
chrome/browser/ui/translate/translate_bubble_model.cc:13: DCHECK(view_type_ !=
TranslateBubbleModel::ADVANCED);
DCHECK_NE

https://codereview.chromium.org/26775007/diff/68001/chrome/browser/ui/transla...
File chrome/browser/ui/translate/translate_bubble_model.h (right):

https://codereview.chromium.org/26775007/diff/68001/chrome/browser/ui/transla...
chrome/browser/ui/translate/translate_bubble_model.h:13: class
TranslateBubbleModel {
Description.

https://codereview.chromium.org/26775007/diff/68001/chrome/browser/ui/transla...
chrome/browser/ui/translate/translate_bubble_model.h:13: class
TranslateBubbleModel {
Is there a reason this class shouldn't be pure virtual?

https://codereview.chromium.org/26775007/diff/68001/chrome/browser/ui/transla...
chrome/browser/ui/translate/translate_bubble_model.h:15: enum ViewType {
Isn't this more of a State rather than a Type?

https://codereview.chromium.org/26775007/diff/68001/chrome/browser/ui/transla...
chrome/browser/ui/translate/translate_bubble_model.h:23: class Observer {
Move into own header.

https://codereview.chromium.org/26775007/diff/68001/chrome/browser/ui/transla...
chrome/browser/ui/translate/translate_bubble_model.h:60: virtual void
SetLanguageBlocked(bool value) = 0;
It's not at all clear what blocked or blacklisted means in this context.

https://codereview.chromium.org/26775007/diff/68001/chrome/browser/ui/transla...
File chrome/browser/ui/translate/translate_bubble_model_impl.h (right):

https://codereview.chromium.org/26775007/diff/68001/chrome/browser/ui/transla...
chrome/browser/ui/translate/translate_bubble_model_impl.h:14: class
TranslateBubbleModelImpl : public TranslateBubbleModel {
Again, description.

https://codereview.chromium.org/26775007/diff/68001/chrome/browser/ui/transla...
chrome/browser/ui/translate/translate_bubble_model_impl.h:24: 
nit: generally no newlines between overriden methods.

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

https://codereview.chromium.org/26775007/diff/68001/chrome/browser/ui/views/t...
chrome/browser/ui/views/translate/translate_bubble_view.h:83: enum {
Use named enums, that way you can cast in switch statement and get compile error
if switch not handled.

[hajimehoshi2, 2013-10-16 00:14:43]

Thank you for your reviewing!  Please let me ask a quick question.

https://codereview.chromium.org/26775007/diff/68001/chrome/browser/ui/transla...
File chrome/browser/ui/translate/translate_bubble_model.h (right):

https://codereview.chromium.org/26775007/diff/68001/chrome/browser/ui/transla...
chrome/browser/ui/translate/translate_bubble_model.h:15: enum ViewType {
On 2013/10/15 16:10:40, sky wrote:
> Isn't this more of a State rather than a Type?

Yep, but do you mean these values should be classes as state pattern?

[hajimehoshi2, 2013-10-16 00:36:09]

On 2013/10/16 00:14:43, hajimehoshi1 wrote:
> Thank you for your reviewing!  Please let me ask a quick question.
> 
>
https://codereview.chromium.org/26775007/diff/68001/chrome/browser/ui/transla...
> File chrome/browser/ui/translate/translate_bubble_model.h (right):
> 
>
https://codereview.chromium.org/26775007/diff/68001/chrome/browser/ui/transla...
> chrome/browser/ui/translate/translate_bubble_model.h:15: enum ViewType {
> On 2013/10/15 16:10:40, sky wrote:
> > Isn't this more of a State rather than a Type?
> 
> Yep, but do you mean these values should be classes as state pattern?

I got what you meant. Sure, the name should be ViewState instead of ViewType.

[hajimehoshi, 2013-10-16 07:35:06]

Thank you very much!

https://codereview.chromium.org/26775007/diff/68001/chrome/browser/ui/transla...
File chrome/browser/ui/translate/language_combobox_model.h (right):

https://codereview.chromium.org/26775007/diff/68001/chrome/browser/ui/transla...
chrome/browser/ui/translate/language_combobox_model.h:16: class
LanguageComboboxModel : public ui::ComboboxModel {
On 2013/10/15 16:10:40, sky wrote:
> Always add a description for classes.

Done.

https://codereview.chromium.org/26775007/diff/68001/chrome/browser/ui/transla...
chrome/browser/ui/translate/language_combobox_model.h:29: int default_index_;
On 2013/10/15 16:10:40, sky wrote:
> const

Done.

https://codereview.chromium.org/26775007/diff/68001/chrome/browser/ui/transla...
File chrome/browser/ui/translate/translate_bubble_model.cc (right):

https://codereview.chromium.org/26775007/diff/68001/chrome/browser/ui/transla...
chrome/browser/ui/translate/translate_bubble_model.cc:13: DCHECK(view_type_ !=
TranslateBubbleModel::ADVANCED);
On 2013/10/15 16:10:40, sky wrote:
> DCHECK_NE

Done.

https://codereview.chromium.org/26775007/diff/68001/chrome/browser/ui/transla...
File chrome/browser/ui/translate/translate_bubble_model.h (right):

https://codereview.chromium.org/26775007/diff/68001/chrome/browser/ui/transla...
chrome/browser/ui/translate/translate_bubble_model.h:13: class
TranslateBubbleModel {
No. I factored the implementation out to TranslateBubbleViewStateTransition.

https://codereview.chromium.org/26775007/diff/68001/chrome/browser/ui/transla...
chrome/browser/ui/translate/translate_bubble_model.h:15: enum ViewType {
On 2013/10/16 00:14:48, hajimehoshi1 wrote:
> On 2013/10/15 16:10:40, sky wrote:
> > Isn't this more of a State rather than a Type?
> 
> Yep, but do you mean these values should be classes as state pattern?

Done.

https://codereview.chromium.org/26775007/diff/68001/chrome/browser/ui/transla...
chrome/browser/ui/translate/translate_bubble_model.h:23: class Observer {
I reimplemented TranslateBubbleView not to use this class and removed this.

https://codereview.chromium.org/26775007/diff/68001/chrome/browser/ui/transla...
chrome/browser/ui/translate/translate_bubble_model.h:60: virtual void
SetLanguageBlocked(bool value) = 0;
Added comments and renamed them to SetNeverTranslateLanguage and
SetNeverTranslateSite.

https://codereview.chromium.org/26775007/diff/68001/chrome/browser/ui/transla...
File chrome/browser/ui/translate/translate_bubble_model_impl.h (right):

https://codereview.chromium.org/26775007/diff/68001/chrome/browser/ui/transla...
chrome/browser/ui/translate/translate_bubble_model_impl.h:14: class
TranslateBubbleModelImpl : public TranslateBubbleModel {
On 2013/10/15 16:10:40, sky wrote:
> Again, description.

Done.

https://codereview.chromium.org/26775007/diff/68001/chrome/browser/ui/transla...
chrome/browser/ui/translate/translate_bubble_model_impl.h:24: 
On 2013/10/15 16:10:40, sky wrote:
> nit: generally no newlines between overriden methods.

Done.

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

https://codereview.chromium.org/26775007/diff/68001/chrome/browser/ui/views/t...
chrome/browser/ui/views/translate/translate_bubble_view.h:83: enum {
On 2013/10/15 16:10:40, sky wrote:
> Use named enums, that way you can cast in switch statement and get compile
error
> if switch not handled.

Done.

[sky, 2013-10-16 14:32:35]

https://codereview.chromium.org/26775007/diff/103001/chrome/browser/ui/transl...
File chrome/browser/ui/translate/language_combobox_model.h (right):

https://codereview.chromium.org/26775007/diff/103001/chrome/browser/ui/transl...
chrome/browser/ui/translate/language_combobox_model.h:17: // user interface to
select an original language or a target.
'an original language or a target' -> language

https://codereview.chromium.org/26775007/diff/103001/chrome/browser/ui/transl...
File chrome/browser/ui/translate/translate_bubble_model.h (right):

https://codereview.chromium.org/26775007/diff/103001/chrome/browser/ui/transl...
chrome/browser/ui/translate/translate_bubble_model.h:21: ADVANCED,
Document ADVANCED. It's not clear how the user gets there, or gets out of it.

https://codereview.chromium.org/26775007/diff/103001/chrome/browser/ui/transl...
File chrome/browser/ui/translate/translate_bubble_model_impl.h (right):

https://codereview.chromium.org/26775007/diff/103001/chrome/browser/ui/transl...
chrome/browser/ui/translate/translate_bubble_model_impl.h:18: explicit
TranslateBubbleModelImpl(
no explicit

https://codereview.chromium.org/26775007/diff/103001/chrome/browser/ui/views/...
File chrome/browser/ui/views/translate/translate_bubble_view.cc (right):

https://codereview.chromium.org/26775007/diff/103001/chrome/browser/ui/views/...
chrome/browser/ui/views/translate/translate_bubble_view.cc:66:
TranslateTabHelper::FromWebContents(web_contents);
nit: indent 2 more

https://codereview.chromium.org/26775007/diff/103001/chrome/browser/ui/views/...
chrome/browser/ui/views/translate/translate_bubble_view.cc:67: if
(!translate_tab_helper)
If there is no translate helper should this code even run? It seems like this
check should be earlier on and this code should assume there is a
TranslateTabHelper.

https://codereview.chromium.org/26775007/diff/103001/chrome/browser/ui/views/...
chrome/browser/ui/views/translate/translate_bubble_view.cc:72: *target =
TranslateManager::GetLanguageCode(*target);
nit: make this one line and have TM::GetLanguageCode take g_browser_pr...

https://codereview.chromium.org/26775007/diff/103001/chrome/browser/ui/views/...
chrome/browser/ui/views/translate/translate_bubble_view.cc:104: return
items_.at(index);
items_[index]?

https://codereview.chromium.org/26775007/diff/103001/chrome/browser/ui/views/...
chrome/browser/ui/views/translate/translate_bubble_view.cc:107: return
items_.at(index).empty();
items_[index]?

https://codereview.chromium.org/26775007/diff/103001/chrome/browser/ui/views/...
chrome/browser/ui/views/translate/translate_bubble_view.cc:123:
TranslateBubbleView* TranslateBubbleView::translate_bubble_view_;
Style guide says statics have:
// static
on previous line.

Also, you need to set to NULL here.

https://codereview.chromium.org/26775007/diff/103001/chrome/browser/ui/views/...
chrome/browser/ui/views/translate/translate_bubble_view.cc:250: default:
Can this be removed?

https://codereview.chromium.org/26775007/diff/103001/chrome/browser/ui/views/...
chrome/browser/ui/views/translate/translate_bubble_view.cc:263: int height =
child_at(model_->GetViewState())->GetPreferredSize().height();
Assuming a particular view order is fragile. Can you make the view you care
about a member and directly access it?

https://codereview.chromium.org/26775007/diff/103001/chrome/browser/ui/views/...
chrome/browser/ui/views/translate/translate_bubble_view.cc:305: default:
Same comment about removing.

https://codereview.chromium.org/26775007/diff/103001/chrome/browser/ui/views/...
chrome/browser/ui/views/translate/translate_bubble_view.cc:361: default:
Here and in all.

https://codereview.chromium.org/26775007/diff/103001/chrome/browser/ui/views/...
File chrome/browser/ui/views/translate/translate_bubble_view.h (right):

https://codereview.chromium.org/26775007/diff/103001/chrome/browser/ui/views/...
chrome/browser/ui/views/translate/translate_bubble_view.h:48: virtual
~TranslateBubbleView();
constructor is typically before destructor. Also, constructor/destructor before
other methods (including statics). Lastly, why is the constructor public if you
have ShowBubble?

[hajimehoshi, 2013-10-17 05:45:41]

Thank you!

https://codereview.chromium.org/26775007/diff/103001/chrome/browser/ui/transl...
File chrome/browser/ui/translate/language_combobox_model.h (right):

https://codereview.chromium.org/26775007/diff/103001/chrome/browser/ui/transl...
chrome/browser/ui/translate/language_combobox_model.h:17: // user interface to
select an original language or a target.
On 2013/10/16 14:32:36, sky wrote:
> 'an original language or a target' -> language

Done.

https://codereview.chromium.org/26775007/diff/103001/chrome/browser/ui/transl...
File chrome/browser/ui/translate/translate_bubble_model.h (right):

https://codereview.chromium.org/26775007/diff/103001/chrome/browser/ui/transl...
chrome/browser/ui/translate/translate_bubble_model.h:21: ADVANCED,
On 2013/10/16 14:32:36, sky wrote:
> Document ADVANCED. It's not clear how the user gets there, or gets out of it.

Done.

https://codereview.chromium.org/26775007/diff/103001/chrome/browser/ui/transl...
File chrome/browser/ui/translate/translate_bubble_model_impl.h (right):

https://codereview.chromium.org/26775007/diff/103001/chrome/browser/ui/transl...
chrome/browser/ui/translate/translate_bubble_model_impl.h:18: explicit
TranslateBubbleModelImpl(
On 2013/10/16 14:32:36, sky wrote:
> no explicit

Done.

https://codereview.chromium.org/26775007/diff/103001/chrome/browser/ui/views/...
File chrome/browser/ui/views/translate/translate_bubble_view.cc (right):

https://codereview.chromium.org/26775007/diff/103001/chrome/browser/ui/views/...
chrome/browser/ui/views/translate/translate_bubble_view.cc:66:
TranslateTabHelper::FromWebContents(web_contents);
On 2013/10/16 14:32:36, sky wrote:
> nit: indent 2 more

Done.

https://codereview.chromium.org/26775007/diff/103001/chrome/browser/ui/views/...
chrome/browser/ui/views/translate/translate_bubble_view.cc:67: if
(!translate_tab_helper)
Right. Simply removed this.

https://codereview.chromium.org/26775007/diff/103001/chrome/browser/ui/views/...
chrome/browser/ui/views/translate/translate_bubble_view.cc:72: *target =
TranslateManager::GetLanguageCode(*target);
On 2013/10/16 14:32:36, sky wrote:
> nit: make this one line and have TM::GetLanguageCode take g_browser_pr...

Done.

https://codereview.chromium.org/26775007/diff/103001/chrome/browser/ui/views/...
chrome/browser/ui/views/translate/translate_bubble_view.cc:104: return
items_.at(index);
On 2013/10/16 14:32:36, sky wrote:
> items_[index]?

Done.

https://codereview.chromium.org/26775007/diff/103001/chrome/browser/ui/views/...
chrome/browser/ui/views/translate/translate_bubble_view.cc:107: return
items_.at(index).empty();
On 2013/10/16 14:32:36, sky wrote:
> items_[index]?

Done.

https://codereview.chromium.org/26775007/diff/103001/chrome/browser/ui/views/...
chrome/browser/ui/views/translate/translate_bubble_view.cc:123:
TranslateBubbleView* TranslateBubbleView::translate_bubble_view_;
On 2013/10/16 14:32:36, sky wrote:
> Style guide says statics have:
> // static
> on previous line.
> 
> Also, you need to set to NULL here.

Done.

https://codereview.chromium.org/26775007/diff/103001/chrome/browser/ui/views/...
chrome/browser/ui/views/translate/translate_bubble_view.cc:250: default:
On 2013/10/16 14:32:36, sky wrote:
> Can this be removed?

Done.

https://codereview.chromium.org/26775007/diff/103001/chrome/browser/ui/views/...
chrome/browser/ui/views/translate/translate_bubble_view.cc:263: int height =
child_at(model_->GetViewState())->GetPreferredSize().height();
On 2013/10/16 14:32:36, sky wrote:
> Assuming a particular view order is fragile. Can you make the view you care
> about a member and directly access it?

Done.

https://codereview.chromium.org/26775007/diff/103001/chrome/browser/ui/views/...
chrome/browser/ui/views/translate/translate_bubble_view.cc:305: default:
On 2013/10/16 14:32:36, sky wrote:
> Same comment about removing.

Done.

https://codereview.chromium.org/26775007/diff/103001/chrome/browser/ui/views/...
chrome/browser/ui/views/translate/translate_bubble_view.cc:361: default:
On 2013/10/16 14:32:36, sky wrote:
> Here and in all.

Done.

https://codereview.chromium.org/26775007/diff/103001/chrome/browser/ui/views/...
File chrome/browser/ui/views/translate/translate_bubble_view.h (right):

https://codereview.chromium.org/26775007/diff/103001/chrome/browser/ui/views/...
chrome/browser/ui/views/translate/translate_bubble_view.h:48: virtual
~TranslateBubbleView();
On 2013/10/16 14:32:36, sky wrote:
> constructor is typically before destructor. Also, constructor/destructor
before
> other methods (including statics). Lastly, why is the constructor public if
you
> have ShowBubble?

Done.

[sky, 2013-10-17 15:39:26]

LGTM

[commit-bot: I haz the power, 2013-10-17 15:56:15]

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

[hajimehoshi, 2013-10-17 16:07:34]

Ah, I found that ERROR is already defined in Win32 environment... I'll add a
prefix "VIEW_STATE_" to each enum names.

[commit-bot: I haz the power, 2013-10-18 03:29:12]

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

[commit-bot: I haz the power, 2013-10-18 18:12:56]

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=win&number...

[commit-bot: I haz the power, 2013-10-18 19:00:31]

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

[commit-bot: I haz the power, 2013-10-18 21:14:37]

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

[commit-bot: I haz the power, 2013-10-19 13:52:41]

Change committed as 229555