[webkit-reviews] review denied: [Bug 52019] side-by-side diffs in the code review tool : [Attachment 78171] Patch

[bugzilla-daemon at webkit.org]

Ojan Vafai <ojan at chromium.org> has denied Ojan Vafai <ojan at chromium.org>'s
request for review:
Bug 52019: side-by-side diffs in the code review tool
https://bugs.webkit.org/show_bug.cgi?id=52019

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

------- Additional Comments from Ojan Vafai <ojan at chromium.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=78171&action=review

>> Websites/bugs.webkit.org/code-review.js:395

> 
> I've been using the "ify" for functions that take their parameter is the
|this| variable.  This one seems to act globally.

OK. I'll think of another name.

>> Websites/bugs.webkit.org/code-review.js:545

> 
> Is this going to cause problems if contents contains < or other HTML control
characters?  Generally, it's better to build these things up using DOM instead
of as strings.

I was careful that we only innerHTML html-escaped text, but I would feel safer
never innerHTML'ing patch lines. I'll rework it.

>> Websites/bugs.webkit.org/code-review.js:703

> 
> This seems like a really strange way of making a link and adding an event
handler.  Why not just use a class and a live event?

Sorry, I forget about jquery's crazy live events. Will fix.

>> Websites/bugs.webkit.org/code-review.js:740

> 
> Why not actually store this in location.hash?

Even if we put it in the hash, I think we'll want a parsed data structure for
this. I'm picturing that we'll be adding a number of things to the hash here.
In the spirit of keeping this patch small, I figured I could put that off for a
future patch.

>> Websites/bugs.webkit.org/code-review.js:755

> 
> Can we actually just move the DOM nodes over instead of converting them to
strings and then back into DOM nodes?

Ironically, I was trying to be more jquery friendly by doing it this way. I'll
rework it to be more DOMy.

>> Websites/bugs.webkit.org/code-review.js:792

> 
> Why not use each() for these iteration patterns?

Habit. I forget jquery has this.

>> Websites/bugs.webkit.org/code-review.js:814

> 
> We probably should avoid spamming the console.

This should never happen. So, it's really logging an error. I'll change the log
text to "Error: ..."