Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2434083002:
Expand WebsiteSettings histograms for HTTP-bad (Closed)
DescriptionExpand WebsiteSettings histograms for HTTP-bad
This CL expands WebsiteSettings histograms so that we can track actions
broken down by each of the following omnibox states:
- valid HTTPS url
- HTTPS URL that has been downgraded because of e.g. mixed content
- HTTPS URL that is marked dangerous (e.g. broken HTTPS or malware)
- HTTP URL that is marked dangerous (e.g. malware)
- HTTP URL that has a "Not secure" warning in the omnibox
BUG=647566
Committed: https://crrev.com/8d67cd7a4a53be683945a0954aa4d4b3f4130302
Cr-Commit-Position: refs/heads/master@{#427017}
Patch Set 1 #Patch Set 2 : fix test description #
Total comments: 4
Patch Set 3 : WebsiteSettings -> PageInfo for new histograms #Patch Set 4 : fix histogram note #
Total comments: 4
Patch Set 5 : lgarron comments #Patch Set 6 : rebase #
Total comments: 10
Patch Set 7 : isherman comments and related cleanup #
Total comments: 2
Patch Set 8 : raymes comment #Patch Set 9 : rebase #
Messages
Total messages: 39 (23 generated)
The CQ bit was checked by estark@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...
estark@chromium.org changed reviewers: + lgarron@chromium.org
lgarron could you please review?
LGTM but a major request: Could you name the new histograms PageInfo instead of WebsiteSettings? This class itself is not renamed yet, but I'm making progress on consolidating down to the "Page Info" name. Since it's a clean break, it would be nice to align these new histograms with that. https://codereview.chromium.org/2434083002/diff/20001/chrome/browser/ui/websi... File chrome/browser/ui/website_settings/website_settings.cc (right): https://codereview.chromium.org/2434083002/diff/20001/chrome/browser/ui/websi... chrome/browser/ui/website_settings/website_settings.cc:425: security_level_ = security_info.security_level; I wanted to avoid introducing this before we get rid of connection tab/Android cruft, but I guess I don't see anything better to do. https://codereview.chromium.org/2434083002/diff/20001/chrome/browser/ui/websi... File chrome/browser/ui/website_settings/website_settings.h (right): https://codereview.chromium.org/2434083002/diff/20001/chrome/browser/ui/websi... chrome/browser/ui/website_settings/website_settings.h:239: security_state::SecurityStateModel::SecurityLevel security_level_ = I don't know our C+ conventions perfectly, but shouldn't this get initialized with al the other instance variables in the constructor?
The CQ bit was checked by estark@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 estark@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 estark@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...
Renamed new histograms WebsiteSettings => PageInfo https://codereview.chromium.org/2434083002/diff/20001/chrome/browser/ui/websi... File chrome/browser/ui/website_settings/website_settings.h (right): https://codereview.chromium.org/2434083002/diff/20001/chrome/browser/ui/websi... chrome/browser/ui/website_settings/website_settings.h:239: security_state::SecurityStateModel::SecurityLevel security_level_ = On 2016/10/21 01:04:57, lgarron wrote: > I don't know our C+ conventions perfectly, but shouldn't this get initialized > with al the other instance variables in the constructor? In-class member initializers are allowed by the style guide, see https://chromium-cpp.appspot.com/ and https://groups.google.com/a/chromium.org/d/msg/chromium-dev/zqB-DySA4V0/eS9wV... An OWNER might come along and tell me to be consistent with the rest of the file though, so we shall see.
estark@chromium.org changed reviewers: + isherman@chromium.org, raymes@chromium.org
raymes, can you please review chrome/browser/ui/website_settings? isherman, can you please review histograms.xml?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...) 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_...)
On 2016/10/21 at 01:17:48, estark wrote: > raymes, can you please review chrome/browser/ui/website_settings? > > isherman, can you please review histograms.xml? Thanks! Still LGTM.
["Publish" to trigger comments, since replying from the bottom of Rietveld didn't do so.) https://codereview.chromium.org/2434083002/diff/20001/chrome/browser/ui/websi... File chrome/browser/ui/website_settings/website_settings.h (right): https://codereview.chromium.org/2434083002/diff/20001/chrome/browser/ui/websi... chrome/browser/ui/website_settings/website_settings.h:239: security_state::SecurityStateModel::SecurityLevel security_level_ = On 2016/10/21 at 01:16:47, estark wrote: > On 2016/10/21 01:04:57, lgarron wrote: > > I don't know our C+ conventions perfectly, but shouldn't this get initialized > > with al the other instance variables in the constructor? > > In-class member initializers are allowed by the style guide, see https://chromium-cpp.appspot.com/ and https://groups.google.com/a/chromium.org/d/msg/chromium-dev/zqB-DySA4V0/eS9wV... > > An OWNER might come along and tell me to be consistent with the rest of the file though, so we shall see. Alright, sounds fine to me. Thanks for the link! https://codereview.chromium.org/2434083002/diff/60001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2434083002/diff/60001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:40298: + Tracks WebsiteSettings actions that take place on an HTTP URL that does not Nit: "Tracks Page Info bubble actions..." https://codereview.chromium.org/2434083002/diff/60001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:40299: + have any warnings associated with it. Nit: "an omnibox security indicator warning"? (To make it clear that e.g. Javascript console warnings aren't covered.)
The CQ bit was checked by estark@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...
https://codereview.chromium.org/2434083002/diff/60001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2434083002/diff/60001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:40298: + Tracks WebsiteSettings actions that take place on an HTTP URL that does not On 2016/10/21 01:22:50, lgarron wrote: > Nit: "Tracks Page Info bubble actions..." Done. https://codereview.chromium.org/2434083002/diff/60001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:40299: + have any warnings associated with it. On 2016/10/21 01:22:50, lgarron wrote: > Nit: "an omnibox security indicator warning"? > > (To make it clear that e.g. Javascript console warnings aren't covered.) Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2434083002/diff/100001/tools/metrics/histogra... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2434083002/diff/100001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:40480: +</histogram> I don't see this histogram being recorded in the C++ code. Am I just missing it? https://codereview.chromium.org/2434083002/diff/100001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:40486: + Tracks Page Info bubbleactions that take place on a valid HTTPS URL with no nit: s/bubbleactions/bubble actions (throughout) https://codereview.chromium.org/2434083002/diff/100001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:40491: +<histogram name="PageInfo.Action.HttpsUrlDangerous" It looks like you're adding a new top-level category, "PageInfo". Would it be reasonable to have these histograms under the WebSettings top-level category -- i.e. "WebSettings.PageInfo.*" -- or to add a new "Security" top-level category? (I'm honestly quite surprised that there isn't a "Security" top-level category already... https://codereview.chromium.org/2434083002/diff/100001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:40507: + a security issue (e.g. no mixed content). nit: Should this say "e.g. mixed content" rather than "e.g. no mixed content"? https://codereview.chromium.org/2434083002/diff/100001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:40520: +<histogram name="PageInfo.Action.HttpWarning" enum="WebsiteSettingsAction"> nit: I'd recommend using an extra dot after "Http" to group the HTTP histograms together (and likewise for the HTTPS histograms).
The CQ bit was checked by estark@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, Ilya. https://codereview.chromium.org/2434083002/diff/100001/tools/metrics/histogra... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2434083002/diff/100001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:40480: +</histogram> On 2016/10/21 16:27:09, Ilya Sherman wrote: > I don't see this histogram being recorded in the C++ code. Am I just missing > it? Uh oh! Thanks for catching that. Done. https://codereview.chromium.org/2434083002/diff/100001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:40486: + Tracks Page Info bubbleactions that take place on a valid HTTPS URL with no On 2016/10/21 16:27:09, Ilya Sherman wrote: > nit: s/bubbleactions/bubble actions (throughout) Done. https://codereview.chromium.org/2434083002/diff/100001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:40491: +<histogram name="PageInfo.Action.HttpsUrlDangerous" On 2016/10/21 16:27:09, Ilya Sherman wrote: > It looks like you're adding a new top-level category, "PageInfo". Would it be > reasonable to have these histograms under the WebSettings top-level category -- > i.e. "WebSettings.PageInfo.*" -- or to add a new "Security" top-level category? > (I'm honestly quite surprised that there isn't a "Security" top-level category > already... I had originally put these under WebsiteSettings, but it turns out that lgarron is on an in-progress mission to rename all WebsiteSettings things to PageInfo (see https://codereview.chromium.org/2434083002/#msg5). Since all the existing WebsiteSettings histograms are security- or privacy-related, I think Security.PageInfo.* would make sense; I'll rename it to that. https://codereview.chromium.org/2434083002/diff/100001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:40507: + a security issue (e.g. no mixed content). On 2016/10/21 16:27:09, Ilya Sherman wrote: > nit: Should this say "e.g. mixed content" rather than "e.g. no mixed content"? Done. https://codereview.chromium.org/2434083002/diff/100001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:40520: +<histogram name="PageInfo.Action.HttpWarning" enum="WebsiteSettingsAction"> On 2016/10/21 16:27:09, Ilya Sherman wrote: > nit: I'd recommend using an extra dot after "Http" to group the HTTP histograms > together (and likewise for the HTTPS histograms). Done (with some naming tweaks to make them more consistent).
Metrics lgtm, thanks!
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Friendly ping to raymes
lgtm https://codereview.chromium.org/2434083002/diff/120001/chrome/browser/ui/webs... File chrome/browser/ui/website_settings/website_settings.h (right): https://codereview.chromium.org/2434083002/diff/120001/chrome/browser/ui/webs... chrome/browser/ui/website_settings/website_settings.h:240: security_state::SecurityStateModel::NONE; Oh, I just asked a reviewer to use the initializer list in https://codereview.chromium.org/2440183002/diff/1/chrome/browser/ui/website_s... It would be nice if we had a style guide decision on this. I tend like having all the initializations in one place rather than having to examine 2 locations in the code, which I tend to see as more bug prone, but I know there are arguments for and against.
https://codereview.chromium.org/2434083002/diff/120001/chrome/browser/ui/webs... File chrome/browser/ui/website_settings/website_settings.h (right): https://codereview.chromium.org/2434083002/diff/120001/chrome/browser/ui/webs... chrome/browser/ui/website_settings/website_settings.h:240: security_state::SecurityStateModel::NONE; On 2016/10/24 02:58:50, raymes wrote: > Oh, I just asked a reviewer to use the initializer list in > https://codereview.chromium.org/2440183002/diff/1/chrome/browser/ui/website_s... > > It would be nice if we had a style guide decision on this. I tend like having > all the initializations in one place rather than having to examine 2 locations > in the code, which I tend to see as more bug prone, but I know there are > arguments for and against. Done.
The CQ bit was checked by estark@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from lgarron@chromium.org, isherman@chromium.org, raymes@chromium.org Link to the patchset: https://codereview.chromium.org/2434083002/#ps160001 (title: "rebase")
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 #9 (id:160001)
Message was sent while issue was closed.
Description was changed from ========== Expand WebsiteSettings histograms for HTTP-bad This CL expands WebsiteSettings histograms so that we can track actions broken down by each of the following omnibox states: - valid HTTPS url - HTTPS URL that has been downgraded because of e.g. mixed content - HTTPS URL that is marked dangerous (e.g. broken HTTPS or malware) - HTTP URL that is marked dangerous (e.g. malware) - HTTP URL that has a "Not secure" warning in the omnibox BUG=647566 ========== to ========== Expand WebsiteSettings histograms for HTTP-bad This CL expands WebsiteSettings histograms so that we can track actions broken down by each of the following omnibox states: - valid HTTPS url - HTTPS URL that has been downgraded because of e.g. mixed content - HTTPS URL that is marked dangerous (e.g. broken HTTPS or malware) - HTTP URL that is marked dangerous (e.g. malware) - HTTP URL that has a "Not secure" warning in the omnibox BUG=647566 Committed: https://crrev.com/8d67cd7a4a53be683945a0954aa4d4b3f4130302 Cr-Commit-Position: refs/heads/master@{#427017} ==========
Message was sent while issue was closed.
Patchset 9 (id:??) landed as https://crrev.com/8d67cd7a4a53be683945a0954aa4d4b3f4130302 Cr-Commit-Position: refs/heads/master@{#427017} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
