Attachment 312135[details] did not pass style-queue:
ERROR: Source/WebCore/inspector/InspectorCanvasAgent.h:54: "virtual" is redundant since function is already declared as "override" [readability/inheritance] [4]
ERROR: Source/WebCore/inspector/InspectorCanvasAgent.h:55: "virtual" is redundant since function is already declared as "override" [readability/inheritance] [4]
ERROR: Source/WebCore/inspector/InspectorCanvasAgent.h:67: "virtual" is redundant since function is already declared as "override" [readability/inheritance] [4]
ERROR: Source/WebCore/inspector/InspectorCanvasAgent.h:68: "virtual" is redundant since function is already declared as "override" [readability/inheritance] [4]
ERROR: Source/WebCore/inspector/InspectorCanvasAgent.h:69: "virtual" is redundant since function is already declared as "override" [readability/inheritance] [4]
Total errors found: 5 in 29 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 312140[details]
Archive of layout-test-results from ews103 for mac-elcapitan
The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews103 Port: mac-elcapitan Platform: Mac OS X 10.11.6
Created attachment 312142[details]
Archive of layout-test-results from ews106 for mac-elcapitan-wk2
The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews106 Port: mac-elcapitan-wk2 Platform: Mac OS X 10.11.6
Created attachment 312143[details]
Archive of layout-test-results from ews116 for mac-elcapitan
The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews116 Port: mac-elcapitan Platform: Mac OS X 10.11.6
Created attachment 312370[details]
Archive of layout-test-results from ews100 for mac-elcapitan
The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews100 Port: mac-elcapitan Platform: Mac OS X 10.11.6
Created attachment 312372[details]
Archive of layout-test-results from ews114 for mac-elcapitan
The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews114 Port: mac-elcapitan Platform: Mac OS X 10.11.6
Created attachment 312376[details]
Archive of layout-test-results from ews102 for mac-elcapitan
The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews102 Port: mac-elcapitan Platform: Mac OS X 10.11.6
Created attachment 312378[details]
Archive of layout-test-results from ews105 for mac-elcapitan-wk2
The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews105 Port: mac-elcapitan-wk2 Platform: Mac OS X 10.11.6
Created attachment 312380[details]
Archive of layout-test-results from ews117 for mac-elcapitan
The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews117 Port: mac-elcapitan Platform: Mac OS X 10.11.6
Created attachment 312489[details]
Archive of layout-test-results from ews113 for mac-elcapitan
The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews113 Port: mac-elcapitan Platform: Mac OS X 10.11.6
Comment on attachment 312475[details]
Patch
Overall, this looks good to me. Not really sure why the tests keep failing, but I am also kinda new to working with the inspector backend.
View in context: https://bugs.webkit.org/attachment.cgi?id=312475&action=review> Source/WebCore/inspector/InspectorInstrumentation.cpp:1224
> +InstrumentingAgents* InspectorInstrumentation::instrumentingAgentsForRenderingContext(WebGLRenderingContextBase* context)
> +{
> + if (!context)
> + return nullptr;
> +
> + HTMLCanvasElement& canvasElement = context->canvas();
> + return instrumentingAgentsForDocument(canvasElement.document());
> +}
> +
Is this ever used? Is it necessary for something that I am unaware of?
> Source/WebInspectorUI/UserInterface/Controllers/CanvasManager.js:56
> + canvasesForFrame(frame)
> + {
> + let canvases = [];
> + for (let canvas of this._canvasIdentifierMap.values()) {
> + if (canvas.parentFrame.id === frame.id)
> + canvases.push(canvas);
> + }
> +
> + return canvases;
> + }
FYI I removed this in my followup patches, but I think it's fine to leave for now.
> Source/WebInspectorUI/UserInterface/Models/Canvas.js:119
> +WebInspector.Canvas.ContextType = {
> + Canvas2D: "canvas-context-type-canvas-2d",
> + WebGL: "canvas-context-type-webgl",
> +};
I think that adding the prefix to these values is unnecessary.
WebInspector.Canvas.ContextType = {
Canvas2D: "canvas-2d",
WebGL: "webgl",
};
> LayoutTests/inspector/canvas/create-canvas-contexts.html:6
> +let contexts = [];
I personally like having values that should always be accessible be attached to window. I can go either way with this, though.
window.contexts = [];
> LayoutTests/inspector/canvas/create-canvas-contexts.html:35
> + if (window.GCController)
I think you can get rid of the if check here, as the test might function differently without GCController. I'd rather get an error saying "GCController.collect is undefined" instead of wondering why the test is failing due to a lack of timely GC.
> LayoutTests/inspector/canvas/create-canvas-contexts.html:84
> + resolve();
NIT: you can move this to be another `.then`
.then(resolve, reject);
> LayoutTests/inspector/canvas/create-canvas-contexts.html:93
> + InspectorTest.evaluateInPage("createCanvas('2d')");
I think that using template strings makes reading these easier, especially when using evaluateInPage.
InspectorTest.evaluateInPage(`createCanvas("2d")`);
Comment on attachment 312475[details]
Patch
View in context: https://bugs.webkit.org/attachment.cgi?id=312475&action=review
Nice tests!
> Source/JavaScriptCore/inspector/protocol/Canvas.json:24
> + { "name": "name", "type": "string", "description": "The CSS canvas identifier, or the canvas element id attribute." },
The canvas element id attribute? This overloading feels weird how about just making it "cssName" and optional? Seems like this could make make the `isCSSCanvas` unnecessary.
> Source/WebCore/html/canvas/WebGLRenderingContextBase.cpp:55
> +#include "InspectorInstrumentation.h"
Unused. In fact all of the WebGLRenderingContextBase includes also seem unused. Seems this was intended for a later change?
> Source/WebCore/inspector/InspectorCanvasAgent.cpp:70
> + if (!m_frontendDispatcher)
> + return;
We shouldn't need to do this check (everywhere in this file). FrontendDispatcher will always be alive in our unique_ptr.
> Source/WebCore/inspector/InspectorCanvasAgent.h:34
> +#include <wtf/HashSet.h>
Unused.
> Source/WebCore/inspector/InspectorCanvasAgent.h:45
> +typedef String ErrorString;
This shouldn't be needed here, an include should handle it.
> Source/WebCore/inspector/InspectorCanvasAgent.h:66
> + // CanvasObserver implementation
We can drop the "implementation" here.
> Source/WebCore/inspector/InspectorInstrumentation.cpp:996
> +void InspectorInstrumentation::didCreateCSSCanvasImpl(InstrumentingAgents* instrumentingAgents, HTMLCanvasElement& canvasElement, const String& name)
Style: Lets put this above InspectorInstrumentation::networkStateChangedImpl
> Source/WebCore/inspector/InspectorInstrumentation.cpp:1216
> +InstrumentingAgents* InspectorInstrumentation::instrumentingAgentsForRenderingContext(WebGLRenderingContextBase* context)
Unused in this patch.
> Source/WebCore/inspector/InspectorInstrumentation.h:248
> + static void didCreateCSSCanvas(HTMLCanvasElement&, const String&);
Style: Lets try to keep the order of functions consistent. Here these are above "networkStateChanged" so lets keep that order.
> Source/WebCore/inspector/InspectorInstrumentation.h:423
> + static void didCreateCSSCanvasImpl(InstrumentingAgents*, HTMLCanvasElement&, const String&);
Style: Move these above networkStateChangedImpl
> Source/WebCore/inspector/InspectorInstrumentation.h:1097
> +inline void InspectorInstrumentation::didCreateCSSCanvas(HTMLCanvasElement& canvasElement, const String& name)
Style: Move these above InspectorInstrumentation::networkStateChanged
> Source/WebInspectorUI/UserInterface/Controllers/CanvasManager.js:51
> + if (canvas.parentFrame.id === frame.id)
Nit: Can we just compare `canvas.parentFrame === frame` instead of getting the ids?
> Source/WebInspectorUI/UserInterface/Controllers/CanvasManager.js:102
> + CanvasWasRemoved: "canvas-manager-canvas-was-removed"
Style: Include the trailing comma to make future changes nicer.
> Source/WebInspectorUI/UserInterface/Models/Canvas.js:50
> + case CanvasAgent.ContextType.Canvas2D:
Style: dedent cases.
> Source/WebInspectorUI/UserInterface/Models/Canvas.js:67
> + case WebInspector.Canvas.ContextType.Canvas2D:
Style: dedent cases.
> Source/WebInspectorUI/UserInterface/Models/Canvas.js:119
> +WebInspector.Canvas.ContextType = {
> + Canvas2D: "canvas-context-type-canvas-2d",
> + WebGL: "canvas-context-type-webgl",
> +};
Style: Might consider making these Symbols or simpler names. I've been moving these enum types (when not events) to simple strings, like just "canvas-2d" and "webgl". It makes it much easier when debugging.
> LayoutTests/inspector/canvas/create-canvas-contexts-expected.txt:9
> +Canvas added.
The others all say "Added canvas" this one feels weird.
> LayoutTests/inspector/canvas/create-canvas-contexts.html:9
> +if (window.testRunner)
> + testRunner.overridePreference("WebKitWebGLEnabled", "1");
I do not think this is necessary. It looks like all ports default this as enabled.
> LayoutTests/inspector/canvas/create-canvas-contexts.html:158
> + if (!canvas) {
> + reject();
> + return;
> + }
Doesn't look like this can happen. Probably would have gone to the catch path anyways.
Created attachment 312500[details]
Archive of layout-test-results from ews102 for mac-elcapitan
The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews102 Port: mac-elcapitan Platform: Mac OS X 10.11.6
Comment on attachment 312475[details]
Patch
View in context: https://bugs.webkit.org/attachment.cgi?id=312475&action=review>> Source/JavaScriptCore/inspector/protocol/Canvas.json:24
>> + { "name": "name", "type": "string", "description": "The CSS canvas identifier, or the canvas element id attribute." },
>
> The canvas element id attribute? This overloading feels weird how about just making it "cssName" and optional? Seems like this could make make the `isCSSCanvas` unnecessary.
Will remove the overload. The element ID was only used for building a display name in the frontend (Canvas #my-canvas"). Adding a DOM.NodeId property to Canvas would allow it to be retrieved in the frontend.
>> Source/WebCore/html/canvas/WebGLRenderingContextBase.cpp:55
>> +#include "InspectorInstrumentation.h"
>
> Unused. In fact all of the WebGLRenderingContextBase includes also seem unused. Seems this was intended for a later change?
Removed, but I'm not sure what "all of the WebGLRenderingContextBase" includes means.
>> Source/WebCore/inspector/InspectorCanvasAgent.cpp:70
>> + return;
>
> We shouldn't need to do this check (everywhere in this file). FrontendDispatcher will always be alive in our unique_ptr.
It looks like InspectorDOMAgent::focusNode() is the only other place making this check. Is it also unnecessary?
>> Source/WebCore/inspector/InspectorCanvasAgent.h:45
>> +typedef String ErrorString;
>
> This shouldn't be needed here, an include should handle it.
I don't think ErrorString is included from anywhere. Every agent defines ErrorString this way in it's header.
>> Source/WebCore/inspector/InspectorInstrumentation.cpp:1224
>> +
>
> Is this ever used? Is it necessary for something that I am unaware of?
Leftover from before splitting up the patch. Good eye.
>> Source/WebInspectorUI/UserInterface/Controllers/CanvasManager.js:51
>> + if (canvas.parentFrame.id === frame.id)
>
> Nit: Can we just compare `canvas.parentFrame === frame` instead of getting the ids?
Yes, but I'm going to remove this per Devin's comment.
>>> Source/WebInspectorUI/UserInterface/Models/Canvas.js:119
>>> +};
>>
>> I think that adding the prefix to these values is unnecessary.
>>
>> WebInspector.Canvas.ContextType = {
>> Canvas2D: "canvas-2d",
>> WebGL: "webgl",
>> };
>
> Style: Might consider making these Symbols or simpler names. I've been moving these enum types (when not events) to simple strings, like just "canvas-2d" and "webgl". It makes it much easier when debugging.
I like shortening them. I'm (irrationally) paranoid about collisions with other enums, so I'll go with Symbols.
>> LayoutTests/inspector/canvas/create-canvas-contexts.html:9
>> + testRunner.overridePreference("WebKitWebGLEnabled", "1");
>
> I do not think this is necessary. It looks like all ports default this as enabled.
Nice.
>> LayoutTests/inspector/canvas/create-canvas-contexts.html:93
>> + InspectorTest.evaluateInPage("createCanvas('2d')");
>
> I think that using template strings makes reading these easier, especially when using evaluateInPage.
>
> InspectorTest.evaluateInPage(`createCanvas("2d")`);
Joe has been doing this too, since it makes adding template literals easier in the future. Will adopt the pattern (at least for all evaluateInPage).
Comment on attachment 312475[details]
Patch
View in context: https://bugs.webkit.org/attachment.cgi?id=312475&action=review>>> Source/JavaScriptCore/inspector/protocol/Canvas.json:24
>>> + { "name": "name", "type": "string", "description": "The CSS canvas identifier, or the canvas element id attribute." },
>>
>> The canvas element id attribute? This overloading feels weird how about just making it "cssName" and optional? Seems like this could make make the `isCSSCanvas` unnecessary.
>
> Will remove the overload. The element ID was only used for building a display name in the frontend (Canvas #my-canvas"). Adding a DOM.NodeId property to Canvas would allow it to be retrieved in the frontend.
Also removing `isCSSCanvas`. Replacing with an optional property `cssCanvasName`. Adding optional property `nodeId` as well.
Created attachment 312519[details]
Archive of layout-test-results from ews104 for mac-elcapitan-wk2
The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews104 Port: mac-elcapitan-wk2 Platform: Mac OS X 10.11.6
Created attachment 312714[details]
Archive of layout-test-results from ews101 for mac-elcapitan
The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews101 Port: mac-elcapitan Platform: Mac OS X 10.11.6
Comment on attachment 312706[details]
Patch
r=me with the one issue on InspectorCanvasAgent.cpp:222.
View in context: https://bugs.webkit.org/attachment.cgi?id=312706&action=review> Source/WebCore/html/HTMLCanvasElement.cpp:251
> + InspectorInstrumentation::didCreateCanvasRenderingContext(*this);
NIT: Does this need to be called after `invalidateStyleAndLayerComposition()`? I am not sure of the process here, but it seems like that is an important step (it always occurs for WebGL contexts).
> Source/WebCore/inspector/InspectorCanvasAgent.cpp:222
> + if (!nodeId) {
> + ErrorString ignored;
> + nodeId = domAgent->pushNodeToFrontend(ignored, domAgent->boundNodeId(&canvasElement.document()), &canvasElement);
> + }
> + canvas->setNodeId(nodeId);
I think there is an edge-case that might cause this code to behave badly. When I was testing, I found that calling InspectorCanvasAgent::enable on a page with active <canvas> contexts would cause this code to run before the main <document> is sent to the frontend (via `WebInspector.domTreeManager.requestDocument`, which calls `DOMAgent.getDocument`). Since "nodeId" is already optional, I think we shouldn't even set a value in the case that the <document> hasn't already been sent to the frontend.
int nodeId = domAgent->boundNodeId(&canvasElement);
if (!nodeId) {
if (int documentNodeId = domAgent->boundNodeId(&canvasElement.document())) {
ErrorString ignored;
nodeId = domAgent->pushNodeToFrontend(ignored, documentNodeId, &canvasElement);
}
}
if (nodeId)
canvas->setNodeId(nodeId);
> Source/WebCore/inspector/InspectorInstrumentation.h:1095
> +
Style: remove spaces.
> Source/WebInspectorUI/UserInterface/Models/Canvas.js:34
> + console.assert(frame);
Is there a reason to not assert that the frame is a WebInspector.Frame?
console.assert(frame instanceof WebInspector.Frame);
> LayoutTests/inspector/canvas/create-canvas-contexts.html:86
> + InspectorTest.evaluateInPage(`createCanvas('2d')`);
Style: should use " instead of '
InspectorTest.evaluateInPage(`createCanvas("2d")`);
> LayoutTests/inspector/canvas/create-canvas-contexts.html:117
> + expression: "createCanvas('2d')",
Ditto.
expression: `createCanvas("2d")`,
> LayoutTests/inspector/canvas/create-canvas-contexts.html:122
> + expression: "createCanvas('webgl')",
Ditto.
expression: `createCanvas("webgl")`,
> LayoutTests/inspector/canvas/create-canvas-contexts.html:127
> + expression: "createOffscreenCanvas('2d')",
Ditto.
expression: `createOffscreenCanvas("2d")`,
> LayoutTests/inspector/canvas/create-canvas-contexts.html:132
> + expression: "createOffscreenCanvas('webgl')",
Ditto.
expression: `createOffscreenCanvas("webgl")`,
> LayoutTests/inspector/canvas/create-canvas-contexts.html:155
> + InspectorTest.evaluateInPage(`createCSSCanvas('${contextId}', '${cssCanvasIdentifier}')`);
Ditto.
InspectorTest.evaluateInPage(`createCSSCanvas("${contextId}", "${cssCanvasIdentifier}")`);
Created attachment 312718[details]
Archive of layout-test-results from ews105 for mac-elcapitan-wk2
The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews105 Port: mac-elcapitan-wk2 Platform: Mac OS X 10.11.6
Created attachment 312719[details]
Archive of layout-test-results from ews112 for mac-elcapitan
The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews112 Port: mac-elcapitan Platform: Mac OS X 10.11.6
Comment on attachment 312706[details]
Patch
View in context: https://bugs.webkit.org/attachment.cgi?id=312706&action=review> Source/WebInspectorUI/UserInterface/Models/Canvas.js:110
> + cookie[WebInspector.Canvas.NodePathCookieKey] = this._domNode ? this._domNode.path || "";
This causes an error. It should be ":", not "||".
cookie[WebInspector.Canvas.NodePathCookieKey] = this._domNode ? this._domNode.path : "";
Created attachment 312945[details]
Archive of layout-test-results from ews100 for mac-elcapitan
The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews100 Port: mac-elcapitan Platform: Mac OS X 10.11.6
Comment on attachment 313041[details]
Patch
r=me
View in context: https://bugs.webkit.org/attachment.cgi?id=313041&action=review> Source/WebCore/inspector/InspectorCanvasAgent.cpp:111
> + if (!m_enabled) {
> + m_canvasEntries.clear();
> + return;
> + }
> +
> + for (auto* canvasElement : canvasesForFrame) {
> + auto canvasEntry = m_canvasEntries.take(canvasElement);
> + m_frontendDispatcher->canvasRemoved(canvasEntry.identifier);
> + }
I don't think we want to do this. We should only be clearing the canvases for the frame that navigated:
for (auto* canvasElement : canvasesForFrame) {
auto canvasEntry = m_canvasEntries.take(canvasElement);
if (m_enabled)
m_frontendDispatcher->canvasRemoved(canvasEntry.identifier);
}
> Source/WebCore/inspector/InspectorCanvasAgent.cpp:132
> + newCanvasEntry.cssCanvasName = m_canvasToCSSCanvasId.get(&canvasElement);
> + m_canvasToCSSCanvasId.remove(&canvasElement);
I think you can just use `take` here like above:
if (m_canvasToCSSCanvasId.contains(&canvasElement))
newCanvasEntry.cssCanvasName = m_canvasToCSSCanvasId.take(&canvasElement);
> Source/WebCore/inspector/InspectorCanvasAgent.cpp:173
> + m_frontendDispatcher->canvasRemoved(identifier);
I think that it might be possible for this to run even after `disable()` has been called. I think that either this should be guarded by a `m_enabled` check or `m_timer.stop()` should be called in `disable()`.
Comment on attachment 313041[details]
Patch
View in context: https://bugs.webkit.org/attachment.cgi?id=313041&action=review> Source/WebCore/inspector/InspectorCanvasAgent.cpp:90
> +void InspectorCanvasAgent::frameNavigated(DocumentLoader* loader)
Why doesn't this just take a Frame& / Frame*? This only uses the DocumentLoader to access the Frame.
>> Source/WebCore/inspector/InspectorCanvasAgent.cpp:132
>> + m_canvasToCSSCanvasId.remove(&canvasElement);
>
> I think you can just use `take` here like above:
>
> if (m_canvasToCSSCanvasId.contains(&canvasElement))
> newCanvasEntry.cssCanvasName = m_canvasToCSSCanvasId.take(&canvasElement);
Looks like this could just be:
newCanvasEntry.cssCanvasName = m_canvasToCSSCanvasId.take(&canvasElement);
Comment on attachment 313041[details]
Patch
View in context: https://bugs.webkit.org/attachment.cgi?id=313041&action=review> Source/WebCore/inspector/InspectorCanvasAgent.cpp:150
> + canvasElement.removeObserver(*this);
HTMLCanvasElement triggers canvasDestroyed in its destructor while iterating its observers:
for (auto& observer : m_observers)
observer->canvasDestroyed(*this);
If CanvasAgent::canvasDestroyed then tries to remove itself as an observer here then we will be mutating the HashMap that we are iterating over.
I suspect this is what the ASSERT is catching and causing crashes.
Comment on attachment 313041[details]
Patch
View in context: https://bugs.webkit.org/attachment.cgi?id=313041&action=review>> Source/WebCore/inspector/InspectorCanvasAgent.cpp:90
>> +void InspectorCanvasAgent::frameNavigated(DocumentLoader* loader)
>
> Why doesn't this just take a Frame& / Frame*? This only uses the DocumentLoader to access the Frame.
I think this was left over from an older implementation. Other agents that define frameNavigated take a Frame&. Will fix.
>> Source/WebCore/inspector/InspectorCanvasAgent.cpp:150
>> + canvasElement.removeObserver(*this);
>
> HTMLCanvasElement triggers canvasDestroyed in its destructor while iterating its observers:
>
> for (auto& observer : m_observers)
> observer->canvasDestroyed(*this);
>
> If CanvasAgent::canvasDestroyed then tries to remove itself as an observer here then we will be mutating the HashMap that we are iterating over.
>
> I suspect this is what the ASSERT is catching and causing crashes.
It's safe for CanvasObservers to assume that the observer is removed as a side effect of canvasDestroyed. Will the call to removeObserver in InspectorCanvasAgent::canvasDestroyed.
>> Source/WebCore/inspector/InspectorCanvasAgent.cpp:173
>> + m_frontendDispatcher->canvasRemoved(identifier);
>
> I think that it might be possible for this to run even after `disable()` has been called. I think that either this should be guarded by a `m_enabled` check or `m_timer.stop()` should be called in `disable()`.
Good catch. Stopping the timer in `disable()` feels cleaner. Will fix.
Comment on attachment 313041[details]
Patch
View in context: https://bugs.webkit.org/attachment.cgi?id=313041&action=review>>> Source/WebCore/inspector/InspectorCanvasAgent.cpp:173
>>> + m_frontendDispatcher->canvasRemoved(identifier);
>>
>> I think that it might be possible for this to run even after `disable()` has been called. I think that either this should be guarded by a `m_enabled` check or `m_timer.stop()` should be called in `disable()`.
>
> Good catch. Stopping the timer in `disable()` feels cleaner. Will fix.
If you go that route, you're also going to have to make sure to call `m_removedCanvasIdentifiers.clear();` so that if the agent is enabled again later on there aren't any leftover removed canvases that dispatch events.
Comment on attachment 313140[details]
Patch
Rejecting attachment 313140[details] from commit-queue.
Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.webkit.org', '--bot-id=webkit-cq-03', 'land-attachment', '--force-clean', '--non-interactive', '--parent-command=commit-queue', 313140, '--port=mac']" exit_code: 2 cwd: /Volumes/Data/EWS/WebKit
Last 500 characters of output:
LayoutTests
:040000 040000 00c58440e2558cd462d45456051fbb940ff4883b df50ec9c69de9742bac5f1e63c6813cd5d3d421b M Source
Current branch master is up to date.
ERROR: Not all changes have been committed into SVN, however the committed
ones (if any) seem to be successfully integrated into the working tree.
Please see the above messages for details.
Failed to run "['git', 'svn', 'dcommit', '--rmdir']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit
Updating OpenSource
Current branch master is up to date.
Full output: http://webkit-queues.webkit.org/results/3944929
2017-06-06 16:15 PDT, Matt Baker
2017-06-06 17:03 PDT, Build Bot
2017-06-06 17:15 PDT, Build Bot
2017-06-06 17:24 PDT, Build Bot
2017-06-08 18:01 PDT, Matt Baker
2017-06-08 19:06 PDT, Build Bot
2017-06-08 19:22 PDT, Build Bot
2017-06-08 20:52 PDT, Matt Baker
2017-06-08 21:54 PDT, Build Bot
2017-06-08 21:58 PDT, Build Bot
2017-06-08 22:16 PDT, Build Bot
2017-06-09 12:50 PDT, Matt Baker
2017-06-09 14:17 PDT, Build Bot
2017-06-09 15:22 PDT, Build Bot
2017-06-09 17:58 PDT, Build Bot
2017-06-12 15:14 PDT, Matt Baker
2017-06-12 15:54 PDT, Build Bot
2017-06-12 16:19 PDT, Build Bot
2017-06-12 16:23 PDT, Build Bot
2017-06-14 18:36 PDT, Matt Baker
2017-06-14 19:21 PDT, Build Bot
2017-06-15 16:46 PDT, Matt Baker
2017-06-15 18:33 PDT, Matt Baker
2017-06-16 14:35 PDT, Matt Baker
2017-06-16 14:38 PDT, Matt Baker
2017-06-16 17:46 PDT, Matt Baker