Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2796283005:
Adding UseCounter specific for extensions (Closed)
DescriptionAdding UseCounter specific for extensions
Replaced Blink.UseCounter.Features by Blink.UseCounter.Extensions.Features when
URL protocol is chrome-extension://
BUG=687169
Review-Url: https://codereview.chromium.org/2796283005
Cr-Commit-Position: refs/heads/master@{#473409}
Committed: https://chromium.googlesource.com/chromium/src/+/fc32d83f891bf73c8ce299d6813baf812907a8c8
Patch Set 1 #
Total comments: 1
Patch Set 2 : Codereview: added extension context and disabled context #Patch Set 3 : update after the great name change #Patch Set 4 : bug fix #
Total comments: 9
Patch Set 5 : Codereview: nit + refactored UseCounterTests unaffected_histograms => all_histograms / affected_his… #
Total comments: 10
Patch Set 6 : Codereview: added tester ExpectTotalCountExcept to simplify UseCounter unit tests #
Total comments: 27
Patch Set 7 : Codereview: nit #Patch Set 8 : Codereview: added Getter for bucket count in histogram tester #Patch Set 9 : Fix compile error #
Total comments: 2
Patch Set 10 : Codereview: nit #Patch Set 11 : Nit #Patch Set 12 : Bud fix: when context is extension, CSSHistograms are never created #Patch Set 13 : Rebase #Patch Set 14 : Fix compile err #Messages
Total messages: 110 (73 generated)
lunalu@chromium.org changed reviewers: + rbyers@chromium.org
Hi Rick, PTAL
Thanks Luna. It doesn't look like this is complete. You've updated the PageVisits bucket (denominator), but you also need some code for the normal ::count case. Perhaps it's simplest to change what the featuresHistogram() method returns depending on the context (like it already does for SVG) to ensure you handle all the cases consistently. You should test the behavior manually by using chrome://histograms/Blink.UseCounter to see the current results (IIRC you may have to hit reload to ensure the data is up-to-date - let me know if it doesn't work reliably, some things have change here and there is a more direct but involved way of dumping the raw histograms from the renderer process). You should try loading some extension pages and verify you see the right histograms getting updated. You'll also need to write a test (see UseCounterTest.cpp). Big picture, we've learned the hard way with metrics that it pays to be extra careful in validating the behavior of changes (both manually and with some automated testing). We need to be sure that we can trust what our metrics are telling us (since there's no real way to debug them in the field). https://codereview.chromium.org/2796283005/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/frame/UseCounter.cpp (right): https://codereview.chromium.org/2796283005/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/frame/UseCounter.cpp:1178: if (url.protocolIs("chrome-extension")) I think m_disableReporting will still be true in this case, right?
The CQ bit was checked by lunalu@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by lunalu@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by lunalu@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 lunalu@chromium.org to run a CQ dry run
Patchset #4 (id:60001) has been deleted
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by lunalu@chromium.org to run a CQ dry run
Patchset #4 (id:80001) has been deleted
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Hi Rick! Thanks for the comments. I manually tested this with lighthouse extension, and the new histogram is behaving as expected. Please take another look at this CL. Cheers
LGTM with nits https://codereview.chromium.org/2796283005/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/frame/UseCounter.cpp (right): https://codereview.chromium.org/2796283005/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/frame/UseCounter.cpp:1171: else if (!SchemeRegistry::ShouldTrackUsageMetricsForScheme(url.Protocol())) nit: avoid the double negative by removing the '!' and swapping the two context values below. https://codereview.chromium.org/2796283005/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/frame/UseCounter.cpp:1353: DEFINE_STATIC_LOCAL(blink::EnumerationHistogram, histogram, Nit: Add DCHECKs to verify we don't have kExtensionsContext or kDisabledContext here and in animated CSS case. https://codereview.chromium.org/2796283005/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/frame/UseCounter.h (right): https://codereview.chromium.org/2796283005/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/frame/UseCounter.h:1669: // The scope represented by this UseCounter instance. Nit: can you add a comment saying that this must be fixed for the duration of a page, but can change when a new page is loaded. "by this instance" want originally meant to imply it was fixed (I should have used const to make that clear of course). https://codereview.chromium.org/2796283005/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/frame/UseCounterTest.cpp (right): https://codereview.chromium.org/2796283005/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/frame/UseCounterTest.cpp:19: const char* const kExtensionFeaturesHistogramName = nit: please add this to the unaffected_histograms list for other tests. I know it's getting a little hard to read - maybe we could simplify things by having a "all_histograms" list that includes everything and verify only the expected ones change (rather than listed all the unaffected ones for each test?). https://codereview.chromium.org/2796283005/diff/100001/tools/metrics/histogra... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2796283005/diff/100001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:5287: + Count of how many page loads use various features for extension pages only. nit: Say explicitly "pages with a chrome-extension://" URL to eliminate any possible ambiguity around what an "extension page" is.
The CQ bit was checked by lunalu@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...
Hi Rick, Thanks for your comments. I made the change as you suggested. Could you please take a look at UseCounterTest.cpp where I added all_histograms and replaced unaffected_histograms by all_histograms / affected_histograms? Thanks https://codereview.chromium.org/2796283005/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/frame/UseCounter.cpp (right): https://codereview.chromium.org/2796283005/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/frame/UseCounter.cpp:1171: else if (!SchemeRegistry::ShouldTrackUsageMetricsForScheme(url.Protocol())) On 2017/04/28 at 15:04:40, Rick Byers wrote: > nit: avoid the double negative by removing the '!' and swapping the two context values below. Done https://codereview.chromium.org/2796283005/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/frame/UseCounter.cpp:1353: DEFINE_STATIC_LOCAL(blink::EnumerationHistogram, histogram, On 2017/04/28 15:04:40, Rick Byers wrote: > Nit: Add DCHECKs to verify we don't have kExtensionsContext or kDisabledContext > here and in animated CSS case. Done. https://codereview.chromium.org/2796283005/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/frame/UseCounter.h (right): https://codereview.chromium.org/2796283005/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/frame/UseCounter.h:1669: // The scope represented by this UseCounter instance. On 2017/04/28 at 15:04:40, Rick Byers wrote: > Nit: can you add a comment saying that this must be fixed for the duration of a page, but can change when a new page is loaded. > "by this instance" want originally meant to imply it was fixed (I should have used const to make that clear of course). Done. Thanks https://codereview.chromium.org/2796283005/diff/100001/tools/metrics/histogra... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2796283005/diff/100001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:5287: + Count of how many page loads use various features for extension pages only. On 2017/04/28 at 15:04:40, Rick Byers wrote: > nit: Say explicitly "pages with a chrome-extension://" URL to eliminate any possible ambiguity around what an "extension page" is. Done
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Thanks for the cleanup Luna! Just trying to see if we can simplify this further: https://codereview.chromium.org/2796283005/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/frame/UseCounterTest.cpp (right): https://codereview.chromium.org/2796283005/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/frame/UseCounterTest.cpp:46: const std::vector<std::string>& affected_histograms, I was thinking that the only affected histograms would be the two above, and so you wouldn't need to list out the others at all... But I guess that was naive. Some ideas below. https://codereview.chromium.org/2796283005/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/frame/UseCounterTest.cpp:123: std::set_difference(all_histograms.begin(), all_histograms.end(), can you also remove "histogram" and "legacy_histogram" from the unaffected set so that they don't need to be explicitly listed as "affected"? https://codereview.chromium.org/2796283005/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/frame/UseCounterTest.cpp:134: {kAnimatedCSSHistogramName, kCSSHistogramName, kFeaturesHistogramName}, Why does this pass without kLegacyFeaturesHistogramName being listed explicitly? It's affected too, right? https://codereview.chromium.org/2796283005/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/frame/UseCounterTest.cpp:134: {kAnimatedCSSHistogramName, kCSSHistogramName, kFeaturesHistogramName}, I assume you need to specify the CSS histograms here because just their page visits bucket gets bumped by the 'didCommitLoad', right? That's a shame. It would be nice to validate that they're otherwise unmodified. If that's the only case, then perhaps we could take a list of histograms which should have only a "page visits" bucket? To do that we'd have to know which bucket is the page visits one, but it should be easy to replace the final argument to the function with a single "getPageVisitsBucketforHistogram" function which can return the bucket number given any histogram name (basically just return 1 if 'CSS' is in the name, and kPageVisits otherwise). WDYT? https://codereview.chromium.org/2796283005/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/frame/UseCounterTest.cpp:171: kSVGFeaturesHistogramName}, Is kSVGFeaturesHistogramName really affected? I don't think it should be.
Hi Rick, Thank you for the comments. I will make the changes accordingly. But before that, would you mind to answer my question upon the "getPageVisitsBucketforHistogram" comment? Thanks https://codereview.chromium.org/2796283005/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/frame/UseCounterTest.cpp (right): https://codereview.chromium.org/2796283005/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/frame/UseCounterTest.cpp:46: const std::vector<std::string>& affected_histograms, On 2017/05/01 21:19:20, Rick Byers wrote: > I was thinking that the only affected histograms would be the two above, and so > you wouldn't need to list out the others at all... But I guess that was naive. > Some ideas below. That was my initial thought. Although everything failed. So I tried to do the set difference thing. https://codereview.chromium.org/2796283005/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/frame/UseCounterTest.cpp:123: std::set_difference(all_histograms.begin(), all_histograms.end(), On 2017/05/01 21:19:20, Rick Byers wrote: > can you also remove "histogram" and "legacy_histogram" from the unaffected set > so that they don't need to be explicitly listed as "affected"? Yes. I thought about that. But I decided explicitly listing it would be more clear of what we were trying to do. But I guess that was unnecessary. https://codereview.chromium.org/2796283005/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/frame/UseCounterTest.cpp:134: {kAnimatedCSSHistogramName, kCSSHistogramName, kFeaturesHistogramName}, On 2017/05/01 21:19:20, Rick Byers wrote: > Why does this pass without kLegacyFeaturesHistogramName being listed explicitly? > It's affected too, right? I didn't include the legacy histograms in all_histograms. I guess I should have. https://codereview.chromium.org/2796283005/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/frame/UseCounterTest.cpp:134: {kAnimatedCSSHistogramName, kCSSHistogramName, kFeaturesHistogramName}, On 2017/05/01 21:19:20, Rick Byers wrote: > I assume you need to specify the CSS histograms here because just their page > visits bucket gets bumped by the 'didCommitLoad', right? That's a shame. It > would be nice to validate that they're otherwise unmodified. > > If that's the only case, then perhaps we could take a list of histograms which > should have only a "page visits" bucket? To do that we'd have to know which > bucket is the page visits one, but it should be easy to replace the final > argument to the function with a single "getPageVisitsBucketforHistogram" > function which can return the bucket number given any histogram name (basically > just return 1 if 'CSS' is in the name, and kPageVisits otherwise). WDYT? That makes sense. I assume "getPageVisitsBucketforHistogram" would be only used for testing? Also, so far, only used for CSS testing? Do we expect more things like CSS would happen in the future? Cause otherwise it just seems like we are using one trick for one particular test. But I don't know. https://codereview.chromium.org/2796283005/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/frame/UseCounterTest.cpp:171: kSVGFeaturesHistogramName}, On 2017/05/01 21:19:20, Rick Byers wrote: > Is kSVGFeaturesHistogramName really affected? I don't think it should be. You are so right. My bad! I just did an exclusive set of the unaffected_histograms set for each test. Apologies for not taking many thoughts into this.
Description was changed from ========== Adding UseCounter specific for extensions Replaced Blink.UseCounter.Features by Blink.UseCounter.Extensions.Features when URL protocol is chrome-extension:// BUG=687167 ========== to ========== Adding UseCounter specific for extensions Replaced Blink.UseCounter.Features by Blink.UseCounter.Extensions.Features when URL protocol is chrome-extension:// BUG=687169 ==========
The CQ bit was checked by lunalu@chromium.org to run a CQ dry run
Done. PTAL
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
lunalu@chromium.org changed reviewers: + thakis@chromium.org
Hi thakis@, could you PTAL at:
base/test/histogram_tester*
Hi asvitkine@, could you PTAL at:
tools/metrics/histograms/histograms.xml
Thanks
lunalu@chromium.org changed reviewers: + jwd@chromium.org
Hi jwd@, could you PTAL at:
tools/metrics/histograms/histograms.xml
Thanks
thakis@chromium.org changed reviewers: + asvitkine@chromium.org
Actually +asvitkine, looks like you forgot to add him. (asvitkine: histograms.xml, and my comment below) https://codereview.chromium.org/2796283005/diff/140001/base/test/histogram_te... File base/test/histogram_tester.cc (right): https://codereview.chromium.org/2796283005/diff/140001/base/test/histogram_te... base/test/histogram_tester.cc:64: void HistogramTester::ExpectTotalCountExcept(const std::string& name, Move this below ExpectTotalCount() so order in cc matches order in h file https://codereview.chromium.org/2796283005/diff/140001/base/test/histogram_te... base/test/histogram_tester.cc:191: void HistogramTester::CheckTotalCountExcept( The new code in this file looks pretty similar to already-existing code in this file. Alexei, do you have a good idea how we could share more code here? https://codereview.chromium.org/2796283005/diff/140001/base/test/histogram_te... File base/test/histogram_tester.h (right): https://codereview.chromium.org/2796283005/diff/140001/base/test/histogram_te... base/test/histogram_tester.h:60: HistogramBase::Sample sample, s/sample/except/ https://codereview.chromium.org/2796283005/diff/140001/base/test/histogram_te... base/test/histogram_tester.h:134: HistogramBase::Sample sample, I'd s/sample/except/ here too https://codereview.chromium.org/2796283005/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/frame/UseCounterTest.cpp (right): https://codereview.chromium.org/2796283005/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/core/frame/UseCounterTest.cpp:34: const std::vector<std::string>& all_histograms = { 1) This is just a vector, why is it a reference? 2) since it's const, call it kAllHistograms 3) Global constructors aren't great. Sure, this just just a test, but you only use it in one place, why not put the definition right in front of its use? 4) You don't even need that this is a vector, you can use std::initializer_list<>, or you can just say (`for (auto histogram : { kAnimat...})`) https://codereview.chromium.org/2796283005/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/core/frame/UseCounterTest.cpp:217: // In practice SVGs always appear to be loaded with an about:blank URL The comment looks strange here. Maybe do // In practice, SVGs always appear to be loaded with an about:blank URL const char kSvgUrl; = "about:blank"; at the top of the file and then pass kSvgUrl without the comment everywhere the comment is currently used?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
https://codereview.chromium.org/2796283005/diff/140001/base/test/histogram_te... File base/test/histogram_tester.h (right): https://codereview.chromium.org/2796283005/diff/140001/base/test/histogram_te... base/test/histogram_tester.h:132: // for histogram |name|. I wonder if it wouldn't be better to just have functions to get the total counts and the counts for the buckets and do this testing in the caller? https://codereview.chromium.org/2796283005/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/frame/UseCounter.cpp (right): https://codereview.chromium.org/2796283005/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/core/frame/UseCounter.cpp:1311: DCHECK(context_ != kDisabledContext); DCHECK_NE https://codereview.chromium.org/2796283005/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/core/frame/UseCounter.cpp:1342: default: Nit: Suggest not having a default clause and explicitly handling all the cases. This way, if a new one is added, compiler will warn the CL author to update the switch. https://codereview.chromium.org/2796283005/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/frame/UseCounterTest.cpp (right): https://codereview.chromium.org/2796283005/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/core/frame/UseCounterTest.cpp:22: const char* const kSVGFeaturesHistogramName = Nit: Use const char kSVGFeaturesHistogramName[] = syntax. This is the preferred way to declare string constants (it results in more compact/efficient code iirc). Please change the other entries in this file to be consistent. https://codereview.chromium.org/2796283005/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/core/frame/UseCounterTest.cpp:40: int GetPageVisitsBucketforHistogram(std::string histogram_name) { const std::string& https://codereview.chromium.org/2796283005/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/core/frame/UseCounterTest.cpp:46: } Nit: " // namespace" Also maybe add inner empty lines within the namespace since the namespace content is now non-trivial. https://codereview.chromium.org/2796283005/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/core/frame/UseCounterTest.cpp:128: for (const auto unaffected_histogram : all_histograms) { const auto& or const std::string& Otherwise, this copies the string in each iteration to the var.
https://codereview.chromium.org/2796283005/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/frame/UseCounterTest.cpp (right): https://codereview.chromium.org/2796283005/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/core/frame/UseCounterTest.cpp:22: const char* const kSVGFeaturesHistogramName = On 2017/05/10 15:17:30, Alexei Svitkine (slow) wrote: > Nit: Use const char kSVGFeaturesHistogramName[] = syntax. > > This is the preferred way to declare string constants (it results in more > compact/efficient code iirc). Please change the other entries in this file to be > consistent. Oh, good catch. To elaborate a bit on this: const char kFoo[] = ... is a bit more efficient than const char* kFoo = ... since in that case the pointer isn't const, only the data it points to is. It's the same as const char* const kFoo = ... (which this file uses), but it's more compact and looks less strange than the double const.
Hi Nico and Alexie, Thank you for your comments. I made a few changes. Please take another look. p.s., I didn't do the refactoring in histogram_tester yet. I'd like to get more consensus before I make any change. Thanks, Luna https://codereview.chromium.org/2796283005/diff/140001/base/test/histogram_te... File base/test/histogram_tester.cc (right): https://codereview.chromium.org/2796283005/diff/140001/base/test/histogram_te... base/test/histogram_tester.cc:64: void HistogramTester::ExpectTotalCountExcept(const std::string& name, On 2017/05/09 19:36:45, Nico wrote: > Move this below ExpectTotalCount() so order in cc matches order in h file Done. https://codereview.chromium.org/2796283005/diff/140001/base/test/histogram_te... base/test/histogram_tester.cc:191: void HistogramTester::CheckTotalCountExcept( On 2017/05/09 19:36:45, Nico wrote: > The new code in this file looks pretty similar to already-existing code in this > file. Alexei, do you have a good idea how we could share more code here? IMHO, we can implement getters (privately if we don't want to expose them and call shared getters in Exepect* and Check*). WDYT? https://codereview.chromium.org/2796283005/diff/140001/base/test/histogram_te... File base/test/histogram_tester.h (right): https://codereview.chromium.org/2796283005/diff/140001/base/test/histogram_te... base/test/histogram_tester.h:60: HistogramBase::Sample sample, On 2017/05/09 19:36:46, Nico wrote: > s/sample/except/ Done. https://codereview.chromium.org/2796283005/diff/140001/base/test/histogram_te... base/test/histogram_tester.h:132: // for histogram |name|. On 2017/05/10 15:17:30, Alexei Svitkine (slow) wrote: > I wonder if it wouldn't be better to just have functions to get the total counts > and the counts for the buckets and do this testing in the caller? That was my initial thought. But after seeing that no other getters were implemented, I decided to write a tester instead (thinking that this might be what the owner would like to keep). But I could totally write 2 callers instead. https://codereview.chromium.org/2796283005/diff/140001/base/test/histogram_te... base/test/histogram_tester.h:134: HistogramBase::Sample sample, On 2017/05/09 19:36:46, Nico wrote: > I'd s/sample/except/ here too Done. https://codereview.chromium.org/2796283005/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/frame/UseCounter.cpp (right): https://codereview.chromium.org/2796283005/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/core/frame/UseCounter.cpp:1311: DCHECK(context_ != kDisabledContext); On 2017/05/10 15:17:30, Alexei Svitkine (slow) wrote: > DCHECK_NE Done. Updated a few other DCHECK's too. https://codereview.chromium.org/2796283005/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/core/frame/UseCounter.cpp:1342: default: On 2017/05/10 15:17:30, Alexei Svitkine (slow) wrote: > Nit: Suggest not having a default clause and explicitly handling all the cases. > This way, if a new one is added, compiler will warn the CL author to update the > switch. Done. https://codereview.chromium.org/2796283005/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/frame/UseCounterTest.cpp (right): https://codereview.chromium.org/2796283005/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/core/frame/UseCounterTest.cpp:22: const char* const kSVGFeaturesHistogramName = On 2017/05/10 15:38:20, Nico wrote: > On 2017/05/10 15:17:30, Alexei Svitkine (slow) wrote: > > Nit: Use const char kSVGFeaturesHistogramName[] = syntax. > > > > This is the preferred way to declare string constants (it results in more > > compact/efficient code iirc). Please change the other entries in this file to > be > > consistent. > > Oh, good catch. To elaborate a bit on this: > > const char kFoo[] = ... > > is a bit more efficient than > > const char* kFoo = ... > > since in that case the pointer isn't const, only the data it points to is. It's > the same as > > const char* const kFoo = ... > > (which this file uses), but it's more compact and looks less strange than the > double const. Makes sense! Thanks for the insight =) Although if in the case of multiple identical strings are defined. Compiler is required to provide separate copies of arrays for const kFoo[], but not for pointer variables. But we are not dealing with the case above. So I will make it more code efficient as suggested :) https://codereview.chromium.org/2796283005/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/core/frame/UseCounterTest.cpp:34: const std::vector<std::string>& all_histograms = { On 2017/05/09 19:36:46, Nico wrote: > 1) This is just a vector, why is it a reference? > 2) since it's const, call it kAllHistograms > 3) Global constructors aren't great. Sure, this just just a test, but you only > use it in one place, why not put the definition right in front of its use? > 4) You don't even need that this is a vector, you can use > std::initializer_list<>, or you can just say (`for (auto histogram : { > kAnimat...})`) I see. Thanks! https://codereview.chromium.org/2796283005/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/core/frame/UseCounterTest.cpp:46: } On 2017/05/10 15:17:30, Alexei Svitkine (slow) wrote: > Nit: " // namespace" > > Also maybe add inner empty lines within the namespace since the namespace > content is now non-trivial. Sorry my bad. https://codereview.chromium.org/2796283005/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/core/frame/UseCounterTest.cpp:128: for (const auto unaffected_histogram : all_histograms) { On 2017/05/10 15:17:30, Alexei Svitkine (slow) wrote: > const auto& or const std::string& > > Otherwise, this copies the string in each iteration to the var. Done. https://codereview.chromium.org/2796283005/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/core/frame/UseCounterTest.cpp:217: // In practice SVGs always appear to be loaded with an about:blank URL On 2017/05/09 19:36:46, Nico wrote: > The comment looks strange here. Maybe do > > // In practice, SVGs always appear to be loaded with an about:blank URL > const char kSvgUrl; = "about:blank"; > > at the top of the file and then pass kSvgUrl without the comment everywhere the > comment is currently used? Done. I did the same for HttpsUrl and ExtensionUrl (both referenced more than once).
The CQ bit was checked by lunalu@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/2796283005/diff/140001/base/test/histogram_te... File base/test/histogram_tester.h (right): https://codereview.chromium.org/2796283005/diff/140001/base/test/histogram_te... base/test/histogram_tester.h:132: // for histogram |name|. On 2017/05/10 19:39:38, loonybear wrote: > On 2017/05/10 15:17:30, Alexei Svitkine (slow) wrote: > > I wonder if it wouldn't be better to just have functions to get the total > counts > > and the counts for the buckets and do this testing in the caller? > > That was my initial thought. But after seeing that no other getters were > implemented, I decided to write a tester instead (thinking that this might be > what the owner would like to keep). But I could totally write 2 callers instead. I agree that it would be different with how the class is currently designed, but I think it would be a positive change. The current design comes with some downsides - specifically if you have multiple calls to this, you won't know which one failed since the file/line number you'll get is from within the helper class and not the test. I think it's fine to introduce getters in this CL and do the tests outside the class - sure it will create an inconsistency but I think it's better than adding very custom Check functions that are only used from a single place.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...) linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
Done. Please take another look. Thanks!
The CQ bit was checked by lunalu@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 lunalu@chromium.org to run a CQ dry run
Patchset #8 (id:180001) has been deleted
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...) linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
I removed default case in UseCounter::FeaturesHistogram as suggested. But now I am encountering this compile error: Not all control paths return a value for win and android bots. Should I add a return histogram at the end? But doesn't that defeat the purpose of not having default case? Thanks
I removed default case in UseCounter::FeaturesHistogram as suggested. But now I am encountering this compile error: Not all control paths return a value for win and android bots. Should I add a return histogram at the end? But doesn't that defeat the purpose of not having default case? Thanks
Please ignore my previous note. I figured it out.
The CQ bit was checked by lunalu@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
This looks great, thank you for the simplification to the tests! LGTM with nit https://codereview.chromium.org/2796283005/diff/220001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/frame/UseCounterTest.cpp (right): https://codereview.chromium.org/2796283005/diff/220001/third_party/WebKit/Sou... third_party/WebKit/Source/core/frame/UseCounterTest.cpp:139: // b. The value of the "page visits" bucket, for "CSS" histograms. nit: This comment seems slightly wrong. Rather than being about "non-CSS" and "CSS" isn't it really about histograms that are for the same or different context than the one under test? Eg. when testing "Features", "CSSFeatures" will have a total count equal to pageVisits, but "SVGImage.CSSFeatures" will have a total count of 0 (since there no SVG images loaded at all).
lgtm
lgtm
Hi thakis, Could you please owner approve base/test/histogram_tester.cc base/test/histogram_tester.h ? Thanks https://codereview.chromium.org/2796283005/diff/220001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/frame/UseCounterTest.cpp (right): https://codereview.chromium.org/2796283005/diff/220001/third_party/WebKit/Sou... third_party/WebKit/Source/core/frame/UseCounterTest.cpp:139: // b. The value of the "page visits" bucket, for "CSS" histograms. On 2017/05/12 17:51:21, Rick Byers wrote: > nit: This comment seems slightly wrong. Rather than being about "non-CSS" and > "CSS" isn't it really about histograms that are for the same or different > context than the one under test? > > Eg. when testing "Features", "CSSFeatures" will have a total count equal to > pageVisits, but "SVGImage.CSSFeatures" will have a total count of 0 (since there > no SVG images loaded at all). Done.
The CQ bit was checked by lunalu@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by lunalu@chromium.org to run a CQ dry run
Patchset #12 (id:280001) has been deleted
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
lgtm
The CQ bit was checked by lunalu@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from asvitkine@chromium.org, jwd@chromium.org, rbyers@chromium.org Link to the patchset: https://codereview.chromium.org/2796283005/#ps300001 (title: "Bud fix: when context is extension, CSSHistograms are never created")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by lunalu@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from thakis@chromium.org, asvitkine@chromium.org, jwd@chromium.org, rbyers@chromium.org Link to the patchset: https://codereview.chromium.org/2796283005/#ps320001 (title: "Rebase")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by lunalu@chromium.org
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by lunalu@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from thakis@chromium.org, asvitkine@chromium.org, jwd@chromium.org, rbyers@chromium.org Link to the patchset: https://codereview.chromium.org/2796283005/#ps340001 (title: "Fix compile err")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
On 2017/05/19 15:02:44, commit-bot: I haz the power wrote: > Try jobs failed on following builders: > chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, > http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...) You'll need to remove tmp.txt.
Patchset #14 (id:340001) has been deleted
The CQ bit was checked by lunalu@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from thakis@chromium.org, asvitkine@chromium.org, jwd@chromium.org, rbyers@chromium.org Link to the patchset: https://codereview.chromium.org/2796283005/#ps360001 (title: "Fix compile err")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
The CQ bit was checked by lunalu@chromium.org
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 360001, "attempt_start_ts": 1495250881521910,
"parent_rev": "d07d1d29fc7cda26a10b4d1283d1af86aae9a755", "commit_rev":
"fc32d83f891bf73c8ce299d6813baf812907a8c8"}
Message was sent while issue was closed.
Description was changed from ========== Adding UseCounter specific for extensions Replaced Blink.UseCounter.Features by Blink.UseCounter.Extensions.Features when URL protocol is chrome-extension:// BUG=687169 ========== to ========== Adding UseCounter specific for extensions Replaced Blink.UseCounter.Features by Blink.UseCounter.Extensions.Features when URL protocol is chrome-extension:// BUG=687169 Review-Url: https://codereview.chromium.org/2796283005 Cr-Commit-Position: refs/heads/master@{#473409} Committed: https://chromium.googlesource.com/chromium/src/+/fc32d83f891bf73c8ce299d6813b... ==========
Message was sent while issue was closed.
Committed patchset #14 (id:360001) as https://chromium.googlesource.com/chromium/src/+/fc32d83f891bf73c8ce299d6813b... |
