Bug 91794 - AX: Calls to AXObjectCache should prefer Node over Renderer
Summary: AX: Calls to AXObjectCache should prefer Node over Renderer
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Accessibility (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Dominic Mazzoni
URL:
Keywords:
Depends on: 87899
Blocks: 50126
  Show dependency treegraph
 
Reported: 2012-07-19 15:52 PDT by Dominic Mazzoni
Modified: 2012-08-15 14:31 PDT (History)
8 users (show)

See Also:


Attachments
Patch (67.05 KB, patch)
2012-07-26 00:36 PDT, Dominic Mazzoni
no flags Details | Formatted Diff | Diff
Fix Chromium port build (67.50 KB, patch)
2012-07-26 08:08 PDT, Dominic Mazzoni
no flags Details | Formatted Diff | Diff
Fix Qt port build (67.60 KB, patch)
2012-07-26 08:52 PDT, Dominic Mazzoni
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from gce-cr-linux-07 (645.83 KB, application/zip)
2012-07-26 10:04 PDT, WebKit Review Bot
no flags Details
Patch (68.83 KB, patch)
2012-08-06 23:39 PDT, Dominic Mazzoni
no flags Details | Formatted Diff | Diff
Patch (66.57 KB, patch)
2012-08-13 23:13 PDT, Dominic Mazzoni
no flags Details | Formatted Diff | Diff
Patch (66.61 KB, patch)
2012-08-14 11:12 PDT, Dominic Mazzoni
no flags Details | Formatted Diff | Diff
Patch (68.00 KB, patch)
2012-08-15 00:05 PDT, Dominic Mazzoni
no flags Details | Formatted Diff | Diff
Patch (68.00 KB, patch)
2012-08-15 00:22 PDT, Dominic Mazzoni
no flags Details | Formatted Diff | Diff
Patch (68.14 KB, patch)
2012-08-15 13:04 PDT, Dominic Mazzoni
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Dominic Mazzoni 2012-07-19 15:52:17 PDT
Once https://bugs.webkit.org/show_bug.cgi?id=87899 lands, the accessibility module will create an AccessibilityNodeObject for each element in a canvas subtree. However, these elements aren't able to receive notifications and might become stale, because notifications only get sent to AXObjectCache for nodes that have a renderer. All calls to AXObjectCache should be changed to specify a node instead of a renderer to fix this.
Comment 1 Dominic Mazzoni 2012-07-19 16:05:19 PDT
Here's one test we should add along with this patch:
* Create an element inside a canvas
* Access its AccessibilityNodeObject
* Reparent it to an element outside the canvas, so it has a renderer
* Now access its AccessibilityRenderObject.
* Make sure the AccessibilityNodeObject get freed and also deleted from the Node->Object hash map.
Comment 2 Dominic Mazzoni 2012-07-26 00:36:13 PDT
Created attachment 154563 [details]
Patch
Comment 3 WebKit Review Bot 2012-07-26 00:49:12 PDT
Comment on attachment 154563 [details]
Patch

Attachment 154563 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/13346633
Comment 4 Early Warning System Bot 2012-07-26 01:06:30 PDT
Comment on attachment 154563 [details]
Patch

Attachment 154563 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/13344647
Comment 5 Gyuyoung Kim 2012-07-26 01:07:24 PDT
Comment on attachment 154563 [details]
Patch

Attachment 154563 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/13349608
Comment 6 Early Warning System Bot 2012-07-26 01:17:04 PDT
Comment on attachment 154563 [details]
Patch

Attachment 154563 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/13343681
Comment 7 Dominic Mazzoni 2012-07-26 08:08:33 PDT
Created attachment 154655 [details]
Fix Chromium port build
Comment 8 Early Warning System Bot 2012-07-26 08:25:57 PDT
Comment on attachment 154655 [details]
Fix Chromium port build

Attachment 154655 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/13353759
Comment 9 Gyuyoung Kim 2012-07-26 08:43:37 PDT
Comment on attachment 154655 [details]
Fix Chromium port build

Attachment 154655 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/13343886
Comment 10 Dominic Mazzoni 2012-07-26 08:52:07 PDT
Created attachment 154661 [details]
Fix Qt port build
Comment 11 WebKit Review Bot 2012-07-26 10:04:48 PDT
Comment on attachment 154661 [details]
Fix Qt port build

Attachment 154661 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/13341842

New failing tests:
accessibility/canvas-fallback-content.html
fast/events/tabindex-focus-blur-all.html
Comment 12 WebKit Review Bot 2012-07-26 10:04:52 PDT
Created attachment 154677 [details]
Archive of layout-test-results from gce-cr-linux-07

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: gce-cr-linux-07  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.39-gcg-201203291735-x86_64-with-Ubuntu-10.04-lucid
Comment 13 Dominic Mazzoni 2012-08-06 23:39:23 PDT
Created attachment 156877 [details]
Patch
Comment 14 Mario Sanchez Prada 2012-08-07 02:29:31 PDT
I have a doubt about this change: if we only emit notifications for nodes instead of renderers, aren't we missing notifications for those objects that have a representation only in the RenderObject tree but not in the DOM tree? (e.g. objects in Shadow DOM trees)
Comment 15 Darin Adler 2012-08-07 14:27:25 PDT
Comment on attachment 156877 [details]
Patch

This seems like it might cause us a step backwards in our ability to deal with generated content, which has render objects but not nodes. What did we do to prevent that?
Comment 16 Dominic Mazzoni 2012-08-07 15:24:16 PDT
Darin and Mario,

To answer both of your questions, this change won't affect any code that sent an accessibility notification on a RenderObject that didn't have an associated Node. All I changed were lines of this form:

From:
axObjectCache()->postNotification(node->renderer(), AXValueChanged, true);

To:
axObjectCache()->postNotification(node, AXValueChanged, true);

Similarly, inside accessibility/ I changed a lot of calls of this form:

From:
return axObjectCache()->getOrCreate(node->renderer());

To:
return axObjectCache()->getOrCreate(node);

In every case, the renderer was accessed via a node - all I did was make it call AXObjectCache using the node instead. The first thing AXObjectCache does is see if that node has a renderer - and if it does, it falls back on the previous behavior. However, if there's a node without a renderer (like in the canvas subtree), it can now send accessibility notifications.

There's still some code that sends accessibility notifications on renderers that aren't associated with a node, and I didn't touch any of that code. I think it's unlikely that very much of that code will be needed in a canvas subtree.

Please let me know if that answers your concerns!
Comment 17 Mario Sanchez Prada 2012-08-09 10:44:40 PDT
(In reply to comment #16)
> [...]
> Please let me know if that answers your concerns!

It does, at least for me (and I guess for Darin too). And yes, I agree there seems not to be any problem with this approach since it looks like everything should keep working as it used to do it before, according to your explanation.

I'm now on a flaky connection and so not the best environment for working :-), but I'll try to save some time tomorrow to help with an informal review of the patch, if possible.

Thanks for the clarification
Comment 18 Mario Sanchez Prada 2012-08-10 15:07:54 PDT
Comment on attachment 156877 [details]
Patch

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

Sorry for the delay. Crazy Friday here :-)

I informally reviewed your patch and it looks good to me overall.
I just have some really minor comments. See them inline below...

> Source/WebCore/accessibility/AXObjectCache.cpp:314
> +    AccessibilityObject* rendererObj = getOrCreate(node->renderer());

I think you should move this line down, right before the first place where rendererObj is used. Also, I think you could re-write this so the scope of nodeObj and rendererObj variables is the minimum required (just to return them if they are not null). Something like this:

   [...]
   if (AccessibilityObject* nodeObj = get(node))
       return nodeObj;

   if (AccessibilityObject* rendererObj = getOrCreate(node->renderer()))
       return rendererObj;
   [...]

> Source/WebCore/accessibility/AXObjectCache.cpp:531
> +        object->contentChanged(); 

Nit. I would probably rewrite it as this:

   if (AccessibilityObject* object = getOrCreate(node))
       object->contentChanged();

> Source/WebCore/accessibility/AXObjectCache.cpp:552
> +        obj->childrenChanged();

Same (nit) here :)

> Source/WebCore/accessibility/AXObjectCache.cpp:618
> +    if (!node)

Shouldn't we return here also if object is null? After all it's used in the call to postNotification.
(I think the same issue would apply to the previous implementation of postNotification for RenderObject*)

> Source/WebCore/accessibility/AXObjectCache.cpp:701
>          obj->handleAriaExpandedChanged();

Same nit again. Maybe it's better something like this?

   if (AccessibilityObject* obj = getOrCreate(node))
       obj->handleAriaExpandedChanged();

> Source/WebCore/accessibility/AXObjectCache.cpp:708
>          obj->handleActiveDescendantChanged();

Ditto.

> Source/WebCore/accessibility/AXObjectCache.cpp:715
> +        static_cast<AccessibilityNodeObject*>(obj)->updateAccessibilityRole();

I think you should use toAccessibilityNodeObject(obj) instead of the static_cast here.

> Source/WebCore/accessibility/AXObjectCache.cpp:782
> +        if (nodeIsTextControl(const_cast<Element*>(element)))

I think you should move this const_cast to nodeIsTextControl and leage this line as it was before?

> Source/WebCore/accessibility/AXObjectCache.cpp:789
> +bool AXObjectCache::nodeIsTextControl(Node* node)

I would leave this const here, as it looks like it makes sense

> Source/WebCore/accessibility/AXObjectCache.cpp:794
> +    const AccessibilityObject* axObject = getOrCreate(node);

Considering the changes I proposed with regarding to const in the previous comments, I would use a const_cast<Node*>(node) here, to use getOrCreate() for the node

> Source/WebCore/accessibility/AXObjectCache.h:127
> +    bool nodeIsTextControl(Node*);

If you accept my suggestion of the const cast, you'll need to update this too

> Source/WebCore/accessibility/AccessibilityRenderObject.cpp:2649
> +bool AccessibilityRenderObject::nodeIsTextControl(Node* node) const

Keep const here

> Source/WebCore/accessibility/AccessibilityRenderObject.h:283
> +    bool nodeIsTextControl(Node*) const;

Ditto

> Source/WebCore/bindings/cpp/WebDOMCustomVoidCallback.cpp:49
> +}

I think this is a mistake in the patch?

> Source/WebCore/dom/Node.cpp:1289
> +      doc->axObjectCache()->attached(this);

Why not using m_document directly?

Also, I'm not 100% sure (I never am!), but I think m_document can never be null for a Node, so you might save one check here

> LayoutTests/platform/gtk/TestExpectations:597
> +BUGWKGTK : accessibility/canvas-fallback-content.html = TEXT

Question: I think the main reason for skipping this test is that the roles for GTK do not match those in the expected file (so I would need to generate later on specific expected results for GTK), right?
Comment 19 Dominic Mazzoni 2012-08-13 23:13:45 PDT
Created attachment 158227 [details]
Patch
Comment 20 Dominic Mazzoni 2012-08-13 23:23:29 PDT
Comment on attachment 156877 [details]
Patch

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

>> Source/WebCore/accessibility/AXObjectCache.cpp:314
>> +    AccessibilityObject* rendererObj = getOrCreate(node->renderer());
> 
> I think you should move this line down, right before the first place where rendererObj is used. Also, I think you could re-write this so the scope of nodeObj and rendererObj variables is the minimum required (just to return them if they are not null). Something like this:
> 
>    [...]
>    if (AccessibilityObject* nodeObj = get(node))
>        return nodeObj;
> 
>    if (AccessibilityObject* rendererObj = getOrCreate(node->renderer()))
>        return rendererObj;
>    [...]

Good catch - actually it looks like the changes to this method aren't needed at all, let's leave it the way it was, which was cleaner than this.

>> Source/WebCore/accessibility/AXObjectCache.cpp:531
>> +        object->contentChanged(); 
> 
> Nit. I would probably rewrite it as this:
> 
>    if (AccessibilityObject* object = getOrCreate(node))
>        object->contentChanged();

Sure, changed throughout.

>> Source/WebCore/accessibility/AXObjectCache.cpp:618
>> +    if (!node)
> 
> Shouldn't we return here also if object is null? After all it's used in the call to postNotification.
> (I think the same issue would apply to the previous implementation of postNotification for RenderObject*)

No, that couldn't happen - if object is null, the while loop above wouldn't have terminated - unless node is null, in which case we return here.

>> Source/WebCore/accessibility/AXObjectCache.cpp:715
>> +        static_cast<AccessibilityNodeObject*>(obj)->updateAccessibilityRole();
> 
> I think you should use toAccessibilityNodeObject(obj) instead of the static_cast here.

Good idea, done.

>> Source/WebCore/accessibility/AXObjectCache.cpp:794
>> +    const AccessibilityObject* axObject = getOrCreate(node);
> 
> Considering the changes I proposed with regarding to const in the previous comments, I would use a const_cast<Node*>(node) here, to use getOrCreate() for the node

Makes sense.

>> Source/WebCore/bindings/cpp/WebDOMCustomVoidCallback.cpp:49
>> +}
> 
> I think this is a mistake in the patch?

Yes, thanks.

>> Source/WebCore/dom/Node.cpp:1289
>> +      doc->axObjectCache()->attached(this);
> 
> Why not using m_document directly?
> 
> Also, I'm not 100% sure (I never am!), but I think m_document can never be null for a Node, so you might save one check here

I'm just copying the pattern above. :)  I'm not 100% sure either so I'd rather keep this code safe rather than add an unsafe op to this large change.

>> LayoutTests/platform/gtk/TestExpectations:597
>> +BUGWKGTK : accessibility/canvas-fallback-content.html = TEXT
> 
> Question: I think the main reason for skipping this test is that the roles for GTK do not match those in the expected file (so I would need to generate later on specific expected results for GTK), right?

That's correct. If the output is all PASS you can just generate an expectations file.

There's a possibility there will be other differences, like we discovered with the last test I added. It's not easy to write a cross-platform accessibility test. :)
Comment 21 Mario Sanchez Prada 2012-08-14 01:25:35 PDT
Comment on attachment 156877 [details]
Patch

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

Thanks for the comments and for attaching a new patch, Dominic.

I'll try to (informally) review the new one asap too.

>>> Source/WebCore/accessibility/AXObjectCache.cpp:314
>>> +    AccessibilityObject* rendererObj = getOrCreate(node->renderer());
>> 
>> I think you should move this line down, right before the first place where rendererObj is used. Also, I think you could re-write this so the scope of nodeObj and rendererObj variables is the minimum required (just to return them if they are not null). Something like this:
>> 
>>    [...]
>>    if (AccessibilityObject* nodeObj = get(node))
>>        return nodeObj;
>> 
>>    if (AccessibilityObject* rendererObj = getOrCreate(node->renderer()))
>>        return rendererObj;
>>    [...]
> 
> Good catch - actually it looks like the changes to this method aren't needed at all, let's leave it the way it was, which was cleaner than this.

Great, even better :-)

>>> Source/WebCore/accessibility/AXObjectCache.cpp:618
>>> +    if (!node)
>> 
>> Shouldn't we return here also if object is null? After all it's used in the call to postNotification.
>> (I think the same issue would apply to the previous implementation of postNotification for RenderObject*)
> 
> No, that couldn't happen - if object is null, the while loop above wouldn't have terminated - unless node is null, in which case we return here.

Ah! It's true. Sorry for the noise

>>> Source/WebCore/dom/Node.cpp:1289
>>> +      doc->axObjectCache()->attached(this);
>> 
>> Why not using m_document directly?
>> 
>> Also, I'm not 100% sure (I never am!), but I think m_document can never be null for a Node, so you might save one check here
> 
> I'm just copying the pattern above. :)  I'm not 100% sure either so I'd rather keep this code safe rather than add an unsafe op to this large change.

Hehe... I know that feeling very well, so I won't push you harder :-)

I'm fine with you leaving that just-in-case extra check there.

>>> LayoutTests/platform/gtk/TestExpectations:597
>>> +BUGWKGTK : accessibility/canvas-fallback-content.html = TEXT
>> 
>> Question: I think the main reason for skipping this test is that the roles for GTK do not match those in the expected file (so I would need to generate later on specific expected results for GTK), right?
> 
> That's correct. If the output is all PASS you can just generate an expectations file.
> 
> There's a possibility there will be other differences, like we discovered with the last test I added. It's not easy to write a cross-platform accessibility test. :)

Yes, it's not easy at all. The fact that role names are different in each platform (so things like shouldBe(axObject.role, 'Role: AXLink') won't work in mac and gtk at the same time) and also that the final a11y hierarchy exposed is not exactly the same (e.g. we "flatten"/"merge" some parts for GTK) makes it very difficult and one of the reasons why we have so many tests (and not just expectations) in platform/<platform>/accessibility.

So I empathize with you on this too, and I'm fine with this bit in your patch
Comment 22 Mario Sanchez Prada 2012-08-14 01:36:13 PDT
Comment on attachment 158227 [details]
Patch

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

This patch looks good to me, so I guess now it's time to ask for an official review over it (Chris?).

See below a couple (just two!) of really minor issues for you to consider in the next iteration, or just when landing if you get the r+.

> Source/WebCore/accessibility/AXObjectCache.cpp:315
> +  

This looks just like a mistake

> Source/WebCore/accessibility/AXObjectCache.cpp:547
> +        obj->childrenChanged();

Same suggestion (nit) than in my previous review:
    if (AccessibilityObject* obj = get(node))
        obj->childrenChanged();
Comment 23 Dominic Mazzoni 2012-08-14 11:12:30 PDT
Comment on attachment 158227 [details]
Patch

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

>> Source/WebCore/accessibility/AXObjectCache.cpp:315
>> +  
> 
> This looks just like a mistake

Fixed.

>> Source/WebCore/accessibility/AXObjectCache.cpp:547
>> +        obj->childrenChanged();
> 
> Same suggestion (nit) than in my previous review:
>     if (AccessibilityObject* obj = get(node))
>         obj->childrenChanged();

Sorry, missed this one. Found a couple more to fix too.
Comment 24 Dominic Mazzoni 2012-08-14 11:12:57 PDT
Created attachment 158373 [details]
Patch
Comment 25 chris fleizach 2012-08-14 17:08:24 PDT
Comment on attachment 158373 [details]
Patch

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

one concern is the textMarker Node cache. Is that getting conflated with the other node cache

Should we make the get and getOrCreate(RenderObject*) methods private now since we expect other callers to use the node methods?

> Source/WebCore/accessibility/AXObjectCache.cpp:705
> +        toAccessibilityNodeObject(obj)->updateAccessibilityRole();

This seems like it could be called on AccessibilityObject. There might be a mock element who wants to update its role. that could avoid this node check

> Source/WebCore/dom/Node.cpp:417
> +        doc->axObjectCache()->remove(this);

we still need the removeNodeForUse() call because i think that's a different cache being used for Node being used in text markers.
If the cache is the same, then we should remove the *NodeForUse methods which ensure that text markers are still valid

> Source/WebCore/dom/Node.cpp:1289
> +        doc->axObjectCache()->attached(this);

attached it past tense, i think it should be attach() for present tense
Comment 26 Dominic Mazzoni 2012-08-14 23:54:40 PDT
Comment on attachment 158373 [details]
Patch

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

> one concern is the textMarker Node cache. Is that getting conflated with the other node cache

The text marker Node cache still exists, it's now just private to AXObjectCache - nodes get added to it in textMarkerDataForVisiblePosition(...), and removed in remove(Node*) - which is now called whenever removeNodeForUse(Node*) was called before.

> Should we make the get and getOrCreate(RenderObject*) methods private now since we expect other callers to use the node methods?

AccessibilityRenderObject still makes use of getOrCreate(RenderObject*) in quite a few places, and they look reasonable. We could make these methods private but make AccessibilityRenderObject a friend class? Or we could move those methods to the bottom of AXObjectCache with a comment that they shouldn't be used from outside the accessibility/ directory.

>> Source/WebCore/accessibility/AXObjectCache.cpp:705
>> +        toAccessibilityNodeObject(obj)->updateAccessibilityRole();
> 
> This seems like it could be called on AccessibilityObject. There might be a mock element who wants to update its role. that could avoid this node check

Good idea. I made updateAccessibilityRole into a virtual method in AccessibilityObject.

>> Source/WebCore/dom/Node.cpp:417
>> +        doc->axObjectCache()->remove(this);
> 
> we still need the removeNodeForUse() call because i think that's a different cache being used for Node being used in text markers.
> If the cache is the same, then we should remove the *NodeForUse methods which ensure that text markers are still valid

I just made remove(Node*) call removeNodeForUse. See above.

>> Source/WebCore/dom/Node.cpp:1289
>> +        doc->axObjectCache()->attached(this);
> 
> attached it past tense, i think it should be attach() for present tense

Done
Comment 27 Dominic Mazzoni 2012-08-15 00:05:27 PDT
Created attachment 158512 [details]
Patch
Comment 28 Gyuyoung Kim 2012-08-15 00:14:11 PDT
Comment on attachment 158512 [details]
Patch

Attachment 158512 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/13494880
Comment 29 Early Warning System Bot 2012-08-15 00:18:27 PDT
Comment on attachment 158512 [details]
Patch

Attachment 158512 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/13494884
Comment 30 Early Warning System Bot 2012-08-15 00:18:52 PDT
Comment on attachment 158512 [details]
Patch

Attachment 158512 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/13509083
Comment 31 Dominic Mazzoni 2012-08-15 00:22:47 PDT
Created attachment 158516 [details]
Patch
Comment 32 chris fleizach 2012-08-15 11:11:34 PDT
Comment on attachment 158516 [details]
Patch

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

i think everything else look ok. just the attach() method confuses me

> Source/WebCore/accessibility/AXObjectCache.cpp:540
> +    get(node);

this comment is a bit confusing for me.  if the node has been reparented it looks like it will be removed from the cache in get(), which would be the opposite of attach
Comment 33 Dominic Mazzoni 2012-08-15 13:00:43 PDT
Comment on attachment 158516 [details]
Patch

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

>> Source/WebCore/accessibility/AXObjectCache.cpp:540
>> +    get(node);
> 
> this comment is a bit confusing for me.  if the node has been reparented it looks like it will be removed from the cache in get(), which would be the opposite of attach

The purpose of this method is to give a chance for AXObjectCache to update, after a node has been attached. I didn't mean to imply that AXObjectCache should be attaching the node - it's a notification that the node was attached, not a command to attach.

The reason we do this on attach is that at the time the node is re-inserted into the tree, it's not clear at that point if it will have a renderer at that new location in the tree, because the style calculation hasn't happened yet. But after Node::attach(), the style calculation is done and the node will either have a renderer or not - so that's the best time for AXObjectCache to make sure that it doesn't have a stale AccessibilityNodeObject that's associated with a node that now has a renderer.

I renamed the method updateCacheAfterNodeIsAttached(Node*), does that make more sense?
Comment 34 Dominic Mazzoni 2012-08-15 13:04:03 PDT
Created attachment 158622 [details]
Patch
Comment 35 chris fleizach 2012-08-15 13:37:42 PDT
Comment on attachment 158622 [details]
Patch

r=me
Comment 36 WebKit Review Bot 2012-08-15 14:31:51 PDT
Comment on attachment 158622 [details]
Patch

Clearing flags on attachment: 158622

Committed r125710: <http://trac.webkit.org/changeset/125710>
Comment 37 WebKit Review Bot 2012-08-15 14:31:58 PDT
All reviewed patches have been landed.  Closing bug.