Partly revert https://codereview.chromium.org/1860273003/ where paint follows

third_party/WebKit/Source/core/frame/LocalFrame.cpp

 /*
  * Copyright (C) 1998, 1999 Torben Weis <weis@kde.org>
  *                     1999 Lars Knoll <knoll@kde.org>
  *                     1999 Antti Koivisto <koivisto@kde.org>
  *                     2000 Simon Hausmann <hausmann@kde.org>
  *                     2000 Stefan Schimanski <1Stein@gmx.de>
  *                     2001 George Staikos <staikos@kde.org>
  * Copyright (C) 2004, 2005, 2006, 2007, 2008, 2009, 2010, 2011 Apple Inc. All rights reserved.
  * Copyright (C) 2005 Alexey Proskuryakov <ap@nypop.com>
  * Copyright (C) 2008 Nokia Corporation and/or its subsidiary(-ies)
@@ skipping 601 matching lines @@
     if (!m_host)
         return 0;
 
     double ratio = m_host->deviceScaleFactor();
     ratio *= pageZoomFactor();
     return ratio;
 }
 
 PassOwnPtr<DragImage> LocalFrame::nodeImage(Node& node)
 {
-    m_view->updateLifecycleToCompositingCleanPlusScrolling();
+    // TODO(wangxianzhu): Don't need synchronized painting.

[pdr., 2016/04/13 22:11:02]

Nit: Don't -> Doesn't.

I'm learning about this code for the first time so forgive some silly questions.

Is there any way we can enforce this comment with an assert instead of a
comment? For example, in the stacktrace in 602961 it looks like we are painting
but we've skipped the paint invalidation step. In FrameView::paintContents we
have an assert that the lifecycle >= DocumentLifecycle::CompositingClean...
should that assert lifecycle >= PaintInvalidationClean with a comment about why?

A second issue is that the root cause seems to be that we are updating collapsed
borders during paint invalidation. Would it be practical to just fix that issue
(maybe by moving recalcCollapsedBordersIfNeeded into
LayoutTable::collapsedBorders?). If not, WDYT about adding something like
LayoutView::updateLifecycleToPaintInvalidationClean() with comments about why
it's necessary?

How important was Chris's original patch? It seems to have raised several issues
(2 of the linked bugs are P1)... should we just fully roll it out? It's not
clear to me why some of the calls to
updateLifecycleToCompositingCleanPlusScrolling are still safe while the ones in
this patch aren't.

[Xianzhu, 2016/04/13 22:51:45]

Done.
Good idea to update/add the asserts which ensures the least lifecycle
requirement. The asserts won't replace this comment which reminds us the extra
thing (synchronized painting) than needed. Will update/add the asserts in a
separate CL because I'm afraid
there might be something unexpected.
We must call recalcCollapsedBordersIfNeeded() during or before paint
invalidation to detect changed cells and invalidate them.
LayoutTable::collapsedBorders() is called from paint code which is too late
(which was a paint invalidation during painting bug).

Because we'll also need paint property update before painting for spv2, and will
integrate paint invalidation into pre-painting tree walk, updating collapsed
borders during paint invalidaiton (to be pre-painting tree walk) seems a
non-issue.

Though the bug is about collapsed borders, it's not the only reason that we
require paint invalidation before painting. We also need paint invalidation to
update some status/flags on LayoutObjects and PaintLayers in order to do correct
painting.
Whether its safe depends what is needed to be ready by the following code. At
this place, the following code does painting, so the document lifecycle should
be ready for painting. They other places are safe because they only need
compositing update ready.

+    m_view->updateAllLifecyclePhases();
     LayoutObject* layoutObject = node.layoutObject();
     if (!layoutObject)
         return nullptr;
 
     // Paint starting at the nearest self painting layer, clipped to the object itself.
     // TODO(pdr): This will also paint the content behind the object if the object contains
     // transparency but the layer is opaque. We could directly call layoutObject->paint(...)
     // (see ObjectPainter::paintAllPhasesAtomically) but this would skip self-painting children.
     PaintLayer* layer = layoutObject->enclosingLayer()->enclosingSelfPaintingLayer();
     IntRect absoluteBoundingBox = layoutObject->absoluteBoundingBoxRectIncludingDescendants();
     FloatRect boundingBox = layer->layoutObject()->absoluteToLocalQuad(FloatQuad(absoluteBoundingBox), UseTransforms).boundingBox();
     DragImageBuilder dragImageBuilder(this, boundingBox, &node);
     {
         PaintLayerPaintingInfo paintingInfo(layer, LayoutRect(boundingBox), GlobalPaintFlattenCompositingLayers, LayoutSize(), 0);
         PaintLayerFlags flags = PaintLayerHaveTransparency | PaintLayerAppliedTransform | PaintLayerUncachedClipRects;
         PaintLayerPainter(*layer).paintLayer(dragImageBuilder.context(), paintingInfo, flags);
     }
     return dragImageBuilder.createImage();
 }
 
 PassOwnPtr<DragImage> LocalFrame::dragImageForSelection(float opacity)
 {
     if (!selection().isRange())
         return nullptr;
 
-    m_view->updateLifecycleToCompositingCleanPlusScrolling();
+    // TODO(wangxianzhu): Don't need synchronized painting.
+    m_view->updateAllLifecyclePhases();
     ASSERT(document()->isActive());
 
     FloatRect paintingRect = FloatRect(selection().bounds());
     DragImageBuilder dragImageBuilder(this, paintingRect, nullptr, opacity);
     GlobalPaintFlags paintFlags = GlobalPaintSelectionOnly | GlobalPaintFlattenCompositingLayers;
     m_view->paintContents(dragImageBuilder.context(), paintFlags, enclosingIntRect(paintingRect));
     return dragImageBuilder.createImage();
 }
 
 String LocalFrame::selectedText() const
@@ skipping 199 matching lines @@
 {
     m_frame->disableNavigation();
 }
 
 FrameNavigationDisabler::~FrameNavigationDisabler()
 {
     m_frame->enableNavigation();
 }
 
 } // namespace blink