Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2262973002:
Switch override containing block sizes to use LayoutRareData (Closed)
|
Created:
4 years, 4 months ago by cbiesinger Modified:
4 years, 4 months ago CC:
chromium-reviews, szager+layoutwatch_chromium.org, zoltan1, blink-reviews-layout_chromium.org, pdr+renderingwatchlist_chromium.org, eae+blinkwatch, leviw+renderwatch, jchaffraix+rendering, blink-reviews Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionSwitch override containing block sizes to use LayoutRareData
...instead of the ugly global map.
R=rego@igalia.com,jfernandez@igalia.com
Committed: https://crrev.com/f19cf1208967951a7e32df31020e5120d551489c
Cr-Commit-Position: refs/heads/master@{#413466}
Patch Set 1 #
Total comments: 2
Patch Set 2 : add a bool #
Messages
Total messages: 20 (9 generated)
The CQ bit was checked by cbiesinger@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Note, I am checking != -2 instead of >= -1 due to https://crbug.com/639620
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2262973002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/layout/LayoutBox.cpp (right): https://codereview.chromium.org/2262973002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/layout/LayoutBox.cpp:1158: return m_rareData && m_rareData->m_overrideContainingBlockContentLogicalHeight != LayoutUnit(-2); thinking about this ... is this "-2" value intended to avoid the double access to the hashmat due to the "contains" and "get and compare" with "-1" ? https://codereview.chromium.org/2262973002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/layout/LayoutBox.h (right): https://codereview.chromium.org/2262973002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/layout/LayoutBox.h:81: // undefined, so we use -2 to indicate that no override has been set. I'm not sure whether we need such a difference between "unset" and "undefined". Perhaps I'm missing something, but I'd always thought when override wasn't set it meant it was "undefined".
On 2016/08/22 14:56:03, jfernandez wrote: > https://codereview.chromium.org/2262973002/diff/1/third_party/WebKit/Source/c... > File third_party/WebKit/Source/core/layout/LayoutBox.cpp (right): > > https://codereview.chromium.org/2262973002/diff/1/third_party/WebKit/Source/c... > third_party/WebKit/Source/core/layout/LayoutBox.cpp:1158: return m_rareData && > m_rareData->m_overrideContainingBlockContentLogicalHeight != LayoutUnit(-2); > thinking about this ... is this "-2" value intended to avoid the double access > to the hashmat due to the "contains" and "get and compare" with "-1" ? I'm confused -- there's no hashmap anymore..? > https://codereview.chromium.org/2262973002/diff/1/third_party/WebKit/Source/c... > File third_party/WebKit/Source/core/layout/LayoutBox.h (right): > > https://codereview.chromium.org/2262973002/diff/1/third_party/WebKit/Source/c... > third_party/WebKit/Source/core/layout/LayoutBox.h:81: // undefined, so we use -2 > to indicate that no override has been set. > I'm not sure whether we need such a difference between "unset" and "undefined". > Perhaps I'm missing something, but I'd always thought when override wasn't set > it meant it was "undefined". No, when no override is set we go through the normal path to find a height. E.g. https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/layout/La... Whereas here we explicitly set it to -1 to force the height to be undefined: https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/layout/La... Unless I misunderstood something?
On 2016/08/22 14:59:09, cbiesinger wrote: > On 2016/08/22 14:56:03, jfernandez wrote: > > > https://codereview.chromium.org/2262973002/diff/1/third_party/WebKit/Source/c... > > File third_party/WebKit/Source/core/layout/LayoutBox.cpp (right): > > > > > https://codereview.chromium.org/2262973002/diff/1/third_party/WebKit/Source/c... > > third_party/WebKit/Source/core/layout/LayoutBox.cpp:1158: return m_rareData && > > m_rareData->m_overrideContainingBlockContentLogicalHeight != LayoutUnit(-2); > > thinking about this ... is this "-2" value intended to avoid the double access > > to the hashmat due to the "contains" and "get and compare" with "-1" ? > > I'm confused -- there's no hashmap anymore..? > You're right. My mistake. See my replies below, where the valid points wee made. > > > https://codereview.chromium.org/2262973002/diff/1/third_party/WebKit/Source/c... > > File third_party/WebKit/Source/core/layout/LayoutBox.h (right): > > > > > https://codereview.chromium.org/2262973002/diff/1/third_party/WebKit/Source/c... > > third_party/WebKit/Source/core/layout/LayoutBox.h:81: // undefined, so we use > -2 > > to indicate that no override has been set. > > I'm not sure whether we need such a difference between "unset" and > "undefined". > > Perhaps I'm missing something, but I'd always thought when override wasn't set > > it meant it was "undefined". > > No, when no override is set we go through the normal path to find a height. E.g. Yes, you are right there is such a difference. My confusion came from the fact that for Grid you always that such override, except for the initial steps of the sizing algorithm. Hence, we only use the hasXXX api to ensure there is not a previous value before initializing it to -1. We also used the hasXXX to discriminate between regular boxes and grid layout logic. Now that we want to generalize the use of the overrideContainingBlockWidth/Height api perhaps we need a broader meaning of hasXXX > https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/layout/La... > > Whereas here we explicitly set it to -1 to force the height to be undefined: > https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/layout/La... > > Unless I misunderstood something? No, you were right, indeed. however, I still don't like too much the idea of introducing a new hacked value to indicate a new state. It's true that now that we don't have the HashMap is not possible to check for an object presence. Shouldn't we define an enumeration with the possible states ?
Fair enough. I'll add a bool for each of the two variables to indicate whether
they are set. I don't think we need an enum -- the meaning should be fairly
clear with a bool (m_hasOverrideContainingBlockLogical{Height,Width}).
The CQ bit was checked by cbiesinger@chromium.org to run a CQ dry run
Please take another look!
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
LGTM
The CQ bit was unchecked by cbiesinger@google.com
The CQ bit was checked by cbiesinger@google.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Committed patchset #2 (id:20001)
Message was sent while issue was closed.
Description was changed from ========== Switch override containing block sizes to use LayoutRareData ...instead of the ugly global map. R=rego@igalia.com,jfernandez@igalia.com ========== to ========== Switch override containing block sizes to use LayoutRareData ...instead of the ugly global map. R=rego@igalia.com,jfernandez@igalia.com Committed: https://crrev.com/f19cf1208967951a7e32df31020e5120d551489c Cr-Commit-Position: refs/heads/master@{#413466} ==========
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/f19cf1208967951a7e32df31020e5120d551489c Cr-Commit-Position: refs/heads/master@{#413466} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
