Bug 28457

Summary:

PrettyPatch tries to show images for deleted files

Product:

WebKit

Reporter:

Eric Seidel (no email) <eric>

Component:

Tools / Tests

Assignee:

Nobody <webkit-unassigned>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

abarth, aroben, cmarcelo, commit-queue, eric, webkit.review.bot

Priority:

Version:

528+ (Nightly build)

Hardware:

OS:

OS X 10.5

Attachments:

Description	Flags
patch	eric: review+
patch v2, rebased and with helper function	none
patch v3, reduce nesting in helper function	none

Description Eric Seidel (no email) 2009-08-19 08:53:06 PDT

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.

Comment 1 Caio Marcelo de Oliveira Filho 2011-03-28 20:58:39 PDT

Created attachment 87254 [details]
patch

Comment 2 Eric Seidel (no email) 2011-03-28 21:25:19 PDT

Comment on attachment 87254 [details]
patch

SGTM.  Should this use a helper function?

Comment 3 Caio Marcelo de Oliveira Filho 2011-03-28 22:43:02 PDT

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.

Comment 4 Caio Marcelo de Oliveira Filho 2011-04-19 08:33:12 PDT

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

Comment 5 Adam Roben (:aroben) 2011-04-19 09:50:16 PDT

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?

Comment 6 Caio Marcelo de Oliveira Filho 2011-04-19 10:11:39 PDT

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

Comment 7 WebKit Commit Bot 2011-04-19 11:08:27 PDT

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>

Comment 8 WebKit Commit Bot 2011-04-19 11:08:33 PDT

All reviewed patches have been landed.  Closing bug.

Comment 9 WebKit Review Bot 2011-04-19 12:40:29 PDT

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