|
|
Created:
3 years, 6 months ago by renjieliu1 Modified:
3 years, 6 months ago CC:
chromium-reviews Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionQuick fix for logging.
Currently two issues:
1) We shouldn't log event for chrome://*
2) The navigation id should comes from NavigationEntry.
BUG=722679
Review-Url: https://codereview.chromium.org/2916873004
Cr-Commit-Position: refs/heads/master@{#479263}
Committed: https://chromium.googlesource.com/chromium/src/+/e2fb02881eb16603ce55e9dbfdf73e1ae4daade3
Patch Set 1 #Patch Set 2 : just use IsTranslatebleURL. #
Total comments: 2
Patch Set 3 : just use IsTranslatebleURL. #
Total comments: 6
Patch Set 4 : add unit test #
Total comments: 2
Patch Set 5 : rebase #
Total comments: 2
Patch Set 6 : updates #
Messages
Total messages: 40 (18 generated)
Description was changed from ========== Quick fix for logging. Currently two issues: 1) We shouldn't log event for chrome://* 2) The navigation id should comes from NavigationEntry. BUG=722679 ========== to ========== Quick fix for logging. Currently two issues: 1) We shouldn't log event for chrome://* 2) The navigation id should comes from NavigationEntry. BUG=722679 ==========
renjieliu@chromium.org changed reviewers: + napper@chromium.org
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...
change the check logic to check whether it is translatable url.PTAL
lgtm https://codereview.chromium.org/2916873004/diff/20001/components/translate/co... File components/translate/core/common/language_detection_logging_helper.h (right): https://codereview.chromium.org/2916873004/diff/20001/components/translate/co... components/translate/core/common/language_detection_logging_helper.h:21: const int navigation_id, No benefit having the const here.
thanks for the review! https://codereview.chromium.org/2916873004/diff/20001/components/translate/co... File components/translate/core/common/language_detection_logging_helper.h (right): https://codereview.chromium.org/2916873004/diff/20001/components/translate/co... components/translate/core/common/language_detection_logging_helper.h:21: const int navigation_id, On 2017/06/02 01:26:47, napper wrote: > No benefit having the const here. Done.
renjieliu@chromium.org changed reviewers: + skym@chromium.org
Hi Sky, Thank you for pointing out the issues! PTAL
https://codereview.chromium.org/2916873004/diff/40001/chrome/browser/translat... File chrome/browser/translate/chrome_translate_client.cc (right): https://codereview.chromium.org/2916873004/diff/40001/chrome/browser/translat... chrome/browser/translate/chrome_translate_client.cc:90: TranslateService::IsTranslatableURL(entry->GetVirtualURL())) { Would be nice to have a test case get filtered by this. https://codereview.chromium.org/2916873004/diff/40001/components/translate/co... File components/translate/core/common/language_detection_logging_helper.h (right): https://codereview.chromium.org/2916873004/diff/40001/components/translate/co... components/translate/core/common/language_detection_logging_helper.h:21: int navigation_id, I think int is 4 bytes, while base::Time::ToInternalValue() is int64_t, which is definitely 8 bytes. I'm pretty sure this is going to mangle the number and bad things are going to happen. auto now = base::Time::Now().ToInternalValue(); LOG(ERROR) << "SKYM now: " << now; int int_now = now; LOG(ERROR) << "SKYM int_now: " << int_now; > SKYM now: 13140911818057375 > SKYM int_now: -185555297 https://codereview.chromium.org/2916873004/diff/40001/components/translate/co... File components/translate/core/common/language_detection_logging_helper_unittest.cc (right): https://codereview.chromium.org/2916873004/diff/40001/components/translate/co... components/translate/core/common/language_detection_logging_helper_unittest.cc:28: ConstructLanguageDetectionEvent(100, details); Probably should have a test case that uses a number that's too big to hold in 4 bytes.
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/2916873004/diff/40001/chrome/browser/translat... File chrome/browser/translate/chrome_translate_client.cc (right): https://codereview.chromium.org/2916873004/diff/40001/chrome/browser/translat... chrome/browser/translate/chrome_translate_client.cc:90: TranslateService::IsTranslatableURL(entry->GetVirtualURL())) { On 2017/06/02 21:22:40, skym wrote: > Would be nice to have a test case get filtered by this. Done. https://codereview.chromium.org/2916873004/diff/40001/components/translate/co... File components/translate/core/common/language_detection_logging_helper.h (right): https://codereview.chromium.org/2916873004/diff/40001/components/translate/co... components/translate/core/common/language_detection_logging_helper.h:21: int navigation_id, On 2017/06/02 21:22:40, skym wrote: > I think int is 4 bytes, while base::Time::ToInternalValue() is int64_t, which is > definitely 8 bytes. I'm pretty sure this is going to mangle the number and bad > things are going to happen. > > auto now = base::Time::Now().ToInternalValue(); > LOG(ERROR) << "SKYM now: " << now; > int int_now = now; > LOG(ERROR) << "SKYM int_now: " << int_now; > > > SKYM now: 13140911818057375 > > SKYM int_now: -185555297 thank you for the catch! https://codereview.chromium.org/2916873004/diff/40001/components/translate/co... File components/translate/core/common/language_detection_logging_helper_unittest.cc (right): https://codereview.chromium.org/2916873004/diff/40001/components/translate/co... components/translate/core/common/language_detection_logging_helper_unittest.cc:28: ConstructLanguageDetectionEvent(100, details); On 2017/06/02 21:22:40, skym wrote: > Probably should have a test case that uses a number that's too big to hold in 4 > bytes. Done.
thanks for the review!
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm https://codereview.chromium.org/2916873004/diff/60001/chrome/browser/translat... File chrome/browser/translate/chrome_translate_client_unittest.cc (right): https://codereview.chromium.org/2916873004/diff/60001/chrome/browser/translat... chrome/browser/translate/chrome_translate_client_unittest.cc:68: EXPECT_EQ(0ul, GetUserEventService()->GetRecordedUserEvents().size()); 0ul seems a bit odd here, 0u would work just fine, right?
https://codereview.chromium.org/2916873004/diff/60001/chrome/browser/translat... File chrome/browser/translate/chrome_translate_client_unittest.cc (right): https://codereview.chromium.org/2916873004/diff/60001/chrome/browser/translat... chrome/browser/translate/chrome_translate_client_unittest.cc:68: EXPECT_EQ(0ul, GetUserEventService()->GetRecordedUserEvents().size()); On 2017/06/06 15:46:56, skym wrote: > 0ul seems a bit odd here, 0u would work just fine, right? Done.
renjieliu@chromium.org changed reviewers: + groby@chromium.org
Why should we not log the language for chrome:// pages? If we care specifically about the languages for translatable pages, would it be better to call LogLanguage from the end of InitiateTranslation, instead? (Which has a couple extra checks for untranslatable pages) https://codereview.chromium.org/2916873004/diff/80001/chrome/browser/translat... File chrome/browser/translate/chrome_translate_client.cc (right): https://codereview.chromium.org/2916873004/diff/80001/chrome/browser/translat... chrome/browser/translate/chrome_translate_client.cc:89: DCHECK(web_contents) << "null contents"; Probably doesn't need the "null contents" text - DCHECK(web_contents) is pretty idiomatic check for null contents.
On 2017/06/07 at 16:37:50, groby wrote: > Why should we not log the language for chrome:// pages? > > If we care specifically about the languages for translatable pages, would it be better to call LogLanguage from the end of InitiateTranslation, instead? (Which has a couple extra checks for untranslatable pages) chrome:// etc pages will be in the users UI language. The logging is specifically for content pages that the user has navigated to. For example, if a user in India has their UI in english but always browses content in Hindi, we don't want a bunch of English language logging events when they visit history. etc. > > https://codereview.chromium.org/2916873004/diff/80001/chrome/browser/translat... > File chrome/browser/translate/chrome_translate_client.cc (right): > > https://codereview.chromium.org/2916873004/diff/80001/chrome/browser/translat... > chrome/browser/translate/chrome_translate_client.cc:89: DCHECK(web_contents) << "null contents"; > Probably doesn't need the "null contents" text - DCHECK(web_contents) is pretty idiomatic check for null contents.
Isn't InitiateTranslation the better place, then? It will always be the authoritative gate for all pages that are actually eligible for translation at all. On Wed, Jun 7, 2017 at 9:55 AM, <napper@chromium.org> wrote: > On 2017/06/07 at 16:37:50, groby wrote: > > Why should we not log the language for chrome:// pages? > > > > If we care specifically about the languages for translatable pages, > would it > be better to call LogLanguage from the end of InitiateTranslation, instead? > (Which has a couple extra checks for untranslatable pages) > > chrome:// etc pages will be in the users UI language. The logging is > specifically for content pages that the user has navigated to. For > example, if a > user in India has their UI in english but always browses content in Hindi, > we > don't want a bunch of English language logging events when they visit > history. > etc. > > > > > > https://codereview.chromium.org/2916873004/diff/80001/ > chrome/browser/translate/chrome_translate_client.cc > > File chrome/browser/translate/chrome_translate_client.cc (right): > > > > > https://codereview.chromium.org/2916873004/diff/80001/ > chrome/browser/translate/chrome_translate_client.cc#newcode89 > > chrome/browser/translate/chrome_translate_client.cc:89: > DCHECK(web_contents) > << "null contents"; > > Probably doesn't need the "null contents" text - DCHECK(web_contents) is > pretty idiomatic check for null contents. > > > > https://codereview.chromium.org/2916873004/ > -- 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.
On 2017/06/07 at 18:14:10, groby wrote: > Isn't InitiateTranslation the better place, then? It will always be the > authoritative gate for all pages that are actually eligible for translation > at all. > > On Wed, Jun 7, 2017 at 9:55 AM, <napper@chromium.org> wrote: > > > On 2017/06/07 at 16:37:50, groby wrote: > > > Why should we not log the language for chrome:// pages? > > > > > > If we care specifically about the languages for translatable pages, > > would it > > be better to call LogLanguage from the end of InitiateTranslation, instead? > > (Which has a couple extra checks for untranslatable pages) > > > > chrome:// etc pages will be in the users UI language. The logging is > > specifically for content pages that the user has navigated to. For > > example, if a > > user in India has their UI in english but always browses content in Hindi, > > we > > don't want a bunch of English language logging events when they visit > > history. > > etc. > > > > > > > > > > https://codereview.chromium.org/2916873004/diff/80001/ > > chrome/browser/translate/chrome_translate_client.cc > > > File chrome/browser/translate/chrome_translate_client.cc (right): > > > > > > > > https://codereview.chromium.org/2916873004/diff/80001/ > > chrome/browser/translate/chrome_translate_client.cc#newcode89 > > > chrome/browser/translate/chrome_translate_client.cc:89: > > DCHECK(web_contents) > > << "null contents"; > > > Probably doesn't need the "null contents" text - DCHECK(web_contents) is > > pretty idiomatic check for null contents. > > > > > > > > https://codereview.chromium.org/2916873004/ > > > > -- > 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. > InitiateTranslation() is in core, so can't call code in Chrome.
That's the whole point of TranslateClient? On Wed, Jun 7, 2017 at 12:34 PM, <napper@chromium.org> wrote: > On 2017/06/07 at 18:14:10, groby wrote: > > Isn't InitiateTranslation the better place, then? It will always be the > > authoritative gate for all pages that are actually eligible for > translation > > at all. > > > > On Wed, Jun 7, 2017 at 9:55 AM, <napper@chromium.org> wrote: > > > > > On 2017/06/07 at 16:37:50, groby wrote: > > > > Why should we not log the language for chrome:// pages? > > > > > > > > If we care specifically about the languages for translatable pages, > > > would it > > > be better to call LogLanguage from the end of InitiateTranslation, > instead? > > > (Which has a couple extra checks for untranslatable pages) > > > > > > chrome:// etc pages will be in the users UI language. The logging is > > > specifically for content pages that the user has navigated to. For > > > example, if a > > > user in India has their UI in english but always browses content in > Hindi, > > > we > > > don't want a bunch of English language logging events when they visit > > > history. > > > etc. > > > > > > > > > > > > > > https://codereview.chromium.org/2916873004/diff/80001/ > > > chrome/browser/translate/chrome_translate_client.cc > > > > File chrome/browser/translate/chrome_translate_client.cc (right): > > > > > > > > > > > https://codereview.chromium.org/2916873004/diff/80001/ > > > chrome/browser/translate/chrome_translate_client.cc#newcode89 > > > > chrome/browser/translate/chrome_translate_client.cc:89: > > > DCHECK(web_contents) > > > << "null contents"; > > > > Probably doesn't need the "null contents" text - > DCHECK(web_contents) is > > > pretty idiomatic check for null contents. > > > > > > > > > > > > https://codereview.chromium.org/2916873004/ > > > > > > > -- > > 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. > > > > InitiateTranslation() is in core, so can't call code in Chrome. > > https://codereview.chromium.org/2916873004/ > -- 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.
On 2017/06/07 at 22:21:46, groby wrote: > That's the whole point of TranslateClient? > > On Wed, Jun 7, 2017 at 12:34 PM, <napper@chromium.org> wrote: > > > On 2017/06/07 at 18:14:10, groby wrote: > > > Isn't InitiateTranslation the better place, then? It will always be the > > > authoritative gate for all pages that are actually eligible for > > translation > > > at all. > > > > > > On Wed, Jun 7, 2017 at 9:55 AM, <napper@chromium.org> wrote: > > > > > > > On 2017/06/07 at 16:37:50, groby wrote: > > > > > Why should we not log the language for chrome:// pages? > > > > > > > > > > If we care specifically about the languages for translatable pages, > > > > would it > > > > be better to call LogLanguage from the end of InitiateTranslation, > > instead? > > > > (Which has a couple extra checks for untranslatable pages) > > > > > > > > chrome:// etc pages will be in the users UI language. The logging is > > > > specifically for content pages that the user has navigated to. For > > > > example, if a > > > > user in India has their UI in english but always browses content in > > Hindi, > > > > we > > > > don't want a bunch of English language logging events when they visit > > > > history. > > > > etc. > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/2916873004/diff/80001/ > > > > chrome/browser/translate/chrome_translate_client.cc > > > > > File chrome/browser/translate/chrome_translate_client.cc (right): > > > > > > > > > > > > > > https://codereview.chromium.org/2916873004/diff/80001/ > > > > chrome/browser/translate/chrome_translate_client.cc#newcode89 > > > > > chrome/browser/translate/chrome_translate_client.cc:89: > > > > DCHECK(web_contents) > > > > << "null contents"; > > > > > Probably doesn't need the "null contents" text - > > DCHECK(web_contents) is > > > > pretty idiomatic check for null contents. > > > > > > > > > > > > > > > > https://codereview.chromium.org/2916873004/ > > > > > > > > > > -- > > > 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. > > > > > > > InitiateTranslation() is in core, so can't call code in Chrome. > > > > https://codereview.chromium.org/2916873004/ > > > > -- > 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. > Note that it is not "pages that are eligible for translation" that we are interested in, rather the detected language of the pages the user chooses to browse, so we can get an understanding of what languages the user chooses to consume content in. If we log in InitiateTranslation() we will lose for example pages that are marked as "do not translate" but otherwise have a valid detected language.
On 2017/06/07 23:47:58, napper wrote: > On 2017/06/07 at 22:21:46, groby wrote: > > That's the whole point of TranslateClient? > > > > On Wed, Jun 7, 2017 at 12:34 PM, <mailto:napper@chromium.org> wrote: > > > > > On 2017/06/07 at 18:14:10, groby wrote: > > > > Isn't InitiateTranslation the better place, then? It will always be the > > > > authoritative gate for all pages that are actually eligible for > > > translation > > > > at all. > > > > > > > > On Wed, Jun 7, 2017 at 9:55 AM, <mailto:napper@chromium.org> wrote: > > > > > > > > > On 2017/06/07 at 16:37:50, groby wrote: > > > > > > Why should we not log the language for chrome:// pages? > > > > > > > > > > > > If we care specifically about the languages for translatable pages, > > > > > would it > > > > > be better to call LogLanguage from the end of InitiateTranslation, > > > instead? > > > > > (Which has a couple extra checks for untranslatable pages) > > > > > > > > > > chrome:// etc pages will be in the users UI language. The logging is > > > > > specifically for content pages that the user has navigated to. For > > > > > example, if a > > > > > user in India has their UI in english but always browses content in > > > Hindi, > > > > > we > > > > > don't want a bunch of English language logging events when they visit > > > > > history. > > > > > etc. > > > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/2916873004/diff/80001/ > > > > > chrome/browser/translate/chrome_translate_client.cc > > > > > > File chrome/browser/translate/chrome_translate_client.cc (right): > > > > > > > > > > > > > > > > > https://codereview.chromium.org/2916873004/diff/80001/ > > > > > chrome/browser/translate/chrome_translate_client.cc#newcode89 > > > > > > chrome/browser/translate/chrome_translate_client.cc:89: > > > > > DCHECK(web_contents) > > > > > << "null contents"; > > > > > > Probably doesn't need the "null contents" text - > > > DCHECK(web_contents) is > > > > > pretty idiomatic check for null contents. > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/2916873004/ > > > > > > > > > > > > > -- > > > > 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 mailto:chromium-reviews+unsubscribe@chromium.org. > > > > > > > > > > InitiateTranslation() is in core, so can't call code in Chrome. > > > > > > https://codereview.chromium.org/2916873004/ > > > > > > > -- > > 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 mailto:chromium-reviews+unsubscribe@chromium.org. > > > > Note that it is not "pages that are eligible for translation" that we are > interested in, rather the detected language of the pages the user chooses to > browse, so we can get an understanding of what languages the user chooses to > consume content in. If we log in InitiateTranslation() we will lose for example > pages that are marked as "do not translate" but otherwise have a valid detected > language. thanks for the clarification! we're interested in the detected language of the webpage every time the language is determined (OnLanguageDetermined), however, logging language detection events for "about:blank" and "chrome:*" does not help us understand user language but only costs more bandwidth and storage space.
https://codereview.chromium.org/2916873004/diff/80001/chrome/browser/translat... File chrome/browser/translate/chrome_translate_client.cc (right): https://codereview.chromium.org/2916873004/diff/80001/chrome/browser/translat... chrome/browser/translate/chrome_translate_client.cc:89: DCHECK(web_contents) << "null contents"; On 2017/06/07 16:37:50, groby wrote: > Probably doesn't need the "null contents" text - DCHECK(web_contents) is pretty > idiomatic check for null contents. Done.
lgtm
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/2916873004/#ps100001 (title: "updates")
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": 1497408239661110, "parent_rev": "359489104921f8f02e67c180832a9f273ddb6b7b", "commit_rev": "e2fb02881eb16603ce55e9dbfdf73e1ae4daade3"}
Message was sent while issue was closed.
Description was changed from ========== Quick fix for logging. Currently two issues: 1) We shouldn't log event for chrome://* 2) The navigation id should comes from NavigationEntry. BUG=722679 ========== to ========== Quick fix for logging. Currently two issues: 1) We shouldn't log event for chrome://* 2) The navigation id should comes from NavigationEntry. BUG=722679 Review-Url: https://codereview.chromium.org/2916873004 Cr-Commit-Position: refs/heads/master@{#479263} Committed: https://chromium.googlesource.com/chromium/src/+/e2fb02881eb16603ce55e9dbfdf7... ==========
Message was sent while issue was closed.
Committed patchset #6 (id:100001) as https://chromium.googlesource.com/chromium/src/+/e2fb02881eb16603ce55e9dbfdf7... |