Add observer for BAAnnots from Javascript

Add observer for BAAnnots from Javascript

This Cl moves the observer code from the CPDFSDK_Widget up into the
CPDFSDK_Annot base class and then adds a second observer for CPDFSDK_BAAnnot
objects.

This allows us to attach an observer to the Annot javascript class which will
update its internal pointer to the BAAnnot if the BAAnnot is destroyed by
the CPDFSDK_PageView being destroyed.

BUG=chromium:642307
Committed: https://pdfium.googlesource.com/pdfium/+/ce04a458828b45035dab46c13e14a1f0ae67a2b7

[dsinclair, 2016-09-01 16:36:14]

dsinclair@chromium.org changed reviewers:
+ thestig@chromium.org, tsepez@chromium.org, weili@chromium.org

[dsinclair, 2016-09-01 16:36:14]

PTAL.

[dsinclair, 2016-09-01 16:36:18]

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

[commit-bot: I haz the power, 2016-09-01 16:36:26]

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-01 16:55:16]

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

[commit-bot: I haz the power, 2016-09-01 16:55:17]

Dry run: This issue passed the CQ dry run.

[Tom Sepez, 2016-09-01 17:15:18]

On 2016/09/01 16:36:14, dsinclair wrote:
> PTAL.

Can we get by with just making a virtual DTOR somewhere rather than having to
subclass the observer?  Is it just a matter of knowing the type at destruction
time?

[dsinclair, 2016-09-01 17:18:29]

On 2016/09/01 17:15:18, Tom Sepez wrote:
> On 2016/09/01 16:36:14, dsinclair wrote:
> > PTAL.
> 
> Can we get by with just making a virtual DTOR somewhere rather than having to
> subclass the observer?  Is it just a matter of knowing the type at destruction
> time?


The problem I ran into was the constructor:

  candidate constructor not viable: no known conversion from 'CPDFSDK_Widget **'
to 'CPDFSDK_Annot **' for 1st argument


I wasn't able to figure out if there was correct magic to coerce the
CPDFSDK_Widget* into a CPDFSDK_Annot**. Everything I tried failed, this was the
way I could figure out how to make it work.

[Tom Sepez, 2016-09-01 17:23:17]

> I wasn't able to figure out if there was correct magic to coerce the
> CPDFSDK_Widget* into a CPDFSDK_Annot**. Everything I tried failed, this was
the
> way I could figure out how to make it work.
what if we replace all the CPDFSDK_Annot** ptrs with a CPDFSDK_Widget**, and
then downcast as needed elsewhere?

[dsinclair, 2016-09-01 17:26:09]

On 2016/09/01 17:23:17, Tom Sepez wrote:
> > I wasn't able to figure out if there was correct magic to coerce the
> > CPDFSDK_Widget* into a CPDFSDK_Annot**. Everything I tried failed, this was
> the
> > way I could figure out how to make it work.
> what if we replace all the CPDFSDK_Annot** ptrs with a CPDFSDK_Widget**, and
> then downcast as needed elsewhere?


I wouldn't be able to pass in the needed CPDFSDK_BAAnnot pointer as it's the
superclass to CPDFSDK_Widget. The hierarchy is:

CPDFSDK_Widget -> CPDFSDK_BAAnnot -> CPDFSDK_Annot.

[Tom Sepez, 2016-09-01 17:44:40]

On 2016/09/01 17:26:09, dsinclair wrote:
> On 2016/09/01 17:23:17, Tom Sepez wrote:
> > > I wasn't able to figure out if there was correct magic to coerce the
> > > CPDFSDK_Widget* into a CPDFSDK_Annot**. Everything I tried failed, this
was
> > the
> > > way I could figure out how to make it work.
> > what if we replace all the CPDFSDK_Annot** ptrs with a CPDFSDK_Widget**, and
> > then downcast as needed elsewhere?
> 
> 
> I wouldn't be able to pass in the needed CPDFSDK_BAAnnot pointer as it's the
> superclass to CPDFSDK_Widget. The hierarchy is:
> 
> CPDFSDK_Widget -> CPDFSDK_BAAnnot -> CPDFSDK_Annot.

Lei, any thoughts? It feels like there ought to be a simpler way but I'm not
seeing it.

[Wei Li, 2016-09-01 17:53:40]

On 2016/09/01 17:26:09, dsinclair wrote:
> On 2016/09/01 17:23:17, Tom Sepez wrote:
> > > I wasn't able to figure out if there was correct magic to coerce the
> > > CPDFSDK_Widget* into a CPDFSDK_Annot**. Everything I tried failed, this
was
> > the
> > > way I could figure out how to make it work.
> > what if we replace all the CPDFSDK_Annot** ptrs with a CPDFSDK_Widget**, and
> > then downcast as needed elsewhere?
> 
> 
> I wouldn't be able to pass in the needed CPDFSDK_BAAnnot pointer as it's the
> superclass to CPDFSDK_Widget. The hierarchy is:
> 
> CPDFSDK_Widget -> CPDFSDK_BAAnnot -> CPDFSDK_Annot.


Can we just release the widget/BAAnnot by calling ReleaseAnnot() when we delete
CPDFSDK_Annot in a pageView?

[dsinclair, 2016-09-01 17:58:04]

On 2016/09/01 17:53:40, Wei Li wrote:
> Can we just release the widget/BAAnnot by calling ReleaseAnnot() when we
delete
> CPDFSDK_Annot in a pageView?


The BAAnnot is released when the pageview is cleaned up. The problem is the
Annot in the Javascript classes has a pointer to the BAAnnot which does not get
(and I don't think can get) cleaned up at that point.

Then, we fire a timer on the page, which calls a method on the Annot, which
calls to the deleted BAAnnot. So, the observer lets us nullify the BAAnnot
pointer in the Annot class and we can bail early during JS processing.

[dsinclair, 2016-09-01 17:59:20]

On 2016/09/01 17:58:04, dsinclair wrote:
> On 2016/09/01 17:53:40, Wei Li wrote:
> > Can we just release the widget/BAAnnot by calling ReleaseAnnot() when we
> delete
> > CPDFSDK_Annot in a pageView?
> 
> 
> The BAAnnot is released when the pageview is cleaned up. The problem is the
> Annot in the Javascript classes has a pointer to the BAAnnot which does not
get
> (and I don't think can get) cleaned up at that point.
> 
> Then, we fire a timer on the page, which calls a method on the Annot, which
> calls to the deleted BAAnnot. So, the observer lets us nullify the BAAnnot
> pointer in the Annot class and we can bail early during JS processing.


Just to clarify, we have:

 * in fpdfsdk/: CPDFSDK_Widget -> CPDFSDK_BAAnnot -> CPDFSDK_Annot
 * in fpdfsdk/javascript/: Annot.

The Annot I'm talking about above is the javascript Annot class, not the
CPDFSDK_Annot class.

[Tom Sepez, 2016-09-01 18:03:30]

https://codereview.chromium.org/2306663002/diff/1/fpdfsdk/javascript/Annot.h
File fpdfsdk/javascript/Annot.h (right):

https://codereview.chromium.org/2306663002/diff/1/fpdfsdk/javascript/Annot.h#...
fpdfsdk/javascript/Annot.h:28: std::unique_ptr<CPDFSDK_BAAnnot::Observer>
m_pObserver;
This can't be a widget observer?

[Tom Sepez, 2016-09-01 18:04:24]

https://codereview.chromium.org/2306663002/diff/1/fpdfsdk/javascript/Annot.cpp
File fpdfsdk/javascript/Annot.cpp (right):

https://codereview.chromium.org/2306663002/diff/1/fpdfsdk/javascript/Annot.cp...
fpdfsdk/javascript/Annot.cpp:97: m_BAAnnot = baannot;
m_BAAnnot can't be a widget?

[dsinclair, 2016-09-01 18:11:59]

https://codereview.chromium.org/2306663002/diff/1/fpdfsdk/javascript/Annot.cpp
File fpdfsdk/javascript/Annot.cpp (right):

https://codereview.chromium.org/2306663002/diff/1/fpdfsdk/javascript/Annot.cp...
fpdfsdk/javascript/Annot.cpp:97: m_BAAnnot = baannot;
On 2016/09/01 18:04:24, Tom Sepez wrote:
> m_BAAnnot can't be a widget?

I don't think so as the Widget inherits from BAAnnot and overrides stuff.
(Things like IsAppearanceValid are overridden in Widget)

https://codereview.chromium.org/2306663002/diff/1/fpdfsdk/javascript/Annot.h
File fpdfsdk/javascript/Annot.h (right):

https://codereview.chromium.org/2306663002/diff/1/fpdfsdk/javascript/Annot.h#...
fpdfsdk/javascript/Annot.h:28: std::unique_ptr<CPDFSDK_BAAnnot::Observer>
m_pObserver;
On 2016/09/01 18:03:30, Tom Sepez wrote:
> This can't be a widget observer?


CPDFSDK_BAAnnot is the parent class of CDPFSDK_Widget, so we wouldn't be able to
create a CPDFSDK_Widget::Observer(CPDFSDK_Widget**) with the m_BAAnnot object.
(Or, am I missing something?)

[Tom Sepez, 2016-09-01 18:12:07]

https://codereview.chromium.org/2306663002/diff/1/fpdfsdk/javascript/Annot.cpp
File fpdfsdk/javascript/Annot.cpp (right):

https://codereview.chromium.org/2306663002/diff/1/fpdfsdk/javascript/Annot.cp...
fpdfsdk/javascript/Annot.cpp:97: m_BAAnnot = baannot;
On 2016/09/01 18:04:24, Tom Sepez wrote:
> m_BAAnnot can't be a widget?

I mean, what if we repalce m_BAAnont with a CPDFSDK_Annot and downcast?

https://codereview.chromium.org/2306663002/diff/1/fpdfsdk/javascript/Annot.h
File fpdfsdk/javascript/Annot.h (right):

https://codereview.chromium.org/2306663002/diff/1/fpdfsdk/javascript/Annot.h#...
fpdfsdk/javascript/Annot.h:28: std::unique_ptr<CPDFSDK_BAAnnot::Observer>
m_pObserver;
On 2016/09/01 18:03:30, Tom Sepez wrote:
> This can't be a widget observer?

I mean, this can't be an CPDFSDK_Annot::Observer ?

[dsinclair, 2016-09-01 18:28:52]

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

[commit-bot: I haz the power, 2016-09-01 18:29:05]

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

[dsinclair, 2016-09-01 18:29:07]

PTAL.

[commit-bot: I haz the power, 2016-09-01 19:24:12]

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

[commit-bot: I haz the power, 2016-09-01 19:24:14]

Dry run: This issue passed the CQ dry run.

[Tom Sepez, 2016-09-01 20:06:23]

Cleaner.  CL description needs update.

https://codereview.chromium.org/2306663002/diff/40001/fpdfsdk/javascript/Anno...
File fpdfsdk/javascript/Annot.cpp (right):

https://codereview.chromium.org/2306663002/diff/40001/fpdfsdk/javascript/Anno...
fpdfsdk/javascript/Annot.cpp:17: return annot ?
static_cast<CPDFSDK_BAAnnot*>(annot) : nullptr;
Do we need the ? operator, casting null OK.  Were you intending on a check? 
Maybe keep anyways.

https://codereview.chromium.org/2306663002/diff/40001/fpdfsdk/javascript/Fiel...
File fpdfsdk/javascript/Field.cpp (right):

https://codereview.chromium.org/2306663002/diff/40001/fpdfsdk/javascript/Fiel...
fpdfsdk/javascript/Field.cpp:275: if (pWidget) {
Need to check pannot here, its the thing that gets cleared.  Also, if the loop
is in terms of widgets, you can just assign to the pAnnot* without casting.

[Lei Zhang, 2016-09-02 01:21:25]

https://codereview.chromium.org/2306663002/diff/40001/fpdfsdk/javascript/Annot.h
File fpdfsdk/javascript/Annot.h (right):

https://codereview.chromium.org/2306663002/diff/40001/fpdfsdk/javascript/Anno...
fpdfsdk/javascript/Annot.h:27: CPDFSDK_Annot* m_Annot = nullptr;
nit: m_pAnnot

https://codereview.chromium.org/2306663002/diff/40001/fpdfsdk/javascript/Anno...
fpdfsdk/javascript/Annot.h:27: CPDFSDK_Annot* m_Annot = nullptr;
Can this stay as a CPDFSDK_BAAnnot*, and thus remove the need for ToBAAnnot() ?

[dsinclair, 2016-09-06 15:44:50]

https://codereview.chromium.org/2306663002/diff/40001/fpdfsdk/javascript/Anno...
File fpdfsdk/javascript/Annot.cpp (right):

https://codereview.chromium.org/2306663002/diff/40001/fpdfsdk/javascript/Anno...
fpdfsdk/javascript/Annot.cpp:17: return annot ?
static_cast<CPDFSDK_BAAnnot*>(annot) : nullptr;
On 2016/09/01 20:06:23, Tom Sepez wrote:
> Do we need the ? operator, casting null OK.  Were you intending on a check? 
> Maybe keep anyways.

Not needed, so removed. Was error on the side to explicit.

https://codereview.chromium.org/2306663002/diff/40001/fpdfsdk/javascript/Annot.h
File fpdfsdk/javascript/Annot.h (right):

https://codereview.chromium.org/2306663002/diff/40001/fpdfsdk/javascript/Anno...
fpdfsdk/javascript/Annot.h:27: CPDFSDK_Annot* m_Annot = nullptr;
On 2016/09/02 01:21:25, Lei Zhang wrote:
> Can this stay as a CPDFSDK_BAAnnot*, and thus remove the need for ToBAAnnot()
?

No, this has to be a CPDFSDK_Annot so we can pass the pointer to the pointer
into the observer.

https://codereview.chromium.org/2306663002/diff/40001/fpdfsdk/javascript/Anno...
fpdfsdk/javascript/Annot.h:27: CPDFSDK_Annot* m_Annot = nullptr;
On 2016/09/02 01:21:25, Lei Zhang wrote:
> nit: m_pAnnot

Sigh, can we start moving away from this notation soon?

https://codereview.chromium.org/2306663002/diff/40001/fpdfsdk/javascript/Fiel...
File fpdfsdk/javascript/Field.cpp (right):

https://codereview.chromium.org/2306663002/diff/40001/fpdfsdk/javascript/Fiel...
fpdfsdk/javascript/Field.cpp:275: if (pWidget) {
On 2016/09/01 20:06:23, Tom Sepez wrote:
> Need to check pannot here, its the thing that gets cleared.  Also, if the loop
> is in terms of widgets, you can just assign to the pAnnot* without casting.

Done.

Not sure what you're referring to about the casting. The static_cast on 272 is
needed otherwise the compiler complains.

cannot initialize a variable of type 'CPDFSDK_Widget *' with an lvalue of type
'CPDFSDK_Annot *'

[dsinclair, 2016-09-06 15:44:53]

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

[commit-bot: I haz the power, 2016-09-06 15:45:01]

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-06 16:02:24]

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

[commit-bot: I haz the power, 2016-09-06 16:02:25]

Dry run: This issue passed the CQ dry run.

[Tom Sepez, 2016-09-06 20:27:05]

https://codereview.chromium.org/2306663002/diff/80001/fpdfsdk/javascript/Annot.h
File fpdfsdk/javascript/Annot.h (right):

https://codereview.chromium.org/2306663002/diff/80001/fpdfsdk/javascript/Anno...
fpdfsdk/javascript/Annot.h:28: std::unique_ptr<CPDFSDK_Annot::Observer>
m_pObserver;
Maybe it should inherit from this rather than having one, eg. the object itself
can observe destruction.  Is that the usual pattern? Does that imply that
Observer is a virtual API?

[dsinclair, 2016-09-06 20:32:20]

https://codereview.chromium.org/2306663002/diff/80001/fpdfsdk/javascript/Annot.h
File fpdfsdk/javascript/Annot.h (right):

https://codereview.chromium.org/2306663002/diff/80001/fpdfsdk/javascript/Anno...
fpdfsdk/javascript/Annot.h:28: std::unique_ptr<CPDFSDK_Annot::Observer>
m_pObserver;
On 2016/09/06 20:27:05, Tom Sepez wrote:
> Maybe it should inherit from this rather than having one, eg. the object
itself
> can observe destruction.  Is that the usual pattern? Does that imply that
> Observer is a virtual API?


We would need to change Observer in that case, currently it accepts the thing to
observe in the constructor, but in this case we don't know the annotation until
SetSDKAnnot is called.

I can make that change, to allow a nullptr as the observed object and provide a
setter, but is that complexity we want in the observer?

I guess my question becomes, is it strange that Annot is-a
CPDFSDK_Annot::Observer but has-a CPDFSDK_Annot?

Feels like the observer is making Annot into a different level of object by
pulling it into CPDFSDK_Annot world?

[Tom Sepez, 2016-09-06 22:12:37]

> We would need to change Observer in that case, currently it accepts the thing
to
> observe in the constructor, but in this case we don't know the annotation
until
> SetSDKAnnot is called.
> 
> I can make that change, to allow a nullptr as the observed object and provide
a
> setter, but is that complexity we want in the observer?
> 
Ah, probably not.

> I guess my question becomes, is it strange that Annot is-a
> CPDFSDK_Annot::Observer but has-a CPDFSDK_Annot?
> 
That feels kinda normal to me.

Anyways LGTM.

[dsinclair, 2016-09-07 12:46:35]

The CQ bit was checked by dsinclair@chromium.org

[commit-bot: I haz the power, 2016-09-07 12:46:42]

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-07 12:46:59]

Description was changed from

==========
Add observer for BAAnnots from Javascript

This Cl moves the observer code from the CPDFSDK_Widget up into the
CPDFSDK_Annot base class and then adds a second observer for CPDFSDK_BAAnnot
objects.

This allows us to attach an observer to the Annot javascript class which will
update its internal pointer to the BAAnnot if the BAAnnot is destroyed by
the CPDFSDK_PageView being destroyed.

BUG=chromium:642307
==========

to

==========
Add observer for BAAnnots from Javascript

This Cl moves the observer code from the CPDFSDK_Widget up into the
CPDFSDK_Annot base class and then adds a second observer for CPDFSDK_BAAnnot
objects.

This allows us to attach an observer to the Annot javascript class which will
update its internal pointer to the BAAnnot if the BAAnnot is destroyed by
the CPDFSDK_PageView being destroyed.

BUG=chromium:642307

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

[commit-bot: I haz the power, 2016-09-07 12:47:00]

Committed patchset #5 (id:80001) as
https://pdfium.googlesource.com/pdfium/+/ce04a458828b45035dab46c13e14a1f0ae67...