Issue 15297006: Translate (Linux): add after infobar UI for server side language detection

Issue 15297006: Translate (Linux): add after infobar UI for server side language detection (Closed)

Created:
7 years, 7 months ago by Takashi Toyoshima

Modified:
7 years, 7 months ago

Reviewers:
Takashi Toyoshima, Peter Kasting, Evan Stade

CC:
chromium-reviews

Base URL:
svn://svn.chromium.org/chrome/trunk/src

Visibility:
Public.

More Reviews

Description

Translate (Linux): add after infobar UI for server side language detection After enabling context menu to translate unknown language, Chrome needs to modify infobar UX for Chrome Translate. This change fixes Linux/Gtk UI. BUG=172593 TBR=pkasting@chromium.org, rohitrao@chromium.org Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=201807

Patch Set 1 #

Total comments: 2

Patch Set 2 : fix style #

Patch Set 3 : (rebase) #

Patch Set 4 : remove debug log #

Patch Set 5 : (rebase) #

Patch Set 6 : renaming #

Total comments: 6

Created: 7 years, 7 months ago

Download [raw] [tar.bz2]

	Unified diffs	Side-by-side diffs	Delta from patch set	Stats (+75 lines, -61 lines)			Patch
M	chrome/app/generated_resources.grd	View		1 chunk	+1 line, -1 line	0 comments	Download
M	chrome/browser/translate/translate_infobar_delegate.h	View	1 2 3 4 5	1 chunk	+11 lines, -14 lines	0 comments	Download
M	chrome/browser/translate/translate_infobar_delegate.cc	View	1 2 3 4 5	1 chunk	+30 lines, -28 lines	2 comments	Download
M	chrome/browser/ui/cocoa/infobars/after_translate_infobar_controller.mm	View	1 2 3 4 5	1 chunk	+1 line, -1 line	0 comments	Download
M	chrome/browser/ui/gtk/infobars/after_translate_infobar_gtk.cc	View	1 2 3 4 5	3 chunks	+29 lines, -16 lines	4 comments	Download
M	chrome/browser/ui/views/infobars/after_translate_infobar.cc	View	1 2 3 4 5	1 chunk	+3 lines, -1 line	0 comments	Download

Messages

Total messages: 8 (0 generated)

Expand Messages | Collapse Messages

Takashi Toyoshima

Hi estade, This is an additional change for gtk specific UI modification. It is related ...

7 years, 7 months ago (2013-05-17 14:13:27 UTC) #1

Evan Stade

lgtm https://codereview.chromium.org/15297006/diff/1/chrome/browser/ui/gtk/infobars/after_translate_infobar_gtk.cc File chrome/browser/ui/gtk/infobars/after_translate_infobar_gtk.cc (right): https://codereview.chromium.org/15297006/diff/1/chrome/browser/ui/gtk/infobars/after_translate_infobar_gtk.cc#newcode71 chrome/browser/ui/gtk/infobars/after_translate_infobar_gtk.cc:71: (swapped_language_combos || can you keep swapped_language_combos || translate_with_auto ...

7 years, 7 months ago (2013-05-18 23:48:50 UTC) #2

Takashi Toyoshima

thanks. https://codereview.chromium.org/15297006/diff/1/chrome/browser/ui/gtk/infobars/after_translate_infobar_gtk.cc File chrome/browser/ui/gtk/infobars/after_translate_infobar_gtk.cc (right): https://codereview.chromium.org/15297006/diff/1/chrome/browser/ui/gtk/infobars/after_translate_infobar_gtk.cc#newcode71 chrome/browser/ui/gtk/infobars/after_translate_infobar_gtk.cc:71: (swapped_language_combos || On 2013/05/18 23:48:50, Evan Stade wrote: ...

7 years, 7 months ago (2013-05-20 06:08:46 UTC) #3

Takashi Toyoshima

Adopt pkasting suggestion from Win UI review. Add TBR pkasting and rohitrao because this change ...

7 years, 7 months ago (2013-05-23 06:16:49 UTC) #4

commit-bot: I haz the power

CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/toyoshim@chromium.org/15297006/13001

7 years, 7 months ago (2013-05-23 06:17:31 UTC) #5

Peter Kasting

https://codereview.chromium.org/15297006/diff/13001/chrome/browser/translate/translate_infobar_delegate.cc File chrome/browser/translate/translate_infobar_delegate.cc (right): https://codereview.chromium.org/15297006/diff/13001/chrome/browser/translate/translate_infobar_delegate.cc#newcode297 chrome/browser/translate/translate_infobar_delegate.cc:297: } else { Tiny nit: Might be better to ...

7 years, 7 months ago (2013-05-23 19:59:56 UTC) #7

Takashi Toyoshima

7 years, 7 months ago (2013-05-27 06:07:51 UTC) #8

Message was sent while issue was closed.

Thanks.
I'll fix them in https://codereview.chromium.org/15757010/

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

https://codereview.chromium.org/15297006/diff/13001/chrome/browser/translate/...
chrome/browser/translate/translate_infobar_delegate.cc:297: } else {
On 2013/05/23 19:59:56, Peter Kasting wrote:
> Tiny nit: Might be better to early-return here so the rest of the function can
> be unindented (and look exactly as it did previously)

Done.

https://codereview.chromium.org/15297006/diff/13001/chrome/browser/ui/gtk/inf...
File chrome/browser/ui/gtk/infobars/after_translate_infobar_gtk.cc (right):

https://codereview.chromium.org/15297006/diff/13001/chrome/browser/ui/gtk/inf...
chrome/browser/ui/gtk/infobars/after_translate_infobar_gtk.cc:41:
DCHECK(strings.size() == 3U);
On 2013/05/23 19:59:56, Peter Kasting wrote:
> Nit: Simpler and uses DCHECK_EQ():
> 
>   DCHECK_EQ(autodetermined_source_language ? 2U : 3U, strings.size());

Done.

https://codereview.chromium.org/15297006/diff/13001/chrome/browser/ui/gtk/inf...
chrome/browser/ui/gtk/infobars/after_translate_infobar_gtk.cc:68:
gtk_box_pack_start(GTK_BOX(hbox),
On 2013/05/23 19:59:56, Peter Kasting wrote:
> Nit: Strange wrapping.  How about:
> 
>   gtk_box_pack_start(
>       GTK_BOX(hbox),
>       (swapped_language_combos || autodetermined_source_language) ?
>           target_lang_combo : original_lang_combo, FALSE, FALSE, 0);
> 
> (Optional: Can move "FALSE, FALSE, 0);" to a subsequent line indented even
with
> first arg.)

Done.

Expand Messages | Collapse Messages