Watch destruction of widgets around OnAAction() method

Watch destruction of widgets around OnAAction() method.

We implemented the CFX_Observable mechanism for detecting
stale objects some time ago; now just use it in more places.

Change method signatures to required an ObservedPtr to
indicate that the callers are aware that the value may be
destroyed out from underneath them.

BUG=649659
Committed: https://pdfium.googlesource.com/pdfium/+/f8074cefb2f8d947fa83d151bcbe080b485d6e6b

[Tom Sepez, 2016-09-26 18:38:16]

The CQ bit was checked by tsepez@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-26 18:38:31]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-26 18:51:16]

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

[commit-bot: I haz the power, 2016-09-26 18:51:17]

Dry run: This issue passed the CQ dry run.

[Tom Sepez, 2016-09-26 20:44:09]

Description was changed from

==========
Watch destruction of widgets around OnAAction() method

BUG=649659
==========

to

==========
Watch destruction of widgets around OnAAction() method.

We implemented the mechanism for detecting stale annots some
time ago; now just use it in more places.

BUG=649659
==========

[Tom Sepez, 2016-09-27 16:38:43]

Description was changed from

==========
Watch destruction of widgets around OnAAction() method.

We implemented the mechanism for detecting stale annots some
time ago; now just use it in more places.

BUG=649659
==========

to

==========
Watch destruction of widgets around OnAAction() method.

We implemented the CFX_Observable mechanism for detecting stale objects some
time ago; now just use it in more places.

Change method signatures to expect an ObservedPtr to indicate
that the callers are aware that the value may be destroyed
out from underneath them.

Pass by non-const ref seems unavoidable: pass by value would add a set
insert/delete on every call, and pass by
pointer to ObservedPtr gets ugly quickly.  The later would
require a lot of (*pAnnnot)->mumble() code changes.

BUG=649659
==========

[Tom Sepez, 2016-09-27 16:38:43]

tsepez@chromium.org changed reviewers:
+ thestig@chromium.org

[Tom Sepez, 2016-09-27 16:39:11]

Description was changed from

==========
Watch destruction of widgets around OnAAction() method.

We implemented the CFX_Observable mechanism for detecting stale objects some
time ago; now just use it in more places.

Change method signatures to expect an ObservedPtr to indicate
that the callers are aware that the value may be destroyed
out from underneath them.

Pass by non-const ref seems unavoidable: pass by value would add a set
insert/delete on every call, and pass by
pointer to ObservedPtr gets ugly quickly.  The later would
require a lot of (*pAnnnot)->mumble() code changes.

BUG=649659
==========

to

==========
Watch destruction of widgets around OnAAction() method.

We implemented the CFX_Observable mechanism for detecting
stale objects some time ago; now just use it in more places.

Change method signatures to expect an ObservedPtr to
indicate that the callers are aware that the value may be
destroyed out from underneath them.

Pass by non-const ref seems unavoidable: pass by value would add a set
insert/delete on every call, and pass by
pointer to ObservedPtr gets ugly quickly.  The later would
require a lot of (*pAnnnot)->mumble() code changes.

BUG=649659
==========

[Tom Sepez, 2016-09-27 16:39:28]

Description was changed from

==========
Watch destruction of widgets around OnAAction() method.

We implemented the CFX_Observable mechanism for detecting
stale objects some time ago; now just use it in more places.

Change method signatures to expect an ObservedPtr to
indicate that the callers are aware that the value may be
destroyed out from underneath them.

Pass by non-const ref seems unavoidable: pass by value would add a set
insert/delete on every call, and pass by
pointer to ObservedPtr gets ugly quickly.  The later would
require a lot of (*pAnnnot)->mumble() code changes.

BUG=649659
==========

to

==========
Watch destruction of widgets around OnAAction() method.

We implemented the CFX_Observable mechanism for detecting
stale objects some time ago; now just use it in more places.

Change method signatures to expect an ObservedPtr to
indicate that the callers are aware that the value may be
destroyed out from underneath them.

Pass by non-const ref seems unavoidable: pass by value
would add a set insert/delete on every call, and pass by
pointer to ObservedPtr gets ugly quickly.  The later would
require a lot of (*pAnnnot)->mumble() code changes.

BUG=649659
==========

[Tom Sepez, 2016-09-27 16:43:58]

Lei, for review.

[Tom Sepez, 2016-09-27 16:44:04]

The CQ bit was checked by tsepez@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-27 16:44:15]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Lei Zhang, 2016-09-27 16:46:29]

Description was changed from

==========
Watch destruction of widgets around OnAAction() method.

We implemented the CFX_Observable mechanism for detecting
stale objects some time ago; now just use it in more places.

Change method signatures to expect an ObservedPtr to
indicate that the callers are aware that the value may be
destroyed out from underneath them.

Pass by non-const ref seems unavoidable: pass by value
would add a set insert/delete on every call, and pass by
pointer to ObservedPtr gets ugly quickly.  The later would
require a lot of (*pAnnnot)->mumble() code changes.

BUG=649659
==========

to

==========
Watch destruction of widgets around OnAAction() method.

We implemented the CFX_Observable mechanism for detecting
stale objects some time ago; now just use it in more places.

Change method signatures to expect an ObservedPtr to
indicate that the callers are aware that the value may be
destroyed out from underneath them.

Pass by non-const ref seems unavoidable: pass by value
would add a set insert/delete on every call, and pass by
pointer to ObservedPtr gets ugly quickly.  The later would
require a lot of (*pAnnot)->mumble() code changes.

BUG=649659
==========

[commit-bot: I haz the power, 2016-09-27 16:56:03]

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

[commit-bot: I haz the power, 2016-09-27 16:56:04]

Dry run: This issue passed the CQ dry run.

[Lei Zhang, 2016-09-27 17:02:22]

Is passing by pointer really that bad? We have (*it)->func() for STL iterators
and it hasn't been the end of the world.

https://codereview.chromium.org/2368403002/diff/60001/fpdfsdk/formfiller/cffl...
File fpdfsdk/formfiller/cffl_formfiller.cpp (right):

https://codereview.chromium.org/2368403002/diff/60001/fpdfsdk/formfiller/cffl...
fpdfsdk/formfiller/cffl_formfiller.cpp:532: if (!pObserved)
Combine into: if (!pObserved || bExit)

Same pattern in a bunch of other places

[Tom Sepez, 2016-09-27 20:21:08]

https://codereview.chromium.org/2368403002/diff/60001/fpdfsdk/formfiller/cffl...
File fpdfsdk/formfiller/cffl_formfiller.cpp (right):

https://codereview.chromium.org/2368403002/diff/60001/fpdfsdk/formfiller/cffl...
fpdfsdk/formfiller/cffl_formfiller.cpp:532: if (!pObserved)
On 2016/09/27 17:02:22, Lei Zhang wrote:
> Combine into: if (!pObserved || bExit)
> 
> Same pattern in a bunch of other places

Ah, ok.  bExit in a few places is an out variable, there we need to be sure to
set it ... done.

[Tom Sepez, 2016-09-27 20:24:40]

Description was changed from

==========
Watch destruction of widgets around OnAAction() method.

We implemented the CFX_Observable mechanism for detecting
stale objects some time ago; now just use it in more places.

Change method signatures to expect an ObservedPtr to
indicate that the callers are aware that the value may be
destroyed out from underneath them.

Pass by non-const ref seems unavoidable: pass by value
would add a set insert/delete on every call, and pass by
pointer to ObservedPtr gets ugly quickly.  The later would
require a lot of (*pAnnot)->mumble() code changes.

BUG=649659
==========

to

==========
Watch destruction of widgets around OnAAction() method.

We implemented the CFX_Observable mechanism for detecting
stale objects some time ago; now just use it in more places.

Change method signatures to expect an ObservedPtr to
indicate that the callers are aware that the value may be
destroyed out from underneath them.

BUG=649659
==========

[Tom Sepez, 2016-09-27 20:25:11]

Description was changed from

==========
Watch destruction of widgets around OnAAction() method.

We implemented the CFX_Observable mechanism for detecting
stale objects some time ago; now just use it in more places.

Change method signatures to expect an ObservedPtr to
indicate that the callers are aware that the value may be
destroyed out from underneath them.

BUG=649659
==========

to

==========
Watch destruction of widgets around OnAAction() method.

We implemented the CFX_Observable mechanism for detecting
stale objects some time ago; now just use it in more places.

Change method signatures to required an ObservedPtr to
indicate that the callers are aware that the value may be
destroyed out from underneath them.

BUG=649659
==========

[Lei Zhang, 2016-09-27 20:45:03]

lgtm

https://codereview.chromium.org/2368403002/diff/100001/fpdfsdk/cpdfsdk_pagevi...
File fpdfsdk/cpdfsdk_pageview.cpp (right):

https://codereview.chromium.org/2368403002/diff/100001/fpdfsdk/cpdfsdk_pagevi...
fpdfsdk/cpdfsdk_pageview.cpp:331: if (pAnnot)
Is this missing an '!' ?

https://codereview.chromium.org/2368403002/diff/100001/fpdfsdk/cpdfsdk_pagevi...
fpdfsdk/cpdfsdk_pageview.cpp:385: if (pAnnotHandlerMgr->Annot_OnLButtonUp(this,
&pFXAnnot, nFlag, point))
Combine the 2 if statements?

[Tom Sepez, 2016-09-27 20:49:32]

https://codereview.chromium.org/2368403002/diff/100001/fpdfsdk/cpdfsdk_pagevi...
File fpdfsdk/cpdfsdk_pageview.cpp (right):

https://codereview.chromium.org/2368403002/diff/100001/fpdfsdk/cpdfsdk_pagevi...
fpdfsdk/cpdfsdk_pageview.cpp:331: if (pAnnot)
On 2016/09/27 20:45:03, Lei Zhang wrote:
> Is this missing an '!' ?

yikes. done.

https://codereview.chromium.org/2368403002/diff/100001/fpdfsdk/cpdfsdk_pagevi...
fpdfsdk/cpdfsdk_pageview.cpp:385: if (pAnnotHandlerMgr->Annot_OnLButtonUp(this,
&pFXAnnot, nFlag, point))
On 2016/09/27 20:45:03, Lei Zhang wrote:
> Combine the 2 if statements?

Done.

[Tom Sepez, 2016-09-27 21:18:20]

The CQ bit was checked by tsepez@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-27 21:18:29]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-27 21:29:19]

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

[commit-bot: I haz the power, 2016-09-27 21:29:19]

Dry run: This issue passed the CQ dry run.

[Tom Sepez, 2016-09-27 21:29:30]

The CQ bit was checked by tsepez@chromium.org

[Tom Sepez, 2016-09-27 21:29:31]

The patchset sent to the CQ was uploaded after l-g-t-m from thestig@chromium.org
Link to the patchset: https://codereview.chromium.org/2368403002/#ps160001
(title: "blown merge")

[commit-bot: I haz the power, 2016-09-27 21:29:39]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-27 21:30:00]

Description was changed from

==========
Watch destruction of widgets around OnAAction() method.

We implemented the CFX_Observable mechanism for detecting
stale objects some time ago; now just use it in more places.

Change method signatures to required an ObservedPtr to
indicate that the callers are aware that the value may be
destroyed out from underneath them.

BUG=649659
==========

to

==========
Watch destruction of widgets around OnAAction() method.

We implemented the CFX_Observable mechanism for detecting
stale objects some time ago; now just use it in more places.

Change method signatures to required an ObservedPtr to
indicate that the callers are aware that the value may be
destroyed out from underneath them.

BUG=649659

Committed:
https://pdfium.googlesource.com/pdfium/+/f8074cefb2f8d947fa83d151bcbe080b485d...
==========

[commit-bot: I haz the power, 2016-09-27 21:30:01]

Committed patchset #9 (id:160001) as
https://pdfium.googlesource.com/pdfium/+/f8074cefb2f8d947fa83d151bcbe080b485d...