Initialize observer_ to nullptr in DevicePermissionsDialogView.

Initialize observer_ to nullptr in DevicePermissionsDialogView.

I noticed while doing some refactoring elsewhere that observer_ was not
being initialized and so early calls to OnDevicesChanged could branch on
an uninitialized value and crash.

Committed: https://crrev.com/b61a9fd6fe1a1a61733b0f08e9f1a209c557ec9d
Cr-Commit-Position: refs/heads/master@{#324338}

[Reilly Grant (use Gerrit), 2015-04-08 22:24:29]

reillyg@chromium.org changed reviewers:
+ benwells@chromium.org

[Reilly Grant (use Gerrit), 2015-04-08 22:24:29]

Trivial patch. Please take a look.

[benwells, 2015-04-08 22:53:22]

https://codereview.chromium.org/1075593003/diff/1/chrome/browser/ui/views/ext...
File chrome/browser/ui/views/extensions/device_permissions_dialog_view.cc
(right):

https://codereview.chromium.org/1075593003/diff/1/chrome/browser/ui/views/ext...
chrome/browser/ui/views/extensions/device_permissions_dialog_view.cc:47:
ui::TableModelObserver* observer_ = nullptr;
since you already have an initializer list in the constructor, why not do it
there, the old fashioned way?

[Reilly Grant (use Gerrit), 2015-04-08 22:57:25]

https://codereview.chromium.org/1075593003/diff/1/chrome/browser/ui/views/ext...
File chrome/browser/ui/views/extensions/device_permissions_dialog_view.cc
(right):

https://codereview.chromium.org/1075593003/diff/1/chrome/browser/ui/views/ext...
chrome/browser/ui/views/extensions/device_permissions_dialog_view.cc:47:
ui::TableModelObserver* observer_ = nullptr;
On 2015/04/08 22:53:22, benwells wrote:
> since you already have an initializer list in the constructor, why not do it
> there, the old fashioned way?

This method guarantees that any future constructor will also inherit this
default behavior. Disregarding the initializer list in the constructor this
feels equivalent to the way that the scoped_refptr above would be initialized to
nullptr because that's what its default constructor does. This is simply working
around the lack of a default constructor for raw pointer types.

[benwells, 2015-04-08 23:44:29]

https://codereview.chromium.org/1075593003/diff/1/chrome/browser/ui/views/ext...
File chrome/browser/ui/views/extensions/device_permissions_dialog_view.cc
(right):

https://codereview.chromium.org/1075593003/diff/1/chrome/browser/ui/views/ext...
chrome/browser/ui/views/extensions/device_permissions_dialog_view.cc:47:
ui::TableModelObserver* observer_ = nullptr;
On 2015/04/08 22:57:25, reillyg wrote:
> On 2015/04/08 22:53:22, benwells wrote:
> > since you already have an initializer list in the constructor, why not do it
> > there, the old fashioned way?
> 
> This method guarantees that any future constructor will also inherit this
> default behavior. Disregarding the initializer list in the constructor this
> feels equivalent to the way that the scoped_refptr above would be initialized
to
> nullptr because that's what its default constructor does. This is simply
working
> around the lack of a default constructor for raw pointer types.

Yes, I understand that this will be caught by all future constructors.

Can you see that it might be confusing for people reading the code?

[Reilly Grant (use Gerrit), 2015-04-09 00:06:36]

https://codereview.chromium.org/1075593003/diff/1/chrome/browser/ui/views/ext...
File chrome/browser/ui/views/extensions/device_permissions_dialog_view.cc
(right):

https://codereview.chromium.org/1075593003/diff/1/chrome/browser/ui/views/ext...
chrome/browser/ui/views/extensions/device_permissions_dialog_view.cc:47:
ui::TableModelObserver* observer_ = nullptr;
On 2015/04/08 23:44:29, benwells wrote:
> On 2015/04/08 22:57:25, reillyg wrote:
> > On 2015/04/08 22:53:22, benwells wrote:
> > > since you already have an initializer list in the constructor, why not do
it
> > > there, the old fashioned way?
> > 
> > This method guarantees that any future constructor will also inherit this
> > default behavior. Disregarding the initializer list in the constructor this
> > feels equivalent to the way that the scoped_refptr above would be
initialized
> to
> > nullptr because that's what its default constructor does. This is simply
> working
> > around the lack of a default constructor for raw pointer types.
> 
> Yes, I understand that this will be caught by all future constructors.
> 
> Can you see that it might be confusing for people reading the code?

I disagree that this is confusing. This method is clearer than constructor
initialization lists because one can tell from reading only the field
declarations that they will all be initialized without having to check that the
(possibly multiple) constructors aren't missing something.

From personal experience I am very likely to shoot myself in the foot by adding
a primitive type field to a class and forgetting to add an initializer to the
constructor. Combining these two in one place seems like a net win.

[benwells, 2015-04-09 00:11:47]

On 2015/04/09 00:06:36, reillyg wrote:
>
https://codereview.chromium.org/1075593003/diff/1/chrome/browser/ui/views/ext...
> File chrome/browser/ui/views/extensions/device_permissions_dialog_view.cc
> (right):
> 
>
https://codereview.chromium.org/1075593003/diff/1/chrome/browser/ui/views/ext...
> chrome/browser/ui/views/extensions/device_permissions_dialog_view.cc:47:
> ui::TableModelObserver* observer_ = nullptr;
> On 2015/04/08 23:44:29, benwells wrote:
> > On 2015/04/08 22:57:25, reillyg wrote:
> > > On 2015/04/08 22:53:22, benwells wrote:
> > > > since you already have an initializer list in the constructor, why not
do
> it
> > > > there, the old fashioned way?
> > > 
> > > This method guarantees that any future constructor will also inherit this
> > > default behavior. Disregarding the initializer list in the constructor
this
> > > feels equivalent to the way that the scoped_refptr above would be
> initialized
> > to
> > > nullptr because that's what its default constructor does. This is simply
> > working
> > > around the lack of a default constructor for raw pointer types.
> > 
> > Yes, I understand that this will be caught by all future constructors.
> > 
> > Can you see that it might be confusing for people reading the code?
> 
> I disagree that this is confusing. This method is clearer than constructor
> initialization lists because one can tell from reading only the field
> declarations that they will all be initialized without having to check that
the
> (possibly multiple) constructors aren't missing something.
> 
> From personal experience I am very likely to shoot myself in the foot by
adding
> a primitive type field to a class and forgetting to add an initializer to the
> constructor. Combining these two in one place seems like a net win.

It's confusing because it is rare in our codebase. But, it sounds like you've
considered all of this which is really what I was wondering. I don't think it
matters that much or is worth arguing over, so lgtm.

[Reilly Grant (use Gerrit), 2015-04-09 00:15:40]

The CQ bit was checked by reillyg@chromium.org

[commit-bot: I haz the power, 2015-04-09 00:16:37]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1075593003/1

[commit-bot: I haz the power, 2015-04-09 02:05:10]

Committed patchset #1 (id:1)

[commit-bot: I haz the power, 2015-04-09 02:05:58]

Patchset 1 (id:??) landed as
https://crrev.com/b61a9fd6fe1a1a61733b0f08e9f1a209c557ec9d
Cr-Commit-Position: refs/heads/master@{#324338}