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

[bugzilla-daemon at webkit.org]

Joseph Pecoraro <joepeck at webkit.org> 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 213294: Patch V1
https://bugs.webkit.org/attachment.cgi?id=213294&action=review

------- Additional Comments from Joseph Pecoraro <joepeck at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=213294&action=review


r=me, Just style nits in the C++ but a suggestion for a change in the
JavaScript. Feel free to put up another patch if you want me to look at the JS
once changed.

> Source/WebCore/inspector/InspectorOverlay.cpp:270
> +	   buildNodeHighlight(m_highlightNode.get(), 0, m_nodeHighlightConfig,
highlight);

Style: nullptr instead of 0.

Its a rather recent style change WebKit is going though with C++11 I think.
This ensures a little extra compile time safety (0 would implicitly work for
either pointer or a int / other primitive types, nullptr would only work for
pointer types).

> Source/WebCore/inspector/InspectorOverlay.cpp:463
> +	   return 0;

Style: nullptr

> Source/WebCore/inspector/InspectorOverlay.cpp:496
> +	   buildRendererHighlight(renderer, 0, config, &highlight);

Style: nullptr

> Source/WebCore/inspector/InspectorOverlay.cpp:501
> +	   RenderRegion* startRegion = 0;
> +	   RenderRegion* endRegion = 0;

Style: nullptr

> Source/WebCore/inspector/InspectorOverlay.cpp:505
> +	   if (!startRegion) {
> +	       ASSERT_NOT_REACHED();
> +	       return 0;

Style: nullptr

This would also read easier without braces. For example:

    ASSERT(startRegion);
    if (!startRegion)
	return nullptr;

> Source/WebCore/inspector/InspectorOverlay.cpp:516
> +		   // Compute the cliping area of the region.

Typo: "cliping" => "clipping"

> Source/WebCore/inspector/InspectorOverlay.cpp:531
> +	   return 0;

Style: nullptr

> Source/WebCore/inspector/InspectorOverlay.cpp:576
> +    if (containingFlowThread) {
> +	   if (containingFlowThread &&
containingFlowThread->isRenderNamedFlowThread()) {

Nit: You can remove the outer if.

> Source/WebCore/inspector/InspectorOverlay.cpp:594
> +	   return 0;
> +
> +    Node* node = m_highlightNode.get();
> +    RenderObject* renderer = node->renderer();
> +    if (!renderer)
> +	   return 0;

Style: nullptr

> Source/WebCore/inspector/InspectorOverlayPage.js:266
> +    var elementTitleTemplate =
document.getElementById("element-title-template");
> +    var elementTitle = elementTitleTemplate.cloneNode(true);
> +    // Remove the template's ID.
> +    elementTitle.id = "";

It would probably be better to just create the elements rather then cloneNode,
since inevitably with cloning we need to querySelector to find nodes. Building
it ourselves we can both create only the elements we need and not have to
search for elements. You would also avoid the temporary situation of having
multiple elements with the same id.

For example (untested):

    function _createElementTitle(elementInfo)
    {
	var elementTitleElement = document.createElement("div");
	elementTitleElement.className = "element-title";

	var tagNameElement =
elementTitle.appendChild(document.createElement("span"));
	tagNameElement.className = "tag-name";
	tagNameElement.textContent = elementInfo.tagName;

	if (elementInfo.idValue) {
	    var idElement =
elementTitle.appendChild(document.createElement("span"));
	    idElement.className = "node-id";
	    idElement.textContent = "#" + elementInfo.idValue;
	}

	if (elementInfo.className) {
	    var className = elementTitle.className.length > 50 ?
elementTitle.className.substring(0, 50) + "\u2026" : elementTitle.className;
	    var classNameElement =
elementTitle.appendChild(document.createElement("span"));
	    classNameElement.className = "class-name";
	    classNameElement.textContent = className;
	}

	…

	return elementTitleElement;
    }