[webkit-reviews] review denied: [Bug 113717] Dashboard refactor: Move dashboard specific history related features to History. : [Attachment 196009] Patch

[bugzilla-daemon at webkit.org]

Ojan Vafai <ojan at chromium.org> has denied Julie Parent <jparent at google.com>'s
request for review:
Bug 113717: Dashboard refactor: Move dashboard specific history related
features to History.
https://bugs.webkit.org/show_bug.cgi?id=113717

Attachment 196009: Patch
https://bugs.webkit.org/attachment.cgi?id=196009&action=review

------- Additional Comments from Ojan Vafai <ojan at chromium.org>
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