Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2968103002:
Adds logging to indicate infobar creation in the native library, (Closed)
|
Created:
3 years, 5 months ago by ramyasharma Modified:
3 years, 5 months ago CC:
asvitkine+watch_chromium.org, chromium-reviews Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdds logging to indicate infobar creation in the native library,
before the control goes to Android.
This is added to get a better idea about the metrics we are seeing.
BUG=720231
TBR=mdjones@chromium.org
Review-Url: https://codereview.chromium.org/2968103002
Cr-Commit-Position: refs/heads/master@{#484562}
Committed: https://chromium.googlesource.com/chromium/src/+/9042646a88a043a9918ffec4851320df1bda6fe2
Patch Set 1 #
Total comments: 6
Patch Set 2 #
Total comments: 2
Patch Set 3 #
Messages
Total messages: 29 (15 generated)
Patchset #1 (id:1) has been deleted
Description was changed from ========== Adds logging to indicate infobar creation in the native library, before the control goes to Android. This is added to get a better idea about the metrics we are seeing. BUG=720231 ========== to ========== Adds logging to indicate infobar creation in the native library, before the control goes to Android. This is added to get a better idea about the metrics we are seeing. BUG=720231 ==========
ramyasharma@chromium.org changed reviewers: + googleo@chromium.org, napper@chromium.org
https://codereview.chromium.org/2968103002/diff/20001/chrome/browser/ui/andro... File chrome/browser/ui/android/infobars/translate_infobar.cc (right): https://codereview.chromium.org/2968103002/diff/20001/chrome/browser/ui/andro... chrome/browser/ui/android/infobars/translate_infobar.cc:21: #include "components/translate/core/browser/translate_browser_metrics.h" sort https://codereview.chromium.org/2968103002/diff/20001/chrome/browser/ui/andro... chrome/browser/ui/android/infobars/translate_infobar.cc:36: } } else { https://codereview.chromium.org/2968103002/diff/20001/chrome/browser/ui/andro... chrome/browser/ui/android/infobars/translate_infobar.cc:37: else { Note that the style guide says don't use else after return
Thanks Jon. PTAL? https://codereview.chromium.org/2968103002/diff/20001/chrome/browser/ui/andro... File chrome/browser/ui/android/infobars/translate_infobar.cc (right): https://codereview.chromium.org/2968103002/diff/20001/chrome/browser/ui/andro... chrome/browser/ui/android/infobars/translate_infobar.cc:21: #include "components/translate/core/browser/translate_browser_metrics.h" On 2017/07/06 05:09:01, napper wrote: > sort Done. https://codereview.chromium.org/2968103002/diff/20001/chrome/browser/ui/andro... chrome/browser/ui/android/infobars/translate_infobar.cc:36: } On 2017/07/06 05:09:01, napper wrote: > } else { Done. https://codereview.chromium.org/2968103002/diff/20001/chrome/browser/ui/andro... chrome/browser/ui/android/infobars/translate_infobar.cc:37: else { On 2017/07/06 05:09:01, napper wrote: > Note that the style guide says don't use else after return Done.
Thanks Jon. PTAL? https://codereview.chromium.org/2968103002/diff/20001/chrome/browser/ui/andro... File chrome/browser/ui/android/infobars/translate_infobar.cc (right): https://codereview.chromium.org/2968103002/diff/20001/chrome/browser/ui/andro... chrome/browser/ui/android/infobars/translate_infobar.cc:21: #include "components/translate/core/browser/translate_browser_metrics.h" On 2017/07/06 05:09:01, napper wrote: > sort Done. https://codereview.chromium.org/2968103002/diff/20001/chrome/browser/ui/andro... chrome/browser/ui/android/infobars/translate_infobar.cc:36: } On 2017/07/06 05:09:01, napper wrote: > } else { Done. https://codereview.chromium.org/2968103002/diff/20001/chrome/browser/ui/andro... chrome/browser/ui/android/infobars/translate_infobar.cc:37: else { On 2017/07/06 05:09:01, napper wrote: > Note that the style guide says don't use else after return Done.
The CQ bit was checked by ramyasharma@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...
add some comment to make it more accurate. On the other side, how do we compare the new logging with front logging? I think the front logging of the Old UI contains initial step and auto translate step, but this will only contain initial step. Right? https://codereview.chromium.org/2968103002/diff/40001/chrome/browser/ui/andro... File chrome/browser/ui/android/infobars/translate_infobar.cc (right): https://codereview.chromium.org/2968103002/diff/40001/chrome/browser/ui/andro... chrome/browser/ui/android/infobars/translate_infobar.cc:33: translate::TranslateBrowserMetrics::ReportInitiationStatus( We need to add same condition (TRANSLATE_STEP_BEFORE_TRANSLATE) as blow which will make the data more accurate. For some corner cases, other translate steps will step in this too. E.g. some user starts translating then closes the inforbar soon before it completes. So the infobar will be created again when page is translated.
Patchset #3 (id:60001) has been deleted
Thanks Leo. PTAL https://codereview.chromium.org/2968103002/diff/40001/chrome/browser/ui/andro... File chrome/browser/ui/android/infobars/translate_infobar.cc (right): https://codereview.chromium.org/2968103002/diff/40001/chrome/browser/ui/andro... chrome/browser/ui/android/infobars/translate_infobar.cc:33: translate::TranslateBrowserMetrics::ReportInitiationStatus( On 2017/07/06 06:36:42, Leo wrote: > We need to add same condition (TRANSLATE_STEP_BEFORE_TRANSLATE) as blow which > will make the data more accurate. For some corner cases, other translate steps > will step in this too. E.g. some user starts translating then closes the > inforbar soon before it completes. So the infobar will be created again when > page is translated. Thanks Leo. Good point. I have moved the logging above the if/else block as it is common to both. And yes, we are only logging brand new infobars, and not auto-config infobars. which is different from Android UI logging.
lgtm
napper@chromium.org changed reviewers: + dvallet@chromium.org
lgtm
lgtm
The CQ bit was checked by ramyasharma@chromium.org
The CQ bit was unchecked by ramyasharma@chromium.org
The CQ bit was checked by ramyasharma@chromium.org
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...)
Description was changed from ========== Adds logging to indicate infobar creation in the native library, before the control goes to Android. This is added to get a better idea about the metrics we are seeing. BUG=720231 ========== to ========== Adds logging to indicate infobar creation in the native library, before the control goes to Android. This is added to get a better idea about the metrics we are seeing. BUG=720231 TBR=mdjones@chromium.org ==========
The CQ bit was checked by ramyasharma@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": 80001, "attempt_start_ts": 1499343186024860,
"parent_rev": "f04c35054e7297a36f6fdaf7f855d02f3621574f", "commit_rev":
"9042646a88a043a9918ffec4851320df1bda6fe2"}
Message was sent while issue was closed.
Description was changed from ========== Adds logging to indicate infobar creation in the native library, before the control goes to Android. This is added to get a better idea about the metrics we are seeing. BUG=720231 TBR=mdjones@chromium.org ========== to ========== Adds logging to indicate infobar creation in the native library, before the control goes to Android. This is added to get a better idea about the metrics we are seeing. BUG=720231 TBR=mdjones@chromium.org Review-Url: https://codereview.chromium.org/2968103002 Cr-Commit-Position: refs/heads/master@{#484562} Committed: https://chromium.googlesource.com/chromium/src/+/9042646a88a043a9918ffec48513... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:80001) as https://chromium.googlesource.com/chromium/src/+/9042646a88a043a9918ffec48513... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
