Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(167)

Issues Search
    My Issues | Starred     Open | Closed | All

Issue 1382263002: Store object definition ID in each js_class. (Closed)

Created:
5 years, 2 months ago by Tom Sepez
Modified:
5 years, 2 months ago
Reviewers:
Lei Zhang
CC:
pdfium-reviews_googlegroups.com
Base URL:
https://pdfium.googlesource.com/pdfium.git@master
Target Ref:
refs/heads/master
Visibility:
Public.

Description

Store object definition ID in each js_class. Avoids doing a lookup via FXJS_V8 for something already known in CJS layer. Also: Consolidate repeated code in JS macros. Remove knowledge that Document is global from FXJS layer R=thestig@chromium.org Committed: https://pdfium.googlesource.com/pdfium/+/f0a5b2803c09f3605dcd606e764ef604f0d2a8ea

Patch Set 1 : Pass 1 #

Patch Set 2 : De-duplicate code by breaking into parts. #

Patch Set 3 : Remove lookup by name #

Total comments: 1

Patch Set 4 : Rebased #

Total comments: 19

Patch Set 5 : Nits. #

Patch Set 6 : More nits. #

Patch Set 7 : Rebase past massive file move. #

Patch Set 8 : Use size_t vars with FX_ArraySize. #

Unified diffs Side-by-side diffs Delta from patch set Stats (+284 lines, -358 lines) Patch
M fpdfsdk/include/jsapi/fxjs_v8.h View 1 2 3 2 chunks +3 lines, -3 lines 0 comments Download
M fpdfsdk/src/javascript/Document.h View 1 2 3 4 5 6 2 chunks +2 lines, -2 lines 0 comments Download
M fpdfsdk/src/javascript/Document.cpp View 1 2 3 4 5 6 7 chunks +8 lines, -20 lines 0 comments Download
M fpdfsdk/src/javascript/Field.h View 1 2 3 4 5 6 1 chunk +1 line, -2 lines 0 comments Download
M fpdfsdk/src/javascript/Field.cpp View 1 2 3 4 5 6 2 chunks +4 lines, -6 lines 0 comments Download
M fpdfsdk/src/javascript/Icon.h View 1 2 3 4 5 6 1 chunk +1 line, -3 lines 0 comments Download
M fpdfsdk/src/javascript/JS_Define.h View 1 2 3 4 5 6 7 7 chunks +167 lines, -161 lines 0 comments Download
M fpdfsdk/src/javascript/JS_EventHandler.cpp View 1 2 3 4 5 6 2 chunks +12 lines, -14 lines 0 comments Download
M fpdfsdk/src/javascript/JS_Runtime.cpp View 1 2 3 4 5 6 3 chunks +21 lines, -22 lines 0 comments Download
M fpdfsdk/src/javascript/app.h View 1 2 3 4 5 6 2 chunks +2 lines, -2 lines 0 comments Download
M fpdfsdk/src/javascript/app.cpp View 1 2 3 4 5 6 5 chunks +32 lines, -62 lines 0 comments Download
M fpdfsdk/src/javascript/color.h View 1 2 3 4 5 6 1 chunk +1 line, -1 line 0 comments Download
M fpdfsdk/src/javascript/console.h View 1 2 3 4 5 6 1 chunk +1 line, -1 line 0 comments Download
M fpdfsdk/src/javascript/event.h View 1 2 3 4 5 6 1 chunk +1 line, -2 lines 0 comments Download
M fpdfsdk/src/javascript/global.h View 1 2 3 4 5 6 1 chunk +1 line, -2 lines 0 comments Download
M fpdfsdk/src/javascript/report.h View 1 2 3 4 5 6 1 chunk +1 line, -2 lines 0 comments Download
M fpdfsdk/src/javascript/util.h View 1 2 3 4 5 6 1 chunk +1 line, -1 line 0 comments Download
M fpdfsdk/src/jsapi/fxjs_v8.cpp View 1 2 3 4 5 6 chunks +25 lines, -52 lines 0 comments Download

Messages

Total messages: 8 (1 generated)
Tom Sepez
Lei, for review. Probably want to start at PS2, then review the diff to PS3.
5 years, 2 months ago (2015-10-02 22:38:29 UTC) #2
Tom Sepez
https://codereview.chromium.org/1382263002/diff/40001/fpdfsdk/include/javascript/JS_Define.h File fpdfsdk/include/javascript/JS_Define.h (left): https://codereview.chromium.org/1382263002/diff/40001/fpdfsdk/include/javascript/JS_Define.h#oldcode194 fpdfsdk/include/javascript/JS_Define.h:194: static JSConstSpec JS_Class_Consts[]; \ note: we make this array ...
5 years, 2 months ago (2015-10-02 22:58:12 UTC) #3
Lei Zhang
lgtm https://codereview.chromium.org/1382263002/diff/60001/fpdfsdk/include/javascript/JS_Define.h File fpdfsdk/include/javascript/JS_Define.h (right): https://codereview.chromium.org/1382263002/diff/60001/fpdfsdk/include/javascript/JS_Define.h#newcode186 fpdfsdk/include/javascript/JS_Define.h:186: /* ========================================= JS_CLASS Would you mind fixing these ...
5 years, 2 months ago (2015-10-06 01:15:21 UTC) #4
Tom Sepez
https://codereview.chromium.org/1382263002/diff/60001/fpdfsdk/include/javascript/JS_Define.h File fpdfsdk/include/javascript/JS_Define.h (right): https://codereview.chromium.org/1382263002/diff/60001/fpdfsdk/include/javascript/JS_Define.h#newcode186 fpdfsdk/include/javascript/JS_Define.h:186: /* ========================================= JS_CLASS On 2015/10/06 01:15:21, Lei Zhang wrote: ...
5 years, 2 months ago (2015-10-06 15:33:20 UTC) #5
Tom Sepez
Lei, let me know if you're ok with me removing the ===================== perforations entirely [makes ...
5 years, 2 months ago (2015-10-06 15:37:21 UTC) #6
Tom Sepez
Committed patchset #8 (id:140001) manually as f0a5b2803c09f3605dcd606e764ef604f0d2a8ea (presubmit successful).
5 years, 2 months ago (2015-10-06 18:10:57 UTC) #7
Lei Zhang
5 years, 2 months ago (2015-10-06 18:16:17 UTC) #8
Message was sent while issue was closed. 
https://codereview.chromium.org/1382263002/diff/60001/fpdfsdk/include/javascr...
File fpdfsdk/include/javascript/JS_Define.h (right):

https://codereview.chromium.org/1382263002/diff/60001/fpdfsdk/include/javascr...
fpdfsdk/include/javascript/JS_Define.h:186: /*
========================================= JS_CLASS
On 2015/10/06 15:33:20, Tom Sepez wrote:
> On 2015/10/06 01:15:21, Lei Zhang wrote:
> > Would you mind fixing these since you are already touching it? They were
meant
> > to be an one line ruler, but clang-format has broken it in two.
> 
> Done. Throughout. I don't think these added much value.

Removing is fine too.

https://codereview.chromium.org/1382263002/diff/60001/fpdfsdk/include/javascr...
fpdfsdk/include/javascript/JS_Define.h:222: for (int i = 0; i <
FX_ArraySize(JS_Class_Consts) - 1; ++i) {     \
On 2015/10/06 15:33:19, Tom Sepez wrote:
> On 2015/10/06 01:15:21, Lei Zhang wrote:
> > Can we get rid of the {0, 0, 0, 0} entry in END_JS_STATIC_CONST() and remove
> the
> > subtract 1 here? Ditto with END_JS_STATIC_PROP() and END_JS_STATIC_METHOD().
> 
> 
> Sadly, no because there is the possibility of classes which don't declare any
of
> these, and you wind up with a zero-sized array, and that then fails to
compile.

Too bad.

https://codereview.chromium.org/1382263002/diff/60001/fpdfsdk/src/jsapi/fxjs_...
File fpdfsdk/src/jsapi/fxjs_v8.cpp (right):

https://codereview.chromium.org/1382263002/diff/60001/fpdfsdk/src/jsapi/fxjs_...
fpdfsdk/src/jsapi/fxjs_v8.cpp:89: const wchar_t* m_ObjName;
On 2015/10/06 15:33:20, Tom Sepez wrote:
> On 2015/10/06 01:15:21, Lei Zhang wrote:
> > const wchar_t* const?
> Why not? Done.

So the pointer can't accidentally change either. The constest!

Powered by Google App Engine
This is Rietveld 408576698