Bug 111621 - Dashboard cleanup: Move all dashboard ui related code into ui.js.
Summary: Dashboard cleanup: Move all dashboard ui related code into ui.js.
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-06 13:44 PST by Julie Parent
Modified: 2013-03-06 19:50 PST (History)
5 users (show)

See Also:


Attachments
Patch (26.07 KB, patch)
2013-03-06 13:48 PST, Julie Parent
no flags Details | Formatted Diff | Diff
Patch (26.32 KB, patch)
2013-03-06 17:00 PST, Julie Parent
no flags Details | Formatted Diff | Diff
Patch for landing (26.23 KB, patch)
2013-03-06 19:01 PST, Julie Parent
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Julie Parent 2013-03-06 13:44:03 PST
Dashboard cleanup: Move all dashboard ui related code into ui.js.
Comment 1 Julie Parent 2013-03-06 13:48:37 PST
Created attachment 191824 [details]
Patch
Comment 2 Ojan Vafai 2013-03-06 13:55:09 PST
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?
Comment 3 Julie Parent 2013-03-06 17:00:08 PST
Created attachment 191869 [details]
Patch
Comment 4 Julie Parent 2013-03-06 17:00:40 PST
Addressed all of Ojan's comments.  Much prettier now, and no longer have mousedown handlers laying around.
Comment 5 Ojan Vafai 2013-03-06 18:47:32 PST
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. :)
Comment 6 Julie Parent 2013-03-06 19:01:17 PST
Created attachment 191889 [details]
Patch for landing
Comment 7 WebKit Review Bot 2013-03-06 19:50:21 PST
Comment on attachment 191889 [details]
Patch for landing

Clearing flags on attachment: 191889

Committed r145028: <http://trac.webkit.org/changeset/145028>
Comment 8 WebKit Review Bot 2013-03-06 19:50:25 PST
All reviewed patches have been landed.  Closing bug.