Bug 38552 - PrettyPatch.pretty_diff("") should not hang
Summary: PrettyPatch.pretty_diff("") should not hang
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Other OS X 10.5
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks: 38505
  Show dependency treegraph
 
Reported: 2010-05-04 15:48 PDT by Eric Seidel (no email)
Modified: 2010-05-04 23:06 PDT (History)
5 users (show)

See Also:


Attachments
Patch (5.31 KB, patch)
2010-05-04 15:52 PDT, Eric Seidel (no email)
no flags Details | Formatted Diff | Diff
Patch (4.29 KB, patch)
2010-05-04 19:39 PDT, Eric Seidel (no email)
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) 2010-05-04 15:48:34 PDT
Disable pretty patch on Chromium Mac to see if that resolves some hangs seen on the Mac Canary
Comment 1 Eric Seidel (no email) 2010-05-04 15:52:48 PDT
Created attachment 55066 [details]
Patch
Comment 2 Dirk Pranke 2010-05-04 16:01:23 PDT
Comment on attachment 55066 [details]
Patch

Can we just make the Mac port set _pretty_patch_available to false instead of returning a dummy text string?
Comment 3 Eric Seidel (no email) 2010-05-04 16:02:19 PDT
That will cause it to return a different dummy text string, but that's totally doable too.
Comment 4 Eric Seidel (no email) 2010-05-04 16:12:41 PDT
Dirk the change you're suggesting would look like this:

chromium_mac.py:

import base

__init__...
   # FIXME: Remove this hack.
   _log.warn("Disabling pretty patch support due to bug 38552");
   base._pretty_patch_available = False


I'm happy to make such.  But I think it's kinda a wash.  They're both pretty hackish.
Comment 5 Dirk Pranke 2010-05-04 17:52:16 PDT
Ah, I see. It would be nice if there was an overridable way of determining if pretty patch was available, but there isn't. Also, I see that what you return from pretty_diff_text() - an error string -- is different than what we return from wdiff_text()  -- an empty string -- if it isn't available.

So your first change is fine.
Comment 6 Eric Seidel (no email) 2010-05-04 19:32:11 PDT
Just going to fix the pretty_diff hang for now.
Comment 7 Eric Seidel (no email) 2010-05-04 19:39:25 PDT
Created attachment 55085 [details]
Patch
Comment 8 Dirk Pranke 2010-05-04 19:57:29 PDT
Comment on attachment 55085 [details]
Patch

LGTM (although I'm not a reviewer).
Comment 9 Shinichiro Hamaji 2010-05-04 22:48:54 PDT
Comment on attachment 55085 [details]
Patch

Looks like a nice fix.
Comment 10 WebKit Commit Bot 2010-05-04 23:06:37 PDT
Comment on attachment 55085 [details]
Patch

Clearing flags on attachment: 55085

Committed r58803: <http://trac.webkit.org/changeset/58803>
Comment 11 WebKit Commit Bot 2010-05-04 23:06:44 PDT
All reviewed patches have been landed.  Closing bug.