Adding presubmit check for WeakPtrFactory in the code

Adding presubmit check for WeakPtrFactory in the code

This change will ensure WeakPtrFactory order is maintained the files
properly.
As per WeakPtrFactory's documentation:
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

[MRV, 2014-10-22 05:21:25]

mohan.reddy@samsung.com changed reviewers:
+ maruel@chromium.org

[MRV, 2014-10-22 05:21:25]

PTAL

[M-A Ruel, 2014-10-22 18:02:29]

Please have someone who know about WeakPtrFactory review this first.

[M-A Ruel, 2014-10-22 18:02:40]

https://chromiumcodereview.appspot.com/669853003/diff/20001/PRESUBMIT.py
File PRESUBMIT.py (right):

https://chromiumcodereview.appspot.com/669853003/diff/20001/PRESUBMIT.py#newc...
PRESUBMIT.py:1282: if (not f.LocalPath().endswith(('.cc', '.cpp', '.h',
'.mm'))):
 if not f.LocalPath().endswith(('.cc', '.cpp', '.h', '.mm')):

[MRV, 2014-10-23 03:06:31]

Thanks Mr. Ruel for review.
I have incorporated the suggested changes.

PTAL

https://chromiumcodereview.appspot.com/669853003/diff/20001/PRESUBMIT.py
File PRESUBMIT.py (right):

https://chromiumcodereview.appspot.com/669853003/diff/20001/PRESUBMIT.py#newc...
PRESUBMIT.py:1282: if (not f.LocalPath().endswith(('.cc', '.cpp', '.h',
'.mm'))):
On 2014/10/22 18:02:40, M-A Ruel wrote:
>  if not f.LocalPath().endswith(('.cc', '.cpp', '.h', '.mm')):

Done.

[MRV, 2014-10-23 13:45:39]

The CQ bit was checked by mohan.reddy@samsung.com

[commit-bot: I haz the power, 2014-10-23 13:46:03]

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

[commit-bot: I haz the power, 2014-10-23 13:46:04]

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

[commit-bot: I haz the power, 2014-10-23 13:46:05]

No LGTM from a valid reviewer yet. Only full committers are accepted.
Even if an LGTM may have been provided, it was from a non-committer or
a provisional committer, _not_ a full super star committer.
See http://www.chromium.org/getting-involved/become-a-committer
Note that this has nothing to do with OWNERS files.

[MRV, 2014-10-23 13:47:12]

On 2014/10/23 13:46:05, I haz the power (commit-bot) wrote:
> No LGTM from a valid reviewer yet. Only full committers are accepted.
> Even if an LGTM may have been provided, it was from a non-committer or
> a provisional committer, _not_ a full super star committer.
> See http://www.chromium.org/getting-involved/become-a-committer
> Note that this has nothing to do with OWNERS files.

Tried to check for all the builders.

[MRV, 2014-11-14 15:25:24]

On 2014/10/23 13:47:12, MRV wrote:
> On 2014/10/23 13:46:05, I haz the power (commit-bot) wrote:
> > No LGTM from a valid reviewer yet. Only full committers are accepted.
> > Even if an LGTM may have been provided, it was from a non-committer or
> > a provisional committer, _not_ a full super star committer.
> > See http://www.chromium.org/getting-involved/become-a-committer
> > Note that this has nothing to do with OWNERS files.
> 
> Tried to check for all the builders.

@Mr. Ruel 
PTAL.

[M-A Ruel, 2014-11-14 15:37:18]

On 2014/10/22 18:02:29, M-A Ruel wrote:
> Please have someone who know about WeakPtrFactory review this first.

^^^

[MRV, 2014-11-14 15:40:52]

On 2014/11/14 15:37:18, M-A Ruel wrote:
> On 2014/10/22 18:02:29, M-A Ruel wrote:
> > Please have someone who know about WeakPtrFactory review this first.
> 
> ^^^

Thanks Mr. Ruel for the feedback.
@Mr. Avi or Mr. Sky, Please review my patch, as you are my reviewers for most of
my patches.
I have added most of the unit testcases which I have encountered and tested
same.

[MRV, 2014-11-14 15:41:20]

mohan.reddy@samsung.com changed reviewers:
+ avi@chromium.org, sky@chromium.org

[Avi (use Gerrit), 2014-11-14 15:57:25]

On 2014/11/14 15:40:52, MRV wrote:
> On 2014/11/14 15:37:18, M-A Ruel wrote:
> > On 2014/10/22 18:02:29, M-A Ruel wrote:
> > > Please have someone who know about WeakPtrFactory review this first.
> > 
> > ^^^
> 
> Thanks Mr. Ruel for the feedback.
> @Mr. Avi or Mr. Sky, Please review my patch, as you are my reviewers for most
of
> my patches.
> I have added most of the unit testcases which I have encountered and tested
> same.

Marc-Antoine explicitly said to find experts in WeakPtrFactory, so the fact that
we've reviewed many of your patches doesn't matter. Go do a blame on weak_ptr.h
and see who to ask. wez, maybe?

[MRV, 2014-11-17 02:58:58]

mohan.reddy@samsung.com changed reviewers:
+ dmichael@chromium.org, wez@chromium.org

[MRV, 2014-11-17 03:01:18]

On 2014/11/14 15:57:25, Avi wrote:
> On 2014/11/14 15:40:52, MRV wrote:
> > On 2014/11/14 15:37:18, M-A Ruel wrote:
> > > On 2014/10/22 18:02:29, M-A Ruel wrote:
> > > > Please have someone who know about WeakPtrFactory review this first.
> > > 
> > > ^^^
> > 
> > Thanks Mr. Ruel for the feedback.
> > @Mr. Avi or Mr. Sky, Please review my patch, as you are my reviewers for
most
> of
> > my patches.
> > I have added most of the unit testcases which I have encountered and tested
> > same.
> 
> Marc-Antoine explicitly said to find experts in WeakPtrFactory, so the fact
that
> we've reviewed many of your patches doesn't matter. Go do a blame on
weak_ptr.h
> and see who to ask. wez, maybe?

Thanks Mr. Avi for the suggestion.
@Mr.wez or Mr.dmichael  PTAL.

[dmichael (off chromium), 2014-11-17 16:20:16]

There's already a clang plugin to check for this, just waiting to be turned on.
Can you just use that?

You could remove the flag if everything builds properly with it now:
https://code.google.com/p/chromium/codesearch#chromium/src/tools/clang/plugin...

[MRV, 2014-11-18 03:35:34]

On 2014/11/17 16:20:16, dmichael wrote:
> There's already a clang plugin to check for this, just waiting to be turned
on.
> Can you just use that?
> 
> You could remove the flag if everything builds properly with it now:
>
https://code.google.com/p/chromium/codesearch#chromium/src/tools/clang/plugin...

@mr. dmicheal, thanks for your review.
I have checked enabling the clang plugin, the plugin is keep giving errors when
there are more then one "WeakPtrFactory" in the same class, for example below
case.

  base::WeakPtrFactory<WindowEventDispatcher> repost_event_factory_;
  base::WeakPtrFactory<WindowEventDispatcher> held_event_factory_;

for example, you can observe same in following files (where WeakPtrFactory
declarations are seen immediately)
content/renderer/pepper/pepper_plugin_instance_impl.h
ui/aura/window_event_dispatcher.h

[dmichael (off chromium), 2014-11-18 17:58:19]

On 2014/11/18 03:35:34, MRV wrote:
> On 2014/11/17 16:20:16, dmichael wrote:
> > There's already a clang plugin to check for this, just waiting to be turned
> on.
> > Can you just use that?
> > 
> > You could remove the flag if everything builds properly with it now:
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/tools/clang/plugin...
> 
> @mr. dmicheal, thanks for your review.
> I have checked enabling the clang plugin, the plugin is keep giving errors
when
> there are more then one "WeakPtrFactory" in the same class, for example below
> case.
> 
>   base::WeakPtrFactory<WindowEventDispatcher> repost_event_factory_;
>   base::WeakPtrFactory<WindowEventDispatcher> held_event_factory_;
> 
> for example, you can observe same in following files (where WeakPtrFactory
> declarations are seen immediately)
> content/renderer/pepper/pepper_plugin_instance_impl.h
> ui/aura/window_event_dispatcher.h
I'd still prefer a Clang checker to a PRESUBMIT check. The Clang check will
happen at compile time, which is more useful. And it's using the actual compiler
front-end rather than regular expressions, so it can be more robust.

I would suggest updating the test:
https://code.google.com/p/chromium/codesearch#chromium/src/tools/clang/plugin...
and fixing the checker:
https://code.google.com/p/chromium/codesearch#chromium/src/tools/clang/plugin...

(Sorry for the bug; I thought I remembered addressing that case, but I guess I
did not.)

If you don't feel comfortable tinkering in Clang, I can fix it.

[Wez, 2014-11-18 18:21:39]

I agree w/ dmichael@ that this should be done in Clang.

At a cursory glance, IIUC the PRESUBMIT.py check simply enforces that
WeakPtrFactory is immediately followed by DISALLOW_COPY_AND_ASSIGN, which means
it only copes with the case of WeakPtrFactory being used to refer to dispense
WeakPtrs to |this|.  It's perfectly valid, though, for WeakPtrFactory to be used
to wrap another class member, or some other object, though, in which case the
positioning of that factory in the containing cass need not be restricted in
this way.

[MRV, 2014-11-19 08:54:56]

On 2014/11/18 18:21:39, Wez wrote:
> I agree w/ dmichael@ that this should be done in Clang.
> 
> At a cursory glance, IIUC the PRESUBMIT.py check simply enforces that
> WeakPtrFactory is immediately followed by DISALLOW_COPY_AND_ASSIGN, which
means
> it only copes with the case of WeakPtrFactory being used to refer to dispense
> WeakPtrs to |this|.  It's perfectly valid, though, for WeakPtrFactory to be
used
> to wrap another class member, or some other object, though, in which case the
> positioning of that factory in the containing cass need not be restricted in
> this way.

Thanks mr. dichael and wez for your feedback, I'll understand same try to come
up with the patch.

[dmichael (off chromium), 2014-11-20 00:05:14]

On 2014/11/19 08:54:56, MRV wrote:
> On 2014/11/18 18:21:39, Wez wrote:
> > I agree w/ dmichael@ that this should be done in Clang.
> > 
> > At a cursory glance, IIUC the PRESUBMIT.py check simply enforces that
> > WeakPtrFactory is immediately followed by DISALLOW_COPY_AND_ASSIGN, which
> means
> > it only copes with the case of WeakPtrFactory being used to refer to
dispense
> > WeakPtrs to |this|.  It's perfectly valid, though, for WeakPtrFactory to be
> used
> > to wrap another class member, or some other object, though, in which case
the
> > positioning of that factory in the containing cass need not be restricted in
> > this way.
> 
> Thanks mr. dichael and wez for your feedback, I'll understand same try to come
> up with the patch.

I put together a patch here:
https://codereview.chromium.org/740083002/
...but it looks like there are still some places to fix.

[MRV, 2014-11-20 14:43:12]

On 2014/11/20 00:05:14, dmichael wrote:
> On 2014/11/19 08:54:56, MRV wrote:
> > On 2014/11/18 18:21:39, Wez wrote:
> > > I agree w/ dmichael@ that this should be done in Clang.
> > > 
> > > At a cursory glance, IIUC the PRESUBMIT.py check simply enforces that
> > > WeakPtrFactory is immediately followed by DISALLOW_COPY_AND_ASSIGN, which
> > means
> > > it only copes with the case of WeakPtrFactory being used to refer to
> dispense
> > > WeakPtrs to |this|.  It's perfectly valid, though, for WeakPtrFactory to
be
> > used
> > > to wrap another class member, or some other object, though, in which case
> the
> > > positioning of that factory in the containing cass need not be restricted
in
> > > this way.
> > 
> > Thanks mr. dichael and wez for your feedback, I'll understand same try to
come
> > up with the patch.
> 
> I put together a patch here:
> https://codereview.chromium.org/740083002/
> ...but it looks like there are still some places to fix.

Mr.dmichael thanks for your review and making fine tuned patch.
I have submitted 3 patches which were some left outs for cleaning up weakptr.
I abandoning this patch now.
Thanks all for your reviews and time.