Bug 111785 - Dashboard cleanup: Create ui.Errors
Summary: Dashboard cleanup: Create ui.Errors
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Julie Parent
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2013-03-07 15:36 PST by Julie Parent
Modified: 2013-03-11 18:09 PDT (History)
5 users (show)

See Also:

Attachments
Patch (8.35 KB, patch)
2013-03-07 15:52 PST, Julie Parent 		no flags Details | Formatted Diff | Diff
Removed some dead code, this is ready for review. (8.22 KB, patch)
2013-03-07 16:58 PST, Julie Parent 		no flags Details | Formatted Diff | Diff
Patch for landing (8.06 KB, patch)
2013-03-11 14:40 PDT, Julie Parent 		no flags Details | Formatted Diff | Diff
Show Obsolete (2) View All Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
Description Julie Parent 2013-03-07 15:36:18 PST 
Dashboard cleanup: Create ui.Errors
Comment 1 Julie Parent 2013-03-07 15:52:39 PST 
Created attachment 192099 [details]
Patch
Comment 2 Julie Parent 2013-03-07 16:58:44 PST 
Created attachment 192110 [details]
Removed some dead code, this is ready for review.
Comment 3 Ojan Vafai 2013-03-11 13:55:57 PDT 
Comment on attachment 192110 [details]
Removed some dead code, this is ready for review.

View in context: https://bugs.webkit.org/attachment.cgi?id=192110&action=review

Just a bunch of style nits.

> Tools/TestResultServer/static-dashboards/ui.js:201
> +    // String of error messages to display to the user.

This comments aren't terribly useful.

> Tools/TestResultServer/static-dashboards/ui.js:202
> +    this._errorMsgs = '';

s/Msgs/Messages

or, even better, this._messages?

> Tools/TestResultServer/static-dashboards/ui.js:204
> +    this._errorElt = null;

s/Elt/Element

or, even better, this._containerElement?

> Tools/TestResultServer/static-dashboards/ui.js:208
> +    // If there are errors, show big and red UI for errors so as to be noticed.

This comment doesn't really add value.

> Tools/TestResultServer/static-dashboards/ui.js:219
> +

Nit: extra newline

> Tools/TestResultServer/static-dashboards/ui.js:222
> +    // Record a new error message.
> +    // @param {string} errorMsg The message to show to the user.

I know this comment was in the old code, but it's pretty useless.

> Tools/TestResultServer/static-dashboards/ui.js:223
> +    addError: function(errorMsg)

s/Msg/Message...or, even better, just "message"
Comment 4 Julie Parent 2013-03-11 14:40:15 PDT 
Created attachment 192562 [details]
Patch for landing
Comment 5 Julie Parent 2013-03-11 14:40:29 PDT 
Comment on attachment 192110 [details]
Removed some dead code, this is ready for review.

View in context: https://bugs.webkit.org/attachment.cgi?id=192110&action=review

>> Tools/TestResultServer/static-dashboards/ui.js:201
>> +    // String of error messages to display to the user.
> 
> This comments aren't terribly useful.

removed

>> Tools/TestResultServer/static-dashboards/ui.js:202
>> +    this._errorMsgs = '';
> 
> s/Msgs/Messages
> 
> or, even better, this._messages?

done

>> Tools/TestResultServer/static-dashboards/ui.js:204
>> +    this._errorElt = null;
> 
> s/Elt/Element
> 
> or, even better, this._containerElement?

done

>> Tools/TestResultServer/static-dashboards/ui.js:208
>> +    // If there are errors, show big and red UI for errors so as to be noticed.
> 
> This comment doesn't really add value.

removed

>> Tools/TestResultServer/static-dashboards/ui.js:219
>> +
> 
> Nit: extra newline

removed

>> Tools/TestResultServer/static-dashboards/ui.js:222
>> +    // @param {string} errorMsg The message to show to the user.
> 
> I know this comment was in the old code, but it's pretty useless.

removed

>> Tools/TestResultServer/static-dashboards/ui.js:223
>> +    addError: function(errorMsg)
> 
> s/Msg/Message...or, even better, just "message"

done
Comment 6 WebKit Review Bot 2013-03-11 18:09:21 PDT 
Comment on attachment 192562 [details]
Patch for landing

Clearing flags on attachment: 192562

Committed r145447: <http://trac.webkit.org/changeset/145447>
Comment 7 WebKit Review Bot 2013-03-11 18:09:25 PDT 
All reviewed patches have been landed.  Closing bug.