Bug 38552

Summary: PrettyPatch.pretty_diff("") should not hang
Product: WebKit Reporter: Eric Seidel (no email) <eric>
Component: New BugsAssignee: Nobody <webkit-unassigned>
Severity: Normal CC: abarth, cjerdonek, commit-queue, dpranke, ojan
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Other   
OS: OS X 10.5   
Bug Depends on:    
Bug Blocks: 38505    
Description Flags
Patch none

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]
Comment 2 Dirk Pranke 2010-05-04 16:01:23 PDT
Comment on attachment 55066 [details]

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:


import base

   # 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]
Comment 8 Dirk Pranke 2010-05-04 19:57:29 PDT
Comment on attachment 55085 [details]

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

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

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.