RESOLVED FIXED 176970
Should not mark a platform as missing in summary page if all expecting metrics are exlucded. 
https://bugs.webkit.org/show_bug.cgi?id=176970
Summary Should not mark a platform as missing in summary page if all expecting metric...
dewei_zhu
Reported 2017-09-14 17:30:41 PDT
Should not mark a platform as missing in summary page if all expecting metrics are exlucded.
Attachments
Patch (2.63 KB, patch)
2017-09-14 17:34 PDT, dewei_zhu 		no flags
DetailsFormatted DiffDiff
Patch for landing (2.88 KB, patch)
2017-09-15 17:28 PDT, dewei_zhu 		no flags
DetailsFormatted DiffDiff
Show Obsolete (1) View All Add attachment proposed patch, testcase, etc.
dewei_zhu
Comment 1 2017-09-14 17:34:40 PDT
Created attachment 320853 [details] Patch
Ryosuke Niwa
Comment 2 2017-09-14 20:26:59 PDT
Comment on attachment 320853 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=320853&action=review > Websites/perf.webkit.org/public/v3/pages/summary-page.js:265 > var foundInSomeMetric = false; > + var excludedMerticCount = 0; Use let. > Websites/perf.webkit.org/public/v3/pages/summary-page.js:281 > - if (!foundInSomeMetric) > + if (!foundInSomeMetric && excludedMerticCount < metrics.length) Now we don't need foundInSomeMetric. You can just check excludedMerticCount < metrics.length since that condition also implies metrics.length.
dewei_zhu
Comment 3 2017-09-15 00:53:22 PDT
Comment on attachment 320853 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=320853&action=review >> Websites/perf.webkit.org/public/v3/pages/summary-page.js:265 >> + var excludedMerticCount = 0; > > Use let. Should I replace all usages of 'var' in this function? >> Websites/perf.webkit.org/public/v3/pages/summary-page.js:281 >> + if (!foundInSomeMetric && excludedMerticCount < metrics.length) > > Now we don't need foundInSomeMetric. You can just check excludedMerticCount < metrics.length > since that condition also implies metrics.length. I don't think we can do this. excludedConfigurations[platform.id()] just specifies the expected missing combinations for a platform. So it could be a subset of metrics. In this case, removing foundInSomeMetric could result in marking a platform missing incorrectly.
dewei_zhu
Comment 4 2017-09-15 00:54:28 PDT
Comment on attachment 320853 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=320853&action=review >>> Websites/perf.webkit.org/public/v3/pages/summary-page.js:281 >>> + if (!foundInSomeMetric && excludedMerticCount < metrics.length) >> >> Now we don't need foundInSomeMetric. You can just check excludedMerticCount < metrics.length >> since that condition also implies metrics.length. > > I don't think we can do this. > excludedConfigurations[platform.id()] just specifies the expected missing combinations for a platform. So it could be a subset of metrics. In this case, removing foundInSomeMetric could result in marking a platform missing incorrectly. For 'excludedConfigurations[platform.id()] just specifies the expected missing combinations for a platform' I meant 'excludedConfigurations[platform.id()] just specifies the expected missing Metrics for a platform.'
dewei_zhu
Comment 5 2017-09-15 17:28:08 PDT
Created attachment 320977 [details] Patch for landing
WebKit Commit Bot
Comment 6 2017-09-15 18:11:31 PDT
Comment on attachment 320977 [details] Patch for landing Clearing flags on attachment: 320977 Committed r222123: <http://trac.webkit.org/changeset/222123>
WebKit Commit Bot
Comment 7 2017-09-15 18:11:32 PDT
All reviewed patches have been landed. Closing bug.
Radar WebKit Bug Importer
Comment 8 2017-09-27 12:28:10 PDT
<rdar://problem/34693333>
Note You need to log in before you can comment on or make changes to this bug.