Bug 28457 – PrettyPatch tries to show images for deleted files

• Status: RESOLVED
• Resolution: FIXED
• Component: Tools / Tests

[Eric Seidel (no email)]

PrettyPatch tries to show images for deleted files
https://bugs.webkit.org/attachment.cgi?id=35114&action=prettypatch

PrettyPatch should recognize that it's a file deletion and not try to show an image.

[Caio Marcelo de Oliveira Filho]

Created attachment 87254 [details]
patch

[Eric Seidel (no email)]

Comment on attachment 87254 [details]
patch

SGTM.  Should this use a helper function?

[Caio Marcelo de Oliveira Filho]

For the HTML generation part you mean?

If so, I'm not sure if it worth adding a function for this case. Seems to me that other cases deserve more a function/method instead of this one.

[Caio Marcelo de Oliveira Filho]

Created attachment 90204 [details]
patch v2, rebased and with helper function

[Adam Roben (:aroben)]

Comment on attachment 90204 [details]
patch v2, rebased and with helper function

View in context: https://bugs.webkit.org/attachment.cgi?id=90204&action=review

> Websites/bugs.webkit.org/PrettyPatch/PrettyPatch.rb:558
> +        def image_to_html
> +            str = ""
> +            if @image_url then
>                  if @image_checksum then
>                      str += "<p>" + @image_checksum + "</p>"
>                  end
>                  str += "<img class='image' src='" + @image_url + "' />"
> +            else
> +                str += "<span class='text'>Image file removed</span>"
> +            end
> +            str
> +        end

Instead of appending to an empty string and then returning it at the end, how about just 'return "this is the string we want"' in each case?

[Caio Marcelo de Oliveira Filho]

Created attachment 90213 [details]
patch v3, reduce nesting in helper function

[WebKit Commit Bot]

Comment on attachment 90213 [details]
patch v3, reduce nesting in helper function

Clearing flags on attachment: 90213

Committed r84266: <http://trac.webkit.org/changeset/84266>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[WebKit Review Bot]

http://trac.webkit.org/changeset/84266 might have broken SnowLeopard Intel Release (Tests)
The following tests are not passing:
plugins/mouse-click-iframe-to-plugin.html