Bug 153085 – AX: Add a boundary value to AXTextStateChangeType

• Status: REOPENED
• Component: Accessibility

[Doug Russell]

Right now selection notifications for accessibility are only posted if selection actually changes, but it is often useful for assistive tech to be able to be to detect user attempts to change selection, even if they fail. Specifically a notification type (boundary) that can tell us when a user attempted to navigate past the boundary of an editable field is useful for providing feedback, for example bonking.

[Doug Russell]

Created attachment 268927 [details]
Patch

[Doug Russell]

Created attachment 268932 [details]
Patch

[Build Bot]

Comment on attachment 268932 [details]
Patch

Attachment 268932 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/688811

New failing tests:
accessibility/mac/selection-boundary-userinfo.html

[Build Bot]

Created attachment 268936 [details]
Archive of layout-test-results from ews100 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews100  Port: mac-yosemite  Platform: Mac OS X 10.10.5

[Build Bot]

Comment on attachment 268932 [details]
Patch

Attachment 268932 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/688818

New failing tests:
accessibility/mac/selection-boundary-userinfo.html

[Build Bot]

Created attachment 268937 [details]
Archive of layout-test-results from ews106 for mac-yosemite-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews106  Port: mac-yosemite-wk2  Platform: Mac OS X 10.10.5

[Build Bot]

Comment on attachment 268932 [details]
Patch

Attachment 268932 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/688834

New failing tests:
accessibility/mac/selection-boundary-userinfo.html

[Build Bot]

Created attachment 268938 [details]
Archive of layout-test-results from ews113 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews113  Port: mac-yosemite  Platform: Mac OS X 10.10.5

[Doug Russell]

Created attachment 268961 [details]
Patch

[Build Bot]

Comment on attachment 268961 [details]
Patch

Attachment 268961 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/690593

New failing tests:
editing/pasteboard/paste-list-001.html
editing/style/block-style-006.html
editing/style/block-style-005.html
fast/spatial-navigation/snav-input.html

[Build Bot]

Created attachment 268969 [details]
Archive of layout-test-results from ews101 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews101  Port: mac-yosemite  Platform: Mac OS X 10.10.5

[Build Bot]

Comment on attachment 268961 [details]
Patch

Attachment 268961 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/690598

New failing tests:
editing/style/block-style-006.html
editing/style/block-style-005.html
editing/pasteboard/paste-list-001.html

[Build Bot]

Created attachment 268970 [details]
Archive of layout-test-results from ews105 for mac-yosemite-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews105  Port: mac-yosemite-wk2  Platform: Mac OS X 10.10.5

[Build Bot]

Comment on attachment 268961 [details]
Patch

Attachment 268961 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/690591

New failing tests:
editing/pasteboard/paste-list-001.html
editing/style/block-style-006.html
editing/style/block-style-005.html
fast/spatial-navigation/snav-input.html

[Build Bot]

Created attachment 268972 [details]
Archive of layout-test-results from ews114 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews114  Port: mac-yosemite  Platform: Mac OS X 10.10.5

[Doug Russell]

Created attachment 268976 [details]
Patch

[Radar WebKit Bug Importer]

<rdar://problem/24191675>

[Doug Russell]

Created attachment 269010 [details]
Patch

[chris fleizach]

Comment on attachment 269010 [details]
Patch

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

> Source/WebCore/ChangeLog:9
> +        an editable elements boundaries.

elements -> element's

> Source/WebCore/accessibility/mac/AXObjectCacheMac.mm:385
> +                [userInfo setObject:[NSNumber numberWithBool:intent.selection.focusChange] forKey:NSAccessibilityTextSelectionChangedFocus];

@(intent.selectin.focusChange)

> Source/WebCore/editing/FrameSelection.cpp:1110
> +        // FIXME: BIDI

do you have a WK bug to reference

> Source/WebCore/editing/FrameSelection.cpp:1164
> +    return (AXTextSelection) { intentDirection, intentGranularity, false };

is the cast necessary (it may very well be, but i'm curious)

> Source/WebCore/editing/VisiblePosition.h:57
> +        , m_isBoundary(false)

m_isBoundary should be false by default. you may not need to initialize

> Source/WebCore/editing/VisiblePosition.h:60
> +    VisiblePosition(bool boundary) : m_affinity(VP_DEFAULT_AFFINITY), m_isBoundary(boundary) { };

should this be "bool boundary = false" so you get the default behavior without changing code to be no?
or should we make a static boundaryVisiblePosition() class method that returns the right object

i know people don't generally like passing around bools where the value is not clear what its for

> LayoutTests/accessibility/mac/selection-boundary-userinfo.html:8
> +<div role="textbox" tabindex=0 id="textbox" contenteditable=true>

can you also add a test with a input type="text"

> LayoutTests/accessibility/mac/selection-boundary-userinfo.html:30
> +            	shouldBe("results[0]", "AXTextStateChangeTypeSelectionMove");

looks like this file has some tabs in it

[Doug Russell]

(In reply to comment #19)
> Comment on attachment 269010 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=269010&action=review
> 
> > Source/WebCore/ChangeLog:9
> > +        an editable elements boundaries.
> 
> elements -> element's

will do

> 
> > Source/WebCore/accessibility/mac/AXObjectCacheMac.mm:385
> > +                [userInfo setObject:[NSNumber numberWithBool:intent.selection.focusChange] forKey:NSAccessibilityTextSelectionChangedFocus];
> 
> @(intent.selectin.focusChange)

will do

> 
> > Source/WebCore/editing/FrameSelection.cpp:1110
> > +        // FIXME: BIDI
> 
> do you have a WK bug to reference

I'll file one

> 
> > Source/WebCore/editing/FrameSelection.cpp:1164
> > +    return (AXTextSelection) { intentDirection, intentGranularity, false };
> 
> is the cast necessary (it may very well be, but i'm curious)

This syntax was suggested by Darin in past reviews, I'll see if the cast can be dropped

> 
> > Source/WebCore/editing/VisiblePosition.h:57
> > +        , m_isBoundary(false)
> 
> m_isBoundary should be false by default. you may not need to initialize

That is how I had it, trying this way to see if that's what was tripping up the windows build (it's one of the bigger things I changed between patches when windows stopped building)

> 
> > Source/WebCore/editing/VisiblePosition.h:60
> > +    VisiblePosition(bool boundary) : m_affinity(VP_DEFAULT_AFFINITY), m_isBoundary(boundary) { };
> 
> should this be "bool boundary = false" so you get the default behavior
> without changing code to be no?
> or should we make a static boundaryVisiblePosition() class method that
> returns the right object

If you want boundary to be false you can just call VisiblePosition(). I can do a static if you prefer but it feels odd to not use a constructor.

> 
> i know people don't generally like passing around bools where the value is
> not clear what its for

And we'd solve that with the static method?

> 
> > LayoutTests/accessibility/mac/selection-boundary-userinfo.html:8
> > +<div role="textbox" tabindex=0 id="textbox" contenteditable=true>
> 
> can you also add a test with a input type="text"

Yup. As is this patch doesn't post a boundary notification when arrowing up in a input (since they're single line controls), but I can file an enhancement for that.

> 
> > LayoutTests/accessibility/mac/selection-boundary-userinfo.html:30
> > +            	shouldBe("results[0]", "AXTextStateChangeTypeSelectionMove");
> 
> looks like this file has some tabs in it

I'll fix that.

[chris fleizach]

Comment on attachment 269010 [details]
Patch

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

>>> Source/WebCore/editing/VisiblePosition.h:60
>>> +    VisiblePosition(bool boundary) : m_affinity(VP_DEFAULT_AFFINITY), m_isBoundary(boundary) { };
>> 
>> should this be "bool boundary = false" so you get the default behavior without changing code to be no?
>> or should we make a static boundaryVisiblePosition() class method that returns the right object
>> 
>> i know people don't generally like passing around bools where the value is not clear what its for
> 
> If you want boundary to be false you can just call VisiblePosition(). I can do a static if you prefer but it feels odd to not use a constructor.

In this case no one would ever use VisiblePosition(false) because that's already exists, so it also feels weird to have a constructor with a parameter when it's not a necessary parameter

that's what I was thinking a 
static VisiblePosition visibleBoundaryPosition(); 
might be more clear

[Doug Russell]

Created attachment 269074 [details]
Patch

[Build Bot]

Comment on attachment 269074 [details]
Patch

Attachment 269074 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/695263

New failing tests:
accessibility/mac/selection-boundary-userinfo.html

[Build Bot]

Created attachment 269083 [details]
Archive of layout-test-results from ews101 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews101  Port: mac-yosemite  Platform: Mac OS X 10.10.5

[Build Bot]

Comment on attachment 269074 [details]
Patch

Attachment 269074 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/695246

New failing tests:
accessibility/mac/selection-boundary-userinfo.html

[Build Bot]

Created attachment 269084 [details]
Archive of layout-test-results from ews116 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews116  Port: mac-yosemite  Platform: Mac OS X 10.10.5

[Doug Russell]

Created attachment 269085 [details]
Patch

[chris fleizach]

are you still trying to resolve windows failure or is it unrelated

[Darin Adler]

Comment on attachment 269085 [details]
Patch

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

Generally looks good. I am concerned about this new VisiblePosition feature. Maybe this is the best way to do it, though. Did you consult with other WebKit editing code experts on the best way?

Also need some help from a Windows expert on figuring out why Windows bot failed to build.

review- because we at least want to make the "explicit" change and to get Windows building

> Source/WebCore/accessibility/mac/AXObjectCacheMac.mm:385
> +            if (intent.selection.focusChange)
> +                [userInfo setObject:@(intent.selection.focusChange) forKey:NSAccessibilityTextSelectionChangedFocus];

Does moving this here fix a bug? If so, do is that fix covered by a test case?

> Source/WebCore/editing/FrameSelection.cpp:1110
> +        // FIXME: BIDI

Would be nice to write a more specific FIXME. I think I know what you are driving at, but explicitly stating it is nicer for other programmers who show up later.

> Source/WebCore/editing/FrameSelection.cpp:1117
> +        // FIXME: BIDI

Ditto.

> Source/WebCore/editing/FrameSelection.cpp:1130
> +    case SentenceBoundary: // FIXME: Boundary should effect direction

Grammar error. We mean "should affect" not "should effect". Also should put a period; we use a sort of sentence style on these comments and call for a period.

> Source/WebCore/editing/FrameSelection.cpp:1137
> +    case ParagraphBoundary: // FIXME: Boundary should effect direction

Ditto.

> Source/WebCore/editing/FrameSelection.cpp:1141
> +    case DocumentBoundary: // FIXME: Boundary should effect direction

Ditto.

> Source/WebCore/editing/FrameSelection.cpp:1224
> +        notifyAccessibilityForSelectionChange((AXTextStateChangeIntent) { AXTextStateChangeTypeSelectionBoundary, textSelectionWithDirectionAndGranularity(direction, granularity) });

Shouldn’t need parentheses around (AXTextStateChangeIntent). Maybe you did that to quiet the mistaken complaint from the style checker?

> Source/WebCore/editing/VisiblePosition.h:53
> +    static VisiblePosition boundaryVisiblePosition();

I’m unclear on how this is intended to be used. We need a comment explaining this since it’s not an obvious concept.

> Source/WebCore/editing/VisiblePosition.h:116
> +    VisiblePosition(bool boundary) : m_affinity(VP_DEFAULT_AFFINITY), m_isBoundary(boundary) { };

Should qualify this with "explicit" so code inside VisiblePosition can’t make the mistake of returning a boolean and have it automatically turned into a VisiblePosition.

> Source/WebCore/editing/VisiblePosition.h:125
>      Position m_deepPosition;
>      EAffinity m_affinity;
> +    bool m_isBoundary = false;

I think it’s dangerous that now we have a type of VisiblePosition that won’t survive a round trop if broken into position and affinity and then made into a VIsiblePosition again. Do we have other options here rather than adding this state to VisiblePosition?

[Doug Russell]

(In reply to comment #29)
> Comment on attachment 269085 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=269085&action=review
> 
> Generally looks good. I am concerned about this new VisiblePosition feature.
> Maybe this is the best way to do it, though. Did you consult with other
> WebKit editing code experts on the best way?
> 
> Also need some help from a Windows expert on figuring out why Windows bot
> failed to build.
> 
> review- because we at least want to make the "explicit" change and to get
> Windows building

I sent the patch to Brent Fulgham hoping for some help on this, but haven't heard back yet.

> 
> > Source/WebCore/accessibility/mac/AXObjectCacheMac.mm:385
> > +            if (intent.selection.focusChange)
> > +                [userInfo setObject:@(intent.selection.focusChange) forKey:NSAccessibilityTextSelectionChangedFocus];
> 
> Does moving this here fix a bug? If so, do is that fix covered by a test
> case?

There's not a known bug related to this, but I noticed that in it's previous location intent.selection.focusChange wasn't guaranteed to be initialized so I moved it up. I can submit it in it's own patch if you'd like me to.

> 
> > Source/WebCore/editing/FrameSelection.cpp:1110
> > +        // FIXME: BIDI
> 
> Would be nice to write a more specific FIXME. I think I know what you are
> driving at, but explicitly stating it is nicer for other programmers who
> show up later.

Will do.

> 
> > Source/WebCore/editing/FrameSelection.cpp:1117
> > +        // FIXME: BIDI
> 
> Ditto.
> 
> > Source/WebCore/editing/FrameSelection.cpp:1130
> > +    case SentenceBoundary: // FIXME: Boundary should effect direction
> 
> Grammar error. We mean "should affect" not "should effect". Also should put
> a period; we use a sort of sentence style on these comments and call for a
> period.

Will do.

> 
> > Source/WebCore/editing/FrameSelection.cpp:1137
> > +    case ParagraphBoundary: // FIXME: Boundary should effect direction
> 
> Ditto.
> 
> > Source/WebCore/editing/FrameSelection.cpp:1141
> > +    case DocumentBoundary: // FIXME: Boundary should effect direction
> 
> Ditto.
> 
> > Source/WebCore/editing/FrameSelection.cpp:1224
> > +        notifyAccessibilityForSelectionChange((AXTextStateChangeIntent) { AXTextStateChangeTypeSelectionBoundary, textSelectionWithDirectionAndGranularity(direction, granularity) });
> 
> Shouldn’t need parentheses around (AXTextStateChangeIntent). Maybe you did
> that to quiet the mistaken complaint from the style checker?

I'll remove them.

> 
> > Source/WebCore/editing/VisiblePosition.h:53
> > +    static VisiblePosition boundaryVisiblePosition();
> 
> I’m unclear on how this is intended to be used. We need a comment explaining
> this since it’s not an obvious concept.

I'll add one.

> 
> > Source/WebCore/editing/VisiblePosition.h:116
> > +    VisiblePosition(bool boundary) : m_affinity(VP_DEFAULT_AFFINITY), m_isBoundary(boundary) { };
> 
> Should qualify this with "explicit" so code inside VisiblePosition can’t
> make the mistake of returning a boolean and have it automatically turned
> into a VisiblePosition.

Will do.

> 
> > Source/WebCore/editing/VisiblePosition.h:125
> >      Position m_deepPosition;
> >      EAffinity m_affinity;
> > +    bool m_isBoundary = false;
> 
> I think it’s dangerous that now we have a type of VisiblePosition that won’t
> survive a round trop if broken into position and affinity and then made into
> a VIsiblePosition again. Do we have other options here rather than adding
> this state to VisiblePosition?

We could thread error state through the modifyMovingX logic to return why the VisiblePosition it's returning is NULL. It would mean touching a lot more code though.

[Doug Russell]

Created attachment 269731 [details]
Patch

[Darin Adler]

Comment on attachment 269085 [details]
Patch

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

>>> Source/WebCore/editing/VisiblePosition.h:125
>>> +    bool m_isBoundary = false;
>> 
>> I think it’s dangerous that now we have a type of VisiblePosition that won’t survive a round trop if broken into position and affinity and then made into a VIsiblePosition again. Do we have other options here rather than adding this state to VisiblePosition?
> 
> We could thread error state through the modifyMovingX logic to return why the VisiblePosition it's returning is NULL. It would mean touching a lot more code though.

One way to do it would be to add a new type that’s either a VisiblePosition or a boundary, rather than adding it to the original VisiblePosition type. You would need to touch more code, but just to change the type from VisiblePosition to the new type. Even though it makes the patch bigger, I think that would likely be better than introducing this concept into all other code using VisiblePosition for any other purpose. Could start as simply as a struct with a boolean and a VisiblePosition in it.

[Doug Russell]

(In reply to comment #32)
> Comment on attachment 269085 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=269085&action=review
> 
> >>> Source/WebCore/editing/VisiblePosition.h:125
> >>> +    bool m_isBoundary = false;
> >> 
> >> I think it’s dangerous that now we have a type of VisiblePosition that won’t survive a round trop if broken into position and affinity and then made into a VIsiblePosition again. Do we have other options here rather than adding this state to VisiblePosition?
> > 
> > We could thread error state through the modifyMovingX logic to return why the VisiblePosition it's returning is NULL. It would mean touching a lot more code though.
> 
> One way to do it would be to add a new type that’s either a VisiblePosition
> or a boundary, rather than adding it to the original VisiblePosition type.
> You would need to touch more code, but just to change the type from
> VisiblePosition to the new type. Even though it makes the patch bigger, I
> think that would likely be better than introducing this concept into all
> other code using VisiblePosition for any other purpose. Could start as
> simply as a struct with a boolean and a VisiblePosition in it.

My latest patch threads a bool * through all the relevant logic. If this works for you I'm happy with it, but I could take the above approach if that's preferable.

[Doug Russell]

Created attachment 269847 [details]
Patch

[Doug Russell]

Created attachment 269848 [details]
patch.patch

Switching back to bool * approach. I'll hold onto the last patch as a possible improvement for the future.

[Doug Russell]

(In reply to comment #35)
> Created attachment 269848 [details]
> patch.patch
> 
> Switching back to bool * approach. I'll hold onto the last patch as a
> possible improvement for the future.

Previous patch doesn't apply. I'll revisit this in the morning.

[Doug Russell]

Created attachment 269870 [details]
patch

Try to fix the GTK build failure

[Doug Russell]

Created attachment 269871 [details]
patch

try to fix GTK build

[Build Bot]

Comment on attachment 269871 [details]
patch

Attachment 269871 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/740612

Number of test failures exceeded the failure limit.

[Build Bot]

Created attachment 269872 [details]
Archive of layout-test-results from ews114 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews114  Port: mac-yosemite  Platform: Mac OS X 10.10.5

[Build Bot]

Comment on attachment 269871 [details]
patch

Attachment 269871 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/740633

Number of test failures exceeded the failure limit.

[Build Bot]

Created attachment 269873 [details]
Archive of layout-test-results from ews100 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews100  Port: mac-yosemite  Platform: Mac OS X 10.10.5

[Build Bot]

Comment on attachment 269871 [details]
patch

Attachment 269871 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/740636

Number of test failures exceeded the failure limit.

[Build Bot]

Created attachment 269874 [details]
Archive of layout-test-results from ews104 for mac-yosemite-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews104  Port: mac-yosemite-wk2  Platform: Mac OS X 10.10.5

[Doug Russell]

Created attachment 269895 [details]
Patch

[Doug Russell]

Changing the return type had more knock on effects than I'm prepared to take on with this patch, so I'm sticking with the bool * approach.

[Darin Adler]

Comment on attachment 269895 [details]
Patch

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

Looks good. Some style issues.

When we use pointers for out arguments like this, normally we set the value of out arguments in all cases, rather than relying on them being initialized on entry. That means there are more places here we would need to set *reachedBoundary to false.

> Source/WebCore/editing/FrameSelection.cpp:763
> +VisiblePosition FrameSelection::modifyMovingRight(TextGranularity granularity, bool * reachedBoundary)

WebKit coding style would use bool* rather than bool * with a space.

I’m surprised you chose the out argument over the "VisiblePosition plus a bool" struct option. Another possibility would be to use Optional<VisiblePosition> and have “no position at all” indicate a boundary. That’s distinct from a null VisiblePosition. But probably too subtle and confusing.

> Source/WebCore/editing/FrameSelection.cpp:796
> +    case LineBoundary: {
> +        pos = rightBoundaryOfLine(startForPlatform(), directionOfEnclosingBlock(), reachedBoundary);
>          break;
> +    }

No braces in a case like this in WebKit coding style.

> Source/WebCore/editing/FrameSelection.cpp:804
> +VisiblePosition FrameSelection::modifyMovingForward(TextGranularity granularity, bool * reachedBoundary)

bool*

> Source/WebCore/editing/FrameSelection.cpp:1014
> +    case LineBoundary: {
> +        pos = leftBoundaryOfLine(startForPlatform(), directionOfEnclosingBlock(), reachedBoundary);
>          break;
> +    }

No braces in a case like this in WebKit coding style.

> Source/WebCore/editing/FrameSelection.h:295
> +    VisiblePosition modifyMovingRight(TextGranularity, bool * = nullptr);
> +    VisiblePosition modifyMovingForward(TextGranularity, bool * = nullptr);

Formatting should be bool* and when the argument meaning is not clear from the type alone, we require an argument name, so should be:

    bool* reachedBoundary = nullptr

I’ll mention this only once, but the issue recurs many other times below.

> Source/WebCore/editing/VisiblePosition.cpp:67
>  {

bool*

> Source/WebCore/editing/VisiblePosition.cpp:78
> +VisiblePosition VisiblePosition::previous(EditingBoundaryCrossingRule rule, bool * reachedBoundary) const

bool*

> Source/WebCore/editing/VisiblePosition.cpp:259
> +VisiblePosition VisiblePosition::left(bool stayInEditableContent, bool * reachedBoundary) const

bool*

I’ll stop mentioning this, since it happens many more times.

[Doug Russell]

Created attachment 270353 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 270353 [details]
Patch

Clearing flags on attachment: 270353

Committed r195949: <http://trac.webkit.org/changeset/195949>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Doug Russell]

Reopening to attach new patch.

[Doug Russell]

Created attachment 270604 [details]
Patch

[Doug Russell]

Comment on attachment 270604 [details]
Patch

This shouldn't have been re-opened. I think this was a webkit-patch bug number typo.