Added chrome://translate-internals

Added chrome://translate-internals.  For now, this shows only the preferences concerned with the translation.

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

[hajimehoshi, 2013-04-12 08:00:47]

Can you take a look at this CL?

[Takashi Toyoshima, 2013-04-12 08:43:35]

Please write proper change title and description.
It's fine to go OWNERS review.

https://codereview.chromium.org/13842010/diff/1/chrome/browser/translate/tran...
File chrome/browser/translate/translate_prefs.h (right):

https://codereview.chromium.org/13842010/diff/1/chrome/browser/translate/tran...
chrome/browser/translate/translate_prefs.h:10: #include <vector>
why?

https://codereview.chromium.org/13842010/diff/1/chrome/browser/ui/webui/chrom...
File chrome/browser/ui/webui/chrome_web_ui_controller_factory.cc (right):

https://codereview.chromium.org/13842010/diff/1/chrome/browser/ui/webui/chrom...
chrome/browser/ui/webui/chrome_web_ui_controller_factory.cc:244: return
&NewWebUI<TranslateInternalsUI>; // tekito
Please remove // tekito :(

https://codereview.chromium.org/13842010/diff/1/chrome/browser/ui/webui/trans...
File chrome/browser/ui/webui/translate_internals/OWNERS (right):

https://codereview.chromium.org/13842010/diff/1/chrome/browser/ui/webui/trans...
chrome/browser/ui/webui/translate_internals/OWNERS:1: hajimehoshi@chromium.org
Only formal committer can be OWNERS

https://codereview.chromium.org/13842010/diff/1/chrome/browser/ui/webui/trans...
File chrome/browser/ui/webui/translate_internals/translate_internals_handler.cc
(right):

https://codereview.chromium.org/13842010/diff/1/chrome/browser/ui/webui/trans...
chrome/browser/ui/webui/translate_internals/translate_internals_handler.cc:47:
NULL,
I think we often use arraysize to run for loop on static array.

https://codereview.chromium.org/13842010/diff/1/chrome/browser/ui/webui/trans...
File chrome/browser/ui/webui/translate_internals/translate_internals_handler.h
(right):

https://codereview.chromium.org/13842010/diff/1/chrome/browser/ui/webui/trans...
chrome/browser/ui/webui/translate_internals/translate_internals_handler.h:21:
TranslateInternalsHandler();
You can write implementation here because the function is empty.

TranslateInternalsHandler() {}

https://codereview.chromium.org/13842010/diff/1/chrome/browser/ui/webui/trans...
chrome/browser/ui/webui/translate_internals/translate_internals_handler.h:22:
virtual ~TranslateInternalsHandler();
ditto.

https://codereview.chromium.org/13842010/diff/1/chrome/browser/ui/webui/trans...
chrome/browser/ui/webui/translate_internals/translate_internals_handler.h:24: //
content::WebUIMessageHandler implementation.
On implementing inherited methods, following style is recommended though this is
not widely known.

// content::WebUIMessageHandler:

or

// content::WebUIMessageHandler methods:

https://codereview.chromium.org/13842010/diff/1/chrome/browser/ui/webui/trans...
chrome/browser/ui/webui/translate_internals/translate_internals_handler.h:28:
void OnRequestInfo(const base::ListValue*);
argument name is missing.

https://codereview.chromium.org/13842010/diff/1/chrome/browser/ui/webui/trans...
File chrome/browser/ui/webui/translate_internals/translate_internals_ui.cc
(right):

https://codereview.chromium.org/13842010/diff/1/chrome/browser/ui/webui/trans...
chrome/browser/ui/webui/translate_internals/translate_internals_ui.cc:29:
source->SetJsonPath("strings.js"); // Why is this needed? I'm not sure.
If you want to ask reviewers, you should not write in source code, but in review
site comment.

[hajimehoshi, 2013-04-12 09:28:52]

I've fixed them.

https://codereview.chromium.org/13842010/diff/1/chrome/browser/translate/tran...
File chrome/browser/translate/translate_prefs.h (right):

https://codereview.chromium.org/13842010/diff/1/chrome/browser/translate/tran...
chrome/browser/translate/translate_prefs.h:10: #include <vector>
I don't remember... I'll remove them.

On 2013/04/12 08:43:35, Takashi Toyoshima (chromium) wrote:
> why?

https://codereview.chromium.org/13842010/diff/1/chrome/browser/ui/webui/chrom...
File chrome/browser/ui/webui/chrome_web_ui_controller_factory.cc (right):

https://codereview.chromium.org/13842010/diff/1/chrome/browser/ui/webui/chrom...
chrome/browser/ui/webui/chrome_web_ui_controller_factory.cc:244: return
&NewWebUI<TranslateInternalsUI>; // tekito
I'm really sorry.

On 2013/04/12 08:43:35, Takashi Toyoshima (chromium) wrote:
> Please remove // tekito :(

https://codereview.chromium.org/13842010/diff/1/chrome/browser/ui/webui/trans...
File chrome/browser/ui/webui/translate_internals/OWNERS (right):

https://codereview.chromium.org/13842010/diff/1/chrome/browser/ui/webui/trans...
chrome/browser/ui/webui/translate_internals/OWNERS:1: hajimehoshi@chromium.org
On 2013/04/12 08:43:35, Takashi Toyoshima (chromium) wrote:
> Only formal committer can be OWNERS

Done.

https://codereview.chromium.org/13842010/diff/1/chrome/browser/ui/webui/trans...
File chrome/browser/ui/webui/translate_internals/translate_internals_handler.h
(right):

https://codereview.chromium.org/13842010/diff/1/chrome/browser/ui/webui/trans...
chrome/browser/ui/webui/translate_internals/translate_internals_handler.h:21:
TranslateInternalsHandler();
On 2013/04/12 08:43:35, Takashi Toyoshima (chromium) wrote:
> You can write implementation here because the function is empty.
> 
> TranslateInternalsHandler() {}

Done.

https://codereview.chromium.org/13842010/diff/1/chrome/browser/ui/webui/trans...
chrome/browser/ui/webui/translate_internals/translate_internals_handler.h:22:
virtual ~TranslateInternalsHandler();
On 2013/04/12 08:43:35, Takashi Toyoshima (chromium) wrote:
> ditto.

Done.

https://codereview.chromium.org/13842010/diff/1/chrome/browser/ui/webui/trans...
chrome/browser/ui/webui/translate_internals/translate_internals_handler.h:24: //
content::WebUIMessageHandler implementation.
On 2013/04/12 08:43:35, Takashi Toyoshima (chromium) wrote:
> On implementing inherited methods, following style is recommended though this
is
> not widely known.
> 
> // content::WebUIMessageHandler:
> 
> or
> 
> // content::WebUIMessageHandler methods:

Done.

https://codereview.chromium.org/13842010/diff/1/chrome/browser/ui/webui/trans...
chrome/browser/ui/webui/translate_internals/translate_internals_handler.h:28:
void OnRequestInfo(const base::ListValue*);
On 2013/04/12 08:43:35, Takashi Toyoshima (chromium) wrote:
> argument name is missing.

Done.

[hajimehoshi, 2013-04-12 09:32:46]

+estade, can you take a look?  Thank you in advance.

On 2013/04/12 09:28:52, hajimehoshi wrote:
> I've fixed them.
> 
>
https://codereview.chromium.org/13842010/diff/1/chrome/browser/translate/tran...
> File chrome/browser/translate/translate_prefs.h (right):
> 
>
https://codereview.chromium.org/13842010/diff/1/chrome/browser/translate/tran...
> chrome/browser/translate/translate_prefs.h:10: #include <vector>
> I don't remember... I'll remove them.
> 
> On 2013/04/12 08:43:35, Takashi Toyoshima (chromium) wrote:
> > why?
> 
>
https://codereview.chromium.org/13842010/diff/1/chrome/browser/ui/webui/chrom...
> File chrome/browser/ui/webui/chrome_web_ui_controller_factory.cc (right):
> 
>
https://codereview.chromium.org/13842010/diff/1/chrome/browser/ui/webui/chrom...
> chrome/browser/ui/webui/chrome_web_ui_controller_factory.cc:244: return
> &NewWebUI<TranslateInternalsUI>; // tekito
> I'm really sorry.
> 
> On 2013/04/12 08:43:35, Takashi Toyoshima (chromium) wrote:
> > Please remove // tekito :(
> 
>
https://codereview.chromium.org/13842010/diff/1/chrome/browser/ui/webui/trans...
> File chrome/browser/ui/webui/translate_internals/OWNERS (right):
> 
>
https://codereview.chromium.org/13842010/diff/1/chrome/browser/ui/webui/trans...
> chrome/browser/ui/webui/translate_internals/OWNERS:1:
mailto:hajimehoshi@chromium.org
> On 2013/04/12 08:43:35, Takashi Toyoshima (chromium) wrote:
> > Only formal committer can be OWNERS
> 
> Done.
> 
>
https://codereview.chromium.org/13842010/diff/1/chrome/browser/ui/webui/trans...
> File chrome/browser/ui/webui/translate_internals/translate_internals_handler.h
> (right):
> 
>
https://codereview.chromium.org/13842010/diff/1/chrome/browser/ui/webui/trans...
> chrome/browser/ui/webui/translate_internals/translate_internals_handler.h:21:
> TranslateInternalsHandler();
> On 2013/04/12 08:43:35, Takashi Toyoshima (chromium) wrote:
> > You can write implementation here because the function is empty.
> > 
> > TranslateInternalsHandler() {}
> 
> Done.
> 
>
https://codereview.chromium.org/13842010/diff/1/chrome/browser/ui/webui/trans...
> chrome/browser/ui/webui/translate_internals/translate_internals_handler.h:22:
> virtual ~TranslateInternalsHandler();
> On 2013/04/12 08:43:35, Takashi Toyoshima (chromium) wrote:
> > ditto.
> 
> Done.
> 
>
https://codereview.chromium.org/13842010/diff/1/chrome/browser/ui/webui/trans...
> chrome/browser/ui/webui/translate_internals/translate_internals_handler.h:24:
//
> content::WebUIMessageHandler implementation.
> On 2013/04/12 08:43:35, Takashi Toyoshima (chromium) wrote:
> > On implementing inherited methods, following style is recommended though
this
> is
> > not widely known.
> > 
> > // content::WebUIMessageHandler:
> > 
> > or
> > 
> > // content::WebUIMessageHandler methods:
> 
> Done.
> 
>
https://codereview.chromium.org/13842010/diff/1/chrome/browser/ui/webui/trans...
> chrome/browser/ui/webui/translate_internals/translate_internals_handler.h:28:
> void OnRequestInfo(const base::ListValue*);
> On 2013/04/12 08:43:35, Takashi Toyoshima (chromium) wrote:
> > argument name is missing.
> 
> Done.

[Evan Stade, 2013-04-12 16:02:25]

https://codereview.chromium.org/13842010/diff/10001/chrome/browser/resources/...
File chrome/browser/resources/translate_internals/index.js (right):

https://codereview.chromium.org/13842010/diff/10001/chrome/browser/resources/...
chrome/browser/resources/translate_internals/index.js:21: console.log(detail);
you probably shouldn't put logs in, but if you do then you should make the
message more useful.

https://codereview.chromium.org/13842010/diff/10001/chrome/browser/resources/...
chrome/browser/resources/translate_internals/index.js:24: pre.innerHTML =
escapeHTML(content);
why don't you just use textContent = ?

https://codereview.chromium.org/13842010/diff/10001/chrome/browser/resources/...
chrome/browser/resources/translate_internals/index.js:29: * This function will
be called by the browser.
there's a style for js documentation which involves documenting each param and
its type. Please use it on this and every function in this file.

https://codereview.chromium.org/13842010/diff/10001/chrome/browser/resources/...
chrome/browser/resources/translate_internals/index.js:37: console.error('Unknown
message:', message);
nit: space after :

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

https://codereview.chromium.org/13842010/diff/10001/chrome/browser/ui/webui/t...
chrome/browser/ui/webui/translate_internals/translate_internals_handler.cc:43:
scoped_ptr<base::DictionaryValue> pref_values(prefs->GetPreferenceValues());
why copy into a dict? What's wrong with GetPreference()?

https://codereview.chromium.org/13842010/diff/10001/chrome/browser/ui/webui/t...
chrome/browser/ui/webui/translate_internals/translate_internals_handler.cc:49:
// The ownership of |copied_value| will be passed to |dict|.
instead of relying on a comment, you can write code that documents itself:

scoped_ptr<base::Value> copied_value(value->DeepCopy());
dict.Set(key, copied_value.release());

Or just

dict.Set(key, value->DeepCopy());

which I happen to think is self-evident enough.

https://codereview.chromium.org/13842010/diff/10001/chrome/browser/ui/webui/t...
chrome/browser/ui/webui/translate_internals/translate_internals_handler.cc:60:
scoped_ptr<base::Value> message_data(base::Value::CreateStringValue(message));
CreateStringValue is deprecated.

https://codereview.chromium.org/13842010/diff/10001/chrome/browser/ui/webui/t...
File chrome/browser/ui/webui/translate_internals/translate_internals_handler.h
(right):

https://codereview.chromium.org/13842010/diff/10001/chrome/browser/ui/webui/t...
chrome/browser/ui/webui/translate_internals/translate_internals_handler.h:28:
void OnRequestInfo(const base::ListValue* args);
docs

https://codereview.chromium.org/13842010/diff/10001/chrome/browser/ui/webui/t...
chrome/browser/ui/webui/translate_internals/translate_internals_handler.h:29:
void SendMessage(const std::string& message, const base::Value& value);
docs

https://codereview.chromium.org/13842010/diff/10001/chrome/browser/ui/webui/t...
File chrome/browser/ui/webui/translate_internals/translate_internals_ui.cc
(right):

https://codereview.chromium.org/13842010/diff/10001/chrome/browser/ui/webui/t...
chrome/browser/ui/webui/translate_internals/translate_internals_ui.cc:29:
source->SetJsonPath("strings.js"); // Why is this needed? I'm not sure.
this comment isn't useful.

You shouldn't need it since you never call AddString or any related function,
and you don't load the strings.js resource from your html.

https://codereview.chromium.org/13842010/diff/10001/chrome/browser/ui/webui/t...
File chrome/browser/ui/webui/translate_internals/translate_internals_ui.h
(right):

https://codereview.chromium.org/13842010/diff/10001/chrome/browser/ui/webui/t...
chrome/browser/ui/webui/translate_internals/translate_internals_ui.h:1: //
Copyright (c) 2013 The Chromium Authors. All rights reserved.
no (c)

https://codereview.chromium.org/13842010/diff/10001/chrome/browser/ui/webui/t...
chrome/browser/ui/webui/translate_internals/translate_internals_ui.h:9: class
TranslateInternalsUI : public content::WebUIController {
class level comment

[hajimehoshi, 2013-04-15 04:01:49]

Thank you for your reviewing.  I fixed these problems.  Can you take a look?

https://codereview.chromium.org/13842010/diff/10001/chrome/browser/resources/...
File chrome/browser/resources/translate_internals/index.js (right):

https://codereview.chromium.org/13842010/diff/10001/chrome/browser/resources/...
chrome/browser/resources/translate_internals/index.js:21: console.log(detail);
I used just for debugging, and forget to remove it.

On 2013/04/12 16:02:25, Evan Stade wrote:
> you probably shouldn't put logs in, but if you do then you should make the
> message more useful.

https://codereview.chromium.org/13842010/diff/10001/chrome/browser/resources/...
chrome/browser/resources/translate_internals/index.js:24: pre.innerHTML =
escapeHTML(content);
On 2013/04/12 16:02:25, Evan Stade wrote:
> why don't you just use textContent = ?

Done.

https://codereview.chromium.org/13842010/diff/10001/chrome/browser/resources/...
chrome/browser/resources/translate_internals/index.js:29: * This function will
be called by the browser.
On 2013/04/12 16:02:25, Evan Stade wrote:
> there's a style for js documentation which involves documenting each param and
> its type. Please use it on this and every function in this file.

Done.

https://codereview.chromium.org/13842010/diff/10001/chrome/browser/resources/...
chrome/browser/resources/translate_internals/index.js:37: console.error('Unknown
message:', message);
console.error adds space between the arugments, doesn't it?

On 2013/04/12 16:02:25, Evan Stade wrote:
> nit: space after :

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

https://codereview.chromium.org/13842010/diff/10001/chrome/browser/ui/webui/t...
chrome/browser/ui/webui/translate_internals/translate_internals_handler.cc:43:
scoped_ptr<base::DictionaryValue> pref_values(prefs->GetPreferenceValues());
On 2013/04/12 16:02:25, Evan Stade wrote:
> why copy into a dict? What's wrong with GetPreference()?

Done.

https://codereview.chromium.org/13842010/diff/10001/chrome/browser/ui/webui/t...
chrome/browser/ui/webui/translate_internals/translate_internals_handler.cc:49:
// The ownership of |copied_value| will be passed to |dict|.
On 2013/04/12 16:02:25, Evan Stade wrote:
> instead of relying on a comment, you can write code that documents itself:
> 
> scoped_ptr<base::Value> copied_value(value->DeepCopy());
> dict.Set(key, copied_value.release());
> 
> Or just
> 
> dict.Set(key, value->DeepCopy());
> 
> which I happen to think is self-evident enough.

Done.

https://codereview.chromium.org/13842010/diff/10001/chrome/browser/ui/webui/t...
chrome/browser/ui/webui/translate_internals/translate_internals_handler.cc:60:
scoped_ptr<base::Value> message_data(base::Value::CreateStringValue(message));
On 2013/04/12 16:02:25, Evan Stade wrote:
> CreateStringValue is deprecated.

Done.

https://codereview.chromium.org/13842010/diff/10001/chrome/browser/ui/webui/t...
File chrome/browser/ui/webui/translate_internals/translate_internals_handler.h
(right):

https://codereview.chromium.org/13842010/diff/10001/chrome/browser/ui/webui/t...
chrome/browser/ui/webui/translate_internals/translate_internals_handler.h:28:
void OnRequestInfo(const base::ListValue* args);
On 2013/04/12 16:02:25, Evan Stade wrote:
> docs

Done.

https://codereview.chromium.org/13842010/diff/10001/chrome/browser/ui/webui/t...
chrome/browser/ui/webui/translate_internals/translate_internals_handler.h:29:
void SendMessage(const std::string& message, const base::Value& value);
On 2013/04/12 16:02:25, Evan Stade wrote:
> docs

Done.

https://codereview.chromium.org/13842010/diff/10001/chrome/browser/ui/webui/t...
File chrome/browser/ui/webui/translate_internals/translate_internals_ui.cc
(right):

https://codereview.chromium.org/13842010/diff/10001/chrome/browser/ui/webui/t...
chrome/browser/ui/webui/translate_internals/translate_internals_ui.cc:29:
source->SetJsonPath("strings.js"); // Why is this needed? I'm not sure.
On 2013/04/12 16:02:25, Evan Stade wrote:
> this comment isn't useful.
> 
> You shouldn't need it since you never call AddString or any related function,
> and you don't load the strings.js resource from your html.

Done.

https://codereview.chromium.org/13842010/diff/10001/chrome/browser/ui/webui/t...
File chrome/browser/ui/webui/translate_internals/translate_internals_ui.h
(right):

https://codereview.chromium.org/13842010/diff/10001/chrome/browser/ui/webui/t...
chrome/browser/ui/webui/translate_internals/translate_internals_ui.h:1: //
Copyright (c) 2013 The Chromium Authors. All rights reserved.
On 2013/04/12 16:02:25, Evan Stade wrote:
> no (c)

Done.

https://codereview.chromium.org/13842010/diff/10001/chrome/browser/ui/webui/t...
chrome/browser/ui/webui/translate_internals/translate_internals_ui.h:9: class
TranslateInternalsUI : public content::WebUIController {
On 2013/04/12 16:02:25, Evan Stade wrote:
> class level comment

Done.

[Evan Stade, 2013-04-15 19:25:13]

https://codereview.chromium.org/13842010/diff/10001/chrome/browser/resources/...
File chrome/browser/resources/translate_internals/index.js (right):

https://codereview.chromium.org/13842010/diff/10001/chrome/browser/resources/...
chrome/browser/resources/translate_internals/index.js:37: console.error('Unknown
message:', message);
On 2013/04/15 04:01:49, hajimehoshi wrote:
> console.error adds space between the arugments, doesn't it?
> 
> On 2013/04/12 16:02:25, Evan Stade wrote:
> > nit: space after :
> 

oh, so it does.

https://codereview.chromium.org/13842010/diff/19001/chrome/browser/resources/...
File chrome/browser/resources/translate_internals/index.html (right):

https://codereview.chromium.org/13842010/diff/19001/chrome/browser/resources/...
chrome/browser/resources/translate_internals/index.html:26: <tabpanel
id="tabpanelPrefs">
classes and IDs use hyphens, i.e. tabpanel-prefs

http://dev.chromium.org/developers/web-development-style-guide

https://codereview.chromium.org/13842010/diff/19001/chrome/browser/resources/...
chrome/browser/resources/translate_internals/index.html:28: <pre></pre>
instead of using <pre>, it's probably better to use <p> and set the
presentational style in css (text-wrap: pre-wrap, etc.)

https://codereview.chromium.org/13842010/diff/19001/chrome/browser/resources/...
File chrome/browser/resources/translate_internals/index.js (right):

https://codereview.chromium.org/13842010/diff/19001/chrome/browser/resources/...
chrome/browser/resources/translate_internals/index.js:37: case 'prefsUpdated':
is this the proper indentation?

https://codereview.chromium.org/13842010/diff/19001/chrome/browser/resources/...
chrome/browser/resources/translate_internals/index.js:41: console.error('Unknown
message: ', message);
you are right, no need for space after :

https://codereview.chromium.org/13842010/diff/19001/chrome/browser/resources/...
chrome/browser/resources/translate_internals/index.js:58:
cr.translateInternals.initialize);
indent this argument to align with argument on previous line (assuming it fits)

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

https://codereview.chromium.org/13842010/diff/19001/chrome/browser/ui/webui/t...
chrome/browser/ui/webui/translate_internals/translate_internals_handler.cc:43:
for (unsigned i = 0; i < arraysize(keys); ++i) {
size_t

https://codereview.chromium.org/13842010/diff/19001/chrome/browser/ui/webui/t...
chrome/browser/ui/webui/translate_internals/translate_internals_handler.cc:48:
dict.Set(key, value->DeepCopy());
nit: combine these two lines

https://codereview.chromium.org/13842010/diff/19001/chrome/browser/ui/webui/t...
chrome/browser/ui/webui/translate_internals/translate_internals_handler.cc:58:
scoped_ptr<base::Value> message_data(new base::StringValue(message));
no need for heap allocation

https://codereview.chromium.org/13842010/diff/19001/chrome/browser/ui/webui/t...
File chrome/browser/ui/webui/translate_internals/translate_internals_ui.h
(right):

https://codereview.chromium.org/13842010/diff/19001/chrome/browser/ui/webui/t...
chrome/browser/ui/webui/translate_internals/translate_internals_ui.h:13: virtual
~TranslateInternalsUI() {}
\n

[hajimehoshi, 2013-04-16 02:01:42]

Fixed them.

https://codereview.chromium.org/13842010/diff/19001/chrome/browser/resources/...
File chrome/browser/resources/translate_internals/index.html (right):

https://codereview.chromium.org/13842010/diff/19001/chrome/browser/resources/...
chrome/browser/resources/translate_internals/index.html:26: <tabpanel
id="tabpanelPrefs">
On 2013/04/15 19:25:13, Evan Stade wrote:
> classes and IDs use hyphens, i.e. tabpanel-prefs
> 
> http://dev.chromium.org/developers/web-development-style-guide

Done.

https://codereview.chromium.org/13842010/diff/19001/chrome/browser/resources/...
chrome/browser/resources/translate_internals/index.html:28: <pre></pre>
On 2013/04/15 19:25:13, Evan Stade wrote:
> instead of using <pre>, it's probably better to use <p> and set the
> presentational style in css (text-wrap: pre-wrap, etc.)

Done.

https://codereview.chromium.org/13842010/diff/19001/chrome/browser/resources/...
File chrome/browser/resources/translate_internals/index.js (right):

https://codereview.chromium.org/13842010/diff/19001/chrome/browser/resources/...
chrome/browser/resources/translate_internals/index.js:37: case 'prefsUpdated':
On 2013/04/15 19:25:13, Evan Stade wrote:
> is this the proper indentation?

Done.

https://codereview.chromium.org/13842010/diff/19001/chrome/browser/resources/...
chrome/browser/resources/translate_internals/index.js:41: console.error('Unknown
message: ', message);
On 2013/04/15 19:25:13, Evan Stade wrote:
> you are right, no need for space after :

Done.

https://codereview.chromium.org/13842010/diff/19001/chrome/browser/resources/...
chrome/browser/resources/translate_internals/index.js:58:
cr.translateInternals.initialize);
On 2013/04/15 19:25:13, Evan Stade wrote:
> indent this argument to align with argument on previous line (assuming it
fits)

Done.

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

https://codereview.chromium.org/13842010/diff/19001/chrome/browser/ui/webui/t...
chrome/browser/ui/webui/translate_internals/translate_internals_handler.cc:43:
for (unsigned i = 0; i < arraysize(keys); ++i) {
On 2013/04/15 19:25:13, Evan Stade wrote:
> size_t

Done.

https://codereview.chromium.org/13842010/diff/19001/chrome/browser/ui/webui/t...
chrome/browser/ui/webui/translate_internals/translate_internals_handler.cc:48:
dict.Set(key, value->DeepCopy());
On 2013/04/15 19:25:13, Evan Stade wrote:
> nit: combine these two lines

Done.

https://codereview.chromium.org/13842010/diff/19001/chrome/browser/ui/webui/t...
chrome/browser/ui/webui/translate_internals/translate_internals_handler.cc:58:
scoped_ptr<base::Value> message_data(new base::StringValue(message));
On 2013/04/15 19:25:13, Evan Stade wrote:
> no need for heap allocation

Done.

https://codereview.chromium.org/13842010/diff/19001/chrome/browser/ui/webui/t...
File chrome/browser/ui/webui/translate_internals/translate_internals_ui.h
(right):

https://codereview.chromium.org/13842010/diff/19001/chrome/browser/ui/webui/t...
chrome/browser/ui/webui/translate_internals/translate_internals_ui.h:13: virtual
~TranslateInternalsUI() {}
On 2013/04/15 19:25:13, Evan Stade wrote:
> \n

Done.

[Evan Stade, 2013-04-16 20:07:19]

lgtm

https://codereview.chromium.org/13842010/diff/25001/chrome/browser/resources/...
File chrome/browser/resources/translate_internals/index.html (right):

https://codereview.chromium.org/13842010/diff/25001/chrome/browser/resources/...
chrome/browser/resources/translate_internals/index.html:1: <!DOCTYPE HTML>
I don't think index.{html,js,css} is the best name for these files. Generally we
call them [feature_name].[extension]

[hajimehoshi, 2013-04-17 02:14:40]

Thanks.

https://codereview.chromium.org/13842010/diff/25001/chrome/browser/resources/...
File chrome/browser/resources/translate_internals/index.html (right):

https://codereview.chromium.org/13842010/diff/25001/chrome/browser/resources/...
chrome/browser/resources/translate_internals/index.html:1: <!DOCTYPE HTML>
On 2013/04/16 20:07:19, Evan Stade wrote:
> I don't think index.{html,js,css} is the best name for these files. Generally
we
> call them [feature_name].[extension]

Done.

[commit-bot: I haz the power, 2013-04-18 00:17:53]

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

[commit-bot: I haz the power, 2013-04-18 00:17:58]

Presubmit check for 13842010-37002 failed and returned exit status 1.

INFO:root:Found 18 file(s).

Running presubmit commit checks ...
Running /b/commit-queue/workdir/chromium/PRESUBMIT.py
Running /b/commit-queue/workdir/chromium/chrome/PRESUBMIT.py
Running /b/commit-queue/workdir/chromium/chrome/browser/resources/PRESUBMIT.py
Running /b/commit-queue/workdir/chromium/chrome/browser/ui/webui/PRESUBMIT.py
Running /b/commit-queue/workdir/chromium/tools/PRESUBMIT.py

** Presubmit ERRORS **
Missing LGTM from an OWNER for these files:
    chrome/chrome_resources.gyp
    chrome/browser/translate/translate_prefs.h
    chrome/browser/translate/translate_prefs.cc
    chrome/chrome_browser_ui.gypi
    chrome/browser/browser_about_handler.cc
    chrome/chrome_repack_resources.gypi

Presubmit checks took 2.1s to calculate.

[hajimehoshi, 2013-04-18 00:22:48]

+sky, +mad, can you take a look?  Thank you in advance.

[MAD, 2013-04-19 21:53:27]

On 2013/04/18 00:22:48, hajimehoshi wrote:
> +sky, +mad, can you take a look?  Thank you in advance.

chrome/browser/translate LGTM...

[sky, 2013-04-22 17:01:01]

What files do you need me to review?

On Fri, Apr 19, 2013 at 2:53 PM,  <mad@chromium.org> wrote:
> On 2013/04/18 00:22:48, hajimehoshi wrote:
>>
>> +sky, +mad, can you take a look?  Thank you in advance.
>
>
> chrome/browser/translate LGTM...
>
>
> https://chromiumcodereview.appspot.com/13842010/

[Takashi Toyoshima (Google), 2013-04-22 17:18:18]

Following files need chrome/OWNERS review.
    chrome/chrome_resources.gyp
    chrome/chrome_browser_ui.gypi
    chrome/browser/browser_about_handler.cc
    chrome/chrome_repack_resources.gypi
I guess Hajime want you to review them.

[sky, 2013-04-22 22:09:47]

LGTM

[commit-bot: I haz the power, 2013-04-23 01:33:08]

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

[commit-bot: I haz the power, 2013-04-23 02:33:08]

Sorry for I got bad news for ya.
Compile failed with a clobber build on win7_aura.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win7_aura&...
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-04-23 02:47:40]

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

[commit-bot: I haz the power, 2013-04-23 06:57:53]

Change committed as 195745