Make ifdefs consistent in WebKit/Source/core/

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

Index: third_party/WebKit/Source/core/layout/LayoutObject.h
diff --git a/third_party/WebKit/Source/core/layout/LayoutObject.h b/third_party/WebKit/Source/core/layout/LayoutObject.h
index 8478f86e332c7f6041d231b7552db5f38e8c24dc..cb8a9290b3b75d2fd58108e018874e07104da9d4 100644
--- a/third_party/WebKit/Source/core/layout/LayoutObject.h
+++ b/third_party/WebKit/Source/core/layout/LayoutObject.h
@@ -292,7 +292,7 @@ class CORE_EXPORT LayoutObject : public ImageResourceObserver,
     return locateFlowThreadContainingBlock();
   }
 
-#if ENABLE(ASSERT)
+#if DCHECK_IS_ON()
   void setHasAXObject(bool flag) { m_hasAXObject = flag; }
   bool hasAXObject() const { return m_hasAXObject; }
 
@@ -322,12 +322,12 @@ class CORE_EXPORT LayoutObject : public ImageResourceObserver,
   }
 
   void assertClearedPaintInvalidationFlags() const {
-#ifndef NDEBUG
     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
       showLayoutTreeForThis();
+#endif
       ASSERT_NOT_REACHED();
     }
-#endif
   }
 
   void assertSubtreeClearedPaintInvalidationFlags() const {
@@ -419,7 +419,7 @@ class CORE_EXPORT LayoutObject : public ImageResourceObserver,
 
   //////////////////////////////////////////
  private:
-#if ENABLE(ASSERT)
+#if DCHECK_IS_ON()
   bool isSetNeedsLayoutForbidden() const { return m_setNeedsLayoutForbidden; }
   void setNeedsLayoutIsForbidden(bool flag) {
     m_setNeedsLayoutForbidden = flag;
@@ -2039,7 +2039,7 @@ class CORE_EXPORT LayoutObject : public ImageResourceObserver,
   void removeShapeImageClient(ShapeValue*);
   void removeCursorImageClient(const CursorList*);
 
-#if ENABLE(ASSERT)
+#if DCHECK_IS_ON()
   void checkBlockPositionedObjectsNeedLayout();
 #endif
 
@@ -2065,7 +2065,7 @@ class CORE_EXPORT LayoutObject : public ImageResourceObserver,
   LayoutObject* m_previous;
   LayoutObject* m_next;
 
-#if ENABLE(ASSERT)
+#if DCHECK_IS_ON()
   unsigned m_hasAXObject : 1;
   unsigned m_setNeedsLayoutForbidden : 1;
 #endif
@@ -2520,7 +2520,7 @@ inline void LayoutObject::clearNeedsLayout() {
   setNeedsPositionedMovementLayout(false);
   setAncestorLineBoxDirty(false);
 
-#if ENABLE(ASSERT)
+#if DCHECK_IS_ON()
   checkBlockPositionedObjectsNeedLayout();
 #endif