[webkit-reviews] review granted: [Bug 51162] anchor the toolbar to the bottom of the diff if the diff doesn't take a full screen of height : [Attachment 76798] Patch

[bugzilla-daemon at webkit.org]

Adam Barth <abarth at webkit.org> has granted Ojan Vafai <ojan at chromium.org>'s
request for review:
Bug 51162: anchor the toolbar to the bottom of the diff if the diff doesn't
take a full screen of height
https://bugs.webkit.org/show_bug.cgi?id=51162

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

------- Additional Comments from Adam Barth <abarth at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=76798&action=review

Ok.  This patch is kind of crazy.  I'm slightly worried that this script is
getting pretty complex.  Well, to be fair, it had kind of gotten out of control
before this patch.  :)

> BugsSite/code-review.js:611
> +    if (hasScrollbar && !toolbar.hasClass('anchored'))
> +	 toolbar.addClass('anchored')

Why not just add the class unconditionally?

> BugsSite/code-review.js:614
> +    if (!hasScrollbar)
> +	 toolbar.removeClass('anchored');

I think there's a toggle concept in jQuery that helps with this.

> BugsSite/code-review.js:644
> +    $(document.body).append('<iframe
id="pseudo_resize_event_iframe"></iframe>');
> +   
document.getElementById('pseudo_resize_event_iframe').contentWindow.addEventLis
tener('resize', onBodyResize);

I would have created the iframe elment as a fragment, using $('<iframe
id="pseudo_resize_event_iframe"></iframe>'), appended the element, and then
attached the event listener via the DOM directly instead of going back to the
document to getElementById.  What if there's already an element with that ID in
the document?  Anyway, it's not a big deal, it just has most assumptions.

This whole resize thing seems pretty crazy.  Is it really the best way?  I'm
willing to believe you since you know way more about this stuff than I do.