Bug 148403 – Add toggle options to build-safari dashboard for hiding/showing certain device types.

• Component: Tools / Tests
• Status: RESOLVED FIXED

[Dean Johnson]

We should have a toggle option for showing and hiding groups of devices at a given time.

[Dean Johnson]

Created attachment 259789 [details]
Patch

[Dean Johnson]

Created attachment 259793 [details]
Patch

[Dean Johnson]

Created attachment 259978 [details]
Patch

[Alexey Proskuryakov]

Could you please attach a screenshot with how this looks on build.webkit.org now?

[Dean Johnson]

Created attachment 260003 [details]
New toggle buttons (top left)

[Alexey Proskuryakov]

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?

[Dean Johnson]

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.

[Alexey Proskuryakov]

Discussed in person, Dean will update the patch.

[Alexey Proskuryakov]

Comment on attachment 259978 [details]
Patch

Marking r- to get this patch out of the review queue.

[Dean Johnson]

Created attachment 260421 [details]
Patch

[David Kilzer (:ddkilzer)]

Comment on attachment 260421 [details]
Patch

r=me

[WebKit Commit Bot]

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

[David Kilzer (:ddkilzer)]

Dean, do you have commit access?  Otherwise, the patch just needs a ChangeLog.

[Dean Johnson]

Created attachment 260454 [details]
Patch

[Dean Johnson]

Comment on attachment 260454 [details]
Patch

Changelog wasn't included.

[Dean Johnson]

Created attachment 260455 [details]
Patch

[Dean Johnson]

:ddkilzer - No commit access yet... still need to land some more patches. :)

[Daniel Bates]

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.

[Dean Johnson]

Created attachment 261113 [details]
Patch

[Dean Johnson]

(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.

[Dean Johnson]

Created attachment 261114 [details]
Patch

[Alexey Proskuryakov]

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?

[Dean Johnson]

(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.

[Darin Adler]

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
> [phone]  Dean Johnson  <[e-mail]>
> +        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.

[Daniel Bates]

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.

[Dean Johnson]

Created attachment 261348 [details]
Patch

[Dean Johnson]

Created attachment 261351 [details]
Patch

[Dean Johnson]

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.

[Daniel Bates]

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?

[Daniel Bates]

We should also delete the local storage associated with the key "hiddenPlatforms".

[David Kilzer (:ddkilzer)]

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.

[Dean Johnson]

Created attachment 261407 [details]
Patch

[Dean Johnson]

Added style changes and David to the "Reviewed by", but I don't believe I have commit access. Could someone else land this?

[David Kilzer (:ddkilzer)]

Comment on attachment 261407 [details]
Patch

r=me to land

[WebKit Commit Bot]

Comment on attachment 261407 [details]
Patch

Clearing flags on attachment: 261407

Committed r189925: <http://trac.webkit.org/changeset/189925>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.