Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2599613002:
[SPInvalidation] Fix caret paint invalidation in sub frames (Closed)
Description[SPInvalidation] Fix caret paint invalidation in sub frames
When the caret is marked dirty in a frame, we need to set proper paint
invalidation flags along the ancestor chain to the root frame, so that
the frame will be checked for paint invalidation during
PrePaintTreeWalk.
We don't need this for non-SlimmingPaintInvalidation because we always
walk all FrameViews during paint invalidation in the mode.
BUG=646176
TEST=editing/caret/caret-hidden-when-unfocused.html with --enable-slimming-paint-invalidation
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
Committed: https://crrev.com/61544da8b33b48508b743eebd043f4be0f652714
Cr-Commit-Position: refs/heads/master@{#440321}
Patch Set 1 #
Total comments: 4
Messages
Total messages: 16 (9 generated)
The CQ bit was checked by wangxianzhu@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...
wangxianzhu@chromium.org changed reviewers: + pdr@chromium.org
This one is an under-paint-invalidation of caret, so is not listed in the paint property DCHECK failures.
Description was changed from ========== [SPInvalidation] Fix caret paint invalidation in sub frames When the caret is marked dirty in a frame, we need to set proper paint invalidation flags along the ancestor chain to the root frame, so that the frame will be checked for paint invalidation during PrePaintTreeWalk. We don't need this for non-SlimmingPaintInvalidation because we always walk all FrameViews during paint invalidation in the mode. BUG=646176 TEST=editing/caret/caret-hidden-when-unfocused.html with --enable-slimming-paint-invalidation ========== to ========== [SPInvalidation] Fix caret paint invalidation in sub frames When the caret is marked dirty in a frame, we need to set proper paint invalidation flags along the ancestor chain to the root frame, so that the frame will be checked for paint invalidation during PrePaintTreeWalk. We don't need this for non-SlimmingPaintInvalidation because we always walk all FrameViews during paint invalidation in the mode. BUG=646176 TEST=editing/caret/caret-hidden-when-unfocused.html with --enable-slimming-paint-invalidation CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 ==========
https://codereview.chromium.org/2599613002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/editing/FrameCaret.cpp (right): https://codereview.chromium.org/2599613002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/editing/FrameCaret.cpp:147: // Ensure the frame will be checked for paint invalidation during Without spinvalidation, how are carets marked as needing paint invalidation? It looks like we have some logic to invalidate them in FrameCaret::invalidateCaretRect... are we missing a call to "node->layoutObject()->setMayNeedPaintInvalidation" there, which would handle crossing frames? https://codereview.chromium.org/2599613002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/editing/FrameCaret.cpp:150: if (auto layoutItem = m_frame->ownerLayoutItem()) I think with LayoutItem you need to check !layoutItem.isNull(). Can you switch this to just using LayoutObject instead? if (auto* layoutObject = m_frame->ownerLayoutObject()) layoutObject->setMayNeedPaintInvalidation();
https://codereview.chromium.org/2599613002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/editing/FrameCaret.cpp (right): https://codereview.chromium.org/2599613002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/editing/FrameCaret.cpp:147: // Ensure the frame will be checked for paint invalidation during On 2016/12/22 01:08:18, pdr (OOO Dec 23 to Jan 2) wrote: > Without spinvalidation, how are carets marked as needing paint invalidation? > > It looks like we have some logic to invalidate them in > FrameCaret::invalidateCaretRect... are we missing a call to > "node->layoutObject()->setMayNeedPaintInvalidation" there, which would handle > crossing frames? Without spinvalidation, we always walk all of the FrameViews: https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/frame/Fra.... Actually we could remove the walk for non-spinvalidation with unconditional setMayNeedPaintInvalidation here. May consider a follow-up. https://codereview.chromium.org/2599613002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/editing/FrameCaret.cpp:150: if (auto layoutItem = m_frame->ownerLayoutItem()) On 2016/12/22 01:08:18, pdr (OOO Dec 23 to Jan 2) wrote: > I think with LayoutItem you need to check !layoutItem.isNull(). > Can you switch this to just using LayoutObject instead? > if (auto* layoutObject = m_frame->ownerLayoutObject()) > layoutObject->setMayNeedPaintInvalidation(); The above is equivalent to "if (auto* layoutObject = m_frame->ownerLayoutObject())", using LayoutItem::operator LayoutObject* (which will be operator bool).
On 2016/12/22 at 01:31:52, wangxianzhu wrote: > https://codereview.chromium.org/2599613002/diff/1/third_party/WebKit/Source/c... > File third_party/WebKit/Source/core/editing/FrameCaret.cpp (right): > > https://codereview.chromium.org/2599613002/diff/1/third_party/WebKit/Source/c... > third_party/WebKit/Source/core/editing/FrameCaret.cpp:147: // Ensure the frame will be checked for paint invalidation during > On 2016/12/22 01:08:18, pdr (OOO Dec 23 to Jan 2) wrote: > > Without spinvalidation, how are carets marked as needing paint invalidation? > > > > It looks like we have some logic to invalidate them in > > FrameCaret::invalidateCaretRect... are we missing a call to > > "node->layoutObject()->setMayNeedPaintInvalidation" there, which would handle > > crossing frames? > > Without spinvalidation, we always walk all of the FrameViews: > https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/frame/Fra.... > > Actually we could remove the walk for non-spinvalidation with unconditional setMayNeedPaintInvalidation here. May consider a follow-up. > > https://codereview.chromium.org/2599613002/diff/1/third_party/WebKit/Source/c... > third_party/WebKit/Source/core/editing/FrameCaret.cpp:150: if (auto layoutItem = m_frame->ownerLayoutItem()) > On 2016/12/22 01:08:18, pdr (OOO Dec 23 to Jan 2) wrote: > > I think with LayoutItem you need to check !layoutItem.isNull(). > > Can you switch this to just using LayoutObject instead? > > if (auto* layoutObject = m_frame->ownerLayoutObject()) > > layoutObject->setMayNeedPaintInvalidation(); > > The above is equivalent to "if (auto* layoutObject = m_frame->ownerLayoutObject())", using LayoutItem::operator LayoutObject* (which will be operator bool). OK LGTM
The CQ bit was unchecked by wangxianzhu@chromium.org
The CQ bit was checked by wangxianzhu@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 1, "attempt_start_ts": 1482371958392270, "parent_rev":
"befa9f7cb371c61cc0865035feb1a6097997f764", "commit_rev":
"529f13cc42b59e46b0b7ce67d1346942ec17cfab"}
Message was sent while issue was closed.
Description was changed from ========== [SPInvalidation] Fix caret paint invalidation in sub frames When the caret is marked dirty in a frame, we need to set proper paint invalidation flags along the ancestor chain to the root frame, so that the frame will be checked for paint invalidation during PrePaintTreeWalk. We don't need this for non-SlimmingPaintInvalidation because we always walk all FrameViews during paint invalidation in the mode. BUG=646176 TEST=editing/caret/caret-hidden-when-unfocused.html with --enable-slimming-paint-invalidation CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 ========== to ========== [SPInvalidation] Fix caret paint invalidation in sub frames When the caret is marked dirty in a frame, we need to set proper paint invalidation flags along the ancestor chain to the root frame, so that the frame will be checked for paint invalidation during PrePaintTreeWalk. We don't need this for non-SlimmingPaintInvalidation because we always walk all FrameViews during paint invalidation in the mode. BUG=646176 TEST=editing/caret/caret-hidden-when-unfocused.html with --enable-slimming-paint-invalidation CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 Review-Url: https://codereview.chromium.org/2599613002 ==========
Message was sent while issue was closed.
Committed patchset #1 (id:1)
Message was sent while issue was closed.
Description was changed from ========== [SPInvalidation] Fix caret paint invalidation in sub frames When the caret is marked dirty in a frame, we need to set proper paint invalidation flags along the ancestor chain to the root frame, so that the frame will be checked for paint invalidation during PrePaintTreeWalk. We don't need this for non-SlimmingPaintInvalidation because we always walk all FrameViews during paint invalidation in the mode. BUG=646176 TEST=editing/caret/caret-hidden-when-unfocused.html with --enable-slimming-paint-invalidation CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 Review-Url: https://codereview.chromium.org/2599613002 ========== to ========== [SPInvalidation] Fix caret paint invalidation in sub frames When the caret is marked dirty in a frame, we need to set proper paint invalidation flags along the ancestor chain to the root frame, so that the frame will be checked for paint invalidation during PrePaintTreeWalk. We don't need this for non-SlimmingPaintInvalidation because we always walk all FrameViews during paint invalidation in the mode. BUG=646176 TEST=editing/caret/caret-hidden-when-unfocused.html with --enable-slimming-paint-invalidation CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 Committed: https://crrev.com/61544da8b33b48508b743eebd043f4be0f652714 Cr-Commit-Position: refs/heads/master@{#440321} ==========
Message was sent while issue was closed.
Patchset 1 (id:??) landed as https://crrev.com/61544da8b33b48508b743eebd043f4be0f652714 Cr-Commit-Position: refs/heads/master@{#440321} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
