Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(71)

Issue 64823005: Copy the implementations of TranslateUIDelegate to TransalteInfobarDelegate (Closed)

Created:
7 years, 1 month ago by hajimehoshi
Modified:
7 years, 1 month ago
CC:
chromium-reviews, Miguel Garcia, droger
Base URL:
https://chromium.googlesource.com/chromium/src.git@master
Visibility:
Public.

Description

Copy the implementations of TranslateUIDelegate to TransalteInfobarDelegate TranslateInfobarDelegate won't own a WebContents by https://codereview.chromium.org/22694006/, while TranslateUIDelegate's implementation relies on the existence of a WebContents. Thus, I copied a part of implementations of TranslateUIDelegate to TranslteInfobarDelegate. BUG=62154 TEST=browser_test --gtest_filter=TranslateManagerBrowserTest.* Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=234759

Patch Set 1 : . #

Total comments: 12

Patch Set 2 : Peter's review #

Total comments: 10

Patch Set 3 : Peter's review (2) #

Total comments: 5

Patch Set 4 : (rebasing) #

Unified diffs Side-by-side diffs Delta from patch set Stats (+244 lines, -95 lines) Patch
M chrome/browser/translate/translate_browser_metrics.h View 1 chunk +8 lines, -0 lines 0 comments Download
M chrome/browser/translate/translate_browser_metrics.cc View 2 chunks +24 lines, -0 lines 0 comments Download
M chrome/browser/translate/translate_infobar_delegate.h View 1 4 chunks +28 lines, -13 lines 0 comments Download
M chrome/browser/translate/translate_infobar_delegate.cc View 1 2 7 chunks +110 lines, -19 lines 0 comments Download
M chrome/browser/translate/translate_manager_browsertest.cc View 1 2 3 1 chunk +1 line, -0 lines 0 comments Download
M chrome/browser/translate/translate_ui_delegate.h View 1 2 3 chunks +16 lines, -13 lines 0 comments Download
M chrome/browser/translate/translate_ui_delegate.cc View 1 2 9 chunks +57 lines, -50 lines 0 comments Download

Messages

Total messages: 14 (0 generated)
hajimehoshi
Can you take a look? Thanks.
7 years, 1 month ago (2013-11-08 09:26:16 UTC) #1
Peter Kasting
LGTM https://codereview.chromium.org/64823005/diff/100001/chrome/browser/translate/translate_infobar_delegate.h File chrome/browser/translate/translate_infobar_delegate.h (right): https://codereview.chromium.org/64823005/diff/100001/chrome/browser/translate/translate_infobar_delegate.h#newcode74 chrome/browser/translate/translate_infobar_delegate.h:74: // updated. Let's not put these comments into ...
7 years, 1 month ago (2013-11-08 23:47:47 UTC) #2
hajimehoshi
Thanks! https://codereview.chromium.org/64823005/diff/100001/chrome/browser/translate/translate_infobar_delegate.h File chrome/browser/translate/translate_infobar_delegate.h (right): https://codereview.chromium.org/64823005/diff/100001/chrome/browser/translate/translate_infobar_delegate.h#newcode74 chrome/browser/translate/translate_infobar_delegate.h:74: // updated. On 2013/11/08 23:47:47, Peter Kasting wrote: ...
7 years, 1 month ago (2013-11-11 04:01:01 UTC) #3
hajimehoshi
I'll wait for Toyoshima-san's response just in case.
7 years, 1 month ago (2013-11-11 04:02:01 UTC) #4
Peter Kasting
(Still LGTM) https://codereview.chromium.org/64823005/diff/190001/chrome/browser/translate/translate_infobar_delegate.cc File chrome/browser/translate/translate_infobar_delegate.cc (right): https://codereview.chromium.org/64823005/diff/190001/chrome/browser/translate/translate_infobar_delegate.cc#newcode44 chrome/browser/translate/translate_infobar_delegate.cc:44: // updated. Nit: Don't bother with this ...
7 years, 1 month ago (2013-11-12 00:03:26 UTC) #5
hajimehoshi
Thank you! https://codereview.chromium.org/64823005/diff/190001/chrome/browser/translate/translate_infobar_delegate.cc File chrome/browser/translate/translate_infobar_delegate.cc (right): https://codereview.chromium.org/64823005/diff/190001/chrome/browser/translate/translate_infobar_delegate.cc#newcode44 chrome/browser/translate/translate_infobar_delegate.cc:44: // updated. On 2013/11/12 00:03:27, Peter Kasting ...
7 years, 1 month ago (2013-11-12 03:07:25 UTC) #6
Takashi Toyoshima
LGTM with optional nits.
7 years, 1 month ago (2013-11-12 22:16:37 UTC) #7
Takashi Toyoshima
https://codereview.chromium.org/64823005/diff/360001/chrome/browser/translate/translate_browser_metrics.cc File chrome/browser/translate/translate_browser_metrics.cc (right): https://codereview.chromium.org/64823005/diff/360001/chrome/browser/translate/translate_browser_metrics.cc#newcode29 chrome/browser/translate/translate_browser_metrics.cc:29: const char kTranslateRevertTranslation[] = "Translate.RevertTranslation"; Ah, I see. The ...
7 years, 1 month ago (2013-11-12 22:16:55 UTC) #8
hajimehoshi
Thank you! https://codereview.chromium.org/64823005/diff/360001/chrome/browser/translate/translate_browser_metrics.cc File chrome/browser/translate/translate_browser_metrics.cc (right): https://codereview.chromium.org/64823005/diff/360001/chrome/browser/translate/translate_browser_metrics.cc#newcode29 chrome/browser/translate/translate_browser_metrics.cc:29: const char kTranslateRevertTranslation[] = "Translate.RevertTranslation"; On 2013/11/12 ...
7 years, 1 month ago (2013-11-13 01:15:20 UTC) #9
Takashi Toyoshima
ok, that's optional. please go ahead.
7 years, 1 month ago (2013-11-13 01:17:07 UTC) #10
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/hajimehoshi@chromium.org/64823005/430001
7 years, 1 month ago (2013-11-13 01:21:01 UTC) #11
commit-bot: I haz the power
Step "update" is always a major failure. Look at the try server FAQ for more ...
7 years, 1 month ago (2013-11-13 01:48:28 UTC) #12
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/hajimehoshi@chromium.org/64823005/430001
7 years, 1 month ago (2013-11-13 02:15:57 UTC) #13
commit-bot: I haz the power
7 years, 1 month ago (2013-11-13 05:07:14 UTC) #14
Message was sent while issue was closed.
Change committed as 234759

Powered by Google App Engine
This is Rietveld 408576698