Bug 39138 – [chromium] Fix LayerChromium hierarchy update methods

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Vangelis Kokkevis]

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().

[Vangelis Kokkevis]

Created attachment 56112 [details]
Proposed patch

[Darin Fisher (:fishd, Google)]

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.

[Vangelis Kokkevis]

Created attachment 56122 [details]
Patch addressing review comments

[Vangelis Kokkevis]

(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.

[Darin Fisher (:fishd, Google)]

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)

[Vangelis Kokkevis]

Created attachment 56125 [details]
patch - Fixing ASSERT issue

[WebKit Commit Bot]

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 [e-mail]...
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).

[Adam Barth]

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.

[Adam Barth]

Created attachment 56157 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 56157 [details]
Patch for landing

Clearing flags on attachment: 56157

Committed r59554: <http://trac.webkit.org/changeset/59554>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[WebKit Review Bot]

http://trac.webkit.org/changeset/59554 might have broken Tiger Intel Release

[Eric Seidel (no email)]

Yes. This broke two builders. I need a way to roll out from my phone. :(

[Adam Barth]

Really?  That seems odd.  I'll investigate.

[Adam Barth]

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.

[Adam Barth]

The bots rolled green again.