Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2206343002:
Add metrics for YouTube Flash embed rewrite. (Closed)
DescriptionAdd metrics for YouTube Flash embed rewrite.
BUG=625985
Committed: https://crrev.com/af8d257f40cc928ec3a251b2341a323c661d4af5
Cr-Commit-Position: refs/heads/master@{#410722}
Patch Set 1 #
Total comments: 6
Patch Set 2 : Addressed comments #Patch Set 3 : Addressed comments #
Total comments: 6
Patch Set 4 : Addressed comments #Patch Set 5 : rebase #Patch Set 6 : rebase, again #
Total comments: 8
Patch Set 7 : #Patch Set 8 : #
Total comments: 4
Patch Set 9 : Addressed comments #
Total comments: 6
Patch Set 10 : Addressed comments #
Total comments: 10
Patch Set 11 : Addressed comments #
Total comments: 12
Patch Set 12 : Addressed comments #
Total comments: 6
Patch Set 13 : Addressed comment #
Depends on Patchset: Dependent Patchsets: Messages
Total messages: 76 (54 generated)
The CQ bit was checked by kdsilva@google.com 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: This issue passed the CQ dry run.
kdsilva@google.com changed reviewers: + mouniuri@chromium.org
Description was changed from ========== Add metrics for YouTube Flash embed rewrite. BUG= ========== to ========== Add metrics for YouTube Flash embed rewrite. BUG=625985 ==========
kdsilva@google.com changed reviewers: + mlamouri@chromium.org - mouniuri@chromium.org
mlamouri@chromium.org changed reviewers: + asvitkine@chromium.org
lgtm, +asvitkine@ for histograms review. Note that one histogram value will be used on Chrome Android where enablejsapi=1 will still trigger a success. https://codereview.chromium.org/2206343002/diff/1/chrome/renderer/chrome_cont... File chrome/renderer/chrome_content_renderer_client.cc (right): https://codereview.chromium.org/2206343002/diff/1/chrome/renderer/chrome_cont... chrome/renderer/chrome_content_renderer_client.cc:193: NUM_PLUGIN_ERROR = 4 Can you add a comment that says that this one should be kept at the end? Also, don't add "= 4" so it will automatically increment based on the previous value. https://codereview.chromium.org/2206343002/diff/1/chrome/renderer/chrome_cont... chrome/renderer/chrome_content_renderer_client.cc:196: const char kFlashYouTubeRewrite[] = "Plugin.Flash.YouTubeRewrite"; Maybe name this `kFlashYouTubeRewriteUma`? https://codereview.chromium.org/2206343002/diff/1/chrome/renderer/chrome_cont... File chrome/renderer/chrome_content_renderer_client_unittest.cc (right): https://codereview.chromium.org/2206343002/diff/1/chrome/renderer/chrome_cont... chrome/renderer/chrome_content_renderer_client_unittest.cc:55: static const char* YOUTUBE_REWRITE_STATUS = "Plugin.Flash.YouTubeRewrite"; You don't need `static`, you are inside an anonymous namespace.
Addressed comments and ran `git cl format` which changed the formatting slightly. https://codereview.chromium.org/2206343002/diff/1/chrome/renderer/chrome_cont... File chrome/renderer/chrome_content_renderer_client.cc (right): https://codereview.chromium.org/2206343002/diff/1/chrome/renderer/chrome_cont... chrome/renderer/chrome_content_renderer_client.cc:193: NUM_PLUGIN_ERROR = 4 On 2016/08/04 at 12:42:02, Mounir Lamouri wrote: > Can you add a comment that says that this one should be kept at the end? Also, don't add "= 4" so it will automatically increment based on the previous value. Done. https://codereview.chromium.org/2206343002/diff/1/chrome/renderer/chrome_cont... chrome/renderer/chrome_content_renderer_client.cc:196: const char kFlashYouTubeRewrite[] = "Plugin.Flash.YouTubeRewrite"; On 2016/08/04 at 12:42:02, Mounir Lamouri wrote: > Maybe name this `kFlashYouTubeRewriteUma`? Done. https://codereview.chromium.org/2206343002/diff/1/chrome/renderer/chrome_cont... File chrome/renderer/chrome_content_renderer_client_unittest.cc (right): https://codereview.chromium.org/2206343002/diff/1/chrome/renderer/chrome_cont... chrome/renderer/chrome_content_renderer_client_unittest.cc:55: static const char* YOUTUBE_REWRITE_STATUS = "Plugin.Flash.YouTubeRewrite"; On 2016/08/04 at 12:42:02, Mounir Lamouri wrote: > You don't need `static`, you are inside an anonymous namespace. Done.
https://codereview.chromium.org/2206343002/diff/40001/chrome/renderer/chrome_... File chrome/renderer/chrome_content_renderer_client.cc (right): https://codereview.chromium.org/2206343002/diff/40001/chrome/renderer/chrome_... chrome/renderer/chrome_content_renderer_client.cc:1446: UMA_HISTOGRAM_ENUMERATION(kFlashYouTubeRewriteUMA, FAILURE_ENABLEJSAPI, UMA macros expand to a lot of code. Please avoid the duplication by making a helper function for logging this histogram. https://codereview.chromium.org/2206343002/diff/40001/chrome/renderer/chrome_... File chrome/renderer/chrome_content_renderer_client_unittest.cc (right): https://codereview.chromium.org/2206343002/diff/40001/chrome/renderer/chrome_... chrome/renderer/chrome_content_renderer_client_unittest.cc:55: const char* YOUTUBE_REWRITE_STATUS = "Plugin.Flash.YouTubeRewrite"; const char kYoutubeRewriteStatus[] = https://codereview.chromium.org/2206343002/diff/40001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2206343002/diff/40001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:42472: +<histogram name="Plugin.Flash.YouTubeRewrite" enum="YouTubeRewriteStatus"> Have you gotten approval from chrome privacy for this? This is required for site-specific UMA histograms.
The CQ bit was checked by kdsilva@google.com 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/2206343002/diff/40001/chrome/renderer/chrome_... File chrome/renderer/chrome_content_renderer_client.cc (right): https://codereview.chromium.org/2206343002/diff/40001/chrome/renderer/chrome_... chrome/renderer/chrome_content_renderer_client.cc:1446: UMA_HISTOGRAM_ENUMERATION(kFlashYouTubeRewriteUMA, FAILURE_ENABLEJSAPI, On 2016/08/04 14:31:18, Alexei Svitkine (slow) wrote: > UMA macros expand to a lot of code. Please avoid the duplication by making a > helper function for logging this histogram. Done. https://codereview.chromium.org/2206343002/diff/40001/chrome/renderer/chrome_... File chrome/renderer/chrome_content_renderer_client_unittest.cc (right): https://codereview.chromium.org/2206343002/diff/40001/chrome/renderer/chrome_... chrome/renderer/chrome_content_renderer_client_unittest.cc:55: const char* YOUTUBE_REWRITE_STATUS = "Plugin.Flash.YouTubeRewrite"; On 2016/08/04 14:31:18, Alexei Svitkine (slow) wrote: > const char kYoutubeRewriteStatus[] = Done. https://codereview.chromium.org/2206343002/diff/40001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2206343002/diff/40001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:42472: +<histogram name="Plugin.Flash.YouTubeRewrite" enum="YouTubeRewriteStatus"> On 2016/08/04 14:31:18, Alexei Svitkine (slow) wrote: > Have you gotten approval from chrome privacy for this? This is required for > site-specific UMA histograms. It will be part of the privacy review. We have details in our design doc here: https://docs.google.com/document/d/1xHBaX2WhfZVmfeWyLNOX76l9cPiXrQNXqRrUMtlvf...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...) mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
The CQ bit was checked by kdsilva@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by kdsilva@google.com 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: This issue passed the CQ dry run.
https://codereview.chromium.org/2206343002/diff/100001/chrome/renderer/chrome... File chrome/renderer/chrome_content_renderer_client.cc (right): https://codereview.chromium.org/2206343002/diff/100001/chrome/renderer/chrome... chrome/renderer/chrome_content_renderer_client.cc:1404: void ChromeContentRendererClient::UpdateUMAMetrics(const int& status, Maybe name this "RecordYouTubeRewriteUMA"? And move this inside the anonymous namespace above. https://codereview.chromium.org/2206343002/diff/100001/chrome/renderer/chrome... chrome/renderer/chrome_content_renderer_client.cc:1405: const int& max) { s/const int&/int/ https://codereview.chromium.org/2206343002/diff/100001/chrome/renderer/chrome... File chrome/renderer/chrome_content_renderer_client.h (right): https://codereview.chromium.org/2206343002/diff/100001/chrome/renderer/chrome... chrome/renderer/chrome_content_renderer_client.h:165: void UpdateUMAMetrics(const int& status, const int& max); No need to add this to the header, you can scope the method to the .cc file. https://codereview.chromium.org/2206343002/diff/100001/chrome/renderer/chrome... File chrome/renderer/chrome_content_renderer_client_unittest.cc (right): https://codereview.chromium.org/2206343002/diff/100001/chrome/renderer/chrome... chrome/renderer/chrome_content_renderer_client_unittest.cc:550: gurl = GURL("http://www.youtube.com/embed/deadbeef?enablejsapi=1"); It seems that the check you do is always the same. What about having an array of a few URLs and then loop trough them and check? Same for the tests below.
The CQ bit was checked by kdsilva@google.com 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 kdsilva@google.com 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 kdsilva@google.com 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...
Patchset #7 (id:120001) has been deleted
https://codereview.chromium.org/2206343002/diff/100001/chrome/renderer/chrome... File chrome/renderer/chrome_content_renderer_client.cc (right): https://codereview.chromium.org/2206343002/diff/100001/chrome/renderer/chrome... chrome/renderer/chrome_content_renderer_client.cc:1404: void ChromeContentRendererClient::UpdateUMAMetrics(const int& status, On 2016/08/05 12:51:09, Mounir Lamouri wrote: > Maybe name this "RecordYouTubeRewriteUMA"? > > And move this inside the anonymous namespace above. Done. https://codereview.chromium.org/2206343002/diff/100001/chrome/renderer/chrome... chrome/renderer/chrome_content_renderer_client.cc:1405: const int& max) { On 2016/08/05 12:51:09, Mounir Lamouri wrote: > s/const int&/int/ Done. https://codereview.chromium.org/2206343002/diff/100001/chrome/renderer/chrome... File chrome/renderer/chrome_content_renderer_client.h (right): https://codereview.chromium.org/2206343002/diff/100001/chrome/renderer/chrome... chrome/renderer/chrome_content_renderer_client.h:165: void UpdateUMAMetrics(const int& status, const int& max); On 2016/08/05 12:51:10, Mounir Lamouri wrote: > No need to add this to the header, you can scope the method to the .cc file. Done. https://codereview.chromium.org/2206343002/diff/100001/chrome/renderer/chrome... File chrome/renderer/chrome_content_renderer_client_unittest.cc (right): https://codereview.chromium.org/2206343002/diff/100001/chrome/renderer/chrome... chrome/renderer/chrome_content_renderer_client_unittest.cc:550: gurl = GURL("http://www.youtube.com/embed/deadbeef?enablejsapi=1"); On 2016/08/05 12:51:10, Mounir Lamouri wrote: > It seems that the check you do is always the same. What about having an array of > a few URLs and then loop trough them and check? > > Same for the tests below. Done. PTAL.
The CQ bit was checked by kdsilva@google.com 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/2206343002/diff/180001/chrome/renderer/chrome... File chrome/renderer/chrome_content_renderer_client.cc (right): https://codereview.chromium.org/2206343002/diff/180001/chrome/renderer/chrome... chrome/renderer/chrome_content_renderer_client.cc:188: enum YouTubeRewriteStatus { Add a comment mentioning it's used for UMA, so values should not be renumbered or re-used. https://codereview.chromium.org/2206343002/diff/180001/chrome/renderer/chrome... chrome/renderer/chrome_content_renderer_client.cc:196: const char kFlashYouTubeRewriteUMA[] = "Plugin.Flash.YouTubeRewrite"; You're duplicating both this and the enum above in the test. Please refactor the code to avoid this duplication. e.g. move these out of anon namespace and expose them in the header file. In fact, I would actually recommend adding a new .cc/.h file for this feature, given chrome_content_renderer_client.cc is quite large. I am okay if you do that in a follow-up CL instead. https://codereview.chromium.org/2206343002/diff/180001/chrome/renderer/chrome... chrome/renderer/chrome_content_renderer_client.cc:198: void RecordYouTubeRewriteUMA(int status, int max) { You don't need the max param, just remove it and inline it below.
https://codereview.chromium.org/2206343002/diff/160001/chrome/renderer/chrome... File chrome/renderer/chrome_content_renderer_client_unittest.cc (right): https://codereview.chromium.org/2206343002/diff/160001/chrome/renderer/chrome... chrome/renderer/chrome_content_renderer_client_unittest.cc:509: ChromeContentRendererClient client; Can you move `client` inside ChromeContentRendererClientMetricsTest and have a getter like you did for histogram_tester_? https://codereview.chromium.org/2206343002/diff/160001/chrome/renderer/chrome... chrome/renderer/chrome_content_renderer_client_unittest.cc:514: const char* test_data[] = { Why not std::string?
The CQ bit was checked by kdsilva@google.com 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/2206343002/diff/160001/chrome/renderer/chrome... File chrome/renderer/chrome_content_renderer_client_unittest.cc (right): https://codereview.chromium.org/2206343002/diff/160001/chrome/renderer/chrome... chrome/renderer/chrome_content_renderer_client_unittest.cc:509: ChromeContentRendererClient client; On 2016/08/05 14:43:50, Mounir Lamouri wrote: > Can you move `client` inside ChromeContentRendererClientMetricsTest and have a > getter like you did for histogram_tester_? Done. https://codereview.chromium.org/2206343002/diff/160001/chrome/renderer/chrome... chrome/renderer/chrome_content_renderer_client_unittest.cc:514: const char* test_data[] = { On 2016/08/05 14:43:51, Mounir Lamouri wrote: > Why not std::string? Done (in patch 9). https://codereview.chromium.org/2206343002/diff/180001/chrome/renderer/chrome... File chrome/renderer/chrome_content_renderer_client.cc (right): https://codereview.chromium.org/2206343002/diff/180001/chrome/renderer/chrome... chrome/renderer/chrome_content_renderer_client.cc:188: enum YouTubeRewriteStatus { On 2016/08/05 14:42:26, Alexei Svitkine (slow) wrote: > Add a comment mentioning it's used for UMA, so values should not be renumbered > or re-used. Done. https://codereview.chromium.org/2206343002/diff/180001/chrome/renderer/chrome... chrome/renderer/chrome_content_renderer_client.cc:196: const char kFlashYouTubeRewriteUMA[] = "Plugin.Flash.YouTubeRewrite"; On 2016/08/05 14:42:26, Alexei Svitkine (slow) wrote: > You're duplicating both this and the enum above in the test. > > Please refactor the code to avoid this duplication. e.g. move these out of anon > namespace and expose them in the header file. > > In fact, I would actually recommend adding a new .cc/.h file for this feature, > given chrome_content_renderer_client.cc is quite large. I am okay if you do that > in a follow-up CL instead. Done. https://codereview.chromium.org/2206343002/diff/180001/chrome/renderer/chrome... chrome/renderer/chrome_content_renderer_client.cc:198: void RecordYouTubeRewriteUMA(int status, int max) { On 2016/08/05 14:42:27, Alexei Svitkine (slow) wrote: > You don't need the max param, just remove it and inline it below. Done.
https://codereview.chromium.org/2206343002/diff/200001/chrome/renderer/chrome... File chrome/renderer/chrome_content_renderer_client.cc (right): https://codereview.chromium.org/2206343002/diff/200001/chrome/renderer/chrome... chrome/renderer/chrome_content_renderer_client.cc:190: void RecordYouTubeRewriteUMA(int status) { Param can be a YouTubeRewriteStatus type. https://codereview.chromium.org/2206343002/diff/200001/chrome/renderer/chrome... File chrome/renderer/chrome_content_renderer_client_unittest.cc (right): https://codereview.chromium.org/2206343002/diff/200001/chrome/renderer/chrome... chrome/renderer/chrome_content_renderer_client_unittest.cc:55: const char kYouTubeRewriteStatus[] = "Plugin.Flash.YouTubeRewrite"; Share this also, please. Declare it in the .h and define it in the .cc file. You can put it in an "internal" namespace to indicate it shouldn't be referenced elsewhere that imports the header. You can do the same for the enum. https://codereview.chromium.org/2206343002/diff/200001/chrome/renderer/chrome... chrome/renderer/chrome_content_renderer_client_unittest.cc:496: void OverrideFlashEmbed(GURL gurl) { const GURL& https://codereview.chromium.org/2206343002/diff/200001/chrome/renderer/chrome... chrome/renderer/chrome_content_renderer_client_unittest.cc:503: }; DISALLOW_COPY_AND)_ASSIGN() https://codereview.chromium.org/2206343002/diff/200001/chrome/renderer/chrome... chrome/renderer/chrome_content_renderer_client_unittest.cc:506: std::unique_ptr<base::HistogramSamples> samples = GetHistogramSamples(); Wrong indent? Run git cl format.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
The CQ bit was checked by kdsilva@google.com 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/2206343002/diff/200001/chrome/renderer/chrome... File chrome/renderer/chrome_content_renderer_client.cc (right): https://codereview.chromium.org/2206343002/diff/200001/chrome/renderer/chrome... chrome/renderer/chrome_content_renderer_client.cc:190: void RecordYouTubeRewriteUMA(int status) { On 2016/08/05 16:34:56, Alexei Svitkine (very slow) wrote: > Param can be a YouTubeRewriteStatus type. Done. https://codereview.chromium.org/2206343002/diff/200001/chrome/renderer/chrome... File chrome/renderer/chrome_content_renderer_client_unittest.cc (right): https://codereview.chromium.org/2206343002/diff/200001/chrome/renderer/chrome... chrome/renderer/chrome_content_renderer_client_unittest.cc:55: const char kYouTubeRewriteStatus[] = "Plugin.Flash.YouTubeRewrite"; On 2016/08/05 16:34:56, Alexei Svitkine (very slow) wrote: > Share this also, please. Declare it in the .h and define it in the .cc file. > > You can put it in an "internal" namespace to indicate it shouldn't be referenced > elsewhere that imports the header. You can do the same for the enum. Done. https://codereview.chromium.org/2206343002/diff/200001/chrome/renderer/chrome... chrome/renderer/chrome_content_renderer_client_unittest.cc:496: void OverrideFlashEmbed(GURL gurl) { On 2016/08/05 16:34:56, Alexei Svitkine (very slow) wrote: > const GURL& Done. https://codereview.chromium.org/2206343002/diff/200001/chrome/renderer/chrome... chrome/renderer/chrome_content_renderer_client_unittest.cc:503: }; On 2016/08/05 16:34:56, Alexei Svitkine (very slow) wrote: > DISALLOW_COPY_AND)_ASSIGN() Done. https://codereview.chromium.org/2206343002/diff/200001/chrome/renderer/chrome... chrome/renderer/chrome_content_renderer_client_unittest.cc:506: std::unique_ptr<base::HistogramSamples> samples = GetHistogramSamples(); On 2016/08/05 16:34:56, Alexei Svitkine (very slow) wrote: > Wrong indent? Run git cl format. Done.
lgtm % comments https://codereview.chromium.org/2206343002/diff/220001/chrome/renderer/chrome... File chrome/renderer/chrome_content_renderer_client.cc (right): https://codereview.chromium.org/2206343002/diff/220001/chrome/renderer/chrome... chrome/renderer/chrome_content_renderer_client.cc:188: } Nit: // namespace internal https://codereview.chromium.org/2206343002/diff/220001/chrome/renderer/chrome... File chrome/renderer/chrome_content_renderer_client.h (right): https://codereview.chromium.org/2206343002/diff/220001/chrome/renderer/chrome... chrome/renderer/chrome_content_renderer_client.h:77: namespace internal { Nit add blank lines within this. https://codereview.chromium.org/2206343002/diff/220001/chrome/renderer/chrome... chrome/renderer/chrome_content_renderer_client.h:87: } // namespace internal There should be 2 spaces before the comment. https://codereview.chromium.org/2206343002/diff/220001/chrome/renderer/chrome... File chrome/renderer/chrome_content_renderer_client_unittest.cc (right): https://codereview.chromium.org/2206343002/diff/220001/chrome/renderer/chrome... chrome/renderer/chrome_content_renderer_client_unittest.cc:523: for (auto data : test_data) { nit: const auto& or const std::string& Otherwise you're copying. Same for the other tests below. https://codereview.chromium.org/2206343002/diff/220001/tools/metrics/histogra... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2206343002/diff/220001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:95792: + <int value="0" label="SUCCESS"/> Nit: These are human readable, so you can make them more so - e.g. "Succeeded". Also, you should document what the difference is between SUCCESS and SUCESS_PARAM_REWRITE somewhere. Maybe both here and in a comment above the enum values. Here, you can document within the <int> tag - i.e. <int ...>More description</int>
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2206343002/diff/220001/chrome/renderer/chrome... File chrome/renderer/chrome_content_renderer_client.h (left): https://codereview.chromium.org/2206343002/diff/220001/chrome/renderer/chrome... chrome/renderer/chrome_content_renderer_client.h:165: nit: I assume this change wasn't intentional?
The CQ bit was checked by kdsilva@google.com 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: This issue passed the CQ dry run.
https://codereview.chromium.org/2206343002/diff/220001/chrome/renderer/chrome... File chrome/renderer/chrome_content_renderer_client.cc (right): https://codereview.chromium.org/2206343002/diff/220001/chrome/renderer/chrome... chrome/renderer/chrome_content_renderer_client.cc:188: } On 2016/08/08 11:32:34, Alexei Svitkine (very slow) wrote: > Nit: // namespace internal Done. https://codereview.chromium.org/2206343002/diff/220001/chrome/renderer/chrome... File chrome/renderer/chrome_content_renderer_client.h (left): https://codereview.chromium.org/2206343002/diff/220001/chrome/renderer/chrome... chrome/renderer/chrome_content_renderer_client.h:165: On 2016/08/08 12:40:28, Mounir Lamouri wrote: > nit: I assume this change wasn't intentional? Ack. Removed unnecessary line I had added. https://codereview.chromium.org/2206343002/diff/220001/chrome/renderer/chrome... File chrome/renderer/chrome_content_renderer_client.h (right): https://codereview.chromium.org/2206343002/diff/220001/chrome/renderer/chrome... chrome/renderer/chrome_content_renderer_client.h:77: namespace internal { On 2016/08/08 11:32:35, Alexei Svitkine (very slow) wrote: > Nit add blank lines within this. Done. https://codereview.chromium.org/2206343002/diff/220001/chrome/renderer/chrome... chrome/renderer/chrome_content_renderer_client.h:87: } // namespace internal On 2016/08/08 11:32:35, Alexei Svitkine (very slow) wrote: > There should be 2 spaces before the comment. Done. https://codereview.chromium.org/2206343002/diff/220001/chrome/renderer/chrome... File chrome/renderer/chrome_content_renderer_client_unittest.cc (right): https://codereview.chromium.org/2206343002/diff/220001/chrome/renderer/chrome... chrome/renderer/chrome_content_renderer_client_unittest.cc:523: for (auto data : test_data) { On 2016/08/08 11:32:35, Alexei Svitkine (very slow) wrote: > nit: const auto& or const std::string& > > Otherwise you're copying. Same for the other tests below. Done. https://codereview.chromium.org/2206343002/diff/220001/tools/metrics/histogra... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2206343002/diff/220001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:95792: + <int value="0" label="SUCCESS"/> On 2016/08/08 11:32:35, Alexei Svitkine (very slow) wrote: > Nit: These are human readable, so you can make them more so - e.g. "Succeeded". > > Also, you should document what the difference is between SUCCESS and > SUCESS_PARAM_REWRITE somewhere. Maybe both here and in a comment above the enum > values. > > Here, you can document within the <int> tag - i.e. <int ...>More > description</int> Done.
kdsilva@google.com changed reviewers: + jam@chromium.org
kdsilva@google.com changed required reviewers: + jam@chromium.org
jam@, could you rubber stamp this CL?
mlamouri@chromium.org changed reviewers: - jam@chromium.org
mlamouri@chromium.org changed required reviewers: - jam@chromium.org
still lgtm https://codereview.chromium.org/2206343002/diff/240001/chrome/renderer/chrome... File chrome/renderer/chrome_content_renderer_client_unittest.cc (right): https://codereview.chromium.org/2206343002/diff/240001/chrome/renderer/chrome... chrome/renderer/chrome_content_renderer_client_unittest.cc:544: "http://www.youtube.com/embed/deadbeef?enablejsapi=1"}; style: move }; to the next line https://codereview.chromium.org/2206343002/diff/240001/chrome/renderer/chrome... chrome/renderer/chrome_content_renderer_client_unittest.cc:635: "http://www.youtube.com/v/deadbeef&start=4&enablejsapi=1?foo=2"}; style: move }; to the next line. https://codereview.chromium.org/2206343002/diff/240001/tools/metrics/histogra... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2206343002/diff/240001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:95841: + Embed was rewritten even though JS API was enabled (Chrome Android only). remove "Chrome"
kdsilva@google.com changed reviewers: + jam@chromium.org
kdsilva@google.com changed required reviewers: + jam@chromium.org
https://codereview.chromium.org/2206343002/diff/240001/chrome/renderer/chrome... File chrome/renderer/chrome_content_renderer_client_unittest.cc (right): https://codereview.chromium.org/2206343002/diff/240001/chrome/renderer/chrome... chrome/renderer/chrome_content_renderer_client_unittest.cc:544: "http://www.youtube.com/embed/deadbeef?enablejsapi=1"}; On 2016/08/09 11:21:47, Mounir Lamouri wrote: > style: move }; to the next line Acknowledged. Keeping it as is because it's what `git cl format` outputs. https://codereview.chromium.org/2206343002/diff/240001/chrome/renderer/chrome... chrome/renderer/chrome_content_renderer_client_unittest.cc:635: "http://www.youtube.com/v/deadbeef&start=4&enablejsapi=1?foo=2"}; On 2016/08/09 11:21:47, Mounir Lamouri wrote: > style: move }; to the next line. Acknowledged. https://codereview.chromium.org/2206343002/diff/240001/tools/metrics/histogra... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2206343002/diff/240001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:95841: + Embed was rewritten even though JS API was enabled (Chrome Android only). On 2016/08/09 11:21:47, Mounir Lamouri wrote: > remove "Chrome" Done.
The CQ bit was checked by kdsilva@google.com 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: This issue passed the CQ dry run.
lgtm
The CQ bit was checked by kdsilva@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from asvitkine@chromium.org, mlamouri@chromium.org Link to the patchset: https://codereview.chromium.org/2206343002/#ps260001 (title: "Addressed comment")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Committed patchset #13 (id:260001)
Message was sent while issue was closed.
Description was changed from ========== Add metrics for YouTube Flash embed rewrite. BUG=625985 ========== to ========== Add metrics for YouTube Flash embed rewrite. BUG=625985 Committed: https://crrev.com/af8d257f40cc928ec3a251b2341a323c661d4af5 Cr-Commit-Position: refs/heads/master@{#410722} ==========
Message was sent while issue was closed.
Patchset 13 (id:??) landed as https://crrev.com/af8d257f40cc928ec3a251b2341a323c661d4af5 Cr-Commit-Position: refs/heads/master@{#410722} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
