Dashboard refactor: Move dashboard specific history related features to History.
Created attachment 196009 [details] Patch
Comment on attachment 196009 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=196009&action=review > Tools/TestResultServer/static-dashboards/aggregate_results.js:49 > + this.dashboardSpecificState[key] = value == 'true'; Ack. This is kind of gross. How about passing in the history object as the first argument to this function? > Tools/TestResultServer/static-dashboards/aggregate_results.js:68 > +var g_history = new history.History(aggregateResultsConfig); > +g_history.parseCrossDashboardParameters(); Do we want to eventually not have a g_history global at all? i.e. just pass it to any functions that need it? If so, you could add the FIXMEs now. > Tools/TestResultServer/static-dashboards/flakiness_dashboard.js:141 > + if (this.crossDashboardState.useTestData) ditto > Tools/TestResultServer/static-dashboards/flakiness_dashboard.js:242 > +// Sets the page state to regenerate the page. I realize you're just copying this comment, but this is no longer accurate. > Tools/TestResultServer/static-dashboards/flakiness_dashboard.js:253 > + for (var currentKey in this.dashboardSpecificState) { > + if (isInvalidKeyForCrossBuilderView(currentKey)) { > + delete this.dashboardSpecificState[currentKey]; > + } > + } I realize you're just copying this comment, but this is no longer accurate. > Tools/TestResultServer/static-dashboards/history.js:109 > +history.History = function(dashboardSpecificConfiguration) I think you can just call this argument "configuration". > Tools/TestResultServer/static-dashboards/history.js:118 > + this._DB_SPECIFIC_INVALIDATING_PARAMETERS = dashboardSpecificConfiguration.dbSpecificInvalidatingParameters; DB? db? Also, this is not a constant, so should be camel-case. > Tools/TestResultServer/static-dashboards/history.js:119 > + this.generatePage = dashboardSpecificConfiguration.generatePageCallback; This can be private? > Tools/TestResultServer/static-dashboards/history.js:125 > +// Map of parameter to other parameter it invalidates. This comment doesn't really add value. May as well delete while you're moving the code. > Tools/TestResultServer/static-dashboards/treemap.js:90 > + generatePageCallback: generatePage, How about just calling this generatePage? You don't explicitly make the other ones have "callback" in the name. > Tools/TestResultServer/static-dashboards/treemap.js:93 > + dbSpecificInvalidatingParameters: DB_SPECIFIC_INVALIDATING_PARAMETERS How about just calling this invalidatingHashParameters
Created attachment 196059 [details] Most review feedback complete.
Comment on attachment 196009 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=196009&action=review I still need to address the "this" object issues, but wanted to reply to the other comments first. >> Tools/TestResultServer/static-dashboards/aggregate_results.js:68 >> +g_history.parseCrossDashboardParameters(); > > Do we want to eventually not have a g_history global at all? i.e. just pass it to any functions that need it? If so, you could add the FIXMEs now. Yeah. Added FIXMEs. >> Tools/TestResultServer/static-dashboards/flakiness_dashboard.js:242 >> +// Sets the page state to regenerate the page. > > I realize you're just copying this comment, but this is no longer accurate. removed >> Tools/TestResultServer/static-dashboards/flakiness_dashboard.js:253 >> + } > > I realize you're just copying this comment, but this is no longer accurate. I'm not sure what line this is in reference to? There isn't a comment here. >> Tools/TestResultServer/static-dashboards/history.js:109 >> +history.History = function(dashboardSpecificConfiguration) > > I think you can just call this argument "configuration". Done. >> Tools/TestResultServer/static-dashboards/history.js:118 >> + this._DB_SPECIFIC_INVALIDATING_PARAMETERS = dashboardSpecificConfiguration.dbSpecificInvalidatingParameters; > > DB? db? > > Also, this is not a constant, so should be camel-case. It is a constant. The name was just copied from before, but fixed to spell out dashboard. >> Tools/TestResultServer/static-dashboards/history.js:125 >> +// Map of parameter to other parameter it invalidates. > > This comment doesn't really add value. May as well delete while you're moving the code. Done. >> Tools/TestResultServer/static-dashboards/treemap.js:90 >> + generatePageCallback: generatePage, > > How about just calling this generatePage? You don't explicitly make the other ones have "callback" in the name. Done. >> Tools/TestResultServer/static-dashboards/treemap.js:93 >> + dbSpecificInvalidatingParameters: DB_SPECIFIC_INVALIDATING_PARAMETERS > > How about just calling this invalidatingHashParameters Because there are cross dashboard ones too?
Comment on attachment 196009 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=196009&action=review >>> Tools/TestResultServer/static-dashboards/flakiness_dashboard.js:253 >>> + } >> >> I realize you're just copying this comment, but this is no longer accurate. > > I'm not sure what line this is in reference to? There isn't a comment here. Not sure what happened here. >>> Tools/TestResultServer/static-dashboards/history.js:118 >>> + this._DB_SPECIFIC_INVALIDATING_PARAMETERS = dashboardSpecificConfiguration.dbSpecificInvalidatingParameters; >> >> DB? db? >> >> Also, this is not a constant, so should be camel-case. > > It is a constant. > > The name was just copied from before, but fixed to spell out dashboard. It's not a constant...you're setting it. By that sense, wouldn't all of these be constants? >>> Tools/TestResultServer/static-dashboards/treemap.js:93 >>> + dbSpecificInvalidatingParameters: DB_SPECIFIC_INVALIDATING_PARAMETERS >> >> How about just calling this invalidatingHashParameters > > Because there are cross dashboard ones too? I guess...you can't set the cross dashboard ones though...that's all private to History.
Comment on attachment 196009 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=196009&action=review >> Tools/TestResultServer/static-dashboards/aggregate_results.js:49 >> + this.dashboardSpecificState[key] = value == 'true'; > > Ack. This is kind of gross. How about passing in the history object as the first argument to this function? You weren't supposed to notice that :) Changed to pass history obj around, here and in all other similar cases. >>>> Tools/TestResultServer/static-dashboards/history.js:118 >>>> + this._DB_SPECIFIC_INVALIDATING_PARAMETERS = dashboardSpecificConfiguration.dbSpecificInvalidatingParameters; >>> >>> DB? db? >>> >>> Also, this is not a constant, so should be camel-case. >> >> It is a constant. >> >> The name was just copied from before, but fixed to spell out dashboard. > > It's not a constant...you're setting it. By that sense, wouldn't all of these be constants? I don't know what I was thinking. It was constant before, but clearly isn't now. Fixed. >> Tools/TestResultServer/static-dashboards/history.js:119 >> + this.generatePage = dashboardSpecificConfiguration.generatePageCallback; > > This can be private? Indeed it can. Good catch. >>>> Tools/TestResultServer/static-dashboards/treemap.js:93 >>>> + dbSpecificInvalidatingParameters: DB_SPECIFIC_INVALIDATING_PARAMETERS >>> >>> How about just calling this invalidatingHashParameters >> >> Because there are cross dashboard ones too? > > I guess...you can't set the cross dashboard ones though...that's all private to History. Ok. Sure, changed.
Created attachment 196161 [details] Finished review feedback
Comment on attachment 196161 [details] Finished review feedback Clearing flags on attachment: 196161 Committed r147457: <http://trac.webkit.org/changeset/147457>
All reviewed patches have been landed. Closing bug.