Issue 1839643009: RELEASE_ASSERT -> CHECK and ASSERT -> DCHECK in web.

Issue 1839643009: RELEASE_ASSERT -> CHECK and ASSERT -> DCHECK in web. (Closed)

Created:
4 years, 8 months ago by kotenkov

Modified:
4 years, 8 months ago

Reviewers:
tkent, haraken

CC:
blink-reviews, chromium-reviews

Base URL:
https://chromium.googlesource.com/chromium/src.git@master

Target Ref:
refs/pending/heads/master

Project:
chromium

Visibility:
Public.

More Reviews

Description

RELEASE_ASSERT -> CHECK and ASSERT -> DCHECK in web. BUG=596760 Committed: https://crrev.com/151deeb377f9456660e9ab835d1c7fa46edc7499 Cr-Commit-Position: refs/heads/master@{#384626}

Patch Set 2 : ASSERT -> DCHECK #

Patch Set 3 : Fix compilation. #

Total comments: 13

Patch Set 4 : Split DCHECKs. #

Patch Set 5 : Use DCHECK_IS_ON() instead of ENABLE(ASSERT). #

Patch Set 7 : One more split and remove redundant DCHECK_IS_ON(). #

Patch Set 8 : One more split and remove redundant DCHECK_IS_ON(). #

Patch Set 9 : Return DCHECK_IS_ON checks. #

Created: 4 years, 8 months ago

Download [raw] [tar.bz2]

	Unified diffs	Side-by-side diffs	Delta from patch set	Stats (+471 lines, -455 lines)			Patch
M	third_party/WebKit/Source/web/AssociatedURLLoader.cpp	View	1 2 3 4 5	5 chunks	+9 lines, -9 lines	0 comments	Download
M	third_party/WebKit/Source/web/AudioOutputDeviceClientImpl.cpp	View	1 2 3 4 5 6	1 chunk	+2 lines, -1 line	0 comments	Download
M	third_party/WebKit/Source/web/ChromeClientImpl.cpp	View	1 2 3 4 5	10 chunks	+16 lines, -16 lines	0 comments	Download
M	third_party/WebKit/Source/web/ColorChooserPopupUIController.cpp	View	1	3 chunks	+4 lines, -4 lines	0 comments	Download
M	third_party/WebKit/Source/web/ColorChooserUIController.cpp	View	1	2 chunks	+2 lines, -2 lines	0 comments	Download
M	third_party/WebKit/Source/web/ContextFeaturesClientImpl.cpp	View	1 2 3 4 5	3 chunks	+3 lines, -3 lines	0 comments	Download
M	third_party/WebKit/Source/web/DatabaseClientImpl.cpp	View	1 2 3 4 5	1 chunk	+1 line, -1 line	0 comments	Download
M	third_party/WebKit/Source/web/DateTimeChooserImpl.cpp	View	1 2 3 4 5	2 chunks	+3 lines, -3 lines	0 comments	Download
M	third_party/WebKit/Source/web/DragClientImpl.cpp	View	1 2 3 4 5	1 chunk	+1 line, -1 line	0 comments	Download
M	third_party/WebKit/Source/web/ExternalDateTimeChooser.cpp	View	1 2 3 4 5	1 chunk	+2 lines, -2 lines	0 comments	Download
M	third_party/WebKit/Source/web/FindInPageCoordinates.cpp	View	1	3 chunks	+4 lines, -4 lines	0 comments	Download
M	third_party/WebKit/Source/web/FrameLoaderClientImpl.cpp	View	1 2 3 4 5	3 chunks	+3 lines, -3 lines	0 comments	Download
M	third_party/WebKit/Source/web/FullscreenController.cpp	View	1 2 3 4 5	2 chunks	+2 lines, -2 lines	0 comments	Download
M	third_party/WebKit/Source/web/InspectorOverlay.cpp	View	1 2 3 4 5	5 chunks	+11 lines, -11 lines	0 comments	Download
M	third_party/WebKit/Source/web/LinkHighlightImpl.cpp	View	1	3 chunks	+6 lines, -6 lines	0 comments	Download
M	third_party/WebKit/Source/web/LocalFileSystemClient.cpp	View	1	1 chunk	+3 lines, -3 lines	0 comments	Download
M	third_party/WebKit/Source/web/NotificationPermissionClientImpl.cpp	View	1 2 3 4 5	1 chunk	+3 lines, -2 lines	0 comments	Download
M	third_party/WebKit/Source/web/PageOverlay.cpp	View	1	2 chunks	+2 lines, -2 lines	0 comments	Download
M	third_party/WebKit/Source/web/PopupMenuImpl.cpp	View	1 2 3 4 5	8 chunks	+11 lines, -11 lines	0 comments	Download
M	third_party/WebKit/Source/web/RemoteFrameOwner.cpp	View	1	1 chunk	+1 line, -1 line	0 comments	Download
M	third_party/WebKit/Source/web/RotationViewportAnchor.cpp	View	1	1 chunk	+2 lines, -2 lines	0 comments	Download
M	third_party/WebKit/Source/web/ServiceWorkerGlobalScopeProxy.cpp	View	1 2 3 4 5	6 chunks	+9 lines, -9 lines	0 comments	Download
M	third_party/WebKit/Source/web/SharedWorkerRepositoryClientImpl.cpp	View	1	2 chunks	+4 lines, -4 lines	0 comments	Download
M	third_party/WebKit/Source/web/StorageQuotaClientImpl.cpp	View	1	1 chunk	+1 line, -1 line	0 comments	Download
M	third_party/WebKit/Source/web/SuspendableScriptExecutor.cpp	View	1 2 3 4 5	1 chunk	+1 line, -1 line	0 comments	Download
M	third_party/WebKit/Source/web/TextFinder.h	View	1 2 3 4 5	1 chunk	+1 line, -1 line	0 comments	Download
M	third_party/WebKit/Source/web/TextFinder.cpp	View	1 2 3 4 5	5 chunks	+8 lines, -6 lines	0 comments	Download
M	third_party/WebKit/Source/web/ValidationMessageClientImpl.cpp	View	1 2 3 4 5	1 chunk	+1 line, -1 line	0 comments	Download
M	third_party/WebKit/Source/web/WebAXObject.cpp	View	1 2 3 4 7 8	3 chunks	+5 lines, -3 lines	0 comments	Download
M	third_party/WebKit/Source/web/WebArrayBufferConverter.cpp	View	1	1 chunk	+1 line, -1 line	0 comments	Download
M	third_party/WebKit/Source/web/WebBlob.cpp	View	1	2 chunks	+2 lines, -2 lines	0 comments	Download
M	third_party/WebKit/Source/web/WebCache.cpp	View	1	1 chunk	+1 line, -1 line	0 comments	Download
M	third_party/WebKit/Source/web/WebDOMActivityLogger.cpp	View	1	1 chunk	+1 line, -1 line	0 comments	Download
M	third_party/WebKit/Source/web/WebDOMFileSystem.cpp	View	1 2 3	6 chunks	+9 lines, -8 lines	0 comments	Download
M	third_party/WebKit/Source/web/WebDataSourceImpl.cpp	View	1 2 3 4 5	1 chunk	+1 line, -1 line	0 comments	Download
M	third_party/WebKit/Source/web/WebDevToolsAgentImpl.h	View	1 2 3 4 5	1 chunk	+1 line, -1 line	0 comments	Download
M	third_party/WebKit/Source/web/WebDevToolsAgentImpl.cpp	View	1 2 3 4 5 7 8	4 chunks	+10 lines, -8 lines	0 comments	Download
M	third_party/WebKit/Source/web/WebDevToolsFrontendImpl.cpp	View	1 2 3 4 5	2 chunks	+2 lines, -2 lines	0 comments	Download
M	third_party/WebKit/Source/web/WebDocument.cpp	View	1 2 3 4 5	1 chunk	+1 line, -1 line	0 comments	Download
M	third_party/WebKit/Source/web/WebElementTest.cpp	View	1 2 3 4 5	1 chunk	+1 line, -1 line	0 comments	Download
M	third_party/WebKit/Source/web/WebEmbeddedWorkerImpl.cpp	View	1 2 3 4 5	8 chunks	+18 lines, -18 lines	0 comments	Download
M	third_party/WebKit/Source/web/WebEntities.cpp	View	1	1 chunk	+1 line, -1 line	0 comments	Download
M	third_party/WebKit/Source/web/WebFrame.cpp	View	1 2 3 4 5	4 chunks	+4 lines, -4 lines	0 comments	Download
M	third_party/WebKit/Source/web/WebFrameSerializer.cpp	View	1	2 chunks	+3 lines, -3 lines	0 comments	Download
M	third_party/WebKit/Source/web/WebFrameSerializerImpl.cpp	View	1	3 chunks	+6 lines, -6 lines	0 comments	Download
M	third_party/WebKit/Source/web/WebFrameWidgetImpl.cpp	View	1 2 3 4 5	9 chunks	+10 lines, -10 lines	0 comments	Download
M	third_party/WebKit/Source/web/WebGeolocationController.cpp	View	1 2 3 4 5	1 chunk	+2 lines, -2 lines	0 comments	Download
M	third_party/WebKit/Source/web/WebGeolocationPermissionRequestManager.cpp	View	1	1 chunk	+1 line, -1 line	0 comments	Download
M	third_party/WebKit/Source/web/WebHeap.cpp	View		1 chunk	+1 line, -1 line	0 comments	Download
M	third_party/WebKit/Source/web/WebHelperPluginImpl.cpp	View	1 2 3 4 5	3 chunks	+5 lines, -5 lines	0 comments	Download
M	third_party/WebKit/Source/web/WebImageDecoder.cpp	View	1	2 chunks	+7 lines, -7 lines	0 comments	Download
M	third_party/WebKit/Source/web/WebInputEventConversion.cpp	View	1 2	1 chunk	+2 lines, -2 lines	0 comments	Download
M	third_party/WebKit/Source/web/WebKit.cpp	View	1	2 chunks	+2 lines, -2 lines	0 comments	Download
M	third_party/WebKit/Source/web/WebLeakDetector.cpp	View	1	2 chunks	+2 lines, -2 lines	0 comments	Download
M	third_party/WebKit/Source/web/WebLocalFrameImpl.cpp	View	1 2 3 4 5	39 chunks	+60 lines, -57 lines	0 comments	Download
M	third_party/WebKit/Source/web/WebMediaDevicesRequest.cpp	View	1 2 3	1 chunk	+4 lines, -3 lines	0 comments	Download
M	third_party/WebKit/Source/web/WebNode.cpp	View	1 2 3 4 5	2 chunks	+2 lines, -2 lines	0 comments	Download
M	third_party/WebKit/Source/web/WebPagePopupImpl.cpp	View	1 2 3 4 5	6 chunks	+10 lines, -10 lines	0 comments	Download
M	third_party/WebKit/Source/web/WebPluginContainerImpl.cpp	View	1 2 3 4 5	8 chunks	+8 lines, -8 lines	0 comments	Download
M	third_party/WebKit/Source/web/WebRemoteFrameImpl.cpp	View	1 2 3 4 5	4 chunks	+9 lines, -9 lines	0 comments	Download
M	third_party/WebKit/Source/web/WebScopedWindowFocusAllowedIndicator.cpp	View	1	1 chunk	+1 line, -1 line	0 comments	Download
M	third_party/WebKit/Source/web/WebSelection.cpp	View	1	1 chunk	+2 lines, -2 lines	0 comments	Download
M	third_party/WebKit/Source/web/WebSettingsImpl.cpp	View	1	1 chunk	+1 line, -1 line	0 comments	Download
M	third_party/WebKit/Source/web/WebSharedWorkerImpl.cpp	View	1 2 3 4 5	4 chunks	+6 lines, -6 lines	0 comments	Download
M	third_party/WebKit/Source/web/WebSpeechRecognitionResult.cpp	View	1	1 chunk	+1 line, -1 line	0 comments	Download
M	third_party/WebKit/Source/web/WebSurroundingText.cpp	View	1 2 3 4 5	1 chunk	+1 line, -1 line	0 comments	Download
M	third_party/WebKit/Source/web/WebUserMediaRequest.cpp	View	1 2 3	2 chunks	+11 lines, -10 lines	0 comments	Download
M	third_party/WebKit/Source/web/WebViewImpl.cpp	View	1 2 3 4 5	39 chunks	+57 lines, -56 lines	0 comments	Download
M	third_party/WebKit/Source/web/WorkerContentSettingsClient.cpp	View	1 2 3 4 5	2 chunks	+2 lines, -2 lines	0 comments	Download
M	third_party/WebKit/Source/web/mac/WebSubstringUtil.mm	View	1	1 chunk	+1 line, -1 line	0 comments	Download
M	third_party/WebKit/Source/web/tests/CompositorWorkerTest.cpp	View	1	1 chunk	+2 lines, -2 lines	0 comments	Download
M	third_party/WebKit/Source/web/tests/FrameTestHelpers.cpp	View	1	2 chunks	+2 lines, -2 lines	0 comments	Download
M	third_party/WebKit/Source/web/tests/PrerenderingTest.cpp	View	1	4 chunks	+8 lines, -8 lines	0 comments	Download
M	third_party/WebKit/Source/web/tests/ScreenWakeLockTest.cpp	View	1	2 chunks	+8 lines, -8 lines	0 comments	Download
M	third_party/WebKit/Source/web/tests/ScrollingCoordinatorTest.cpp	View	1	10 chunks	+11 lines, -11 lines	0 comments	Download
M	third_party/WebKit/Source/web/tests/VisualViewportTest.cpp	View	1 2 3 4 5	3 chunks	+4 lines, -4 lines	0 comments	Download
M	third_party/WebKit/Source/web/tests/WebDocumentTest.cpp	View	1	1 chunk	+1 line, -1 line	0 comments	Download
M	third_party/WebKit/Source/web/tests/WebFrameTest.cpp	View	1 2 3 4 5	3 chunks	+3 lines, -3 lines	0 comments	Download
M	third_party/WebKit/Source/web/tests/WebPluginContainerTest.cpp	View	1 2 3 4 5	21 chunks	+23 lines, -23 lines	0 comments	Download
M	third_party/WebKit/Source/web/tests/WebViewTest.cpp	View	1 2 3 4 5	5 chunks	+6 lines, -6 lines	0 comments	Download
M	third_party/WebKit/Source/web/tests/sim/SimCompositor.cpp	View	1	1 chunk	+3 lines, -3 lines	0 comments	Download
M	third_party/WebKit/Source/web/tests/sim/SimDisplayItemList.cpp	View		1 chunk	+1 line, -1 line	0 comments	Download
M	third_party/WebKit/Source/web/tests/sim/SimNetwork.cpp	View	1	3 chunks	+3 lines, -3 lines	0 comments	Download
M	third_party/WebKit/Source/web/tests/sim/SimRequest.cpp	View	1 2 3	2 chunks	+5 lines, -4 lines	0 comments	Download

Messages

Total messages: 21 (6 generated)

Expand Messages | Collapse Messages | Show Generated Messages | Hide Generated Messages

kotenkov

Continuing what I've started in https://codereview.chromium.org/1840163002/ . For convenience, I have split RELEASE_ASSERT and ASSERT ...

4 years, 8 months ago (2016-03-31 20:06:04 UTC) #2

tkent

> In some places I had to use DCHECK instead of DCHECK_*, because the latter ...

4 years, 8 months ago (2016-03-31 23:05:21 UTC) #3

kotenkov

On 2016/03/31 23:05:21, tkent wrote: > > In some places I had to use DCHECK ...

4 years, 8 months ago (2016-04-01 14:14:32 UTC) #4

tkent

lgtm https://codereview.chromium.org/1839643009/diff/100001/third_party/WebKit/Source/web/AudioOutputDeviceClientImpl.cpp File third_party/WebKit/Source/web/AudioOutputDeviceClientImpl.cpp (right): https://codereview.chromium.org/1839643009/diff/100001/third_party/WebKit/Source/web/AudioOutputDeviceClientImpl.cpp#newcode29 third_party/WebKit/Source/web/AudioOutputDeviceClientImpl.cpp:29: DCHECK(context && context->isDocument()); Please split this into two. ...

4 years, 8 months ago (2016-04-01 14:41:06 UTC) #5

commit-bot: I haz the power

CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1839643009/140001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1839643009/140001

4 years, 8 months ago (2016-04-01 15:20:22 UTC) #8

commit-bot: I haz the power

Try jobs failed on following builders: chromeos_daisy_chromium_compile_only_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_chromium_compile_only_ng/builds/115178)

4 years, 8 months ago (2016-04-01 15:34:35 UTC) #10

kotenkov

On 2016/04/01 15:34:35, commit-bot: I haz the power wrote: > Try jobs failed on following ...

4 years, 8 months ago (2016-04-01 15:48:44 UTC) #11

On 2016/04/01 15:34:35, commit-bot: I haz the power wrote:
> Try jobs failed on following builders:
>   chromeos_daisy_chromium_compile_only_ng on tryserver.chromium.linux
> (JOB_FAILED,
>
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...)

I'm having problems on
https://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy...
that are related to the DCHECK_IS_ON() removal.
There are the following errors:

../../third_party/WebKit/Source/web/WebAXObject.cpp: In member function
'blink::WebRect blink::WebAXObject::boundingBoxRect() const':
../../third_party/WebKit/Source/web/WebAXObject.cpp:635:50: error:
'isLayoutClean' was not declared in this scope
     DCHECK(isLayoutClean(m_private->getDocument()));
                                                  ^
../../base/logging.h:360:5: note: in definition of macro 'LAZY_STREAM'
   !(condition) ? (void) 0 : ::logging::LogMessageVoidify() & (stream)
     ^
../../third_party/WebKit/Source/web/WebAXObject.cpp:635:5: note: in expansion of
macro 'DCHECK'
     DCHECK(isLayoutClean(m_private->getDocument()));
     ^

../../third_party/WebKit/Source/web/WebDevToolsAgentImpl.cpp: In destructor
'virtual blink::WebDevToolsAgentImpl::~WebDevToolsAgentImpl()':
../../third_party/WebKit/Source/web/WebDevToolsAgentImpl.cpp:328:12: error:
'm_hasBeenDisposed' was not declared in this scope
     DCHECK(m_hasBeenDisposed);
            ^
../../base/logging.h:360:5: note: in definition of macro 'LAZY_STREAM'
   !(condition) ? (void) 0 : ::logging::LogMessageVoidify() & (stream)
     ^
../../third_party/WebKit/Source/web/WebDevToolsAgentImpl.cpp:328:5: note: in
expansion of macro 'DCHECK'
     DCHECK(m_hasBeenDisposed);

The DCHECK is preprocessed as if DCHECK_IS_ON() was true, but isLayoutClean()
and m_hasBeenDisposed are not declared, so DCHECK_IS_ON() seems to be false.
How is this possible and how can I fix it? (The obvious solution is to wrap
these DCHECKs in #if DCHECK_IS_ON(), but it's a bit strange).

kotenkov

On 2016/04/01 15:48:44, kotenkov wrote: > On 2016/04/01 15:34:35, commit-bot: I haz the power wrote: ...

4 years, 8 months ago (2016-04-01 15:51:39 UTC) #12

On 2016/04/01 15:48:44, kotenkov wrote:
> On 2016/04/01 15:34:35, commit-bot: I haz the power wrote:
> > Try jobs failed on following builders:
> >   chromeos_daisy_chromium_compile_only_ng on tryserver.chromium.linux
> > (JOB_FAILED,
> >
>
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...)
> 
> I'm having problems on
>
https://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy...
> that are related to the DCHECK_IS_ON() removal.
> There are the following errors:
> 
> ../../third_party/WebKit/Source/web/WebAXObject.cpp: In member function
> 'blink::WebRect blink::WebAXObject::boundingBoxRect() const':
> ../../third_party/WebKit/Source/web/WebAXObject.cpp:635:50: error:
> 'isLayoutClean' was not declared in this scope
>      DCHECK(isLayoutClean(m_private->getDocument()));
>                                                   ^
> ../../base/logging.h:360:5: note: in definition of macro 'LAZY_STREAM'
>    !(condition) ? (void) 0 : ::logging::LogMessageVoidify() & (stream)
>      ^
> ../../third_party/WebKit/Source/web/WebAXObject.cpp:635:5: note: in expansion
of
> macro 'DCHECK'
>      DCHECK(isLayoutClean(m_private->getDocument()));
>      ^
> 
> ../../third_party/WebKit/Source/web/WebDevToolsAgentImpl.cpp: In destructor
> 'virtual blink::WebDevToolsAgentImpl::~WebDevToolsAgentImpl()':
> ../../third_party/WebKit/Source/web/WebDevToolsAgentImpl.cpp:328:12: error:
> 'm_hasBeenDisposed' was not declared in this scope
>      DCHECK(m_hasBeenDisposed);
>             ^
> ../../base/logging.h:360:5: note: in definition of macro 'LAZY_STREAM'
>    !(condition) ? (void) 0 : ::logging::LogMessageVoidify() & (stream)
>      ^
> ../../third_party/WebKit/Source/web/WebDevToolsAgentImpl.cpp:328:5: note: in
> expansion of macro 'DCHECK'
>      DCHECK(m_hasBeenDisposed);
> 
> The DCHECK is preprocessed as if DCHECK_IS_ON() was true, but isLayoutClean()
> and m_hasBeenDisposed are not declared, so DCHECK_IS_ON() seems to be false.
> How is this possible and how can I fix it? (The obvious solution is to wrap
> these DCHECKs in #if DCHECK_IS_ON(), but it's a bit strange).

I was wrong, its the DCHECK. It requires its operands to exist. Here is the
preprocessor output for release build:

    !(0 ? !(m_hasBeenDisposed) : false) ? (void) 0 :
::logging::LogMessageVoidify() &
(logging::LogMessage("../../third_party/WebKit/Source/web/WebDevToolsAgentImpl.cpp",
328, logging::LOG_INFO).stream()) << "Check failed: " "m_hasBeenDisposed" ". ";

tkent

lgtm. You're right. We need to wrap DCHECK(condition) with DCHECK_IS_ON() if |condition| refers to symbols ...

4 years, 8 months ago (2016-04-01 16:19:33 UTC) #14

kotenkov

There is another problem. There seems to be a regression after previous CL in which ...

4 years, 8 months ago (2016-04-01 16:32:10 UTC) #15

tkent

web/ layer is not performance sensitive. You may land this CL as is.

4 years, 8 months ago (2016-04-01 16:46:18 UTC) #16

commit-bot: I haz the power

CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1839643009/160001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1839643009/160001

4 years, 8 months ago (2016-04-01 16:47:00 UTC) #18

commit-bot: I haz the power

4 years, 8 months ago (2016-04-01 17:58:04 UTC) #21

Message was sent while issue was closed.

Patchset 9 (id:??) landed as
https://crrev.com/151deeb377f9456660e9ab835d1c7fa46edc7499
Cr-Commit-Position: refs/heads/master@{#384626}

Expand Messages | Collapse Messages | Show Generated Messages | Hide Generated Messages