Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1933193002:
Restructure LayoutSVGText::layout (Closed)
|
Created:
4 years, 7 months ago by fs Modified:
4 years, 7 months ago CC:
fs, blink-reviews, blink-reviews-layout_chromium.org, chromium-reviews, krit, eae+blinkwatch, f(malita), gyuyoung2, jchaffraix+rendering, kouhei+svg_chromium.org, leviw+renderwatch, pdr+renderingwatchlist_chromium.org, pdr+svgwatchlist_chromium.org, rwlbuis, Stephen Chennney, szager+layoutwatch_chromium.org, zoltan1 Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionRestructure LayoutSVGText::layout
"Uncascade" LayoutSVGText::layout by separating the handling of the two
flags (m_needsTextMetricsUpdate, m_needsPositioningValuesUpdate) into
sequential blocks. Add assert to verify consistency.
BUG=607906
Committed: https://crrev.com/731c92d1821ff04b324eb334f484337005d713c9
Cr-Commit-Position: refs/heads/master@{#390691}
Patch Set 1 #
Total comments: 8
Messages
Total messages: 13 (6 generated)
Description was changed from ========== Restructure LayoutSVGText::layout BUG= ========== to ========== Restructure LayoutSVGText::layout "Uncascade" LayoutSVGText::layout by separating the handling of the two flags (m_needsTextMetricsUpdate, m_needsPositioningValueUpdate) into sequential blocks. Add assert to verify consistency. BUG= ==========
Description was changed from ========== Restructure LayoutSVGText::layout "Uncascade" LayoutSVGText::layout by separating the handling of the two flags (m_needsTextMetricsUpdate, m_needsPositioningValueUpdate) into sequential blocks. Add assert to verify consistency. BUG= ========== to ========== Restructure LayoutSVGText::layout "Uncascade" LayoutSVGText::layout by separating the handling of the two flags (m_needsTextMetricsUpdate, m_needsPositioningValuesUpdate) into sequential blocks. Add assert to verify consistency. BUG= ==========
Description was changed from ========== Restructure LayoutSVGText::layout "Uncascade" LayoutSVGText::layout by separating the handling of the two flags (m_needsTextMetricsUpdate, m_needsPositioningValuesUpdate) into sequential blocks. Add assert to verify consistency. BUG= ========== to ========== Restructure LayoutSVGText::layout "Uncascade" LayoutSVGText::layout by separating the handling of the two flags (m_needsTextMetricsUpdate, m_needsPositioningValuesUpdate) into sequential blocks. Add assert to verify consistency. BUG=607906 ==========
fs@opera.com changed reviewers: + fmalita@chromium.org, schenney@chromium.org
The CQ bit was checked by schenney@chromium.org
lgtm
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1933193002/1 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1933193002/1
Message was sent while issue was closed.
Committed patchset #1 (id:1)
Message was sent while issue was closed.
LGTM (belated) with some nits - feel free to ignore or address in other CLs touching this code. https://codereview.chromium.org/1933193002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/layout/svg/LayoutSVGText.cpp (left): https://codereview.chromium.org/1933193002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/layout/svg/LayoutSVGText.cpp:200: collectLayoutAttributes(this, m_layoutAttributes); I guess updateFontMetrics() doesn't depend on collectLayoutAttributes()? https://codereview.chromium.org/1933193002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/layout/svg/LayoutSVGText.cpp (right): https://codereview.chromium.org/1933193002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/layout/svg/LayoutSVGText.cpp:197: ASSERT(!m_needsReordering); Nit: can we move the assert either at the beginning or closer to use location? https://codereview.chromium.org/1933193002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/layout/svg/LayoutSVGText.cpp:227: updateParentBoundaries = true; Can we use a TemporaryChange<bool> in the outer scope to avoid an explicit reset below? (would have to be wrapped in Optional<>, see if it makes sense) https://codereview.chromium.org/1933193002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/layout/svg/LayoutSVGText.cpp:260: m_needsReordering = false; Nit: make this unconditional?
Message was sent while issue was closed.
On 2016/04/29 at 17:50:54, fmalita wrote: > LGTM (belated) with some nits - feel free to ignore or address in other CLs touching this code. Comments below. Addressed in https://codereview.chromium.org/1933413002. https://codereview.chromium.org/1933193002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/layout/svg/LayoutSVGText.cpp (left): https://codereview.chromium.org/1933193002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/layout/svg/LayoutSVGText.cpp:200: collectLayoutAttributes(this, m_layoutAttributes); On 2016/04/29 at 17:50:54, f(malita) wrote: > I guess updateFontMetrics() doesn't depend on collectLayoutAttributes()? Correct. https://codereview.chromium.org/1933193002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/layout/svg/LayoutSVGText.cpp (right): https://codereview.chromium.org/1933193002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/layout/svg/LayoutSVGText.cpp:197: ASSERT(!m_needsReordering); On 2016/04/29 at 17:50:54, f(malita) wrote: > Nit: can we move the assert either at the beginning or closer to use location? Moved to beginning. https://codereview.chromium.org/1933193002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/layout/svg/LayoutSVGText.cpp:227: updateParentBoundaries = true; On 2016/04/29 at 17:50:54, f(malita) wrote: > Can we use a TemporaryChange<bool> in the outer scope to avoid an explicit reset below? > > (would have to be wrapped in Optional<>, see if it makes sense) Didn't look too shabby, but m_needsReordering is a bitfield, so TemporaryChange won't work (since you can't take the address of a bitfield. https://codereview.chromium.org/1933193002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/layout/svg/LayoutSVGText.cpp:260: m_needsReordering = false; On 2016/04/29 at 17:50:54, f(malita) wrote: > Nit: make this unconditional? Done.
Message was sent while issue was closed.
Patchset 1 (id:??) landed as https://crrev.com/731c92d1821ff04b324eb334f484337005d713c9 Cr-Commit-Position: refs/heads/master@{#390691}
Message was sent while issue was closed.
Description was changed from ========== Restructure LayoutSVGText::layout "Uncascade" LayoutSVGText::layout by separating the handling of the two flags (m_needsTextMetricsUpdate, m_needsPositioningValuesUpdate) into sequential blocks. Add assert to verify consistency. BUG=607906 ========== to ========== Restructure LayoutSVGText::layout "Uncascade" LayoutSVGText::layout by separating the handling of the two flags (m_needsTextMetricsUpdate, m_needsPositioningValuesUpdate) into sequential blocks. Add assert to verify consistency. BUG=607906 Committed: https://crrev.com/731c92d1821ff04b324eb334f484337005d713c9 Cr-Commit-Position: refs/heads/master@{#390691} ========== |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
