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

Issues Search
    My Issues | Starred     Open | Closed | All

Issue 1389163007: Pass IJS_Runtime, not IJS_Context, to native object constructors. (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

Pass IJS_Runtime, not IJS_Context, to native object constructors. This better separates the two IJS_ classes, with the IJS_Context taking on its proper role of describing an event. There's no need for the event details for object creation, so this gets much cleaner. Move some JS error reporting code from CJS_Context to CJS_Runtime. Make InitInstance() and ExitInstance() voids, they always return TRUE and we never check the result anyways. R=thestig@chromium.org Committed: https://pdfium.googlesource.com/pdfium/+/848a13b6777cbff8cc4aea3ab3d69eaa0b82ae6c

Patch Set 1 #

Patch Set 2 : Make InitInstance() void. #

Patch Set 3 : comment #

Total comments: 8

Patch Set 4 : Nits, no pass-by-non-const-ref #

Total comments: 1

Patch Set 5 : Kill dead code. #

Patch Set 6 : Same for fpdfsdk/src/fsdk_baseform.cpp #

Patch Set 7 : rebased #

Patch Set 8 : Merge changes into stub. #

Unified diffs Side-by-side diffs Delta from patch set Stats (+115 lines, -180 lines) Patch
M fpdfsdk/include/javascript/IJavaScript.h View 1 2 3 4 5 6 3 chunks +6 lines, -1 line 0 comments Download
M fpdfsdk/include/jsapi/fxjs_v8.h View 1 2 3 4 chunks +7 lines, -9 lines 0 comments Download
M fpdfsdk/src/fsdk_actionhandler.cpp View 1 2 3 4 6 chunks +12 lines, -73 lines 0 comments Download
M fpdfsdk/src/fsdk_baseform.cpp View 1 2 3 4 5 5 chunks +4 lines, -6 lines 0 comments Download
M fpdfsdk/src/javascript/Document.h View 1 1 chunk +1 line, -1 line 0 comments Download
M fpdfsdk/src/javascript/Document.cpp View 1 7 chunks +11 lines, -17 lines 0 comments Download
M fpdfsdk/src/javascript/Field.h View 1 1 chunk +1 line, -1 line 0 comments Download
M fpdfsdk/src/javascript/Field.cpp View 1 3 chunks +6 lines, -12 lines 0 comments Download
M fpdfsdk/src/javascript/JS_Context.h View 1 2 3 1 chunk +1 line, -1 line 0 comments Download
M fpdfsdk/src/javascript/JS_Context.cpp View 1 2 3 2 chunks +7 lines, -11 lines 0 comments Download
M fpdfsdk/src/javascript/JS_Define.h View 1 chunk +8 lines, -8 lines 0 comments Download
M fpdfsdk/src/javascript/JS_EventHandler.cpp View 2 chunks +4 lines, -4 lines 0 comments Download
M fpdfsdk/src/javascript/JS_Object.h View 1 1 chunk +2 lines, -2 lines 0 comments Download
M fpdfsdk/src/javascript/JS_Runtime.h View 1 2 3 1 chunk +3 lines, -0 lines 0 comments Download
M fpdfsdk/src/javascript/JS_Runtime.cpp View 1 2 3 4 5 6 2 chunks +13 lines, -1 line 0 comments Download
M fpdfsdk/src/javascript/JS_Runtime_Stub.cpp View 1 2 3 4 5 6 7 2 chunks +7 lines, -1 line 0 comments Download
M fpdfsdk/src/javascript/app.cpp View 1 2 3 4 chunks +4 lines, -7 lines 0 comments Download
M fpdfsdk/src/javascript/global.h View 1 1 chunk +1 line, -1 line 0 comments Download
M fpdfsdk/src/javascript/global.cpp View 1 1 chunk +6 lines, -11 lines 0 comments Download
M fpdfsdk/src/jsapi/fxjs_v8.cpp View 1 2 3 6 chunks +9 lines, -11 lines 0 comments Download
M fpdfsdk/src/jsapi/fxjs_v8_embeddertest.cpp View 1 2 3 2 chunks +2 lines, -2 lines 0 comments Download

Messages

Total messages: 11 (1 generated)
Tom Sepez
Lei, for review. I think this makes things more sensible.
5 years, 2 months ago (2015-10-08 23:25:22 UTC) #2
Lei Zhang
roll -> role
5 years, 2 months ago (2015-10-08 23:45:48 UTC) #3
Tom Sepez
On 2015/10/08 23:45:48, Lei Zhang wrote: > roll -> role Geez. Done.
5 years, 2 months ago (2015-10-08 23:47:01 UTC) #4
Lei Zhang
lgtm https://codereview.chromium.org/1389163007/diff/40001/fpdfsdk/include/javascript/IJavaScript.h File fpdfsdk/include/javascript/IJavaScript.h (right): https://codereview.chromium.org/1389163007/diff/40001/fpdfsdk/include/javascript/IJavaScript.h#newcode19 fpdfsdk/include/javascript/IJavaScript.h:19: // Records the details of an event and ...
5 years, 2 months ago (2015-10-08 23:52:01 UTC) #5
Tom Sepez
https://codereview.chromium.org/1389163007/diff/40001/fpdfsdk/include/javascript/IJavaScript.h File fpdfsdk/include/javascript/IJavaScript.h (right): https://codereview.chromium.org/1389163007/diff/40001/fpdfsdk/include/javascript/IJavaScript.h#newcode19 fpdfsdk/include/javascript/IJavaScript.h:19: // Records the details of an event and causes ...
5 years, 2 months ago (2015-10-09 00:14:20 UTC) #6
Tom Sepez
Please take another look. Changes percolated into other files.
5 years, 2 months ago (2015-10-09 00:15:31 UTC) #7
Lei Zhang
lgtm https://codereview.chromium.org/1389163007/diff/60001/fpdfsdk/src/fsdk_actionhandler.cpp File fpdfsdk/src/fsdk_actionhandler.cpp (right): https://codereview.chromium.org/1389163007/diff/60001/fpdfsdk/src/fsdk_actionhandler.cpp#newcode352 fpdfsdk/src/fsdk_actionhandler.cpp:352: // CBCL_FormNotify::MsgBoxJSError(pPageView->GetPageViewWnd(), I wish I didn't see this. ...
5 years, 2 months ago (2015-10-09 01:09:29 UTC) #8
Tom Sepez
On 2015/10/09 01:09:29, Lei Zhang wrote: > lgtm > > https://codereview.chromium.org/1389163007/diff/60001/fpdfsdk/src/fsdk_actionhandler.cpp > File fpdfsdk/src/fsdk_actionhandler.cpp (right): ...
5 years, 2 months ago (2015-10-09 17:25:13 UTC) #9
Lei Zhang
++lgtm
5 years, 2 months ago (2015-10-09 18:06:03 UTC) #10
Tom Sepez
5 years, 2 months ago (2015-10-09 20:14:51 UTC) #11
Message was sent while issue was closed. 
Committed patchset #8 (id:2) manually as
848a13b6777cbff8cc4aea3ab3d69eaa0b82ae6c (presubmit successful).

Powered by Google App Engine
This is Rietveld 408576698