Added the 'Translation Logs' tab to chrome://translate-internals/

Added the 'Translation Logs' tab to chrome://translate-internals/ 

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

[hajimehoshi, 2013-05-07 08:32:17]

Can you take a look this CL?

mad:
Can you take a look at chrome/browser/translate/* and
chrome/renderer/translate/*?

estade:
Can you take a look at chrome/browser/autofill/*,
chrome/browser/resources/translate_internals/* and
chrome/browser/ui/webui/translate_internals/*?

sky:
I'd like you to review chrome/chrome_common.gypi and
chrome/browser/extensions/api/tabs/tabs_api.cc.

cdn:
Please look at chrome/common/*.

Thank you in advance.

[sky, 2013-05-07 14:46:00]

LGTM

[Cris Neckar, 2013-05-07 19:33:38]

ipc changes lgtm

[MAD, 2013-05-07 19:49:05]

A few comments...

Thanks!

BYE
MAD

https://chromiumcodereview.appspot.com/14494004/diff/7001/chrome/browser/tran...
File chrome/browser/translate/translate_tab_helper.h (right):

https://chromiumcodereview.appspot.com/14494004/diff/7001/chrome/browser/tran...
chrome/browser/translate/translate_tab_helper.h:13: class GURL;
This is not needed. Right?

https://chromiumcodereview.appspot.com/14494004/diff/7001/chrome/renderer/tra...
File chrome/renderer/translate/translate_helper.h (right):

https://chromiumcodereview.appspot.com/14494004/diff/7001/chrome/renderer/tra...
chrome/renderer/translate/translate_helper.h:112: std::string* cld_language =
NULL,
We don't use default value for function arguments in Chrome.

https://chromiumcodereview.appspot.com/14494004/diff/7001/chrome/renderer/tra...
chrome/renderer/translate/translate_helper.h:123: // |is_cld_reliable| is
optional. This will be set as true is CLD saids the
is CLD saids -> if CLD says.

https://chromiumcodereview.appspot.com/14494004/diff/7001/chrome/renderer/tra...
chrome/renderer/translate/translate_helper.h:124: // detection is reliable,
otherwise not.
No need for otherwise in comments when it's bool.

https://chromiumcodereview.appspot.com/14494004/diff/7001/chrome/renderer/tra...
chrome/renderer/translate/translate_helper.h:126: bool* is_cld_reliable = NULL);
Again.

[hajimehoshi, 2013-05-08 04:06:16]

Thank you!

https://codereview.chromium.org/14494004/diff/7001/chrome/browser/translate/t...
File chrome/browser/translate/translate_tab_helper.h (right):

https://codereview.chromium.org/14494004/diff/7001/chrome/browser/translate/t...
chrome/browser/translate/translate_tab_helper.h:13: class GURL;
Right.  Thanks.

On 2013/05/07 19:49:05, MAD wrote:
> This is not needed. Right?

https://codereview.chromium.org/14494004/diff/7001/chrome/renderer/translate/...
File chrome/renderer/translate/translate_helper.h (right):

https://codereview.chromium.org/14494004/diff/7001/chrome/renderer/translate/...
chrome/renderer/translate/translate_helper.h:112: std::string* cld_language =
NULL,
Done.  Added a test to translate_helper_unittest.cc to check these returned
values.

On 2013/05/07 19:49:05, MAD wrote:
> We don't use default value for function arguments in Chrome.

https://codereview.chromium.org/14494004/diff/7001/chrome/renderer/translate/...
chrome/renderer/translate/translate_helper.h:123: // |is_cld_reliable| is
optional. This will be set as true is CLD saids the
On 2013/05/07 19:49:05, MAD wrote:
> is CLD saids -> if CLD says.

Done.

https://codereview.chromium.org/14494004/diff/7001/chrome/renderer/translate/...
chrome/renderer/translate/translate_helper.h:124: // detection is reliable,
otherwise not.
On 2013/05/07 19:49:05, MAD wrote:
> No need for otherwise in comments when it's bool.

Done.

https://codereview.chromium.org/14494004/diff/7001/chrome/renderer/translate/...
chrome/renderer/translate/translate_helper.h:126: bool* is_cld_reliable = NULL);
On 2013/05/07 19:49:05, MAD wrote:
> Again.

Done.

[Evan Stade, 2013-05-09 04:39:00]

https://codereview.chromium.org/14494004/diff/22001/chrome/browser/resources/...
File chrome/browser/resources/translate_internals/detection_logs.html (right):

https://codereview.chromium.org/14494004/diff/22001/chrome/browser/resources/...
chrome/browser/resources/translate_internals/detection_logs.html:13: <th
class="detection-logs-content-language">Language detected by the content</th>
80

https://codereview.chromium.org/14494004/diff/22001/chrome/browser/resources/...
File chrome/browser/resources/translate_internals/translate_internals.css
(right):

https://codereview.chromium.org/14494004/diff/22001/chrome/browser/resources/...
chrome/browser/resources/translate_internals/translate_internals.css:84:
.detection-logs-is-cld-relialbe {
spelling

https://codereview.chromium.org/14494004/diff/22001/chrome/browser/resources/...
File chrome/browser/resources/translate_internals/translate_internals.js
(right):

https://codereview.chromium.org/14494004/diff/22001/chrome/browser/resources/...
chrome/browser/resources/translate_internals/translate_internals.js:123: * Addes
'0's to |number| as a string.  |width| is a number of '0'.
width is not the number of 0s. It's the total number of digits.

https://codereview.chromium.org/14494004/diff/22001/chrome/browser/resources/...
chrome/browser/resources/translate_internals/translate_internals.js:135: for
(var i = 0; i < restWidth; i++)
Array(restWidth).join('0') + numberStr;

https://codereview.chromium.org/14494004/diff/22001/chrome/browser/resources/...
chrome/browser/resources/translate_internals/translate_internals.js:143: *
'2006-01-02 15:04:05'.  The user's locale is used.
the user's locale is used? What does that mean? locale-specific formatting for
the date/time is NOT used.

https://codereview.chromium.org/14494004/diff/22001/chrome/browser/resources/...
chrome/browser/resources/translate_internals/translate_internals.js:146: *
@return {string} the formatted string
comments should sentence-like (punctuation + capitalization).

https://codereview.chromium.org/14494004/diff/22001/chrome/browser/resources/...
chrome/browser/resources/translate_internals/translate_internals.js:187: td =
document.createElement('td');
you should create a local function for this instead of repeating the 4 lines
over and over

[hajimehoshi, 2013-05-09 05:59:40]

Thank you!

https://codereview.chromium.org/14494004/diff/22001/chrome/browser/resources/...
File chrome/browser/resources/translate_internals/detection_logs.html (right):

https://codereview.chromium.org/14494004/diff/22001/chrome/browser/resources/...
chrome/browser/resources/translate_internals/detection_logs.html:13: <th
class="detection-logs-content-language">Language detected by the content</th>
On 2013/05/09 04:39:00, Evan Stade wrote:
> 80

Done.

https://codereview.chromium.org/14494004/diff/22001/chrome/browser/resources/...
File chrome/browser/resources/translate_internals/translate_internals.css
(right):

https://codereview.chromium.org/14494004/diff/22001/chrome/browser/resources/...
chrome/browser/resources/translate_internals/translate_internals.css:84:
.detection-logs-is-cld-relialbe {
On 2013/05/09 04:39:00, Evan Stade wrote:
> spelling

Done.

https://codereview.chromium.org/14494004/diff/22001/chrome/browser/resources/...
File chrome/browser/resources/translate_internals/translate_internals.js
(right):

https://codereview.chromium.org/14494004/diff/22001/chrome/browser/resources/...
chrome/browser/resources/translate_internals/translate_internals.js:123: * Addes
'0's to |number| as a string.  |width| is a number of '0'.
On 2013/05/09 04:39:00, Evan Stade wrote:
> width is not the number of 0s. It's the total number of digits.

Done.

https://codereview.chromium.org/14494004/diff/22001/chrome/browser/resources/...
chrome/browser/resources/translate_internals/translate_internals.js:135: for
(var i = 0; i < restWidth; i++)
Array(restWidth).join('0') produces the string whose length is restWidth - 1,
right?

On 2013/05/09 04:39:00, Evan Stade wrote:
> Array(restWidth).join('0') + numberStr;

https://codereview.chromium.org/14494004/diff/22001/chrome/browser/resources/...
chrome/browser/resources/translate_internals/translate_internals.js:143: *
'2006-01-02 15:04:05'.  The user's locale is used.
I meant that the user's timezone will be used.  Anyway, I'll remove the sentence
because it could be confusion.

On 2013/05/09 04:39:00, Evan Stade wrote:
> the user's locale is used? What does that mean? locale-specific formatting for
> the date/time is NOT used.

https://codereview.chromium.org/14494004/diff/22001/chrome/browser/resources/...
chrome/browser/resources/translate_internals/translate_internals.js:146: *
@return {string} the formatted string
On 2013/05/09 04:39:00, Evan Stade wrote:
> comments should sentence-like (punctuation + capitalization).

Done.

https://codereview.chromium.org/14494004/diff/22001/chrome/browser/resources/...
chrome/browser/resources/translate_internals/translate_internals.js:187: td =
document.createElement('td');
On 2013/05/09 04:39:00, Evan Stade wrote:
> you should create a local function for this instead of repeating the 4 lines
> over and over

Done.

[MAD, 2013-05-09 14:10:29]

chrome/[browser|renderer]/translate/* LGTM

Thanks!

BYE
MAD...

[hajimehoshi, 2013-05-14 01:12:24]

ping +estade

On 2013/05/09 14:10:29, MAD wrote:
> chrome/[browser|renderer]/translate/* LGTM
> 
> Thanks!
> 
> BYE
> MAD...

[Evan Stade, 2013-05-18 23:42:58]

lgtm with comments

https://codereview.chromium.org/14494004/diff/50001/chrome/browser/resources/...
File chrome/browser/resources/translate_internals/translate_internals.js
(right):

https://codereview.chromium.org/14494004/diff/50001/chrome/browser/resources/...
chrome/browser/resources/translate_internals/translate_internals.js:123: * Addes
'0's to |number| as a string.  |width| is length of the
please keep the number of spaces after a period consistent in this file.

https://codereview.chromium.org/14494004/diff/50001/chrome/browser/resources/...
chrome/browser/resources/translate_internals/translate_internals.js:130:
function fillWithZeros(number, width) {
nit: call the function padWithZeros

https://codereview.chromium.org/14494004/diff/50001/chrome/browser/resources/...
chrome/browser/resources/translate_internals/translate_internals.js:136:
Array(restWidth + 1).join('0') + numberStr;
you seem to be missing an assignment here? did you mean to return this value?

https://codereview.chromium.org/14494004/diff/50001/chrome/browser/ui/webui/t...
File chrome/browser/ui/webui/translate_internals/translate_internals_handler.cc
(right):

https://codereview.chromium.org/14494004/diff/50001/chrome/browser/ui/webui/t...
chrome/browser/ui/webui/translate_internals/translate_internals_handler.cc:22:
TranslateManager::GetInstance()->AddObserver(this);
nit: I think it's preferable to use ScopedObserver

[hajimehoshi, 2013-05-20 01:11:34]

Thank you!

https://codereview.chromium.org/14494004/diff/50001/chrome/browser/resources/...
File chrome/browser/resources/translate_internals/translate_internals.js
(right):

https://codereview.chromium.org/14494004/diff/50001/chrome/browser/resources/...
chrome/browser/resources/translate_internals/translate_internals.js:123: * Addes
'0's to |number| as a string.  |width| is length of the
On 2013/05/18 23:42:58, Evan Stade wrote:
> please keep the number of spaces after a period consistent in this file.

Done.

https://codereview.chromium.org/14494004/diff/50001/chrome/browser/resources/...
chrome/browser/resources/translate_internals/translate_internals.js:130:
function fillWithZeros(number, width) {
On 2013/05/18 23:42:58, Evan Stade wrote:
> nit: call the function padWithZeros

Done.

https://codereview.chromium.org/14494004/diff/50001/chrome/browser/resources/...
chrome/browser/resources/translate_internals/translate_internals.js:136:
Array(restWidth + 1).join('0') + numberStr;
Right, I meant to return this...

On 2013/05/18 23:42:58, Evan Stade wrote:
> you seem to be missing an assignment here? did you mean to return this value?

[commit-bot: I haz the power, 2013-05-20 01:31:40]

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

[commit-bot: I haz the power, 2013-05-20 03:11:22]

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-05-20 03:28:04]

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

[commit-bot: I haz the power, 2013-05-20 05:07:43]

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

[commit-bot: I haz the power, 2013-05-20 05:40:27]

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

[commit-bot: I haz the power, 2013-05-20 07:15:00]

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

[commit-bot: I haz the power, 2013-05-20 07:53:45]

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

[commit-bot: I haz the power, 2013-05-20 09:33:29]

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

[Takashi Toyoshima, 2013-05-20 10:20:05]

PageLanguageDetection fails in tries and it looks a real failure.
Please check and fix it.

[commit-bot: I haz the power, 2013-05-21 04:21:37]

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

[hajimehoshi, 2013-05-21 04:21:57]

Right.  The type of the message (NOTIFICATION_TAB_LANGUAGE_DETERMINED) was
changed, and some types in the tests didn't match it.

On 2013/05/20 10:20:05, Takashi Toyoshima (chromium) wrote:
> PageLanguageDetection fails in tries and it looks a real failure.
> Please check and fix it.

[commit-bot: I haz the power, 2013-05-21 09:30:48]

Failed to apply patch for chrome/browser/translate/translate_tab_helper.cc:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file chrome/browser/translate/translate_tab_helper.cc
  Hunk #2 FAILED at 42.
  1 out of 2 hunks FAILED -- saving rejects to file
chrome/browser/translate/translate_tab_helper.cc.rej

Patch:       chrome/browser/translate/translate_tab_helper.cc
Index: chrome/browser/translate/translate_tab_helper.cc
diff --git a/chrome/browser/translate/translate_tab_helper.cc
b/chrome/browser/translate/translate_tab_helper.cc
index
3a14a471ec12714af3f4b2938769784c22920867..4dd71bba104575d629d4d060db84468f2a898ffa
100644
--- a/chrome/browser/translate/translate_tab_helper.cc
+++ b/chrome/browser/translate/translate_tab_helper.cc
@@ -6,6 +6,7 @@
 
 #include "chrome/browser/translate/page_translated_details.h"
 #include "chrome/common/chrome_notification_types.h"
+#include "chrome/common/language_detection_details.h"
 #include "chrome/common/render_messages.h"
 #include "content/public/browser/notification_service.h"
 #include "content/public/browser/web_contents.h"
@@ -41,15 +42,16 @@ void TranslateTabHelper::DidNavigateAnyFrame(
   language_state_.DidNavigate(details);
 }
 
-void TranslateTabHelper::OnLanguageDetermined(const std::string& language,
-                                              bool page_translatable) {
-  language_state_.LanguageDetermined(language, page_translatable);
+void TranslateTabHelper::OnLanguageDetermined(
+    const LanguageDetectionDetails& details,
+    bool page_translatable) {
+  language_state_.LanguageDetermined(details.adopted_language,
+                                     page_translatable);
 
-  std::string lang = language;
   content::NotificationService::current()->Notify(
       chrome::NOTIFICATION_TAB_LANGUAGE_DETERMINED,
       content::Source<WebContents>(web_contents()),
-      content::Details<std::string>(&lang));
+      content::Details<const LanguageDetectionDetails>(&details));
 }
 
 void TranslateTabHelper::OnPageTranslated(int32 page_id,

[commit-bot: I haz the power, 2013-05-21 10:25:52]

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

[commit-bot: I haz the power, 2013-05-21 16:40:54]

Change committed as 201314