[webkit-reviews] review granted: [Bug 14380] Long DOM ancestry breadcrumb lists get cut off : [Attachment 17271] Patch

[bugzilla-daemon at webkit.org]

Adam Roben <aroben at apple.com> has granted Timothy Hatcher
<timothy at hatcher.name>'s request for review:
Bug 14380: Long DOM ancestry breadcrumb lists get cut off
http://bugs.webkit.org/show_bug.cgi?id=14380

Attachment 17271: Patch
http://bugs.webkit.org/attachment.cgi?id=17271&action=edit

------- Additional Comments from Adam Roben <aroben at apple.com>
+	     if (event.detail >= 2 ||
event.currentTarget.hasStyleClass("dimmed"))

I think the event.detail part of this line could use a comment.

+	 // The order of the crumbs in the document is oposite of the visual
order.

Typo: oposite -> opposite

+	     for (var significantIndex = (crumbs.childNodes.length - 1);
significantIndex >= 0; --significantIndex)

The parentheses around the subtraction aren't needed.

+	     // Walk in towards the significant crumb by calculating the side
that has the most crumbs
+	     // and shrinking that first. Once both sides are equal, favor the
shrinking the left side,
+	     // then alternate sides.

This comment makes it sound like you are going to do 3 loops, but you only do
one (which is a good thing). I think we can simplify the description a bit to
something like "Shrink crumbs one at a time by applying the shrinkingFunction
until the crumbs fit in the crumbsContainer or we run out of crumbs to shrink.
Crumbs are shrunk in order of descending distance from the signifcant crumb,
with a tie going to crumbs on the left."

+	     var startInset = 0;
+	     var endInset = 0;
+	     var lastIndex = crumbs.childNodes.length - 1;
+	     var startOffset = significantIndex;
+	     var endOffset = (lastIndex - significantIndex);
+
+	     while (startOffset > 0 || endOffset > 0) {
+		 if (startOffset > endOffset) {
+		     // Shrink from the right side.
+		     var shrinkCrumb = crumbs.childNodes[startInset];
+		     ++startInset;
+		     startOffset = (significantIndex - startInset);
+		 } else {
+		     // Shrink from the left side.
+		     var shrinkCrumb = crumbs.childNodes[(lastIndex -
endInset)];
+		     ++endInset;
+		     endOffset = (lastIndex - significantIndex - endInset);
+		 }

I think this could all be made a little simpler, like so:

var startIndex = 0;
var endIndex = crumbs.childNodes.length - 1;
while (startIndex != significantIndex || endIndex != significantIndex) {
    var startDistance = significantIndex - startIndex;
    var endDistance = endIndex - significantIndex;
    if (startDistance > endDistance) {
	var shrinkCrumb = crumbs.childNodes[startIndex];
	++startIndex;
    } else {
	var shrinkCrumb = crumbs.childNodes[endIndex];
	--endIndex;
    }

+	     if ((crumbs.offsetWidth + rightPadding) <
crumbsContainer.offsetWidth)
+		 return; // No need to compact the crumbs more.

Maybe you can put this check in a helper function, since you have to do it in
so many places.

r=me