Fix implicit access to raw pointer of scoped_refptr.

Fix implicit access to raw pointer of scoped_refptr.

In following patch https://codereview.chromium.org/1963323004/
scoped_refptr will have the previous flawed "Safe Bool Idiom"
implementation removed.

This feature was incorrectly used throughout the codebase
with code implicitly accessing the raw pointer from
scoped_refptr instances. A correct "Safe Bool Idiom" would
have prevented that, but the point is moot as we can now
move to Cxx11 "explicit operator bool()".

Usage has been corrected in two ways:
+ Logic tests no longer compare pointer values, but use a
  boolean expression (typically just the variable name).
+ Casts to boolean types are now explicit with stati_cast<bool>().

BUG=610048
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel

Committed: https://crrev.com/4dac7f0d4c5050a5a827c2a1a5cd581afe1ae733
Cr-Commit-Position: refs/heads/master@{#393125}

[scheib, 2016-05-07 05:17:19]

Description was changed from

==========
Use explicit operator bool in scoped_refptr.

Previously the "Safe Bool Idiom" was implemented using a typedef ...
Testable work around. The C++11 explicit conversion operator feature
is now available and this hack is no longer required.

Usage has been corrected in two ways:
+ Logic tests no longer compare pointer values, but use a boolean expression
(typically just the variable name).
+ Casts to boolean types are now explicit with stati_cast<bool>().

BUG=610048
==========

to

==========
Use explicit operator bool in scoped_refptr.

Previously the "Safe Bool Idiom" was implemented using a typedef ...
Testable work around. The C++11 explicit conversion operator feature
is now available and this hack is no longer required.

Usage has been corrected in two ways:
+ Logic tests no longer compare pointer values, but use a boolean expression
(typically just the variable name).
+ Casts to boolean types are now explicit with stati_cast<bool>().

BUG=610048
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

[danakj, 2016-05-10 21:37:01]

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

[danakj, 2016-05-10 21:37:02]

Double check your NE/EQ stuffs.

https://codereview.chromium.org/1958823002/diff/60001/chrome/browser/themes/t...
File chrome/browser/themes/theme_service.cc (right):

https://codereview.chromium.org/1958823002/diff/60001/chrome/browser/themes/t...
chrome/browser/themes/theme_service.cc:571: if (theme_supplier_)
i'm meh about this change

https://codereview.chromium.org/1958823002/diff/60001/chrome/browser/ui/views...
File chrome/browser/ui/views/certificate_selector_browsertest.cc (right):

https://codereview.chromium.org/1958823002/diff/60001/chrome/browser/ui/views...
chrome/browser/ui/views/certificate_selector_browsertest.cc:77:
ASSERT_FALSE(client_1_);
TRUE

https://codereview.chromium.org/1958823002/diff/60001/chrome/browser/ui/views...
chrome/browser/ui/views/certificate_selector_browsertest.cc:81:
ASSERT_FALSE(client_2_);
TRUE

https://codereview.chromium.org/1958823002/diff/60001/components/tracing/chil...
File components/tracing/child_memory_dump_manager_delegate_impl.cc (right):

https://codereview.chromium.org/1958823002/diff/60001/components/tracing/chil...
components/tracing/child_memory_dump_manager_delegate_impl.cc:43: DCHECK(ctmf_
== nullptr || (ctmf == nullptr && ctmf_task_runner_));
you could write this

DCHECK(!ctmf_ || (!ctmf && ctmf_task_runner_))

if you're trying to remove "== nullptr" stuff. Or you could leave it.

https://codereview.chromium.org/1958823002/diff/60001/device/bluetooth/bluez/...
File device/bluetooth/bluez/bluetooth_advertisement_bluez_unittest.cc (right):

https://codereview.chromium.org/1958823002/diff/60001/device/bluetooth/bluez/...
device/bluetooth/bluez/bluetooth_advertisement_bluez_unittest.cc:223:
EXPECT_FALSE(advertisement);
TRUE

https://codereview.chromium.org/1958823002/diff/60001/ppapi/tests/ppapi_nacl_...
File ppapi/tests/ppapi_nacl_tests_newlib.nmf (right):

https://codereview.chromium.org/1958823002/diff/60001/ppapi/tests/ppapi_nacl_...
ppapi/tests/ppapi_nacl_tests_newlib.nmf:4: "x86-64": {
This looks like an unrelated change?

[danakj, 2016-05-10 21:37:46]

https://codereview.chromium.org/1958823002/diff/60001/base/memory/ref_counted.h
File base/memory/ref_counted.h (right):

https://codereview.chromium.org/1958823002/diff/60001/base/memory/ref_counted...
base/memory/ref_counted.h:367: private:
There's no reason to make a private block for this friend line.

[danakj, 2016-05-10 21:38:52]

https://codereview.chromium.org/1958823002/diff/60001/base/memory/ref_counted.h
File base/memory/ref_counted.h (right):

https://codereview.chromium.org/1958823002/diff/60001/base/memory/ref_counted...
base/memory/ref_counted.h:367: private:
On 2016/05/10 21:37:46, danakj wrote:
> There's no reason to make a private block for this friend line.

(I'm also wondering why it's here, we're calling .get() which is public on the
<U> friend.)

[scheib, 2016-05-10 22:23:41]

Patchset #2 (id:20001) has been deleted

[scheib, 2016-05-10 22:23:53]

Patchset #1 (id:1) has been deleted

[scheib, 2016-05-10 22:24:07]

Patchset #1 (id:40001) has been deleted

[scheib, 2016-05-10 22:24:20]

Patchset #2 (id:80001) has been deleted

[scheib, 2016-05-10 22:25:00]

Thanks:

https://codereview.chromium.org/1958823002/diff/60001/base/memory/ref_counted.h
File base/memory/ref_counted.h (right):

https://codereview.chromium.org/1958823002/diff/60001/base/memory/ref_counted...
base/memory/ref_counted.h:367: private:
On 2016/05/10 21:38:51, danakj wrote:
> On 2016/05/10 21:37:46, danakj wrote:
> > There's no reason to make a private block for this friend line.
> 
> (I'm also wondering why it's here, we're calling .get() which is public on the
> <U> friend.)

Done, moved friend to bottom private section and commented why it is needed
(move constructor).

https://codereview.chromium.org/1958823002/diff/60001/chrome/browser/themes/t...
File chrome/browser/themes/theme_service.cc (right):

https://codereview.chromium.org/1958823002/diff/60001/chrome/browser/themes/t...
chrome/browser/themes/theme_service.cc:571: if (theme_supplier_)
On 2016/05/10 21:37:02, danakj wrote:
> i'm meh about this change

Ok. I considered also:
- a static_cast here: too verbose and distracting.
- dropping the 'loaded_pack' variable entirely: but lost the semantic meaning
original author intended of why theme_supplier implies themes are loaded.

https://codereview.chromium.org/1958823002/diff/60001/chrome/browser/ui/views...
File chrome/browser/ui/views/certificate_selector_browsertest.cc (right):

https://codereview.chromium.org/1958823002/diff/60001/chrome/browser/ui/views...
chrome/browser/ui/views/certificate_selector_browsertest.cc:77:
ASSERT_FALSE(client_1_);
On 2016/05/10 21:37:02, danakj wrote:
> TRUE

Done.

https://codereview.chromium.org/1958823002/diff/60001/chrome/browser/ui/views...
chrome/browser/ui/views/certificate_selector_browsertest.cc:81:
ASSERT_FALSE(client_2_);
On 2016/05/10 21:37:02, danakj wrote:
> TRUE

Done.

https://codereview.chromium.org/1958823002/diff/60001/components/tracing/chil...
File components/tracing/child_memory_dump_manager_delegate_impl.cc (right):

https://codereview.chromium.org/1958823002/diff/60001/components/tracing/chil...
components/tracing/child_memory_dump_manager_delegate_impl.cc:43: DCHECK(ctmf_
== nullptr || (ctmf == nullptr && ctmf_task_runner_));
On 2016/05/10 21:37:02, danakj wrote:
> you could write this
> 
> DCHECK(!ctmf_ || (!ctmf && ctmf_task_runner_))
> 
> if you're trying to remove "== nullptr" stuff. Or you could leave it.

Done.

https://codereview.chromium.org/1958823002/diff/60001/device/bluetooth/bluez/...
File device/bluetooth/bluez/bluetooth_advertisement_bluez_unittest.cc (right):

https://codereview.chromium.org/1958823002/diff/60001/device/bluetooth/bluez/...
device/bluetooth/bluez/bluetooth_advertisement_bluez_unittest.cc:223:
EXPECT_FALSE(advertisement);
On 2016/05/10 21:37:02, danakj wrote:
> TRUE

Done.

https://codereview.chromium.org/1958823002/diff/60001/ppapi/tests/ppapi_nacl_...
File ppapi/tests/ppapi_nacl_tests_newlib.nmf (right):

https://codereview.chromium.org/1958823002/diff/60001/ppapi/tests/ppapi_nacl_...
ppapi/tests/ppapi_nacl_tests_newlib.nmf:4: "x86-64": {
On 2016/05/10 21:37:02, danakj wrote:
> This looks like an unrelated change?

Done.

[scheib, 2016-05-10 23:51:45]

Also, I'm pondering if I should split this patch into the fixes and then the
core change to scoped_refptr. That would hedge against a possible need to revert
the scoped_refptr change, making that revert patch much smaller and less likely
to have conflict. Thoughts?

[danakj, 2016-05-11 00:46:04]

On Tue, May 10, 2016 at 4:51 PM, <scheib@chromium.org> wrote:

> Also, I'm pondering if I should split this patch into the fixes and then
> the
> core change to scoped_refptr. That would hedge against a possible need to
> revert
> the scoped_refptr change, making that revert patch much smaller and less
> likely
> to have conflict. Thoughts?
>

I think that is a good idea.

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[scheib, 2016-05-11 01:17:56]

Description was changed from

==========
Use explicit operator bool in scoped_refptr.

Previously the "Safe Bool Idiom" was implemented using a typedef ...
Testable work around. The C++11 explicit conversion operator feature
is now available and this hack is no longer required.

Usage has been corrected in two ways:
+ Logic tests no longer compare pointer values, but use a boolean expression
(typically just the variable name).
+ Casts to boolean types are now explicit with stati_cast<bool>().

BUG=610048
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

to

==========
Fix implicit access to raw pointer of scoped_refptr.

In following patch https://codereview.chromium.org/1963323004/
scoped_refptr will have the previous flawed "Safe Bool Idiom"
implementation removed.

This feature was incorrectly used throughout the codebase
with code implicitly accessing the raw pointer from
scoped_refptr instances. A correct "Safe Bool Idiom" would
have prevented that, but the point is moot as we can now
move to Cxx11 "explicit operator bool()".

Usage has been corrected in two ways:
+ Logic tests no longer compare pointer values, but use a
  boolean expression (typically just the variable name).
+ Casts to boolean types are now explicit with stati_cast<bool>().

BUG=610048
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

[scheib, 2016-05-11 01:32:28]

Ok, split out. 

Also, is this a TBR situation or should I add more OWNERS?

[danakj, 2016-05-11 20:01:02]

LGTM.

This is what src/OWNERS is for. You can ask one of them to review or ask them to
TBR.

[scheib, 2016-05-11 21:09:35]

scheib@chromium.org changed reviewers:
+ ben@chromium.org

[scheib, 2016-05-11 21:09:36]

ben, OWNERS please.

[Ben Goodger (Google), 2016-05-11 22:51:27]

took a quick sampling... lgtm

[scheib, 2016-05-11 22:55:22]

The CQ bit was checked by scheib@chromium.org

[commit-bot: I haz the power, 2016-05-11 22:56:08]

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

[commit-bot: I haz the power, 2016-05-12 00:55:14]

Description was changed from

==========
Fix implicit access to raw pointer of scoped_refptr.

In following patch https://codereview.chromium.org/1963323004/
scoped_refptr will have the previous flawed "Safe Bool Idiom"
implementation removed.

This feature was incorrectly used throughout the codebase
with code implicitly accessing the raw pointer from
scoped_refptr instances. A correct "Safe Bool Idiom" would
have prevented that, but the point is moot as we can now
move to Cxx11 "explicit operator bool()".

Usage has been corrected in two ways:
+ Logic tests no longer compare pointer values, but use a
  boolean expression (typically just the variable name).
+ Casts to boolean types are now explicit with stati_cast<bool>().

BUG=610048
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

to

==========
Fix implicit access to raw pointer of scoped_refptr.

In following patch https://codereview.chromium.org/1963323004/
scoped_refptr will have the previous flawed "Safe Bool Idiom"
implementation removed.

This feature was incorrectly used throughout the codebase
with code implicitly accessing the raw pointer from
scoped_refptr instances. A correct "Safe Bool Idiom" would
have prevented that, but the point is moot as we can now
move to Cxx11 "explicit operator bool()".

Usage has been corrected in two ways:
+ Logic tests no longer compare pointer values, but use a
  boolean expression (typically just the variable name).
+ Casts to boolean types are now explicit with stati_cast<bool>().

BUG=610048
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

[commit-bot: I haz the power, 2016-05-12 00:55:16]

Committed patchset #3 (id:120001)

[commit-bot: I haz the power, 2016-05-12 00:56:31]

Description was changed from

==========
Fix implicit access to raw pointer of scoped_refptr.

In following patch https://codereview.chromium.org/1963323004/
scoped_refptr will have the previous flawed "Safe Bool Idiom"
implementation removed.

This feature was incorrectly used throughout the codebase
with code implicitly accessing the raw pointer from
scoped_refptr instances. A correct "Safe Bool Idiom" would
have prevented that, but the point is moot as we can now
move to Cxx11 "explicit operator bool()".

Usage has been corrected in two ways:
+ Logic tests no longer compare pointer values, but use a
  boolean expression (typically just the variable name).
+ Casts to boolean types are now explicit with stati_cast<bool>().

BUG=610048
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

to

==========
Fix implicit access to raw pointer of scoped_refptr.

In following patch https://codereview.chromium.org/1963323004/
scoped_refptr will have the previous flawed "Safe Bool Idiom"
implementation removed.

This feature was incorrectly used throughout the codebase
with code implicitly accessing the raw pointer from
scoped_refptr instances. A correct "Safe Bool Idiom" would
have prevented that, but the point is moot as we can now
move to Cxx11 "explicit operator bool()".

Usage has been corrected in two ways:
+ Logic tests no longer compare pointer values, but use a
  boolean expression (typically just the variable name).
+ Casts to boolean types are now explicit with stati_cast<bool>().

BUG=610048
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel

Committed: https://crrev.com/4dac7f0d4c5050a5a827c2a1a5cd581afe1ae733
Cr-Commit-Position: refs/heads/master@{#393125}
==========

[commit-bot: I haz the power, 2016-05-12 00:56:34]

Patchset 3 (id:??) landed as
https://crrev.com/4dac7f0d4c5050a5a827c2a1a5cd581afe1ae733
Cr-Commit-Position: refs/heads/master@{#393125}