Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1547263002:
Fix SVGInlineTextMetricsIterator not to keep a pointer to LineLayoutItem (Closed)
|
Created:
4 years, 12 months ago by kojii Modified:
4 years, 11 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. |
DescriptionFix SVGInlineTextMetricsIterator not to keep a pointer to LineLayoutItem
This patch changes SVGInlineTextMetricsIterator to keep an instanace of
LineLayoutSVGInlineText, not a pointer to. LineLayoutItem and its
subclasses are oftentimes allocated on stack that keeping pointers
should be avoided.
Additional ASSERTs in this CL fail existing tests. These existing tests
cover the fix.
BUG=572399, 499321
Committed: https://crrev.com/12bc246bc452c91dc3f6155480898a93cb8a78e5
Cr-Commit-Position: refs/heads/master@{#367064}
Patch Set 1 : Just add ASSERT to ensure existing tests break #Patch Set 2 : Fix LineLayoutSVGInlineText* to LineLayoutSVGInlineText #
Total comments: 1
Patch Set 3 : eae review, remove excessive assert #
Messages
Total messages: 18 (12 generated)
Description was changed from ========== Fix SVGInlineTextMetricsIterator BUG=572399 ========== to ========== Fix SVGInlineTextMetricsIterator BUG=572399, 499321 ==========
Description was changed from ========== Fix SVGInlineTextMetricsIterator BUG=572399, 499321 ========== to ========== Fix SVGInlineTextMetricsIterator to not keep a pointer to LineLayoutItem This patch changes SVGInlineTextMetricsIterator to keep an instanace of LineLayoutSVGInlineText, not a pointer of. LineLayoutItem and its subclasses are oftentimes allocated on stack. Additional ASSERTs in this CL fail existing tests, so these existing tests cover the fix. BUG=572399, 499321 ==========
kojii@chromium.org changed reviewers: + eae@chromium.org, pilgrim@chromium.org
Description was changed from ========== Fix SVGInlineTextMetricsIterator to not keep a pointer to LineLayoutItem This patch changes SVGInlineTextMetricsIterator to keep an instanace of LineLayoutSVGInlineText, not a pointer of. LineLayoutItem and its subclasses are oftentimes allocated on stack. Additional ASSERTs in this CL fail existing tests, so these existing tests cover the fix. BUG=572399, 499321 ========== to ========== Fix SVGInlineTextMetricsIterator to not keep a pointer to LineLayoutItem This patch changes SVGInlineTextMetricsIterator to keep an instanace of LineLayoutSVGInlineText, not a pointer of. LineLayoutItem and its subclasses are oftentimes allocated on stack that keeping pointers should be avoided. Additional ASSERTs in this CL fail existing tests, so these existing tests cover the fix. BUG=572399, 499321 ==========
Description was changed from ========== Fix SVGInlineTextMetricsIterator to not keep a pointer to LineLayoutItem This patch changes SVGInlineTextMetricsIterator to keep an instanace of LineLayoutSVGInlineText, not a pointer of. LineLayoutItem and its subclasses are oftentimes allocated on stack that keeping pointers should be avoided. Additional ASSERTs in this CL fail existing tests, so these existing tests cover the fix. BUG=572399, 499321 ========== to ========== Fix SVGInlineTextMetricsIterator to not keep a pointer to LineLayoutItem This patch changes SVGInlineTextMetricsIterator to keep an instanace of LineLayoutSVGInlineText, not a pointer of. LineLayoutItem and its subclasses are oftentimes allocated on stack that keeping pointers should be avoided. Additional ASSERTs in this CL fail existing tests, so these existing tests cover the fix. BUG=572399, 499321 ==========
Description was changed from ========== Fix SVGInlineTextMetricsIterator to not keep a pointer to LineLayoutItem This patch changes SVGInlineTextMetricsIterator to keep an instanace of LineLayoutSVGInlineText, not a pointer of. LineLayoutItem and its subclasses are oftentimes allocated on stack that keeping pointers should be avoided. Additional ASSERTs in this CL fail existing tests, so these existing tests cover the fix. BUG=572399, 499321 ========== to ========== Fix SVGInlineTextMetricsIterator to not keep a pointer to LineLayoutItem This patch changes SVGInlineTextMetricsIterator to keep an instanace of LineLayoutSVGInlineText, not a pointer of. LineLayoutItem and its subclasses are oftentimes allocated on stack that keeping pointers should be avoided. Additional ASSERTs in this CL fail existing tests. These existing tests cover the fix. BUG=572399, 499321 ==========
Description was changed from ========== Fix SVGInlineTextMetricsIterator to not keep a pointer to LineLayoutItem This patch changes SVGInlineTextMetricsIterator to keep an instanace of LineLayoutSVGInlineText, not a pointer of. LineLayoutItem and its subclasses are oftentimes allocated on stack that keeping pointers should be avoided. Additional ASSERTs in this CL fail existing tests. These existing tests cover the fix. BUG=572399, 499321 ========== to ========== Fix SVGInlineTextMetricsIterator not to keep a pointer to LineLayoutItem This patch changes SVGInlineTextMetricsIterator to keep an instanace of LineLayoutSVGInlineText, not a pointer of. LineLayoutItem and its subclasses are oftentimes allocated on stack that keeping pointers should be avoided. Additional ASSERTs in this CL fail existing tests. These existing tests cover the fix. BUG=572399, 499321 ==========
Description was changed from ========== Fix SVGInlineTextMetricsIterator not to keep a pointer to LineLayoutItem This patch changes SVGInlineTextMetricsIterator to keep an instanace of LineLayoutSVGInlineText, not a pointer of. LineLayoutItem and its subclasses are oftentimes allocated on stack that keeping pointers should be avoided. Additional ASSERTs in this CL fail existing tests. These existing tests cover the fix. BUG=572399, 499321 ========== to ========== Fix SVGInlineTextMetricsIterator not to keep a pointer to LineLayoutItem This patch changes SVGInlineTextMetricsIterator to keep an instanace of LineLayoutSVGInlineText, not a pointer to. LineLayoutItem and its subclasses are oftentimes allocated on stack that keeping pointers should be avoided. Additional ASSERTs in this CL fail existing tests. These existing tests cover the fix. BUG=572399, 499321 ==========
PTAL.
LGTM w/nit Thanks for working on this. https://codereview.chromium.org/1547263002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/layout/api/LineLayoutSVGInlineText.h (right): https://codereview.chromium.org/1547263002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/api/LineLayoutSVGInlineText.h:118: void assertCharacterOffsetFromMetricsList() Is this really necessary? Wouldn't the length assert in next combined with the offset assert in advance catch all cases where it's wrong?
Thanks for quick review. Agree that the assert is excessive, I'll remove it. It just catches early and helped investigations, but other asserts should catch later on.
The CQ bit was checked by kojii@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from eae@chromium.org Link to the patchset: https://codereview.chromium.org/1547263002/#ps40001 (title: "eae review, remove excessive assert")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1547263002/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1547263002/40001
Message was sent while issue was closed.
Description was changed from ========== Fix SVGInlineTextMetricsIterator not to keep a pointer to LineLayoutItem This patch changes SVGInlineTextMetricsIterator to keep an instanace of LineLayoutSVGInlineText, not a pointer to. LineLayoutItem and its subclasses are oftentimes allocated on stack that keeping pointers should be avoided. Additional ASSERTs in this CL fail existing tests. These existing tests cover the fix. BUG=572399, 499321 ========== to ========== Fix SVGInlineTextMetricsIterator not to keep a pointer to LineLayoutItem This patch changes SVGInlineTextMetricsIterator to keep an instanace of LineLayoutSVGInlineText, not a pointer to. LineLayoutItem and its subclasses are oftentimes allocated on stack that keeping pointers should be avoided. Additional ASSERTs in this CL fail existing tests. These existing tests cover the fix. BUG=572399, 499321 ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Description was changed from ========== Fix SVGInlineTextMetricsIterator not to keep a pointer to LineLayoutItem This patch changes SVGInlineTextMetricsIterator to keep an instanace of LineLayoutSVGInlineText, not a pointer to. LineLayoutItem and its subclasses are oftentimes allocated on stack that keeping pointers should be avoided. Additional ASSERTs in this CL fail existing tests. These existing tests cover the fix. BUG=572399, 499321 ========== to ========== Fix SVGInlineTextMetricsIterator not to keep a pointer to LineLayoutItem This patch changes SVGInlineTextMetricsIterator to keep an instanace of LineLayoutSVGInlineText, not a pointer to. LineLayoutItem and its subclasses are oftentimes allocated on stack that keeping pointers should be avoided. Additional ASSERTs in this CL fail existing tests. These existing tests cover the fix. BUG=572399, 499321 Committed: https://crrev.com/12bc246bc452c91dc3f6155480898a93cb8a78e5 Cr-Commit-Position: refs/heads/master@{#367064} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/12bc246bc452c91dc3f6155480898a93cb8a78e5 Cr-Commit-Position: refs/heads/master@{#367064} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
