Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
690483003:
Log major user interactions with the WebsiteSettings (origin info) bubble. (Closed)
DescriptionLog major user interactions with the WebsiteSettings (origin info) bubble.
BUG=413439
Committed: https://crrev.com/04a9350dc121b9596ae80041d8da69adbbc5e607
Cr-Commit-Position: refs/heads/master@{#302679}
Patch Set 1 : #Patch Set 2 : Remove old actions. #
Total comments: 20
Patch Set 3 : Make Ilya's suggested changes and support OSX. #
Total comments: 1
Patch Set 4 : Fix indentation in switch clause. #
Messages
Total messages: 29 (10 generated)
Patchset #1 (id:1) has been deleted
lgarron@chromium.org changed reviewers: + isherman@chromium.org
Ilya, would you mind checking if this meets your standards for semantically awesome histogramming? :-)
https://codereview.chromium.org/690483003/diff/40001/chrome/browser/ui/views/... File chrome/browser/ui/views/website_settings/website_settings_popup_view.cc (right): https://codereview.chromium.org/690483003/diff/40001/chrome/browser/ui/views/... chrome/browser/ui/views/website_settings/website_settings_popup_view.cc:425: } nit: No need for curly braces for case stmts. https://codereview.chromium.org/690483003/diff/40001/chrome/browser/ui/views/... chrome/browser/ui/views/website_settings/website_settings_popup_view.cc:437: assert(false); nit: Did you mean "NOTREACHED()"? https://codereview.chromium.org/690483003/diff/40001/chrome/browser/ui/websit... File chrome/browser/ui/website_settings/website_settings.cc (right): https://codereview.chromium.org/690483003/diff/40001/chrome/browser/ui/websit... chrome/browser/ui/website_settings/website_settings.cc:213: UMA_HISTOGRAM_ENUMERATION("WebsiteSettings", This histogram currently just has a namespace, but is itself unnamed. It's like writing "namespace website_settings { class { ... }; }". Please give the histogram a name as well as a namespace :) https://codereview.chromium.org/690483003/diff/40001/chrome/browser/ui/websit... chrome/browser/ui/website_settings/website_settings.cc:213: UMA_HISTOGRAM_ENUMERATION("WebsiteSettings", Also, is "WebsiteSettings" a namespace that's already in use? If not, might there be an existing namespace that you could use, rather than defining a new one? https://codereview.chromium.org/690483003/diff/40001/chrome/browser/ui/websit... chrome/browser/ui/website_settings/website_settings.cc:220: UMA_HISTOGRAM_ENUMERATION("WebsiteSettings.HTTPS_URL", Optional nit: I'd write "HTTPS_URL" as "SecureUrl" or "HttpsUrl" or something like that. https://codereview.chromium.org/690483003/diff/40001/tools/metrics/actions/ac... File tools/metrics/actions/actions.xml (left): https://codereview.chromium.org/690483003/diff/40001/tools/metrics/actions/ac... tools/metrics/actions/actions.xml:11438: </action> Please mark these as <obsolete> rather than deleting them. https://codereview.chromium.org/690483003/diff/40001/tools/metrics/histograms... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/690483003/diff/40001/tools/metrics/histograms... tools/metrics/histograms/histograms.xml:53956: + <int value="1" label="Selected Permissions Tab"/> nit: "Tab" -> "tab" (throughout)
https://codereview.chromium.org/690483003/diff/40001/chrome/browser/ui/views/... File chrome/browser/ui/views/website_settings/website_settings_popup_view.cc (right): https://codereview.chromium.org/690483003/diff/40001/chrome/browser/ui/views/... chrome/browser/ui/views/website_settings/website_settings_popup_view.cc:437: assert(false); On 2014/10/29 23:55:42, Ilya Sherman wrote: > nit: Did you mean "NOTREACHED()"? For this and the braces, I was following the C++ style guide for switch statements: http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Loops_and_Swi... http://www.chromium.org/developers/coding-style doesn't seem to contain anything to override it. If everyone's using some other conventions for Chromium, are those documented anywhere? :-P https://codereview.chromium.org/690483003/diff/40001/chrome/browser/ui/websit... File chrome/browser/ui/website_settings/website_settings.cc (right): https://codereview.chromium.org/690483003/diff/40001/chrome/browser/ui/websit... chrome/browser/ui/website_settings/website_settings.cc:213: UMA_HISTOGRAM_ENUMERATION("WebsiteSettings", On 2014/10/29 23:55:42, Ilya Sherman wrote: > This histogram currently just has a namespace, but is itself unnamed. It's like > writing "namespace website_settings { class { ... }; }". Please give the > histogram a name as well as a namespace :) I originally planned just to have this histogram (in the top-level namespace) to keep it simple, and then added the other. I suppose I could name this WebsiteSettings.Actions. (This is why I remarked that the namespacing for Actions was easier. ;-) https://codereview.chromium.org/690483003/diff/40001/chrome/browser/ui/websit... chrome/browser/ui/website_settings/website_settings.cc:213: UMA_HISTOGRAM_ENUMERATION("WebsiteSettings", On 2014/10/29 23:55:42, Ilya Sherman wrote: > Also, is "WebsiteSettings" a namespace that's already in use? If not, might > there be an existing namespace that you could use, rather than defining a new > one? It's used by WebsiteSettings.PermissionChanged (unowned, maybe I should take ownership). In addition, this histogram is going to supersede two actions: - WebsiteSettings_Opened - WebsiteSettings_CookiesDialogOpened https://codereview.chromium.org/690483003/diff/40001/tools/metrics/actions/ac... File tools/metrics/actions/actions.xml (left): https://codereview.chromium.org/690483003/diff/40001/tools/metrics/actions/ac... tools/metrics/actions/actions.xml:11438: </action> On 2014/10/29 23:55:42, Ilya Sherman wrote: > Please mark these as <obsolete> rather than deleting them. Uh, sure. (I was about to note that I've deleted histograms in the past, but then I remembered I was also adding them to the open-source repo. :-P) https://codereview.chromium.org/690483003/diff/40001/tools/metrics/histograms... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/690483003/diff/40001/tools/metrics/histograms... tools/metrics/histograms/histograms.xml:53956: + <int value="1" label="Selected Permissions Tab"/> On 2014/10/29 23:55:42, Ilya Sherman wrote: > nit: "Tab" -> "tab" (throughout) Sure. But Permissions and Connection stay capitalized, right?
https://codereview.chromium.org/690483003/diff/40001/chrome/browser/ui/views/... File chrome/browser/ui/views/website_settings/website_settings_popup_view.cc (right): https://codereview.chromium.org/690483003/diff/40001/chrome/browser/ui/views/... chrome/browser/ui/views/website_settings/website_settings_popup_view.cc:437: assert(false); On 2014/10/30 00:10:19, lgarron wrote: > On 2014/10/29 23:55:42, Ilya Sherman wrote: > > nit: Did you mean "NOTREACHED()"? > > For this and the braces, I was following the C++ style guide for switch > statements: > > http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Loops_and_Swi... > > http://www.chromium.org/developers/coding-style doesn't seem to contain anything > to override it. > > If everyone's using some other conventions for Chromium, are those documented > anywhere? :-P Note that the style guide says "case blocks in switch statements can have curly braces or not, depending on your preference". In general, in Chromium code we omit curly braces for case blocks unless they are necessary for the code to compile. (If the local style in this file is different, then feel free to ignore this comment.) For assert(false) vs NOTREACHED, Chromium code does not, in general, use "assert"; we use DCHECK, CHECK, and NOTREACHED. I'm not sure this is documented anywhere, but it's not purely a stylistic concern -- DCHECK has slightly different semantics vs. assert. https://codereview.chromium.org/690483003/diff/40001/tools/metrics/histograms... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/690483003/diff/40001/tools/metrics/histograms... tools/metrics/histograms/histograms.xml:53956: + <int value="1" label="Selected Permissions Tab"/> On 2014/10/30 00:10:19, lgarron wrote: > On 2014/10/29 23:55:42, Ilya Sherman wrote: > > nit: "Tab" -> "tab" (throughout) > > Sure. But Permissions and Connection stay capitalized, right? I dunno, I think it comes down to personal preference. If I were writing the text, I'd probably wrap them in quotes, or else write them in lowercase. But I don't see the string "Permissions Tab" or "Connection Tab" in the UI, so writing "Tab" with an uppercase "T" is surprising no matter what.
https://codereview.chromium.org/690483003/diff/40001/chrome/browser/ui/views/... File chrome/browser/ui/views/website_settings/website_settings_popup_view.cc (right): https://codereview.chromium.org/690483003/diff/40001/chrome/browser/ui/views/... chrome/browser/ui/views/website_settings/website_settings_popup_view.cc:425: } On 2014/10/29 23:55:42, Ilya Sherman wrote: > nit: No need for curly braces for case stmts. Acknowledged. https://codereview.chromium.org/690483003/diff/40001/chrome/browser/ui/views/... chrome/browser/ui/views/website_settings/website_settings_popup_view.cc:437: assert(false); On 2014/10/30 01:32:56, Ilya Sherman wrote: > On 2014/10/30 00:10:19, lgarron wrote: > > On 2014/10/29 23:55:42, Ilya Sherman wrote: > > > nit: Did you mean "NOTREACHED()"? > > > > For this and the braces, I was following the C++ style guide for switch > > statements: > > > > > http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Loops_and_Swi... > > > > http://www.chromium.org/developers/coding-style doesn't seem to contain > anything > > to override it. > > > > If everyone's using some other conventions for Chromium, are those documented > > anywhere? :-P > > Note that the style guide says "case blocks in switch statements can have curly > braces or not, depending on your preference". In general, in Chromium code we > omit curly braces for case blocks unless they are necessary for the code to > compile. (If the local style in this file is different, then feel free to > ignore this comment.) > > For assert(false) vs NOTREACHED, Chromium code does not, in general, use > "assert"; we use DCHECK, CHECK, and NOTREACHED. I'm not sure this is documented > anywhere, but it's not purely a stylistic concern -- DCHECK has slightly > different semantics vs. assert. Acknowledged. https://codereview.chromium.org/690483003/diff/40001/chrome/browser/ui/websit... File chrome/browser/ui/website_settings/website_settings.cc (right): https://codereview.chromium.org/690483003/diff/40001/chrome/browser/ui/websit... chrome/browser/ui/website_settings/website_settings.cc:213: UMA_HISTOGRAM_ENUMERATION("WebsiteSettings", On 2014/10/29 23:55:42, Ilya Sherman wrote: > This histogram currently just has a namespace, but is itself unnamed. It's like > writing "namespace website_settings { class { ... }; }". Please give the > histogram a name as well as a namespace :) Acknowledged. https://codereview.chromium.org/690483003/diff/40001/chrome/browser/ui/websit... chrome/browser/ui/website_settings/website_settings.cc:220: UMA_HISTOGRAM_ENUMERATION("WebsiteSettings.HTTPS_URL", On 2014/10/29 23:55:42, Ilya Sherman wrote: > Optional nit: I'd write "HTTPS_URL" as "SecureUrl" or "HttpsUrl" or something > like that. SecureURL is probably a bad idea, but HttpsUrl sounds find, even if it's ugly. Are you fine with having the following two histograms? WebsiteSettings.Action WebsiteSettings.Action.HttpsUrl https://codereview.chromium.org/690483003/diff/40001/tools/metrics/actions/ac... File tools/metrics/actions/actions.xml (left): https://codereview.chromium.org/690483003/diff/40001/tools/metrics/actions/ac... tools/metrics/actions/actions.xml:11438: </action> On 2014/10/29 23:55:42, Ilya Sherman wrote: > Please mark these as <obsolete> rather than deleting them. Acknowledged. https://codereview.chromium.org/690483003/diff/40001/tools/metrics/histograms... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/690483003/diff/40001/tools/metrics/histograms... tools/metrics/histograms/histograms.xml:53956: + <int value="1" label="Selected Permissions Tab"/> On 2014/10/30 01:32:56, Ilya Sherman wrote: > On 2014/10/30 00:10:19, lgarron wrote: > > On 2014/10/29 23:55:42, Ilya Sherman wrote: > > > nit: "Tab" -> "tab" (throughout) > > > > Sure. But Permissions and Connection stay capitalized, right? > > I dunno, I think it comes down to personal preference. If I were writing the > text, I'd probably wrap them in quotes, or else write them in lowercase. But I > don't see the string "Permissions Tab" or "Connection Tab" in the UI, so writing > "Tab" with an uppercase "T" is surprising no matter what. Acknowledged.
Alright, I've updated everything (and also added the logging to OSX). Would you please take another look, Ilya? :-)
Alright, I've updated everything (and also added the logging to OSX). Would you please take another look, Ilya? :-)
LGTM, thanks. https://codereview.chromium.org/690483003/diff/60001/chrome/browser/ui/cocoa/... File chrome/browser/ui/cocoa/website_settings/website_settings_bubble_controller.mm (right): https://codereview.chromium.org/690483003/diff/60001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/website_settings/website_settings_bubble_controller.mm:957: NOTREACHED(); nit: Each of lines 948-952 and 955-957 should be indented two more spaces.
Patchset #4 (id:80001) has been deleted
lgarron@chromium.org changed reviewers: + markusheintz@chromium.org
Markus, would you please review the C++ changes? :-)
LGTM
The CQ bit was checked by lgarron@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/690483003/100001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
The CQ bit was checked by lgarron@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/690483003/100001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
lgarron@chromium.org changed reviewers: + rsesek@chromium.org
rsesek@: Would you please review the OSX code? (website_settings_bubble_controller.mm)
cocoa/ LGTM
The CQ bit was checked by lgarron@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/690483003/100001
Message was sent while issue was closed.
Committed patchset #4 (id:100001)
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/04a9350dc121b9596ae80041d8da69adbbc5e607 Cr-Commit-Position: refs/heads/master@{#302679} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
