Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2437863005:
Remove PLT.* histograms. (Closed)
|
Created:
4 years, 2 months ago by Bryan McQuade Modified:
4 years, 1 month ago CC:
chromium-apps-reviews_chromium.org, chromium-reviews, creis+watch_chromium.org, darin-cc_chromium.org, extensions-reviews_chromium.org, jam, mlamouri+watch-content_chromium.org, nasko+codewatch_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionRemove PLT.* histograms.
This change removes logging for all remaining PLT.* histograms.
PLT.* histograms have been replaced by PageLoad.* histograms.
PLT.* histograms have been marked as obsolete for several months,
and have not been used in the histogram dashboard during that time
period. Additionally, many PLT.* histograms are known to report
erroneous time values (see crbug.com/615781 for details).
Code that was only used by page_load_histograms.h/.cc is removed
in the following subsequent changes:
* https://codereview.chromium.org/2448543002
* https://codereview.chromium.org/2449553002
* https://codereview.chromium.org/2446533003
* https://codereview.chromium.org/2442213003
BUG=384330
Committed: https://crrev.com/0d26ebb35d19b768b4fc03defbc9c22d5d7a2c7d
Cr-Commit-Position: refs/heads/master@{#429250}
Patch Set 1 #Patch Set 2 : remove dead test #Patch Set 3 : restore undead code #Patch Set 4 : restore mistakenly removed code #Patch Set 5 : revert removal of dependent code #Patch Set 6 : fix name #Patch Set 7 : rebase #
Messages
Total messages: 50 (37 generated)
The CQ bit was checked by bmcquade@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 bmcquade@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_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 bmcquade@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...
Description was changed from ========== Remove PLT.* histograms. This change removes logging for all remaining PLT.* histograms. PLT.* histograms have been replaced by PageLoad.* equivalents. PLT.* histograms have been marked as obsolete for several months, and have not been used in the histogram dashboard during that time period. Additionally, many PLT.* histograms are known to report erroneous time values. This change also removes code that is dead as a result of removing page_load_histograms.h/.cc. BUG=657993 ========== to ========== Remove PLT.* histograms. This change removes logging for all remaining PLT.* histograms. PLT.* histograms have been replaced by PageLoad.*. PLT.* histograms have been marked as obsolete for several months, and have not been used in the histogram dashboard during that time period. Additionally, many PLT.* histograms are known to report erroneous time values. This change also removes code that is dead as a result of removing page_load_histograms.h/.cc. BUG=657993 ==========
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_...)
bmcquade@chromium.org changed reviewers: + csharrison@chromium.org
PTAL, thanks! The android tests are failing but I'm not sure it's due to my change. Will investigtate further to make sure.
Wow! This is amazing o.O Do you think some of this could be broken up into separate patches, or it is too intertwined?
On 2016/10/21 at 17:02:05, csharrison wrote: > Wow! This is amazing o.O > > Do you think some of this could be broken up into separate patches, or it is too intertwined? It could definitely be broken up & I agree that's a better way to go. I'll split it up. I wanted to make sure you thought this looked sane first. Will break it up and send out the pieces shortly. -bryan
On 2016/10/21 17:11:44, Bryan McQuade wrote: > On 2016/10/21 at 17:02:05, csharrison wrote: > > Wow! This is amazing o.O > > > > Do you think some of this could be broken up into separate patches, or it is > too intertwined? > > It could definitely be broken up & I agree that's a better way to go. I'll split > it up. I wanted to make sure you thought this looked sane first. Will break it > up and send out the pieces shortly. > > -bryan I briefly looked at the structure and it looks great. The hooks you removed look right, I just think they can be removed one by one (or bunch by bunch).
The CQ bit was checked by bmcquade@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...
Description was changed from ========== Remove PLT.* histograms. This change removes logging for all remaining PLT.* histograms. PLT.* histograms have been replaced by PageLoad.*. PLT.* histograms have been marked as obsolete for several months, and have not been used in the histogram dashboard during that time period. Additionally, many PLT.* histograms are known to report erroneous time values. This change also removes code that is dead as a result of removing page_load_histograms.h/.cc. BUG=657993 ========== to ========== Remove PLT.* histograms. This change removes logging for all remaining PLT.* histograms. PLT.* histograms have been replaced by PageLoad.*. PLT.* histograms have been marked as obsolete for several months, and have not been used in the histogram dashboard during that time period. Additionally, many PLT.* histograms are known to report erroneous time values. This change also removes code that is dead as a result of removing page_load_histograms.h/.cc. BUG=384330 ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by bmcquade@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...
Description was changed from ========== Remove PLT.* histograms. This change removes logging for all remaining PLT.* histograms. PLT.* histograms have been replaced by PageLoad.*. PLT.* histograms have been marked as obsolete for several months, and have not been used in the histogram dashboard during that time period. Additionally, many PLT.* histograms are known to report erroneous time values. This change also removes code that is dead as a result of removing page_load_histograms.h/.cc. BUG=384330 ========== to ========== Remove PLT.* histograms. This change removes logging for all remaining PLT.* histograms. PLT.* histograms have been replaced by PageLoad.*. PLT.* histograms have been marked as obsolete for several months, and have not been used in the histogram dashboard during that time period. Additionally, many PLT.* histograms are known to report erroneous time values. Code that was only used by page_load_histograms.h/.cc is removed in the following subsequent changes: * https://codereview.chromium.org/2448543002 * https://codereview.chromium.org/2449553002 * https://codereview.chromium.org/2446533003 * https://codereview.chromium.org/2442213003 BUG=384330 ==========
On 2016/10/21 at 17:14:38, csharrison wrote: > On 2016/10/21 17:11:44, Bryan McQuade wrote: > > On 2016/10/21 at 17:02:05, csharrison wrote: > > > Wow! This is amazing o.O > > > > > > Do you think some of this could be broken up into separate patches, or it is > > too intertwined? > > > > It could definitely be broken up & I agree that's a better way to go. I'll split > > it up. I wanted to make sure you thought this looked sane first. Will break it > > up and send out the pieces shortly. > > > > -bryan > > I briefly looked at the structure and it looks great. The hooks you removed look right, I just think they can be removed one by one (or bunch by bunch). Great, thanks! I factored the dead code removal into separate changes, which are now linked from the issue description for this bug. So this change is now ready for review. I don't want to commit it, though, until I get confirmation from the extensions code folks that they understand why we should remove these histograms and their extension-specific variants.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Description was changed from ========== Remove PLT.* histograms. This change removes logging for all remaining PLT.* histograms. PLT.* histograms have been replaced by PageLoad.*. PLT.* histograms have been marked as obsolete for several months, and have not been used in the histogram dashboard during that time period. Additionally, many PLT.* histograms are known to report erroneous time values. Code that was only used by page_load_histograms.h/.cc is removed in the following subsequent changes: * https://codereview.chromium.org/2448543002 * https://codereview.chromium.org/2449553002 * https://codereview.chromium.org/2446533003 * https://codereview.chromium.org/2442213003 BUG=384330 ========== to ========== Remove PLT.* histograms. This change removes logging for all remaining PLT.* histograms. PLT.* histograms have been replaced by PageLoad.* histograms. PLT.* histograms have been marked as obsolete for several months, and have not been used in the histogram dashboard during that time period. Additionally, many PLT.* histograms are known to report erroneous time values. Code that was only used by page_load_histograms.h/.cc is removed in the following subsequent changes: * https://codereview.chromium.org/2448543002 * https://codereview.chromium.org/2449553002 * https://codereview.chromium.org/2446533003 * https://codereview.chromium.org/2442213003 BUG=384330 ==========
Description was changed from ========== Remove PLT.* histograms. This change removes logging for all remaining PLT.* histograms. PLT.* histograms have been replaced by PageLoad.* histograms. PLT.* histograms have been marked as obsolete for several months, and have not been used in the histogram dashboard during that time period. Additionally, many PLT.* histograms are known to report erroneous time values. Code that was only used by page_load_histograms.h/.cc is removed in the following subsequent changes: * https://codereview.chromium.org/2448543002 * https://codereview.chromium.org/2449553002 * https://codereview.chromium.org/2446533003 * https://codereview.chromium.org/2442213003 BUG=384330 ========== to ========== Remove PLT.* histograms. This change removes logging for all remaining PLT.* histograms. PLT.* histograms have been replaced by PageLoad.* histograms. PLT.* histograms have been marked as obsolete for several months, and have not been used in the histogram dashboard during that time period. Additionally, many PLT.* histograms are known to report erroneous time values (see crbug.com/615781 for details). Code that was only used by page_load_histograms.h/.cc is removed in the following subsequent changes: * https://codereview.chromium.org/2448543002 * https://codereview.chromium.org/2449553002 * https://codereview.chromium.org/2446533003 * https://codereview.chromium.org/2442213003 BUG=384330 ==========
code LGTM
bmcquade@chromium.org changed reviewers: + rkaplow@chromium.org
+rkaplow for histograms change Rob, there aren't any histograms.xml changes here since the PLT.* metrics are all already deprecated, but I wanted to get a review on this from someone on the metrics side. Can you take a look when you have a chance? Thanks!
The CQ bit was checked by bmcquade@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: ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
lgtm awesome!
bmcquade@chromium.org changed reviewers: + jochen@chromium.org
jochen, PTAL for chrome/ changes, thanks!
The CQ bit was checked by bmcquade@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: This issue passed the CQ dry run.
lgtm
The CQ bit was checked by bmcquade@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from csharrison@chromium.org, rkaplow@chromium.org Link to the patchset: https://codereview.chromium.org/2437863005/#ps120001 (title: "rebase")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Committed patchset #7 (id:120001)
Message was sent while issue was closed.
Description was changed from ========== Remove PLT.* histograms. This change removes logging for all remaining PLT.* histograms. PLT.* histograms have been replaced by PageLoad.* histograms. PLT.* histograms have been marked as obsolete for several months, and have not been used in the histogram dashboard during that time period. Additionally, many PLT.* histograms are known to report erroneous time values (see crbug.com/615781 for details). Code that was only used by page_load_histograms.h/.cc is removed in the following subsequent changes: * https://codereview.chromium.org/2448543002 * https://codereview.chromium.org/2449553002 * https://codereview.chromium.org/2446533003 * https://codereview.chromium.org/2442213003 BUG=384330 ========== to ========== Remove PLT.* histograms. This change removes logging for all remaining PLT.* histograms. PLT.* histograms have been replaced by PageLoad.* histograms. PLT.* histograms have been marked as obsolete for several months, and have not been used in the histogram dashboard during that time period. Additionally, many PLT.* histograms are known to report erroneous time values (see crbug.com/615781 for details). Code that was only used by page_load_histograms.h/.cc is removed in the following subsequent changes: * https://codereview.chromium.org/2448543002 * https://codereview.chromium.org/2449553002 * https://codereview.chromium.org/2446533003 * https://codereview.chromium.org/2442213003 BUG=384330 Committed: https://crrev.com/0d26ebb35d19b768b4fc03defbc9c22d5d7a2c7d Cr-Commit-Position: refs/heads/master@{#429250} ==========
Message was sent while issue was closed.
Patchset 7 (id:??) landed as https://crrev.com/0d26ebb35d19b768b4fc03defbc9c22d5d7a2c7d Cr-Commit-Position: refs/heads/master@{#429250} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
