Bug 98457 - ScrollingStateNodes should be referenced via IDs on RenderLayerBacking
Summary: ScrollingStateNodes should be referenced via IDs on RenderLayerBacking
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Beth Dakin
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-10-04 15:20 PDT by Beth Dakin
Modified: 2012-10-10 20:39 PDT (History)
14 users (show)

See Also:


Attachments
Patch (30.99 KB, patch)
2012-10-04 15:46 PDT, Beth Dakin
webkit-ews: commit-queue-
Details | Formatted Diff | Diff
Patch that should build (34.25 KB, patch)
2012-10-04 17:29 PDT, Beth Dakin
simon.fraser: review-
peter+ews: commit-queue-
Details | Formatted Diff | Diff
Patch (35.38 KB, patch)
2012-10-07 14:07 PDT, Beth Dakin
gyuyoung.kim: commit-queue-
Details | Formatted Diff | Diff
Patch (35.38 KB, patch)
2012-10-07 19:59 PDT, Beth Dakin
buildbot: commit-queue-
Details | Formatted Diff | Diff
Patch (35.40 KB, patch)
2012-10-08 11:20 PDT, Beth Dakin
peter+ews: commit-queue-
Details | Formatted Diff | Diff
Patch (34.83 KB, patch)
2012-10-08 11:52 PDT, Beth Dakin
webkit.review.bot: commit-queue-
Details | Formatted Diff | Diff
Patch (34.87 KB, patch)
2012-10-08 13:09 PDT, Beth Dakin
simon.fraser: review-
Details | Formatted Diff | Diff
Patch (35.47 KB, patch)
2012-10-08 14:58 PDT, Beth Dakin
no flags Details | Formatted Diff | Diff
Patch (37.48 KB, patch)
2012-10-08 19:36 PDT, Beth Dakin
simon.fraser: review+
webkit.review.bot: commit-queue-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Beth Dakin 2012-10-04 15:20:59 PDT
The next step to getting the ScrollingTree into shape where it can take multiple nodes is to associate RenderLayerBackings with ScrollingStateNodes.
Comment 1 Beth Dakin 2012-10-04 15:46:48 PDT
Created attachment 167191 [details]
Patch
Comment 2 Early Warning System Bot 2012-10-04 15:52:29 PDT
Comment on attachment 167191 [details]
Patch

Attachment 167191 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/14152687
Comment 3 Peter Beverloo (cr-android ews) 2012-10-04 15:55:44 PDT
Comment on attachment 167191 [details]
Patch

Attachment 167191 [details] did not pass cr-android-ews (chromium-android):
Output: http://queues.webkit.org/results/14171445
Comment 4 Gyuyoung Kim 2012-10-04 16:05:04 PDT
Comment on attachment 167191 [details]
Patch

Attachment 167191 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/14173327
Comment 5 Early Warning System Bot 2012-10-04 16:41:51 PDT
Comment on attachment 167191 [details]
Patch

Attachment 167191 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/14183158
Comment 6 Build Bot 2012-10-04 16:56:37 PDT
Comment on attachment 167191 [details]
Patch

Attachment 167191 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/14176279
Comment 7 James Robinson 2012-10-04 17:00:40 PDT
Do you have to make the caller of ScrollingCoordinator aware of the ScrollingStateNodes?  I thought it was the ScrollingCoordinator's job to deal with these mappings.
Comment 8 Beth Dakin 2012-10-04 17:02:39 PDT
(In reply to comment #7)
> Do you have to make the caller of ScrollingCoordinator aware of the ScrollingStateNodes?  I thought it was the ScrollingCoordinator's job to deal with these mappings.

I am trying to keep it mostly internal to ScrollingCoordinator. The actual ScrollingStateNodes are definitely contained within the class still. Is there something in particular that you think violates that?
Comment 9 Beth Dakin 2012-10-04 17:04:24 PDT
(In reply to comment #7)
> Do you have to make the caller of ScrollingCoordinator aware of the ScrollingStateNodes?  I thought it was the ScrollingCoordinator's job to deal with these mappings.

Maybe you are referring to these functions:

    void detachFromStateTree(RenderLayerBacking*);
    void clearStateTree();

Perhaps it would be more appropriate to re-name those so that they don't reference the state tree.
Comment 10 Beth Dakin 2012-10-04 17:29:59 PDT
Created attachment 167215 [details]
Patch that should build

AKA ERMAHGERD THE ERF DERFS

Maybe there is a better way to get this building, but ScrollingCoordinator already seems like such a mess with the if-defs that I'm not really sure what the best choice is.
Comment 11 Peter Beverloo (cr-android ews) 2012-10-04 17:40:05 PDT
Comment on attachment 167215 [details]
Patch that should build

Attachment 167215 [details] did not pass cr-android-ews (chromium-android):
Output: http://queues.webkit.org/results/14175308
Comment 12 Simon Fraser (smfr) 2012-10-04 17:47:56 PDT
Comment on attachment 167215 [details]
Patch that should build

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

> Source/WebCore/dom/Document.cpp:4064
>              v->cacheCurrentScrollPosition();
> -            if (page() && page()->mainFrame() == m_frame)
> +            if (page && page->mainFrame() == m_frame) {
>                  v->resetScrollbarsAndClearContentsSize();
> -            else
> +                if (ScrollingCoordinator* scrollingCoordinator = page->scrollingCoordinator())
> +                    scrollingCoordinator->clearStateTree();
> +            } else

Maybe this should be done in a separate patch? This patch is big enough already.

> Source/WebCore/page/scrolling/ScrollingCoordinator.cpp:255
> +    // ensureRootStateNodeForFrameView() can fail if, for example, the FrameView does not have a
> +    // RenderLayerBacking. If that is the case, then we should not do any more work here.

The comment would be more useful if it said when this might happen (to allow a developer to reproduce the scenario).

> Source/WebCore/page/scrolling/ScrollingCoordinator.cpp:462
> +static RenderLayerBacking* backingForFrameView(FrameView* frameView)

I'm not sure why SC ever needs to know about RLB.

> Source/WebCore/page/scrolling/ScrollingCoordinator.cpp:503
> +void ScrollingCoordinator::detachFromStateTree(RenderLayerBacking* backing)

Why not just pass the ID in here?

> Source/WebCore/page/scrolling/ScrollingCoordinator.cpp:549
> +    if (!rootLayerID) {
> +        rootLayerID = generateScrollLayerID();
> +        backing->setScrollLayerID(rootLayerID);
> +    }

I think this code should live in RLB.

> Source/WebCore/rendering/RenderLayer.cpp:4462
> +        if (isRootLayer() && compositor()->scrollLayer()) {
> +            if (Frame* frame = renderer()->frame()) {
> +                if (Page* page = frame->page()) {
> +                    if (ScrollingCoordinator* scrollingCoordinator = page->scrollingCoordinator())
> +                        scrollingCoordinator->frameViewRootLayerDidChange(frame->view());
> +                }
> +            }
> +        }

Seems like an odd place to put this. I think we should try to keep all of the calls into SC in RLC.
Comment 13 Simon Fraser (smfr) 2012-10-04 17:54:28 PDT
http://trac.webkit.org/changeset/130439
Comment 14 James Robinson 2012-10-05 15:16:41 PDT
ScrollingCoordinator is really kind of a mess.  What would you think about making the interface just a vtable and separating the implementation bits into separate subclasses instead of one implemenation that's all sliced up to hell?
Comment 15 Beth Dakin 2012-10-07 13:27:10 PDT
(In reply to comment #13)
> http://trac.webkit.org/changeset/130439

Simon, did you mean to paste this somewhere else?
Comment 16 Beth Dakin 2012-10-07 13:34:37 PDT
(In reply to comment #14)
> ScrollingCoordinator is really kind of a mess.  What would you think about making the interface just a vtable and separating the implementation bits into separate subclasses instead of one implemenation that's all sliced up to hell?

I definitely agree that ScrollingCoordinator is a huge mess. I didn't realize how bad it was until I wrote this patch. I don't love the idea of virtualization here because we would be adding run-time cost for something that is known at compile-time. This may not actually be a performance-critical area, but I still think it's a best-practice that is worth sticking to.

What could work instead would be to make ScrollingCoordinator.h a very small file that looks something like this:

#if PLATFORM(MAC)
#include "ScrollingCoordinatorMac.h"
typedef ScrollingCoordinatorMac ScrollingCoordinator;
#endif

#if PLATFORM(CHROMIUM)
#include "ScrollingCoordinatorChromium.h"
typedef ScrollingCoordiantorChromium ScrollingCoordinator;
#endif

// Other platforms here.

And then ScrollingCoordinatorMac and ScrollingCoordinatorChromium could inherit from a ScrollingCoordinatorBase that could contain any legitimately shared functionality.

At this point, I do think something like that would be greatly preferable to this rat's nest, but some of the forking is unfortunate. I guess we're already in that state though.

I'm not going to focus on this re-structuring for this patch because it will be a big task on its own. But I do think we should do this.
Comment 17 Beth Dakin 2012-10-07 13:39:49 PDT
(In reply to comment #12)
> (From update of attachment 167215 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=167215&action=review
> 
> > Source/WebCore/dom/Document.cpp:4064
> >              v->cacheCurrentScrollPosition();
> > -            if (page() && page()->mainFrame() == m_frame)
> > +            if (page && page->mainFrame() == m_frame) {
> >                  v->resetScrollbarsAndClearContentsSize();
> > -            else
> > +                if (ScrollingCoordinator* scrollingCoordinator = page->scrollingCoordinator())
> > +                    scrollingCoordinator->clearStateTree();
> > +            } else
> 
> Maybe this should be done in a separate patch? This patch is big enough already.
> 

This code is required in this patch because this patch also makes it so ScrollingStateTree does not automatically create a root node upon its own creation. With that change in place, this code is required to avoid crashing. Both of these things could technically be done in another patch, but there would have to be some weird temporary code that accounts for the fact that we would always creat root state node that is not ready to be ScrollingCoordinator's HashMap since it hasn't been associated with a FrameView. I don't think it's the right trade-off to do this work in a separate patch, so I'm going to leave it in for now.

> > Source/WebCore/page/scrolling/ScrollingCoordinator.cpp:255
> > +    // ensureRootStateNodeForFrameView() can fail if, for example, the FrameView does not have a
> > +    // RenderLayerBacking. If that is the case, then we should not do any more work here.
> 
> The comment would be more useful if it said when this might happen (to allow a developer to reproduce the scenario).
> 

I changed this into an assertion since that is the most accurate comment.

> > Source/WebCore/page/scrolling/ScrollingCoordinator.cpp:462
> > +static RenderLayerBacking* backingForFrameView(FrameView* frameView)
> 
> I'm not sure why SC ever needs to know about RLB.
> 

I changed this so that it does not.

> > Source/WebCore/page/scrolling/ScrollingCoordinator.cpp:503
> > +void ScrollingCoordinator::detachFromStateTree(RenderLayerBacking* backing)
> 
> Why not just pass the ID in here?
> 

Fixed.

> > Source/WebCore/page/scrolling/ScrollingCoordinator.cpp:549
> > +    if (!rootLayerID) {
> > +        rootLayerID = generateScrollLayerID();
> > +        backing->setScrollLayerID(rootLayerID);
> > +    }
> 
> I think this code should live in RLB.
> 

Moved it.

> > Source/WebCore/rendering/RenderLayer.cpp:4462
> > +        if (isRootLayer() && compositor()->scrollLayer()) {
> > +            if (Frame* frame = renderer()->frame()) {
> > +                if (Page* page = frame->page()) {
> > +                    if (ScrollingCoordinator* scrollingCoordinator = page->scrollingCoordinator())
> > +                        scrollingCoordinator->frameViewRootLayerDidChange(frame->view());
> > +                }
> > +            }
> > +        }
> 
> Seems like an odd place to put this. I think we should try to keep all of the calls into SC in RLC.

Okay, I moved it back to RLC, and I put it after RLC's call to ensureBacking() so that we can be sure we have a backing.

Simon, I am re-opening this bug because I don't think you meant to close it.
Comment 18 Beth Dakin 2012-10-07 14:07:27 PDT
Created attachment 167487 [details]
Patch
Comment 19 Gyuyoung Kim 2012-10-07 14:12:47 PDT
Comment on attachment 167487 [details]
Patch

Attachment 167487 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/14205437
Comment 20 Early Warning System Bot 2012-10-07 14:13:35 PDT
Comment on attachment 167487 [details]
Patch

Attachment 167487 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/14204434
Comment 21 Early Warning System Bot 2012-10-07 14:16:25 PDT
Comment on attachment 167487 [details]
Patch

Attachment 167487 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/14205438
Comment 22 WebKit Review Bot 2012-10-07 14:16:26 PDT
Comment on attachment 167487 [details]
Patch

Attachment 167487 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/14221069
Comment 23 Peter Beverloo (cr-android ews) 2012-10-07 14:21:54 PDT
Comment on attachment 167487 [details]
Patch

Attachment 167487 [details] did not pass cr-android-ews (chromium-android):
Output: http://queues.webkit.org/results/14207331
Comment 24 Build Bot 2012-10-07 14:33:09 PDT
Comment on attachment 167487 [details]
Patch

Attachment 167487 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/14217137
Comment 25 Peter Beverloo (cr-android ews) 2012-10-07 15:24:04 PDT
Comment on attachment 167487 [details]
Patch

Attachment 167487 [details] did not pass cr-android-ews (chromium-android):
Output: http://queues.webkit.org/results/14211206
Comment 26 Beth Dakin 2012-10-07 19:59:17 PDT
Created attachment 167496 [details]
Patch
Comment 27 Build Bot 2012-10-07 20:09:00 PDT
Comment on attachment 167496 [details]
Patch

Attachment 167496 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/14204509
Comment 28 WebKit Review Bot 2012-10-07 20:09:37 PDT
Comment on attachment 167496 [details]
Patch

Attachment 167496 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/14201597
Comment 29 Peter Beverloo (cr-android ews) 2012-10-07 20:18:10 PDT
Comment on attachment 167496 [details]
Patch

Attachment 167496 [details] did not pass cr-android-ews (chromium-android):
Output: http://queues.webkit.org/results/14214200
Comment 30 Beth Dakin 2012-10-08 11:20:57 PDT
Created attachment 167570 [details]
Patch
Comment 31 Peter Beverloo (cr-android ews) 2012-10-08 11:40:30 PDT
Comment on attachment 167570 [details]
Patch

Attachment 167570 [details] did not pass cr-android-ews (chromium-android):
Output: http://queues.webkit.org/results/14211442
Comment 32 Beth Dakin 2012-10-08 11:52:48 PDT
Created attachment 167577 [details]
Patch
Comment 33 WebKit Review Bot 2012-10-08 12:59:01 PDT
Comment on attachment 167577 [details]
Patch

Attachment 167577 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/14215424
Comment 34 Peter Beverloo (cr-android ews) 2012-10-08 13:06:48 PDT
Comment on attachment 167577 [details]
Patch

Attachment 167577 [details] did not pass cr-android-ews (chromium-android):
Output: http://queues.webkit.org/results/14229209
Comment 35 Beth Dakin 2012-10-08 13:09:07 PDT
Created attachment 167592 [details]
Patch
Comment 36 Simon Fraser (smfr) 2012-10-08 14:07:02 PDT
Comment on attachment 167592 [details]
Patch

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

> Source/WebCore/loader/HistoryController.cpp:131
> +        Page* page = m_frame->page();
> +        if (page && page->mainFrame() == m_frame) {
> +            if (ScrollingCoordinator* scrollingCoordinator = page->scrollingCoordinator())
> +                scrollingCoordinator->frameViewRootLayerDidChange(view);
> +        }

This is a bit mysterious here, and it's a shame to call into ScrollingCoordinator from yet another file. What work does it need to do?

> Source/WebCore/page/FrameView.h:137
> +    uint64_t scrollLayerID() const;

Does this have to be public on FrameView? Can't FrameView pass its own ID into the SC?

> Source/WebCore/page/scrolling/ScrollingCoordinator.cpp:153
> +

Blanky.

> Source/WebCore/page/scrolling/ScrollingCoordinator.cpp:473
> +    ASSERT(node->isScrollingStateScrollingNode());
> +    return toScrollingStateScrollingNode(node);

toScrollingStateScrollingNode will ASSERT for you; no need to ASSERT here too.

> Source/WebCore/page/scrolling/ScrollingStateNode.cpp:92
> +    size_t size = m_children->size();
> +    for (size_t i = 0; i < size; ++i)
> +        m_children->at(i)->removeChild(node);

Looks like you're iterating the array while mutating it here.

> Source/WebCore/rendering/RenderLayerBacking.cpp:972
> +static uint64_t generateScrollLayerID()
> +{
> +    static uint64_t uniqueScrollLayerID = 1;
> +    return uniqueScrollLayerID++;
> +}

I think the ID generation should be on ScrollingCoordinator. Calling could look like:

m_id = scrollingCoordinator->attach(scrollingCoordinator->uniqueNodeID(), someConstraintsData)

where attach returns the ID passed in, for convenience.

> Source/WebCore/rendering/RenderLayerBacking.cpp:978
> +void RenderLayerBacking::attachToScrollingCoordinator()
> +{
> +    if (!m_scrollLayerID)
> +        m_scrollLayerID = generateScrollLayerID();
> +}

But this isn't attaching, it's just making an ID.
Comment 37 Beth Dakin 2012-10-08 14:19:09 PDT
(In reply to comment #36)
> (From update of attachment 167592 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=167592&action=review
> 
> > Source/WebCore/loader/HistoryController.cpp:131
> > +        Page* page = m_frame->page();
> > +        if (page && page->mainFrame() == m_frame) {
> > +            if (ScrollingCoordinator* scrollingCoordinator = page->scrollingCoordinator())
> > +                scrollingCoordinator->frameViewRootLayerDidChange(view);
> > +        }
> 
> This is a bit mysterious here, and it's a shame to call into ScrollingCoordinator from yet another file. What work does it need to do?
> 

This creates a root state node for the cached page. There is none otherwise because the root state node for the previous page was destroyed when its FrameView and backing went away.

> > Source/WebCore/page/FrameView.h:137
> > +    uint64_t scrollLayerID() const;
> 
> Does this have to be public on FrameView? Can't FrameView pass its own ID into the SC?
> 

It definitely does need to be public. ScrollingCoordinator needs to be able to access this ID whenever it has messages to send to the scrolling thread.

> > Source/WebCore/page/scrolling/ScrollingCoordinator.cpp:153
> > +
> 
> Blanky.
> 

Fixed.

> > Source/WebCore/page/scrolling/ScrollingCoordinator.cpp:473
> > +    ASSERT(node->isScrollingStateScrollingNode());
> > +    return toScrollingStateScrollingNode(node);
> 
> toScrollingStateScrollingNode will ASSERT for you; no need to ASSERT here too.
> 

Fixed.

> > Source/WebCore/page/scrolling/ScrollingStateNode.cpp:92
> > +    size_t size = m_children->size();
> > +    for (size_t i = 0; i < size; ++i)
> > +        m_children->at(i)->removeChild(node);
> 
> Looks like you're iterating the array while mutating it here.
> 

Hmm, kind of. I don't think this is actually dangerous. We NOT mutating the array we are iterating. We are potentially mutating the array that belongs to one of the objects in the array that we are iterating, if that makes sense.

> > Source/WebCore/rendering/RenderLayerBacking.cpp:972
> > +static uint64_t generateScrollLayerID()
> > +{
> > +    static uint64_t uniqueScrollLayerID = 1;
> > +    return uniqueScrollLayerID++;
> > +}
> 
> I think the ID generation should be on ScrollingCoordinator. Calling could look like:
> 
> m_id = scrollingCoordinator->attach(scrollingCoordinator->uniqueNodeID(), someConstraintsData)
> 
> where attach returns the ID passed in, for convenience.
> 

Okay. Your last comments made me think you wanted this code on RLB, so I move it there. I see what you want now. That is fine.

> > Source/WebCore/rendering/RenderLayerBacking.cpp:978
> > +void RenderLayerBacking::attachToScrollingCoordinator()
> > +{
> > +    if (!m_scrollLayerID)
> > +        m_scrollLayerID = generateScrollLayerID();
> > +}
> 
> But this isn't attaching, it's just making an ID.

Do you have another name suggestion? I was trying to avoid being overly-explict about the state tree since we are trying to keep that internal to ScrollingCoordinator, so I chose attachToScrollingCoordinator(). We could do something like addToScrollLayerHashMap().
Comment 38 Beth Dakin 2012-10-08 14:58:34 PDT
Created attachment 167624 [details]
Patch
Comment 39 Simon Fraser (smfr) 2012-10-08 15:38:48 PDT
Comment on attachment 167624 [details]
Patch

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

> Source/WebCore/dom/Document.cpp:4063
>              v->cacheCurrentScrollPosition();
> -            if (page() && page()->mainFrame() == m_frame)
> +            if (page && page->mainFrame() == m_frame) {
>                  v->resetScrollbarsAndClearContentsSize();
> -            else
> +                if (ScrollingCoordinator* scrollingCoordinator = page->scrollingCoordinator())
> +                    scrollingCoordinator->clearStateTree();

Let's file a bug to clean up the page cache save/restore and make it more symmetrical.

> Source/WebCore/page/scrolling/ScrollingCoordinator.h:144
> +    uint64_t attachToStateTree(uint64_t);

It's still odd that attach() isn't really attaching anything; it's making a node in the tree that will be filled in later. Can we just create the node with a new ID when we "fill in" that node later?

I think we should have a typedef ("ScrollingNodeID") for the uint64_t too.
Comment 40 Beth Dakin 2012-10-08 19:36:18 PDT
Created attachment 167674 [details]
Patch
Comment 41 WebKit Review Bot 2012-10-08 21:45:16 PDT
Comment on attachment 167674 [details]
Patch

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

New failing tests:
ScrollingCoordinatorChromiumTest.wheelEventHandler
Comment 42 James Robinson 2012-10-09 11:32:53 PDT
(In reply to comment #41)
> (From update of attachment 167674 [details])
> Attachment 167674 [details] did not pass chromium-ews (chromium-xvfb):
> Output: http://queues.webkit.org/results/14209754
> 
> New failing tests:
> ScrollingCoordinatorChromiumTest.wheelEventHandler

You plan to fix this before landing, right?
Comment 43 Beth Dakin 2012-10-09 11:35:25 PDT
(In reply to comment #42)
> (In reply to comment #41)
> > (From update of attachment 167674 [details] [details])
> > Attachment 167674 [details] [details] did not pass chromium-ews (chromium-xvfb):
> > Output: http://queues.webkit.org/results/14209754
> > 
> > New failing tests:
> > ScrollingCoordinatorChromiumTest.wheelEventHandler
> 
> You plan to fix this before landing, right?

I believe I know how to fix this, yes. And I have that fix in place. I'm going to land and watch the bots.
Comment 44 Beth Dakin 2012-10-09 11:47:53 PDT
http://trac.webkit.org/changeset/130783
Comment 45 Beth Dakin 2012-10-10 16:41:12 PDT
This caused https://bugs.webkit.org/show_bug.cgi?id=98968
Comment 46 Beth Dakin 2012-10-10 20:39:15 PDT
This also caused https://bugs.webkit.org/show_bug.cgi?id=98984