[webkit-reviews] review denied: [Bug 39138] [chromium] Fix LayerChromium hierarchy update methods : [Attachment 56112] Proposed patch

[bugzilla-daemon at webkit.org]

Darin Fisher (:fishd, Google) <fishd at chromium.org> has denied Vangelis Kokkevis
<vangelis at chromium.org>'s request for review:
Bug 39138: [chromium] Fix LayerChromium hierarchy update methods
https://bugs.webkit.org/show_bug.cgi?id=39138

Attachment 56112: Proposed patch
https://bugs.webkit.org/attachment.cgi?id=56112&action=review

------- Additional Comments from Darin Fisher (:fishd, Google)
<fishd at chromium.org>
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.