Bug 70241

Created attachment 111305 [details] patch

Attachment 111305 [details] did not pass style-queue: Failed to run "['Tools/Scripts/update-webkit', '--chromium']" exit_code: 2 Updating OpenSource M Source/WebKit2/WebProcess/WebCoreSupport/WebGraphicsLayer.h M Source/WebKit2/WebProcess/WebCoreSupport/WebGraphicsLayer.cpp M Source/WebKit2/WebProcess/WebPage/LayerTreeHost.h M Source/WebKit2/WebProcess/WebPage/qt/LayerTreeHostQt.cpp A Source/WebKit2/WebProcess/WebPage/qt/LayerTreeHostQt.h M Source/WebKit2/WebProcess/WebPage/TiledDrawingArea.cpp M Source/WebKit2/WebProcess/WebPage/DrawingAreaImpl.cpp M Source/WebKit2/WebProcess/WebPage/WebPage.cpp A Source/WebKit2/WebProcess/WebPage/LayerTreeHost.messages.in M Source/WebKit2/WebProcess/WebPage/LayerTreeHost.cpp M Source/WebKit2/WebProcess/WebPage/DrawingAreaImpl.h M Source/WebKit2/WebProcess/WebPage/TiledDrawingArea.h M Source/WebKit2/WebProcess/WebPage/DrawingArea.h A Source/WebKit2/UIProcess/qt/LayerTreeHostProxyQt.cpp M Source/WebKit2/UIProcess/DrawingAreaProxyImpl.h A Source/WebKit2/UIProcess/LayerTreeHostProxy.h M Source/WebKit2/UIProcess/DrawingAreaProxyImpl.cpp M Source/WebKit2/UIProcess/WebPageProxy.cpp M Source/WebKit2/UIProcess/DrawingAreaProxy.cpp A Source/WebKit2/UIProcess/LayerTreeHostProxy.messages.in M Source/WebKit2/UIProcess/DrawingAreaProxy.h M Source/WebKit2/Platform/CoreIPC/MessageID.h M Source/WebKit2/DerivedSources.pro M Source/WebKit2/Scripts/webkit2/messages.py M Source/WebKit2/ChangeLog M Source/WebKit2/Shared/qt/LayerTreeContextQt.cpp M Source/WebKit2/Shared/WebLayerTreeInfo.cpp M Source/WebKit2/Shared/LayerTreeContext.h M Source/WebKit2/Shared/WebLayerTreeInfo.h M Source/WebKit2/WebKit2.pro r97639 = 6a21199564eb7173886ffb3ae167adbf7188a1d2 (refs/remotes/trunk) First, rewinding head to replay your work on top of it... Fast-forwarded master to refs/remotes/trunk. Updating chromium port dependencies using gclient... Error: Can't switch the checkout to http://v8.googlecode.com/svn/branches/3.6@9637; UUID don't match and there is local changes in /mnt/git/webkit-style-queue/Source/WebKit/chromium/v8. Delete the directory and try again. Re-trying 'depot_tools/gclient sync' Error: Can't switch the checkout to http://v8.googlecode.com/svn/branches/3.6@9637; UUID don't match and there is local changes in /mnt/git/webkit-style-queue/Source/WebKit/chromium/v8. Delete the directory and try again. Re-trying 'depot_tools/gclient sync' Error: Can't switch the checkout to http://v8.googlecode.com/svn/branches/3.6@9637; UUID don't match and there is local changes in /mnt/git/webkit-style-queue/Source/WebKit/chromium/v8. Delete the directory and try again. Error: 'depot_tools/gclient sync' failed 3 tries and returned 256 at Tools/Scripts/update-webkit-chromium line 107. Re-trying 'depot_tools/gclient sync' No such file or directory at Tools/Scripts/update-webkit line 104. If any of these errors are false positives, please file a bug against check-webkit-style.

Comment on attachment 111305 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=111305&action=review > Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapper.mm:897 > actions = actionElementActions; all buttons should expose the press action, not only if they have an actionElement

Comment on attachment 111305 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=111305&action=review > Source/WebCore/accessibility/AccessibilitySpinButton.cpp:36 > + return adoptRef(new AccessibilitySpinButton()); Extra unneeded () in there. > Source/WebCore/accessibility/AccessibilitySpinButton.cpp:40 > + : AccessibilityMockObject() You should be able to omit this initializer. > Source/WebCore/accessibility/AccessibilitySpinButton.cpp:77 > + m_spinButtonElement->renderer()->absoluteFocusRingQuads(quads); Incorrect indentation. > Source/WebCore/accessibility/AccessibilitySpinButton.cpp:109 > + : AccessibilityMockObject() You should be able to omit this line. > Source/WebCore/accessibility/AccessibilitySpinButton.cpp:115 > + return adoptRef(new AccessibilitySpinButtonPart()); Extra unneeded () in there. > Source/WebCore/accessibility/AccessibilitySpinButton.cpp:126 > + LayoutRect parentRect = parentObject()->elementRect(); > + if (m_isIncrementor) > + parentRect.setHeight(parentRect.height()/2); > + else { > + parentRect.setY(parentRect.y() + parentRect.height()/2); > + parentRect.setHeight(parentRect.height()/2); > + } We normally put spaces around operators like "/". This geometry code doesn’t belong here in the accessibility object. The knowledge of what part of the button is what should be in the renderer, not here. > Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapper.mm:1140 > + tempArray = [[NSMutableArray alloc] initWithArray:attributes]; > + [tempArray addObject:NSAccessibilityIncrementButtonAttribute]; > + [tempArray addObject:NSAccessibilityDecrementButtonAttribute]; > + incrementorAttrs = [[NSArray alloc] initWithArray:tempArray]; > + [tempArray release]; This could be done by using initWithObjects: in 1 line of code instead of 5. > Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapper.mm:1573 > + if (toAccessibilitySpinButtonPart(m_object)->isIncrementor()) > + return NSAccessibilityIncrementArrowSubrole; > + else > + return NSAccessibilityDecrementArrowSubrole; In WebKit coding style we avoid else after return. > Source/WebCore/accessibility/AccessibilityObject.h:517 > + static LayoutRect boundingBoxForQuads(RenderObject*, Vector<FloatQuad>&); Should be const Vector<FloatQuad>&. > Source/WebCore/accessibility/AccessibilityMockObject.h:40 > + virtual bool isMockObject() const { return true; } Should be private. > Source/WebCore/accessibility/AccessibilityObject.cpp:411 > + const size_t n = quads.size(); No need for the const here. Normally we’d use the name “size” or “count” for this. > Source/WebCore/accessibility/AccessibilitySpinButton.h:36 > + Normally we don’t have this blank line. > Source/WebCore/accessibility/AccessibilitySpinButton.h:49 > + AccessibilitySpinButton(); Can this be private? > Source/WebCore/accessibility/AccessibilityMockObject.cpp:33 > AccessibilityMockObject::AccessibilityMockObject() > - : m_parent(0) > + : AccessibilityObject() > + , m_parent(0) This change should not be necessary. > Source/WebCore/html/shadow/TextControlInnerElements.cpp:346 > + Extra whitespace added. Should get rid of that.

Comment on attachment 111305 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=111305&action=review >> Source/WebCore/accessibility/AccessibilitySpinButton.cpp:126 >> + } > > We normally put spaces around operators like "/". > > This geometry code doesn’t belong here in the accessibility object. The knowledge of what part of the button is what should be in the renderer, not here. Unfortunately there's no distinguishable renderer for these inner control objects. In the hit testing for this it checks if the point is below or above the midpoint. I can't find any another reference to these specific controls. I'll add a FIXME to take care of this when it's more clear how to accomplish this >> Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapper.mm:1140 >> + [tempArray release]; > > This could be done by using initWithObjects: in 1 line of code instead of 5. We have to start with the "attributes" array, so we can't directly call initWithObjects unfortunately and get the behavior we want

(In reply to comment #6) > http://trac.webkit.org/changeset/97669 Fixed chromium failure in http://trac.webkit.org/changeset/97677

It appears that this patch caused 20+ tests to crash on Windows bots: http://build.webkit.org/results/Windows%207%20Release%20(Tests)/r97675%20(17199)/results.html http://build.webkit.org/builders/Windows%20XP%20Debug%20%28Tests%29/builds/33254

I'm sorry but I'm going to roll out the change for now since we're losing a significant test due to nrwt exiting early of these crashes.

Reopen the bug because the patch was rolled out in http://trac.webkit.org/changeset/97721.

(In reply to comment #10) > Reopen the bug because the patch was rolled out in http://trac.webkit.org/changeset/97721. Very strange. Not clear why this change is crashing every AX test on windows

I will roll in this piece by piece since I don't see why the original patch would only cause crashes on windows Part 1 http://trac.webkit.org/changeset/97753

(In reply to comment #12) > I will roll in this piece by piece since I don't see why the original patch would only cause crashes on windows > > Part 1 > http://trac.webkit.org/changeset/97753 Part 2 http://trac.webkit.org/changeset/97755 part 3 http://trac.webkit.org/changeset/97756 Part 4 http://trac.webkit.org/changeset/97757 part 5 http://trac.webkit.org/changeset/97759 So far so good it appears

(In reply to comment #13) > (In reply to comment #12) Part 6 http://trac.webkit.org/changeset/97769 I do not see any Windows related crashes this time...

Modifying/Adding new files in WebCore.vcproj and running webkit check style on it results in "Source/WebCore/WebCore.vcproj/WebCore.vcproj:25583: mismatched tag [xml/syntax] [5]" while adding new files in WebCore.vcporj. Here is my observation : In http://trac.webkit.org/changeset/97756 Index: Source/WebCore/WebCore.vcproj/WebCore.vcproj =================================================================== --- Source/WebCore/WebCore.vcproj/WebCore.vcproj (revision 97638) +++ Source/WebCore/WebCore.vcproj/WebCore.vcproj (working copy) @@ -25221,6 +25221,13 @@ RelativePath="..\accessibility\AccessibilitySlider.h" > </File> + <File + RelativePath="..\accessibility\AccessibilitySpinButton.cpp" + > + </File> + <File + RelativePath="..\accessibility\AccessibilitySpinButton.h" + > <File RelativePath="..\accessibility\AccessibilityTable.cpp" > Here closing </File> tag is missing for AccessibilitySpinButton.h. Hence the style checker complains about everything that we add/modify in WebCore.vcproj. Adding the required closing </File> tag resolves the issue.

(In reply to comment #15) > Modifying/Adding new files in WebCore.vcproj and running webkit check style on it results in > "Source/WebCore/WebCore.vcproj/WebCore.vcproj:25583: mismatched tag [xml/syntax] [5]" while adding new files in WebCore.vcporj. > > Here is my observation : > Here closing </File> tag is missing for AccessibilitySpinButton.h. > Hence the style checker complains about everything that we add/modify in WebCore.vcproj. > > Adding the required closing </File> tag resolves the issue. Thanks for catching that. Fixed with http://trac.webkit.org/changeset/97857