CJS_Timer should observe CJS_Runtime destruction.

CJS_Timer should observe CJS_Runtime destruction.

Also remove dead CJS_EmbedObj::{Begin,End}Timer code.

BUG=539107
R=tsepez@chromium.org

Committed: https://pdfium.googlesource.com/pdfium/+/794c9b67d3d519342aa7e15052766f7d4a99f551

[Lei Zhang, 2015-10-04 21:56:49]

Patchset #1 (id:1) has been deleted

[Lei Zhang, 2015-10-04 21:58:34]

Patchset #1 (id:20001) has been deleted

[Lei Zhang, 2015-10-04 21:58:41]

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

[Tom Sepez, 2015-10-04 22:39:38]

lgtm

https://codereview.chromium.org/1384883002/diff/40001/fpdfsdk/include/javascr...
File fpdfsdk/include/javascript/JS_Object.h (right):

https://codereview.chromium.org/1384883002/diff/40001/fpdfsdk/include/javascr...
fpdfsdk/include/javascript/JS_Object.h:108: // CJS_Runtime::DestructionObserver
nit: It's just ::Observe, no???

https://codereview.chromium.org/1384883002/diff/40001/fpdfsdk/include/javascr...
fpdfsdk/include/javascript/JS_Object.h:120: const int m_nType;  // 0:Interval;
1:TimeOut
nit: struct probably packs tighter if the type and timeout are adjacent.
Personally, I'd put type first.

https://codereview.chromium.org/1384883002/diff/40001/fpdfsdk/src/javascript/...
File fpdfsdk/src/javascript/JS_Runtime.cpp (right):

https://codereview.chromium.org/1384883002/diff/40001/fpdfsdk/src/javascript/...
fpdfsdk/src/javascript/JS_Runtime.cpp:182: m_observers.insert(observer);
nit: do we want to assert it isn't already in there?

[Lei Zhang, 2015-10-04 23:01:28]

https://codereview.chromium.org/1384883002/diff/40001/fpdfsdk/include/javascr...
File fpdfsdk/include/javascript/JS_Object.h (right):

https://codereview.chromium.org/1384883002/diff/40001/fpdfsdk/include/javascr...
fpdfsdk/include/javascript/JS_Object.h:108: // CJS_Runtime::DestructionObserver
On 2015/10/04 22:39:37, Tom Sepez wrote:
> nit: It's just ::Observe, no???

Done, forgot to rename.

https://codereview.chromium.org/1384883002/diff/40001/fpdfsdk/include/javascr...
fpdfsdk/include/javascript/JS_Object.h:120: const int m_nType;  // 0:Interval;
1:TimeOut
On 2015/10/04 22:39:37, Tom Sepez wrote:
> nit: struct probably packs tighter if the type and timeout are adjacent.
> Personally, I'd put type first.

Done.

And this should be an enum, but that's another for day.

https://codereview.chromium.org/1384883002/diff/40001/fpdfsdk/src/javascript/...
File fpdfsdk/src/javascript/JS_Runtime.cpp (right):

https://codereview.chromium.org/1384883002/diff/40001/fpdfsdk/src/javascript/...
fpdfsdk/src/javascript/JS_Runtime.cpp:182: m_observers.insert(observer);
On 2015/10/04 22:39:38, Tom Sepez wrote:
> nit: do we want to assert it isn't already in there?

Sure. I put it as:

ASSERT(m_observers.find(observer) == m_observers.end());

because if I write it as:

bool added = m_observers.insert(observer).second;
ASSERT(added);

then I get compiler warnings about unused variables.

[Lei Zhang, 2015-10-04 23:01:57]

Committed patchset #2 (id:60001) manually as
794c9b67d3d519342aa7e15052766f7d4a99f551 (presubmit successful).

[Tom Sepez, 2015-10-07 17:00:31]

lgtm