Bug 149608 - Dashboard metrics has JS errors from toggle button patch
Summary: Dashboard metrics has JS errors from toggle button patch
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Dean Johnson
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2015-09-28 15:46 PDT by Dean Johnson
Modified: 2015-09-28 21:31 PDT (History)
1 user (show)

See Also:

Attachments
Patch (10.54 KB, patch)
2015-09-28 15:51 PDT, Dean Johnson 		no flags Details | Formatted Diff | Diff
Patch (11.68 KB, patch)
2015-09-28 16:12 PDT, Dean Johnson 		no flags Details | Formatted Diff | Diff
Show Obsolete (1) 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 Dean Johnson 2015-09-28 15:46:36 PDT 
The Dashboard metrics page has JS errors that were introduced with https://bugs.webkit.org/show_bug.cgi?id=148403. This patch fixes them and also moves some of the identical logic from dashboard/Scripts/Main.js and dashboard/Scripts/MetricsMain.js to dashboard/Scripts/Settings.js.
Comment 1 Dean Johnson 2015-09-28 15:51:39 PDT 
Created attachment 262025 [details]
Patch
Comment 2 Alexey Proskuryakov 2015-09-28 15:55:56 PDT 
Comment on attachment 262025 [details]
Patch

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

> Tools/BuildSlaveSupport/build.webkit.org-config/public_html/dashboard/Scripts/MetricsMain.js:346
> +        var settingsWrapper = document.createElement("div");

Eventually we should refactor this to have less copy/pasted code.

> Tools/BuildSlaveSupport/build.webkit.org-config/public_html/dashboard/Scripts/Settings.js:91
> +        if (platformName)
> +            return platformName.substr(0, platformName.indexOf("-"));
> +        return ''

Coding style nit: we prefer early return, so this should be

if (!platformName)
    return ''
return platformName.substr(0, platformName.indexOf("-"));
Comment 3 Dean Johnson 2015-09-28 16:12:40 PDT 
Created attachment 262026 [details]
Patch
Comment 4 Dean Johnson 2015-09-28 16:13:21 PDT 
(In reply to comment #2)
> Comment on attachment 262025 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=262025&action=review
> 
> > Tools/BuildSlaveSupport/build.webkit.org-config/public_html/dashboard/Scripts/MetricsMain.js:346
> > +        var settingsWrapper = document.createElement("div");
> 
> Eventually we should refactor this to have less copy/pasted code.
> 
Agreed.

> > Tools/BuildSlaveSupport/build.webkit.org-config/public_html/dashboard/Scripts/Settings.js:91
> > +        if (platformName)
> > +            return platformName.substr(0, platformName.indexOf("-"));
> > +        return ''
> 
> Coding style nit: we prefer early return, so this should be
> 
> if (!platformName)
>     return ''
> return platformName.substr(0, platformName.indexOf("-"));
Fixed in latest patch.
Comment 5 WebKit Commit Bot 2015-09-28 21:31:41 PDT 
Comment on attachment 262026 [details]
Patch

Clearing flags on attachment: 262026

Committed r190303: <http://trac.webkit.org/changeset/190303>
Comment 6 WebKit Commit Bot 2015-09-28 21:31:43 PDT 
All reviewed patches have been landed.  Closing bug.