Bug 28457 - PrettyPatch tries to show images for deleted files
Summary: PrettyPatch tries to show images for deleted files
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC OS X 10.5
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2009-08-19 08:53 PDT by Eric Seidel (no email)
Modified: 2011-04-19 12:40 PDT (History)
6 users (show)

See Also:


Attachments
patch (2.58 KB, patch)
2011-03-28 20:58 PDT, Caio Marcelo de Oliveira Filho
eric: review+
Details | Formatted Diff | Diff
patch v2, rebased and with helper function (3.05 KB, patch)
2011-04-19 08:33 PDT, Caio Marcelo de Oliveira Filho
no flags Details | Formatted Diff | Diff
patch v3, reduce nesting in helper function (3.07 KB, patch)
2011-04-19 10:11 PDT, Caio Marcelo de Oliveira Filho
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
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