Summary: | Decouple the percent height and positioned descendants maps. | ||
---|---|---|---|
Product: | WebKit | Reporter: | zalan <zalan> |
Component: | Layout and Rendering | Assignee: | zalan <zalan> |
Status: | RESOLVED FIXED | ||
Severity: | Normal | CC: | buildbot, cdumez, commit-queue, esprehn+autocc, glenn, hyatt, kondapallykalyan, rniwa, simon.fraser |
Priority: | P2 | ||
Version: | WebKit Nightly Build | ||
Hardware: | Unspecified | ||
OS: | Unspecified | ||
Attachments: |
Description
zalan
2016-06-14 20:07:32 PDT
Created attachment 281322 [details]
Patch
Comment on attachment 281322 [details] Patch Attachment 281322 [details] did not pass mac-ews (mac): Output: http://webkit-queues.webkit.org/results/1504857 New failing tests: fast/block/fixed-position-reparent-when-transition-is-removed.html Created attachment 281327 [details]
Archive of layout-test-results from ews102 for mac-yosemite
The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews102 Port: mac-yosemite Platform: Mac OS X 10.10.5
Comment on attachment 281322 [details] Patch Attachment 281322 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.webkit.org/results/1504870 New failing tests: fast/block/fixed-position-reparent-when-transition-is-removed.html Created attachment 281328 [details]
Archive of layout-test-results from ews105 for mac-yosemite-wk2
The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews105 Port: mac-yosemite-wk2 Platform: Mac OS X 10.10.5
Comment on attachment 281322 [details] Patch Attachment 281322 [details] did not pass ios-sim-ews (ios-simulator-wk2): Output: http://webkit-queues.webkit.org/results/1504872 New failing tests: fast/block/fixed-position-reparent-when-transition-is-removed.html Created attachment 281330 [details]
Archive of layout-test-results from ews126 for ios-simulator-wk2
The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews126 Port: ios-simulator-wk2 Platform: Mac OS X 10.11.4
Comment on attachment 281322 [details] Patch Attachment 281322 [details] did not pass mac-debug-ews (mac): Output: http://webkit-queues.webkit.org/results/1504895 New failing tests: fast/block/fixed-position-reparent-when-transition-is-removed.html Created attachment 281332 [details]
Archive of layout-test-results from ews114 for mac-yosemite
The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews114 Port: mac-yosemite Platform: Mac OS X 10.10.5
Created attachment 281333 [details]
Patch
Comment on attachment 281333 [details]
Patch
r=me
Comment on attachment 281333 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=281333&action=review > Source/WebCore/rendering/RenderBlock.cpp:98 > static TrackedDescendantsMap* gPercentHeightDescendantsMap; We normally don't use 'g' prefix for statics. > Source/WebCore/rendering/RenderBlock.cpp:108 > + auto* descendantSet = gPercentHeightDescendantsMap->get(&container); Could use HashMap::ensure() and std::make_unique<>() > Source/WebCore/rendering/RenderBlock.cpp:121 > + auto* containerSet = gPercentHeightContainerMap->get(&descendant); Could use HashMap::ensure() and std::make_unique<>() > Source/WebCore/rendering/RenderBlock.cpp:148 > auto* descendantSet = descendantsMapIterator->value.get(); Auto& descendantSet = *descendantsMapIterator->value; > Source/WebCore/rendering/RenderBlock.cpp:158 > + void addDescendant(const RenderBlock& containingBlock, RenderBox& positionedDescendant, bool moveDescendantToEnd = false) The bool parameter would read nicer at call sites if this used an enum class: Enum class MoveDescendantToEnd { No, Yes } > Source/WebCore/rendering/RenderBlock.cpp:204 > + for (const auto* renderer : *descendants) Const does not really bring much. > Source/WebCore/rendering/RenderBlock.cpp:214 > + typedef HashMap<const RenderBlock*, std::unique_ptr<TrackedRendererListHashSet>> DescendantsMap; We using using instead of typedef these days: Using DescendantsMap = HashMap<const RenderBlock*, std::unique_ptr<TrackedRendererListHashSet>>; > Source/WebCore/rendering/RenderBlock.cpp:325 > + if (std::unique_ptr<TrackedRendererListHashSet> descendantSet = gPercentHeightDescendantsMap->take(block)) { We would usually do the if (!....) Return early. > Source/WebCore/rendering/RenderBlock.cpp:327 > for (TrackedRendererListHashSet::iterator descendant = descendantSet->begin(); descendant != end; ++descendant) { Would probably be nicer with a new range loop. > Source/WebCore/rendering/RenderBlock.cpp:328 > + TrackedContainerMap::iterator it = gPercentHeightContainerMap->find(*descendant); auto > Source/WebCore/rendering/RenderBlock.cpp:3834 > + for (auto it = positionedDescendants->begin(), end = positionedDescendants->end(); it != end; ++it) { New range loop? Created attachment 281428 [details]
Patch
(In reply to comment #12) > Comment on attachment 281333 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=281333&action=review > > > Source/WebCore/rendering/RenderBlock.cpp:98 > > static TrackedDescendantsMap* gPercentHeightDescendantsMap; > > We normally don't use 'g' prefix for statics. > > > Source/WebCore/rendering/RenderBlock.cpp:108 > > + auto* descendantSet = gPercentHeightDescendantsMap->get(&container); > > Could use HashMap::ensure() and std::make_unique<>() > > > Source/WebCore/rendering/RenderBlock.cpp:121 > > + auto* containerSet = gPercentHeightContainerMap->get(&descendant); > > Could use HashMap::ensure() and std::make_unique<>() > > > Source/WebCore/rendering/RenderBlock.cpp:148 > > auto* descendantSet = descendantsMapIterator->value.get(); > > Auto& descendantSet = *descendantsMapIterator->value; > > > Source/WebCore/rendering/RenderBlock.cpp:158 > > + void addDescendant(const RenderBlock& containingBlock, RenderBox& positionedDescendant, bool moveDescendantToEnd = false) > > The bool parameter would read nicer at call sites if this used an enum class: > Enum class MoveDescendantToEnd { No, Yes } > > > Source/WebCore/rendering/RenderBlock.cpp:204 > > + for (const auto* renderer : *descendants) > > Const does not really bring much. > > > Source/WebCore/rendering/RenderBlock.cpp:214 > > + typedef HashMap<const RenderBlock*, std::unique_ptr<TrackedRendererListHashSet>> DescendantsMap; > > We using using instead of typedef these days: > Using DescendantsMap = HashMap<const RenderBlock*, > std::unique_ptr<TrackedRendererListHashSet>>; > > > Source/WebCore/rendering/RenderBlock.cpp:325 > > + if (std::unique_ptr<TrackedRendererListHashSet> descendantSet = gPercentHeightDescendantsMap->take(block)) { > > We would usually do the if (!....) Return early. > > > Source/WebCore/rendering/RenderBlock.cpp:327 > > for (TrackedRendererListHashSet::iterator descendant = descendantSet->begin(); descendant != end; ++descendant) { > > Would probably be nicer with a new range loop. > > > Source/WebCore/rendering/RenderBlock.cpp:328 > > + TrackedContainerMap::iterator it = gPercentHeightContainerMap->find(*descendant); > > auto > > > Source/WebCore/rendering/RenderBlock.cpp:3834 > > + for (auto it = positionedDescendants->begin(), end = positionedDescendants->end(); it != end; ++it) { > > New range loop? Done. Comment on attachment 281428 [details] Patch Clearing flags on attachment: 281428 Committed r202123: <http://trac.webkit.org/changeset/202123> All reviewed patches have been landed. Closing bug. |