Bug 111621 – Dashboard cleanup: Move all dashboard ui related code into ui.js.

• Component: Tools / Tests
• Status: RESOLVED FIXED

[Julie Parent]

Dashboard cleanup: Move all dashboard ui related code into ui.js.

[Julie Parent]

Created attachment 191824 [details]
Patch

[Ojan Vafai]

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

A few random thoughts. I don't feel strongly about any of these, so feel free to ignore them and/or do them in a followup patch.

> Tools/TestResultServer/static-dashboards/ui.js:35
> +ui.hidePopup = function()

How about ui.popup.hide/ui.popup.show?

> Tools/TestResultServer/static-dashboards/ui.js:67
> +ui.checkboxHTML = function(queryParameter, label, isChecked, opt_extraJavaScript)

How about we move all these HTML returning things into their own namespace as well:
ui.html.checkbox
ui.html.testTypeSwitcher
ui.html._topLink
etc.

> Tools/TestResultServer/static-dashboards/ui.js:193
> +document.addEventListener('mousedown', function(e) {
> +    // Clear the open popup, unless the click was inside the popup.
> +    var popup = $('popup');
> +    if (popup && e.target != popup && !(popup.compareDocumentPosition(e.target) & 16))
> +        ui.hidePopup();
> +}, false);

Can we add this event listener in showPopup and remove it in hidePopup?

[Julie Parent]

Created attachment 191869 [details]
Patch

[Julie Parent]

Addressed all of Ojan's comments.  Much prettier now, and no longer have mousedown handlers laying around.

[Ojan Vafai]

Comment on attachment 191869 [details]
Patch

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

Please remove the comment before committing.

> Tools/TestResultServer/static-dashboards/ui.js:30
> +// @fileoverview Common utilities for generating ui for use in the
> +// dashboards.

This isn't google style....we don't need what comments that state the obvious. :)

[Julie Parent]

Created attachment 191889 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 191889 [details]
Patch for landing

Clearing flags on attachment: 191889

Committed r145028: <http://trac.webkit.org/changeset/145028>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.