Bug 47974 - [chromium] Chromium Mac should use WebKit's image decoders
Summary: [chromium] Chromium Mac should use WebKit's image decoders
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: Adam Barth
URL:
Keywords:
Depends on: 48188
Blocks: 49021
  Show dependency treegraph
 
Reported: 2010-10-20 00:47 PDT by Adam Barth
Modified: 2010-11-04 13:28 PDT (History)
7 users (show)

See Also:


Attachments
work-in-progres (3.10 KB, patch)
2010-10-20 00:50 PDT, Adam Barth
no flags Details | Formatted Diff | Diff
builds and passes at least one test (7.38 KB, patch)
2010-10-20 13:28 PDT, Adam Barth
no flags Details | Formatted Diff | Diff
looks right in test_shell (7.45 KB, patch)
2010-10-20 14:55 PDT, Adam Barth
no flags Details | Formatted Diff | Diff
seems to work (11.14 KB, patch)
2010-10-20 18:42 PDT, Adam Barth
no flags Details | Formatted Diff | Diff
Patch (12.32 KB, patch)
2010-10-21 00:18 PDT, Adam Barth
no flags Details | Formatted Diff | Diff
Patch (15.65 KB, patch)
2010-10-21 00:37 PDT, Adam Barth
no flags Details | Formatted Diff | Diff
Patch (17.50 KB, patch)
2010-10-22 12:57 PDT, Adam Barth
no flags Details | Formatted Diff | Diff
Patch (17.50 KB, patch)
2010-10-22 13:12 PDT, Adam Barth
no flags Details | Formatted Diff | Diff
Patch (17.48 KB, patch)
2010-10-22 14:57 PDT, Adam Barth
no flags Details | Formatted Diff | Diff
updated with style fixes (19.55 KB, patch)
2010-10-22 15:20 PDT, Adam Barth
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Adam Barth 2010-10-20 00:47:02 PDT
[chromium] Chromium Mac should use WebKit's image decoders
Comment 1 Adam Barth 2010-10-20 00:50:24 PDT
Created attachment 71261 [details]
work-in-progres
Comment 2 Adam Barth 2010-10-20 00:52:16 PDT
Turns out this is mostly donking around with the build system.  It looks like the main thing I have to do is implement ImageDecoderCG, which is the adapter between the image decoder output and the native bitmap.  I'll have to find some documentation about how to actually use CoreGraphics.  :)
Comment 3 Adam Barth 2010-10-20 13:28:11 PDT
Created attachment 71329 [details]
builds and passes at least one test
Comment 4 Adam Barth 2010-10-20 14:55:59 PDT
Created attachment 71343 [details]
looks right in test_shell
Comment 5 Adam Barth 2010-10-20 18:42:45 PDT
Created attachment 71379 [details]
seems to work
Comment 6 Pascal Massimino 2010-10-20 18:52:44 PDT
(In reply to comment #5)
> Created an attachment (id=71379) [details]
> seems to work

review tool is acting strange to me, so i copy-paste the comments here:



View in context: https://bugs.webkit.org/attachment.cgi?id=71379&action=review
> WebCore/platform/image-decoders/ImageDecoder.cpp:142
> +    memset(m_bytes, 0, m_size.width() * m_size.height() * sizeof(PixelData));

i'm not familiar with m_bytes, but this looks strange. Shouldn't it be something like: memset(m_bytes.data(), 0, ...) ?

> WebKitTools/Scripts/webkitpy/layout_tests/port/chromium_mac.py:109
> +        return True

probably not final, right?
Comment 7 Adam Barth 2010-10-20 18:56:24 PDT
> View in context: https://bugs.webkit.org/attachment.cgi?id=71379&action=review
> > WebCore/platform/image-decoders/ImageDecoder.cpp:142
> > +    memset(m_bytes, 0, m_size.width() * m_size.height() * sizeof(PixelData));
> 
> i'm not familiar with m_bytes, but this looks strange. Shouldn't it be something like: memset(m_bytes.data(), 0, ...) ?

m_bytes is now just a raw data so we can write through to the buffer quickly.

> > WebKitTools/Scripts/webkitpy/layout_tests/port/chromium_mac.py:109
> > +        return True
> 
> probably not final, right?

Nope.  I just don't have wdiff installed and this check was a PITA for me.
Comment 8 Pascal Massimino 2010-10-20 19:07:11 PDT
(In reply to comment #5)
> Created an attachment (id=71379) [details]
> seems to work

review tool is acting strange to me, so i copy-paste the comments here:



View in context: https://bugs.webkit.org/attachment.cgi?id=71379&action=review
> WebCore/platform/image-decoders/ImageDecoder.cpp:142
> +    memset(m_bytes, 0, m_size.width() * m_size.height() * sizeof(PixelData));

i'm not familiar with m_bytes, but this looks strange. Shouldn't it be something like: memset(m_bytes.data(), 0, ...) ?

> WebKitTools/Scripts/webkitpy/layout_tests/port/chromium_mac.py:109
> +        return True

probably not final, right?(In reply to comment #7)
> > View in context: https://bugs.webkit.org/attachment.cgi?id=71379&action=review
> > > WebCore/platform/image-decoders/ImageDecoder.cpp:142
> > > +    memset(m_bytes, 0, m_size.width() * m_size.height() * sizeof(PixelData));
> > 
> > i'm not familiar with m_bytes, but this looks strange. Shouldn't it be something like: memset(m_bytes.data(), 0, ...) ?
> 
> m_bytes is now just a raw data so we can write through to the buffer quickly.
> 

oh, i see. then, wouldn't it be safer to replace
   m_size.width() * m_size.height() * sizeof(PixelData)
by backingStore.size() (or equivalent) ?

> > > WebKitTools/Scripts/webkitpy/layout_tests/port/chromium_mac.py:109
> > > +        return True
> > 
> > probably not final, right?
> 
> Nope.  I just don't have wdiff installed and this check was a PITA for me.

oh, ok
Comment 9 Adam Barth 2010-10-20 20:17:37 PDT
The color profile thing looks pretty easy too.  I'll probably do it in a separate patch though.  We'll need to use this code:

http://trac.imagemagick.org/browser/lcms/trunk/jpegicc/iccjpeg.h
http://trac.imagemagick.org/browser/lcms/trunk/jpegicc/iccjpeg.c

I'm trying to figure out what license it has.
Comment 10 Adam Barth 2010-10-21 00:18:25 PDT
Created attachment 71397 [details]
Patch
Comment 11 Adam Barth 2010-10-21 00:20:16 PDT
AFACT, this patch works.  Of course, it doesn't have color profile support.  I looked into that and it shouldn't be that hard.  One approach is to review this patch and the color profile patch and then land them together.
Comment 12 Peter Kasting 2010-10-21 00:27:02 PDT
Let's explicitly not worry about color profiles in the first patch, and do that second.
Comment 13 Peter Kasting 2010-10-21 00:31:55 PDT
Comment on attachment 71397 [details]
Patch

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

> WebCore/platform/image-decoders/ImageDecoder.h:186
> +        PixelData* m_bytes;   // The memory is backed by m_backingStore.

Do you need to change any of the other ImageDecoderXXX.cpp files due to this modification?

> WebCore/platform/image-decoders/cg/ImageDecoderCG.cpp:66
> +        alphaInfo | kCGBitmapByteOrder32Host, dataProvider.get(), 0, false, kCGRenderingIntentDefault);

What! Line-wrapping?!
Comment 14 Adam Barth 2010-10-21 00:36:31 PDT
Comment on attachment 71397 [details]
Patch

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

>> WebCore/platform/image-decoders/ImageDecoder.h:186
>> +        PixelData* m_bytes;   // The memory is backed by m_backingStore.
> 
> Do you need to change any of the other ImageDecoderXXX.cpp files due to this modification?

Yep!

>> WebCore/platform/image-decoders/cg/ImageDecoderCG.cpp:66
>> +    return CGImageCreate(width(), height(), 8, 32, width() * sizeof(PixelData), colorSpace.get(),
>> +        alphaInfo | kCGBitmapByteOrder32Host, dataProvider.get(), 0, false, kCGRenderingIntentDefault);
> 
> What! Line-wrapping?!

Crazy, I know.
Comment 15 Adam Barth 2010-10-21 00:37:05 PDT
Created attachment 71398 [details]
Patch
Comment 16 Peter Kasting 2010-10-21 13:31:48 PDT
Comment on attachment 71398 [details]
Patch

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

I don't know CG well enough to review ImageDecoderCG.cpp, but the rest of the changes look fine to me.

> WebCore/WebCore.gyp/WebCore.gyp:-1184
> -            ['exclude', 'platform/image-decoders/xbm/XBMImageDecoder\\.(cpp|h)$'],

Thanks for getting rid of this!

> WebCore/platform/image-decoders/ImageDecoder.h:142
> +#else

Nit: For clarity, might want to not define this typedef at all for QT and Skia.
Comment 17 Adam Barth 2010-10-21 14:12:54 PDT
Comment on attachment 71398 [details]
Patch

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

>> WebCore/platform/image-decoders/ImageDecoder.h:142
>> +#if PLATFORM(CF)
>> +        typedef RetainPtr<CFMutableDataRef> NativeBackingStore;
>> +#else
> 
> Nit: For clarity, might want to not define this typedef at all for QT and Skia.

Okiedokes.
Comment 18 Adam Barth 2010-10-21 14:25:52 PDT
+eric for CG knowledge.
Comment 19 Peter Kasting 2010-10-21 14:33:06 PDT
Eric: for context, the intended memory model of RGBA32Buffer is:

* Most frames should call setSize() exactly once to allocate the memory they need for a framebuffer of that size.
* Standard copying (operator=) should just result in an additional ref of the underlying pixel data.  copyBitmapData() is for when a deep copy is needed; it should drop any ref to existing framebuffer data and make a complete copy of the framebuffer in the provided object.
* asNewNativeImage should return a NativeImagePtr that holds a ref to the framebuffer data.  It does not need to copy the data, just ref it.
Comment 20 Adam Barth 2010-10-21 14:38:43 PDT
> * Standard copying (operator=) should just result in an additional ref of the underlying pixel data.  copyBitmapData() is for when a deep copy is needed; it should drop any ref to existing framebuffer data and make a complete copy of the framebuffer in the provided object.

This patch does a deep copy for operator=.  I can change this to just a ref if that's important, it just costs #ifdefs.
Comment 21 Peter Kasting 2010-10-21 14:44:43 PDT
(In reply to comment #20)
> This patch does a deep copy for operator=.

For performance reasons, you should probably change it.

I would make sure you've done two things:

* Read all comments in ImageDecoder.h.  For example, that notes that operator=() can just ref and not have to copy.
* Read ImageDecoderSkia.cpp carefully and understand why it has overridden every function, and what they do.  The memory/lifetime/refcount picture on Skia should be identical (or very close) to that on CG.  I think you've already gotten most of the important bits.
Comment 22 Adam Barth 2010-10-21 14:57:41 PDT
> For performance reasons, you should probably change it.

Ok.

> I would make sure you've done two things:
> 
> * Read all comments in ImageDecoder.h.  For example, that notes that operator=() can just ref and not have to copy.
> * Read ImageDecoderSkia.cpp carefully and understand why it has overridden every function, and what they do.  The memory/lifetime/refcount picture on Skia should be identical (or very close) to that on CG.  I think you've already gotten most of the important bits.

Yeah, I did all of that.  We don't need to override as many of the functions because CG lets us disentangle ownership of the backing store from the image object.
Comment 23 Peter Kasting 2010-10-21 17:44:46 PDT
(In reply to comment #22)
> Yeah, I did all of that.  We don't need to override as many of the functions because CG lets us disentangle ownership of the backing store from the image object.

Yep, that's what it looked like to me too.  I think the operator=() thing is the only case that needed changing.
Comment 24 Adam Barth 2010-10-21 18:00:36 PDT
> Yep, that's what it looked like to me too.  I think the operator=() thing is the only case that needed changing.

I'll make that change tomorrow.
Comment 25 Adam Barth 2010-10-22 12:57:44 PDT
Created attachment 71588 [details]
Patch
Comment 26 WebKit Review Bot 2010-10-22 13:01:54 PDT
Attachment 71588 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1
WebCore/platform/image-decoders/ImageDecoder.h:190:  One space before end of line comments  [whitespace/comments] [5]
Total errors found: 1 in 14 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 27 Adam Barth 2010-10-22 13:12:07 PDT
Created attachment 71591 [details]
Patch
Comment 28 WebKit Review Bot 2010-10-22 13:14:45 PDT
Attachment 71591 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1
WebCore/platform/image-decoders/ImageDecoder.h:190:  One space before end of line comments  [whitespace/comments] [5]
Total errors found: 1 in 14 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 29 Eric Seidel (no email) 2010-10-22 13:58:02 PDT
Comment on attachment 71591 [details]
Patch

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

> WebCore/platform/image-decoders/cg/ImageDecoderCG.cpp:38
> +    m_backingStore = other.m_backingStore;
> +    m_bytes = reinterpret_cast<PixelData*>(CFDataGetMutableBytePtr(m_backingStore.get()));

I suspect this method might cause a copy.

> WebCore/platform/image-decoders/cg/ImageDecoderCG.cpp:40
> +    // FIXME: The rest of this function seems redundant with RGBA32Buffer::copyBitmapData.
> +    m_size = other.m_size;

It seems very strange to me that these 3 are separate members.  Why does this need to be the case?

> WebCore/platform/image-decoders/cg/ImageDecoderCG.cpp:60
> +    m_bytes = reinterpret_cast<PixelData*>(CFDataGetMutableBytePtr(m_backingStore.get()));

Why do you need mutable pointers?

> WebCore/platform/image-decoders/cg/ImageDecoderCG.cpp:70
> +    RetainPtr<CGColorSpaceRef> colorSpace(AdoptCF, CGColorSpaceCreateWithName(kCGColorSpaceGenericRGB));

I think you want DeviceRGB, not GenericRGB.  Generic will still to color-matching for you.  You want Device color spaces which do not do color matching as they're already in the colorspace of the device.

> WebCore/platform/image-decoders/cg/ImageDecoderCG.cpp:76
> +    return CGImageCreate(width(), height(), 8, 32, width() * sizeof(PixelData), colorSpace.get(),
> +        alphaInfo | kCGBitmapByteOrder32Host, dataProvider.get(), 0, false, kCGRenderingIntentDefault);

Hmmm... My data my be old here, but I would have expected you to use CGBitmapImageCreate here.
Comment 30 Adam Barth 2010-10-22 14:22:42 PDT
Comment on attachment 71591 [details]
Patch

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

>> WebCore/platform/image-decoders/cg/ImageDecoderCG.cpp:38
>> +    m_bytes = reinterpret_cast<PixelData*>(CFDataGetMutableBytePtr(m_backingStore.get()));
> 
> I suspect this method might cause a copy.

CFDataGetMutableBytePtr ?  The documentation says that's the way to write data into the CFMutatableData object.  I'm not sure how you'd be able to write the underly memory if this made a copy.

>> WebCore/platform/image-decoders/cg/ImageDecoderCG.cpp:40
>> +    m_size = other.m_size;
> 
> It seems very strange to me that these 3 are separate members.  Why does this need to be the case?

m_size is the width / height of the buffer in pixels.  m_backingStore knows the size of the buffer in bytes, but not what its geometry.  m_bytes is just a cached pointer to the underlying memory because we need to read it for every pixel write, which is extremely hot.  We don't want to go through CF for every pixel.

>> WebCore/platform/image-decoders/cg/ImageDecoderCG.cpp:60
>> +    m_bytes = reinterpret_cast<PixelData*>(CFDataGetMutableBytePtr(m_backingStore.get()));
> 
> Why do you need mutable pointers?

We need a mutable point because setRGBA writes to the memory.  We use CF to allocate the memory so that CGImage can hold onto it.  However, after allocating the memory, we write to directly.

>> WebCore/platform/image-decoders/cg/ImageDecoderCG.cpp:70
>> +    // FIXME: Figure out the right color space.
>> +    RetainPtr<CGColorSpaceRef> colorSpace(AdoptCF, CGColorSpaceCreateWithName(kCGColorSpaceGenericRGB));
> 
> I think you want DeviceRGB, not GenericRGB.  Generic will still to color-matching for you.  You want Device color spaces which do not do color matching as they're already in the colorspace of the device.

I'll research this, but let me try to confirm my understanding:

1) We have a bunch of bytes in an image file.
2) We have a device that transforms bytes in a given color space to a bunch of photons in a room.

What I'd like to say on this line of code is that the bytes in my image file are expressed as coordinates in a particular colorspace, the moral equivalent of sRGB on Windows.  Now, of course, that's probably a lie, but that's the lie I want to tell.

I'm fine with the operating system transforming the sRGB color space to a device specific color space (e.g., that the user has created to calibrate the photons emitted by their monitor).  My initial reading of the documentation was that DeviceRGB is a device-specific color space that's after this transformation.  Does that differ from your understanding?  (In either case, I'll go back an read some more.)

>> WebCore/platform/image-decoders/cg/ImageDecoderCG.cpp:76
>> +    return CGImageCreate(width(), height(), 8, 32, width() * sizeof(PixelData), colorSpace.get(),
>> +        alphaInfo | kCGBitmapByteOrder32Host, dataProvider.get(), 0, false, kCGRenderingIntentDefault);
> 
> Hmmm... My data my be old here, but I would have expected you to use CGBitmapImageCreate here.

I can't find a function by that name.  I'm looking at Table 11-1 of http://developer.apple.com/library/mac/#documentation/GraphicsImaging/Conceptual/drawingwithquartz2d/dq_images/dq_images.html#//apple_ref/doc/uid/TP30001066-CH212-TPXREF101

Do you mean CGBitmapContextCreateImage?  That seems a bit different: "Creates an image by copying the bits from a bitmap graphics context."  In particular, we don't want the copy.
Comment 31 Adam Barth 2010-10-22 14:57:05 PDT
Created attachment 71598 [details]
Patch
Comment 32 Adam Barth 2010-10-22 15:01:00 PDT
> >> WebCore/platform/image-decoders/cg/ImageDecoderCG.cpp:70
> >> +    // FIXME: Figure out the right color space.
> >> +    RetainPtr<CGColorSpaceRef> colorSpace(AdoptCF, CGColorSpaceCreateWithName(kCGColorSpaceGenericRGB));
> > 
> > I think you want DeviceRGB, not GenericRGB.  Generic will still to color-matching for you.  You want Device color spaces which do not do color matching as they're already in the colorspace of the device.
> 
> I'll research this, but let me try to confirm my understanding:
> 
> 1) We have a bunch of bytes in an image file.
> 2) We have a device that transforms bytes in a given color space to a bunch of photons in a room.
> 
> What I'd like to say on this line of code is that the bytes in my image file are expressed as coordinates in a particular colorspace, the moral equivalent of sRGB on Windows.  Now, of course, that's probably a lie, but that's the lie I want to tell.
> 
> I'm fine with the operating system transforming the sRGB color space to a device specific color space (e.g., that the user has created to calibrate the photons emitted by their monitor).  My initial reading of the documentation was that DeviceRGB is a device-specific color space that's after this transformation.  Does that differ from your understanding?  (In either case, I'll go back an read some more.)

Ok.  I've looked into this, both in the documentation in elsewhere in WebKit.  My understanding was a bit simplistic.  There's apparently a third layer hiding below.  WebKit generally looks to use CGColorSpaceCreateDeviceRGB(), which seems to have changed behavior after 10.4 to be the same as kCGColorSpaceGenericRGB, which is different than kCGColorSpaceSRGB.  I've updated the patch to use the same idiom as the example code I saw in WebKit.
Comment 33 WebKit Review Bot 2010-10-22 15:01:37 PDT
Attachment 71598 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1
WebCore/platform/image-decoders/ImageDecoder.h:190:  One space before end of line comments  [whitespace/comments] [5]
Total errors found: 1 in 14 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 34 Eric Seidel (no email) 2010-10-22 15:13:50 PDT
Comment on attachment 71598 [details]
Patch

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

Seems you have some style errors.  I won't be able to r+ this because I feel this is a policy decision for the chromium port to make.

> WebCore/platform/image-decoders/ImageDecoder.h:92
> +        void copyReferenceToBitmapData(const RGBA32Buffer& other);

"other" not needed.

> WebCore/platform/image-decoders/cg/ImageDecoderCG.cpp:70
> +    RetainPtr<CGColorSpaceRef> colorSpace(AdoptCF, CGColorSpaceCreateDeviceRGB());

Seems this could be static, we don't need a new colorspace for each image.
Comment 35 Adam Barth 2010-10-22 15:20:50 PDT
Created attachment 71602 [details]
updated with style fixes
Comment 36 Adam Barth 2010-10-22 17:00:48 PDT
Committed r70369: <http://trac.webkit.org/changeset/70369>
Comment 37 Dimitri Glazkov (Google) 2010-10-23 09:25:55 PDT
(In reply to comment #36)
> Committed r70369: <http://trac.webkit.org/changeset/70369>

We might have to roll this one out and let it cook a bit more. I started looking at the expectation changes, and there are new artifacts on the page that weren't there before, like:

http://build.chromium.org/buildbot/layout_test_results/webkit-rel-mac-webkit-org/results/layout-test-results/fast/inline-block/001-actual.png

vs.

http://trac.webkit.org/export/70391/trunk/LayoutTests/platform/mac/fast/inline-block/001-expected.png

See it here: http://test-results.appspot.com/dashboards/flakiness_dashboard.html#tests=fast%2Finline-block%2F001.html&showExpectations=true&useWebKitCanary=true 

I am sure it's just some mis-interaction of the pixel dump with the new decoders, but until we're sure, we shouldn't be rolling this into Chromium.
Comment 38 Adam Barth 2010-10-23 09:37:57 PDT
Do we have those problems on win or linux?
Comment 39 Dimitri Glazkov (Google) 2010-10-23 09:40:09 PDT
(In reply to comment #38)
> Do we have those problems on win or linux?

Nope.
Comment 40 Dimitri Glazkov (Google) 2010-10-23 10:05:48 PDT
Reverted r70369 for reason:

Caused weird artifacts in expected results.

Committed r70394: <http://trac.webkit.org/changeset/70394>
Comment 41 Adam Barth 2010-10-27 20:03:29 PDT
I regenerated new pixel results on my machine and looked through all 2500 image mismatches (this includes Leopard / Snow Leopard diff and the diffs from this patch).

I can't reproduce that artifact.  With the patch we fail these tests:

fast/css/color-correction-on-backgrounds.html
fast/css/color-correction.html
fast/image/pdf-as-background.html
svg/W3C-SVG-1.1/struct-image-03-t.svg

which seems expected b/c we don't have color correction yet.  I certainly could have missed a needle in the haystack, however.
Comment 42 Adam Barth 2010-10-27 20:27:28 PDT
AFACT, this patch is ready to land.  However, I can't even find the chromium canary bots on the FYI waterfall, much less see if they are green.
Comment 43 Peter Kasting 2010-10-28 11:49:48 PDT
(In reply to comment #42)
> AFACT, this patch is ready to land.  However, I can't even find the chromium canary bots on the FYI waterfall, much less see if they are green.

See the thread "Changes to the chromium buildbots and build.chromium.org" on chromium-dev.

The canaries now have a dedicated waterfall at http://build.chromium.org/p/chromium.webkit/waterfall .
Comment 44 Adam Barth 2010-10-28 13:42:36 PDT
> The canaries now have a dedicated waterfall at http://build.chromium.org/p/chromium.webkit/waterfall .

Cool.  That's much nicer than the tinyurl I had been using.
Comment 45 Adam Barth 2010-10-28 23:56:06 PDT
Committed r70846: <http://trac.webkit.org/changeset/70846>
Comment 46 Adam Barth 2010-10-29 03:12:12 PDT
Committed r70858: <http://trac.webkit.org/changeset/70858>