Bug 39138 - [chromium] Fix LayerChromium hierarchy update methods
Summary: [chromium] Fix LayerChromium hierarchy update methods
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC OS X 10.5
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2010-05-14 14:35 PDT by Vangelis Kokkevis
Modified: 2010-05-15 16:53 PDT (History)
7 users (show)

See Also:


Attachments
Proposed patch (5.76 KB, patch)
2010-05-14 14:48 PDT, Vangelis Kokkevis
fishd: review-
Details | Formatted Diff | Diff
Patch addressing review comments (5.80 KB, patch)
2010-05-14 16:53 PDT, Vangelis Kokkevis
no flags Details | Formatted Diff | Diff
patch - Fixing ASSERT issue (5.83 KB, patch)
2010-05-14 17:08 PDT, Vangelis Kokkevis
no flags Details | Formatted Diff | Diff
Patch for landing (6.10 KB, patch)
2010-05-15 09:12 PDT, Adam Barth
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Vangelis Kokkevis 2010-05-14 14:35:24 PDT
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().
Comment 1 Vangelis Kokkevis 2010-05-14 14:48:57 PDT
Created attachment 56112 [details]
Proposed patch
Comment 2 Darin Fisher (:fishd, Google) 2010-05-14 16:30:33 PDT
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.
Comment 3 Vangelis Kokkevis 2010-05-14 16:53:20 PDT
Created attachment 56122 [details]
Patch addressing review comments
Comment 4 Vangelis Kokkevis 2010-05-14 16:54:29 PDT
(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 5 Darin Fisher (:fishd, Google) 2010-05-14 17:00:53 PDT
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)
Comment 6 Vangelis Kokkevis 2010-05-14 17:08:47 PDT
Created attachment 56125 [details]
patch - Fixing ASSERT issue
Comment 7 WebKit Commit Bot 2010-05-15 08:01:57 PDT
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).
Comment 8 Adam Barth 2010-05-15 09:09:04 PDT
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.
Comment 9 Adam Barth 2010-05-15 09:12:50 PDT
Created attachment 56157 [details]
Patch for landing
Comment 10 WebKit Commit Bot 2010-05-15 15:19:14 PDT
Comment on attachment 56157 [details]
Patch for landing

Clearing flags on attachment: 56157

Committed r59554: <http://trac.webkit.org/changeset/59554>
Comment 11 WebKit Commit Bot 2010-05-15 15:19:21 PDT
All reviewed patches have been landed.  Closing bug.
Comment 12 WebKit Review Bot 2010-05-15 16:11:12 PDT
http://trac.webkit.org/changeset/59554 might have broken Tiger Intel Release
Comment 13 Eric Seidel (no email) 2010-05-15 16:21:15 PDT
Yes. This broke two builders. I need a way to roll out from my phone. :(
Comment 14 Adam Barth 2010-05-15 16:21:51 PDT
Really?  That seems odd.  I'll investigate.
Comment 15 Adam Barth 2010-05-15 16:23:28 PDT
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.
Comment 16 Adam Barth 2010-05-15 16:53:48 PDT
The bots rolled green again.