Make Observers into a templated class

Make Observers into a templated class

Committed: https://pdfium.googlesource.com/pdfium/+/7b68f616e49235267eeac8db51aadade6d60e243

[Tom Sepez, 2016-09-06 23:50:22]

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

[Tom Sepez, 2016-09-06 23:50:46]

Dan, Lei, for discussion.  Maybe we should just go all the way and make this a
template like this.

[dsinclair, 2016-09-07 12:53:06]

On 2016/09/06 23:50:46, Tom Sepez wrote:
> Dan, Lei, for discussion.  Maybe we should just go all the way and make this a
> template like this.

Can you update this to ToT. I'd like to see it with the BAAnnot changes in there
as well.

[dsinclair, 2016-09-07 16:51:53]

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

https://codereview.chromium.org/2311343003/diff/20001/fpdfsdk/javascript/Fiel...
fpdfsdk/javascript/Field.cpp:272: CFX_Observable<CPDFSDK_Annot>::Observer
observer(&pAnnot);
If we make the Widget and BAAnnot Observable, can we get around the casting
needs and store the correct types now? Or, will C++ get annoyed if both Widget
and BAAnnot are public Observable<thing> and widget inherits from BAAnnot?

[Tom Sepez, 2016-09-07 16:52:17]

> Can you update this to ToT. I'd like to see it with the BAAnnot changes in
there
> as well.
Udpated.

[Tom Sepez, 2016-09-07 16:54:48]

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

https://codereview.chromium.org/2311343003/diff/40001/fpdfsdk/javascript/Fiel...
fpdfsdk/javascript/Field.cpp:273: CFX_WideString sValue =
Note: I put back two casts to eliminate being confused about the aliasing
between pWidget and pAnnot.

[Tom Sepez, 2016-09-07 16:56:08]

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

[commit-bot: I haz the power, 2016-09-07 16:56:11]

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

[Tom Sepez, 2016-09-07 17:09:14]

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

[commit-bot: I haz the power, 2016-09-07 17:09:22]

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

[Tom Sepez, 2016-09-07 17:10:17]

https://codereview.chromium.org/2311343003/diff/60001/core/fxcrt/include/cfx_...
File core/fxcrt/include/cfx_observable.h (right):

https://codereview.chromium.org/2311343003/diff/60001/core/fxcrt/include/cfx_...
core/fxcrt/include/cfx_observable.h:16: class Observer {
An Observable<>::Observer that observes observables.

[commit-bot: I haz the power, 2016-09-07 18:33:46]

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

[commit-bot: I haz the power, 2016-09-07 18:33:46]

Dry run: Try jobs failed on following builders:
  linux_xfa on master.tryserver.client.pdfium (JOB_FAILED,
https://build.chromium.org/p/tryserver.client.pdfium/builders/linux_xfa/build...)

[Tom Sepez, 2016-09-07 19:03:18]

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

https://codereview.chromium.org/2311343003/diff/20001/fpdfsdk/javascript/Fiel...
fpdfsdk/javascript/Field.cpp:272: CFX_Observable<CPDFSDK_Annot>::Observer
observer(&pAnnot);
On 2016/09/07 16:51:53, dsinclair wrote:
> If we make the Widget and BAAnnot Observable, can we get around the casting
> needs and store the correct types now? Or, will C++ get annoyed if both Widget
> and BAAnnot are public Observable<thing> and widget inherits from BAAnnot?

That would be the "diamond problem", and you'd need virtual inheritance.

[Tom Sepez, 2016-09-07 20:32:30]

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

[commit-bot: I haz the power, 2016-09-07 20:32:37]

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

[Tom Sepez, 2016-09-07 20:32:48]

Ok, PS#5 for review.

[Tom Sepez, 2016-09-07 20:38:42]

On 2016/09/07 20:32:48, Tom Sepez wrote:
> Ok, PS#5 for review.

ps#6 even better.  Thanks Dan.

[dsinclair, 2016-09-07 20:53:16]

The CQ bit was checked by dsinclair@chromium.org

[dsinclair, 2016-09-07 20:53:17]

lgtm

[commit-bot: I haz the power, 2016-09-07 20:53:25]

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

[Tom Sepez, 2016-09-07 20:58:41]

The CQ bit was unchecked by tsepez@chromium.org

[Lei Zhang, 2016-09-07 21:01:30]

lgtm

https://codereview.chromium.org/2311343003/diff/100001/fpdfsdk/javascript/Fie...
File fpdfsdk/javascript/Field.cpp (right):

https://codereview.chromium.org/2311343003/diff/100001/fpdfsdk/javascript/Fie...
fpdfsdk/javascript/Field.cpp:275: if (pAnnot) {
Isn't this always true?

[Tom Sepez, 2016-09-07 21:10:28]

https://codereview.chromium.org/2311343003/diff/100001/fpdfsdk/javascript/Fie...
File fpdfsdk/javascript/Field.cpp (right):

https://codereview.chromium.org/2311343003/diff/100001/fpdfsdk/javascript/Fie...
fpdfsdk/javascript/Field.cpp:275: if (pAnnot) {
On 2016/09/07 21:01:29, Lei Zhang wrote:
> Isn't this always true?

No, that's the whole point of the observer.  If we delete the widget as a side
effect of the OnFormat(), the observer nukes the pointer for us.

[Tom Sepez, 2016-09-07 21:11:03]

The CQ bit was checked by tsepez@chromium.org

[commit-bot: I haz the power, 2016-09-07 21:11:12]

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 21:11:30]

Description was changed from

==========
Make Observers into a templated class
==========

to

==========
Make Observers into a templated class

Committed:
https://pdfium.googlesource.com/pdfium/+/7b68f616e49235267eeac8db51aadade6d60...
==========

[commit-bot: I haz the power, 2016-09-07 21:11:31]

Committed patchset #6 (id:100001) as
https://pdfium.googlesource.com/pdfium/+/7b68f616e49235267eeac8db51aadade6d60...