Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2796323002:
Network traffic annotation added to translate_url_fetcher. (Closed)
|
Created:
3 years, 8 months ago by Ramin Halavati Modified:
3 years, 7 months ago CC:
chromium-reviews, battre, yyushkina Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionNetwork traffic annotation added to translate_url_fetcher.
Network traffic annotation is added to network request of
components/translate/core/browser/translate_url_fetcher.cc
BUG=656607
Review-Url: https://codereview.chromium.org/2796323002
Cr-Commit-Position: refs/heads/master@{#469284}
Committed: https://chromium.googlesource.com/chromium/src/+/835c333ccaca4bd41342b3e07b9d63f5fe554a52
Patch Set 1 #
Total comments: 23
Patch Set 2 : Annotation updated. #
Total comments: 3
Patch Set 3 : Annotation updated. #
Total comments: 7
Patch Set 4 : Comments addressed. #Patch Set 5 : Comments addressed. #
Total comments: 8
Patch Set 6 : Comments addressed. #
Total comments: 8
Patch Set 7 : Chromium changed to Chrome. #
Total comments: 4
Patch Set 8 : Annotation updated. #Patch Set 9 : Annotation updated. #
Total comments: 2
Patch Set 10 : nits #Messages
Total messages: 41 (8 generated)
rhalavati@chromium.org changed reviewers: + toyoshim@chromium.org
We are annotating all network requests in Chromium with a new NetworkTrafficAnnotation scheme. This allows enterprise users of Chrome to audit the requests and configure Chrome in a way that meets their security policies and expectations. Furthermore, it allows us to generate better debugging data in chrome://net-internals and measure bandwidth consumption on a per-request-type basis. I've modified translate_url_fetcher and added annotation template to it. Please review it and suggest the required answers for the empty parts (marked with '...'). Please note that the claims should be thorough and covering all cases. In case you believe that annotation should be passed to the modified function instead of originating from it, please tell me to change the CL accordingly. Please take a look at the protobuf scheme in: https://cs.chromium.org/chromium/src/tools/traffic_annotation/traffic_annotat... for the definition of the annotations. You can find a sample annotation in: https://cs.chromium.org/chromium/src/components/spellcheck/browser/spelling_s... Entriprise policy options are here: https://cs.chromium.org/chromium/src/out/Debug/gen/components/policy/proto/ch... And more description on enterprise policy settings is here: http://dev.chromium.org/administrators/policy-list-3 Please tell me if you need any clarifications from my side. If you want to learn more, see: https://docs.google.com/document/d/1FWEFTHzdqIA1wHUo_DuSpJjppW22otCMPCOA5NumhJU.
Description was changed from ========== Network traffic annotation added to translate_url_fetcher. Network traffic annotation is added to network request of components/translate/core/browser/translate_url_fetcher.cc BUG=656607 ========== to ========== Network traffic annotation added to translate_url_fetcher. Network traffic annotation is added to network request of components/translate/core/browser/translate_url_fetcher.cc BUG=656607 ==========
toyoshim@chromium.org changed reviewers: + rogerm@chromium.org
Sorry, I took sick leaves last week. Ramin: I added descriptions as review comments. My explanations should cover two of three callers, TranslateLanguageList and TranslateScript. But, I'm not familiar with the third caller, RankerModelLoader. Roger: Can you update my descriptions to cover RankerModelLoader? https://codereview.chromium.org/2796323002/diff/1/components/translate/core/b... File components/translate/core/browser/translate_url_fetcher.cc (right): https://codereview.chromium.org/2796323002/diff/1/components/translate/core/b... components/translate/core/browser/translate_url_fetcher.cc:50: net::NetworkTrafficAnnotationTag traffic_annotation = rogerm: can you modify following my descriptions to followup ranker_model? ranker_model_loader also uses TranslateURLFetcher, but I'm not familiar with it. https://codereview.chromium.org/2796323002/diff/1/components/translate/core/b... components/translate/core/browser/translate_url_fetcher.cc:53: sender: "..." sender: "translate" https://codereview.chromium.org/2796323002/diff/1/components/translate/core/b... components/translate/core/browser/translate_url_fetcher.cc:54: description: "..." description: "Chrome can provide a feature to translate user visiting web sites through Google Translate service. If the feature is enabled, Chrome makes a network request for Google Translate service to know supporting languages, and to fetch a library to perform translations" https://codereview.chromium.org/2796323002/diff/1/components/translate/core/b... components/translate/core/browser/translate_url_fetcher.cc:55: trigger: "..." trigger: "When Chrome translates a web site, it triggers a request to fetch a supporting language list in the first translation, and will trigger another request to fetch the library once a day to make sure it is up to date." https://codereview.chromium.org/2796323002/diff/1/components/translate/core/b... components/translate/core/browser/translate_url_fetcher.cc:56: data: "..." data: "Translation library that is obtained via this interface would perform actual translation, and it will send words and phrases in the site to the server to translate it. But requests that TranslateURLFetcher make should not send anything". https://codereview.chromium.org/2796323002/diff/1/components/translate/core/b... components/translate/core/browser/translate_url_fetcher.cc:57: destination: WEBSITE/GOOGLE_OWNED_SERVICE/OTHER destination: GOOGLE_OWNED_SERVICE https://codereview.chromium.org/2796323002/diff/1/components/translate/core/b... components/translate/core/browser/translate_url_fetcher.cc:61: setting: "..." setting: "chrome://settings/ Languages section has a setting to offer translations and it should stop making network requests for fetching the language list and the library." I'm not sure if this setting affects ranker_model too. https://codereview.chromium.org/2796323002/diff/1/components/translate/core/b... components/translate/core/browser/translate_url_fetcher.cc:64: policy_options {mode: MANDATORY/RECOMMENDED/UNSET} policy_options: RECOMMENDED (according to http://dev.chromium.org/administrators/policy-list-3) https://codereview.chromium.org/2796323002/diff/1/components/translate/core/b... components/translate/core/browser/translate_url_fetcher.cc:65: [POLICY_NAME]: ... //(value to disable it) POLICY_NAME is TranslateEnabled. I think disabling value is "false", but I didn't try it. This policy should overwrite the user setting of chrome://settings. https://codereview.chromium.org/2796323002/diff/1/components/translate/core/b... components/translate/core/browser/translate_url_fetcher.cc:68: policy_exception_justification: "..." n/a
Thank you very much, annotation updated, please review. I've added one inline question, also if you feel that the other use case requires a totally different annotation, I can change the CL so that annotation is passed to this function from the one calling it. https://codereview.chromium.org/2796323002/diff/1/components/translate/core/b... File components/translate/core/browser/translate_url_fetcher.cc (right): https://codereview.chromium.org/2796323002/diff/1/components/translate/core/b... components/translate/core/browser/translate_url_fetcher.cc:53: sender: "..." On 2017/04/10 09:18:57, Takashi Toyoshima wrote: > sender: "translate" Done. https://codereview.chromium.org/2796323002/diff/1/components/translate/core/b... components/translate/core/browser/translate_url_fetcher.cc:54: description: "..." On 2017/04/10 09:18:58, Takashi Toyoshima wrote: > description: "Chrome can provide a feature to translate user visiting web sites > through Google Translate service. If the feature is enabled, Chrome makes a > network request for Google Translate service to know supporting languages, and > to fetch a library to perform translations" Done. https://codereview.chromium.org/2796323002/diff/1/components/translate/core/b... components/translate/core/browser/translate_url_fetcher.cc:56: data: "..." On 2017/04/10 09:18:57, Takashi Toyoshima wrote: > data: "Translation library that is obtained via this interface would perform > actual translation, and it will send words and phrases in the site to the server > to translate it. But requests that TranslateURLFetcher make should not send > anything". Done. https://codereview.chromium.org/2796323002/diff/1/components/translate/core/b... components/translate/core/browser/translate_url_fetcher.cc:57: destination: WEBSITE/GOOGLE_OWNED_SERVICE/OTHER On 2017/04/10 09:18:57, Takashi Toyoshima wrote: > destination: GOOGLE_OWNED_SERVICE Done. https://codereview.chromium.org/2796323002/diff/1/components/translate/core/b... components/translate/core/browser/translate_url_fetcher.cc:61: setting: "..." On 2017/04/10 09:18:58, Takashi Toyoshima wrote: > setting: "chrome://settings/ Languages section has a setting to offer > translations and it should stop making network requests for fetching the > language list and the library." > > I'm not sure if this setting affects ranker_model too. Done. https://codereview.chromium.org/2796323002/diff/1/components/translate/core/b... components/translate/core/browser/translate_url_fetcher.cc:64: policy_options {mode: MANDATORY/RECOMMENDED/UNSET} On 2017/04/10 09:18:58, Takashi Toyoshima wrote: > policy_options: RECOMMENDED (according to > http://dev.chromium.org/administrators/policy-list-3) Thank you. RECOMMENDED=true is the default case. Here we set the value that disables it which is MANDATORY=false. https://codereview.chromium.org/2796323002/diff/1/components/translate/core/b... components/translate/core/browser/translate_url_fetcher.cc:65: [POLICY_NAME]: ... //(value to disable it) On 2017/04/10 09:18:58, Takashi Toyoshima wrote: > POLICY_NAME is TranslateEnabled. I think disabling value is "false", but I > didn't try it. This policy should overwrite the user setting of > chrome://settings. Done. https://codereview.chromium.org/2796323002/diff/1/components/translate/core/b... components/translate/core/browser/translate_url_fetcher.cc:68: policy_exception_justification: "..." On 2017/04/10 09:18:57, Takashi Toyoshima wrote: > n/a Done. https://codereview.chromium.org/2796323002/diff/20001/components/translate/co... File components/translate/core/browser/translate_url_fetcher.cc (right): https://codereview.chromium.org/2796323002/diff/20001/components/translate/co... components/translate/core/browser/translate_url_fetcher.cc:69: "TranslateURLFetcher makes should not send anything." So my understanding is that this network request just asks for the list, without any parameters, and asks for libraries, by sending language code. Actual 'sending the words' are done by another requests from the library and not routed through this request. Am I right?
https://codereview.chromium.org/2796323002/diff/1/components/translate/core/b... File components/translate/core/browser/translate_url_fetcher.cc (right): https://codereview.chromium.org/2796323002/diff/1/components/translate/core/b... components/translate/core/browser/translate_url_fetcher.cc:54: description: "..." On 2017/04/10 09:18:58, Takashi Toyoshima wrote: > description: "Chrome can provide a feature to translate user visiting web sites > through Google Translate service. If the feature is enabled, Chrome makes a > network request for Google Translate service to know supporting languages, and > to fetch a library to perform translations" How about: Chrome can provide translations for the web sites visited by the user. If this feature is enabled, Chrome sends network requests on startup to download the list of supported languages, a library to perform translations, and a predictive model to know when to offer translation.
lgtm if Roger's update is applied. https://codereview.chromium.org/2796323002/diff/1/components/translate/core/b... File components/translate/core/browser/translate_url_fetcher.cc (right): https://codereview.chromium.org/2796323002/diff/1/components/translate/core/b... components/translate/core/browser/translate_url_fetcher.cc:54: description: "..." Thanks, Roger. Ramin, can you update your patch set with this modified description? https://codereview.chromium.org/2796323002/diff/1/components/translate/core/b... components/translate/core/browser/translate_url_fetcher.cc:64: policy_options {mode: MANDATORY/RECOMMENDED/UNSET} Thank you for correction. https://codereview.chromium.org/2796323002/diff/20001/components/translate/co... File components/translate/core/browser/translate_url_fetcher.cc (right): https://codereview.chromium.org/2796323002/diff/20001/components/translate/co... components/translate/core/browser/translate_url_fetcher.cc:69: "TranslateURLFetcher makes should not send anything." Yes, right. That library is written in JavaScript, and Chrome Translate injects the script into the translating page behind a dedicated v8 isolated world. Since 'sending the words' are performed by the injected script, it does not use net::URLFetcher directly, but it just uses HTML/JavaScript functionalities to communicate with the server.
rhalavati@chromium.org changed reviewers: + msramek@chromium.org
Takashi, Roger, Thank you, I made a minor update in data, please review. Martin, Any comments? https://codereview.chromium.org/2796323002/diff/1/components/translate/core/b... File components/translate/core/browser/translate_url_fetcher.cc (right): https://codereview.chromium.org/2796323002/diff/1/components/translate/core/b... components/translate/core/browser/translate_url_fetcher.cc:54: description: "..." On 2017/04/11 06:39:26, Takashi Toyoshima wrote: > Thanks, Roger. > > Ramin, can you update your patch set with this modified description? Done. https://codereview.chromium.org/2796323002/diff/1/components/translate/core/b... components/translate/core/browser/translate_url_fetcher.cc:55: trigger: "..." On 2017/04/10 09:18:57, Takashi Toyoshima wrote: > trigger: "When Chrome translates a web site, it triggers a request to fetch a > supporting language list in the first translation, and will trigger another > request to fetch the library once a day to make sure it is up to date." Done. https://codereview.chromium.org/2796323002/diff/20001/components/translate/co... File components/translate/core/browser/translate_url_fetcher.cc (right): https://codereview.chromium.org/2796323002/diff/20001/components/translate/co... components/translate/core/browser/translate_url_fetcher.cc:69: "TranslateURLFetcher makes should not send anything." On 2017/04/11 06:39:26, Takashi Toyoshima wrote: > Yes, right. > > That library is written in JavaScript, and Chrome Translate injects the script > into the translating page behind a dedicated v8 isolated world. Since 'sending > the words' are performed by the injected script, it does not use net::URLFetcher > directly, but it just uses HTML/JavaScript functionalities to communicate with > the server. Done.
still lgtm https://codereview.chromium.org/2796323002/diff/40001/components/translate/co... File components/translate/core/browser/translate_url_fetcher.cc (right): https://codereview.chromium.org/2796323002/diff/40001/components/translate/co... components/translate/core/browser/translate_url_fetcher.cc:66: "Language id of the library that is required. Translation library " > Language id of the library that is required. Precisely said, Chrome send the current locale on fetching the list of supported languages. This is for obtaining human readable language names in users' language (but these strings are not used today, AFAIK). So, this is not for the library.
added some clarifying info you might consider Otherwise, LGTM. https://codereview.chromium.org/2796323002/diff/40001/components/translate/co... File components/translate/core/browser/translate_url_fetcher.cc (right): https://codereview.chromium.org/2796323002/diff/40001/components/translate/co... components/translate/core/browser/translate_url_fetcher.cc:61: "When Chromium translates a web site, it triggers a request to " I'm not sure what level of detail/precision is required... but, here's a clarification (targeted to the CL author, not the end user; so, some word-smith-fu required): Chrome downloads the predictive model on startup. It downloads the language list and library the first time Chrome considers offering a translation.
https://codereview.chromium.org/2796323002/diff/40001/components/translate/co... File components/translate/core/browser/translate_url_fetcher.cc (right): https://codereview.chromium.org/2796323002/diff/40001/components/translate/co... components/translate/core/browser/translate_url_fetcher.cc:61: "When Chromium translates a web site, it triggers a request to " Do you think the predictive model downloading can be stopped by the translation settings and enterprise policy? I remind that the language detection is exposed to users via Chrome Extension API. So it probably continues to work even if the translation is disabled? https://developer.chrome.com/extensions/tabs#method-detectLanguage
Thank you very much, I updated the annotation and added your last comment in 'settings' field. Does your last comment mean that even if a policy is defined to stop translation, Chrome Extension API may trigger this request? https://codereview.chromium.org/2796323002/diff/40001/components/translate/co... File components/translate/core/browser/translate_url_fetcher.cc (right): https://codereview.chromium.org/2796323002/diff/40001/components/translate/co... components/translate/core/browser/translate_url_fetcher.cc:61: "When Chromium translates a web site, it triggers a request to " On 2017/04/11 14:04:19, Roger McFarlane (Chromium) wrote: > I'm not sure what level of detail/precision is required... but, here's a > clarification (targeted to the CL author, not the end user; so, some > word-smith-fu required): > > Chrome downloads the predictive model on startup. It downloads the language list > and library the first time Chrome considers offering a translation. > Done. https://codereview.chromium.org/2796323002/diff/40001/components/translate/co... components/translate/core/browser/translate_url_fetcher.cc:61: "When Chromium translates a web site, it triggers a request to " On 2017/04/12 05:49:18, Takashi Toyoshima wrote: > Do you think the predictive model downloading can be stopped by the translation > settings and enterprise policy? > > I remind that the language detection is exposed to users via Chrome Extension > API. So it probably continues to work even if the translation is disabled? > > https://developer.chrome.com/extensions/tabs#method-detectLanguage Acknowledged. https://codereview.chromium.org/2796323002/diff/40001/components/translate/co... components/translate/core/browser/translate_url_fetcher.cc:66: "Language id of the library that is required. Translation library " On 2017/04/11 10:00:06, Takashi Toyoshima wrote: > > Language id of the library that is required. > > Precisely said, Chrome send the current locale on fetching the list of supported > languages. This is for obtaining human readable language names in users' > language (but these strings are not used today, AFAIK). > > So, this is not for the library. Done.
https://codereview.chromium.org/2796323002/diff/40001/components/translate/co... File components/translate/core/browser/translate_url_fetcher.cc (right): https://codereview.chromium.org/2796323002/diff/40001/components/translate/co... components/translate/core/browser/translate_url_fetcher.cc:61: "When Chromium translates a web site, it triggers a request to " On 2017/04/12 05:49:18, Takashi Toyoshima wrote: > Do you think the predictive model downloading can be stopped by the translation > settings and enterprise policy? > > I remind that the language detection is exposed to users via Chrome Extension > API. So it probably continues to work even if the translation is disabled? > > https://developer.chrome.com/extensions/tabs#method-detectLanguage That's a good idea. But currently no, the download happens if the user's doesn't have the latest model already cached locally. Filed http://crbug.com/710851
Ramin: Yes, even if the translation is disabled by settings, language detection runs on each page navigation. This happens regardless of extension API calls. The API is just for obtaining the detected language id. As Roger said, currently the language detection would download the latest ranker model at the first run if Chrome does not have it yet.
On 2017/04/13 03:43:21, Takashi Toyoshima wrote: > Ramin: > Yes, even if the translation is disabled by settings, language detection runs on > each page navigation. This happens regardless of extension API calls. The API is > just for obtaining the detected language id. > As Roger said, currently the language detection would download the latest ranker > model at the first run if Chrome does not have it yet. Thanks Takashi, Roger. I have some questions/verifications to clarify my image of the whole thing: 1- So regardless of the settings or policy, at each Chrome startup, it checks if the latest language model is cached? To do so, does it send anything or just queries the version of latest model? 2- Do Chrome Extension API calls (or responses) use this request? If not, do we need to mention them here? 3- Does the settings just controls offering translation and downloading the library? Is the library downloaded (by this request) after user chooses to translate and the library is not already cached?
> 1- So regardless of the settings or policy, at each Chrome startup, it checks if > the latest language model is cached? To do so, does it send anything or just > queries the version of latest model? Roger would know better on this. > 2- Do Chrome Extension API calls (or responses) use this request? If not, do we > need to mention them here? No and No. > 3- Does the settings just controls offering translation and downloading the > library? Is the library downloaded (by this request) after user chooses to > translate and the library is not already cached? OK, let me clarify details. Also, I investigated current behavior, and noticed an unexpected behavior. On launching Chrome, it will download the list of supported languages if the translation is enabled. Otherwise, it does not. (Sorry, I thought this download happens on the first translation, but I overlooked code that also trigger this download on launching) When user make the first navigation, language detection runs against the page after page load finishes. This will require the latest language model, but I'm not sure the exact timing when Chrome downloads the model. Roger can explain this. After the language detection finishes, Chrome decide if chrome would offer a translation only when the translation is enabled. If chrome decide to offer a translation, it popup a bubble UI. Even if it decide not to offer, Chrome still have an user interface to trigger translation. If users triggers a translation via the bubble UI or others, Chrome checks if it has a fresh library that is fetched within 24 hours. Chrome does not cache the library beyond browser sessions. If Chrome does not have a fresh library, Chrome downloads the library and uses it to translate the page. The library would send words and phrases via XHR to translate it to the target language. The was one more unaware case, probably a bug we should fix. When Chrome open chrome://settings, it downloads the list of supported language list even if the translation is disabled. I filed a bug for this (crbug.com/711217).
Re: 1 It is a cookie-less GET request with no payload to download the model from a URL specified in the client's Finch configuration. On Apr 13, 2017 2:01 AM, <rhalavati@chromium.org> wrote: > On 2017/04/13 03:43:21, Takashi Toyoshima wrote: > > Ramin: > > Yes, even if the translation is disabled by settings, language detection > runs > on > > each page navigation. This happens regardless of extension API calls. > The API > is > > just for obtaining the detected language id. > > As Roger said, currently the language detection would download the latest > ranker > > model at the first run if Chrome does not have it yet. > > Thanks Takashi, Roger. I have some questions/verifications to clarify my > image > of the whole thing: > > 1- So regardless of the settings or policy, at each Chrome startup, it > checks if > the latest language model is cached? To do so, does it send anything or > just > queries the version of latest model? > > 2- Do Chrome Extension API calls (or responses) use this request? If not, > do we > need to mention them here? > > 3- Does the settings just controls offering translation and downloading the > library? Is the library downloaded (by this request) after user chooses to > translate and the library is not already cached? > > https://codereview.chromium.org/2796323002/ > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
Hit send too early... It first loads the model from disk cache. If the cached model came from the same URL as specified by Finch and has not expired the chrome does NOT make any request to fetch the model. Models are stored in gstatic and are immutable and indefinitely cache-able. Model update is done by sending a new URL via Finch. On Apr 13, 2017 10:17 AM, "Roger McFarlane" <rogerm@chromium.org> wrote: > Re: 1 > > It is a cookie-less GET request with no payload to download the model from > a URL specified in the client's Finch configuration. > > On Apr 13, 2017 2:01 AM, <rhalavati@chromium.org> wrote: > >> On 2017/04/13 03:43:21, Takashi Toyoshima wrote: >> > Ramin: >> > Yes, even if the translation is disabled by settings, language >> detection runs >> on >> > each page navigation. This happens regardless of extension API calls. >> The API >> is >> > just for obtaining the detected language id. >> > As Roger said, currently the language detection would download the >> latest >> ranker >> > model at the first run if Chrome does not have it yet. >> >> Thanks Takashi, Roger. I have some questions/verifications to clarify my >> image >> of the whole thing: >> >> 1- So regardless of the settings or policy, at each Chrome startup, it >> checks if >> the latest language model is cached? To do so, does it send anything or >> just >> queries the version of latest model? >> >> 2- Do Chrome Extension API calls (or responses) use this request? If not, >> do we >> need to mention them here? >> >> 3- Does the settings just controls offering translation and downloading >> the >> library? Is the library downloaded (by this request) after user chooses to >> translate and the library is not already cached? >> >> https://codereview.chromium.org/2796323002/ >> > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
Roger, Takashi, Thank you very much. I updated the text based on your comments, please review again.
https://codereview.chromium.org/2796323002/diff/80001/components/translate/co... File components/translate/core/browser/translate_url_fetcher.cc (right): https://codereview.chromium.org/2796323002/diff/80001/components/translate/co... components/translate/core/browser/translate_url_fetcher.cc:64: "latest model is provided by finch. If latest model is already " Probably, we should not use the word 'finch' outside chrome/ directory? "field trial" or "field study" is the common word to explain this in content, blink, and probably in components. https://codereview.chromium.org/2796323002/diff/80001/components/translate/co... components/translate/core/browser/translate_url_fetcher.cc:71: "Current local is sent to fetch the list of supperted lanaguges. " s/local/locale/ https://codereview.chromium.org/2796323002/diff/80001/components/translate/co... components/translate/core/browser/translate_url_fetcher.cc:83: "settings under Languages." Sorry for changing explanations, but could you also say that "but the list of supported language will be downloaded regardless of the setting". https://codereview.chromium.org/2796323002/diff/80001/components/translate/co... components/translate/core/browser/translate_url_fetcher.cc:89: } shall we have the policy_exception_justification entry too, for the language list downloads? Justification: the list is needed for rendering user interfaces, and Chrome does not send privacy/security sensitive data to the server on downloading it. Please visit https://bugs.chromium.org/p/chromium/issues/detail?id=711217#c6 to see related discussions and PM's decision.
Comments addressed, please review. https://codereview.chromium.org/2796323002/diff/80001/components/translate/co... File components/translate/core/browser/translate_url_fetcher.cc (right): https://codereview.chromium.org/2796323002/diff/80001/components/translate/co... components/translate/core/browser/translate_url_fetcher.cc:64: "latest model is provided by finch. If latest model is already " On 2017/04/18 07:00:08, Takashi Toyoshima wrote: > Probably, we should not use the word 'finch' outside chrome/ directory? "field > trial" or "field study" is the common word to explain this in content, blink, > and probably in components. Done. https://codereview.chromium.org/2796323002/diff/80001/components/translate/co... components/translate/core/browser/translate_url_fetcher.cc:71: "Current local is sent to fetch the list of supperted lanaguges. " On 2017/04/18 07:00:08, Takashi Toyoshima wrote: > s/local/locale/ Done. https://codereview.chromium.org/2796323002/diff/80001/components/translate/co... components/translate/core/browser/translate_url_fetcher.cc:83: "settings under Languages." On 2017/04/18 07:00:08, Takashi Toyoshima wrote: > Sorry for changing explanations, but could you also say that "but the list of > supported language will be downloaded regardless of the setting". Done, thanks for being very precise about it. https://codereview.chromium.org/2796323002/diff/80001/components/translate/co... components/translate/core/browser/translate_url_fetcher.cc:89: } On 2017/04/18 07:00:08, Takashi Toyoshima wrote: > shall we have the policy_exception_justification entry too, for the language > list downloads? > > Justification: the list is needed for rendering user interfaces, and Chrome does > not send privacy/security sensitive data to the server on downloading it. > > Please visit https://bugs.chromium.org/p/chromium/issues/detail?id=711217#c6 to > see related discussions and PM's decision. Done, I have added it, we will make a decision in the next round of reviewing annotations on precise guidelines when both policy and exception justification are available. We might add a new field for it.
Sorry, I noticed this in the last review, but there is one another kind of style nits we may want to fix. Others lgtm. https://codereview.chromium.org/2796323002/diff/100001/components/translate/c... File components/translate/core/browser/translate_url_fetcher.cc (right): https://codereview.chromium.org/2796323002/diff/100001/components/translate/c... components/translate/core/browser/translate_url_fetcher.cc:55: "Chromium can provide translations for the web sites visited by " Sorry, one more nits. According to the style guideline, we should not use "Chromium" in the code base, but use "Chrome" instead, even for the open sourced builds. https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md... https://codereview.chromium.org/2796323002/diff/100001/components/translate/c... components/translate/core/browser/translate_url_fetcher.cc:56: "the user. If this feature is enabled, Chromium sends network " ditto https://codereview.chromium.org/2796323002/diff/100001/components/translate/c... components/translate/core/browser/translate_url_fetcher.cc:61: "When Chromium starts, it downloads the list of supported " ditto https://codereview.chromium.org/2796323002/diff/100001/components/translate/c... components/translate/core/browser/translate_url_fetcher.cc:82: "translate pages that aren't in a language you read.' in Chromium " ditto
Comments addressed, please review. https://codereview.chromium.org/2796323002/diff/100001/components/translate/c... File components/translate/core/browser/translate_url_fetcher.cc (right): https://codereview.chromium.org/2796323002/diff/100001/components/translate/c... components/translate/core/browser/translate_url_fetcher.cc:55: "Chromium can provide translations for the web sites visited by " On 2017/04/18 07:27:36, Takashi Toyoshima wrote: > Sorry, one more nits. > According to the style guideline, we should not use "Chromium" in the code base, > but use "Chrome" instead, even for the open sourced builds. > > https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md... You are right. We had missed this rule. I will change this CL and will create another CL in future to revert all Chromiums to Chrome. https://codereview.chromium.org/2796323002/diff/100001/components/translate/c... components/translate/core/browser/translate_url_fetcher.cc:56: "the user. If this feature is enabled, Chromium sends network " On 2017/04/18 07:27:36, Takashi Toyoshima wrote: > ditto Done. https://codereview.chromium.org/2796323002/diff/100001/components/translate/c... components/translate/core/browser/translate_url_fetcher.cc:61: "When Chromium starts, it downloads the list of supported " On 2017/04/18 07:27:36, Takashi Toyoshima wrote: > ditto Done. https://codereview.chromium.org/2796323002/diff/100001/components/translate/c... components/translate/core/browser/translate_url_fetcher.cc:82: "translate pages that aren't in a language you read.' in Chromium " On 2017/04/18 07:27:36, Takashi Toyoshima wrote: > ditto Done.
lgtm
Thank you Takashi. Martin, Any comments?
https://codereview.chromium.org/2796323002/diff/120001/components/translate/c... File components/translate/core/browser/translate_url_fetcher.cc (right): https://codereview.chromium.org/2796323002/diff/120001/components/translate/c... components/translate/core/browser/translate_url_fetcher.cc:62: "for translation, and a predictive model to detect the language of " The predictive model isn't for language detection; it predicts the utility of prompting the user with the translation (what is the probability the user actually wants/needs the translation). The language detection model is statically linked to Chrome (i.e., not downloaded).
Roger, Comment addressed, please review. https://codereview.chromium.org/2796323002/diff/120001/components/translate/c... File components/translate/core/browser/translate_url_fetcher.cc (right): https://codereview.chromium.org/2796323002/diff/120001/components/translate/c... components/translate/core/browser/translate_url_fetcher.cc:62: "for translation, and a predictive model to detect the language of " On 2017/04/18 20:48:19, Roger McFarlane (Chromium) wrote: > The predictive model isn't for language detection; it predicts the utility of > prompting the user with the translation (what is the probability the user > actually wants/needs the translation). > > The language detection model is statically linked to Chrome (i.e., not > downloaded). Done.
Sorry, I didn't realize you hadn't just made a correction and landed this change already. That said... https://codereview.chromium.org/2796323002/diff/120001/components/translate/c... File components/translate/core/browser/translate_url_fetcher.cc (right): https://codereview.chromium.org/2796323002/diff/120001/components/translate/c... components/translate/core/browser/translate_url_fetcher.cc:62: "for translation, and a predictive model to detect the language of " On 2017/04/19 06:07:05, Ramin Halavati wrote: > On 2017/04/18 20:48:19, Roger McFarlane (Chromium) wrote: > > The predictive model isn't for language detection; it predicts the utility of > > prompting the user with the translation (what is the probability the user > > actually wants/needs the translation). > > > > The language detection model is statically linked to Chrome (i.e., not > > downloaded). > > Done. I'm not sure why you removed so much text. There seems to be some confusion. There are two models: - language detection model: statically linked to chrome - predictive model to infer whether a translation from the detected language to some other language would be useful/accepted by the user. This is downloaded and cached when chrome starts. The previous version of the annotation seemed to be conflating these two models. My comment was that the predictive model is not the same thing as the language detection model (the latter of which is not downloaded).
Roger, I had misunderstood your comment, changed the annotation to reflect that only the prediction model to offer translation is downloaded. Please review. https://codereview.chromium.org/2796323002/diff/120001/components/translate/c... File components/translate/core/browser/translate_url_fetcher.cc (right): https://codereview.chromium.org/2796323002/diff/120001/components/translate/c... components/translate/core/browser/translate_url_fetcher.cc:62: "for translation, and a predictive model to detect the language of " On 2017/05/03 14:34:09, Roger McFarlane (Chromium) wrote: > On 2017/04/19 06:07:05, Ramin Halavati wrote: > > On 2017/04/18 20:48:19, Roger McFarlane (Chromium) wrote: > > > The predictive model isn't for language detection; it predicts the utility > of > > > prompting the user with the translation (what is the probability the user > > > actually wants/needs the translation). > > > > > > The language detection model is statically linked to Chrome (i.e., not > > > downloaded). > > > > Done. > > I'm not sure why you removed so much text. There seems to be some confusion. > > There are two models: > > - language detection model: statically linked to chrome > > - predictive model to infer whether a translation from > the detected language to some other language would be > useful/accepted by the user. This is downloaded and > cached when chrome starts. > > The previous version of the annotation seemed to be conflating these two models. > My comment was that the predictive model is not the same thing as the language > detection model (the latter of which is not downloaded). Oh sorry, I misunderstood it.
lgtm
Thanks Roger. Martin, Any comments?
LGTM https://codereview.chromium.org/2796323002/diff/160001/components/translate/c... File components/translate/core/browser/translate_url_fetcher.cc (right): https://codereview.chromium.org/2796323002/diff/160001/components/translate/c... components/translate/core/browser/translate_url_fetcher.cc:81: "Current locale is sent to fetch the list of supperted lanaguges. " typo: supported
Thank you, comment addressed, landing. https://codereview.chromium.org/2796323002/diff/160001/components/translate/c... File components/translate/core/browser/translate_url_fetcher.cc (right): https://codereview.chromium.org/2796323002/diff/160001/components/translate/c... components/translate/core/browser/translate_url_fetcher.cc:81: "Current locale is sent to fetch the list of supperted lanaguges. " On 2017/05/03 16:43:14, msramek (recovering) wrote: > typo: supported Done.
The CQ bit was checked by rhalavati@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from toyoshim@chromium.org, msramek@chromium.org, rogerm@chromium.org Link to the patchset: https://codereview.chromium.org/2796323002/#ps180001 (title: "nits")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 180001, "attempt_start_ts": 1493873512941070,
"parent_rev": "07f715aee7d56fd113ac152d5e0b34dd1ece3ff6", "commit_rev":
"835c333ccaca4bd41342b3e07b9d63f5fe554a52"}
Message was sent while issue was closed.
Description was changed from ========== Network traffic annotation added to translate_url_fetcher. Network traffic annotation is added to network request of components/translate/core/browser/translate_url_fetcher.cc BUG=656607 ========== to ========== Network traffic annotation added to translate_url_fetcher. Network traffic annotation is added to network request of components/translate/core/browser/translate_url_fetcher.cc BUG=656607 Review-Url: https://codereview.chromium.org/2796323002 Cr-Commit-Position: refs/heads/master@{#469284} Committed: https://chromium.googlesource.com/chromium/src/+/835c333ccaca4bd41342b3e07b9d... ==========
Message was sent while issue was closed.
Committed patchset #10 (id:180001) as https://chromium.googlesource.com/chromium/src/+/835c333ccaca4bd41342b3e07b9d... |
