Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
431983005:
FrameSelection::updateApperance for caret should not cause layout. (Closed)
DescriptionFrameSelection::updateApperance for caret should not cause layout.
We do not need update caret rectangle synchronously because we just need
updated caret in painting.
Thus this CL delays computing new caret rectangle.
Source/core/editing/FrameSelection.cpp:
- Add ResetCaretBlinkOption to updateAppearance(). The option is set
to ResetCaretBlink when it is called from setSelection.
If ResetCaretBlink, we reset caret blinking.
If we need to repaint caret, set |m_caretRectDirty| flag.
If |m_caretRectDirty| is set, we call PageAnimator::scheduleVisualUpdate to
trigger repaint.
For range, create VisibleSelection without validation like
HTMLTextFormControlElement::setSelectionRange.
- Add the setCaretRectNeedsUpdate function to just set |m_caretRectDirty| flag
and call new scheduleVisualUpdate function, which calls
PageAnimator::scheduleVisualUpdate.
- FrameSelection::invalidateCaretRect does
1. Checks the dirty flag.
2. Gets new caret rectangle and node which has the caret renderer.
3. if caret is changed, invalidate the new caret rect and the old caret rect.
4. Caches the new caret rect and node. Sets dirty flag off.
This function is similar to old recomputeCaretRect(deleted).
- Delete unused updateRenderTreeIfneeded() from notifyRenderOfSelectionChange().
In old days, notifyRenderOfSelectionChange() used renderer.
- Refactor FrameSelection::absoluteCaretBounds() to call updateCaretRect
directly. Since we update layout, ASSERT that document's lifecycle is not in
InPaintInvalidation.
Source/core/editing/Caret.cpp:
- Remove document->updateRenderTreeIfNeeded() from CaretBase::updateCaretRect
and call it from caller.
- In FrameSelection::paintCaret, call without updateRenderTreeIfNeeded because
the tree must be updated on the painting phase.
Source/core/frame/FrameView.cpp:
- FrameView::invalidateTreeIfNeeded calls FrameSelection::InvalidateCaretRect
to invalidate caret rect for each frame.
Source/core/html/HTMLTextFormControlElementTest.cpp:
- Add new test to confirm setSelectionRange with another start/end not cause
layout.
- Delete FrameSelectionLocalCaretRectDoesNotCauseLayout because localCaretRect
is removed.
Layouttests:
- This cl reduces redundant RenderObject::invalidatePaintRectangle calls so we
need to delete lines from expected.txt but result images are still same.
BUG=382809, 397303, 403317
TEST=Layouttests/fast/forms/textarea-scrollbar.html, textarea-scrolled-type.html
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=180269
Patch Set 1 : wip #
Total comments: 2
Patch Set 2 : Add updateLocalCaretRect function #
Total comments: 6
Patch Set 3 : Generalized to contenteditable #Patch Set 4 : Fix a repaint expectation #
Total comments: 2
Patch Set 5 : Fix linux repaint expectations #Patch Set 6 : Fix mac test #Patch Set 7 : Rebase #
Total comments: 1
Patch Set 8 : Move caret recomputing to PageWedgetDelegate::layout #Patch Set 9 : Delay invalidation #
Total comments: 2
Patch Set 10 : Let FrameSelection rember the previous rect. #
Total comments: 14
Patch Set 11 : Update #
Total comments: 6
Patch Set 12 : Fix test failures #
Messages
Total messages: 55 (0 generated)
Awesome! The change is very small. (^_^)b https://codereview.chromium.org/431983005/diff/40001/Source/core/editing/Fram... File Source/core/editing/FrameSelection.cpp (right): https://codereview.chromium.org/431983005/diff/40001/Source/core/editing/Fram... Source/core/editing/FrameSelection.cpp:1296: localCaretRect(); The name of |localCaretRect()| make me to feel it is a function without side effect, but it updates |m_absCaretBoundsDirty|. It is better to rename it?
https://codereview.chromium.org/431983005/diff/40001/Source/core/editing/Fram... File Source/core/editing/FrameSelection.cpp (right): https://codereview.chromium.org/431983005/diff/40001/Source/core/editing/Fram... Source/core/editing/FrameSelection.cpp:1296: localCaretRect(); On 2014/08/04 01:44:23, Yosi_UTC9 wrote: > The name of |localCaretRect()| make me to feel it is a function without side > effect, but it updates |m_absCaretBoundsDirty|. It is better to rename it? Done.
Removing code from a generic location and only adding it back in for text controls is a scary change, so it deserves wider review from folks more seasoned with the editing and rendering code. Will try to take a look soon if I can find time away from the code yellow. If not, hopefully one of the other reviewers I've added can take a look.
I'll take a look tomorrow, indeed that's a scary change (by description, didn't look at the code yet). To unsubscribe from this group and stop receiving emails from it, send an email to blink-reviews+unsubscribe@chromium.org.
PTAL?
I don't think this patch actually does anything, but adding a call to updateLayoutIgnorePendingStylesheets from inside paint() is bad. We'll always have updated before we get there, but still, something is broken when paint is calling into methods that assume that layout could have been dirty. https://codereview.chromium.org/431983005/diff/80001/Source/core/editing/Fram... File Source/core/editing/FrameSelection.cpp (right): https://codereview.chromium.org/431983005/diff/80001/Source/core/editing/Fram... Source/core/editing/FrameSelection.cpp:277: updateAppearance(); updateAppearance creates a VisiblePosition which is going to cause a layout inside its constructor. I don't think this patch does anything. https://codereview.chromium.org/431983005/diff/80001/Source/core/editing/Fram... Source/core/editing/FrameSelection.cpp:1295: updateLocalCaretRect(); Calling updateLayoutIgnorePendingStylesheets from inside paint() is not correct, this should both crash and would have caused security bugs. https://codereview.chromium.org/431983005/diff/80001/Source/core/editing/Fram... Source/core/editing/FrameSelection.cpp:1546: bool isTextForm = isTextFormControl(m_selection); I don't think we want to special case stuff like this.
https://codereview.chromium.org/431983005/diff/80001/Source/core/editing/Fram... File Source/core/editing/FrameSelection.cpp (right): https://codereview.chromium.org/431983005/diff/80001/Source/core/editing/Fram... Source/core/editing/FrameSelection.cpp:277: updateAppearance(); On 2014/08/05 01:00:02, esprehn wrote: > updateAppearance creates a VisiblePosition which is going to cause a layout > inside its constructor. I don't think this patch does anything. Yes. We call layout to get right caret position in content editable elements and decide if we repaint caret to another position. However, if we call updateAppearance more than once before paint, current implementation can call layout each time before painting. This causes bad performance. In this CL, I want to call at most one layout for textarea for caret painting regardless of updateAppearance calls. https://codereview.chromium.org/431983005/diff/80001/Source/core/editing/Fram... Source/core/editing/FrameSelection.cpp:1295: updateLocalCaretRect(); On 2014/08/05 01:00:02, esprehn wrote: > Calling updateLayoutIgnorePendingStylesheets from inside paint() is not correct, > this should both crash and would have caused security bugs. OK. How about calling updateLocalCaretRect in WebViewImpl::beginFrame? WebViewImpl::beginFrame is called before paint and calls callback functions inserted by Document::requestAnimationFrame for example. https://codereview.chromium.org/431983005/diff/80001/Source/core/editing/Fram... Source/core/editing/FrameSelection.cpp:1546: bool isTextForm = isTextFormControl(m_selection); On 2014/08/05 01:00:02, esprehn wrote: > I don't think we want to special case stuff like this. Actually, this CL is for improvement benchmark score using textarea: http://browserbench.org/Speedometer/ Thus I want to create a fast path for HTMLTextFormControlElement.
PTAL. I would like 2 or more round trip a day if possible. When is your core time? Thanks.
On 2014/08/05 at 22:26:56, yoichio wrote: > PTAL. It doesn't look like you updated the patch?
On 2014/08/05 22:42:27, leviw wrote: > On 2014/08/05 at 22:26:56, yoichio wrote: > > PTAL. > It doesn't look like you updated the patch? esprehn@ commented about why this patch's did such implementations. I answered the reasons.
I don't think we want to land special cases for textarea like this. Why can't you just fix the general issue?
On 2014/08/05 23:35:15, esprehn wrote: > I don't think we want to land special cases for textarea like this. Why can't > you just fix the general issue? Done. Thus this cl reduces redundant RenderObject::invalidatePaintRectangle calls so we need to delete lines from expected.txt but result images are still same.
https://codereview.chromium.org/431983005/diff/140001/Source/web/WebViewImpl.cpp File Source/web/WebViewImpl.cpp (right): https://codereview.chromium.org/431983005/diff/140001/Source/web/WebViewImpl.... Source/web/WebViewImpl.cpp:1747: if (frame && frame->isLocalFrame()) { nit: We don't need to have "{}" for single line then clause.
Looks all green. PTAL. https://codereview.chromium.org/431983005/diff/140001/Source/web/WebViewImpl.cpp File Source/web/WebViewImpl.cpp (right): https://codereview.chromium.org/431983005/diff/140001/Source/web/WebViewImpl.... Source/web/WebViewImpl.cpp:1747: if (frame && frame->isLocalFrame()) { On 2014/08/07 01:09:55, Yosi_UTC9 wrote: > nit: We don't need to have "{}" for single line then clause. Done.
Ping.
Please don't depend on calling into VisiblePosition for layout, add an explicit one instead. I also think you want to be in PageWidgetDelegate::layout not in WebViewImpl.
ou shouldn't need to explain how commit works in the description. What does "Unfortunately, webkit_tests is under single thread so we can't confirm the repainting." mean? You definitely can write a test for this. abarth@ What's the right spot to do the invalidation for carets?
> What does "Unfortunately, webkit_tests is under single thread so we can't > confirm the repainting." mean? You definitely can write a test for this. In HTMLTextFormControlElementTest.cpp, I write: L133 input().setSelectionRange(2, 2); L134 EXPECT_EQ(startLayoutCount, layoutCount()); L135 frameSelection.updateLocalCaretRect(); L136 newCaretRect = frameSelection.localCaretRectWithoutUpdateForTesting(); I want to confirm that after setSelectionRange called, caret is updated. L133 queues repaint task which calls FrameSelection::updateLocalCaretRect(). Thus if the task is called, we get updated |newCaretRect| without L135. Current HTMLTextFormControlElementTest doesn't have painting thread. Perhaps I can make it and write something 'callPendingTask()' in L135. How can we do?
> abarth@ What's the right spot to do the invalidation for carets? Presumably during InPaintInvalidation we should issue an invalidation for the caret after walking the RenderObjects...
https://codereview.chromium.org/431983005/diff/200001/Source/web/WebViewImpl.cpp File Source/web/WebViewImpl.cpp (right): https://codereview.chromium.org/431983005/diff/200001/Source/web/WebViewImpl.... Source/web/WebViewImpl.cpp:1748: toLocalFrame(frame)->selection().recomputeCaretRect(); This is much too early. What if the GraphicsLayer for the caret gets created or destroyed during the compositing update? We want to do this work when the DocumentLifecycle is InPaintInvalidation.
We invalidate for caret in FrameSelection::updateAppearance for now. This CL targets reducing the invalidation calling and layout calling. I agree with moving invalidation to proper spot but let me do it later. > Please don't depend on calling into VisiblePosition for layout, add an explicit > one instead. I also think you want to be in PageWidgetDelegate::layout not in > WebViewImpl. Done. > https://codereview.chromium.org/431983005/diff/200001/Source/web/WebViewImpl.cpp > File Source/web/WebViewImpl.cpp (right): > > https://codereview.chromium.org/431983005/diff/200001/Source/web/WebViewImpl.... > Source/web/WebViewImpl.cpp:1748: > toLocalFrame(frame)->selection().recomputeCaretRect(); > This is much too early. What if the GraphicsLayer for the caret gets created or > destroyed during the compositing update? We want to do this work when the > DocumentLifecycle is InPaintInvalidation. Sorry, I should use updateLocalRect instead of recomputeCaretRect. I want to get updated caret rectangle and don't want invalidation.
> Sorry, I should use updateLocalRect instead of recomputeCaretRect. I should use recomputeCaretRect(NotInvalidation) instead of recomputeCaretRect. PTAL.
Ping.
On 2014/08/11 12:44:31, yoichio wrote: > Ping. PTAL. Elliott?
Can we structure this code in the same way as repaint after layout? The FrameSelection remembers the previous rect for the caret. When something happens to change where the caret should be drawn, we set a dirty bit. During InPaintInvalidation, we check the dirty bit. If the dirty bit is set, we compute the new rect for the caret and compare it to the old rect. If the rects are different, we issue invalidations for the old and new locations.
On 2014/08/11 23:37:59, abarth wrote: > Can we structure this code in the same way as repaint after layout? The > FrameSelection remembers the previous rect for the caret. When something > happens to change where the caret should be drawn, we set a dirty bit. During > InPaintInvalidation, we check the dirty bit. If the dirty bit is set, we > compute the new rect for the caret and compare it to the old rect. If the rects > are different, we issue invalidations for the old and new locations. No, we can't. That's because layout is triggered by FrameSelection::invalidateCaretRect for repainting caret.
On 2014/08/12 at 01:15:56, yoichio wrote: > On 2014/08/11 23:37:59, abarth wrote: > > Can we structure this code in the same way as repaint after layout? The > > FrameSelection remembers the previous rect for the caret. When something > > happens to change where the caret should be drawn, we set a dirty bit. During > > InPaintInvalidation, we check the dirty bit. If the dirty bit is set, we > > compute the new rect for the caret and compare it to the old rect. If the rects > > are different, we issue invalidations for the old and new locations. > > No, we can't. > That's because layout is triggered by FrameSelection::invalidateCaretRect for repainting caret. When we're in the InPaintInvalidation phase, layout will already be up-to-date. There will be no need to trigger layout.
> > No, we can't. > > That's because layout is triggered by FrameSelection::invalidateCaretRect for > repainting caret. > > When we're in the InPaintInvalidation phase, layout will already be up-to-date. > There will be no need to trigger layout. You mean the InPaintInvalidation phase is in FrameView::invalidateTreeIfNeededRecursive, right? If that, when do we call FrameView::invalidateTreeIfNeededRecursive? In chrome, ThreadProxy::BeginMainFrame calls FrameView::invalidateTreeIfNeededRecursive and ThreadProxy::BeginMainFrame is called by FrameSelection::invalidateCaretRect via IPCs and deep call stacks. Especially for caret repainting, ThreadProxy::BeginMainFrame is only called by FrameSelection::invalidateCaretRect. We are not into InPaintInvalidation phase without FrameSelection::invalidateCaretRect.
On 2014/08/12 at 02:36:34, yoichio wrote: > You mean the InPaintInvalidation phase is in FrameView::invalidateTreeIfNeededRecursive, right? invalidateTreeIfNeededRecursive runs while the DocumentLifecycle is in the InPaintInvalidation state. We can run other stuff in that state too. > If that, when do we call FrameView::invalidateTreeIfNeededRecursive? We continue to call invalidateTreeIfNeededRecursive in the same lifecycle state. Both just issue invalidations. There's no conflict or other ordering dependency between the two tasks. > In chrome, ThreadProxy::BeginMainFrame calls FrameView::invalidateTreeIfNeededRecursive > and ThreadProxy::BeginMainFrame is called by FrameSelection::invalidateCaretRect via IPCs and deep call stacks. > Especially for caret repainting, ThreadProxy::BeginMainFrame is only called by FrameSelection::invalidateCaretRect. When you mark the dirty bits I mentioned before, you should ask the PageAnimator to scheduleVisualUpdate. That will cause the system to drive the DocumentLifecycle all the way to producing a new frame, including going through the InPaintInvalidation state. > We are not into InPaintInvalidation phase without FrameSelection::invalidateCaretRect. Presumably we're able to draw content on the screen without invalidateCaretRect. I'm just suggesting you make caret invalidations work the same way that CSS works in the engine.
> Presumably we're able to draw content on the screen without invalidateCaretRect. I got. Thank your suggestions. Done. PTAL. In FrameSelection, we call PageAnimator::scheduleVisualUpdate to to trigger repaint. FrameView::invalidateTreeIfNeeded calls FrameSelection::InvalidateCaretRect to update caret rect and invalidate if needed. If caret is updated, FrameSelection::InvalidateCaretRect does nothing.
> The FrameSelection remembers the previous rect for the caret. ^^^ Where is this done?
https://codereview.chromium.org/431983005/diff/300001/Source/core/frame/Frame... File Source/core/frame/FrameView.cpp (right): https://codereview.chromium.org/431983005/diff/300001/Source/core/frame/Frame... Source/core/frame/FrameView.cpp:1003: frame->selection().invalidateCaretRect(); Suppose the caret moved from one LocalFrame to another during this animation frame. Where do we invalidate the old caret location? You didn't use the design I suggested.
Here's a complete design: 1. The FrameSelection remembers the previous rect for the caret. 2. When something happens to change where the caret should be drawn, we set a dirty bit and schedule a visual update. 3. During InPaintInvalidation, we check the dirty bit. a. If the dirty bit is set, we compute the new rect for the caret and compare it to the old rect. b. If the rects are different (including if they're in different frames), we issue invalidations for the old and new locations. We should be able to delete a substantial amount of crazy code when we switch to this design.
PTAL. > 1. The FrameSelection remembers the previous rect for the caret. FrameSelection::m_previouseCaretRect(and m_previouseCaretNode) does. > 2. When something happens to change where the caret should be drawn, we set a > dirty bit and schedule a visual update. m_caretRectDirty as as dirty bit. > 3. During InPaintInvalidation, we check the dirty bit. > a. If the dirty bit is set, we compute the new rect for the caret and compare > it to the old rect. > b. If the rects are different (including if they're in different frames), we >issue invalidations for the old and new locations. FrameSelection::invalidateCaretRect() does. >We should be able to delete a substantial amount of crazy code when we switch to >this design. What is crazy code? CaretBase looks and I removed some dependencies but DragCaretController uses CaretBase functions.
https://codereview.chromium.org/431983005/diff/300001/Source/core/frame/Frame... File Source/core/frame/FrameView.cpp (right): https://codereview.chromium.org/431983005/diff/300001/Source/core/frame/Frame... Source/core/frame/FrameView.cpp:1003: frame->selection().invalidateCaretRect(); On 2014/08/12 19:36:25, abarth wrote: > Suppose the caret moved from one LocalFrame to another during this animation > frame. Where do we invalidate the old caret location? You didn't use the > design I suggested. Call each LocalFrame invalidation. Thus a focused out LocalFrame invalidates old caret location and a focued in LocalFrame invalidates new caret location.
https://codereview.chromium.org/431983005/diff/320002/Source/core/editing/Fra... File Source/core/editing/FrameSelection.cpp (right): https://codereview.chromium.org/431983005/diff/320002/Source/core/editing/Fra... Source/core/editing/FrameSelection.cpp:1269: RenderObject* renderer; = 0 https://codereview.chromium.org/431983005/diff/320002/Source/core/editing/Fra... Source/core/editing/FrameSelection.cpp:1277: ASSERT(m_frame->document()->renderView()); There no need for this ASSERT https://codereview.chromium.org/431983005/diff/320002/Source/core/editing/Fra... Source/core/editing/FrameSelection.cpp:1563: if (m_caretRectDirty) How can m_caretRectDirty become true without calling scheduleVisualUpdate()? We should have everyone who sets m_caretRectDirty to true do so via setCaretRectNeedsUpdate() https://codereview.chromium.org/431983005/diff/320002/Source/core/editing/Fra... Source/core/editing/FrameSelection.cpp:1615: m_frame->document()->updateLayoutIgnorePendingStylesheets(); Why is this call necessary? https://codereview.chromium.org/431983005/diff/320002/Source/core/editing/Fra... Source/core/editing/FrameSelection.cpp:1615: m_frame->document()->updateLayoutIgnorePendingStylesheets(); Why is this call necessary? https://codereview.chromium.org/431983005/diff/320002/Source/core/editing/Fra... File Source/core/editing/FrameSelection.h (right): https://codereview.chromium.org/431983005/diff/320002/Source/core/editing/Fra... Source/core/editing/FrameSelection.h:95: }; Why do we need this option? https://codereview.chromium.org/431983005/diff/320002/Source/core/editing/Fra... Source/core/editing/FrameSelection.h:171: void updateAppearance(UpdateAppearanceOption = None); What does updateAppearance do? Does it update the appearance?
https://codereview.chromium.org/431983005/diff/320002/Source/core/editing/Fra... File Source/core/editing/FrameSelection.cpp (right): https://codereview.chromium.org/431983005/diff/320002/Source/core/editing/Fra... Source/core/editing/FrameSelection.cpp:1269: RenderObject* renderer; On 2014/08/13 20:42:44, abarth wrote: > = 0 Done. https://codereview.chromium.org/431983005/diff/320002/Source/core/editing/Fra... Source/core/editing/FrameSelection.cpp:1277: ASSERT(m_frame->document()->renderView()); On 2014/08/13 20:42:44, abarth wrote: > There no need for this ASSERT Done. https://codereview.chromium.org/431983005/diff/320002/Source/core/editing/Fra... Source/core/editing/FrameSelection.cpp:1563: if (m_caretRectDirty) On 2014/08/13 20:42:44, abarth wrote: > How can m_caretRectDirty become true without calling scheduleVisualUpdate()? We > should have everyone who sets m_caretRectDirty to true do so via > setCaretRectNeedsUpdate() Done. https://codereview.chromium.org/431983005/diff/320002/Source/core/editing/Fra... Source/core/editing/FrameSelection.cpp:1615: m_frame->document()->updateLayoutIgnorePendingStylesheets(); On 2014/08/13 20:42:44, abarth wrote: > Why is this call necessary? Done. https://codereview.chromium.org/431983005/diff/320002/Source/core/editing/Fra... Source/core/editing/FrameSelection.cpp:1619: } Remove these lines because we check if we paint or not(and trigger repaint) in updateAppearance. https://codereview.chromium.org/431983005/diff/320002/Source/core/editing/Fra... File Source/core/editing/FrameSelection.h (right): https://codereview.chromium.org/431983005/diff/320002/Source/core/editing/Fra... Source/core/editing/FrameSelection.h:95: }; On 2014/08/13 20:42:44, abarth wrote: > Why do we need this option? To use restart caret blinking, for example, moving caret. https://codereview.chromium.org/431983005/diff/320002/Source/core/editing/Fra... Source/core/editing/FrameSelection.h:171: void updateAppearance(UpdateAppearanceOption = None); On 2014/08/13 20:42:44, abarth wrote: > What does updateAppearance do? Does it update the appearance? Yes, it updates. The name was confusing. Renamed.
LGTM Thanks for iterating on this CL. I suspect there are still things we could improve about this code, but we can save those for a future CL. :) https://codereview.chromium.org/431983005/diff/350001/Source/core/editing/Fra... File Source/core/editing/FrameSelection.cpp (right): https://codereview.chromium.org/431983005/diff/350001/Source/core/editing/Fra... Source/core/editing/FrameSelection.cpp:1236: m_frame->document()->updateLayoutIgnorePendingStylesheets(); Is this call needed? When do we query in the absoluteCaretBounds at a time when layout could be out of date? https://codereview.chromium.org/431983005/diff/350001/Source/core/editing/Fra... Source/core/editing/FrameSelection.cpp:1543: bool isCaretRectNeedsUpdate = false; willNeedCaretRectUpdate https://codereview.chromium.org/431983005/diff/350001/Source/core/editing/Fra... File Source/core/editing/FrameSelection.h (right): https://codereview.chromium.org/431983005/diff/350001/Source/core/editing/Fra... Source/core/editing/FrameSelection.h:170: // Painting. I'd remove this comment. It doesn't make much sense.
The CQ bit was checked by yoichio@chromium.org
Thank you your kindness reviews! https://codereview.chromium.org/431983005/diff/350001/Source/core/editing/Fra... File Source/core/editing/FrameSelection.cpp (right): https://codereview.chromium.org/431983005/diff/350001/Source/core/editing/Fra... Source/core/editing/FrameSelection.cpp:1236: m_frame->document()->updateLayoutIgnorePendingStylesheets(); On 2014/08/13 22:01:24, abarth wrote: > Is this call needed? When do we query in the absoluteCaretBounds at a time when > layout could be out of date? No. Removed. https://codereview.chromium.org/431983005/diff/350001/Source/core/editing/Fra... Source/core/editing/FrameSelection.cpp:1543: bool isCaretRectNeedsUpdate = false; On 2014/08/13 22:01:24, abarth wrote: > willNeedCaretRectUpdate Done. https://codereview.chromium.org/431983005/diff/350001/Source/core/editing/Fra... File Source/core/editing/FrameSelection.h (right): https://codereview.chromium.org/431983005/diff/350001/Source/core/editing/Fra... Source/core/editing/FrameSelection.h:170: // Painting. On 2014/08/13 22:01:24, abarth wrote: > I'd remove this comment. It doesn't make much sense. Done.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/yoichio@chromium.org/431983005/370001
On 2014/08/13 at 22:36:33, yoichio wrote: > No. Removed. You might want to add an ASSERT to that effect. For example, you might want to ASSERT that Document's lifecycle().state() == DocumentLifecycle::InPaintInvalidation if that's accurate.
The CQ bit was unchecked by yoichio@chromium.org
On 2014/08/13 22:55:18, abarth wrote: > On 2014/08/13 at 22:36:33, yoichio wrote: > > No. Removed. > > You might want to add an ASSERT to that effect. For example, you might want to > ASSERT that Document's lifecycle().state() == > DocumentLifecycle::InPaintInvalidation if that's accurate. Sorry, I missed. This is called in both InPaintInvalidation or not. I changed absoluteCaretBounds via refactoring the existing implementation, which is that absoluteCaretBounds calls recomputeRect which calls localCaretRect.
Use m_previousCaretRect to paintCaret instead of localCaretRectWithoutUpdate(), which is not updated until we call absoluteCaretBounds(). AbsoluteCaretBounds needs update layout so add ASSERTION that Document's lifecycle is not DocumentLifecycle::InPaintInvalidation.
In my short investigation, FrameSelection::absoluteCaretBounds is called when the lifecycle is StyleClean or LayoutClean or PaintInvalidationClean.
The CQ bit was checked by yoichio@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/yoichio@chromium.org/431983005/410001
The CQ bit was unchecked by yoichio@chromium.org
The CQ bit was checked by yoichio@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/yoichio@chromium.org/431983005/450001
Sorry for my confusing comments. Updates from the abarth@'s LGTM are: - Add Rebaselines instead of fixing each platform expected results. - Remove document->updateRenderTreeIfNeeded() from CaretBase::updateCaretRect and call it from caller. In FrameSelection::paintCaret, call without updateRenderTreeIfNeeded because the tree must be updated on the painting phase. - In FrameSelection::absoluteCaretBounds(), call update layout and ASSERT that document's lifecycle is not in InPaintInvalidation. Looks green. Committing.
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: android_blink_compile_dbg on tryserver.blink (http://build.chromium.org/p/tryserver.blink/builders/android_blink_compile_db...) linux_blink_rel on tryserver.blink (http://build.chromium.org/p/tryserver.blink/builders/linux_blink_rel/builds/2...)
Message was sent while issue was closed.
Committed patchset #12 (450001) as 180269 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
