Make ifdefs consistent in WebKit/Source/core/

third_party/WebKit/Source/core/layout/LayoutObject.h

 /*
  * Copyright (C) 2000 Lars Knoll (knoll@kde.org)
  *           (C) 2000 Antti Koivisto (koivisto@kde.org)
  *           (C) 2000 Dirk Mueller (mueller@kde.org)
  *           (C) 2004 Allan Sandfeld Jensen (kde@carewolf.com)
  * Copyright (C) 2003, 2004, 2005, 2006, 2007, 2008, 2009, 2012 Apple Inc.
  *               All rights reserved.
  * Copyright (C) 2009 Google Inc. All rights reserved.
  *
  * This library is free software; you can redistribute it and/or
@@ skipping 274 matching lines @@
   LayoutBox* enclosingScrollableBox() const;
 
   // Function to return our enclosing flow thread if we are contained inside
   // one. This function follows the containing block chain.
   LayoutFlowThread* flowThreadContainingBlock() const {
     if (!isInsideFlowThread())
       return nullptr;
     return locateFlowThreadContainingBlock();
   }
 
-#if ENABLE(ASSERT)
+#if DCHECK_IS_ON()
   void setHasAXObject(bool flag) { m_hasAXObject = flag; }
   bool hasAXObject() const { return m_hasAXObject; }
 
   // Helper class forbidding calls to setNeedsLayout() during its lifetime.
   class SetLayoutNeededForbiddenScope {
    public:
     explicit SetLayoutNeededForbiddenScope(LayoutObject&);
     ~SetLayoutNeededForbiddenScope();
 
    private:
     LayoutObject& m_layoutObject;
     bool m_preexistingForbidden;
   };
 
   void assertLaidOut() const {
 #ifndef NDEBUG
     if (needsLayout())
       showLayoutTreeForThis();
 #endif
     SECURITY_DCHECK(!needsLayout());
   }
 
   void assertSubtreeIsLaidOut() const {
     for (const LayoutObject* layoutObject = this; layoutObject;
          layoutObject = layoutObject->nextInPreOrder())
       layoutObject->assertLaidOut();
   }
 
   void assertClearedPaintInvalidationFlags() const {
+    if (paintInvalidationStateIsDirty()) {

[Yuta Kitamura, 2016/12/01 08:18:56]

Why did this get out of #ifndef block? This check should only be
performed only if NDEBUG is not defined, shouldn't it?

[mstensho (USE GERRIT), 2016/12/01 08:29:21]

This code is already inside an #if DCHECK_IS_ON(), which
paintInvalidationStateIsDirty() is conditioned on, so that should be fine. The
change is that we want to assert if this method returns true, even if NDEBUG is
defined, but we cannot call showLayoutTreeForThis() if NDEBUG is defined.

[Alexander Alekseev, 2016/12/01 08:54:39]

Yes, you are absolutely right.

[Yuta Kitamura, 2016/12/01 09:51:06]

I still don't agree. I feel like there may be some reason for excluding
this ASSERT_NOT_REACHED for non-debug builds (like the check being
executed too frequent, etc).

You change adds extra code to non-debug DCHECK builds without confiming
whether it is safe or not. I think you should stick to keeping the
current behavior in this patch, since changing this behavior is not
part of this patch's goal.

You can create another patch to change this later, asking the original
author of this code or some other layout OWNER.

[mstensho (USE GERRIT), 2016/12/01 10:00:37]

See assertLaidOut() for a similar pattern. That one throws a security assert.
That's not necessary here, though.

We might of course rewrite it as:

#ifndef NDEBUG
    if (paintInvalidationStateIsDirty())
      showLayoutTreeForThis();
#endif
    DCHECK(!paintInvalidationStateIsDirty());

[Yuta Kitamura, 2016/12/01 10:56:50]

Please don't do that either unless you confirm adding that
DCHECK is safe. My advice here remains to be "don't change
the behavior in this patch."

I'm not proficient in layout code, so I can't approve
any behavior change here.

[mstensho (USE GERRIT), 2016/12/01 12:44:10]

I'm a layout owner, but landing this piece as a separate CL is of course fine
too.

[Alexander Alekseev, 2016/12/02 03:39:31]

That's fine. Let's move this to another CL.

 #ifndef NDEBUG
-    if (paintInvalidationStateIsDirty()) {
       showLayoutTreeForThis();
+#endif
       ASSERT_NOT_REACHED();
     }
-#endif
   }
 
   void assertSubtreeClearedPaintInvalidationFlags() const {
     for (const LayoutObject* layoutObject = this; layoutObject;
          layoutObject = layoutObject->nextInPreOrder())
       layoutObject->assertClearedPaintInvalidationFlags();
   }
 
 #endif
 
@@ skipping 71 matching lines @@
     // if we're not a LayoutFlowThread.
     // A LayoutFlowThread is always considered to be inside itself, so it never
     // has to change its state in response to parent changes.
     bool insideFlowThread = parent && parent->isInsideFlowThread();
     if (insideFlowThread != isInsideFlowThread() && !isLayoutFlowThread())
       setIsInsideFlowThreadIncludingDescendants(insideFlowThread);
   }
 
   //////////////////////////////////////////
  private:
-#if ENABLE(ASSERT)
+#if DCHECK_IS_ON()
   bool isSetNeedsLayoutForbidden() const { return m_setNeedsLayoutForbidden; }
   void setNeedsLayoutIsForbidden(bool flag) {
     m_setNeedsLayoutForbidden = flag;
   }
 #endif
 
   void addAbsoluteRectForLayer(IntRect& result);
   bool requiresAnonymousTableWrappers(const LayoutObject*) const;
 
   // Gets pseudoStyle from Shadow host(in case of input elements)
@@ skipping 1599 matching lines @@
   void removeFromLayoutFlowThreadRecursive(LayoutFlowThread*);
 
   ComputedStyle* cachedFirstLineStyle() const;
   StyleDifference adjustStyleDifference(StyleDifference) const;
 
   Color selectionColor(int colorProperty, const GlobalPaintFlags) const;
 
   void removeShapeImageClient(ShapeValue*);
   void removeCursorImageClient(const CursorList*);
 
-#if ENABLE(ASSERT)
+#if DCHECK_IS_ON()
   void checkBlockPositionedObjectsNeedLayout();
 #endif
 
   bool isTextOrSVGChild() const {
     return isText() || (isSVG() && !isSVGRoot());
   }
 
   static bool isAllowedToModifyLayoutTreeStructure(Document&);
 
   // Returns the parent for paint invalidation.
   // - For LayoutView, returns the owner layout object in the containing frame
   //   if any or nullptr;
   // - For multi-column spanner, returns the spanner placeholder;
   // - Otherwise returns parent().
   LayoutObject* paintInvalidationParent() const;
 
   RefPtr<ComputedStyle> m_style;
 
   // Oilpan: This untraced pointer to the owning Node is considered safe.
   UntracedMember<Node> m_node;
 
   LayoutObject* m_parent;
   LayoutObject* m_previous;
   LayoutObject* m_next;
 
-#if ENABLE(ASSERT)
+#if DCHECK_IS_ON()
   unsigned m_hasAXObject : 1;
   unsigned m_setNeedsLayoutForbidden : 1;
 #endif
 
 #define ADD_BOOLEAN_BITFIELD(name, Name) \
  private:                                \
   unsigned m_##name : 1;                 \
                                          \
  public:                                 \
   bool name() const { return m_##name; } \
@@ skipping 434 matching lines @@
   setMayNeedPaintInvalidation();
 
   // Clear needsLayout flags.
   setSelfNeedsLayout(false);
   setPosChildNeedsLayout(false);
   setNeedsSimplifiedNormalFlowLayout(false);
   setNormalChildNeedsLayout(false);
   setNeedsPositionedMovementLayout(false);
   setAncestorLineBoxDirty(false);
 
-#if ENABLE(ASSERT)
+#if DCHECK_IS_ON()
   checkBlockPositionedObjectsNeedLayout();
 #endif
 
   setScrollAnchorDisablingStyleChanged(false);
 }
 
 inline void LayoutObject::setChildNeedsLayout(MarkingBehavior markParents,
                                               SubtreeLayoutScope* layouter) {
   ASSERT(!isSetNeedsLayoutForbidden());
   bool alreadyNeededLayout = normalChildNeedsLayout();
@@ skipping 111 matching lines @@
 CORE_EXPORT void showLineTree(const blink::LayoutObject*);
 CORE_EXPORT void showLayoutTree(const blink::LayoutObject* object1);
 // We don't make object2 an optional parameter so that showLayoutTree
 // can be called from gdb easily.
 CORE_EXPORT void showLayoutTree(const blink::LayoutObject* object1,
                                 const blink::LayoutObject* object2);
 
 #endif
 
 #endif  // LayoutObject_h