Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2897563002:
Implement client side logging for language detection. (Closed)
|
Created:
3 years, 7 months ago by renjieliu1 Modified:
3 years, 7 months ago CC:
chromium-reviews Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionImplement client side logging for language detection.
Integration steps described in:
https://docs.google.com/document/d/1es1cCRFhdag4ljGSPfXfSMlEx5lMgFZ3SGHMaBcU_D8/edit#heading=h.qdctas2f6c6e
BUG=722679
Review-Url: https://codereview.chromium.org/2897563002
Cr-Commit-Position: refs/heads/master@{#474972}
Committed: https://chromium.googlesource.com/chromium/src/+/d8a3849678bf6750fa1a0a6cfbfeab9744070cc4
Patch Set 1 #Patch Set 2 : fix empty entry #
Total comments: 4
Patch Set 3 : fix #
Total comments: 12
Patch Set 4 : updates #
Total comments: 13
Patch Set 5 : add comment #
Total comments: 1
Patch Set 6 : update comment #
Messages
Total messages: 52 (33 generated)
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...
renjieliu@chromium.org changed reviewers: + napper@chromium.org
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
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 unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
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 unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
fix the try bots, PTAL
lgtm https://codereview.chromium.org/2897563002/diff/20001/chrome/browser/translat... File chrome/browser/translate/chrome_translate_client.cc (right): https://codereview.chromium.org/2897563002/diff/20001/chrome/browser/translat... chrome/browser/translate/chrome_translate_client.cc:104: syncer::UserEventService* user_event_service = * const https://codereview.chromium.org/2897563002/diff/20001/chrome/browser/translat... chrome/browser/translate/chrome_translate_client.cc:107: auto* entry = web_contents->GetController().GetLastCommittedEntry(); const auto* const?
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...
thanks for the review! https://codereview.chromium.org/2897563002/diff/20001/chrome/browser/translat... File chrome/browser/translate/chrome_translate_client.cc (right): https://codereview.chromium.org/2897563002/diff/20001/chrome/browser/translat... chrome/browser/translate/chrome_translate_client.cc:104: syncer::UserEventService* user_event_service = On 2017/05/23 03:15:23, napper wrote: > * const Done. https://codereview.chromium.org/2897563002/diff/20001/chrome/browser/translat... chrome/browser/translate/chrome_translate_client.cc:107: auto* entry = web_contents->GetController().GetLastCommittedEntry(); On 2017/05/23 03:15:23, napper wrote: > const auto* const? Done.
Description was changed from ========== Implement client side logging for language detection. Integration steps described in: https://docs.google.com/document/d/1es1cCRFhdag4ljGSPfXfSMlEx5lMgFZ3SGHMaBcU_... BUG=722679 ========== to ========== Implement client side logging for language detection. Integration steps described in: https://docs.google.com/document/d/1es1cCRFhdag4ljGSPfXfSMlEx5lMgFZ3SGHMaBcU_... BUG=722679 ==========
renjieliu@chromium.org changed reviewers: + skym@chromium.org
Hi Sky, I have implemented the gaia logging as described here: https://docs.google.com/document/d/1es1cCRFhdag4ljGSPfXfSMlEx5lMgFZ3SGHMaBcU_... would you mind taking a look? Also, I wonder what's the next step for us? Thanks a lot!
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm If there are any Finch trial groups you care about, you could register for them as well. https://codereview.chromium.org/2897563002/diff/40001/chrome/browser/translat... File chrome/browser/translate/chrome_translate_client.cc (right): https://codereview.chromium.org/2897563002/diff/40001/chrome/browser/translat... chrome/browser/translate/chrome_translate_client.cc:79: specifics->set_navigation_id(entry_id); So, this is kind of an open question, what's the best way to identify navigations. For the time being, you need to use the timestamp of the navigation entry->GetTimestamp().ToInternalValue() At the very least, the field UserEventSpecifics::navigation_id is poorly named. I don't think you're the only one that's going to jump to this assumption. You can read more ramblings about identifying navigations at https://docs.google.com/document/d/1ZeMUbFCxUkAlIOzOv4r9eXhLemZH2VR8npfiyXigL... , we haven't agreed on what is the best approach going forwards, and nothing has been implemented to really solve things yet. We might eventually go back to using NavigationEntry::GetUniqueID(), but we're not there yet. https://codereview.chromium.org/2897563002/diff/40001/chrome/browser/translat... chrome/browser/translate/chrome_translate_client.cc:82: auto* const lang = lang_detection.add_detected_languages(); I'm not a huge fan of this auto, but I suppose this is largely stylistic. https://codereview.chromium.org/2897563002/diff/40001/chrome/browser/translat... chrome/browser/translate/chrome_translate_client.cc:87: lang_detection.set_adopted_language(details.adopted_language); I'm sorry, I should have commented about this when you made the proto, but what do you think of renaming "adopted_language" to "adopted_language_code"? It seems confusing given there's a concept of a sync_pb::Langauge, of which this field is not. https://codereview.chromium.org/2897563002/diff/40001/chrome/browser/translat... chrome/browser/translate/chrome_translate_client.cc:99: if (profile == nullptr) { Can this happen? Just convenience for tests? https://codereview.chromium.org/2897563002/diff/40001/chrome/browser/translat... chrome/browser/translate/chrome_translate_client.cc:103: // Get user event service. I'd omit this comment. https://codereview.chromium.org/2897563002/diff/40001/chrome/browser/translat... chrome/browser/translate/chrome_translate_client.cc:112: return; Early return to avoid a single expression below seems a bit drastic to me.
thank you for the review! https://codereview.chromium.org/2897563002/diff/40001/chrome/browser/translat... File chrome/browser/translate/chrome_translate_client.cc (right): https://codereview.chromium.org/2897563002/diff/40001/chrome/browser/translat... chrome/browser/translate/chrome_translate_client.cc:79: specifics->set_navigation_id(entry_id); On 2017/05/23 22:45:42, skym wrote: > So, this is kind of an open question, what's the best way to identify > navigations. For the time being, you need to use the timestamp of the navigation > > entry->GetTimestamp().ToInternalValue() > > At the very least, the field UserEventSpecifics::navigation_id is poorly named. > I don't think you're the only one that's going to jump to this assumption. > > You can read more ramblings about identifying navigations at > https://docs.google.com/document/d/1ZeMUbFCxUkAlIOzOv4r9eXhLemZH2VR8npfiyXigL... > , we haven't agreed on what is the best approach going forwards, and nothing has > been implemented to really solve things yet. > > We might eventually go back to using NavigationEntry::GetUniqueID(), but we're > not there yet. thank you for the clarification! https://codereview.chromium.org/2897563002/diff/40001/chrome/browser/translat... chrome/browser/translate/chrome_translate_client.cc:82: auto* const lang = lang_detection.add_detected_languages(); On 2017/05/23 22:45:42, skym wrote: > I'm not a huge fan of this auto, but I suppose this is largely stylistic. Acknowledged. https://codereview.chromium.org/2897563002/diff/40001/chrome/browser/translat... chrome/browser/translate/chrome_translate_client.cc:87: lang_detection.set_adopted_language(details.adopted_language); On 2017/05/23 22:45:42, skym wrote: > I'm sorry, I should have commented about this when you made the proto, but what > do you think of renaming "adopted_language" to "adopted_language_code"? It seems > confusing given there's a concept of a sync_pb::Langauge, of which this field is > not. That's a good suggestion! Changed the proto. https://codereview.chromium.org/2897563002/diff/40001/chrome/browser/translat... chrome/browser/translate/chrome_translate_client.cc:99: if (profile == nullptr) { On 2017/05/23 22:45:41, skym wrote: > Can this happen? Just convenience for tests? you're right, previously I thought if user is not logged in the profile would be null, but apparently if user's not logged in, a default profile will be returned. https://codereview.chromium.org/2897563002/diff/40001/chrome/browser/translat... chrome/browser/translate/chrome_translate_client.cc:103: // Get user event service. On 2017/05/23 22:45:42, skym wrote: > I'd omit this comment. Done. https://codereview.chromium.org/2897563002/diff/40001/chrome/browser/translat... chrome/browser/translate/chrome_translate_client.cc:112: return; On 2017/05/23 22:45:42, skym wrote: > Early return to avoid a single expression below seems a bit drastic to me. Done.
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 unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
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, skym@chromium.org Link to the patchset: https://codereview.chromium.org/2897563002/#ps60001 (title: "updates")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
renjieliu@chromium.org changed reviewers: + groby@chromium.org
Hi Rachel, We're doing client side logging for page language detection, can you do a owner review on the cl? Thanks a lot! :)
Since there is some logic, does it make sense to add a test? https://codereview.chromium.org/2897563002/diff/60001/chrome/browser/translat... File chrome/browser/translate/chrome_translate_client.cc (right): https://codereview.chromium.org/2897563002/diff/60001/chrome/browser/translat... chrome/browser/translate/chrome_translate_client.cc:81: specifics->set_navigation_id(base::Time::Now().ToInternalValue()); Note: Time::Now() is not guaranteed to be increasing (monotonically or otherwise) - if that is an issue, you'll need to find another time base. https://codereview.chromium.org/2897563002/diff/60001/chrome/browser/translat... chrome/browser/translate/chrome_translate_client.cc:84: auto* const lang = lang_detection.add_detected_languages(); Wait - isn't this a mutable pointer to a constant lang? I.e. the set_language_code() calls should fail? You probably want either auto const*, or better auto* https://codereview.chromium.org/2897563002/diff/60001/chrome/browser/translat... chrome/browser/translate/chrome_translate_client.cc:107: // If entry is null, we don't record the page. The logic is clear from the code - can you change the comment to explain _why_ we don't record the page on NullPtr? (Also, if we rely on NavigationEntry, should this be a NavigationObserver?)
https://codereview.chromium.org/2897563002/diff/60001/chrome/browser/translat... File chrome/browser/translate/chrome_translate_client.cc (right): https://codereview.chromium.org/2897563002/diff/60001/chrome/browser/translat... chrome/browser/translate/chrome_translate_client.cc:81: specifics->set_navigation_id(base::Time::Now().ToInternalValue()); On 2017/05/25 at 21:35:29, groby wrote: > Note: Time::Now() is not guaranteed to be increasing (monotonically or otherwise) - if that is an issue, you'll need to find another time base. See here for the ongoing discussion: https://docs.google.com/document/d/1ZeMUbFCxUkAlIOzOv4r9eXhLemZH2VR8npfiyXigL... https://codereview.chromium.org/2897563002/diff/60001/chrome/browser/translat... chrome/browser/translate/chrome_translate_client.cc:84: auto* const lang = lang_detection.add_detected_languages(); On 2017/05/25 at 21:35:29, groby wrote: > Wait - isn't this a mutable pointer to a constant lang? I.e. the set_language_code() calls should fail? > > You probably want either auto const*, or better auto* Note that the pointer is const (so lang++ won't work) but the object pointed to by the pointer is non-const (so lang->set_language_code() will work).
Hi Rachel, Thanks for the review! As for test, I totally agreed with you that we should add a test. However, since we're getting UserEventService through UserEventFactory::GetForProfile https://cs.chromium.org/chromium/src/chrome/browser/sync/user_event_service_f... It is really hard to test for the current setting without modifying a lot. I think what we can do in future is we can add a test util in UserEventServiceFactory to take TestingProfile https://cs.chromium.org/chromium/src/chrome/test/base/testing_profile.h?gsn=P... and return a testing UserEventService. I think this can make the unit test easier. WDYT? +skym Thanks a lot! https://codereview.chromium.org/2897563002/diff/60001/chrome/browser/translat... File chrome/browser/translate/chrome_translate_client.cc (right): https://codereview.chromium.org/2897563002/diff/60001/chrome/browser/translat... chrome/browser/translate/chrome_translate_client.cc:81: specifics->set_navigation_id(base::Time::Now().ToInternalValue()); On 2017/05/25 21:49:26, napper wrote: > On 2017/05/25 at 21:35:29, groby wrote: > > Note: Time::Now() is not guaranteed to be increasing (monotonically or > otherwise) - if that is an issue, you'll need to find another time base. > > See here for the ongoing discussion: > https://docs.google.com/document/d/1ZeMUbFCxUkAlIOzOv4r9eXhLemZH2VR8npfiyXigL... Acknowledged. https://codereview.chromium.org/2897563002/diff/60001/chrome/browser/translat... chrome/browser/translate/chrome_translate_client.cc:81: specifics->set_navigation_id(base::Time::Now().ToInternalValue()); On 2017/05/25 21:35:29, groby wrote: > Note: Time::Now() is not guaranteed to be increasing (monotonically or > otherwise) - if that is an issue, you'll need to find another time base. currently the suggested way is to use time to identify the navigation, we probably need to change that in future https://codereview.chromium.org/2897563002/diff/60001/chrome/browser/translat... chrome/browser/translate/chrome_translate_client.cc:84: auto* const lang = lang_detection.add_detected_languages(); On 2017/05/25 21:49:26, napper wrote: > On 2017/05/25 at 21:35:29, groby wrote: > > Wait - isn't this a mutable pointer to a constant lang? I.e. the > set_language_code() calls should fail? > > > > You probably want either auto const*, or better auto* > > Note that the pointer is const (so lang++ won't work) but the object pointed to > by the pointer is non-const (so lang->set_language_code() will work). thanks for the clarification! :) https://codereview.chromium.org/2897563002/diff/60001/chrome/browser/translat... chrome/browser/translate/chrome_translate_client.cc:84: auto* const lang = lang_detection.add_detected_languages(); On 2017/05/25 21:35:29, groby wrote: > Wait - isn't this a mutable pointer to a constant lang? I.e. the > set_language_code() calls should fail? > > You probably want either auto const*, or better auto* > this is to a const pointer to a mutable object https://codereview.chromium.org/2897563002/diff/60001/chrome/browser/translat... chrome/browser/translate/chrome_translate_client.cc:107: // If entry is null, we don't record the page. On 2017/05/25 21:35:29, groby wrote: > The logic is clear from the code - can you change the comment to explain _why_ > we don't record the page on NullPtr? (Also, if we rely on NavigationEntry, > should this be a NavigationObserver?) Added a comment. I wonder are you suggesting web_contents_observer: https://cs.chromium.org/chromium/src/content/public/browser/web_contents_obse... Currently we're relying on CLD3 to determine the language and Register the information in the page(https://cs.chromium.org/chromium/src/components/translate/content/renderer/translate_helper.cc?q=translate_helper&l=132), thus we can get the language info in ChromeTranslateClient::OnLanguageDetermined.
lgtm https://codereview.chromium.org/2897563002/diff/60001/chrome/browser/translat... File chrome/browser/translate/chrome_translate_client.cc (right): https://codereview.chromium.org/2897563002/diff/60001/chrome/browser/translat... chrome/browser/translate/chrome_translate_client.cc:81: specifics->set_navigation_id(base::Time::Now().ToInternalValue()); On 2017/05/26 00:26:45, renjieliu1 wrote: > On 2017/05/25 21:35:29, groby wrote: > > Note: Time::Now() is not guaranteed to be increasing (monotonically or > > otherwise) - if that is an issue, you'll need to find another time base. > > currently the suggested way is to use time to identify the navigation, we > probably need to change that in future I'm not sure what your requirements are, so pick what works for you :) Just pointing out the weirdness of Chrome's time. (BTW: DefaultTickClock is at least monotonic, and you can mock it for testing. Should you need it) https://codereview.chromium.org/2897563002/diff/60001/chrome/browser/translat... chrome/browser/translate/chrome_translate_client.cc:84: auto* const lang = lang_detection.add_detected_languages(); On 2017/05/26 00:26:45, renjieliu1 wrote: > On 2017/05/25 21:49:26, napper wrote: > > On 2017/05/25 at 21:35:29, groby wrote: > > > Wait - isn't this a mutable pointer to a constant lang? I.e. the > > set_language_code() calls should fail? > > > > > > You probably want either auto const*, or better auto* > > > > Note that the pointer is const (so lang++ won't work) but the object pointed > to > > by the pointer is non-const (so lang->set_language_code() will work). > > thanks for the clarification! :) Indeed. Thank you. I can't believe I *still* read them the wrong way around, even though I know I read them the wrong way around ;) https://codereview.chromium.org/2897563002/diff/60001/chrome/browser/translat... chrome/browser/translate/chrome_translate_client.cc:107: // If entry is null, we don't record the page. On 2017/05/26 00:26:45, renjieliu1 wrote: > On 2017/05/25 21:35:29, groby wrote: > > The logic is clear from the code - can you change the comment to explain _why_ > > we don't record the page on NullPtr? (Also, if we rely on NavigationEntry, > > should this be a NavigationObserver?) > > Added a comment. > > I wonder are you suggesting web_contents_observer: > https://cs.chromium.org/chromium/src/content/public/browser/web_contents_obse... > > Currently we're relying on CLD3 to determine the language and Register the > information in the > page(https://cs.chromium.org/chromium/src/components/translate/content/renderer/translate_helper.cc?q=translate_helper&l=132), > thus we can get the language info in > ChromeTranslateClient::OnLanguageDetermined. No, I meant the NavigationObserver :) It gets notified whenever new NavigationEntry is created, IIRC. But what you have seems to work fine. https://codereview.chromium.org/2897563002/diff/80001/chrome/browser/translat... File chrome/browser/translate/chrome_translate_client.cc (right): https://codereview.chromium.org/2897563002/diff/80001/chrome/browser/translat... chrome/browser/translate/chrome_translate_client.cc:108: // The navigation entry can be null in situations like download. Do we have a list of those situations? Sorry I'm so nit-picky about this - but the docs say GetLastCommittedEntry() can only be nullptr if there is no committed entry, and that makes no sense for OnLanguageDetermined. I don't think it's worth holding up the CL for, but if you have more info or a link to add here, much appreciated.
On 2017/05/26 01:28:48, groby wrote: > lgtm > > https://codereview.chromium.org/2897563002/diff/60001/chrome/browser/translat... > File chrome/browser/translate/chrome_translate_client.cc (right): > > https://codereview.chromium.org/2897563002/diff/60001/chrome/browser/translat... > chrome/browser/translate/chrome_translate_client.cc:81: > specifics->set_navigation_id(base::Time::Now().ToInternalValue()); > On 2017/05/26 00:26:45, renjieliu1 wrote: > > On 2017/05/25 21:35:29, groby wrote: > > > Note: Time::Now() is not guaranteed to be increasing (monotonically or > > > otherwise) - if that is an issue, you'll need to find another time base. > > > > currently the suggested way is to use time to identify the navigation, we > > probably need to change that in future > > I'm not sure what your requirements are, so pick what works for you :) Just > pointing out the weirdness of Chrome's time. (BTW: DefaultTickClock is at least > monotonic, and you can mock it for testing. Should you need it) > > https://codereview.chromium.org/2897563002/diff/60001/chrome/browser/translat... > chrome/browser/translate/chrome_translate_client.cc:84: auto* const lang = > lang_detection.add_detected_languages(); > On 2017/05/26 00:26:45, renjieliu1 wrote: > > On 2017/05/25 21:49:26, napper wrote: > > > On 2017/05/25 at 21:35:29, groby wrote: > > > > Wait - isn't this a mutable pointer to a constant lang? I.e. the > > > set_language_code() calls should fail? > > > > > > > > You probably want either auto const*, or better auto* > > > > > > Note that the pointer is const (so lang++ won't work) but the object pointed > > to > > > by the pointer is non-const (so lang->set_language_code() will work). > > > > thanks for the clarification! :) > > Indeed. Thank you. I can't believe I *still* read them the wrong way around, > even though I know I read them the wrong way around ;) > > https://codereview.chromium.org/2897563002/diff/60001/chrome/browser/translat... > chrome/browser/translate/chrome_translate_client.cc:107: // If entry is null, we > don't record the page. > On 2017/05/26 00:26:45, renjieliu1 wrote: > > On 2017/05/25 21:35:29, groby wrote: > > > The logic is clear from the code - can you change the comment to explain > _why_ > > > we don't record the page on NullPtr? (Also, if we rely on NavigationEntry, > > > should this be a NavigationObserver?) > > > > Added a comment. > > > > I wonder are you suggesting web_contents_observer: > > > https://cs.chromium.org/chromium/src/content/public/browser/web_contents_obse... > > > > Currently we're relying on CLD3 to determine the language and Register the > > information in the > > > page(https://cs.chromium.org/chromium/src/components/translate/content/renderer/translate_helper.cc?q=translate_helper&l=132), > > thus we can get the language info in > > ChromeTranslateClient::OnLanguageDetermined. > > No, I meant the NavigationObserver :) It gets notified whenever new > NavigationEntry is created, IIRC. But what you have seems to work fine. > > https://codereview.chromium.org/2897563002/diff/80001/chrome/browser/translat... > File chrome/browser/translate/chrome_translate_client.cc (right): > > https://codereview.chromium.org/2897563002/diff/80001/chrome/browser/translat... > chrome/browser/translate/chrome_translate_client.cc:108: // The navigation entry > can be null in situations like download. > Do we have a list of those situations? > > Sorry I'm so nit-picky about this - but the docs say GetLastCommittedEntry() can > only be nullptr if there is no committed entry, and that makes no sense for > OnLanguageDetermined. I don't think it's worth holding up the CL for, but if you > have more info or a link to add here, much appreciated. added another case which is initial blank page, https://cs.chromium.org/chromium/src/components/favicon/content/content_favic... thanks for the note, I will add more use case if I can find more! :)
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, skym@chromium.org, groby@chromium.org Link to the patchset: https://codereview.chromium.org/2897563002/#ps100001 (title: "update comment")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
The CQ bit was checked by renjieliu@chromium.org
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": 100001, "attempt_start_ts": 1495789138866980,
"parent_rev": "2751e50876e2591e8268c124001b58dabfab6828", "commit_rev":
"d8a3849678bf6750fa1a0a6cfbfeab9744070cc4"}
Message was sent while issue was closed.
Description was changed from ========== Implement client side logging for language detection. Integration steps described in: https://docs.google.com/document/d/1es1cCRFhdag4ljGSPfXfSMlEx5lMgFZ3SGHMaBcU_... BUG=722679 ========== to ========== Implement client side logging for language detection. Integration steps described in: https://docs.google.com/document/d/1es1cCRFhdag4ljGSPfXfSMlEx5lMgFZ3SGHMaBcU_... BUG=722679 Review-Url: https://codereview.chromium.org/2897563002 Cr-Commit-Position: refs/heads/master@{#474972} Committed: https://chromium.googlesource.com/chromium/src/+/d8a3849678bf6750fa1a0a6cfbfe... ==========
Message was sent while issue was closed.
Committed patchset #6 (id:100001) as https://chromium.googlesource.com/chromium/src/+/d8a3849678bf6750fa1a0a6cfbfe... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
