LayerChromium methods for handling sublayers do not always properly update the parent to child and child to parent pointers in sync. For example, insertSublayer() will not remove the layer from it's current parent before re-parenting it and a similar issue exists with setSublayers().
Created attachment 56112 [details] Proposed patch
Comment on attachment 56112 [details] Proposed patch WebCore/ChangeLog:5 + comment #0 from the bug report contains a nice description of the issues this change is fixing. it'd be good to include that info here. WebCore/platform/graphics/chromium/LayerChromium.cpp:81 + removeAllSublayers(); It looks like the point here is to trigger the call to notifySyncRequired? The clearing of m_sublayers does not seem to be critical since that will happen anyways because we are inside ~LayerChromium. WebCore/platform/graphics/chromium/LayerChromium.cpp:175 + if (referenceIndex == -1) nit: it's good to use ASSERT_NOT_REACHED so you don't have to repeat the expression in cases like this. LGTM otherwise. The ownership changes make sense. R- for these nits.
Created attachment 56122 [details] Patch addressing review comments
(In reply to comment #2) > (From update of attachment 56112 [details]) > WebCore/ChangeLog:5 > + > comment #0 from the bug report contains a nice description of the > issues this change is fixing. it'd be good to include that info > here. > Done. > WebCore/platform/graphics/chromium/LayerChromium.cpp:81 > + removeAllSublayers(); > It looks like the point here is to trigger the call to notifySyncRequired? > The clearing of m_sublayers does not seem to be critical since that will > happen anyways because we are inside ~LayerChromium. > Good point. I don't actually need to call notifySyncRequired since to get to the destructor this layer must have already been removed from a parent that would have called the method. > WebCore/platform/graphics/chromium/LayerChromium.cpp:175 > + if (referenceIndex == -1) > nit: it's good to use ASSERT_NOT_REACHED so you don't have to > repeat the expression in cases like this. Done. > > LGTM otherwise. The ownership changes make sense. R- for these nits.
Comment on attachment 56122 [details] Patch addressing review comments WebCore/platform/graphics/chromium/LayerChromium.cpp:174 + ASSERT_NOT_REACHED(); it looked like you had previously made this return to avoid crashing in release mode. is that still important? if not, then this could just be written ASSERT(referenceIndex != -1)
Created attachment 56125 [details] patch - Fixing ASSERT issue
Comment on attachment 56125 [details] patch - Fixing ASSERT issue Rejecting patch 56125 from commit-queue. Unexpected failure when landing patch! Please file a bug against webkit-patch. Failed to run "['./WebKitTools/Scripts/webkit-patch', '--status-host=webkit-commit-queue.appspot.com', 'land-attachment', '--force-clean', '--build', '--non-interactive', '--ignore-builders', '--build-style=both', '--quiet', 56125, '--test', '--parent-command=commit-queue', '--no-update']" exit_code: 1 Logging in as eseidel@chromium.org... Fetching: https://bugs.webkit.org/attachment.cgi?id=56125&action=edit Fetching: https://bugs.webkit.org/show_bug.cgi?id=39138&ctype=xml Processing 1 patch from 1 bug. Cleaning working directory Processing patch 56125 from bug 39138. ERROR: /Users/eseidel/Projects/CommitQueue/WebCore/ChangeLog neither lists a valid reviewer nor contains the string "Unreviewed" or "Rubber stamp" (case insensitive).
In the future, please do not remove the Reviewed by NOBODY line from the ChangeLog. The tools use that to fill in the proper reviewer.
Created attachment 56157 [details] Patch for landing
Comment on attachment 56157 [details] Patch for landing Clearing flags on attachment: 56157 Committed r59554: <http://trac.webkit.org/changeset/59554>
All reviewed patches have been landed. Closing bug.
http://trac.webkit.org/changeset/59554 might have broken Tiger Intel Release
Yes. This broke two builders. I need a way to roll out from my phone. :(
Really? That seems odd. I'll investigate.
The gtk failures is a different test, and it's only failed once. fast/workers/storage/execute-sql-args-worker.html This patch didn't touch any non-chromium code. I'm running the tests again to confirm.
The bots rolled green again.