Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1197853005:
Collecting statistics on iframe permissions use. (Closed)
|
Created:
5 years, 6 months ago by keenanb Modified:
5 years, 5 months ago Reviewers:
felt, mlamouri (slow - plz ping), jww, Alexei Svitkine (slow), palmer, Tom Sepez, Charlie Reis CC:
chromium-reviews, markusheintz_, Miguel Garcia Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionCollecting statistics on iframe permissions use.
The implementation of planned changes to iframe permissions handling
should hinge on the expected impact of the changes.
This rough patch allows us to collect some information about the expected impact
by monitoring how often an iframe requests permissions,
what kinds it requests, and what permissions its parent has or doesn't.
TBR=tsepez@chromium.org
Committed: https://crrev.com/edebda0b1b4feb0cc6f3d4a665a58efcc9f42d22
Cr-Commit-Position: refs/heads/master@{#338373}
Patch Set 1 #
Total comments: 29
Patch Set 2 : #Patch Set 3 : Made suggested fixes and changed logging details. #
Total comments: 15
Patch Set 4 : Fixed naming and style. #
Total comments: 22
Patch Set 5 : Cleaned up text and removed GetOrigin DCHECK. #
Total comments: 15
Patch Set 6 : Reworded bias comment and fixed naming. #
Total comments: 1
Patch Set 7 : Fixed to use permission status. #
Total comments: 8
Patch Set 8 : Fixed NOTREACHED calls. #
Total comments: 7
Patch Set 9 : Fixed documentation. #
Total comments: 3
Patch Set 10 : Implemented PERMISSION_STATUS_LAST more cleanly. #Patch Set 11 : Just rebased. #
Total comments: 7
Patch Set 12 : Fixed histogram name chaching issue. #Patch Set 13 : Avoided crash when missing PermissionManager. #
Total comments: 1
Patch Set 14 : Fixed nits. #Patch Set 15 : Just rebased. #
Messages
Total messages: 61 (13 generated)
keenanb@google.com changed reviewers: + jww@chromium.org
hey joel. i forgot to add reviewers on friday. this is a tiny bit of code to collect some UMA metrics. can you take a look and let me know about any small or big issues?
I was wondering about that :-) I'm about to jump on a flight, but I'll take a look tonight. On Mon, Jun 22, 2015, 2:03 PM <keenanb@google.com> wrote: > Reviewers: jww, > > Message: > hey joel. i forgot to add reviewers on friday. > > this is a tiny bit of code to collect some UMA metrics. can you take a look > and > let me know about any small or big issues? > > Description: > Collecting statistics on iframe permissions use. > > The implementation of planned changes to iframe permissions handling should > hinge on the expected impact of the changes. This rough patch allows us to > collect some information about the expected impact by monitoring how often > an > iframe requests permissions, what kinds it requests, and what permissions > its > parent has or doesn't. > > BUG= > > Please review this at https://codereview.chromium.org/1197853005/ > > Base URL: https://chromium.googlesource.com/chromium/src.git@master > > Affected files (+100, -6 lines): > M chrome/browser/content_settings/permission_context_base.cc > M chrome/browser/content_settings/permission_context_uma_util.h > M chrome/browser/content_settings/permission_context_uma_util.cc > > > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
mlamouri@chromium.org changed reviewers: + mlamouri@chromium.org
I think you are missing the histograms.xml changes. Otherwise, I left some drive-by comments. I will have another look tomorrow morning. https://codereview.chromium.org/1197853005/diff/1/chrome/browser/content_sett... File chrome/browser/content_settings/permission_context_base.cc (right): https://codereview.chromium.org/1197853005/diff/1/chrome/browser/content_sett... chrome/browser/content_settings/permission_context_base.cc:113: // Some permissions may not be used over unsecured http. I would prefer to avoid this comment because the general idea of the code is simple to understand. The only information missing is what is a secure origin but I don't think we should have a comment explaining that because it is something that can evolve with time. WDYT? https://codereview.chromium.org/1197853005/diff/1/chrome/browser/content_sett... File chrome/browser/content_settings/permission_context_uma_util.h (right): https://codereview.chromium.org/1197853005/diff/1/chrome/browser/content_sett... chrome/browser/content_settings/permission_context_uma_util.h:9: #include "components/content_settings/core/browser/host_content_settings_map.h" I think you could forward declare instead of including here. https://codereview.chromium.org/1197853005/diff/1/chrome/browser/content_sett... chrome/browser/content_settings/permission_context_uma_util.h:20: HostContentSettingsMap* hostContentSettingsMap, style: host_content_settings_map
felt@chromium.org changed reviewers: + felt@chromium.org
driveby review: I see a bunch of TODOs and FIXMEs in chrome/browser/content_settings/permission_context_uma_util.cc. * You should resolve those now, before we review again. * Or if these are actually about questions you have / places you are stuck, explicitly ask your reviewers your questions. https://codereview.chromium.org/1197853005/diff/1/chrome/browser/content_sett... File chrome/browser/content_settings/permission_context_uma_util.cc (right): https://codereview.chromium.org/1197853005/diff/1/chrome/browser/content_sett... chrome/browser/content_settings/permission_context_uma_util.cc:253: // TODO(keenanb): is calling GetOrigin redundant? Test out the behavior, do you need to call GetOrigin?
As felt said, it's a bit hard to look over this without addressing those TODOs/FIXMEs. Additionally, you should need histograms.xml additions to make all of these UMA collections actually work (https://code.google.com/p/chromium/codesearch#chromium/src/tools/metrics/hist...) https://codereview.chromium.org/1197853005/diff/1/chrome/browser/content_sett... File chrome/browser/content_settings/permission_context_base.cc (right): https://codereview.chromium.org/1197853005/diff/1/chrome/browser/content_sett... chrome/browser/content_settings/permission_context_base.cc:113: // Some permissions may not be used over unsecured http. On 2015/06/22 22:10:57, Mounir Lamouri wrote: > I would prefer to avoid this comment because the general idea of the code is > simple to understand. The only information missing is what is a secure origin > but I don't think we should have a comment explaining that because it is > something that can evolve with time. WDYT? Additionally, "what is a secure origin" is defined in the comment for IsOriginSecure: https://code.google.com/p/chromium/codesearch#chromium/src/content/public/com... https://codereview.chromium.org/1197853005/diff/1/chrome/browser/content_sett... File chrome/browser/content_settings/permission_context_uma_util.cc (right): https://codereview.chromium.org/1197853005/diff/1/chrome/browser/content_sett... chrome/browser/content_settings/permission_context_uma_util.cc:155: // TODO(keenanb) I don't really think this is necessary at all, so per felt's comment, I would just get rid of the TODO. Given that this will just be used for UMA collection, it's not really a big deal if it reaches an unknown content setting or permission type (thus no need to assert unreached). https://codereview.chromium.org/1197853005/diff/1/chrome/browser/content_sett... chrome/browser/content_settings/permission_context_uma_util.cc:178: // NOTREACHED() << "CONTENT SETTING " See above comment. https://codereview.chromium.org/1197853005/diff/1/chrome/browser/content_sett... chrome/browser/content_settings/permission_context_uma_util.cc:261: base::StringPrintf( Use base::SStringPrintf instead to avoid the call to c_str() (which is often not a good thing to do, since that assumes ASCII): https://code.google.com/p/chromium/codesearch#chromium/src/base/strings/strin... https://codereview.chromium.org/1197853005/diff/1/chrome/browser/content_sett... chrome/browser/content_settings/permission_context_uma_util.cc:269: UMA_HISTOGRAM_ENUMERATION( See above re: SStringPrintf. https://codereview.chromium.org/1197853005/diff/1/chrome/browser/content_sett... chrome/browser/content_settings/permission_context_uma_util.cc:270: base::StringPrintf("ContentSettings.PermissionRequested_TopFrame.%s", Personally, I think this would be a little easier to follow if there was only one UMA_HISTOGRAM_UNUMERATION() call, and the "IFrame"/"TopFrame" was another %s. https://codereview.chromium.org/1197853005/diff/1/chrome/browser/content_sett... File chrome/browser/content_settings/permission_context_uma_util.h (right): https://codereview.chromium.org/1197853005/diff/1/chrome/browser/content_sett... chrome/browser/content_settings/permission_context_uma_util.h:18: static void PermissionRequested( Is there a particular reason you reordered this?
https://codereview.chromium.org/1197853005/diff/1/chrome/browser/content_sett... File chrome/browser/content_settings/permission_context_uma_util.cc (right): https://codereview.chromium.org/1197853005/diff/1/chrome/browser/content_sett... chrome/browser/content_settings/permission_context_uma_util.cc:155: // TODO(keenanb) On 2015/06/23 at 05:32:58, jww wrote: > I don't really think this is necessary at all, so per felt's comment, I would just get rid of the TODO. Given that this will just be used for UMA collection, it's not really a big deal if it reaches an unknown content setting or permission type (thus no need to assert unreached). You might actually want to have no default case so the compilation will break if a new PermissionType is added. https://codereview.chromium.org/1197853005/diff/1/chrome/browser/content_sett... chrome/browser/content_settings/permission_context_uma_util.cc:178: // NOTREACHED() << "CONTENT SETTING " On 2015/06/23 at 05:32:58, jww wrote: > See above comment. ditto. Also, you probably don't need SessionOnly, Default and DetectImportant. They are not used for permissions AFAICT. https://codereview.chromium.org/1197853005/diff/1/chrome/browser/content_sett... chrome/browser/content_settings/permission_context_uma_util.cc:248: hostContentSettingsMap->GetContentSettingAndMaybeUpdateLastUsage( I'm not sure you want to call the "MaybeUpdateLastUsage" variant of GetContentSetting(). Recording a permission shouldn't mark the permission as being used. https://codereview.chromium.org/1197853005/diff/1/chrome/browser/content_sett... chrome/browser/content_settings/permission_context_uma_util.cc:257: if (requester_is_off_origin_iframe) { I guess you can get ride of the bool and directly do the check here. Also, you could do a direct string comparaison. These two strings should be origins. I think it would be fair to make this an explicit contract by adding DCHECK() at the beginning of the method (ie. DCHECK(requesting_orinig.GetOring() == requesting_origin) and same for embedding_origin)
thanks for your feedback. i'm in the process of uploading a new patch. it is for minor testing and will have printfs and such removed before the next (real) upload. (sorry if some of my responses to your messages are a little out of sync with what i actually did.) https://codereview.chromium.org/1197853005/diff/1/chrome/browser/content_sett... File chrome/browser/content_settings/permission_context_base.cc (right): https://codereview.chromium.org/1197853005/diff/1/chrome/browser/content_sett... chrome/browser/content_settings/permission_context_base.cc:113: // Some permissions may not be used over unsecured http. On 2015/06/22 22:10:57, Mounir Lamouri wrote: > I would prefer to avoid this comment because the general idea of the code is > simple to understand. The only information missing is what is a secure origin > but I don't think we should have a comment explaining that because it is > something that can evolve with time. WDYT? Done. https://codereview.chromium.org/1197853005/diff/1/chrome/browser/content_sett... File chrome/browser/content_settings/permission_context_uma_util.cc (right): https://codereview.chromium.org/1197853005/diff/1/chrome/browser/content_sett... chrome/browser/content_settings/permission_context_uma_util.cc:155: // TODO(keenanb) On 2015/06/23 05:32:58, jww wrote: > I don't really think this is necessary at all, so per felt's comment, I would > just get rid of the TODO. Given that this will just be used for UMA collection, > it's not really a big deal if it reaches an unknown content setting or > permission type (thus no need to assert unreached). Done. https://codereview.chromium.org/1197853005/diff/1/chrome/browser/content_sett... chrome/browser/content_settings/permission_context_uma_util.cc:155: // TODO(keenanb) On 2015/06/23 10:02:32, Mounir Lamouri wrote: > On 2015/06/23 at 05:32:58, jww wrote: > > I don't really think this is necessary at all, so per felt's comment, I would > just get rid of the TODO. Given that this will just be used for UMA collection, > it's not really a big deal if it reaches an unknown content setting or > permission type (thus no need to assert unreached). > > You might actually want to have no default case so the compilation will break if > a new PermissionType is added. Done. https://codereview.chromium.org/1197853005/diff/1/chrome/browser/content_sett... chrome/browser/content_settings/permission_context_uma_util.cc:178: // NOTREACHED() << "CONTENT SETTING " On 2015/06/23 05:32:58, jww wrote: > See above comment. Done. https://codereview.chromium.org/1197853005/diff/1/chrome/browser/content_sett... chrome/browser/content_settings/permission_context_uma_util.cc:178: // NOTREACHED() << "CONTENT SETTING " On 2015/06/23 10:02:32, Mounir Lamouri wrote: > On 2015/06/23 at 05:32:58, jww wrote: > > See above comment. > > ditto. Also, you probably don't need SessionOnly, Default and DetectImportant. > They are not used for permissions AFAICT. Done. https://codereview.chromium.org/1197853005/diff/1/chrome/browser/content_sett... chrome/browser/content_settings/permission_context_uma_util.cc:248: hostContentSettingsMap->GetContentSettingAndMaybeUpdateLastUsage( On 2015/06/23 10:02:32, Mounir Lamouri wrote: > I'm not sure you want to call the "MaybeUpdateLastUsage" variant of > GetContentSetting(). Recording a permission shouldn't mark the permission as > being used. Done. https://codereview.chromium.org/1197853005/diff/1/chrome/browser/content_sett... chrome/browser/content_settings/permission_context_uma_util.cc:253: // TODO(keenanb): is calling GetOrigin redundant? On 2015/06/22 22:56:01, felt wrote: > Test out the behavior, do you need to call GetOrigin? i had tested it. it looks to be always a pure origin. but "looks to be" isn't good enough. there are many different call sites for the enclosing function. we'll call GetOrigin to be safe. https://codereview.chromium.org/1197853005/diff/1/chrome/browser/content_sett... chrome/browser/content_settings/permission_context_uma_util.cc:257: if (requester_is_off_origin_iframe) { On 2015/06/23 10:02:32, Mounir Lamouri wrote: > I guess you can get ride of the bool and directly do the check here. Also, you > could do a direct string comparaison. These two strings should be origins. I > think it would be fair to make this an explicit contract by adding DCHECK() at > the beginning of the method (ie. DCHECK(requesting_orinig.GetOring() == > requesting_origin) and same for embedding_origin) thanks for the tip. partly done. i'd rather keep the bool because it makes the code easy to read and extend. feel free to comment on this issue again on the next iteration though. https://codereview.chromium.org/1197853005/diff/1/chrome/browser/content_sett... chrome/browser/content_settings/permission_context_uma_util.cc:261: base::StringPrintf( On 2015/06/23 05:32:58, jww wrote: > Use base::SStringPrintf instead to avoid the call to c_str() (which is often not > a good thing to do, since that assumes ASCII): > https://code.google.com/p/chromium/codesearch#chromium/src/base/strings/strin... Acknowledged. https://codereview.chromium.org/1197853005/diff/1/chrome/browser/content_sett... chrome/browser/content_settings/permission_context_uma_util.cc:269: UMA_HISTOGRAM_ENUMERATION( On 2015/06/23 05:32:58, jww wrote: > See above re: SStringPrintf. Acknowledged. https://codereview.chromium.org/1197853005/diff/1/chrome/browser/content_sett... chrome/browser/content_settings/permission_context_uma_util.cc:270: base::StringPrintf("ContentSettings.PermissionRequested_TopFrame.%s", On 2015/06/23 05:32:58, jww wrote: > Personally, I think this would be a little easier to follow if there was only > one UMA_HISTOGRAM_UNUMERATION() call, and the "IFrame"/"TopFrame" was another > %s. good point. but that would only almost work. the extra "TopFrame%s" in the first case would require and extra and nested printf to make it work that way. i'm going to try to improve it in other ways. https://codereview.chromium.org/1197853005/diff/1/chrome/browser/content_sett... File chrome/browser/content_settings/permission_context_uma_util.h (right): https://codereview.chromium.org/1197853005/diff/1/chrome/browser/content_sett... chrome/browser/content_settings/permission_context_uma_util.h:9: #include "components/content_settings/core/browser/host_content_settings_map.h" On 2015/06/22 22:10:57, Mounir Lamouri wrote: > I think you could forward declare instead of including here. forward declaring causes "member access into incomplete type". https://codereview.chromium.org/1197853005/diff/1/chrome/browser/content_sett... chrome/browser/content_settings/permission_context_uma_util.h:18: static void PermissionRequested( On 2015/06/23 05:32:58, jww wrote: > Is there a particular reason you reordered this? a permission request is different from a user's permission action. https://codereview.chromium.org/1197853005/diff/1/chrome/browser/content_sett... chrome/browser/content_settings/permission_context_uma_util.h:20: HostContentSettingsMap* hostContentSettingsMap, On 2015/06/22 22:10:57, Mounir Lamouri wrote: > style: host_content_settings_map Done.
This patch should be clean and complete (for what it attempts). The histogram ContentSettings.PermissionRequested.OffOrigin_Geolocation records the permission setting of the top frame. It could be better named. https://codereview.chromium.org/1197853005/diff/40001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/1197853005/diff/40001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:72536: + " this " symbol was inserted automatically by a script. i can't tell whether it is a bug.
https://codereview.chromium.org/1197853005/diff/40001/chrome/browser/content_... File chrome/browser/content_settings/permission_context_uma_util.cc (right): https://codereview.chromium.org/1197853005/diff/40001/chrome/browser/content_... chrome/browser/content_settings/permission_context_uma_util.cc:155: return "?"; NOT_REACHED(); And after the switch() |return "";| Feel free to |return "";| in the case ::NUM but it will unlikely compile on all platforms. https://codereview.chromium.org/1197853005/diff/40001/chrome/browser/content_... chrome/browser/content_settings/permission_context_uma_util.cc:235: static_cast<base::HistogramBase::Sample>(PermissionType::NUM)); Out of curiosity, why do we want to count these? https://codereview.chromium.org/1197853005/diff/40001/chrome/browser/content_... File chrome/browser/content_settings/permission_context_uma_util.h (right): https://codereview.chromium.org/1197853005/diff/40001/chrome/browser/content_... chrome/browser/content_settings/permission_context_uma_util.h:9: #include "components/content_settings/core/browser/host_content_settings_map.h" nit: forward declare. https://codereview.chromium.org/1197853005/diff/40001/chrome/browser/content_... chrome/browser/content_settings/permission_context_uma_util.h:20: HostContentSettingsMap* host_content_settings_map, nit: I think it would be better to have this as the fourth argument to keep consistency between the methods. Also, maybe you could keep it in the same position in the file? https://codereview.chromium.org/1197853005/diff/40001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/1197853005/diff/40001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:3915: +<histogram name="ContentSettings.PermissionRequested.OffOrigin" nit: s/OffOrigin/CrossOrigin/ ? https://codereview.chromium.org/1197853005/diff/40001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:3922: + origin has that permission type already allowed, blocked, or neither. The description is confusing. Especially the "already". We might end up counting embedders that never had their permissions changed, right? https://codereview.chromium.org/1197853005/diff/40001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:3926: +<histogram name="ContentSettings.PermissionRequested.OnOrigin" nit: s/OnOrigin/SameOrigin/ ?
Hopefully one last review will finish this off. https://codereview.chromium.org/1197853005/diff/40001/chrome/browser/content_... File chrome/browser/content_settings/permission_context_uma_util.cc (right): https://codereview.chromium.org/1197853005/diff/40001/chrome/browser/content_... chrome/browser/content_settings/permission_context_uma_util.cc:155: return "?"; On 2015/06/29 14:23:07, Mounir Lamouri wrote: > NOT_REACHED(); > > And after the switch() |return "";| > > Feel free to |return "";| in the case ::NUM but it will unlikely compile on all > platforms. Done. https://codereview.chromium.org/1197853005/diff/40001/chrome/browser/content_... chrome/browser/content_settings/permission_context_uma_util.cc:235: static_cast<base::HistogramBase::Sample>(PermissionType::NUM)); On 2015/06/29 14:23:07, Mounir Lamouri wrote: > Out of curiosity, why do we want to count these? to know the ratio. maybe we don't want it though. https://codereview.chromium.org/1197853005/diff/40001/chrome/browser/content_... File chrome/browser/content_settings/permission_context_uma_util.h (right): https://codereview.chromium.org/1197853005/diff/40001/chrome/browser/content_... chrome/browser/content_settings/permission_context_uma_util.h:9: #include "components/content_settings/core/browser/host_content_settings_map.h" On 2015/06/29 14:23:07, Mounir Lamouri wrote: > nit: forward declare. Done. https://codereview.chromium.org/1197853005/diff/40001/chrome/browser/content_... chrome/browser/content_settings/permission_context_uma_util.h:20: HostContentSettingsMap* host_content_settings_map, On 2015/06/29 14:23:07, Mounir Lamouri wrote: > nit: I think it would be better to have this as the fourth argument to keep > consistency between the methods. Also, maybe you could keep it in the same > position in the file? i moved the declaration because originally there was no particular ordering, not alphabetical or logical. and more importantly, originally the orderings in *.cc and *.h were inconsistent. this way, the ordering is consistent and logical. https://codereview.chromium.org/1197853005/diff/40001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/1197853005/diff/40001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:3915: +<histogram name="ContentSettings.PermissionRequested.OffOrigin" On 2015/06/29 14:23:08, Mounir Lamouri wrote: > nit: s/OffOrigin/CrossOrigin/ ? i actually agree with you, but "OffOrigin" is common in histograms.xml, whereas "CrossOrigin" is not used. https://codereview.chromium.org/1197853005/diff/40001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:3922: + origin has that permission type already allowed, blocked, or neither. On 2015/06/29 14:23:07, Mounir Lamouri wrote: > The description is confusing. Especially the "already". We might end up counting > embedders that never had their permissions changed, right? you are right; it is confusing. i changed it. https://codereview.chromium.org/1197853005/diff/40001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:3926: +<histogram name="ContentSettings.PermissionRequested.OnOrigin" On 2015/06/29 14:23:07, Mounir Lamouri wrote: > nit: s/OnOrigin/SameOrigin/ ? Done.
Looking much better! https://codereview.chromium.org/1197853005/diff/60001/chrome/browser/content_... File chrome/browser/content_settings/permission_context_uma_util.cc (right): https://codereview.chromium.org/1197853005/diff/60001/chrome/browser/content_... chrome/browser/content_settings/permission_context_uma_util.cc:155: case PermissionType::NUM: nit: Can you make this "default" and then drop the return value at the end? https://codereview.chromium.org/1197853005/diff/60001/chrome/browser/content_... chrome/browser/content_settings/permission_context_uma_util.cc:158: return ""; If you do leave in this empty string return value, change to return std::string(); https://codereview.chromium.org/1197853005/diff/60001/chrome/browser/content_... chrome/browser/content_settings/permission_context_uma_util.cc:214: // TODO(keenanb): It may not be worth worrying about, Change this to "We have decided not to worry about it for now, but..." (or something equivalent. The point is, we've considered it so there's no "may" for now :-) https://codereview.chromium.org/1197853005/diff/60001/chrome/browser/content_... chrome/browser/content_settings/permission_context_uma_util.cc:220: nit: extraneous newline. https://codereview.chromium.org/1197853005/diff/60001/chrome/browser/content_... chrome/browser/content_settings/permission_context_uma_util.cc:222: DCHECK(embedding_origin.GetOrigin() == embedding_origin); Why are these DCHECKs necessary? In particular, nothing in the comments to this function indicate that requesting_origin or embedding_origin *must* be origin only, so a future caller might use this function and be very surprised by this behavior. Thus, you need to either: (a) Document this behavior in the header file comments for this function. (b) Remove this DCHECK and (as mentioned below), call GetOrigin() when the origins are being used. Unless there's a very good reason not to, I strongly prefer (b) as (a) is not a common paradigm in Chrome. https://codereview.chromium.org/1197853005/diff/60001/chrome/browser/content_... chrome/browser/content_settings/permission_context_uma_util.cc:223: bool cross_origin_iframe = (requesting_origin != embedding_origin); If you're able to get rid of the above DCHECKs, change this comparison to (request_origin.GetOrigin() != embedding_origin.GetOrigin()). https://codereview.chromium.org/1197853005/diff/60001/chrome/browser/content_... chrome/browser/content_settings/permission_context_uma_util.cc:231: PermissionTypeToString(type), Formatting looks a little funky to me here (but I could be mistaken); make sure to run git cl format. https://codereview.chromium.org/1197853005/diff/60001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/1197853005/diff/60001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:3921: + an off-origin iframe permission request (for a given permission type. See nit: This parenthetical reads a bit oddly to me. Maybe take away the parentheses, so it reads like: "...an off-origin iframe permission request for a given permission type. See the corresponding histogram suffixes." https://codereview.chromium.org/1197853005/diff/60001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:3927: + enum="PermissionType"> Shouldn't there be an 's' at the end of this, i.e. "PermissionTypes"? https://codereview.chromium.org/1197853005/diff/60001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:72536: + " What's up with this """?
https://codereview.chromium.org/1197853005/diff/60001/chrome/browser/content_... File chrome/browser/content_settings/permission_context_uma_util.cc (right): https://codereview.chromium.org/1197853005/diff/60001/chrome/browser/content_... chrome/browser/content_settings/permission_context_uma_util.cc:155: case PermissionType::NUM: On 2015/06/29 21:26:59, jww wrote: > nit: Can you make this "default" and then drop the return value at the end? the way it is now, it will fail fast and loud when a new permission type is added to PermissionType. using "default" would spoil that. https://codereview.chromium.org/1197853005/diff/60001/chrome/browser/content_... chrome/browser/content_settings/permission_context_uma_util.cc:158: return ""; On 2015/06/29 21:26:59, jww wrote: > If you do leave in this empty string return value, change to return > std::string(); Done. https://codereview.chromium.org/1197853005/diff/60001/chrome/browser/content_... chrome/browser/content_settings/permission_context_uma_util.cc:214: // TODO(keenanb): It may not be worth worrying about, On 2015/06/29 21:26:59, jww wrote: > Change this to "We have decided not to worry about it for now, but..." (or > something equivalent. The point is, we've considered it so there's no "may" for > now :-) good point! done. https://codereview.chromium.org/1197853005/diff/60001/chrome/browser/content_... chrome/browser/content_settings/permission_context_uma_util.cc:220: On 2015/06/29 21:26:59, jww wrote: > nit: extraneous newline. Done. https://codereview.chromium.org/1197853005/diff/60001/chrome/browser/content_... chrome/browser/content_settings/permission_context_uma_util.cc:222: DCHECK(embedding_origin.GetOrigin() == embedding_origin); On 2015/06/29 21:26:59, jww wrote: > Why are these DCHECKs necessary? In particular, nothing in the comments to this > function indicate that requesting_origin or embedding_origin *must* be origin > only, so a future caller might use this function and be very surprised by this > behavior. > > Thus, you need to either: > (a) Document this behavior in the header file comments for this function. > (b) Remove this DCHECK and (as mentioned below), call GetOrigin() when the > origins are being used. > > Unless there's a very good reason not to, I strongly prefer (b) as (a) is not a > common paradigm in Chrome. (b) it is. https://codereview.chromium.org/1197853005/diff/60001/chrome/browser/content_... chrome/browser/content_settings/permission_context_uma_util.cc:223: bool cross_origin_iframe = (requesting_origin != embedding_origin); On 2015/06/29 21:26:59, jww wrote: > If you're able to get rid of the above DCHECKs, change this comparison to > (request_origin.GetOrigin() != embedding_origin.GetOrigin()). Done. https://codereview.chromium.org/1197853005/diff/60001/chrome/browser/content_... chrome/browser/content_settings/permission_context_uma_util.cc:231: PermissionTypeToString(type), On 2015/06/29 21:26:59, jww wrote: > Formatting looks a little funky to me here (but I could be mistaken); make sure > to run git cl format. i believe i did. i'll make sure to again. https://codereview.chromium.org/1197853005/diff/60001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/1197853005/diff/60001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:3921: + an off-origin iframe permission request (for a given permission type. See On 2015/06/29 21:26:59, jww wrote: > nit: This parenthetical reads a bit oddly to me. Maybe take away the > parentheses, so it reads like: > > "...an off-origin iframe permission request for a given permission type. See the > corresponding histogram suffixes." Done. https://codereview.chromium.org/1197853005/diff/60001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:3927: + enum="PermissionType"> On 2015/06/29 21:26:59, jww wrote: > Shouldn't there be an 's' at the end of this, i.e. "PermissionTypes"? no. there is an enum called PermissionType. there just happens to be a histogram suffixes entry with basically the same name. https://codereview.chromium.org/1197853005/diff/60001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:72536: + " On 2015/06/29 21:26:59, jww wrote: > What's up with this """? i have no idea. a clean-up script automatically inserted it, and i don't know if it is due to a bug. i mentioned it in my last upload. i've deleted it now and the validator doesn't miss it.
lgtm, with my one comment. However, make sure mlamouri@ and felt@ are happy, too. In particular, make sure mlamouri@ or felt@ looks at the suffixes business in histograms.xml because I really don't know how that works. https://codereview.chromium.org/1197853005/diff/60001/chrome/browser/content_... File chrome/browser/content_settings/permission_context_uma_util.cc (right): https://codereview.chromium.org/1197853005/diff/60001/chrome/browser/content_... chrome/browser/content_settings/permission_context_uma_util.cc:155: case PermissionType::NUM: On 2015/06/29 22:43:44, keenanb wrote: > On 2015/06/29 21:26:59, jww wrote: > > nit: Can you make this "default" and then drop the return value at the end? > > the way it is now, it will fail fast and loud when a new permission type is > added to PermissionType. using "default" would spoil that. Fair. https://codereview.chromium.org/1197853005/diff/60001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/1197853005/diff/60001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:3927: + enum="PermissionType"> On 2015/06/29 22:43:45, keenanb wrote: > On 2015/06/29 21:26:59, jww wrote: > > Shouldn't there be an 's' at the end of this, i.e. "PermissionTypes"? > > no. there is an enum called PermissionType. there just happens > to be a histogram suffixes entry with basically the same name. Yikes! That kinda sucks. Do we really need to change the "PermissionActions" to "PermissionTypes"? Seems confusing to me.
On 2015/06/29 22:53:23, jww wrote: > https://codereview.chromium.org/1197853005/diff/60001/tools/metrics/histogram... > File tools/metrics/histograms/histograms.xml (right): > > https://codereview.chromium.org/1197853005/diff/60001/tools/metrics/histogram... > tools/metrics/histograms/histograms.xml:3927: + enum="PermissionType"> > On 2015/06/29 22:43:45, keenanb wrote: > > On 2015/06/29 21:26:59, jww wrote: > > > Shouldn't there be an 's' at the end of this, i.e. "PermissionTypes"? > > > > no. there is an enum called PermissionType. there just happens > > to be a histogram suffixes entry with basically the same name. > > Yikes! That kinda sucks. Do we really need to change the "PermissionActions" to > "PermissionTypes"? Seems confusing to me. well then there would be both an enum called PermissionAction (without "s") and a suffixes entry called PermissionActions (with "s"). besides, it doesn't make sense to call it "PermissionActions" when it is being used for something unrelated.
Sorry, I still have a few comments. It looks way better but the UMA we have for permissions are a bit dusty. I would like to have these two to be better :) https://codereview.chromium.org/1197853005/diff/80001/chrome/browser/content_... File chrome/browser/content_settings/permission_context_uma_util.cc (right): https://codereview.chromium.org/1197853005/diff/80001/chrome/browser/content_... chrome/browser/content_settings/permission_context_uma_util.cc:158: return std::string(); nit: could you add a NOTREACHED() here too? Sorry, I should have pointed that before. https://codereview.chromium.org/1197853005/diff/80001/chrome/browser/content_... chrome/browser/content_settings/permission_context_uma_util.cc:219: // That may be a job for RAPPOR. nit: the wrapping of that comment block seems off. It should wrap at 80 characters. Also, I think I would leave a comment like this instead: """ In order to improve the security models for iframes around permissions, we need to know the ratio between same origin and cross origin iframes requesting permissions and the permission state of their embedder when cross origin. This ration might be biased if an iframe makes multiple requests that end up being saved but it was considered as statically insignificant. """ https://codereview.chromium.org/1197853005/diff/80001/chrome/browser/content_... chrome/browser/content_settings/permission_context_uma_util.cc:223: embedding_origin, embedding_origin, permission, std::string()); Could you get the profile instead of HostContentSettingsMap and call GetPermissionStatus()? Then, you print the PermissionStatus. https://codereview.chromium.org/1197853005/diff/80001/chrome/browser/content_... File chrome/browser/content_settings/permission_context_uma_util.h (right): https://codereview.chromium.org/1197853005/diff/80001/chrome/browser/content_... chrome/browser/content_settings/permission_context_uma_util.h:9: #include "components/content_settings/core/common/content_settings.h" nit: why do you need this include? https://codereview.chromium.org/1197853005/diff/80001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/1197853005/diff/80001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:3915: +<histogram name="ContentSettings.PermissionRequested.OffOrigin" Could you rename this to "Permissions.Requested.CrossOrigin"? I prefer to remove the "ContentSettings" name from there and we should use "CrossOrigin". I wouldn't worry about consistency here. https://codereview.chromium.org/1197853005/diff/80001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:3926: +<histogram name="ContentSettings.PermissionRequested.SameOrigin" ditto: "Permissions.Requested.SameOrigin" https://codereview.chromium.org/1197853005/diff/80001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:72526: +<histogram_suffixes name="PermissionTypes"> That change is not correct, it would change the UMA values. They are meant to be GRANTED/DENIED/DISMISSED/IGNORED. That change will make them be Geolocation/Notifications/... You might need a new one with PermissionStatus.
https://codereview.chromium.org/1197853005/diff/80001/chrome/browser/content_... File chrome/browser/content_settings/permission_context_uma_util.cc (right): https://codereview.chromium.org/1197853005/diff/80001/chrome/browser/content_... chrome/browser/content_settings/permission_context_uma_util.cc:158: return std::string(); On 2015/06/30 10:27:20, Mounir Lamouri wrote: > nit: could you add a NOTREACHED() here too? Sorry, I should have pointed that > before. might as well just move NOTREACHED. https://codereview.chromium.org/1197853005/diff/80001/chrome/browser/content_... chrome/browser/content_settings/permission_context_uma_util.cc:219: // That may be a job for RAPPOR. On 2015/06/30 10:27:20, Mounir Lamouri wrote: > nit: the wrapping of that comment block seems off. It should wrap at 80 > characters. > > Also, I think I would leave a comment like this instead: > """ > In order to improve the security models for iframes around permissions, > we need to know the ratio between same origin and cross origin iframes > requesting permissions and the permission state of their embedder when > cross origin. > This ration might be biased if an iframe makes multiple requests that > end up being saved but it was considered as statically insignificant. > """ Done. https://codereview.chromium.org/1197853005/diff/80001/chrome/browser/content_... chrome/browser/content_settings/permission_context_uma_util.cc:223: embedding_origin, embedding_origin, permission, std::string()); On 2015/06/30 10:27:20, Mounir Lamouri wrote: > Could you get the profile instead of HostContentSettingsMap and call > GetPermissionStatus()? Then, you print the PermissionStatus. Profile does not expose GetPermissionStatus; it's PermissionContext that does. i chose not to pass either of those in order to avoid circular dependency. let me know what you think. https://codereview.chromium.org/1197853005/diff/80001/chrome/browser/content_... File chrome/browser/content_settings/permission_context_uma_util.h (right): https://codereview.chromium.org/1197853005/diff/80001/chrome/browser/content_... chrome/browser/content_settings/permission_context_uma_util.h:9: #include "components/content_settings/core/common/content_settings.h" On 2015/06/30 10:27:20, Mounir Lamouri wrote: > nit: why do you need this include? that's a vestige. removed. https://codereview.chromium.org/1197853005/diff/80001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/1197853005/diff/80001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:3915: +<histogram name="ContentSettings.PermissionRequested.OffOrigin" On 2015/06/30 10:27:20, Mounir Lamouri wrote: > Could you rename this to "Permissions.Requested.CrossOrigin"? I prefer to remove > the "ContentSettings" name from there and we should use "CrossOrigin". I > wouldn't worry about consistency here. done. consistency aside, that naming makes more sense. https://codereview.chromium.org/1197853005/diff/80001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:3926: +<histogram name="ContentSettings.PermissionRequested.SameOrigin" On 2015/06/30 10:27:21, Mounir Lamouri wrote: > ditto: "Permissions.Requested.SameOrigin" Done. https://codereview.chromium.org/1197853005/diff/80001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:72526: +<histogram_suffixes name="PermissionTypes"> On 2015/06/30 10:27:20, Mounir Lamouri wrote: > That change is not correct, it would change the UMA values. They are meant to be > GRANTED/DENIED/DISMISSED/IGNORED. That change will make them be > Geolocation/Notifications/... > > You might need a new one with PermissionStatus. i don't understand. can you double check to see whether you are mixing something up?
lgtm if you use PermissionStatus instead of ContentSetting for the permission. You might want to ask a histograms owner for the histograms.xml changes. https://codereview.chromium.org/1197853005/diff/80001/chrome/browser/content_... File chrome/browser/content_settings/permission_context_uma_util.cc (right): https://codereview.chromium.org/1197853005/diff/80001/chrome/browser/content_... chrome/browser/content_settings/permission_context_uma_util.cc:223: embedding_origin, embedding_origin, permission, std::string()); On 2015/06/30 at 21:20:08, keenanb wrote: > On 2015/06/30 10:27:20, Mounir Lamouri wrote: > > Could you get the profile instead of HostContentSettingsMap and call > > GetPermissionStatus()? Then, you print the PermissionStatus. > > Profile does not expose GetPermissionStatus; it's PermissionContext that does. i chose not to pass either of those in order to avoid circular dependency. > > let me know what you think. You can get the PermissionManager from a profile (Profile::GetPermissionManager()). https://codereview.chromium.org/1197853005/diff/100001/tools/metrics/histogra... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/1197853005/diff/100001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:72826: +<histogram_suffixes name="PermissionTypes"> I think that changes isn't correct. CrossOrigin isn't using PermissionActions while the other histograms in that list do. Though, I'm no histograms expert so I might be wrong.
hey, mlamouri or jww. could you double check this before i commit. you both approved earlier, but i had to make slightly broader changes in order to implement mlamouri's suggestion. also, mlamouri, i think i've got the histogram stuff right.
Generally still lgtm, with a few nits. Once you've corrected those, make sure to add asvitkine@chromium.org to the reviewers to check the histograms.xml file. https://codereview.chromium.org/1197853005/diff/120001/chrome/browser/content... File chrome/browser/content_settings/permission_context_uma_util.cc (right): https://codereview.chromium.org/1197853005/diff/120001/chrome/browser/content... chrome/browser/content_settings/permission_context_uma_util.cc:160: NOTREACHED(); This NOTREACHED will, well, never be reached :-) The NOTREACHED should be before the return statement above it. If possible, remove the return statement entirely, although I'm not sure that will compile. https://codereview.chromium.org/1197853005/diff/120001/content/child/backgrou... File content/child/background_sync/background_sync_provider.cc (right): https://codereview.chromium.org/1197853005/diff/120001/content/child/backgrou... content/child/background_sync/background_sync_provider.cc:278: break; Shouldn't this be a NOTREACHED? https://codereview.chromium.org/1197853005/diff/120001/content/child/permissi... File content/child/permissions/permission_dispatcher.cc (right): https://codereview.chromium.org/1197853005/diff/120001/content/child/permissi... content/child/permissions/permission_dispatcher.cc:60: break; Again, shouldn't this be a NOTREACHED (and of course add a comment to all these functions specifying that PERMISSION_STATUS_NUM should never be passed in) https://codereview.chromium.org/1197853005/diff/120001/tools/metrics/histogra... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/1197853005/diff/120001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:63782: +<enum name="PermissionStatus" type="int"> Shouldn't this be far below in the <enums> section? Also, make sure you run the histograms.xml tools to pretty print and verify the file.
On 2015/07/07 18:41:46, jww wrote: > Generally still lgtm, with a few nits. Once you've corrected those, make sure to > add mailto:asvitkine@chromium.org to the reviewers to check the histograms.xml file. > > https://codereview.chromium.org/1197853005/diff/120001/chrome/browser/content... > File chrome/browser/content_settings/permission_context_uma_util.cc (right): > > https://codereview.chromium.org/1197853005/diff/120001/chrome/browser/content... > chrome/browser/content_settings/permission_context_uma_util.cc:160: > NOTREACHED(); > This NOTREACHED will, well, never be reached :-) The NOTREACHED should be before > the return statement above it. If possible, remove the return statement > entirely, although I'm not sure that will compile. > > https://codereview.chromium.org/1197853005/diff/120001/content/child/backgrou... > File content/child/background_sync/background_sync_provider.cc (right): > > https://codereview.chromium.org/1197853005/diff/120001/content/child/backgrou... > content/child/background_sync/background_sync_provider.cc:278: break; > Shouldn't this be a NOTREACHED? > > https://codereview.chromium.org/1197853005/diff/120001/content/child/permissi... > File content/child/permissions/permission_dispatcher.cc (right): > > https://codereview.chromium.org/1197853005/diff/120001/content/child/permissi... > content/child/permissions/permission_dispatcher.cc:60: break; > Again, shouldn't this be a NOTREACHED (and of course add a comment to all these > functions specifying that PERMISSION_STATUS_NUM should never be passed in) > > https://codereview.chromium.org/1197853005/diff/120001/tools/metrics/histogra... > File tools/metrics/histograms/histograms.xml (right): > > https://codereview.chromium.org/1197853005/diff/120001/tools/metrics/histogra... > tools/metrics/histograms/histograms.xml:63782: +<enum name="PermissionStatus" > type="int"> > Shouldn't this be far below in the <enums> section? Also, make sure you run the > histograms.xml tools to pretty print and verify the file. To be clear, you'll need a bunch of other OWNER approvals as well, not just asvitkine@chromium.org.
https://codereview.chromium.org/1197853005/diff/120001/chrome/browser/content... File chrome/browser/content_settings/permission_context_uma_util.cc (right): https://codereview.chromium.org/1197853005/diff/120001/chrome/browser/content... chrome/browser/content_settings/permission_context_uma_util.cc:160: NOTREACHED(); On 2015/07/07 18:41:46, jww wrote: > This NOTREACHED will, well, never be reached :-) The NOTREACHED should be before > the return statement above it. If possible, remove the return statement > entirely, although I'm not sure that will compile. oh, derp. done. https://codereview.chromium.org/1197853005/diff/120001/content/child/backgrou... File content/child/background_sync/background_sync_provider.cc (right): https://codereview.chromium.org/1197853005/diff/120001/content/child/backgrou... content/child/background_sync/background_sync_provider.cc:278: break; On 2015/07/07 18:41:46, jww wrote: > Shouldn't this be a NOTREACHED? Done. https://codereview.chromium.org/1197853005/diff/120001/content/child/permissi... File content/child/permissions/permission_dispatcher.cc (right): https://codereview.chromium.org/1197853005/diff/120001/content/child/permissi... content/child/permissions/permission_dispatcher.cc:60: break; On 2015/07/07 18:41:46, jww wrote: > Again, shouldn't this be a NOTREACHED (and of course add a comment to all these > functions specifying that PERMISSION_STATUS_NUM should never be passed in) Done. https://codereview.chromium.org/1197853005/diff/120001/tools/metrics/histogra... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/1197853005/diff/120001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:63782: +<enum name="PermissionStatus" type="int"> On 2015/07/07 18:41:46, jww wrote: > Shouldn't this be far below in the <enums> section? Also, make sure you run the > histograms.xml tools to pretty print and verify the file. i was confused for a second and double checked, but the enum is indeed in the enums section. it just didn't look like it because this viewer does not mark elisions well.
https://codereview.chromium.org/1197853005/diff/140001/content/child/backgrou... File content/child/background_sync/background_sync_provider.cc (right): https://codereview.chromium.org/1197853005/diff/140001/content/child/backgrou... content/child/background_sync/background_sync_provider.cc:278: // PERMISSION_STATUS_NUM should never be passed into this function. See my comment about this comment in permission_dispatcher.cc. https://codereview.chromium.org/1197853005/diff/140001/content/child/permissi... File content/child/permissions/permission_dispatcher.cc (right): https://codereview.chromium.org/1197853005/diff/140001/content/child/permissi... content/child/permissions/permission_dispatcher.cc:60: // PERMISSION_STATUS_NUM should never be passed into this function. It appears you didn't add a NOTREACHED into the switch() statement. Additionally, please remove the comment here in the background_sync_provider; this is self evident. What I would like to see is a comment in the header file or at least before the function definition specifying what values the PermissionStatus arguments can take (namely, it cannot be PERMISSION_STATUS_NUM). The idea is that this is the API developers will use, and it should be clear when they look at the definition what they can and cannot put in as a value.
https://codereview.chromium.org/1197853005/diff/140001/content/public/common/... File content/public/common/permission_status.mojom (right): https://codereview.chromium.org/1197853005/diff/140001/content/public/common/... content/public/common/permission_status.mojom:11: NUM I'm confused but maybe I'm missing something. Why are you adding this here?
https://codereview.chromium.org/1197853005/diff/140001/content/child/backgrou... File content/child/background_sync/background_sync_provider.cc (right): https://codereview.chromium.org/1197853005/diff/140001/content/child/backgrou... content/child/background_sync/background_sync_provider.cc:278: // PERMISSION_STATUS_NUM should never be passed into this function. On 2015/07/07 21:14:23, jww wrote: > See my comment about this comment in permission_dispatcher.cc. Done. https://codereview.chromium.org/1197853005/diff/140001/content/child/permissi... File content/child/permissions/permission_dispatcher.cc (right): https://codereview.chromium.org/1197853005/diff/140001/content/child/permissi... content/child/permissions/permission_dispatcher.cc:60: // PERMISSION_STATUS_NUM should never be passed into this function. On 2015/07/07 21:14:23, jww wrote: > It appears you didn't add a NOTREACHED into the switch() statement. > > Additionally, please remove the comment here in the background_sync_provider; > this is self evident. What I would like to see is a comment in the header file > or at least before the function definition specifying what values the > PermissionStatus arguments can take (namely, it cannot be > PERMISSION_STATUS_NUM). The idea is that this is the API developers will use, > and it should be clear when they look at the definition what they can and cannot > put in as a value. sort of done. (this function is local to this file.) https://codereview.chromium.org/1197853005/diff/140001/content/public/common/... File content/public/common/permission_status.mojom (right): https://codereview.chromium.org/1197853005/diff/140001/content/public/common/... content/public/common/permission_status.mojom:11: NUM On 2015/07/07 21:18:47, Mounir Lamouri wrote: > I'm confused but maybe I'm missing something. Why are you adding this here? UMA_HISTOGRAM_ENUMERATION needs a boundary value. correct me if there's a better way.
https://codereview.chromium.org/1197853005/diff/140001/content/public/common/... File content/public/common/permission_status.mojom (right): https://codereview.chromium.org/1197853005/diff/140001/content/public/common/... content/public/common/permission_status.mojom:11: NUM On 2015/07/08 at 00:37:21, keenanb wrote: > On 2015/07/07 21:18:47, Mounir Lamouri wrote: > > I'm confused but maybe I'm missing something. Why are you adding this here? > > UMA_HISTOGRAM_ENUMERATION needs a boundary value. correct me if there's a better way. Can you do: LAST = ASK, instead of NUM. Then, you could pass LAST + 1. I think it will prevent the new entry in switch statements.
https://codereview.chromium.org/1197853005/diff/160001/content/child/backgrou... File content/child/background_sync/background_sync_provider.cc (right): https://codereview.chromium.org/1197853005/diff/160001/content/child/backgrou... content/child/background_sync/background_sync_provider.cc:257: void BackgroundSyncProvider::GetPermissionStatusCallback( Sorry to beat on a dead horse here, but can you put a comment above here about the values |status| is allowed to take? I know this isn't visible outside the file, but if anyone comes and modifies your code later and uses this function, it would be good to make the API as clear as possibly.
https://codereview.chromium.org/1197853005/diff/160001/content/child/backgrou... File content/child/background_sync/background_sync_provider.cc (right): https://codereview.chromium.org/1197853005/diff/160001/content/child/backgrou... content/child/background_sync/background_sync_provider.cc:257: void BackgroundSyncProvider::GetPermissionStatusCallback( On 2015/07/08 15:53:58, jww wrote: > Sorry to beat on a dead horse here, but can you put a comment above here about > the values |status| is allowed to take? I know this isn't visible outside the > file, but if anyone comes and modifies your code later and uses this function, > it would be good to make the API as clear as possibly. (Although if you make mlamouri's change, then I think all of these comments can disappear completely, right?)
i took mlamouri's advice. https://codereview.chromium.org/1197853005/diff/160001/content/child/backgrou... File content/child/background_sync/background_sync_provider.cc (right): https://codereview.chromium.org/1197853005/diff/160001/content/child/backgrou... content/child/background_sync/background_sync_provider.cc:257: void BackgroundSyncProvider::GetPermissionStatusCallback( On 2015/07/08 15:55:09, jww wrote: > On 2015/07/08 15:53:58, jww wrote: > > Sorry to beat on a dead horse here, but can you put a comment above here about > > the values |status| is allowed to take? I know this isn't visible outside the > > file, but if anyone comes and modifies your code later and uses this function, > > it would be good to make the API as clear as possibly. > > (Although if you make mlamouri's change, then I think all of these comments can > disappear completely, right?) yeah. i've disappeared them in celebration.
still lgtm. Note that you will need to rebase your CL because the files have moved to chrome/browser/permissions/. Also, you still need to ask for an histograms.xml review, right?
On 2015/07/08 22:55:21, Mounir Lamouri wrote: > still lgtm. > > Note that you will need to rebase your CL because the files have moved to > chrome/browser/permissions/. Also, you still need to ask for an histograms.xml > review, right? yeah. tomorrow, i will add several owners, with jww's help, after i rebase.
keenanb@google.com changed reviewers: + asvitkine@chromium.org, tsepez@chromium.org
tsepez, i made a tiny change to content/public/common/permission_status.mojom. could you take a look? asvitkine, could you make sure my changes to tools/metrics/histograms/histograms.xml are correct?
https://codereview.chromium.org/1197853005/diff/200001/chrome/browser/permiss... File chrome/browser/permissions/permission_context_uma_util.cc (right): https://codereview.chromium.org/1197853005/diff/200001/chrome/browser/permiss... chrome/browser/permissions/permission_context_uma_util.cc:226: "Permissions.Requested.CrossOrigin_" + PermissionTypeToString(type), This is not correct. The macros cache the histogram object at the call site, so if this is hit with different name params, it will be logged incorrectly. You can either use the API directly (i.e. by inlining what the macro expands to) or use a separate macro for each suffix. https://codereview.chromium.org/1197853005/diff/200001/tools/metrics/histogra... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/1197853005/diff/200001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:28369: +<histogram name="Permissions.Requested.SameOrigin" enum="PermissionType"> Seems like Permissions. is a new top-level prefix. Can you instead find an existing prefix to nest under?
https://codereview.chromium.org/1197853005/diff/200001/chrome/browser/permiss... File chrome/browser/permissions/permission_context_uma_util.cc (right): https://codereview.chromium.org/1197853005/diff/200001/chrome/browser/permiss... chrome/browser/permissions/permission_context_uma_util.cc:226: "Permissions.Requested.CrossOrigin_" + PermissionTypeToString(type), On 2015/07/09 17:42:03, Alexei Svitkine wrote: > This is not correct. The macros cache the histogram object at the call site, so > if this is hit with different name params, it will be logged incorrectly. You > can either use the API directly (i.e. by inlining what the macro expands to) or > use a separate macro for each suffix. keenan, look at how https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/int... does it for reference
https://codereview.chromium.org/1197853005/diff/200001/chrome/browser/permiss... File chrome/browser/permissions/permission_context_uma_util.cc (right): https://codereview.chromium.org/1197853005/diff/200001/chrome/browser/permiss... chrome/browser/permissions/permission_context_uma_util.cc:226: "Permissions.Requested.CrossOrigin_" + PermissionTypeToString(type), On 2015/07/09 17:42:03, Alexei Svitkine wrote: > This is not correct. The macros cache the histogram object at the call site, so > if this is hit with different name params, it will be logged incorrectly. You > can either use the API directly (i.e. by inlining what the macro expands to) or > use a separate macro for each suffix. Done. https://codereview.chromium.org/1197853005/diff/200001/tools/metrics/histogra... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/1197853005/diff/200001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:28369: +<histogram name="Permissions.Requested.SameOrigin" enum="PermissionType"> On 2015/07/09 17:42:03, Alexei Svitkine wrote: > Seems like Permissions. is a new top-level prefix. Can you instead find an > existing prefix to nest under? i was originally planning to use ContentSettings.PermissionRequested, but was advised and agreed to use Permissions.Requested. It is much more fitting and would be preferred for future stats collection anyhow. what drawbacks are there to using a new top-level prefix?
https://codereview.chromium.org/1197853005/diff/200001/chrome/browser/permiss... File chrome/browser/permissions/permission_context_uma_util.cc (right): https://codereview.chromium.org/1197853005/diff/200001/chrome/browser/permiss... chrome/browser/permissions/permission_context_uma_util.cc:226: "Permissions.Requested.CrossOrigin_" + PermissionTypeToString(type), On 2015/07/09 19:33:50, keenanb wrote: > On 2015/07/09 17:42:03, Alexei Svitkine wrote: > > This is not correct. The macros cache the histogram object at the call site, > so > > if this is hit with different name params, it will be logged incorrectly. You > > can either use the API directly (i.e. by inlining what the macro expands to) > or > > use a separate macro for each suffix. > > Done. I don't see any changes. Did you forget to upload a new patchset? https://codereview.chromium.org/1197853005/diff/200001/tools/metrics/histogra... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/1197853005/diff/200001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:28369: +<histogram name="Permissions.Requested.SameOrigin" enum="PermissionType"> On 2015/07/09 19:33:50, keenanb wrote: > On 2015/07/09 17:42:03, Alexei Svitkine wrote: > > Seems like Permissions. is a new top-level prefix. Can you instead find an > > existing prefix to nest under? > > i was originally planning to use ContentSettings.PermissionRequested, but was > advised and agreed to use Permissions.Requested. It is much more fitting and > would be preferred for future stats collection anyhow. > > what drawbacks are there to using a new top-level prefix? How many new histograms will be added with this top-level prefix? The drawbacks is cluttering top-level categories - i.e. same thing as not to have too many top-level source directories under src/. But maybe if you are planning to use this top-level category for a number of future metrics, then this is OK.
On 2015/07/09 19:51:03, Alexei Svitkine wrote: > https://codereview.chromium.org/1197853005/diff/200001/chrome/browser/permiss... > File chrome/browser/permissions/permission_context_uma_util.cc (right): > > https://codereview.chromium.org/1197853005/diff/200001/chrome/browser/permiss... > chrome/browser/permissions/permission_context_uma_util.cc:226: > "Permissions.Requested.CrossOrigin_" + PermissionTypeToString(type), > On 2015/07/09 19:33:50, keenanb wrote: > > On 2015/07/09 17:42:03, Alexei Svitkine wrote: > > > This is not correct. The macros cache the histogram object at the call site, > > so > > > if this is hit with different name params, it will be logged incorrectly. > You > > > can either use the API directly (i.e. by inlining what the macro expands to) > > or > > > use a separate macro for each suffix. > > > > Done. > > I don't see any changes. Did you forget to upload a new patchset? > > https://codereview.chromium.org/1197853005/diff/200001/tools/metrics/histogra... > File tools/metrics/histograms/histograms.xml (right): > > https://codereview.chromium.org/1197853005/diff/200001/tools/metrics/histogra... > tools/metrics/histograms/histograms.xml:28369: +<histogram > name="Permissions.Requested.SameOrigin" enum="PermissionType"> > On 2015/07/09 19:33:50, keenanb wrote: > > On 2015/07/09 17:42:03, Alexei Svitkine wrote: > > > Seems like Permissions. is a new top-level prefix. Can you instead find an > > > existing prefix to nest under? > > > > i was originally planning to use ContentSettings.PermissionRequested, but was > > advised and agreed to use Permissions.Requested. It is much more fitting and > > would be preferred for future stats collection anyhow. > > > > what drawbacks are there to using a new top-level prefix? > > How many new histograms will be added with this top-level prefix? The drawbacks > is cluttering top-level categories - i.e. same thing as not to have too many > top-level source directories under src/. But maybe if you are planning to use > this top-level category for a number of future metrics, then this is OK. oh, whoops. patch set uploaded now. OK. i'll talk with my team and see how likely we are to use "Permissions." for additional metrics and then make a decision.
lgtm
palmer@chromium.org changed reviewers: + palmer@chromium.org
LGTM.
creis@chromium.org changed reviewers: + creis@chromium.org
content/ LGTM
The CQ bit was checked by keenanb@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from jww@chromium.org, mlamouri@chromium.org Link to the patchset: https://codereview.chromium.org/1197853005/#ps220001 (title: "Fixed histogram name chaching issue.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1197853005/220001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
The CQ bit was checked by jww@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1197853005/220001
On 2015/07/09 22:46:13, commit-bot: I haz the power wrote: > CQ is trying da patch. Follow status at > https://chromium-cq-status.appspot.com/patch-status/1197853005/220001 tsepez: FYI, based on the palmer and creis reviews, I TBR'd you and clicked "commit" since your lgtm is technically necessary.
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_chromeos_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) (exceeded global retry quota)
https://codereview.chromium.org/1197853005/diff/240001/chrome/browser/permiss... File chrome/browser/permissions/permission_context_uma_util.cc (right): https://codereview.chromium.org/1197853005/diff/240001/chrome/browser/permiss... chrome/browser/permissions/permission_context_uma_util.cc:224: if (manager) { nit: Change this to if (!manager) return; so it's clear that this is an exceptional case. Also, please remove the comment since it is self evident. Otherwise, still lgtm.
The CQ bit was checked by jww@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from mlamouri@chromium.org, asvitkine@chromium.org, palmer@chromium.org, creis@chromium.org, jww@chromium.org Link to the patchset: https://codereview.chromium.org/1197853005/#ps280001 (title: "Just rebased.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1197853005/280001
Message was sent while issue was closed.
Committed patchset #15 (id:280001)
Message was sent while issue was closed.
Patchset 15 (id:??) landed as https://crrev.com/edebda0b1b4feb0cc6f3d4a665a58efcc9f42d22 Cr-Commit-Position: refs/heads/master@{#338373} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
