[webkit-reviews] review granted: [Bug 122291] Web Inspector: [CSS Regions] Display the correct fragment boxes for content inside flow threads : [Attachment 213625] Patch V3

[bugzilla-daemon at webkit.org]

Timothy Hatcher <timothy at apple.com> has granted Alexandru Chiculita
<achicu at adobe.com>'s request for review:
Bug 122291: Web Inspector: [CSS Regions] Display the correct fragment boxes for
content inside flow threads
https://bugs.webkit.org/show_bug.cgi?id=122291

Attachment 213625: Patch V3
https://bugs.webkit.org/attachment.cgi?id=213625&action=review

------- Additional Comments from Timothy Hatcher <timothy at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=213625&action=review


> Source/WebCore/inspector/InspectorOverlayPage.css:84
> +    /* Add a space to separate the property name from the value. */
> +    content: " ";

This isn't accessibility friendly. Can it be added in the DOM instead? (Not
that the overlay is friendly to accessibility already, but less barriers the
better.)

> Source/WebCore/inspector/InspectorOverlayPage.js:281
> +    new DOMBuilder("div", className)
> +	   .appendTo(this.element)
> +	   .appendSpan("css-property", propertyName)
> +	   .appendSpan("value", value);

The chaining here confused me. It would be clearer to do: var builder = new
DOMBuilder… and not chain all the calls.

> Source/WebCore/inspector/InspectorOverlayPage.js:292
> +    return (value && value.length > maxLength) ? value.substring(0, 50) +
"\u2026" : value;

No need for the ( ) around the condition.

> Source/WebCore/inspector/InspectorOverlayPage.js:311
> +    var titleBuilder = new DOMBuilder("div", "element-title")
> +	   .appendTo(document.getElementById("element-title-container"))
> +
> +	   .appendSpanIfNotNull("tag-name", elementInfo.tagName)
> +	   .appendSpanIfNotNull("node-id", elementInfo.idValue, "#")
> +	   .appendSpanIfNotNull("class-name",
_truncateString(elementInfo.className, 50))
> +
> +	   .appendSpan("node-width", elementInfo.nodeWidth)
> +	   .appendSpan("px", "px \xd7 ") // \xd7 is the code for the &times;
HTML entity.
> +	   .appendSpan("node-height", elementInfo.nodeHeight)
> +	   .appendSpan("px", "px")
> +
> +	   .appendPropertyIfNotNull("region-flow-name", "Region Flow",
elementInfo.regionFlowInfo ? elementInfo.regionFlowInfo.name : null)
> +	   .appendPropertyIfNotNull("content-flow-name", "Content Flow",
elementInfo.contentFlowInfo ? elementInfo.contentFlowInfo.name : null);
> +

Ditto here about the extreme chaining. I think it is less clear to read.

> Source/WebCore/inspector/InspectorOverlayPage.js:503
> -function drawNodeHighlight(highlight)
> -{
> +function _drawFragmentHighlight(highlight) {

The { should be on the next line like it was before.

> Source/WebCore/inspector/InspectorOverlayPage.js:514
> +	   var regionClipQuad = highlight.region.quad;
> +	   quadToPath(regionClipQuad).clip();

Could be one line. Not much is gained by making a variable.