|
|
Created:
3 years, 7 months ago by renjieliu1 Modified:
3 years, 7 months ago CC:
chromium-reviews, sync-reviews_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionInitial proto for language detection.
Design doc:
https://docs.google.com/document/d/1-q-vrxqNZDnwajpOwMWB_Z1YlzQUiApJZmSUzGkojv4/edit#heading=h.3bxjhvo21dyq
Integration Steps:
https://docs.google.com/document/d/1es1cCRFhdag4ljGSPfXfSMlEx5lMgFZ3SGHMaBcU_D8/edit#
BUG=722679
Review-Url: https://codereview.chromium.org/2892553003
Cr-Commit-Position: refs/heads/master@{#473113}
Committed: https://chromium.googlesource.com/chromium/src/+/0b301f711dbc495c3b552d083d0e6d88d0e9db73
Patch Set 1 #
Total comments: 14
Patch Set 2 : update proto #
Total comments: 4
Patch Set 3 : change proportion to is_reliable #Patch Set 4 : change proportion to is_reliable #
Total comments: 2
Patch Set 5 : add comment #
Total comments: 6
Patch Set 6 : update #
Total comments: 2
Patch Set 7 : fix #
Messages
Total messages: 30 (13 generated)
Description was changed from ========== Initial proto for language detection. Design doc: https://docs.google.com/document/d/1-q-vrxqNZDnwajpOwMWB_Z1YlzQUiApJZmSUzGkoj... Integration Steps: https://docs.google.com/document/d/1es1cCRFhdag4ljGSPfXfSMlEx5lMgFZ3SGHMaBcU_... BUG=722679 ========== to ========== Initial proto for language detection. Design doc: https://docs.google.com/document/d/1-q-vrxqNZDnwajpOwMWB_Z1YlzQUiApJZmSUzGkoj... Integration Steps: https://docs.google.com/document/d/1es1cCRFhdag4ljGSPfXfSMlEx5lMgFZ3SGHMaBcU_... BUG=722679 ==========
renjieliu@chromium.org changed reviewers: + amoylan@chromium.org, napper@chromium.org
https://codereview.chromium.org/2892553003/diff/1/components/sync/protocol/us... File components/sync/protocol/user_event_specifics.proto (right): https://codereview.chromium.org/2892553003/diff/1/components/sync/protocol/us... components/sync/protocol/user_event_specifics.proto:16: // Language detection output Add . to comment https://codereview.chromium.org/2892553003/diff/1/components/sync/protocol/us... components/sync/protocol/user_event_specifics.proto:19: optional string language_code = 1; Can you add a comment as to what language code format is used? https://codereview.chromium.org/2892553003/diff/1/components/sync/protocol/us... components/sync/protocol/user_event_specifics.proto:20: optional int32 percent = 2; int8? https://codereview.chromium.org/2892553003/diff/1/components/sync/protocol/us... components/sync/protocol/user_event_specifics.proto:21: } Add a comment describing what percent is. Also percent doesnt seem like a very clear name. https://codereview.chromium.org/2892553003/diff/1/components/sync/protocol/us... components/sync/protocol/user_event_specifics.proto:22: // Top n languages. Typically we just log the top one language, but for Change to "Typically we just log the top language" https://codereview.chromium.org/2892553003/diff/1/components/sync/protocol/us... components/sync/protocol/user_event_specifics.proto:23: // page that we're not confident, we may log up to 3 top languages. "a page we are not confidence about," https://codereview.chromium.org/2892553003/diff/1/components/sync/protocol/us... components/sync/protocol/user_event_specifics.proto:24: repeated Language languages = 1; Would detected_languages be a better name here?
The CQ bit was checked by renjieliu@chromium.org to run a CQ dry run
thanks for the review! https://codereview.chromium.org/2892553003/diff/1/components/sync/protocol/us... File components/sync/protocol/user_event_specifics.proto (right): https://codereview.chromium.org/2892553003/diff/1/components/sync/protocol/us... components/sync/protocol/user_event_specifics.proto:16: // Language detection output On 2017/05/18 04:55:06, napper wrote: > Add . to comment Done. https://codereview.chromium.org/2892553003/diff/1/components/sync/protocol/us... components/sync/protocol/user_event_specifics.proto:19: optional string language_code = 1; On 2017/05/18 04:55:06, napper wrote: > Can you add a comment as to what language code format is used? Done. https://codereview.chromium.org/2892553003/diff/1/components/sync/protocol/us... components/sync/protocol/user_event_specifics.proto:20: optional int32 percent = 2; On 2017/05/18 04:55:06, napper wrote: > int8? int32 is the minimum we can get for proto buffer :( https://codereview.chromium.org/2892553003/diff/1/components/sync/protocol/us... components/sync/protocol/user_event_specifics.proto:21: } On 2017/05/18 04:55:06, napper wrote: > Add a comment describing what percent is. Also percent doesnt seem like a very > clear name. how about proportion? https://codereview.chromium.org/2892553003/diff/1/components/sync/protocol/us... components/sync/protocol/user_event_specifics.proto:22: // Top n languages. Typically we just log the top one language, but for On 2017/05/18 04:55:07, napper wrote: > Change to "Typically we just log the top language" Done. https://codereview.chromium.org/2892553003/diff/1/components/sync/protocol/us... components/sync/protocol/user_event_specifics.proto:23: // page that we're not confident, we may log up to 3 top languages. On 2017/05/18 04:55:06, napper wrote: > "a page we are not confidence about," Done. https://codereview.chromium.org/2892553003/diff/1/components/sync/protocol/us... components/sync/protocol/user_event_specifics.proto:24: repeated Language languages = 1; On 2017/05/18 04:55:06, napper wrote: > Would detected_languages be a better name here? sounds good :)
thanks for the review!
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2892553003/diff/20001/components/sync/protoco... File components/sync/protocol/user_event_specifics.proto (right): https://codereview.chromium.org/2892553003/diff/20001/components/sync/protoco... components/sync/protocol/user_event_specifics.proto:22: optional int32 proportion = 2; As discussed offline, switch to a bool is_reliable flag. https://codereview.chromium.org/2892553003/diff/20001/components/sync/protoco... components/sync/protocol/user_event_specifics.proto:51: LanguageDetection translate_language_detection = 5; Just language_detection
The CQ bit was checked by renjieliu@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by renjieliu@chromium.org to run a CQ dry run
https://codereview.chromium.org/2892553003/diff/20001/components/sync/protoco... File components/sync/protocol/user_event_specifics.proto (right): https://codereview.chromium.org/2892553003/diff/20001/components/sync/protoco... components/sync/protocol/user_event_specifics.proto:22: optional int32 proportion = 2; On 2017/05/18 06:02:13, napper wrote: > As discussed offline, switch to a bool is_reliable flag. Done. https://codereview.chromium.org/2892553003/diff/20001/components/sync/protoco... components/sync/protocol/user_event_specifics.proto:51: LanguageDetection translate_language_detection = 5; On 2017/05/18 06:02:13, napper wrote: > Just language_detection Done.
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
lgtm
lgtm https://codereview.chromium.org/2892553003/diff/60001/components/sync/protoco... File components/sync/protocol/user_event_specifics.proto (right): https://codereview.chromium.org/2892553003/diff/60001/components/sync/protoco... components/sync/protocol/user_event_specifics.proto:28: optional string adopted_language = 2; Please add a short comment about adopted_language. Especially as we usually ask each other, what exactly that is :)
https://codereview.chromium.org/2892553003/diff/60001/components/sync/protoco... File components/sync/protocol/user_event_specifics.proto (right): https://codereview.chromium.org/2892553003/diff/60001/components/sync/protoco... components/sync/protocol/user_event_specifics.proto:28: optional string adopted_language = 2; On 2017/05/18 06:36:21, amoylan wrote: > Please add a short comment about adopted_language. > Especially as we usually ask each other, what exactly that is :) Done.
renjieliu@chromium.org changed reviewers: + skym@chromium.org
Hi Sky, I'm implementing chrome language event logging. I followed your sample cl here: https://codereview.chromium.org/2875203002/ The final proto we have discussed is here: https://docs.google.com/document/d/1-q-vrxqNZDnwajpOwMWB_Z1YlzQUiApJZmSUzGkoj... Do you mind taking a look? Thanks a lot!
lgtm https://codereview.chromium.org/2892553003/diff/80001/components/sync/protoco... File components/sync/protocol/user_event_specifics.proto (right): https://codereview.chromium.org/2892553003/diff/80001/components/sync/protoco... components/sync/protocol/user_event_specifics.proto:25: // Top n languages. Typically we just log the top language, but for page that Is it correct to assume that this is an ordered list, where if there are 3 languages, the highest confidence should be first, and the lowest should be last? https://codereview.chromium.org/2892553003/diff/80001/components/sync/protoco... components/sync/protocol/user_event_specifics.proto:32: message FieldTrialEvent { Can you put this above the translate message definition? I was hoping to keep the order the same as in the oneof below.
https://codereview.chromium.org/2892553003/diff/80001/components/sync/protoco... File components/sync/protocol/user_event_specifics.proto (right): https://codereview.chromium.org/2892553003/diff/80001/components/sync/protoco... components/sync/protocol/user_event_specifics.proto:29: optional string adopted_language = 2; What if we only specify the adopted language if it is different from the first detected language? I think this would save a lot of space.
thanks for the review! https://codereview.chromium.org/2892553003/diff/80001/components/sync/protoco... File components/sync/protocol/user_event_specifics.proto (right): https://codereview.chromium.org/2892553003/diff/80001/components/sync/protoco... components/sync/protocol/user_event_specifics.proto:25: // Top n languages. Typically we just log the top language, but for page that On 2017/05/19 03:22:35, skym wrote: > Is it correct to assume that this is an ordered list, where if there are 3 > languages, the highest confidence should be first, and the lowest should be > last? yes :) https://codereview.chromium.org/2892553003/diff/80001/components/sync/protoco... components/sync/protocol/user_event_specifics.proto:29: optional string adopted_language = 2; On 2017/05/19 03:30:58, napper wrote: > What if we only specify the adopted language if it is different from the first > detected language? I think this would save a lot of space. Done. https://codereview.chromium.org/2892553003/diff/80001/components/sync/protoco... components/sync/protocol/user_event_specifics.proto:32: message FieldTrialEvent { On 2017/05/19 03:22:35, skym wrote: > Can you put this above the translate message definition? I was hoping to keep > the order the same as in the oneof below. Done.
https://codereview.chromium.org/2892553003/diff/100001/components/sync/protoc... File components/sync/protocol/user_event_specifics.proto (right): https://codereview.chromium.org/2892553003/diff/100001/components/sync/protoc... components/sync/protocol/user_event_specifics.proto:38: // It will be stored only it's different from the first detected language. only if it's
https://codereview.chromium.org/2892553003/diff/100001/components/sync/protoc... File components/sync/protocol/user_event_specifics.proto (right): https://codereview.chromium.org/2892553003/diff/100001/components/sync/protoc... components/sync/protocol/user_event_specifics.proto:38: // It will be stored only it's different from the first detected language. On 2017/05/19 04:16:55, napper wrote: > only if it's Done. Thanks :)
The CQ bit was checked by renjieliu@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from napper@chromium.org, amoylan@chromium.org, skym@chromium.org Link to the patchset: https://codereview.chromium.org/2892553003/#ps120001 (title: "fix")
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": 120001, "attempt_start_ts": 1495172231991540, "parent_rev": "6dc2830c525f150b928052245fd669834eee5f4c", "commit_rev": "0b301f711dbc495c3b552d083d0e6d88d0e9db73"}
Message was sent while issue was closed.
Description was changed from ========== Initial proto for language detection. Design doc: https://docs.google.com/document/d/1-q-vrxqNZDnwajpOwMWB_Z1YlzQUiApJZmSUzGkoj... Integration Steps: https://docs.google.com/document/d/1es1cCRFhdag4ljGSPfXfSMlEx5lMgFZ3SGHMaBcU_... BUG=722679 ========== to ========== Initial proto for language detection. Design doc: https://docs.google.com/document/d/1-q-vrxqNZDnwajpOwMWB_Z1YlzQUiApJZmSUzGkoj... Integration Steps: https://docs.google.com/document/d/1es1cCRFhdag4ljGSPfXfSMlEx5lMgFZ3SGHMaBcU_... BUG=722679 Review-Url: https://codereview.chromium.org/2892553003 Cr-Commit-Position: refs/heads/master@{#473113} Committed: https://chromium.googlesource.com/chromium/src/+/0b301f711dbc495c3b552d083d0e... ==========
Message was sent while issue was closed.
Committed patchset #7 (id:120001) as https://chromium.googlesource.com/chromium/src/+/0b301f711dbc495c3b552d083d0e... |