Bug 148403 - Add toggle options to build-safari dashboard for hiding/showing certain device types.
Summary: Add toggle options to build-safari dashboard for hiding/showing certain devic...
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-08-24 15:44 PDT by Dean Johnson
Modified: 2015-09-17 12:42 PDT (History)
6 users (show)

See Also:


Attachments
Patch (12.50 KB, patch)
2015-08-24 15:59 PDT, Dean Johnson
no flags Details | Formatted Diff | Diff
Patch (12.50 KB, patch)
2015-08-24 16:46 PDT, Dean Johnson
no flags Details | Formatted Diff | Diff
Patch (10.94 KB, patch)
2015-08-26 13:51 PDT, Dean Johnson
no flags Details | Formatted Diff | Diff
New toggle buttons (top left) (1.25 MB, image/png)
2015-08-26 17:19 PDT, Dean Johnson
no flags Details
Patch (12.72 KB, patch)
2015-09-02 09:36 PDT, Dean Johnson
no flags Details | Formatted Diff | Diff
Patch (12.72 KB, patch)
2015-09-02 17:18 PDT, Dean Johnson
no flags Details | Formatted Diff | Diff
Patch (15.76 KB, patch)
2015-09-02 17:26 PDT, Dean Johnson
no flags Details | Formatted Diff | Diff
Patch (16.12 KB, patch)
2015-09-14 10:14 PDT, Dean Johnson
no flags Details | Formatted Diff | Diff
Patch (16.20 KB, patch)
2015-09-14 10:52 PDT, Dean Johnson
no flags Details | Formatted Diff | Diff
Patch (20.89 KB, patch)
2015-09-16 18:49 PDT, Dean Johnson
no flags Details | Formatted Diff | Diff
Patch (20.98 KB, patch)
2015-09-16 19:00 PDT, Dean Johnson
no flags Details | Formatted Diff | Diff
Patch (20.79 KB, patch)
2015-09-17 11:45 PDT, Dean Johnson
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Dean Johnson 2015-08-24 15:44:13 PDT
We should have a toggle option for showing and hiding groups of devices at a given time.
Comment 1 Dean Johnson 2015-08-24 15:59:43 PDT
Created attachment 259789 [details]
Patch
Comment 2 Dean Johnson 2015-08-24 16:46:13 PDT
Created attachment 259793 [details]
Patch
Comment 3 Dean Johnson 2015-08-26 13:51:40 PDT
Created attachment 259978 [details]
Patch
Comment 4 Alexey Proskuryakov 2015-08-26 17:04:00 PDT
Could you please attach a screenshot with how this looks on build.webkit.org now?
Comment 5 Dean Johnson 2015-08-26 17:19:05 PDT
Created attachment 260003 [details]
New toggle buttons (top left)
Comment 6 Alexey Proskuryakov 2015-08-26 20:38:03 PDT
I still think that having two different ways to control the options is not right. Why do platform labels change the color, but accessibility changes the text?
Comment 7 Dean Johnson 2015-08-27 08:50:21 PDT
I'm not sure I get what you mean. Platform labels (the individual hide buttons you mean?) change the color of the text to reflect whether any elements of a given device type are hidden. The accessibility options merely change the hue of that color.
Comment 8 Alexey Proskuryakov 2015-08-27 20:11:12 PDT
Discussed in person, Dean will update the patch.
Comment 9 Alexey Proskuryakov 2015-08-28 13:08:24 PDT
Comment on attachment 259978 [details]
Patch

Marking r- to get this patch out of the review queue.
Comment 10 Dean Johnson 2015-09-02 09:36:45 PDT
Created attachment 260421 [details]
Patch
Comment 11 David Kilzer (:ddkilzer) 2015-09-02 15:24:51 PDT
Comment on attachment 260421 [details]
Patch

r=me
Comment 12 WebKit Commit Bot 2015-09-02 16:27:53 PDT
Comment on attachment 260421 [details]
Patch

Rejecting attachment 260421 [details] from commit-queue.

Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.webkit.org', '--bot-id=webkit-cq-03', 'land-attachment', '--force-clean', '--non-interactive', '--parent-command=commit-queue', 260421, '--port=mac']" exit_code: 2 cwd: /Volumes/Data/EWS/WebKit

Last 500 characters of output:
148403.
Found no modified ChangeLogs, cannot create a commit message.
All changes require a ChangeLog.  See:
 http://webkit.org/coding/contributing.html
Found no modified ChangeLogs, cannot create a commit message.
All changes require a ChangeLog.  See:
 http://webkit.org/coding/contributing.html
Found no modified ChangeLogs, cannot create a commit message.
All changes require a ChangeLog.  See:
 http://webkit.org/coding/contributing.html
Updating OpenSource
Current branch master is up to date.

Full output: http://webkit-queues.webkit.org/results/134472
Comment 13 David Kilzer (:ddkilzer) 2015-09-02 16:29:12 PDT
Dean, do you have commit access?  Otherwise, the patch just needs a ChangeLog.
Comment 14 Dean Johnson 2015-09-02 17:18:59 PDT
Created attachment 260454 [details]
Patch
Comment 15 Dean Johnson 2015-09-02 17:21:13 PDT
Comment on attachment 260454 [details]
Patch

Changelog wasn't included.
Comment 16 Dean Johnson 2015-09-02 17:26:47 PDT
Created attachment 260455 [details]
Patch
Comment 17 Dean Johnson 2015-09-02 17:28:10 PDT
:ddkilzer - No commit access yet... still need to land some more patches. :)
Comment 18 Daniel Bates 2015-09-02 21:21:11 PDT
Comment on attachment 260455 [details]
Patch

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

> Tools/ChangeLog:4
> +

Missing Reviewed by line. Is commit-queue smart enough to add one?

> Tools/BuildSlaveSupport/build.webkit.org-config/public_html/dashboard/Scripts/Main.js:108
> +    var hiddenPlatforms = settings.getObject("hiddenPlatforms") || [];

Nit: ' (single quotes) => " (double quotes)

> Tools/BuildSlaveSupport/build.webkit.org-config/public_html/dashboard/Scripts/Main.js:113
> +    var hiddenDevices = {'all': hiddenPlatforms.length > 0}

Ditto.

> Tools/BuildSlaveSupport/build.webkit.org-config/public_html/dashboard/Scripts/Main.js:115
> +        var deviceType = hiddenPlatforms[i].split('-')[0]

We duplicate this parsing logic at least three times in this patch. Using String.split() as a way to extract the platform family name is also inefficient and will cause us to malloc a new string for each of the substrings. I suggest we extract the parsing logic into a shared function and make use of String.substr() and String.indexOf():

function parsePlatformFamily(platformName)
{
    return platformName.substr(0, platformName.indexOf("-"));
}

> Tools/BuildSlaveSupport/build.webkit.org-config/public_html/dashboard/Scripts/Main.js:276
> +        settingsWrapper.setAttribute("class", "unhide hidden settingsWrapper")

I would write this as:

settingsWrapper.className = "unhide hidden settingsWrapper";

> Tools/BuildSlaveSupport/build.webkit.org-config/public_html/dashboard/Scripts/Main.js:278
> +        optionsWrapper.setAttribute("class", "unhide hidden options-wrapper")

Similarly, I would write this as:

optionsWrapper.className = "unhide hidden options-wrapper";

Nit: "options-wrapper" ;=>   "optionsWrapper"

> Tools/BuildSlaveSupport/build.webkit.org-config/public_html/dashboard/Scripts/Main.js:300
> +        for (var deviceType in deviceTypes) {

I think "device" is a bit disingenuous, especially for platforms that begin with prefix "linux". Maybe "platform family" would be a better way to describe our categorization?

> Tools/BuildSlaveSupport/build.webkit.org-config/public_html/dashboard/Scripts/Main.js:306
> +            (function (deviceToToggle, platformsOnPage) {
> +            deviceShowToggle.addEventListener("click", function () {
> +                settings.toggleShowDevices(deviceToToggle, platformsOnPage);
> +            })}(deviceType, categorizedQueuesByPlatformAndBuildType));

Can we write this using Function.bind()?

> Tools/BuildSlaveSupport/build.webkit.org-config/public_html/dashboard/Scripts/Main.js:307
> +            deviceShowToggle.setAttribute("class", "unhide hidden deviceShowToggleButton");

I would write this as:

deviceShowToggle.className = "unhide hidden deviceShowToggleButton".

On another note, can we come up with a better name for the CSS class deviceShowToggleButton and variable deviceShowToggle? Maybe platformFamilyToggleButton and familyToggle?

> Tools/BuildSlaveSupport/build.webkit.org-config/public_html/dashboard/Scripts/Main.js:308
> +            deviceShowToggle.setAttribute("id", deviceType + "-deviceShowToggleButton");

As far as I can tell we do not make use of the HTML id attribute of this buttons. Unless you plan to make use of them in another patch then I suggest we do not assign an id to these elements.

> Tools/BuildSlaveSupport/build.webkit.org-config/public_html/dashboard/Scripts/Settings.js:115
> +                var currentState = hiddenPlatforms.indexOf(platforms[i].name) > -1 ? 'hide' : 'show';

Nit: ' (single quotes) => " (double quotes)

> Tools/BuildSlaveSupport/build.webkit.org-config/public_html/dashboard/Scripts/Settings.js:117
> +                    action = currentState == 'hide' ? 'show' : 'hide';

Ditto.

> Tools/BuildSlaveSupport/build.webkit.org-config/public_html/dashboard/Scripts/Settings.js:121
> +                if (action != currentState) {
> +                    this.toggleHiddenPlatform(platforms[i].name);
> +                }

Please remove curly braces.
Comment 19 Dean Johnson 2015-09-14 10:14:48 PDT
Created attachment 261113 [details]
Patch
Comment 20 Dean Johnson 2015-09-14 10:17:48 PDT
(In reply to comment #18)
> Comment on attachment 260455 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=260455&action=review
> 
> > Tools/ChangeLog:4
> > +
> 
> Missing Reviewed by line. Is commit-queue smart enough to add one?
> 
> > Tools/BuildSlaveSupport/build.webkit.org-config/public_html/dashboard/Scripts/Main.js:108
> > +    var hiddenPlatforms = settings.getObject("hiddenPlatforms") || [];
> 
> Nit: ' (single quotes) => " (double quotes)
> 
> > Tools/BuildSlaveSupport/build.webkit.org-config/public_html/dashboard/Scripts/Main.js:113
> > +    var hiddenDevices = {'all': hiddenPlatforms.length > 0}
> 
> Ditto.
> 
> > Tools/BuildSlaveSupport/build.webkit.org-config/public_html/dashboard/Scripts/Main.js:115
> > +        var deviceType = hiddenPlatforms[i].split('-')[0]
> 
> We duplicate this parsing logic at least three times in this patch. Using
> String.split() as a way to extract the platform family name is also
> inefficient and will cause us to malloc a new string for each of the
> substrings. I suggest we extract the parsing logic into a shared function
> and make use of String.substr() and String.indexOf():
> 
> function parsePlatformFamily(platformName)
> {
>     return platformName.substr(0, platformName.indexOf("-"));
> }
> 
> > Tools/BuildSlaveSupport/build.webkit.org-config/public_html/dashboard/Scripts/Main.js:276
> > +        settingsWrapper.setAttribute("class", "unhide hidden settingsWrapper")
> 
> I would write this as:
> 
> settingsWrapper.className = "unhide hidden settingsWrapper";
> 
> > Tools/BuildSlaveSupport/build.webkit.org-config/public_html/dashboard/Scripts/Main.js:278
> > +        optionsWrapper.setAttribute("class", "unhide hidden options-wrapper")
> 
> Similarly, I would write this as:
> 
> optionsWrapper.className = "unhide hidden options-wrapper";
> 
Although I like this more, it would be inconsistent with how we set classes elsewhere.
> Nit: "options-wrapper" ;=>   "optionsWrapper"
> 
> > Tools/BuildSlaveSupport/build.webkit.org-config/public_html/dashboard/Scripts/Main.js:300
> > +        for (var deviceType in deviceTypes) {
> 
> I think "device" is a bit disingenuous, especially for platforms that begin
> with prefix "linux". Maybe "platform family" would be a better way to
> describe our categorization?
> 
> > Tools/BuildSlaveSupport/build.webkit.org-config/public_html/dashboard/Scripts/Main.js:306
> > +            (function (deviceToToggle, platformsOnPage) {
> > +            deviceShowToggle.addEventListener("click", function () {
> > +                settings.toggleShowDevices(deviceToToggle, platformsOnPage);
> > +            })}(deviceType, categorizedQueuesByPlatformAndBuildType));
> 
> Can we write this using Function.bind()?
> 
> > Tools/BuildSlaveSupport/build.webkit.org-config/public_html/dashboard/Scripts/Main.js:307
> > +            deviceShowToggle.setAttribute("class", "unhide hidden deviceShowToggleButton");
> 
> I would write this as:
> 
> deviceShowToggle.className = "unhide hidden deviceShowToggleButton".
> 
> On another note, can we come up with a better name for the CSS class
> deviceShowToggleButton and variable deviceShowToggle? Maybe
> platformFamilyToggleButton and familyToggle?
> 
> > Tools/BuildSlaveSupport/build.webkit.org-config/public_html/dashboard/Scripts/Main.js:308
> > +            deviceShowToggle.setAttribute("id", deviceType + "-deviceShowToggleButton");
> 
> As far as I can tell we do not make use of the HTML id attribute of this
> buttons. Unless you plan to make use of them in another patch then I suggest
> we do not assign an id to these elements.
> 
> > Tools/BuildSlaveSupport/build.webkit.org-config/public_html/dashboard/Scripts/Settings.js:115
> > +                var currentState = hiddenPlatforms.indexOf(platforms[i].name) > -1 ? 'hide' : 'show';
> 
> Nit: ' (single quotes) => " (double quotes)
> 
> > Tools/BuildSlaveSupport/build.webkit.org-config/public_html/dashboard/Scripts/Settings.js:117
> > +                    action = currentState == 'hide' ? 'show' : 'hide';
> 
> Ditto.
> 
> > Tools/BuildSlaveSupport/build.webkit.org-config/public_html/dashboard/Scripts/Settings.js:121
> > +                if (action != currentState) {
> > +                    this.toggleHiddenPlatform(platforms[i].name);
> > +                }
> 
> Please remove curly braces.
Fixed the rest of the changes and nits.
Comment 21 Dean Johnson 2015-09-14 10:52:51 PDT
Created attachment 261114 [details]
Patch
Comment 22 Alexey Proskuryakov 2015-09-14 12:43:20 PDT
I tried the patch locally, the behavior looks good to me.

One nit: it's surprising that enabled state is bold, and disabled it regular+strikethrough. Why two style changes at once?
Comment 23 Dean Johnson 2015-09-14 14:16:03 PDT
(In reply to comment #22)
> I tried the patch locally, the behavior looks good to me.
> 
> One nit: it's surprising that enabled state is bold, and disabled it
> regular+strikethrough. Why two style changes at once?

I personally found that bolding the buttons made it very clear which views were enabled, whereas without my glasses, the strikethrough could be unclear.

Looks like there could be a small issue with a button displaying that all platforms for a given family are shown when they aren't, if new bots are added.

Going to refactor to fix the above issue.
Comment 24 Darin Adler 2015-09-16 10:20:14 PDT
Comment on attachment 261114 [details]
Patch

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

Change to the webpage looks OK. I’d love to review the UI rather than the patch, but I guess I can look after it’s deployed.

> Tools/ChangeLog:2
> +2015-09-14  Dean Johnson  <dean_johnson@apple.com>
> +        Added toggle options to Settings for hiding and showing certain platform families on the dashboard.

Missing blank line.

> Tools/ChangeLog:5
> +        Reviewed by MISSING!

Strange. The "OOPS" is important in these; our tool understands it and fills in the reviewer name. This non-standard one won’t work for commit queue.
Comment 25 Daniel Bates 2015-09-16 12:32:56 PDT
Comment on attachment 261114 [details]
Patch

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

r-'ing this patch because it has a correctness issue remarked in comment #23.

> Tools/BuildSlaveSupport/build.webkit.org-config/public_html/dashboard/Scripts/Main.js:302
> +        unhideAllButton.setAttribute("class", "unhide hidden platformFamilyToggleButton");

Can we use the Element.classList API here?

> Tools/BuildSlaveSupport/build.webkit.org-config/public_html/dashboard/Scripts/Main.js:312
> +            platformFamilyToggle.setAttribute("class", "unhide hidden platformFamilyToggleButton");

Ditto.

> Tools/BuildSlaveSupport/build.webkit.org-config/public_html/dashboard/Scripts/Main.js:313
> +            platformFamilyToggle.setAttribute("id", platformFamily + "-platformFamilyToggleButton");

As far as I can tell we never make use of these ids. So, I would remove them.

> Tools/BuildSlaveSupport/build.webkit.org-config/public_html/dashboard/Styles/Main.css:236
> +    tabindex: 0; /* Accessibility */

This is not valid CSS property.
Comment 26 Dean Johnson 2015-09-16 18:49:50 PDT
Created attachment 261348 [details]
Patch
Comment 27 Dean Johnson 2015-09-16 19:00:52 PDT
Created attachment 261351 [details]
Patch
Comment 28 Dean Johnson 2015-09-16 19:03:27 PDT
Most recent patch applies Dan's classList comments from comment #25. The rest of the comments were already applied, except for the 'id' comment, as 'id' is used in Scripts/Main.js:updateHiddenPlatforms.
Comment 29 Daniel Bates 2015-09-16 22:11:10 PDT
Comment on attachment 261351 [details]
Patch

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

> Tools/ChangeLog:8
> +        This patch removes "hiddenPlatforms" from use in the code and local storage. We also removed

This sentence does not read well.

Nit: "removed" => "remove"

> Tools/ChangeLog:10
> +        families! Examples of these are "mac", "ios", and "linux". "Show All Platforms" was also removed

Nit: I do not see the need to use an exclamation point. I suggest using a period.

> Tools/BuildSlaveSupport/build.webkit.org-config/public_html/dashboard/Scripts/Main.js:126
> +function initPlatformsByFamily()
> +{
> +    var platforms = Dashboard.sortedPlatforms;
> +    for (var i in platforms) {
> +        // Make sure the platform will be displayed on the page before considering its platform family.
> +        if (!categorizedQueuesByPlatformAndBuildType[platforms[i].name])
> +            continue;
> +
> +        var platformFamily = parsePlatformFamily(platforms[i].name);
> +        if (platformsByFamily[platformFamily])
> +            platformsByFamily[platformFamily].push(platforms[i].name)
> +        else
> +            platformsByFamily[platformFamily] = [platforms[i].name]
> +    }
> +}

I would have written this as:

function initializePlatformsByFamilyMap()
{
    var platforms = Dashboard.sortedPlatforms;
    for (var i in platforms) {
        var name = platforms[i].name;
        if (!categorizedQueuesByPlatformAndBuildType[name])
            continue;
        var platformFamilyName = parsePlatformFamily(name);
        var platformFamily = platformsByFamily[platformFamilyName];
        if (!platformFamily)
            platformFamily = platformsByFamily[platformFamilyName] = [];
        platformFamily.push(name);
    }
}

> Tools/BuildSlaveSupport/build.webkit.org-config/public_html/dashboard/Scripts/Main.js:128
> +function updateToggleButtons()

Maybe a better name for this function would be updatePlatformFamilySegmentedControl? or updatePlatformFamilyToolbar? or updatePlatformFamilyButtons?

> Tools/BuildSlaveSupport/build.webkit.org-config/public_html/dashboard/Scripts/Main.js:131
> +    var hiddenFamilyButtons = {"all": hiddenPlatformFamilies.length > 0};

The plurality of this variable name and the word "hidden" makes it sound like this variable represents an array of buttons that are hidden. Can we come up with a better name?

Alternatively, we could special case the handling of the unhide all button by saving or reference to the button once we find it upon traversal (below) and then update it outside of the logic for the other buttons. Or we could define a special CSS class name for the unhide all button, say unhideAllPlatformFamilyToggleButton, whose style is identical to platformFamilyToggleButton. Then add specialized logic to update the state of the unhide all button.

> Tools/BuildSlaveSupport/build.webkit.org-config/public_html/dashboard/Scripts/Main.js:136
> +    for (var i=0; i < platformFamilyButtons.length; i++) {

Nit: Missing space characters on both sides of the equal (=) sign.

Nit: This for-loop uses a post-increment operation for incrementing the step variable, i, where as the for-loop above (line 132) uses a pre-increment operation. We should pick a style for incrementing the step variable, say pre-increment, and stick with it throughout the dashboard code for consistency.

> Tools/BuildSlaveSupport/build.webkit.org-config/public_html/dashboard/Scripts/Main.js:137
> +        var hiddenPlatformFamily = parsePlatformFamily(platformFamilyButtons[i].id);

It is unnecessary to parse the family name from the id of the button given that the text content of the button is exactly the platform family name. Moreover, the proposed approach abuses the purpose of the function parsePlatformFamily() to parse the id of the button, which happens to follow a similar naming convention as a valid platform name (the characters before the first dash character '-' is the platform family name). It is more straightforward to use the text content of the element and write this line as:
 
var platformFamily = platformFamilyButtons[i].textContent;

(I intentionally renamed the variable from hiddenPlatformFamily to platformFamily. See my remark below for my reasoning).

Then we do not need to assign an id to each button.

On another note, the name of this variable, hiddenPlatformFamily, is disingenuous. This family may be visible (we will decide below). I suggest that we rename hiddenPlatformFamily to platformFamily.

> Tools/BuildSlaveSupport/build.webkit.org-config/public_html/dashboard/Scripts/Main.js:139
> +            platformFamilyButtons[i].classList.add("familyShown");

We reference platformFamilyButtons[i] three times in this for-loop. We should compute it once and cache it in a local variable near the beginning of the for-loop body and then reference the local variable throughout this for-loop.

> Tools/BuildSlaveSupport/build.webkit.org-config/public_html/dashboard/Scripts/Main.js:141
> +            platformFamilyButtons[i].classList.remove('familyShown');

Nit: ' (single quotes) => " (double quote)

> Tools/BuildSlaveSupport/build.webkit.org-config/public_html/dashboard/Scripts/Main.js:154
> +        for (var j = 0; j < platformsByFamily[platformFamily].length; ++j) {

This for-loop computes platformsByFamily[platformFamily] twice on each iteration. We should cache platformsByFamily[platformFamily] in the body of the outer for-loop and then reference it in the control flow statement and body of the inner for-loop.

> Tools/BuildSlaveSupport/build.webkit.org-config/public_html/dashboard/Scripts/Main.js:243
> +        var logoImage = document.createElement("img");
> +        logoImage.classList.add("logo");
> +        cell.appendChild(logoImage);

Did you mean to add this? I mean, we already added a logo image above (lines 232-234).

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

This is OK as-is. Maybe a better name for this variable would be settingsPanel? or settingsBar?

> Tools/BuildSlaveSupport/build.webkit.org-config/public_html/dashboard/Scripts/Main.js:313
> +        var allPlatforms = document.getElementsByClassName("platform");

This variable is unused. Please remove it.

> Tools/BuildSlaveSupport/build.webkit.org-config/public_html/dashboard/Scripts/Main.js:314
> +        var platformFamilies = {};

Ditto.

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

This is OK as-is. Maybe a better name for this variable would be platformFamilySegmentedControl? or platformFamilyToolbar?

> Tools/BuildSlaveSupport/build.webkit.org-config/public_html/dashboard/Scripts/Main.js:317
> +        platformFamilyToggleWrapper.setAttribute("class", "unhide hidden familyToggleWrapper");

We should use the Element.classList API here as we did for settingsWrapper (line 313).

> Tools/BuildSlaveSupport/build.webkit.org-config/public_html/dashboard/Scripts/Main.js:320
> +        unhideAllButton.addEventListener("click", function () { settings.clearHiddenPlatformFamilies();});

Nit: Missing space character before the '}'.

> Tools/BuildSlaveSupport/build.webkit.org-config/public_html/dashboard/Scripts/Main.js:322
> +        unhideAllButton.setAttribute("id", "all-platformFamilyToggleButton");

See my remark on line 137 on how we can remove the need for assigning an id to this element.

> Tools/BuildSlaveSupport/build.webkit.org-config/public_html/dashboard/Scripts/Main.js:331
> +            platformFamilyToggle.addEventListener("click", function () {
> +                settings.toggleHiddenPlatformFamily(this.toString());
> +            }.bind(platformFamily));

We can take advantage of the fact that the text of the clicked element is the platform family name (by line 334) and simplify this line to read:

platformFamilyToggle.addEventListener("click", function (mouseEvent) { settings.toggleHiddenPlatformFamily(mouseEvent.target.textContent); });

> Tools/BuildSlaveSupport/build.webkit.org-config/public_html/dashboard/Scripts/Main.js:333
> +            platformFamilyToggle.setAttribute("id", platformFamily + "-platformFamilyToggleButton");

See my remark on line 137 on how we can remove the need for assigning an id to this element.

> Tools/BuildSlaveSupport/build.webkit.org-config/public_html/dashboard/Scripts/MetricsMain.js:81
> +            if (-1 === hiddenPlatformFamilies.indexOf(parsePlatformFamily(queue.platform)))

I would have written this as:

if (!hiddenPlatformFamilies.contains(parsePlatformFamily(queue.platform)))

> Tools/BuildSlaveSupport/build.webkit.org-config/public_html/dashboard/Styles/Main.css:221
> +    background: rgb(233, 231, 223);

Although this is OK as-is, I suggest that we specify the background color using the CSS property background-color instead of the shorthand property background so as to be consistent with how we specify background colors for other elements.

> Tools/BuildSlaveSupport/build.webkit.org-config/public_html/dashboard/Styles/Main.css:222
> +    z-index: 2;

Can we set z-index to 1?

> Tools/BuildSlaveSupport/build.webkit.org-config/public_html/dashboard/Styles/Main.css:227
> +    display: block;

Is this necessary?

> Tools/BuildSlaveSupport/build.webkit.org-config/public_html/dashboard/Styles/Main.css:243
> +div.familyShown {

Can we come up with a better name for this CSS class? Maybe familyIsVisible?

> Tools/BuildSlaveSupport/build.webkit.org-config/public_html/dashboard/Styles/Main.css:249
> +    position: inherit;

Is this necessary?
Comment 30 Daniel Bates 2015-09-16 22:34:30 PDT
We should also delete the local storage associated with the key "hiddenPlatforms".
Comment 31 David Kilzer (:ddkilzer) 2015-09-17 11:26:23 PDT
Comment on attachment 261351 [details]
Patch

r=me with fixes to the style issues and the unused variables.  Everything else seems like it could be addressed in a follow-up patch, or when changes are made later.
Comment 32 Dean Johnson 2015-09-17 11:45:23 PDT
Created attachment 261407 [details]
Patch
Comment 33 Dean Johnson 2015-09-17 11:46:26 PDT
Added style changes and David to the "Reviewed by", but I don't believe I have commit access. Could someone else land this?
Comment 34 David Kilzer (:ddkilzer) 2015-09-17 11:55:15 PDT
Comment on attachment 261407 [details]
Patch

r=me to land
Comment 35 WebKit Commit Bot 2015-09-17 12:42:53 PDT
Comment on attachment 261407 [details]
Patch

Clearing flags on attachment: 261407

Committed r189925: <http://trac.webkit.org/changeset/189925>
Comment 36 WebKit Commit Bot 2015-09-17 12:42:59 PDT
All reviewed patches have been landed.  Closing bug.