WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
NEW
140138
Optimize GraphicsLayer::removeAllChildren()
https://bugs.webkit.org/show_bug.cgi?id=140138
Summary
Optimize GraphicsLayer::removeAllChildren()
Zan Dobersek
Reported
2015-01-06 11:52:16 PST
Optimize GraphicsLayer::removeAllChildren()
Attachments
Patch
(1.81 KB, patch)
2015-01-06 11:59 PST
,
Zan Dobersek
no flags
Details
Formatted Diff
Diff
Patch
(6.34 KB, patch)
2015-01-07 06:31 PST
,
Zan Dobersek
no flags
Details
Formatted Diff
Diff
Patch
(6.92 KB, patch)
2015-02-02 10:14 PST
,
Zan Dobersek
no flags
Details
Formatted Diff
Diff
Patch
(6.94 KB, patch)
2015-02-02 13:41 PST
,
Zan Dobersek
no flags
Details
Formatted Diff
Diff
Archive of layout-test-results from ews106 for mac-mavericks-wk2
(247.49 KB, application/zip)
2015-02-02 14:31 PST
,
Build Bot
no flags
Details
Patch
(7.01 KB, patch)
2015-02-04 10:16 PST
,
Zan Dobersek
simon.fraser
: review-
Details
Formatted Diff
Diff
Show Obsolete
(4)
View All
Add attachment
proposed patch, testcase, etc.
Zan Dobersek
Comment 1
2015-01-06 11:59:30 PST
Created
attachment 244083
[details]
Patch
Simon Fraser (smfr)
Comment 2
2015-01-06 12:16:28 PST
Comment on
attachment 244083
[details]
Patch This breaks Mac/iOS because removeFromParent is no longer called, so call to noteSublayersChanged() no longer happens.
Zan Dobersek
Comment 3
2015-01-07 05:54:07 PST
(In reply to
comment #2
)
> Comment on
attachment 244083
[details]
> Patch > > This breaks Mac/iOS because removeFromParent is no longer called, so call to > noteSublayersChanged() no longer happens.
Did not spot that one.
Zan Dobersek
Comment 4
2015-01-07 06:31:00 PST
Created
attachment 244153
[details]
Patch
Chris Dumez
Comment 5
2015-01-27 20:48:42 PST
Comment on
attachment 244153
[details]
Patch View in context:
https://bugs.webkit.org/attachment.cgi?id=244153&action=review
> Source/WebCore/platform/graphics/ca/GraphicsLayerCA.h:78 > + WEBCORE_EXPORT virtual void removeAllChildren() override;
Please add __ZN7WebCore15GraphicsLayerCA17removeAllChildrenEv to WebCore.exp.in then run Tools/Scripts/sort-export-file. This should fix mac build
Zan Dobersek
Comment 6
2015-02-02 10:14:52 PST
Created
attachment 245884
[details]
Patch
Simon Fraser (smfr)
Comment 7
2015-02-02 10:41:29 PST
Comment on
attachment 245884
[details]
Patch View in context:
https://bugs.webkit.org/attachment.cgi?id=245884&action=review
> Source/WebCore/platform/graphics/ca/GraphicsLayerCA.cpp:456 > + noteSublayersChanged(); > + GraphicsLayer::removeAllChildren();
I think this should be flipped.
> Source/WebCore/platform/graphics/texmap/coordinated/CoordinatedGraphicsLayer.cpp:191 > + didChangeChildren(); > + GraphicsLayer::removeAllChildren();
Ditto.
Zan Dobersek
Comment 8
2015-02-02 13:41:20 PST
Created
attachment 245897
[details]
Patch
Build Bot
Comment 9
2015-02-02 14:31:32 PST
Comment on
attachment 245897
[details]
Patch
Attachment 245897
[details]
did not pass mac-wk2-ews (mac-wk2): Output:
http://webkit-queues.appspot.com/results/5014396556279808
Number of test failures exceeded the failure limit.
Build Bot
Comment 10
2015-02-02 14:31:36 PST
Created
attachment 245901
[details]
Archive of layout-test-results from ews106 for mac-mavericks-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: ews106 Port: mac-mavericks-wk2 Platform: Mac OS X 10.9.5
Zan Dobersek
Comment 11
2015-02-04 09:59:54 PST
Comment on
attachment 245884
[details]
Patch View in context:
https://bugs.webkit.org/attachment.cgi?id=245884&action=review
>> Source/WebCore/platform/graphics/ca/GraphicsLayerCA.cpp:456 >> + GraphicsLayer::removeAllChildren(); > > I think this should be flipped.
This (and the previous iteration as well) caused a bunch of assertions on Mac -- noteSublayersChanged() has to be called for parent once for each child that is being removed.
Zan Dobersek
Comment 12
2015-02-04 09:59:58 PST
Comment on
attachment 245884
[details]
Patch View in context:
https://bugs.webkit.org/attachment.cgi?id=245884&action=review
>> Source/WebCore/platform/graphics/ca/GraphicsLayerCA.cpp:456 >> + GraphicsLayer::removeAllChildren(); > > I think this should be flipped.
This (and the previous iteration as well) caused a bunch of assertions on Mac -- noteSublayersChanged() has to be called for parent once for each child that is being removed.
Zan Dobersek
Comment 13
2015-02-04 10:16:40 PST
Created
attachment 246037
[details]
Patch
Darin Adler
Comment 14
2015-02-07 07:54:31 PST
Comment on
attachment 246037
[details]
Patch View in context:
https://bugs.webkit.org/attachment.cgi?id=246037&action=review
> Source/WebCore/platform/graphics/ca/GraphicsLayerCA.cpp:465 > + for (size_t i = 0; i < m_children.size(); ++i) > + noteSublayersChanged();
Do we really need to call noteSublayersChanged over and over again? How is that useful?
Simon Fraser (smfr)
Comment 15
2015-02-07 08:52:41 PST
Comment on
attachment 246037
[details]
Patch View in context:
https://bugs.webkit.org/attachment.cgi?id=246037&action=review
>> Source/WebCore/platform/graphics/ca/GraphicsLayerCA.cpp:465 >> + noteSublayersChanged(); > > Do we really need to call noteSublayersChanged over and over again? How is that useful?
No, we only need to call it once.
Zan Dobersek
Comment 16
2015-02-10 08:43:45 PST
Comment on
attachment 246037
[details]
Patch View in context:
https://bugs.webkit.org/attachment.cgi?id=246037&action=review
>>> Source/WebCore/platform/graphics/ca/GraphicsLayerCA.cpp:465 >>> + noteSublayersChanged(); >> >> Do we really need to call noteSublayersChanged over and over again? How is that useful? > > No, we only need to call it once.
As I wrote in
comment #11
, Mac EWS rejected the patch that only called noteSublayersChanged() (
comment #9
and
comment #10
). I'm able to confirm the problem on Mac, with lots of crashes regardless of whether noteSublayersChanged() is called before or after GraphicsLayer::removeAllChildren(). I know nothing about CA and why the crashes occur, but keeping the previous behaviour of notifying about changed sublayers for each removed layer works without a problem. I'll attach backtraces from a debug build.
Zan Dobersek
Comment 17
2015-02-10 08:43:47 PST
Comment on
attachment 246037
[details]
Patch View in context:
https://bugs.webkit.org/attachment.cgi?id=246037&action=review
>>> Source/WebCore/platform/graphics/ca/GraphicsLayerCA.cpp:465 >>> + noteSublayersChanged(); >> >> Do we really need to call noteSublayersChanged over and over again? How is that useful? > > No, we only need to call it once.
As I wrote in
comment #11
, Mac EWS rejected the patch that only called noteSublayersChanged() (
comment #9
and
comment #10
). I'm able to confirm the problem on Mac, with lots of crashes regardless of whether noteSublayersChanged() is called before or after GraphicsLayer::removeAllChildren(). I know nothing about CA and why the crashes occur, but keeping the previous behaviour of notifying about changed sublayers for each removed layer works without a problem. I'll attach backtraces from a debug build.
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug