Maintaing the proper order of initialization WeakPtrFactory

Maintaing the proper order of initialization WeakPtrFactory

Changing in the intialization order of WeakPtrFactory in "src/base" module
such that all member variables should appear before the WeakPtrFactory
to ensure that any WeakPtrs to Controller are invalidated before its members
variable's destructors are executed, rendering them invalid.

BUG=303818
Committed: https://crrev.com/7ce4adfebbda4208511fea7d58eb3e96e296337f
Cr-Commit-Position: refs/heads/master@{#294741}

[ARUNKK, 2014-09-12 10:30:40]

kulkarni.a@samsung.com changed reviewers:
+ mark@chromium.org, thakis@chromium.org

[ARUNKK, 2014-09-12 10:30:41]

mark@chromium.org: Please review changes in 

thakis@chromium.org: Please review changes in 

PTAL.

[ARUNKK, 2014-09-12 10:56:11]

kulkarni.a@samsung.com changed reviewers:
+ darin@chromium.org
- thakis@chromium.org

[Mark Mentovai, 2014-09-12 12:30:28]

I’m currently unable to do reviews that aren’t related to bugs 386208 and
399276. Please find another reviewer.

[ARUNKK, 2014-09-12 13:27:07]

kulkarni.a@samsung.com changed reviewers:
- mark@chromium.org

[ARUNKK, 2014-09-12 13:35:53]

kulkarni.a@samsung.com changed reviewers:
+ ajwong@chromium.org, brettw@chromium.org
- darin@chromium.org

[Nico, 2014-09-12 17:44:52]

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

[Nico, 2014-09-12 17:44:52]

lgtm, thanks!

[ARUNKK, 2014-09-12 21:51:33]

The CQ bit was checked by kulkarni.a@samsung.com

[commit-bot: I haz the power, 2014-09-12 21:53:49]

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

[commit-bot: I haz the power, 2014-09-12 22:17:58]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-09-12 22:18:00]

Try jobs failed on following builders:
  android_chromium_gn_compile_rel on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_chromiu...)

[ARUNKK, 2014-09-13 08:28:39]

The CQ bit was checked by kulkarni.a@samsung.com

[commit-bot: I haz the power, 2014-09-13 08:29:45]

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

[commit-bot: I haz the power, 2014-09-13 08:53:15]

Committed patchset #1 (id:1) as cbd982690789d1b17683dab5c237d5591ed605ad

[commit-bot: I haz the power, 2014-09-13 08:57:55]

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

[Charlie, 2014-11-06 22:57:05]

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

[Charlie, 2014-11-06 22:57:06]

This seems to be generating a compile error for me when I try to create a
2-argument CancelableCallback.

https://codereview.chromium.org/565123002/diff/1/base/cancelable_callback.h
File base/cancelable_callback.h (right):

https://codereview.chromium.org/565123002/diff/1/base/cancelable_callback.h#n...
base/cancelable_callback.h:205: : weak_factory_(this),
I'm getting an error in the constructor now that these arguments are in a
different order. Wow, am I the only customer of this code?

[Nico, 2014-11-06 22:59:49]

The joy of template specializations :-/

https://codereview.chromium.org/565123002/diff/1/base/cancelable_callback.h
File base/cancelable_callback.h (right):

https://codereview.chromium.org/565123002/diff/1/base/cancelable_callback.h#n...
base/cancelable_callback.h:117:
base::WeakPtrFactory<CancelableCallback<void(void)> > weak_factory_;
Heh, changing that down there and not up here looks like a bug too. We should
probably convert this class to use a variadic template instead of the manual
copypasta we have today, together with fixing this bug. ckehoe, do you want to
give this a try?

[Charlie, 2014-11-06 23:03:08]

Um. I can do the simple fix. But I am no template wizard :-)

On Thu Nov 06 2014 at 2:59:49 PM <thakis@chromium.org> wrote:

> The joy of template specializations :-/
>
>
> https://codereview.chromium.org/565123002/diff/1/base/
> cancelable_callback.h
> File base/cancelable_callback.h (right):
>
> https://codereview.chromium.org/565123002/diff/1/base/
> cancelable_callback.h#newcode117
> base/cancelable_callback.h:117
>
<https://codereview.chromium.org/565123002/diff/1/base/cancelable_callback.h#n...>
> :
> base::WeakPtrFactory<CancelableCallback<void(void)> > weak_factory_;
> Heh, changing that down there and not up here looks like a bug too. We
> should probably convert this class to use a variadic template instead of
> the manual copypasta we have today, together with fixing this bug.
> ckehoe, do you want to give this a try?
>
> https://codereview.chromium.org/565123002/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Nico, 2014-11-06 23:10:29]

I think the template wizard fix is fairly mechanical: Take the one-argument
A1 version, replace `template<typename A1>` with `template typename...
A1>`, and then replace `A1` with `A1...`. Delete the 0 and 2 argument
versions, the new variadic version should cover them. Maybe give it a try,
and I can try to talk you through if you run into problems?

On Thu, Nov 6, 2014 at 3:03 PM, Charlie Kehoe <ckehoe@chromium.org> wrote:

> Um. I can do the simple fix. But I am no template wizard :-)
>
>
> On Thu Nov 06 2014 at 2:59:49 PM <thakis@chromium.org> wrote:
>
>> The joy of template specializations :-/
>>
>>
>> https://codereview.chromium.org/565123002/diff/1/base/
>> cancelable_callback.h
>> File base/cancelable_callback.h (right):
>>
>> https://codereview.chromium.org/565123002/diff/1/base/
>> cancelable_callback.h#newcode117
>> base/cancelable_callback.h:117
>>
<https://codereview.chromium.org/565123002/diff/1/base/cancelable_callback.h#n...>
>> :
>> base::WeakPtrFactory<CancelableCallback<void(void)> > weak_factory_;
>> Heh, changing that down there and not up here looks like a bug too. We
>> should probably convert this class to use a variadic template instead of
>> the manual copypasta we have today, together with fixing this bug.
>> ckehoe, do you want to give this a try?
>>
>> https://codereview.chromium.org/565123002/
>>
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Charlie, 2014-11-07 19:23:35]

Thanks, I think I got it. Code review coming your way :-)

On Thu Nov 06 2014 at 3:10:28 PM Nico Weber <thakis@chromium.org> wrote:

> I think the template wizard fix is fairly mechanical: Take the
> one-argument A1 version, replace `template<typename A1>` with `template
> typename... A1>`, and then replace `A1` with `A1...`. Delete the 0 and 2
> argument versions, the new variadic version should cover them. Maybe give
> it a try, and I can try to talk you through if you run into problems?
>
> On Thu, Nov 6, 2014 at 3:03 PM, Charlie Kehoe <ckehoe@chromium.org> wrote:
>
>> Um. I can do the simple fix. But I am no template wizard :-)
>>
>>
>> On Thu Nov 06 2014 at 2:59:49 PM <thakis@chromium.org> wrote:
>>
>>> The joy of template specializations :-/
>>>
>>>
>>> https://codereview.chromium.org/565123002/diff/1/base/cancel
>>> able_callback.h
>>> File base/cancelable_callback.h (right):
>>>
>>> https://codereview.chromium.org/565123002/diff/1/base/cancel
>>> able_callback.h#newcode117
>>> base/cancelable_callback.h:117
>>>
<https://codereview.chromium.org/565123002/diff/1/base/cancelable_callback.h#n...>
>>> :
>>> base::WeakPtrFactory<CancelableCallback<void(void)> > weak_factory_;
>>> Heh, changing that down there and not up here looks like a bug too. We
>>> should probably convert this class to use a variadic template instead of
>>> the manual copypasta we have today, together with fixing this bug.
>>> ckehoe, do you want to give this a try?
>>>
>>> https://codereview.chromium.org/565123002/
>>>
>>
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.