Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2513893002:
Make ifdefs consistent in WebKit/Source/core/ (Closed)
|
Created:
4 years, 1 month ago by Alexander Alekseev Modified:
4 years ago CC:
chromium-reviews, tzik, blink-reviews-platform-graphics_chromium.org, dshwang, eae+blinkwatch, fs, kouhei+svg_chromium.org, rwlbuis, krit, drott+blinkwatch_chromium.org, szager+layoutwatch_chromium.org, Justin Novosad, Rik, jchaffraix+rendering, blink-reviews, gyuyoung2, pdr+svgwatchlist_chromium.org, ajuma+watch_chromium.org, Mads Ager (chromium), zoltan1, blink-reviews-layout_chromium.org, jbroman, pdr+graphicswatchlist_chromium.org, haraken, danakj+watch_chromium.org, pdr+renderingwatchlist_chromium.org, leviw+renderwatch, f(malita), oilpan-reviews, kouhei+heap_chromium.org, kinuko+fileapi, nhiroki Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionMake ifdefs consistent in WebKit/Source/core/ .
This CL addresses a few problems with #ifdefs:
1) DCHECK requires the presence of variables used in it even if DCHECK_IS_ON()
evaluates to 0. Therefore, if debug variable is defined only when DCHECK_IS_ON()
is true, its usage must be guarded the same way.
2) If debug method in base class is defined only when DCHECK_IS_ON() is true,
its descendants must be guarded with the same check.
BUG=666660
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
Committed: https://crrev.com/5dd87e237131833c489f0b3ce5b231526318b37c
Cr-Commit-Position: refs/heads/master@{#435865}
Patch Set 1 #
Total comments: 4
Patch Set 2 : Update after review. #Patch Set 3 : Fix build #Patch Set 4 : Fix build. Update after review. #
Total comments: 9
Patch Set 5 : Fix build. #Patch Set 6 : Added #if DCHECK_IS_ON where needed. #
Total comments: 25
Patch Set 7 : Rebased. #Patch Set 8 : Limited fix to /core/ only. #
Total comments: 1
Patch Set 9 : Allow show* functions in debug build. #
Total comments: 2
Patch Set 10 : Remove extra #include #
Total comments: 8
Patch Set 11 : Revert small part #
Messages
Total messages: 76 (45 generated)
Description was changed from ========== Make ifdefs consistent in WebKit. Debug version of Chrome is not buildable because of inconsistent #ifdefs on NDEBUG/ASSERT/IS_DCHECK_ON . This change makes them consistemt in WebKit. BUG=666660 ========== to ========== Make ifdefs consistent in WebKit. Debug version of Chrome is not buildable because of inconsistent #ifdefs on NDEBUG/ASSERT/IS_DCHECK_ON . This change makes them consistemt in WebKit. BUG=666660 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 ==========
The CQ bit was checked by alemate@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...
alemate@chromium.org changed reviewers: + nhiroki@chromium.org, pdr@chromium.org
Please review: nhiroki@ : modules/filesystem/FileWriterSync.cpp pdr@ : All the other files.
mstensho@opera.com changed reviewers: + mstensho@opera.com
I didn't go through everything, but it seems to me that you could just switch over some problematic ASSERTs to using DCHECK instead, rather than using #if around it. Can you check? https://codereview.chromium.org/2513893002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/frame/LocalDOMWindow.cpp (left): https://codereview.chromium.org/2513893002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/frame/LocalDOMWindow.cpp:379: ASSERT(!EventDispatchForbiddenScope::isEventDispatchForbidden()); Just use DCHECK() instead? https://codereview.chromium.org/2513893002/diff/1/third_party/WebKit/Source/p... File third_party/WebKit/Source/platform/Timer.h (left): https://codereview.chromium.org/2513893002/diff/1/third_party/WebKit/Source/p... third_party/WebKit/Source/platform/Timer.h:199: ASSERT(m_thread == currentThread()); Wouldn't it work without the #if if you just used DCHECK instead of ASSERT?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
yutak@chromium.org changed reviewers: + yutak@chromium.org
Generally speaking, you should NOT guard ASSERT with DCHECK_IS_ON. ASSERT should automatically do that for you. I believe we continuously build debug versions, so I assume they should compile fine. I guess you are building with a non-standard configuration. Can you tell us how you compile it?
The CQ bit was checked by alemate@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...
On 2016/11/18 09:29:51, Yuta Kitamura wrote: > Generally speaking, you should NOT guard ASSERT with > DCHECK_IS_ON. ASSERT should automatically do that for you. > > I believe we continuously build debug versions, so I assume > they should compile fine. I guess you are building with > a non-standard configuration. Can you tell us how you compile > it? I sometimes need full stack in stack trace, so I use debug build. But quite often I get several crashes on DCHECKs before I get to the point, where I need my stack trace. So I build debug configuration without DCHECKs. I usually just define DCHECK_IS_ON 0 in logging.h and everything builds just fine. But at some moment I was not easily possible, because of inconsistency between "#ifdef"s. Mostly in WebKit. (And one file in zlib.) Chrome code is consistent in "#ifdef"s . This CL just makes "#ifdef"s consistent. https://codereview.chromium.org/2513893002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/frame/LocalDOMWindow.cpp (left): https://codereview.chromium.org/2513893002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/frame/LocalDOMWindow.cpp:379: ASSERT(!EventDispatchForbiddenScope::isEventDispatchForbidden()); On 2016/11/18 08:45:54, mstensho wrote: > Just use DCHECK() instead? Done. https://codereview.chromium.org/2513893002/diff/1/third_party/WebKit/Source/p... File third_party/WebKit/Source/platform/Timer.h (left): https://codereview.chromium.org/2513893002/diff/1/third_party/WebKit/Source/p... third_party/WebKit/Source/platform/Timer.h:199: ASSERT(m_thread == currentThread()); On 2016/11/18 08:45:54, mstensho wrote: > Wouldn't it work without the #if if you just used DCHECK instead of ASSERT? Done. https://codereview.chromium.org/2513893002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/frame/LocalDOMWindow.cpp (right): https://codereview.chromium.org/2513893002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/frame/LocalDOMWindow.cpp:379: DCHECK(!EventDispatchForbiddenScope::isEventDispatchForbidden()); isEventDispatchForbidden() is defined only if DCHECKs are on. https://codereview.chromium.org/2513893002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/layout/LayoutObject.cpp (right): https://codereview.chromium.org/2513893002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/LayoutObject.cpp:3455: paintInvalidationStateIsDirty()); paintInvalidationStateIsDirty() is defined only if '#ifndef NDEBUG' https://codereview.chromium.org/2513893002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/layout/LayoutObject.h (right): https://codereview.chromium.org/2513893002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/LayoutObject.h:1939: virtual bool paintInvalidationStateIsDirty() const { This is called above only if '#ifndef NDEBUG' https://codereview.chromium.org/2513893002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/svg/SVGScriptElement.h (right): https://codereview.chromium.org/2513893002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/svg/SVGScriptElement.h:46: bool isAnimatableAttribute(const QualifiedName&) const override; This is declared in the base class only if DCHECK_IS_ON(). https://codereview.chromium.org/2513893002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/filesystem/FileWriterSync.cpp (right): https://codereview.chromium.org/2513893002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/filesystem/FileWriterSync.cpp:44: DCHECK(m_complete); m_copplete is declared only if DCHECK_IS_ON() https://codereview.chromium.org/2513893002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/graphics/paint/ClipDisplayItem.h (right): https://codereview.chromium.org/2513893002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/graphics/paint/ClipDisplayItem.h:69: return DisplayItem::isClipType(otherType); This is declared in the base class only if DCHECK_IS_ON() https://codereview.chromium.org/2513893002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/graphics/paint/ClipPathDisplayItem.h (right): https://codereview.chromium.org/2513893002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/graphics/paint/ClipPathDisplayItem.h:54: bool isEndAndPairedWith(DisplayItem::Type otherType) const final { This is declared in the base class only if DCHECK_IS_ON() https://codereview.chromium.org/2513893002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/graphics/paint/CompositingDisplayItem.h (right): https://codereview.chromium.org/2513893002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/graphics/paint/CompositingDisplayItem.h:81: bool isEndAndPairedWith(DisplayItem::Type otherType) const final { This is called when '#ifdef NDEBUG' https://codereview.chromium.org/2513893002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/graphics/paint/DisplayItem.h (right): https://codereview.chromium.org/2513893002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/graphics/paint/DisplayItem.h:422: bool isEndAndPairedWith(DisplayItem::Type otherType) const override = 0; This is declared in the base class only if NDEBUG
The CQ bit was checked by alemate@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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...) chromeos_x86-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_x86-ge...)
The CQ bit was checked by alemate@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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
The CQ bit was checked by alemate@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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
I put back some of "#if IS_DCHECK_ON()" because DCHECK actually requires its arguments to build even in Release build.
I'm a bit confused about these changes. You mean DCHECK() can be tested even if
DCHECK_IS_ON() is false, right? I feel it's a bug of the macro...
... ah, I understand the situation! According to the definition of DCHECK() (see
following a code snippet), a condition in DCHECK() appears on sourcecode even if
DCHECK_IS_ON() is false so that it can suppress a warning about an unused
variable.
<COPIED FROM base/logging.h>
// DCHECK et al. make sure to reference |condition| regardless of
// whether DCHECKs are enabled; this is so that we don't get unused
// variable warnings if the only use of a variable is in a DCHECK.
// This behavior is different from DLOG_IF et al.
#define DCHECK(condition) \
LAZY_STREAM(LOG_STREAM(DCHECK), DCHECK_IS_ON() ? !(condition) : false) \
<< "Check failed: " #condition ". "
Considering it, these changes make sense to me.
https://codereview.chromium.org/2513893002/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/filesystem/FileWriterSync.cpp (right): https://codereview.chromium.org/2513893002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/filesystem/FileWriterSync.cpp:44: #if DCHECK_IS_ON() Instead of adding DCHECK_IS_ON(), can you completely remove these macros from FileWriterSync.{cpp,h}? I think bool operations are not expensive and improving readability is more beneficial.
Almost all of the changes are incorrect, hence NOT LGTM. To disable DCHECK/ASSERT, you should set a GN variable dcheck_always_on to false. You should not ever touch the definition of DCHECK_IS_ON() (or anything in logging.h). https://codereview.chromium.org/2513893002/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/frame/LocalDOMWindow.cpp (right): https://codereview.chromium.org/2513893002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/frame/LocalDOMWindow.cpp:379: #if DCHECK_IS_ON() This is not necessary. DCHECK is guarded with DCHECK_IS_ON(), so you shouldn't have to do it yourself. https://codereview.chromium.org/2513893002/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/layout/LayoutObject.cpp (right): https://codereview.chromium.org/2513893002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/layout/LayoutObject.cpp:3453: #ifndef NDEBUG This is wrong. This shouldn't be necessary. https://codereview.chromium.org/2513893002/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/layout/LayoutObject.h (right): https://codereview.chromium.org/2513893002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/layout/LayoutObject.h:1938: #ifndef NDEBUG This is wrong (and the original code is wrong, too). This should be "#if ENABLE(ASSERT)". This is how we say "if ASSERT() is enabled". https://codereview.chromium.org/2513893002/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/svg/SVGScriptElement.cpp (right): https://codereview.chromium.org/2513893002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/svg/SVGScriptElement.cpp:146: #if DCHECK_IS_ON() Ditto. https://codereview.chromium.org/2513893002/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/svg/SVGScriptElement.h (right): https://codereview.chromium.org/2513893002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/svg/SVGScriptElement.h:45: #if DCHECK_IS_ON() The original code is correct. https://codereview.chromium.org/2513893002/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/filesystem/FileWriterSync.cpp (right): https://codereview.chromium.org/2513893002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/filesystem/FileWriterSync.cpp:44: #if DCHECK_IS_ON() Ditto. All these changes are wrong.
On 2016/11/21 05:32:06, Yuta Kitamura wrote: > Almost all of the changes are incorrect, hence NOT LGTM. > > To disable DCHECK/ASSERT, you should set a GN variable > dcheck_always_on to false. You should not ever touch > the definition of DCHECK_IS_ON() (or anything in logging.h). As my previous comment, it looks like that dcheck_always_on()==false does not completely disable(remove) DCHECKs and compilation may fail if the check has a condition wrapped with DCHECK_IS_ON() like this: #if DCHECK_IS_ON() bool m_isValid; #endif DCHECK(m_isValid); Anyway, I agree that we shouldn't directly tweak logging.h.
On 2016/11/21 06:23:15, nhiroki wrote: > On 2016/11/21 05:32:06, Yuta Kitamura wrote: > > Almost all of the changes are incorrect, hence NOT LGTM. > > > > To disable DCHECK/ASSERT, you should set a GN variable > > dcheck_always_on to false. You should not ever touch > > the definition of DCHECK_IS_ON() (or anything in logging.h). > > As my previous comment, it looks like that dcheck_always_on()==false does not > completely disable(remove) DCHECKs and compilation may fail if the check has a > condition wrapped with DCHECK_IS_ON() like this: > > #if DCHECK_IS_ON() > bool m_isValid; > #endif > DCHECK(m_isValid); > > Anyway, I agree that we shouldn't directly tweak logging.h. Ah hah, that seems legit. Will look into those again.
Okay, most of the changes seems reasonable. (I retract
my previous NOT L-G-T-M here.)
But please:
- Fix the change description so that it would be more
understandable.
(1) "Debug version" does not indicate DCHECK-disabled
builds.
(2) It does not explain the core problem here: "DCHECK
requires the presence of variables used in it".
- Split your change to smaller pieces according to the
module/ownership boundaries, and ask module OWNERs
if those changes are okay for them. Module owners
may be able to propose a better solution, just like
nhiroki did.
https://codereview.chromium.org/2513893002/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/frame/LocalDOMWindow.cpp (right):
https://codereview.chromium.org/2513893002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/frame/LocalDOMWindow.cpp:379: #if DCHECK_IS_ON()
This is OK.
https://codereview.chromium.org/2513893002/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/layout/LayoutObject.cpp (right):
https://codereview.chromium.org/2513893002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/LayoutObject.cpp:3453: #ifndef NDEBUG
NDEBUG does not make sense here. Use DCHECK_IS_ON() or ENABLE(ASSERT) as
mentioned earlier.
https://codereview.chromium.org/2513893002/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/layout/LayoutObject.h (right):
https://codereview.chromium.org/2513893002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/LayoutObject.h:1938: #ifndef NDEBUG
On 2016/11/21 05:32:06, Yuta Kitamura wrote:
> This is wrong (and the original code is wrong, too).
>
> This should be "#if ENABLE(ASSERT)". This is how we say "if ASSERT() is
> enabled".
This comment was wrong.
This should be DCHECK_IS_ON() or ENABLE(ASSERT), and all the use
sites must consistently use either DCHECK or ASSERT.
https://codereview.chromium.org/2513893002/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/svg/SVGScriptElement.cpp (right):
https://codereview.chromium.org/2513893002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/svg/SVGScriptElement.cpp:146: #if DCHECK_IS_ON()
On 2016/11/21 05:32:06, Yuta Kitamura wrote:
> Ditto.
You are correct here too.
https://codereview.chromium.org/2513893002/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/svg/SVGScriptElement.h (right):
https://codereview.chromium.org/2513893002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/svg/SVGScriptElement.h:45: #if DCHECK_IS_ON()
On 2016/11/21 05:32:06, Yuta Kitamura wrote:
> The original code is correct.
I was wrong. You are correct.
https://codereview.chromium.org/2513893002/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/filesystem/FileWriterSync.cpp (right):
https://codereview.chromium.org/2513893002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/filesystem/FileWriterSync.cpp:44: #if
DCHECK_IS_ON()
On 2016/11/21 05:32:06, Yuta Kitamura wrote:
> Ditto. All these changes are wrong.
I was wrong. I agree with nhiroki here.
https://codereview.chromium.org/2513893002/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/Timer.cpp (right):
https://codereview.chromium.org/2513893002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/Timer.cpp:62: DCHECK(m_thread ==
currentThread());
Ditto for this file.
https://codereview.chromium.org/2513893002/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/Timer.h (right):
https://codereview.chromium.org/2513893002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/Timer.h:200: DCHECK(m_thread ==
currentThread());
Use DCHECK_EQ.
https://codereview.chromium.org/2513893002/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/graphics/GraphicsContext.h (right):
https://codereview.chromium.org/2513893002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/graphics/GraphicsContext.h:452: int
m_layerCount;
You don't have to change this.
https://codereview.chromium.org/2513893002/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/graphics/paint/DisplayItem.h (right):
https://codereview.chromium.org/2513893002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/graphics/paint/DisplayItem.h:421: #ifndef
NDEBUG
This should be DCHECK_IS_ON().
https://codereview.chromium.org/2513893002/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/heap/PersistentNode.cpp (right):
https://codereview.chromium.org/2513893002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/heap/PersistentNode.cpp:38:
DCHECK(persistentCount == m_persistentCount);
Use DCHECK_EQ.
Description was changed from ========== Make ifdefs consistent in WebKit. Debug version of Chrome is not buildable because of inconsistent #ifdefs on NDEBUG/ASSERT/IS_DCHECK_ON . This change makes them consistemt in WebKit. BUG=666660 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 ========== to ========== Make ifdefs consistent in WebKit/Source/core/ . This CL addresses a few problems with #ifdefs: 1) DCHECK requires the presence of variables used in it even if DCHECK_IS_ON() evaluates to 0. Therefore, if debug variable is defined only when DCHECK_IS_ON() is true, its usage must be guarded the same way. 2) If debug method in base class is defined only when DCHECK_IS_ON() is true, its descendants must be guarded with the same check. BUG=666660 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 ==========
I updated Cl description and spun off two parts of it leaving only /core/ . Please review. https://codereview.chromium.org/2513893002/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/layout/LayoutObject.cpp (right): https://codereview.chromium.org/2513893002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/layout/LayoutObject.cpp:3453: #ifndef NDEBUG On 2016/11/21 09:26:11, Yuta Kitamura wrote: > NDEBUG does not make sense here. Use DCHECK_IS_ON() or ENABLE(ASSERT) as > mentioned earlier. Done. https://codereview.chromium.org/2513893002/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/filesystem/FileWriterSync.cpp (right): https://codereview.chromium.org/2513893002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/filesystem/FileWriterSync.cpp:44: #if DCHECK_IS_ON() On 2016/11/21 03:24:57, nhiroki wrote: > Instead of adding DCHECK_IS_ON(), can you completely remove these macros from > FileWriterSync.{cpp,h}? I think bool operations are not expensive and improving > readability is more beneficial. Done. https://codereview.chromium.org/2513893002/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/Timer.cpp (right): https://codereview.chromium.org/2513893002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/Timer.cpp:62: DCHECK(m_thread == currentThread()); On 2016/11/21 09:26:12, Yuta Kitamura wrote: > Ditto for this file. Done. https://codereview.chromium.org/2513893002/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/Timer.h (right): https://codereview.chromium.org/2513893002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/Timer.h:200: DCHECK(m_thread == currentThread()); On 2016/11/21 09:26:12, Yuta Kitamura wrote: > Use DCHECK_EQ. Done. https://codereview.chromium.org/2513893002/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/graphics/GraphicsContext.h (right): https://codereview.chromium.org/2513893002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/graphics/GraphicsContext.h:452: int m_layerCount; On 2016/11/21 09:26:12, Yuta Kitamura wrote: > You don't have to change this. Compilation of DCHECK_GT fails on windows with "signed/unsigned mismatch". https://codereview.chromium.org/2513893002/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/graphics/paint/DisplayItem.h (right): https://codereview.chromium.org/2513893002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/graphics/paint/DisplayItem.h:421: #ifndef NDEBUG On 2016/11/21 09:26:12, Yuta Kitamura wrote: > This should be DCHECK_IS_ON(). Done. https://codereview.chromium.org/2513893002/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/heap/PersistentNode.cpp (right): https://codereview.chromium.org/2513893002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/heap/PersistentNode.cpp:38: DCHECK(persistentCount == m_persistentCount); On 2016/11/21 09:26:12, Yuta Kitamura wrote: > Use DCHECK_EQ. Done.
alemate@chromium.org changed reviewers: - nhiroki@chromium.org
Looks like you're moving debug dump functions over from #ifndef NDEBUG to #if DCHECK_IS_ON(). I don't think that's right, and I'd like to see that resolved before reviewing any further. https://codereview.chromium.org/2513893002/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/layout/LayoutObject.h (right): https://codereview.chromium.org/2513893002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/core/layout/LayoutObject.h:434: #if DCHECK_IS_ON() I don't think these have anything to do with assertions. I call them from gdb. I don't know if they are used for other things conditioned on !NDEBUG in the code, but probably not. Anyway, I think #ifndef NDEBUG is the right way here.
On 2016/11/29 12:06:12, mstensho wrote: > Looks like you're moving debug dump functions over from #ifndef NDEBUG to #if > DCHECK_IS_ON(). I don't think that's right, and I'd like to see that resolved > before reviewing any further. > > https://codereview.chromium.org/2513893002/diff/140001/third_party/WebKit/Sou... > File third_party/WebKit/Source/core/layout/LayoutObject.h (right): > > https://codereview.chromium.org/2513893002/diff/140001/third_party/WebKit/Sou... > third_party/WebKit/Source/core/layout/LayoutObject.h:434: #if DCHECK_IS_ON() > I don't think these have anything to do with assertions. I call them from gdb. I > don't know if they are used for other things conditioned on !NDEBUG in the code, > but probably not. > > Anyway, I think #ifndef NDEBUG is the right way here. But if function is used in DCHECK(), it must be either enabled when DCHECKS are on (for example, when dcheck_always_on build option is set), or this particular DCHECK should be inside #ifdef NDEBUG. What is the correct solution here?
On 2016/11/30 00:25:19, Alexander Alekseev wrote: > On 2016/11/29 12:06:12, mstensho wrote: > > Looks like you're moving debug dump functions over from #ifndef NDEBUG to #if > > DCHECK_IS_ON(). I don't think that's right, and I'd like to see that resolved > > before reviewing any further. > > > > > https://codereview.chromium.org/2513893002/diff/140001/third_party/WebKit/Sou... > > File third_party/WebKit/Source/core/layout/LayoutObject.h (right): > > > > > https://codereview.chromium.org/2513893002/diff/140001/third_party/WebKit/Sou... > > third_party/WebKit/Source/core/layout/LayoutObject.h:434: #if DCHECK_IS_ON() > > I don't think these have anything to do with assertions. I call them from gdb. > I > > don't know if they are used for other things conditioned on !NDEBUG in the > code, > > but probably not. > > > > Anyway, I think #ifndef NDEBUG is the right way here. > > But if function is used in DCHECK(), it must be either enabled when DCHECKS are > on (for example, when dcheck_always_on build option is set), or this particular > DCHECK should be inside #ifdef NDEBUG. > What is the correct solution here? The various show* methods are not called from DCHECK code as far as I know. In your patch the only case I saw was "assertLaidOut" but in that cause you can leave the NDEBUG block within the DCHECK_IN_ON block. They exist for debug builds, as mstensho@ says and should not ideally depend on DCHECK state. They should continue to be NDEBUG controlled.
On 2016/11/30 00:50:42, Stephen Chennney wrote: > On 2016/11/30 00:25:19, Alexander Alekseev wrote: > > On 2016/11/29 12:06:12, mstensho wrote: > > > Looks like you're moving debug dump functions over from #ifndef NDEBUG to > #if > > > DCHECK_IS_ON(). I don't think that's right, and I'd like to see that > resolved > > > before reviewing any further. > > > > > > > > > https://codereview.chromium.org/2513893002/diff/140001/third_party/WebKit/Sou... > > > File third_party/WebKit/Source/core/layout/LayoutObject.h (right): > > > > > > > > > https://codereview.chromium.org/2513893002/diff/140001/third_party/WebKit/Sou... > > > third_party/WebKit/Source/core/layout/LayoutObject.h:434: #if DCHECK_IS_ON() > > > I don't think these have anything to do with assertions. I call them from > gdb. > > I > > > don't know if they are used for other things conditioned on !NDEBUG in the > > code, > > > but probably not. > > > > > > Anyway, I think #ifndef NDEBUG is the right way here. > > > > But if function is used in DCHECK(), it must be either enabled when DCHECKS > are > > on (for example, when dcheck_always_on build option is set), or this > particular > > DCHECK should be inside #ifdef NDEBUG. > > What is the correct solution here? > > The various show* methods are not called from DCHECK code as far as I know. In > your patch the only case I saw was "assertLaidOut" but in that cause you can > leave the NDEBUG block within the DCHECK_IN_ON block. They exist for debug > builds, as mstensho@ says and should not ideally depend on DCHECK state. They > should continue to be NDEBUG controlled. Ah, now I understand what's wrong here. assertClearedPaintInvalidationFlags() uses showLayoutTreeForThis(); So I just added #ifndef NDEBUG to it and the patch became much smaller. PTAL.
The CQ bit was checked by alemate@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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: blimp_linux_dbg on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) cast_shell_linux on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) chromeos_amd64-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) chromeos_x86-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) chromium_presubmit on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_chromeos_compile_dbg_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_clobber_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_layout_tests_slimming_paint_v2 on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL)
The CQ bit was checked by alemate@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...
https://codereview.chromium.org/2513893002/diff/160001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/layout/LayoutObject.cpp (right): https://codereview.chromium.org/2513893002/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/core/layout/LayoutObject.cpp:35: #include "core/dom/Node.h" No need for this. It's quite evident that LayoutObject.h already includes this somehow. See LayoutObject::isPseudoElement(), for instance.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2513893002/diff/160001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/layout/LayoutObject.cpp (right): https://codereview.chromium.org/2513893002/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/core/layout/LayoutObject.cpp:35: #include "core/dom/Node.h" On 2016/11/30 10:00:13, mstensho wrote: > No need for this. It's quite evident that LayoutObject.h already includes this > somehow. See LayoutObject::isPseudoElement(), for instance. Done.
lgtm
The CQ bit was checked by alemate@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...
Looks good except for the #ifndef block change noted below. LGTM if the #ifndef block is reverted back to the original range. https://codereview.chromium.org/2513893002/diff/180001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/layout/LayoutObject.h (right): https://codereview.chromium.org/2513893002/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/core/layout/LayoutObject.h:325: if (paintInvalidationStateIsDirty()) { Why did this get out of #ifndef block? This check should only be performed only if NDEBUG is not defined, shouldn't it?
https://codereview.chromium.org/2513893002/diff/180001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/layout/LayoutObject.h (right): https://codereview.chromium.org/2513893002/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/core/layout/LayoutObject.h:325: if (paintInvalidationStateIsDirty()) { On 2016/12/01 08:18:56, Yuta Kitamura wrote: > Why did this get out of #ifndef block? This check should only be > performed only if NDEBUG is not defined, shouldn't it? 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.
https://codereview.chromium.org/2513893002/diff/180001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/layout/LayoutObject.h (right): https://codereview.chromium.org/2513893002/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/core/layout/LayoutObject.h:325: if (paintInvalidationStateIsDirty()) { On 2016/12/01 08:29:21, mstensho wrote: > On 2016/12/01 08:18:56, Yuta Kitamura wrote: > > Why did this get out of #ifndef block? This check should only be > > performed only if NDEBUG is not defined, shouldn't it? > > 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. Yes, you are absolutely right.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
[The last conditional L-G-T-M still holds.] https://codereview.chromium.org/2513893002/diff/180001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/layout/LayoutObject.h (right): https://codereview.chromium.org/2513893002/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/core/layout/LayoutObject.h:325: if (paintInvalidationStateIsDirty()) { On 2016/12/01 08:29:21, mstensho wrote: > On 2016/12/01 08:18:56, Yuta Kitamura wrote: > > Why did this get out of #ifndef block? This check should only be > > performed only if NDEBUG is not defined, shouldn't it? > > 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. 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.
https://codereview.chromium.org/2513893002/diff/180001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/layout/LayoutObject.h (right): https://codereview.chromium.org/2513893002/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/core/layout/LayoutObject.h:325: if (paintInvalidationStateIsDirty()) { On 2016/12/01 09:51:06, Yuta Kitamura wrote: > On 2016/12/01 08:29:21, mstensho wrote: > > On 2016/12/01 08:18:56, Yuta Kitamura wrote: > > > Why did this get out of #ifndef block? This check should only be > > > performed only if NDEBUG is not defined, shouldn't it? > > > > 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. > > 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. 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());
https://codereview.chromium.org/2513893002/diff/180001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/layout/LayoutObject.h (right): https://codereview.chromium.org/2513893002/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/core/layout/LayoutObject.h:325: if (paintInvalidationStateIsDirty()) { On 2016/12/01 10:00:37, mstensho wrote: > On 2016/12/01 09:51:06, Yuta Kitamura wrote: > > On 2016/12/01 08:29:21, mstensho wrote: > > > On 2016/12/01 08:18:56, Yuta Kitamura wrote: > > > > Why did this get out of #ifndef block? This check should only be > > > > performed only if NDEBUG is not defined, shouldn't it? > > > > > > 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. > > > > 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. > > 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()); 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.
https://codereview.chromium.org/2513893002/diff/180001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/layout/LayoutObject.h (right): https://codereview.chromium.org/2513893002/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/core/layout/LayoutObject.h:325: if (paintInvalidationStateIsDirty()) { On 2016/12/01 10:56:50, Yuta Kitamura wrote: > On 2016/12/01 10:00:37, mstensho wrote: > > On 2016/12/01 09:51:06, Yuta Kitamura wrote: > > > On 2016/12/01 08:29:21, mstensho wrote: > > > > On 2016/12/01 08:18:56, Yuta Kitamura wrote: > > > > > Why did this get out of #ifndef block? This check should only be > > > > > performed only if NDEBUG is not defined, shouldn't it? > > > > > > > > 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. > > > > > > 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. > > > > 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()); > > 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. I'm a layout owner, but landing this piece as a separate CL is of course fine too.
https://codereview.chromium.org/2513893002/diff/180001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/layout/LayoutObject.h (right): https://codereview.chromium.org/2513893002/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/core/layout/LayoutObject.h:325: if (paintInvalidationStateIsDirty()) { On 2016/12/01 12:44:10, mstensho wrote: > On 2016/12/01 10:56:50, Yuta Kitamura wrote: > > On 2016/12/01 10:00:37, mstensho wrote: > > > On 2016/12/01 09:51:06, Yuta Kitamura wrote: > > > > On 2016/12/01 08:29:21, mstensho wrote: > > > > > On 2016/12/01 08:18:56, Yuta Kitamura wrote: > > > > > > Why did this get out of #ifndef block? This check should only be > > > > > > performed only if NDEBUG is not defined, shouldn't it? > > > > > > > > > > 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. > > > > > > > > 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. > > > > > > 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()); > > > > 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. > > I'm a layout owner, but landing this piece as a separate CL is of course fine > too. That's fine. Let's move this to another CL.
I removed the questionable block from this CL and I am going to land this. Many thanks to everyone for the help!
The CQ bit was checked by alemate@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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by alemate@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from mstensho@opera.com, yutak@chromium.org Link to the patchset: https://codereview.chromium.org/2513893002/#ps200001 (title: "Revert small part")
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": 200001, "attempt_start_ts": 1480657537642620,
"parent_rev": "2d98b9f0c16de0f5d133b5931e2c1c6db641670a", "commit_rev":
"ced18e845de24473aebf9b4268cc8d0104661b17"}
Message was sent while issue was closed.
Description was changed from ========== Make ifdefs consistent in WebKit/Source/core/ . This CL addresses a few problems with #ifdefs: 1) DCHECK requires the presence of variables used in it even if DCHECK_IS_ON() evaluates to 0. Therefore, if debug variable is defined only when DCHECK_IS_ON() is true, its usage must be guarded the same way. 2) If debug method in base class is defined only when DCHECK_IS_ON() is true, its descendants must be guarded with the same check. BUG=666660 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 ========== to ========== Make ifdefs consistent in WebKit/Source/core/ . This CL addresses a few problems with #ifdefs: 1) DCHECK requires the presence of variables used in it even if DCHECK_IS_ON() evaluates to 0. Therefore, if debug variable is defined only when DCHECK_IS_ON() is true, its usage must be guarded the same way. 2) If debug method in base class is defined only when DCHECK_IS_ON() is true, its descendants must be guarded with the same check. BUG=666660 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 ==========
Message was sent while issue was closed.
Committed patchset #11 (id:200001)
Message was sent while issue was closed.
Description was changed from ========== Make ifdefs consistent in WebKit/Source/core/ . This CL addresses a few problems with #ifdefs: 1) DCHECK requires the presence of variables used in it even if DCHECK_IS_ON() evaluates to 0. Therefore, if debug variable is defined only when DCHECK_IS_ON() is true, its usage must be guarded the same way. 2) If debug method in base class is defined only when DCHECK_IS_ON() is true, its descendants must be guarded with the same check. BUG=666660 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 ========== to ========== Make ifdefs consistent in WebKit/Source/core/ . This CL addresses a few problems with #ifdefs: 1) DCHECK requires the presence of variables used in it even if DCHECK_IS_ON() evaluates to 0. Therefore, if debug variable is defined only when DCHECK_IS_ON() is true, its usage must be guarded the same way. 2) If debug method in base class is defined only when DCHECK_IS_ON() is true, its descendants must be guarded with the same check. BUG=666660 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 Committed: https://crrev.com/5dd87e237131833c489f0b3ce5b231526318b37c Cr-Commit-Position: refs/heads/master@{#435865} ==========
Message was sent while issue was closed.
Patchset 11 (id:??) landed as https://crrev.com/5dd87e237131833c489f0b3ce5b231526318b37c Cr-Commit-Position: refs/heads/master@{#435865} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
