Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1208993011:
New thin layer of API extension chrome.i18n.detectLanguage (Closed)
|
Created:
5 years, 5 months ago by amalika Modified:
5 years, 5 months ago Reviewers:
Takashi Toyoshima, not at google - send to devlin, Alexei Svitkine (slow), Ken Rockot(use gerrit already) CC:
mcindy, chaotian, smn Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionNew thin layer of API extension chrome.i18n.detectLanguage
R=kalman@chromium.org, toyoshim@chromium.org
BUG=
Committed: https://crrev.com/0cf819bae6edb14d99b5731d6a37cf2b8d7e4f82
Cr-Commit-Position: refs/heads/master@{#339819}
Patch Set 1 #
Total comments: 30
Patch Set 2 : Fixed after the review #
Total comments: 28
Patch Set 3 : Fixed issues and changes to functions due to added LanguageDetectionResult struct #
Total comments: 26
Patch Set 4 : Cleaned up the code #
Total comments: 24
Patch Set 5 : CLD2 call modified/errors fixed. #
Total comments: 8
Patch Set 6 : Final corrections #Patch Set 7 : After pulling from master branch #Patch Set 8 : CLD2 dependency added #
Total comments: 4
Patch Set 9 : cld2_platforn_impl added #Patch Set 10 : Final version before submit #
Total comments: 2
Patch Set 11 : Style errors fixed #
Messages
Total messages: 93 (37 generated)
Quick comments for the first round of review. I review this patch as a Chrome Translate OWNERS, but note that I'm not familiar with extension area. So I might suggest wrong things here. If so, please follow extensions OWNERS comments. https://codereview.chromium.org/1208993011/diff/1/chrome/browser/extensions/a... File chrome/browser/extensions/api/DEPS (right): https://codereview.chromium.org/1208993011/diff/1/chrome/browser/extensions/a... chrome/browser/extensions/api/DEPS:8: # cld version 2 library This comment looks meaningless because it's trivial. If you want to say something here, 'for chrome.i18n.detectLanguage' will be clear in this context to know why we need this library here. https://codereview.chromium.org/1208993011/diff/1/chrome/browser/extensions/a... File chrome/browser/extensions/api/i18n/i18n_api.cc (right): https://codereview.chromium.org/1208993011/diff/1/chrome/browser/extensions/a... chrome/browser/extensions/api/i18n/i18n_api.cc:19: no empty line? https://codereview.chromium.org/1208993011/diff/1/chrome/browser/extensions/a... chrome/browser/extensions/api/i18n/i18n_api.cc:75: text = *params->text; remove line 68, 75, and line 76 can be GetLanguage(*params->text);. https://codereview.chromium.org/1208993011/diff/1/chrome/browser/extensions/a... chrome/browser/extensions/api/i18n/i18n_api.cc:88: const char* tld_hint = ""; // asummed no cld hints provided can you change this to 'const char tld_hint[] = "";' ? https://codereview.chromium.org/1208993011/diff/1/chrome/browser/extensions/a... chrome/browser/extensions/api/i18n/i18n_api.cc:123: return; this return is not needed. https://codereview.chromium.org/1208993011/diff/1/chrome/browser/extensions/a... chrome/browser/extensions/api/i18n/i18n_api.cc:151: return; ditto https://codereview.chromium.org/1208993011/diff/1/chrome/browser/extensions/a... chrome/browser/extensions/api/i18n/i18n_api.cc:177: return; ditto https://codereview.chromium.org/1208993011/diff/1/chrome/browser/extensions/a... chrome/browser/extensions/api/i18n/i18n_api.cc:180: remove one of two empty lines? https://codereview.chromium.org/1208993011/diff/1/chrome/browser/extensions/a... File chrome/browser/extensions/api/i18n/i18n_api.h (right): https://codereview.chromium.org/1208993011/diff/1/chrome/browser/extensions/a... chrome/browser/extensions/api/i18n/i18n_api.h:12: // Added remove this comment line https://codereview.chromium.org/1208993011/diff/1/chrome/browser/extensions/a... chrome/browser/extensions/api/i18n/i18n_api.h:26: typedef struct { I think 'typedef struct { ... }' is not famous in current chromium coding style. class like syntax is used often recently. https://codereview.chromium.org/1208993011/diff/1/chrome/browser/extensions/a... chrome/browser/extensions/api/i18n/i18n_api.h:32: private: wrong indent. It would be useful to try 'git cl format' to check such errors. https://codereview.chromium.org/1208993011/diff/1/chrome/browser/extensions/a... File chrome/browser/extensions/api/tabs/tabs_api.cc (right): https://codereview.chromium.org/1208993011/diff/1/chrome/browser/extensions/a... chrome/browser/extensions/api/tabs/tabs_api.cc:1999: // "Hello World!" Did you forget to remove this comment, or add for some purpose? https://codereview.chromium.org/1208993011/diff/1/chrome/test/data/extensions... File chrome/test/data/extensions/api_test/i18n/test.js (right): https://codereview.chromium.org/1208993011/diff/1/chrome/test/data/extensions... chrome/test/data/extensions/api_test/i18n/test.js:62: /*, Why this part is commented out? If this does not work for now, I prefer adding this test when it gets working.
The CQ bit was checked by amalika@google.com
The CQ bit was unchecked by amalika@google.com
https://codereview.chromium.org/1208993011/diff/20001/chrome/common/extension... File chrome/common/extensions/api/i18n.json (right): https://codereview.chromium.org/1208993011/diff/20001/chrome/common/extension... chrome/common/extensions/api/i18n.json:20: {"name": "languages", "type": "array", "items": {"type": "string"}, "description": "Array of the accept languages of the browser, such as en-US,en,zh-CN"} You may want to define a separate type for languages, even though it's a string, it would let you centralise the documentation. "types": { "id": "LanguageCode", "type": "string", "description": "..." } then use "$ref": "LanguageCode" https://codereview.chromium.org/1208993011/diff/20001/chrome/common/extension... chrome/common/extensions/api/i18n.json:69: "description": "Defaults to the null string." Why would we support null strings? https://codereview.chromium.org/1208993011/diff/20001/chrome/common/extension... chrome/common/extensions/api/i18n.json:80: "is_reliable": { "type": "boolean", "description": "CLD detected language reliability" }, APIs typically use camelcase. https://codereview.chromium.org/1208993011/diff/20001/chrome/common/extension... chrome/common/extensions/api/i18n.json:91: "language": { "type": "string", "description": "An ISO language code such as <code>en</code> or <code>fr</code>. For a complete list of languages supported by this method, see <a href='http://src.chromium.org/viewvc/chrome/trunk/src/third_party/cld/languages/internal/languages.cc'>kLanguageInfoTable</a>. The 2nd to 4th columns will be checked and the first non-NULL value will be returned except for Simplified Chinese for which zh-CN will be returned. For an unknown language, <code>und</code> will be returned." }, Why return "und" rather than null or something? What does it even mean to say that 80% of the text is an unknown language?
https://codereview.chromium.org/1208993011/diff/20001/chrome/browser/extensio... File chrome/browser/extensions/api/i18n/i18n_api.cc (right): https://codereview.chromium.org/1208993011/diff/20001/chrome/browser/extensio... chrome/browser/extensions/api/i18n/i18n_api.cc:133: } The i18n API is compiled to a typed struct, see out/{Debug,Release}/gen/chrome/common/extensions/api/i18n.{h,cc} - better to use that than setting Values directly. https://codereview.chromium.org/1208993011/diff/20001/chrome/browser/extensio... chrome/browser/extensions/api/i18n/i18n_api.cc:144: DetectedLanguage detected_languages[3]) { Is there a way to implement this stuff without passing around an array of precisely size 3? It's a bit of an odd interface IMO. At least define 3 as a constant somewhere. https://codereview.chromium.org/1208993011/diff/20001/chrome/browser/extensio... File chrome/browser/extensions/api/i18n/i18n_api.h (right): https://codereview.chromium.org/1208993011/diff/20001/chrome/browser/extensio... chrome/browser/extensions/api/i18n/i18n_api.h:32: class I18nDetectLanguageFunction : public ChromeAsyncExtensionFunction { Inherit from UIThreadExtensionFunction not ChromeAsyncExtensionFunction. https://codereview.chromium.org/1208993011/diff/20001/chrome/browser/extensio... chrome/browser/extensions/api/i18n/i18n_api.h:38: const bool is_reliable); There is no point having const things that aren't pointers or references, unless it's actually a constant, which this isn't.
mcindy@google.com changed reviewers: + mcindy@google.com
https://codereview.chromium.org/1208993011/diff/20001/chrome/common/extension... File chrome/common/extensions/api/i18n.json (right): https://codereview.chromium.org/1208993011/diff/20001/chrome/common/extension... chrome/common/extensions/api/i18n.json:20: {"name": "languages", "type": "array", "items": {"type": "string"}, "description": "Array of the accept languages of the browser, such as en-US,en,zh-CN"} On 2015/07/08 19:27:30, kalman wrote: > You may want to define a separate type for languages, even though it's a string, > it would let you centralise the documentation. > > "types": { > "id": "LanguageCode", > "type": "string", > "description": "..." > } > > then use "$ref": "LanguageCode" Our 'languages' in detectLanguage is a different type from this one. This one is an array of strings, while we use an array of object. Is it still necessary to define it in a separate type even though we use it only once? https://codereview.chromium.org/1208993011/diff/20001/chrome/common/extension... chrome/common/extensions/api/i18n.json:69: "description": "Defaults to the null string." On 2015/07/08 19:27:30, kalman wrote: > Why would we support null strings? The API and the CLD are implemented to handle null strings https://codereview.chromium.org/1208993011/diff/20001/chrome/common/extension... chrome/common/extensions/api/i18n.json:80: "is_reliable": { "type": "boolean", "description": "CLD detected language reliability" }, On 2015/07/08 19:27:30, kalman wrote: > APIs typically use camelcase. Done. https://codereview.chromium.org/1208993011/diff/20001/chrome/common/extension... chrome/common/extensions/api/i18n.json:91: "language": { "type": "string", "description": "An ISO language code such as <code>en</code> or <code>fr</code>. For a complete list of languages supported by this method, see <a href='http://src.chromium.org/viewvc/chrome/trunk/src/third_party/cld/languages/internal/languages.cc'>kLanguageInfoTable</a>. The 2nd to 4th columns will be checked and the first non-NULL value will be returned except for Simplified Chinese for which zh-CN will be returned. For an unknown language, <code>und</code> will be returned." }, On 2015/07/08 19:27:30, kalman wrote: > Why return "und" rather than null or something? What does it even mean to say > that 80% of the text is an unknown language? und means that the CLD cannot determine the language. So 80% of unknown means that CLD cannot detect 80% of the input string.
https://codereview.chromium.org/1208993011/diff/20001/chrome/common/extension... File chrome/common/extensions/api/i18n.json (right): https://codereview.chromium.org/1208993011/diff/20001/chrome/common/extension... chrome/common/extensions/api/i18n.json:20: {"name": "languages", "type": "array", "items": {"type": "string"}, "description": "Array of the accept languages of the browser, such as en-US,en,zh-CN"} On 2015/07/08 21:26:12, mcindy wrote: > On 2015/07/08 19:27:30, kalman wrote: > > You may want to define a separate type for languages, even though it's a > string, > > it would let you centralise the documentation. > > > > "types": { > > "id": "LanguageCode", > > "type": "string", > > "description": "..." > > } > > > > then use "$ref": "LanguageCode" > > Our 'languages' in detectLanguage is a different type from this one. This one is > an array of strings, while we use an array of object. Is it still necessary to > define it in a separate type even though we use it only once? I mean purely the string. It would still be an array vs an array of objects, but of that string. https://codereview.chromium.org/1208993011/diff/20001/chrome/common/extension... chrome/common/extensions/api/i18n.json:69: "description": "Defaults to the null string." On 2015/07/08 21:26:12, mcindy wrote: > On 2015/07/08 19:27:30, kalman wrote: > > Why would we support null strings? > > The API and the CLD are implemented to handle null strings What API? We're making our API right now. It doesn't make sense to try to translate a null string, whether or not the C++ library supports it. https://codereview.chromium.org/1208993011/diff/20001/chrome/common/extension... chrome/common/extensions/api/i18n.json:91: "language": { "type": "string", "description": "An ISO language code such as <code>en</code> or <code>fr</code>. For a complete list of languages supported by this method, see <a href='http://src.chromium.org/viewvc/chrome/trunk/src/third_party/cld/languages/internal/languages.cc'>kLanguageInfoTable</a>. The 2nd to 4th columns will be checked and the first non-NULL value will be returned except for Simplified Chinese for which zh-CN will be returned. For an unknown language, <code>und</code> will be returned." }, On 2015/07/08 21:26:12, mcindy wrote: > On 2015/07/08 19:27:30, kalman wrote: > > Why return "und" rather than null or something? What does it even mean to say > > that 80% of the text is an unknown language? > > und means that the CLD cannot determine the language. So 80% of unknown means > that CLD cannot detect 80% of the input string. IMO a nicer API would be to either have a null language.
i18n_api.cc and i18_api.h reviewed https://codereview.chromium.org/1208993011/diff/1/chrome/browser/extensions/a... File chrome/browser/extensions/api/DEPS (right): https://codereview.chromium.org/1208993011/diff/1/chrome/browser/extensions/a... chrome/browser/extensions/api/DEPS:8: # cld version 2 library On 2015/07/08 03:19:41, Takashi Toyoshima (chromium) wrote: > This comment looks meaningless because it's trivial. > If you want to say something here, 'for chrome.i18n.detectLanguage' will be > clear in this context to know why we need this library here. Done. https://codereview.chromium.org/1208993011/diff/1/chrome/browser/extensions/a... File chrome/browser/extensions/api/i18n/i18n_api.cc (right): https://codereview.chromium.org/1208993011/diff/1/chrome/browser/extensions/a... chrome/browser/extensions/api/i18n/i18n_api.cc:19: On 2015/07/08 03:19:41, Takashi Toyoshima (chromium) wrote: > no empty line? Done. https://codereview.chromium.org/1208993011/diff/1/chrome/browser/extensions/a... chrome/browser/extensions/api/i18n/i18n_api.cc:75: text = *params->text; On 2015/07/08 03:19:41, Takashi Toyoshima (chromium) wrote: > remove line 68, 75, and line 76 can be GetLanguage(*params->text);. Done. https://codereview.chromium.org/1208993011/diff/1/chrome/browser/extensions/a... chrome/browser/extensions/api/i18n/i18n_api.cc:88: const char* tld_hint = ""; // asummed no cld hints provided On 2015/07/08 03:19:41, Takashi Toyoshima (chromium) wrote: > can you change this to 'const char tld_hint[] = "";' ? Done. https://codereview.chromium.org/1208993011/diff/1/chrome/browser/extensions/a... chrome/browser/extensions/api/i18n/i18n_api.cc:123: return; On 2015/07/08 03:19:41, Takashi Toyoshima (chromium) wrote: > this return is not needed. Done. https://codereview.chromium.org/1208993011/diff/1/chrome/browser/extensions/a... chrome/browser/extensions/api/i18n/i18n_api.cc:123: return; On 2015/07/08 03:19:41, Takashi Toyoshima (chromium) wrote: > this return is not needed. Done. https://codereview.chromium.org/1208993011/diff/1/chrome/browser/extensions/a... chrome/browser/extensions/api/i18n/i18n_api.cc:123: return; On 2015/07/08 03:19:41, Takashi Toyoshima (chromium) wrote: > this return is not needed. Done. https://codereview.chromium.org/1208993011/diff/1/chrome/browser/extensions/a... chrome/browser/extensions/api/i18n/i18n_api.cc:151: return; On 2015/07/08 03:19:41, Takashi Toyoshima (chromium) wrote: > ditto Done. https://codereview.chromium.org/1208993011/diff/1/chrome/browser/extensions/a... chrome/browser/extensions/api/i18n/i18n_api.cc:177: return; On 2015/07/08 03:19:41, Takashi Toyoshima (chromium) wrote: > ditto Done. https://codereview.chromium.org/1208993011/diff/1/chrome/browser/extensions/a... chrome/browser/extensions/api/i18n/i18n_api.cc:177: return; On 2015/07/08 03:19:41, Takashi Toyoshima (chromium) wrote: > ditto Done. https://codereview.chromium.org/1208993011/diff/1/chrome/browser/extensions/a... chrome/browser/extensions/api/i18n/i18n_api.cc:180: On 2015/07/08 03:19:41, Takashi Toyoshima (chromium) wrote: > remove one of two empty lines? Done. https://codereview.chromium.org/1208993011/diff/1/chrome/browser/extensions/a... File chrome/browser/extensions/api/i18n/i18n_api.h (right): https://codereview.chromium.org/1208993011/diff/1/chrome/browser/extensions/a... chrome/browser/extensions/api/i18n/i18n_api.h:12: // Added On 2015/07/08 03:19:41, Takashi Toyoshima (chromium) wrote: > remove this comment line Done. https://codereview.chromium.org/1208993011/diff/1/chrome/browser/extensions/a... chrome/browser/extensions/api/i18n/i18n_api.h:26: typedef struct { On 2015/07/08 03:19:41, Takashi Toyoshima (chromium) wrote: > I think 'typedef struct { ... }' is not famous in current chromium coding style. > class like syntax is used often recently. Done. https://codereview.chromium.org/1208993011/diff/1/chrome/browser/extensions/a... chrome/browser/extensions/api/i18n/i18n_api.h:32: private: On 2015/07/08 03:19:41, Takashi Toyoshima (chromium) wrote: > wrong indent. > It would be useful to try 'git cl format' to check such errors. Done. https://codereview.chromium.org/1208993011/diff/1/chrome/browser/extensions/a... File chrome/browser/extensions/api/tabs/tabs_api.cc (right): https://codereview.chromium.org/1208993011/diff/1/chrome/browser/extensions/a... chrome/browser/extensions/api/tabs/tabs_api.cc:1999: // "Hello World!" On 2015/07/08 03:19:42, Takashi Toyoshima (chromium) wrote: > Did you forget to remove this comment, or add for some purpose? Ops! That was a mistake. I reverted this file back to the origin/master https://codereview.chromium.org/1208993011/diff/1/chrome/test/data/extensions... File chrome/test/data/extensions/api_test/i18n/test.js (right): https://codereview.chromium.org/1208993011/diff/1/chrome/test/data/extensions... chrome/test/data/extensions/api_test/i18n/test.js:62: /*, On 2015/07/08 03:19:42, Takashi Toyoshima (chromium) wrote: > Why this part is commented out? If this does not work for now, I prefer adding > this test when it gets working. Done. https://codereview.chromium.org/1208993011/diff/20001/chrome/browser/extensio... File chrome/browser/extensions/api/i18n/i18n_api.cc (right): https://codereview.chromium.org/1208993011/diff/20001/chrome/browser/extensio... chrome/browser/extensions/api/i18n/i18n_api.cc:144: DetectedLanguage detected_languages[3]) { On 2015/07/08 19:34:53, kalman wrote: > Is there a way to implement this stuff without passing around an array of > precisely size 3? It's a bit of an odd interface IMO. At least define 3 as a > constant somewhere. Done. https://codereview.chromium.org/1208993011/diff/20001/chrome/browser/extensio... File chrome/browser/extensions/api/i18n/i18n_api.h (right): https://codereview.chromium.org/1208993011/diff/20001/chrome/browser/extensio... chrome/browser/extensions/api/i18n/i18n_api.h:32: class I18nDetectLanguageFunction : public ChromeAsyncExtensionFunction { On 2015/07/08 19:34:53, kalman wrote: > Inherit from UIThreadExtensionFunction not ChromeAsyncExtensionFunction. Changed to inherit from AsyncExtensionFunction instead as per comments here: https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ext... https://codereview.chromium.org/1208993011/diff/20001/chrome/browser/extensio... chrome/browser/extensions/api/i18n/i18n_api.h:38: const bool is_reliable); On 2015/07/08 19:34:53, kalman wrote: > There is no point having const things that aren't pointers or references, unless > it's actually a constant, which this isn't. Done.
https://codereview.chromium.org/1208993011/diff/20001/chrome/browser/extensio... File chrome/browser/extensions/api/i18n/i18n_api.h (right): https://codereview.chromium.org/1208993011/diff/20001/chrome/browser/extensio... chrome/browser/extensions/api/i18n/i18n_api.h:32: class I18nDetectLanguageFunction : public ChromeAsyncExtensionFunction { On 2015/07/08 21:58:03, amalika wrote: > On 2015/07/08 19:34:53, kalman wrote: > > Inherit from UIThreadExtensionFunction not ChromeAsyncExtensionFunction. > > Changed to inherit from AsyncExtensionFunction instead as per comments here: > https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ext... The comment right above that says DEPRECATED. Please use UIThreadExtensionFunction.
https://codereview.chromium.org/1208993011/diff/20001/chrome/common/extension... File chrome/common/extensions/api/i18n.json (right): https://codereview.chromium.org/1208993011/diff/20001/chrome/common/extension... chrome/common/extensions/api/i18n.json:91: "language": { "type": "string", "description": "An ISO language code such as <code>en</code> or <code>fr</code>. For a complete list of languages supported by this method, see <a href='http://src.chromium.org/viewvc/chrome/trunk/src/third_party/cld/languages/internal/languages.cc'>kLanguageInfoTable</a>. The 2nd to 4th columns will be checked and the first non-NULL value will be returned except for Simplified Chinese for which zh-CN will be returned. For an unknown language, <code>und</code> will be returned." }, On 2015/07/08 21:47:59, kalman wrote: > On 2015/07/08 21:26:12, mcindy wrote: > > On 2015/07/08 19:27:30, kalman wrote: > > > Why return "und" rather than null or something? What does it even mean to > say > > > that 80% of the text is an unknown language? > > > > und means that the CLD cannot determine the language. So 80% of unknown means > > that CLD cannot detect 80% of the input string. > > IMO a nicer API would be to either have a null language. Did you mean "null" in string or null type?
https://codereview.chromium.org/1208993011/diff/20001/chrome/common/extension... File chrome/common/extensions/api/i18n.json (right): https://codereview.chromium.org/1208993011/diff/20001/chrome/common/extension... chrome/common/extensions/api/i18n.json:91: "language": { "type": "string", "description": "An ISO language code such as <code>en</code> or <code>fr</code>. For a complete list of languages supported by this method, see <a href='http://src.chromium.org/viewvc/chrome/trunk/src/third_party/cld/languages/internal/languages.cc'>kLanguageInfoTable</a>. The 2nd to 4th columns will be checked and the first non-NULL value will be returned except for Simplified Chinese for which zh-CN will be returned. For an unknown language, <code>und</code> will be returned." }, On 2015/07/08 23:12:29, mcindy wrote: > On 2015/07/08 21:47:59, kalman wrote: > > On 2015/07/08 21:26:12, mcindy wrote: > > > On 2015/07/08 19:27:30, kalman wrote: > > > > Why return "und" rather than null or something? What does it even mean to > > say > > > > that 80% of the text is an unknown language? > > > > > > und means that the CLD cannot determine the language. So 80% of unknown > means > > > that CLD cannot detect 80% of the input string. > > > > IMO a nicer API would be to either have a null language. > > Did you mean "null" in string or null type? I mean to make the language an optional field.
https://codereview.chromium.org/1208993011/diff/20001/chrome/common/extension... File chrome/common/extensions/api/i18n.json (right): https://codereview.chromium.org/1208993011/diff/20001/chrome/common/extension... chrome/common/extensions/api/i18n.json:69: "description": "Defaults to the null string." On 2015/07/08 21:47:59, kalman wrote: > On 2015/07/08 21:26:12, mcindy wrote: > > On 2015/07/08 19:27:30, kalman wrote: > > > Why would we support null strings? > > > > The API and the CLD are implemented to handle null strings > > What API? We're making our API right now. It doesn't make sense to try to > translate a null string, whether or not the C++ library supports it. Currently, when the user passes in null value, the chrome.i18n.detectLanguage returns an error. Would it suffice to change the description of this param?
https://codereview.chromium.org/1208993011/diff/20001/chrome/common/extension... File chrome/common/extensions/api/i18n.json (right): https://codereview.chromium.org/1208993011/diff/20001/chrome/common/extension... chrome/common/extensions/api/i18n.json:69: "description": "Defaults to the null string." On 2015/07/08 23:28:02, mcindy wrote: > On 2015/07/08 21:47:59, kalman wrote: > > On 2015/07/08 21:26:12, mcindy wrote: > > > On 2015/07/08 19:27:30, kalman wrote: > > > > Why would we support null strings? > > > > > > The API and the CLD are implemented to handle null strings > > > > What API? We're making our API right now. It doesn't make sense to try to > > translate a null string, whether or not the C++ library supports it. > > Currently, when the user passes in null value, the chrome.i18n.detectLanguage > returns an error. Would it suffice to change the description of this param? Just don't make it optional. Then when the extension passes in a null value, it will throw an exception.
https://codereview.chromium.org/1208993011/diff/1/chrome/browser/extensions/a... File chrome/browser/extensions/api/i18n/i18n_api.h (right): https://codereview.chromium.org/1208993011/diff/1/chrome/browser/extensions/a... chrome/browser/extensions/api/i18n/i18n_api.h:26: typedef struct { Not yet. struct DetectedLanguage { std::string language; ... };
https://codereview.chromium.org/1208993011/diff/20001/chrome/common/extension... File chrome/common/extensions/api/i18n.json (right): https://codereview.chromium.org/1208993011/diff/20001/chrome/common/extension... chrome/common/extensions/api/i18n.json:20: {"name": "languages", "type": "array", "items": {"type": "string"}, "description": "Array of the accept languages of the browser, such as en-US,en,zh-CN"} On 2015/07/08 21:47:59, kalman wrote: > On 2015/07/08 21:26:12, mcindy wrote: > > On 2015/07/08 19:27:30, kalman wrote: > > > You may want to define a separate type for languages, even though it's a > > string, > > > it would let you centralise the documentation. > > > > > > "types": { > > > "id": "LanguageCode", > > > "type": "string", > > > "description": "..." > > > } > > > > > > then use "$ref": "LanguageCode" > > > > Our 'languages' in detectLanguage is a different type from this one. This one > is > > an array of strings, while we use an array of object. Is it still necessary to > > define it in a separate type even though we use it only once? > > I mean purely the string. It would still be an array vs an array of objects, but > of that string. Done. https://codereview.chromium.org/1208993011/diff/20001/chrome/common/extension... chrome/common/extensions/api/i18n.json:69: "description": "Defaults to the null string." On 2015/07/08 23:29:07, kalman wrote: > On 2015/07/08 23:28:02, mcindy wrote: > > On 2015/07/08 21:47:59, kalman wrote: > > > On 2015/07/08 21:26:12, mcindy wrote: > > > > On 2015/07/08 19:27:30, kalman wrote: > > > > > Why would we support null strings? > > > > > > > > The API and the CLD are implemented to handle null strings > > > > > > What API? We're making our API right now. It doesn't make sense to try to > > > translate a null string, whether or not the C++ library supports it. > > > > Currently, when the user passes in null value, the chrome.i18n.detectLanguage > > returns an error. Would it suffice to change the description of this param? > > Just don't make it optional. Then when the extension passes in a null value, it > will throw an exception. Done. https://codereview.chromium.org/1208993011/diff/20001/chrome/common/extension... chrome/common/extensions/api/i18n.json:91: "language": { "type": "string", "description": "An ISO language code such as <code>en</code> or <code>fr</code>. For a complete list of languages supported by this method, see <a href='http://src.chromium.org/viewvc/chrome/trunk/src/third_party/cld/languages/internal/languages.cc'>kLanguageInfoTable</a>. The 2nd to 4th columns will be checked and the first non-NULL value will be returned except for Simplified Chinese for which zh-CN will be returned. For an unknown language, <code>und</code> will be returned." }, On 2015/07/08 23:13:31, kalman wrote: > On 2015/07/08 23:12:29, mcindy wrote: > > On 2015/07/08 21:47:59, kalman wrote: > > > On 2015/07/08 21:26:12, mcindy wrote: > > > > On 2015/07/08 19:27:30, kalman wrote: > > > > > Why return "und" rather than null or something? What does it even mean > to > > > say > > > > > that 80% of the text is an unknown language? > > > > > > > > und means that the CLD cannot determine the language. So 80% of unknown > > means > > > > that CLD cannot detect 80% of the input string. > > > > > > IMO a nicer API would be to either have a null language. > > > > Did you mean "null" in string or null type? > > I mean to make the language an optional field. Done.
Number of changes due to added LanguageDetectionResult struct https://codereview.chromium.org/1208993011/diff/20001/chrome/browser/extensio... File chrome/browser/extensions/api/i18n/i18n_api.cc (right): https://codereview.chromium.org/1208993011/diff/20001/chrome/browser/extensio... chrome/browser/extensions/api/i18n/i18n_api.cc:133: } On 2015/07/08 19:34:53, kalman wrote: > The i18n API is compiled to a typed struct, see > out/{Debug,Release}/gen/chrome/common/extensions/api/i18n.{h,cc} - better to use > that than setting Values directly. Done. https://codereview.chromium.org/1208993011/diff/20001/chrome/browser/extensio... File chrome/browser/extensions/api/i18n/i18n_api.h (right): https://codereview.chromium.org/1208993011/diff/20001/chrome/browser/extensio... chrome/browser/extensions/api/i18n/i18n_api.h:32: class I18nDetectLanguageFunction : public ChromeAsyncExtensionFunction { On 2015/07/08 22:11:49, kalman wrote: > On 2015/07/08 21:58:03, amalika wrote: > > On 2015/07/08 19:34:53, kalman wrote: > > > Inherit from UIThreadExtensionFunction not ChromeAsyncExtensionFunction. > > > > Changed to inherit from AsyncExtensionFunction instead as per comments here: > > > https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ext... > > The comment right above that says DEPRECATED. Please use > UIThreadExtensionFunction. Done.
https://codereview.chromium.org/1208993011/diff/40001/chrome/browser/extensio... File chrome/browser/extensions/api/DEPS (right): https://codereview.chromium.org/1208993011/diff/40001/chrome/browser/extensio... chrome/browser/extensions/api/DEPS:9: "+tools/json_schema_compiler", Do you really needs to add the json_schema_compiler rule? It should be implicit in the dependency on chrome/common/extensions - which surely must happen somewhere? https://codereview.chromium.org/1208993011/diff/40001/chrome/browser/extensio... File chrome/browser/extensions/api/i18n/i18n_api.cc (right): https://codereview.chromium.org/1208993011/diff/40001/chrome/browser/extensio... chrome/browser/extensions/api/i18n/i18n_api.cc:18: #define CLD_NUM_OF_LANGS 3 use "const int kCldNumLands = 3" not #define https://codereview.chromium.org/1208993011/diff/40001/chrome/browser/extensio... chrome/browser/extensions/api/i18n/i18n_api.cc:20: namespace GetAcceptLanguages = extensions::api::i18n::GetAcceptLanguages; It makes sense to move this namespace alias inside the "namespace extensions" block (not your code, I know). https://codereview.chromium.org/1208993011/diff/40001/chrome/browser/extensio... chrome/browser/extensions/api/i18n/i18n_api.cc:150: double normalized_score3[CLD_NUM_OF_LANGS]; just |normalized_scores|, take out all references to 3 other than the constant. https://codereview.chromium.org/1208993011/diff/40001/chrome/browser/extensio... chrome/browser/extensions/api/i18n/i18n_api.cc:152: cld_language = CLD2::ExtDetectLanguageSummaryCheckUTF8( Declare and initialize |cld_language| here. https://codereview.chromium.org/1208993011/diff/40001/chrome/browser/extensio... chrome/browser/extensions/api/i18n/i18n_api.cc:175: LanguageDetectionResult* result = new LanguageDetectionResult( I don't see where this result is destroyed; but in any case, this is exactly what is already generated for you. See the output in out/Debug/gen/chrome/common/extensions/api/i18n.h https://codereview.chromium.org/1208993011/diff/40001/chrome/browser/extensio... chrome/browser/extensions/api/i18n/i18n_api.cc:185: for (int i = 0; i< CLD_NUM_OF_LANGS; i++) { "i <" not "i<" https://codereview.chromium.org/1208993011/diff/40001/chrome/browser/extensio... chrome/browser/extensions/api/i18n/i18n_api.cc:201: break; Didn't we establish not to use "un"? https://codereview.chromium.org/1208993011/diff/40001/chrome/browser/extensio... File chrome/browser/extensions/api/i18n/i18n_api.h (right): https://codereview.chromium.org/1208993011/diff/40001/chrome/browser/extensio... chrome/browser/extensions/api/i18n/i18n_api.h:30: struct DetectedLanguage { You shouldn't need to make this public, and in fact, ideally you'd be able to forward-declare them (I can't remember if linked_ptr<DetectedLanguage> is forward-declaration friendly, worth a shot though). ... and all that said, really, this should already be generated for you in out/Debug/gen/chrome/common/extensions/api/i18n.h you #include it with #include "chrome/common/extensions/api/i18n.h" https://codereview.chromium.org/1208993011/diff/40001/chrome/browser/extensio... chrome/browser/extensions/api/i18n/i18n_api.h:44: struct LanguageDetectionResult { You should be able to define this entirely in an anonymous namespace in the .cc file. https://codereview.chromium.org/1208993011/diff/40001/chrome/browser/extensio... chrome/browser/extensions/api/i18n/i18n_api.h:46: LanguageDetectionResult(bool is_reliable, This is not the style we use, but running "git cl format" will fix that all for you. https://codereview.chromium.org/1208993011/diff/40001/chrome/browser/extensio... chrome/browser/extensions/api/i18n/i18n_api.h:59: std::vector<linked_ptr<DetectedLanguage> > detected_langs; No space needed between "> >" anymore. Same goes for everywhere else that there is a space between "> >".
amalika@google.com changed reviewers: - toyoshim@chromium.org
Before making any further changes https://codereview.chromium.org/1208993011/diff/40001/chrome/browser/extensio... File chrome/browser/extensions/api/i18n/i18n_api.h (right): https://codereview.chromium.org/1208993011/diff/40001/chrome/browser/extensio... chrome/browser/extensions/api/i18n/i18n_api.h:30: struct DetectedLanguage { On 2015/07/10 16:21:09, kalman wrote: > You shouldn't need to make this public, and in fact, ideally you'd be able to > forward-declare them (I can't remember if linked_ptr<DetectedLanguage> is > forward-declaration friendly, worth a shot though). > > ... and all that said, really, this should already be generated for you in > out/Debug/gen/chrome/common/extensions/api/i18n.h > > you #include it with #include "chrome/common/extensions/api/i18n.h" I think we finally understand what you meant in this and comment from last patch. Those structs are already auto generated in "chrome/common/extensions/api/i18n.h" so there is no need to define them ourselves and instead use them. Is that right? If that is the case, then I would remove two structs defined here. Is it acceptable to use typedef to make code more readable? ** typedef struct extensions::api::i18n::DetectLanguage::Results::Result LanguageDetectionResult; If it is a bad practice, should I just define ** namespace LanguageDetectionResult = extensions::api::i18n::DetectLanguage::Results; Thank you!
https://codereview.chromium.org/1208993011/diff/40001/chrome/browser/extensio... File chrome/browser/extensions/api/i18n/i18n_api.h (right): https://codereview.chromium.org/1208993011/diff/40001/chrome/browser/extensio... chrome/browser/extensions/api/i18n/i18n_api.h:30: struct DetectedLanguage { On 2015/07/10 18:18:36, amalika wrote: > On 2015/07/10 16:21:09, kalman wrote: > > You shouldn't need to make this public, and in fact, ideally you'd be able to > > forward-declare them (I can't remember if linked_ptr<DetectedLanguage> is > > forward-declaration friendly, worth a shot though). > > > > ... and all that said, really, this should already be generated for you in > > out/Debug/gen/chrome/common/extensions/api/i18n.h > > > > you #include it with #include "chrome/common/extensions/api/i18n.h" > > I think we finally understand what you meant in this and comment from last > patch. Those structs are already auto generated in > "chrome/common/extensions/api/i18n.h" so there is no need to define them > ourselves and instead use them. Is that right? > If that is the case, then I would remove two structs defined here. Is it > acceptable to use typedef to make code more readable? > ** typedef struct extensions::api::i18n::DetectLanguage::Results::Result > LanguageDetectionResult; > > > If it is a bad practice, should I just define > ** namespace LanguageDetectionResult = > extensions::api::i18n::DetectLanguage::Results; > > Thank you! Yep you can typedef, but only in the .cc file (not the header).
amalika@google.com changed reviewers: - mcindy@google.com
Cleaned up the code https://codereview.chromium.org/1208993011/diff/40001/chrome/browser/extensio... File chrome/browser/extensions/api/DEPS (right): https://codereview.chromium.org/1208993011/diff/40001/chrome/browser/extensio... chrome/browser/extensions/api/DEPS:9: "+tools/json_schema_compiler", On 2015/07/10 16:21:09, kalman wrote: > Do you really needs to add the json_schema_compiler rule? It should be implicit > in the dependency on chrome/common/extensions - which surely must happen > somewhere? Done. https://codereview.chromium.org/1208993011/diff/40001/chrome/browser/extensio... File chrome/browser/extensions/api/i18n/i18n_api.cc (right): https://codereview.chromium.org/1208993011/diff/40001/chrome/browser/extensio... chrome/browser/extensions/api/i18n/i18n_api.cc:18: #define CLD_NUM_OF_LANGS 3 On 2015/07/10 16:21:09, kalman wrote: > use "const int kCldNumLands = 3" not #define Done. https://codereview.chromium.org/1208993011/diff/40001/chrome/browser/extensio... chrome/browser/extensions/api/i18n/i18n_api.cc:20: namespace GetAcceptLanguages = extensions::api::i18n::GetAcceptLanguages; On 2015/07/10 16:21:09, kalman wrote: > It makes sense to move this namespace alias inside the "namespace extensions" > block (not your code, I know). Done. https://codereview.chromium.org/1208993011/diff/40001/chrome/browser/extensio... chrome/browser/extensions/api/i18n/i18n_api.cc:150: double normalized_score3[CLD_NUM_OF_LANGS]; On 2015/07/10 16:21:09, kalman wrote: > just |normalized_scores|, take out all references to 3 other than the constant. Done. https://codereview.chromium.org/1208993011/diff/40001/chrome/browser/extensio... chrome/browser/extensions/api/i18n/i18n_api.cc:152: cld_language = CLD2::ExtDetectLanguageSummaryCheckUTF8( On 2015/07/10 16:21:09, kalman wrote: > Declare and initialize |cld_language| here. Done. https://codereview.chromium.org/1208993011/diff/40001/chrome/browser/extensio... chrome/browser/extensions/api/i18n/i18n_api.cc:175: LanguageDetectionResult* result = new LanguageDetectionResult( On 2015/07/10 16:21:09, kalman wrote: > I don't see where this result is destroyed; but in any case, this is exactly > what is already generated for you. See the output in > out/Debug/gen/chrome/common/extensions/api/i18n.h Done. https://codereview.chromium.org/1208993011/diff/40001/chrome/browser/extensio... chrome/browser/extensions/api/i18n/i18n_api.cc:185: for (int i = 0; i< CLD_NUM_OF_LANGS; i++) { On 2015/07/10 16:21:09, kalman wrote: > "i <" not "i<" Done. https://codereview.chromium.org/1208993011/diff/40001/chrome/browser/extensio... chrome/browser/extensions/api/i18n/i18n_api.cc:201: break; On 2015/07/10 16:21:09, kalman wrote: > Didn't we establish not to use "un"? Done. https://codereview.chromium.org/1208993011/diff/40001/chrome/browser/extensio... File chrome/browser/extensions/api/i18n/i18n_api.h (right): https://codereview.chromium.org/1208993011/diff/40001/chrome/browser/extensio... chrome/browser/extensions/api/i18n/i18n_api.h:30: struct DetectedLanguage { On 2015/07/13 21:25:10, kalman wrote: > On 2015/07/10 18:18:36, amalika wrote: > > On 2015/07/10 16:21:09, kalman wrote: > > > You shouldn't need to make this public, and in fact, ideally you'd be able > to > > > forward-declare them (I can't remember if linked_ptr<DetectedLanguage> is > > > forward-declaration friendly, worth a shot though). > > > > > > ... and all that said, really, this should already be generated for you in > > > out/Debug/gen/chrome/common/extensions/api/i18n.h > > > > > > you #include it with #include "chrome/common/extensions/api/i18n.h" > > > > I think we finally understand what you meant in this and comment from last > > patch. Those structs are already auto generated in > > "chrome/common/extensions/api/i18n.h" so there is no need to define them > > ourselves and instead use them. Is that right? > > If that is the case, then I would remove two structs defined here. Is it > > acceptable to use typedef to make code more readable? > > ** typedef struct extensions::api::i18n::DetectLanguage::Results::Result > > LanguageDetectionResult; > > > > > > If it is a bad practice, should I just define > > ** namespace LanguageDetectionResult = > > extensions::api::i18n::DetectLanguage::Results; > > > > Thank you! > > Yep you can typedef, but only in the .cc file (not the header). Done. https://codereview.chromium.org/1208993011/diff/40001/chrome/browser/extensio... chrome/browser/extensions/api/i18n/i18n_api.h:44: struct LanguageDetectionResult { On 2015/07/10 16:21:09, kalman wrote: > You should be able to define this entirely in an anonymous namespace in the .cc > file. Acknowledged. https://codereview.chromium.org/1208993011/diff/40001/chrome/browser/extensio... chrome/browser/extensions/api/i18n/i18n_api.h:46: LanguageDetectionResult(bool is_reliable, On 2015/07/10 16:21:09, kalman wrote: > This is not the style we use, but running "git cl format" will fix that all for > you. Done. https://codereview.chromium.org/1208993011/diff/40001/chrome/browser/extensio... chrome/browser/extensions/api/i18n/i18n_api.h:59: std::vector<linked_ptr<DetectedLanguage> > detected_langs; On 2015/07/10 16:21:09, kalman wrote: > No space needed between "> >" anymore. Same goes for everywhere else that there > is a space between "> >". Done.
toyoshim@chromium.org changed reviewers: + toyoshim@chromium.org
lg with one nit in terms of translate/. Please wait for kalman's approval for others. https://codereview.chromium.org/1208993011/diff/60001/chrome/browser/extensio... File chrome/browser/extensions/api/i18n/i18n_api.cc (right): https://codereview.chromium.org/1208993011/diff/60001/chrome/browser/extensio... chrome/browser/extensions/api/i18n/i18n_api.cc:97: CLD2::ExtDetectLanguageSummaryCheckUTF8( Should we refer crbug.com/444258 here so that we can update this code when we solve the problem? Or if we can share more code with components/translate/core/language_detection/language_detection_util.cc, that's great.
https://codereview.chromium.org/1208993011/diff/60001/chrome/browser/extensio... File chrome/browser/extensions/api/i18n/i18n_api.cc (right): https://codereview.chromium.org/1208993011/diff/60001/chrome/browser/extensio... chrome/browser/extensions/api/i18n/i18n_api.cc:19: // Typedef only introduced to avoid unreadable code. comment not necessary. https://codereview.chromium.org/1208993011/diff/60001/chrome/browser/extensio... chrome/browser/extensions/api/i18n/i18n_api.cc:23: LanguageDetectionResult; c++11 has "using LanguageDetectionResult = ..." not typedef. https://codereview.chromium.org/1208993011/diff/60001/chrome/browser/extensio... chrome/browser/extensions/api/i18n/i18n_api.cc:26: const int kCldNumLangs = 3; comment; maintain existing blank line spacing https://codereview.chromium.org/1208993011/diff/60001/chrome/browser/extensio... chrome/browser/extensions/api/i18n/i18n_api.cc:72: return RespondLater(); This should all be: scoped_ptr<..> params(...); EXTENSION_FUNCTION_VALIDATE(params); return RespondNow(ArgumentList(GetLanguage(params->text))); and GetLanguage should return a scoped_ptr<Value> of the result. no need for SendLanguagesResult, calling SetResult/SendResponse separtately. https://codereview.chromium.org/1208993011/diff/60001/chrome/browser/extensio... chrome/browser/extensions/api/i18n/i18n_api.cc:77: const std::string utf8_text(text); const on local variable values (i.e. those that aren't pointers) are pointless, and IMO they detract from readability. https://codereview.chromium.org/1208993011/diff/60001/chrome/browser/extensio... chrome/browser/extensions/api/i18n/i18n_api.cc:79: const char* raw_utf8_bytes = utf8_text.c_str(); writing these to local variables before using them detracts from readability. just inline the casting as part of the CLD2:: call. https://codereview.chromium.org/1208993011/diff/60001/chrome/browser/extensio... chrome/browser/extensions/api/i18n/i18n_api.cc:85: CLD2::CLDHints cldhints = {NULL, tld_hint, encoding_hint, language_hint}; NULL -> nullptr - and I think there is overuse of local variables here as well. makes things look more complicated than they really are. https://codereview.chromium.org/1208993011/diff/60001/chrome/browser/extensio... File chrome/browser/extensions/api/i18n/i18n_api.h (left): https://codereview.chromium.org/1208993011/diff/60001/chrome/browser/extensio... chrome/browser/extensions/api/i18n/i18n_api.h:14: Leave this blank line in i.e. don't delete it. https://codereview.chromium.org/1208993011/diff/60001/chrome/browser/extensio... File chrome/browser/extensions/api/i18n/i18n_api.h (right): https://codereview.chromium.org/1208993011/diff/60001/chrome/browser/extensio... chrome/browser/extensions/api/i18n/i18n_api.h:30: ResponseAction Run() override; For overrides of an interface you usually do // UIThreadExtensionFunction: ResponseAction Run() override; it's also nice for readability to have blank lines between sections of header files (i.e. between destructor, overrides, then the non-override functions). https://codereview.chromium.org/1208993011/diff/60001/chrome/browser/extensio... chrome/browser/extensions/api/i18n/i18n_api.h:37: std::vector<linked_ptr<extensions::api::i18n::DetectLanguage::Results:: "extensions::" prefix not necessary anywhere, this is already in extensions namespace.
mcindy@google.com changed reviewers: + mcindy@google.com
https://codereview.chromium.org/1208993011/diff/60001/chrome/browser/extensio... File chrome/browser/extensions/api/i18n/i18n_api.cc (right): https://codereview.chromium.org/1208993011/diff/60001/chrome/browser/extensio... chrome/browser/extensions/api/i18n/i18n_api.cc:79: const char* raw_utf8_bytes = utf8_text.c_str(); On 2015/07/14 18:42:57, kalman wrote: > writing these to local variables before using them detracts from readability. > just inline the casting as part of the CLD2:: call. We're currently modifying the CLD2:: call to accomodate with the bug toyoshi mentioned and we will be using these local variables twice. Is it still okay to put the casting inline as part of the call? Because I'm worried it will make it look a bit redundant and unclear.
https://codereview.chromium.org/1208993011/diff/60001/chrome/browser/extensio... File chrome/browser/extensions/api/i18n/i18n_api.cc (right): https://codereview.chromium.org/1208993011/diff/60001/chrome/browser/extensio... chrome/browser/extensions/api/i18n/i18n_api.cc:79: const char* raw_utf8_bytes = utf8_text.c_str(); On 2015/07/14 21:38:27, mcindy wrote: > On 2015/07/14 18:42:57, kalman wrote: > > writing these to local variables before using them detracts from readability. > > just inline the casting as part of the CLD2:: call. > > We're currently modifying the CLD2:: call to accomodate with the bug toyoshi > mentioned and we will be using these local variables twice. Is it still okay to > put the casting inline as part of the call? Because I'm worried it will make it > look a bit redundant and unclear. I'd make it a cast for now and change it if/when it needs to later.
https://codereview.chromium.org/1208993011/diff/60001/chrome/browser/extensio... File chrome/browser/extensions/api/i18n/i18n_api.cc (right): https://codereview.chromium.org/1208993011/diff/60001/chrome/browser/extensio... chrome/browser/extensions/api/i18n/i18n_api.cc:19: // Typedef only introduced to avoid unreadable code. On 2015/07/14 18:42:57, kalman wrote: > comment not necessary. Done. https://codereview.chromium.org/1208993011/diff/60001/chrome/browser/extensio... chrome/browser/extensions/api/i18n/i18n_api.cc:23: LanguageDetectionResult; On 2015/07/14 18:42:58, kalman wrote: > c++11 has "using LanguageDetectionResult = ..." not typedef. Done. https://codereview.chromium.org/1208993011/diff/60001/chrome/browser/extensio... chrome/browser/extensions/api/i18n/i18n_api.cc:26: const int kCldNumLangs = 3; On 2015/07/14 18:42:58, kalman wrote: > comment; maintain existing blank line spacing Done. https://codereview.chromium.org/1208993011/diff/60001/chrome/browser/extensio... chrome/browser/extensions/api/i18n/i18n_api.cc:72: return RespondLater(); On 2015/07/14 18:42:57, kalman wrote: > This should all be: > > scoped_ptr<..> params(...); > EXTENSION_FUNCTION_VALIDATE(params); > return RespondNow(ArgumentList(GetLanguage(params->text))); > > and GetLanguage should return a scoped_ptr<Value> of the result. > > no need for SendLanguagesResult, calling SetResult/SendResponse separtately. Done. https://codereview.chromium.org/1208993011/diff/60001/chrome/browser/extensio... File chrome/browser/extensions/api/i18n/i18n_api.h (left): https://codereview.chromium.org/1208993011/diff/60001/chrome/browser/extensio... chrome/browser/extensions/api/i18n/i18n_api.h:14: On 2015/07/14 18:42:58, kalman wrote: > Leave this blank line in i.e. don't delete it. Done. https://codereview.chromium.org/1208993011/diff/60001/chrome/browser/extensio... File chrome/browser/extensions/api/i18n/i18n_api.h (right): https://codereview.chromium.org/1208993011/diff/60001/chrome/browser/extensio... chrome/browser/extensions/api/i18n/i18n_api.h:30: ResponseAction Run() override; On 2015/07/14 18:42:58, kalman wrote: > For overrides of an interface you usually do > > // UIThreadExtensionFunction: > ResponseAction Run() override; > > it's also nice for readability to have blank lines between sections of header > files (i.e. between destructor, overrides, then the non-override functions). Done. https://codereview.chromium.org/1208993011/diff/60001/chrome/browser/extensio... chrome/browser/extensions/api/i18n/i18n_api.h:37: std::vector<linked_ptr<extensions::api::i18n::DetectLanguage::Results:: On 2015/07/14 18:42:58, kalman wrote: > "extensions::" prefix not necessary anywhere, this is already in extensions > namespace. Done.
Fixed and modified the code style errors https://codereview.chromium.org/1208993011/diff/60001/chrome/browser/extensio... File chrome/browser/extensions/api/i18n/i18n_api.cc (right): https://codereview.chromium.org/1208993011/diff/60001/chrome/browser/extensio... chrome/browser/extensions/api/i18n/i18n_api.cc:77: const std::string utf8_text(text); On 2015/07/14 18:42:57, kalman wrote: > const on local variable values (i.e. those that aren't pointers) are pointless, > and IMO they detract from readability. Done. https://codereview.chromium.org/1208993011/diff/60001/chrome/browser/extensio... chrome/browser/extensions/api/i18n/i18n_api.cc:79: const char* raw_utf8_bytes = utf8_text.c_str(); On 2015/07/14 21:48:33, kalman wrote: > On 2015/07/14 21:38:27, mcindy wrote: > > On 2015/07/14 18:42:57, kalman wrote: > > > writing these to local variables before using them detracts from > readability. > > > just inline the casting as part of the CLD2:: call. > > > > We're currently modifying the CLD2:: call to accomodate with the bug toyoshi > > mentioned and we will be using these local variables twice. Is it still okay > to > > put the casting inline as part of the call? Because I'm worried it will make > it > > look a bit redundant and unclear. > > I'd make it a cast for now and change it if/when it needs to later. Done. https://codereview.chromium.org/1208993011/diff/60001/chrome/browser/extensio... chrome/browser/extensions/api/i18n/i18n_api.cc:85: CLD2::CLDHints cldhints = {NULL, tld_hint, encoding_hint, language_hint}; On 2015/07/14 18:42:58, kalman wrote: > NULL -> nullptr - and I think there is overuse of local variables here as well. > makes things look more complicated than they really are. Done. https://codereview.chromium.org/1208993011/diff/60001/chrome/browser/extensio... chrome/browser/extensions/api/i18n/i18n_api.cc:97: CLD2::ExtDetectLanguageSummaryCheckUTF8( On 2015/07/14 07:28:16, Takashi Toyoshima (chromium) wrote: > Should we refer crbug.com/444258 here so that we can update this code when we > solve the problem? > > Or if we can share more code with > components/translate/core/language_detection/language_detection_util.cc, that's > great. Done.
lgtm https://codereview.chromium.org/1208993011/diff/80001/chrome/browser/extensio... File chrome/browser/extensions/api/i18n/i18n_api.cc (right): https://codereview.chromium.org/1208993011/diff/80001/chrome/browser/extensio... chrome/browser/extensions/api/i18n/i18n_api.cc:26: // max number of languages detected by CLD2 Use properly formatted sentences, so "Max" not "max" and end the comment in a period. https://codereview.chromium.org/1208993011/diff/80001/chrome/browser/extensio... chrome/browser/extensions/api/i18n/i18n_api.cc:27: const int kCldNumLangs = 3; Blank line below this. https://codereview.chromium.org/1208993011/diff/80001/chrome/browser/extensio... chrome/browser/extensions/api/i18n/i18n_api.cc:80: bool is_plain_text = true; // assumed the text is plain text "assume" not "assumed" https://codereview.chromium.org/1208993011/diff/80001/chrome/browser/extensio... chrome/browser/extensions/api/i18n/i18n_api.cc:81: int flags = 0; // assumed 0 flags provided A comment like "assume 0 flags provided" is not very useful. Perhaps: "no flags, see compact_lang_det.h for details"
lgtm amalika: you will need to add additional reviewers for extensions/ and tools/ to submit.
amalika@google.com changed reviewers: + asvitkine@chromium.org - mcindy@google.com
Adding asvitkine@chromium.org as an owner of extensions/browser/extension_function_histogram_value.h for this chrome.i18n.detectLanguage API. https://codereview.chromium.org/1208993011/diff/80001/chrome/browser/extensio... File chrome/browser/extensions/api/i18n/i18n_api.cc (right): https://codereview.chromium.org/1208993011/diff/80001/chrome/browser/extensio... chrome/browser/extensions/api/i18n/i18n_api.cc:26: // max number of languages detected by CLD2 On 2015/07/14 23:16:14, kalman wrote: > Use properly formatted sentences, so "Max" not "max" and end the comment in a > period. Done. https://codereview.chromium.org/1208993011/diff/80001/chrome/browser/extensio... chrome/browser/extensions/api/i18n/i18n_api.cc:27: const int kCldNumLangs = 3; On 2015/07/14 23:16:14, kalman wrote: > Blank line below this. Done. https://codereview.chromium.org/1208993011/diff/80001/chrome/browser/extensio... chrome/browser/extensions/api/i18n/i18n_api.cc:80: bool is_plain_text = true; // assumed the text is plain text On 2015/07/14 23:16:14, kalman wrote: > "assume" not "assumed" Done. https://codereview.chromium.org/1208993011/diff/80001/chrome/browser/extensio... chrome/browser/extensions/api/i18n/i18n_api.cc:81: int flags = 0; // assumed 0 flags provided On 2015/07/14 23:16:14, kalman wrote: > A comment like "assume 0 flags provided" is not very useful. Perhaps: > "no flags, see compact_lang_det.h for details" Done.
lgtm
The CQ bit was checked by amalika@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from toyoshim@chromium.org, kalman@chromium.org Link to the patchset: https://codereview.chromium.org/1208993011/#ps100001 (title: "Final corrections")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1208993011/100001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_compile_dbg_32_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) ios_dbg_simulator_ninja on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...) ios_rel_device_ninja on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...) mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...) win8_chromium_ng on tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win8_chromium_ng/...) win_chromium_compile_dbg_ng on tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...) win_chromium_rel_ng on tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...) (exceeded global retry quota)
The CQ bit was checked by amalika@google.com
The CQ bit was unchecked by amalika@google.com
The CQ bit was checked by amalika@google.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1208993011/100001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_arm64_dbg_recipe on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/android_arm64_d...) android_chromium_gn_compile_dbg on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/android_chromiu...) android_clang_dbg_recipe on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/android_clang_d...) cast_shell_android on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_andr...) cast_shell_linux on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...) chromium_presubmit on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...) linux_arm_compile on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_arm_compi...) linux_chromium_asan_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_chromeos_compile_dbg_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_chromeos_ozone_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_compile_dbg_32_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_gn_chromeos_rel on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_gn_rel on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) ios_dbg_simulator_ninja on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...) mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...) win8_chromium_ng on tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win8_chromium_ng/...) (exceeded global retry quota)
The CQ bit was checked by amalika@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from toyoshim@chromium.org, asvitkine@chromium.org, kalman@chromium.org Link to the patchset: https://codereview.chromium.org/1208993011/#ps120001 (title: "After pulling from master branch")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1208993011/120001
The CQ bit was unchecked by commit-bot@chromium.org
Exceeded global retry quota
The CQ bit was checked by amalika@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1208993011/140001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_rel_ng on tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...) (exceeded global retry quota)
The CQ bit was checked by mcindy@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1208993011/140001
amalika: This is a real issue. You need to fix build rules to link the dependent library for windows.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win8_chromium_ng on tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win8_chromium_ng/...)
Patchset #9 (id:160001) has been deleted
Patchset #9 (id:180001) has been deleted
Patchset #9 (id:200001) has been deleted
Patchset #9 (id:220001) has been deleted
Patchset #8 (id:140001) has been deleted
The CQ bit was checked by amalika@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1208993011/240001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win8_chromium_ng on tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win8_chromium_ng/...)
The CQ bit was checked by amalika@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1208993011/280001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/1208993011/diff/280001/chrome/browser/extensi... File chrome/browser/extensions/api/api_registration.gyp (right): https://codereview.chromium.org/1208993011/diff/280001/chrome/browser/extensi... chrome/browser/extensions/api/api_registration.gyp:38: [ 'cld_version==0 or cld_version==2', { why the ==0 ?
amalika@google.com changed reviewers: - mcindy@google.com
https://codereview.chromium.org/1208993011/diff/280001/chrome/browser/extensi... File chrome/browser/extensions/api/api_registration.gyp (right): https://codereview.chromium.org/1208993011/diff/280001/chrome/browser/extensi... chrome/browser/extensions/api/api_registration.gyp:38: [ 'cld_version==0 or cld_version==2', { On 2015/07/21 21:40:58, kalman wrote: > why the ==0 ? According to this file: https://code.google.com/p/chromium/codesearch#chromium/src/build/common.gypi&... # Set the version of CLD. # 0: Don't specify the version. This option is for the Finch testing. # 1: Use only CLD1. # 2: Use only CLD2. Usage example: https://code.google.com/p/chromium/codesearch#chromium/src/chrome/chrome.gyp&...
kalman@chromium.org changed reviewers: + rockot@chromium.org
Ok that makes sense, thanks. lgtm. +rockot in case I'm wrong about the gyp stuff.
rockot@chromium.org changed reviewers: + andrewhayden@chromium.org
Adding andrewhayden@ as third_party/{cld,cld_2} OWNER: When does version 1 get
used? In general does a cld dependent built as part of chrome need to worry
about the cld_version==1 case?
https://codereview.chromium.org/1208993011/diff/240001/chrome/browser/extensi...
File chrome/browser/extensions/api/api_registration.gyp (right):
https://codereview.chromium.org/1208993011/diff/240001/chrome/browser/extensi...
chrome/browser/extensions/api/api_registration.gyp:38: [ 'cld_version==0 or
cld_version==2', {
nit: no need for the space after [
https://codereview.chromium.org/1208993011/diff/240001/chrome/browser/extensi...
chrome/browser/extensions/api/api_registration.gyp:43: }],
i don't really know anything about CLD. I wonder if it's also a good idea to add
a clause for version 1, something like this:
['cld_version==1', {
'dependencies': [
'<(DEPTH)/third_party/cld/cld.gyp:cld',
},
}],
On 2015/07/21 22:28:39, Ken Rockot wrote:
> Adding andrewhayden@ as third_party/{cld,cld_2} OWNER: When does version 1 get
> used? In general does a cld dependent built as part of chrome need to worry
> about the cld_version==1 case?
>
>
https://codereview.chromium.org/1208993011/diff/240001/chrome/browser/extensi...
> File chrome/browser/extensions/api/api_registration.gyp (right):
>
>
https://codereview.chromium.org/1208993011/diff/240001/chrome/browser/extensi...
> chrome/browser/extensions/api/api_registration.gyp:38: [ 'cld_version==0 or
> cld_version==2', {
> nit: no need for the space after [
>
>
https://codereview.chromium.org/1208993011/diff/240001/chrome/browser/extensi...
> chrome/browser/extensions/api/api_registration.gyp:43: }],
> i don't really know anything about CLD. I wonder if it's also a good idea to
add
> a clause for version 1, something like this:
>
> ['cld_version==1', {
> 'dependencies': [
> '<(DEPTH)/third_party/cld/cld.gyp:cld',
> },
> }],
(and actually this needs an owners review anyway since it's adding a new cld
entry to DEPS)
https://codereview.chromium.org/1208993011/diff/240001/chrome/browser/extensi... File chrome/browser/extensions/api/api_registration.gyp (right): https://codereview.chromium.org/1208993011/diff/240001/chrome/browser/extensi... chrome/browser/extensions/api/api_registration.gyp:38: [ 'cld_version==0 or cld_version==2', { On 2015/07/21 22:28:39, Ken Rockot wrote: > nit: no need for the space after [ Done. https://codereview.chromium.org/1208993011/diff/240001/chrome/browser/extensi... chrome/browser/extensions/api/api_registration.gyp:43: }], On 2015/07/21 22:28:39, Ken Rockot wrote: > i don't really know anything about CLD. I wonder if it's also a good idea to add > a clause for version 1, something like this: > > ['cld_version==1', { > 'dependencies': [ > '<(DEPTH)/third_party/cld/cld.gyp:cld', > }, > }], As of November 20, 2014 the sole remaining platform using CLD1 is Android. Our API is only available on desktop. In i18n.cc file we also only have support for CLD2 as per reason above, so I guess it would be unnecessary to add a dependency for CLD1?
On 2015/07/21 22:40:57, amalika wrote: > https://codereview.chromium.org/1208993011/diff/240001/chrome/browser/extensi... > File chrome/browser/extensions/api/api_registration.gyp (right): > > https://codereview.chromium.org/1208993011/diff/240001/chrome/browser/extensi... > chrome/browser/extensions/api/api_registration.gyp:38: [ 'cld_version==0 or > cld_version==2', { > On 2015/07/21 22:28:39, Ken Rockot wrote: > > nit: no need for the space after [ > > Done. > > https://codereview.chromium.org/1208993011/diff/240001/chrome/browser/extensi... > chrome/browser/extensions/api/api_registration.gyp:43: }], > On 2015/07/21 22:28:39, Ken Rockot wrote: > > i don't really know anything about CLD. I wonder if it's also a good idea to > add > > a clause for version 1, something like this: > > > > ['cld_version==1', { > > 'dependencies': [ > > '<(DEPTH)/third_party/cld/cld.gyp:cld', > > }, > > }], > > As of November 20, 2014 the sole remaining platform using CLD1 is Android. Our > API is only available on desktop. > In i18n.cc file we also only have support for CLD2 as per reason above, so I > guess it would be unnecessary to add a dependency for CLD1? that sounds reasonable. lgtm
rockot@chromium.org changed reviewers: - andrewhayden@chromium.org
also i didn't see toyoshim@ on the reviewer list already, so adding andrewhayden@ to reviewers was unnecessary - removed
The CQ bit was checked by amalika@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from toyoshim@chromium.org, asvitkine@chromium.org, kalman@chromium.org Link to the patchset: https://codereview.chromium.org/1208993011/#ps300001 (title: "Style errors fixed")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1208993011/300001
Message was sent while issue was closed.
Committed patchset #11 (id:300001)
Message was sent while issue was closed.
Patchset 11 (id:??) landed as https://crrev.com/0cf819bae6edb14d99b5731d6a37cf2b8d7e4f82 Cr-Commit-Position: refs/heads/master@{#339819}
Message was sent while issue was closed.
I've created a revert of this CL: https://codereview.chromium.org/1244343002/ (suspected of breaking sizes on Windows; see http://crbug.com/512666). |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
