Remove BLINK_ASSERT() and BLINK_ASSERT_NOT_REACHED() macros.

Remove BLINK_ASSERT() and BLINK_ASSERT_NOT_REACHED() macros.

BUG=596760
Committed: https://crrev.com/7104e93bb1a32ba3cd9e1d798d02a1f54cf0c0ff
Cr-Commit-Position: refs/heads/master@{#386010}

[kotenkov, 2016-04-03 19:18:15]

kotenkov@yandex-team.ru changed reviewers:
+ tkent@chromium.org

[kotenkov, 2016-04-03 19:18:16]

There is no info on BLINK_ASSERT* here,
https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/TL0NkNXIT1w ,
but I assume that we can replace them with DCHECKS. Am I right?

Is it correct to add a dependency on base/logging.h in public/platform?

[tkent, 2016-04-03 22:54:48]

tkent@chromium.org changed reviewers:
+ esprehn@chromium.org

[tkent, 2016-04-03 22:59:06]

> There is no info on BLINK_ASSERT* here,
https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/TL0NkNXIT1w ,
but I assume that we can replace them with DCHECKS. Am I right?

Yes.

However, I think we can *remove* all of instances of BLINK_ASSERT_NOT_REACHED().
 The functions with it should be pure virtual.

https://codereview.chromium.org/1851293002/diff/1/third_party/WebKit/public/p...
File third_party/WebKit/public/platform/WebCommon.h (left):

https://codereview.chromium.org/1851293002/diff/1/third_party/WebKit/public/p...
third_party/WebKit/public/platform/WebCommon.h:99: BLINK_COMMON_EXPORT void
failedAssertion(const char* file, int line, const char* function, const char*
assertion);
We should remove failedAssertion() implementation in
platform/exported/WebCommon.cpp.

https://codereview.chromium.org/1851293002/diff/1/third_party/WebKit/public/p...
File third_party/WebKit/public/platform/WebCommon.h (right):

https://codereview.chromium.org/1851293002/diff/1/third_party/WebKit/public/p...
third_party/WebKit/public/platform/WebCommon.h:78: #include "base/logging.h"
WebCommon.h shouldn't include base/logging.h.  Each of files using DCHECK should
include base/logging.h.

[haraken, 2016-04-03 23:53:46]

haraken@chromium.org changed reviewers:
+ haraken@chromium.org

[haraken, 2016-04-03 23:53:47]

https://codereview.chromium.org/1851293002/diff/1/third_party/WebKit/public/p...
File third_party/WebKit/public/platform/WebCommon.h (right):

https://codereview.chromium.org/1851293002/diff/1/third_party/WebKit/public/p...
third_party/WebKit/public/platform/WebCommon.h:78: #include "base/logging.h"
On 2016/04/03 22:59:06, tkent wrote:
> WebCommon.h shouldn't include base/logging.h.  Each of files using DCHECK
should
> include base/logging.h.

This is a tricky part since core/, modules/ and web/ are not allowed to include
base/. But we want to allow core/, modules/ and web/ to use macros defined in
Chromium.

As far as I understand, our plan is:

- For normal classes, define a thin wrapper in wtf/ or platform/ to be used by
core/, modules/ and web/
- For macros, allow core/, modules/ and web/ to use them directly.
- core/, modules/ and web/ cannot include base/ anyway.

To meet the requirements, maybe do we need to introduce wtf/macros.h which
includes base/logging.h etc? Or would there be any better way?

[kotenkov, 2016-04-04 07:46:34]

https://codereview.chromium.org/1851293002/diff/1/third_party/WebKit/public/p...
File third_party/WebKit/public/platform/WebCommon.h (right):

https://codereview.chromium.org/1851293002/diff/1/third_party/WebKit/public/p...
third_party/WebKit/public/platform/WebCommon.h:78: #include "base/logging.h"
> To meet the requirements, maybe do we need to introduce wtf/macros.h which
> includes base/logging.h etc? Or would there be any better way?

There already exists such a file: wtf/Assertions.h. The problem is that we
cannot include wtf from outside of BLINK_IMPLEMENTATION.

[tkent, 2016-04-04 09:03:17]

https://codereview.chromium.org/1851293002/diff/1/third_party/WebKit/public/p...
File third_party/WebKit/public/platform/WebCommon.h (right):

https://codereview.chromium.org/1851293002/diff/1/third_party/WebKit/public/p...
third_party/WebKit/public/platform/WebCommon.h:78: #include "base/logging.h"
On 2016/04/04 at 07:46:34, kotenkov wrote:
> > To meet the requirements, maybe do we need to introduce wtf/macros.h which
> > includes base/logging.h etc? Or would there be any better way?
> 
> There already exists such a file: wtf/Assertions.h. The problem is that we
cannot include wtf from outside of BLINK_IMPLEMENTATION.

Right.  So, I think using base/logging.h directly in public/platform is a
reasonable exception.

[esprehn, 2016-04-04 21:42:26]

This still has NOTREACHED() for lots of things that should be pure virtual I
think.

[kotenkov, 2016-04-05 05:47:46]

On 2016/04/04 21:42:26, esprehn wrote:
> This still has NOTREACHED() for lots of things that should be pure virtual I
> think.

That's because the CL is not ready yet. There are NOTREACHED in
third_party/WebKit/public/platform/WebBlobRegistry.h
third_party/WebKit/public/web/WebFrameClient.h
third_party/WebKit/public/web/modules/serviceworker/WebServiceWorkerContextClient.h
To fix them, I need to change some code outside of blink, because some methods
are not implemented in the descendants. I'll do it today.

[kotenkov, 2016-04-06 05:37:50]

On 2016/04/05 05:47:46, kotenkov wrote:
> On 2016/04/04 21:42:26, esprehn wrote:
> > This still has NOTREACHED() for lots of things that should be pure virtual I
> > think.
> 
> That's because the CL is not ready yet. There are NOTREACHED in
> third_party/WebKit/public/platform/WebBlobRegistry.h
> third_party/WebKit/public/web/WebFrameClient.h
>
third_party/WebKit/public/web/modules/serviceworker/WebServiceWorkerContextClient.h
> To fix them, I need to change some code outside of blink, because some methods
> are not implemented in the descendants. I'll do it today.

It's done. PTAL again.

[haraken, 2016-04-06 05:39:16]

On 2016/04/06 05:37:50, kotenkov wrote:
> On 2016/04/05 05:47:46, kotenkov wrote:
> > On 2016/04/04 21:42:26, esprehn wrote:
> > > This still has NOTREACHED() for lots of things that should be pure virtual
I
> > > think.
> > 
> > That's because the CL is not ready yet. There are NOTREACHED in
> > third_party/WebKit/public/platform/WebBlobRegistry.h
> > third_party/WebKit/public/web/WebFrameClient.h
> >
>
third_party/WebKit/public/web/modules/serviceworker/WebServiceWorkerContextClient.h
> > To fix them, I need to change some code outside of blink, because some
methods
> > are not implemented in the descendants. I'll do it today.
> 
> It's done. PTAL again.

This looks good, but we might want to fix the performance regression of CHECK
before making further replacements.

[kotenkov, 2016-04-06 06:29:54]

On 2016/04/06 05:39:16, haraken wrote:
> On 2016/04/06 05:37:50, kotenkov wrote:
> > On 2016/04/05 05:47:46, kotenkov wrote:
> > > On 2016/04/04 21:42:26, esprehn wrote:
> > > > This still has NOTREACHED() for lots of things that should be pure
virtual
> I
> > > > think.
> > > 
> > > That's because the CL is not ready yet. There are NOTREACHED in
> > > third_party/WebKit/public/platform/WebBlobRegistry.h
> > > third_party/WebKit/public/web/WebFrameClient.h
> > >
> >
>
third_party/WebKit/public/web/modules/serviceworker/WebServiceWorkerContextClient.h
> > > To fix them, I need to change some code outside of blink, because some
> methods
> > > are not implemented in the descendants. I'll do it today.
> > 
> > It's done. PTAL again.
> 
> This looks good, but we might want to fix the performance regression of CHECK
> before making further replacements.

Actually, I don't think that this CL is related, because it is the ASSERT ->
DCHECK conversion here, and they have similar performance, according to my
measurements.
I can run some perf try jobs if you tell me which ones would be appropriate.
But of course we can wait, just to not create the noise.

[haraken, 2016-04-06 06:38:18]

LGTM

[kotenkov, 2016-04-06 07:10:48]

kotenkov@yandex-team.ru changed reviewers:
+ caitkp@chromium.org, liberato@chromium.org, sievers@chromium.org

[kotenkov, 2016-04-06 07:10:49]

caitkp@chromium.org: Please review changes in components

liberato@chromium.org: Please review changes in media

sievers@chromium.org: Please review changes in content

[liberato (no reviews please), 2016-04-06 14:39:44]

https://codereview.chromium.org/1851293002/diff/120001/third_party/WebKit/pub...
File third_party/WebKit/public/web/WebFrameClient.h (right):

https://codereview.chromium.org/1851293002/diff/120001/third_party/WebKit/pub...
third_party/WebKit/public/web/WebFrameClient.h:724: virtual void
checkIfAudioSinkExistsAndIsAuthorized(const WebString& sinkId, const
WebSecurityOrigin&, WebSetSinkIdCallbacks*) = 0;
everything else in the default implementation returns something similar to "no".
 why was this an assert rather than cb->OnError(NotSupported)?

is it because of the "takes ownership" part?  could one just delete the cb
before returning?

[no sievers, 2016-04-06 17:26:28]

content lgtm

https://codereview.chromium.org/1851293002/diff/120001/content/test/mock_webf...
File content/test/mock_webframeclient.h (right):

https://codereview.chromium.org/1851293002/diff/120001/content/test/mock_webf...
content/test/mock_webframeclient.h:12: class MockWebFrameClient : public
blink::WebFrameClient {
nit: 'public:'

[Cait (Slow), 2016-04-06 17:28:22]

components/ lgtm

[liberato (no reviews please), 2016-04-06 18:04:42]

On 2016/04/06 17:28:22, Cait wrote:
> components/ lgtm

media/ lgtm assuming that the answer to my previous question is "because we
don't like that idea".  :)

-fl

[kotenkov, 2016-04-06 18:07:57]

On 2016/04/06 18:04:42, liberato wrote:
> On 2016/04/06 17:28:22, Cait wrote:
> > components/ lgtm
> 
> media/ lgtm assuming that the answer to my previous question is "because we
> don't like that idea".  :)
> 
> -fl

Actually, I think it's a great idea, I've already made the patch and waiting for
local compilation now.

[kotenkov, 2016-04-06 19:29:45]

On 2016/04/06 18:07:57, kotenkov wrote:
> On 2016/04/06 18:04:42, liberato wrote:
> > On 2016/04/06 17:28:22, Cait wrote:
> > > components/ lgtm
> > 
> > media/ lgtm assuming that the answer to my previous question is "because we
> > don't like that idea".  :)
> > 
> > -fl
> 
> Actually, I think it's a great idea, I've already made the patch and waiting
for
> local compilation now.

I've implemented liberato's suggestion, it helped to make the patch smaller.
Need approval for blink code again, PTAL.

[esprehn, 2016-04-06 20:02:02]

https://codereview.chromium.org/1851293002/diff/140001/third_party/WebKit/pub...
File third_party/WebKit/public/web/WebFrameClient.h (right):

https://codereview.chromium.org/1851293002/diff/140001/third_party/WebKit/pub...
third_party/WebKit/public/web/WebFrameClient.h:728:
callback->onError(WebSetSinkIdError::NotSupported);
this doesn't really do anything I think?

You don't use the callbacks argument, you access a nullptr on the stack?

I think you wanted to do callbacks-> here? Dunno what the unique_ptr is for.

[liberato (no reviews please), 2016-04-06 21:02:34]

https://codereview.chromium.org/1851293002/diff/140001/third_party/WebKit/pub...
File third_party/WebKit/public/web/WebFrameClient.h (right):

https://codereview.chromium.org/1851293002/diff/140001/third_party/WebKit/pub...
third_party/WebKit/public/web/WebFrameClient.h:728:
callback->onError(WebSetSinkIdError::NotSupported);
On 2016/04/06 20:02:02, esprehn wrote:
> this doesn't really do anything I think?
> 
> You don't use the callbacks argument, you access a nullptr on the stack?
> 
> I think you wanted to do callbacks-> here? Dunno what the unique_ptr is for.

i think that "std::unique... callback(callbacks)" was intended, to delete
|callbacks| on return.

putting in an explicit delete might be clearer in this case.

[kotenkov, 2016-04-07 06:03:28]

https://codereview.chromium.org/1851293002/diff/120001/third_party/WebKit/pub...
File third_party/WebKit/public/web/WebFrameClient.h (right):

https://codereview.chromium.org/1851293002/diff/120001/third_party/WebKit/pub...
third_party/WebKit/public/web/WebFrameClient.h:724: virtual void
checkIfAudioSinkExistsAndIsAuthorized(const WebString& sinkId, const
WebSecurityOrigin&, WebSetSinkIdCallbacks*) = 0;
On 2016/04/06 14:39:43, liberato wrote:
> is it because of the "takes ownership" part?

Yes, this is the precise reason:
https://codereview.chromium.org/1416123005/diff2/80001:100001/third_party/Web...

> could one just delete the cb before returning?
I guess, nobody had thought about it before you suggested it. =)

https://codereview.chromium.org/1851293002/diff/140001/third_party/WebKit/pub...
File third_party/WebKit/public/web/WebFrameClient.h (right):

https://codereview.chromium.org/1851293002/diff/140001/third_party/WebKit/pub...
third_party/WebKit/public/web/WebFrameClient.h:728:
callback->onError(WebSetSinkIdError::NotSupported);
On 2016/04/06 21:02:34, liberato wrote:
> On 2016/04/06 20:02:02, esprehn wrote:
> > this doesn't really do anything I think?
> > 
> > You don't use the callbacks argument, you access a nullptr on the stack?
> > 
> > I think you wanted to do callbacks-> here? Dunno what the unique_ptr is for.
> 
> i think that "std::unique... callback(callbacks)" was intended, to delete
> |callbacks| on return.
> 
> putting in an explicit delete might be clearer in this case.

You are correct. Okay, I've changed it to explicit delete.

[tkent, 2016-04-07 23:51:05]

third_party/WebKit lgtm

[kotenkov, 2016-04-08 05:28:36]

The CQ bit was checked by kotenkov@yandex-team.ru

[kotenkov, 2016-04-08 05:28:36]

The patchset sent to the CQ was uploaded after l-g-t-m from
liberato@chromium.org, caitkp@chromium.org, haraken@chromium.org,
sievers@chromium.org
Link to the patchset: https://codereview.chromium.org/1851293002/#ps160001
(title: "Check the |callbacks| for nullptr and use explicit delete.")

[commit-bot: I haz the power, 2016-04-08 05:28:49]

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

[commit-bot: I haz the power, 2016-04-08 06:52:21]

Committed patchset #9 (id:160001)

[commit-bot: I haz the power, 2016-04-08 06:53:26]

Description was changed from

==========
Remove BLINK_ASSERT() and BLINK_ASSERT_NOT_REACHED() macros.

BUG=596760
==========

to

==========
Remove BLINK_ASSERT() and BLINK_ASSERT_NOT_REACHED() macros.

BUG=596760

Committed: https://crrev.com/7104e93bb1a32ba3cd9e1d798d02a1f54cf0c0ff
Cr-Commit-Position: refs/heads/master@{#386010}
==========

[commit-bot: I haz the power, 2016-04-08 06:53:27]

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