Bug 167524 – Add support for Trac instances that host multiple projects.

• Status: RESOLVED
• Resolution: FIXED
• Component: Tools / Tests

[Kocsen Chung]

Add support for Trac instances that host multiple projects.

[Kocsen Chung]

Created attachment 299953 [details]
Patch

[Alexey Proskuryakov]

Comment on attachment 299953 [details]
Patch

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

> Tools/BuildSlaveSupport/build.webkit.org-config/public_html/dashboard/Scripts/Trac.js:120
> -        console.assert(fromDate <= toDate);
> +        if (fromDate > toDate)
> +            throw RangeError("Parameter fromDate should be before or on toDate.");

Why this change? This is logically an assertion, so it's cleaner to express it as such.

Just like in C/C++, adding a unit test that violates an assertion is not worth it.

> Tools/BuildSlaveSupport/build.webkit.org-config/public_html/dashboard/Scripts/Trac.js:131
> +            "&from=" + encodeURIComponent(toDay.toISOString().slice(0, 10)) +
> +            "&daysback=" + encodeURIComponent((toDay - fromDay) / 1000 / 60 / 60 / 24);

Why add encodeURIComponent here if it's not needed?

[Kocsen Chung]

(In reply to comment #2)
> Comment on attachment 299953 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=299953&action=review
> 
> > Tools/BuildSlaveSupport/build.webkit.org-config/public_html/dashboard/Scripts/Trac.js:120
> > -        console.assert(fromDate <= toDate);
> > +        if (fromDate > toDate)
> > +            throw RangeError("Parameter fromDate should be before or on toDate.");
> 
> Why this change? This is logically an assertion, so it's cleaner to express
> it as such.
> 
> Just like in C/C++, adding a unit test that violates an assertion is not
> worth it.

It seemed too lenient to allow a range violation of this kind get away with an assertion. Additionally, it would allow for testing this code path. The way I interpret it, an assertion is an error and should be treated as such; especially in production code.
However, I do see your point that it may not be worth it to test this assert violation. Moreover I concur that it would be cleaner to express it as such. I will revert this change.

> 
> > Tools/BuildSlaveSupport/build.webkit.org-config/public_html/dashboard/Scripts/Trac.js:131
> > +            "&from=" + encodeURIComponent(toDay.toISOString().slice(0, 10)) +
> > +            "&daysback=" + encodeURIComponent((toDay - fromDay) / 1000 / 60 / 60 / 24);
> 
> Why add encodeURIComponent here if it's not needed?

Mostly as a security mechanism to not trust date input and as a result avoid unexpected or erroneous requests to the server. In my performance profiling, this had minimal effect and can provide security benefits.

Thanks for the review, Alexey.

[Kocsen Chung]

Created attachment 300139 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 300139 [details]
Patch

Clearing flags on attachment: 300139

Committed r211388: <http://trac.webkit.org/changeset/211388>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.