Bug 113717 – Dashboard refactor: Move dashboard specific history related features to History.

• Status: RESOLVED
• Resolution: FIXED
• Component: Tools / Tests

[Julie Parent]

Dashboard refactor: Move dashboard specific history related features to History.

[Julie Parent]

Created attachment 196009 [details]
Patch

[Ojan Vafai]

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

[Julie Parent]

Created attachment 196059 [details]
Most review feedback complete.

[Julie Parent]

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?

[Ojan Vafai]

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.

[Julie Parent]

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.

[Julie Parent]

Created attachment 196161 [details]
Finished review feedback

[WebKit Review Bot]

Comment on attachment 196161 [details]
Finished review feedback

Clearing flags on attachment: 196161

Committed r147457: <http://trac.webkit.org/changeset/147457>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.