Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2459923002:
[Offline pages] Adding metrics for OIB's Open online button (Closed)
Description[Offline pages] Adding metrics for OIB's Open online button
Adds metrics for the following aspects:
* Open online button is shown when OIB is shown for offline page
* Browser is online when open online button is clicked in OIB
BUG=656804
Committed: https://crrev.com/78247f3f61fd14570b57e6ac4415f79bcda48e9c
Cr-Commit-Position: refs/heads/master@{#430447}
Patch Set 1 #Patch Set 2 : Rebasing #
Total comments: 4
Patch Set 3 : Addressing feedback #
Total comments: 4
Patch Set 4 : Addressing final comments - updating histogram documentation #
Messages
Total messages: 23 (14 generated)
The CQ bit was checked by fgorski@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: mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by fgorski@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...
fgorski@chromium.org changed reviewers: + asvitkine@chromium.org, tedchoc@chromium.org
PTAL tedchoc: chrome/android/java/src/org/chromium/chrome/browser/pageinfo/WebsiteSettingsPopup.java asvitkine: tools/metrics/histograms/histograms.xml
lgtm
https://codereview.chromium.org/2459923002/diff/20001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2459923002/diff/20001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:39719: +<histogram name="OfflinePages.OIB.OnlineWhenOpenOnlineButtonClicked" Nit: I'm not sure most Chrome devs would know what OIB is. I would spell it out. https://codereview.chromium.org/2459923002/diff/20001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:39720: + enum="Boolean"> Please use a more specific Boolean - add it. Same for the one below.
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 fgorski@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...
Alexei, PTAL https://codereview.chromium.org/2459923002/diff/20001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2459923002/diff/20001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:39719: +<histogram name="OfflinePages.OIB.OnlineWhenOpenOnlineButtonClicked" On 2016/11/07 18:34:51, Alexei Svitkine (very slow) wrote: > Nit: I'm not sure most Chrome devs would know what OIB is. I would spell it out. Done. https://codereview.chromium.org/2459923002/diff/20001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:39720: + enum="Boolean"> On 2016/11/07 18:34:51, Alexei Svitkine (very slow) wrote: > Please use a more specific Boolean - add it. Same for the one below. Ok. I am trying to reuse more specific existing enums. Let me know if that works. If not, I'll gladly go further, but please explain the rationale for defining more specific types in a few sentences.
lgtm % comments Generally, the advantage of using descriptive enums is that it's much easier to understand the metric when viewing on the dashboard - rather than try to reason what true/false means. The new ones you chose seem fine to me as long as you make the descriptions use the same terminology. https://codereview.chromium.org/2459923002/diff/40001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2459923002/diff/40001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:39918: + Indicates whether the browser was online when Open online button was clicked I'm ok if you want to use the Connected terminology - which it seems you're using both in the name and enum here. But if so, please update the description to use the same terminology. https://codereview.chromium.org/2459923002/diff/40001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:39928: + Indicates whether Open online was shown in Website Settings popup, when it Ditto: "was visible"?
Thanks for review. All addressed. https://codereview.chromium.org/2459923002/diff/40001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2459923002/diff/40001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:39918: + Indicates whether the browser was online when Open online button was clicked On 2016/11/07 23:40:00, Alexei Svitkine (very slow) wrote: > I'm ok if you want to use the Connected terminology - which it seems you're > using both in the name and enum here. But if so, please update the description > to use the same terminology. Done. https://codereview.chromium.org/2459923002/diff/40001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:39928: + Indicates whether Open online was shown in Website Settings popup, when it On 2016/11/07 23:40:00, Alexei Svitkine (very slow) wrote: > Ditto: "was visible"? Done.
The CQ bit was checked by fgorski@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from tedchoc@chromium.org, asvitkine@chromium.org Link to the patchset: https://codereview.chromium.org/2459923002/#ps60001 (title: "Addressing final comments - updating histogram documentation")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Description was changed from ========== [Offline pages] Adding metrics for OIB's Open online button Adds metrics for the following aspects: * Open online button is shown when OIB is shown for offline page * Browser is online when open online button is clicked in OIB BUG=656804 ========== to ========== [Offline pages] Adding metrics for OIB's Open online button Adds metrics for the following aspects: * Open online button is shown when OIB is shown for offline page * Browser is online when open online button is clicked in OIB BUG=656804 Committed: https://crrev.com/78247f3f61fd14570b57e6ac4415f79bcda48e9c Cr-Commit-Position: refs/heads/master@{#430447} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/78247f3f61fd14570b57e6ac4415f79bcda48e9c Cr-Commit-Position: refs/heads/master@{#430447} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
