[webkit-reviews] review granted: [Bug 26210] PrettyPatch should display added images inline : [Attachment 30997] Add support for inline display of png files from svn-create-patch patches

[bugzilla-daemon at webkit.org]

Adam Roben (aroben) <aroben at apple.com> has granted Eric Seidel
<eric at webkit.org>'s request for review:
Bug 26210: PrettyPatch should display added images inline
https://bugs.webkit.org/show_bug.cgi?id=26210

Attachment 30997: Add support for inline display of png files from
svn-create-patch patches
https://bugs.webkit.org/attachment.cgi?id=30997&action=review

------- Additional Comments from Adam Roben (aroben) <aroben at apple.com>
> +.image {
> +    border: 2px solid black;
> +}

Why not just key this off the img tag?

> +
>  .context, .context .lineNumber {
>      color: #849;
>      background-color: #fef;
> @@ -191,17 +197,25 @@ EOF
>		       break
>		   when BINARY_FILE_MARKER_FORMAT
>		       @binary = true
> +		       if (IMAGE_FILE_MARKER_FORMAT.match(lines[i + 1])) then

This will probably do something bad on the last iteration of the loop when i ==
lines.length - 1. But maybe we know that isn't the case given that we just saw
a binary file marker?

> +	       lines_with_contents = lines[startOfSections...lines.length]
> +	       @sections = DiffSection.parse(lines_with_contents) unless
@binary
> +	       @image_url = "data:image/png;base64," + lines_with_contents.join
if @image

I'm surprised you didn't have to chomp the newlines off each line.

> +	       if @image then
> +		   str += "<img class='image' src='" + @image_url + "' />"

No need for the space or the slash before the right angle bracket.

r=me