Chromium Code Reviews
Help | Chromium Project | Gerrit Changes | Sign in
(16)

Issue 1353193004: Add signatures to FXJS_V8. (Closed)

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

Description

Patch Set 1 #

Total comments: 2

Patch Set 2 : Rebase, spelling of Global #

Patch Set 3 : Bad cast for perisolatedata #

Patch Set 4 : New Tests #

Patch Set 5 : Fix calling this.method() #

Total comments: 6

Patch Set 6 : Use instance template only. #

Unified diffs Side-by-side diffs Delta from patch set Stats (+154 lines, -100 lines) Patch
M fpdfsdk/src/jsapi/fxjs_v8.cpp View 1 2 3 4 5 13 chunks +71 lines, -100 lines 0 comments Download
A testing/resources/javascript/apply.in View 1 2 3 1 chunk +75 lines, -0 lines 0 comments Download
A testing/resources/javascript/apply_expected.txt View 1 2 3 1 chunk +8 lines, -0 lines 0 comments Download

Messages

Total messages: 22 (3 generated)
Tom Sepez
Jochen, this is my best guess as how we would use signatures in PDFium, based ...
4 years, 2 months ago (2015-09-18 22:14:54 UTC) #2
Tom Sepez
On 2015/09/18 22:14:54, Tom Sepez wrote: > Jochen, this is my best guess as how ...
4 years, 2 months ago (2015-09-22 15:12:08 UTC) #3
krasin
Hi jochen, this CL resolves a hard blocker for Control Flow Integrity launch on Linux, ...
4 years, 2 months ago (2015-09-22 22:10:28 UTC) #5
jochen (gone - plz use gerrit)
lgtm https://codereview.chromium.org/1353193004/diff/1/fpdfsdk/src/jsapi/fxjs_v8.cpp File fpdfsdk/src/jsapi/fxjs_v8.cpp (right): https://codereview.chromium.org/1353193004/diff/1/fpdfsdk/src/jsapi/fxjs_v8.cpp#newcode21 fpdfsdk/src/jsapi/fxjs_v8.cpp:21: static v8::Global<v8::ObjectTemplate> g_DefaultGloabalObjectTemplate; spelling of Global
4 years, 2 months ago (2015-09-25 11:30:19 UTC) #6
krasin
rslgtm
4 years, 2 months ago (2015-09-28 17:48:42 UTC) #7
krasin
lgtm
4 years, 2 months ago (2015-09-28 17:48:53 UTC) #8
Tom Sepez
https://codereview.chromium.org/1353193004/diff/1/fpdfsdk/src/jsapi/fxjs_v8.cpp File fpdfsdk/src/jsapi/fxjs_v8.cpp (right): https://codereview.chromium.org/1353193004/diff/1/fpdfsdk/src/jsapi/fxjs_v8.cpp#newcode21 fpdfsdk/src/jsapi/fxjs_v8.cpp:21: static v8::Global<v8::ObjectTemplate> g_DefaultGloabalObjectTemplate; On 2015/09/25 11:30:19, jochen wrote: > ...
4 years, 2 months ago (2015-09-28 18:13:38 UTC) #9
Tom Sepez
Note to self: JS tests now failing, as if the check is over-zealous: -Alert: PASS: ...
4 years, 2 months ago (2015-09-28 18:18:19 UTC) #10
Tom Sepez
PS#5 fixes: > -Alert: PASS: this.removeField() threw error Document.removeField: Incorrect > number of parameters passed ...
4 years, 2 months ago (2015-09-28 19:49:56 UTC) #11
jochen (gone - plz use gerrit)
https://codereview.chromium.org/1353193004/diff/80001/fpdfsdk/src/jsapi/fxjs_v8.cpp File fpdfsdk/src/jsapi/fxjs_v8.cpp (right): https://codereview.chromium.org/1353193004/diff/80001/fpdfsdk/src/jsapi/fxjs_v8.cpp#newcode70 fpdfsdk/src/jsapi/fxjs_v8.cpp:70: v8::Local<v8::Signature> sig = v8::Signature::New(pIsolate, fun); you appear to put ...
4 years, 2 months ago (2015-09-29 07:30:47 UTC) #12
Tom Sepez
https://codereview.chromium.org/1353193004/diff/80001/fpdfsdk/src/jsapi/fxjs_v8.cpp File fpdfsdk/src/jsapi/fxjs_v8.cpp (right): https://codereview.chromium.org/1353193004/diff/80001/fpdfsdk/src/jsapi/fxjs_v8.cpp#newcode70 fpdfsdk/src/jsapi/fxjs_v8.cpp:70: v8::Local<v8::Signature> sig = v8::Signature::New(pIsolate, fun); On 2015/09/29 07:30:47, jochen ...
4 years, 2 months ago (2015-09-29 15:50:34 UTC) #13
jochen (gone - plz use gerrit)
https://codereview.chromium.org/1353193004/diff/80001/fpdfsdk/src/jsapi/fxjs_v8.cpp File fpdfsdk/src/jsapi/fxjs_v8.cpp (right): https://codereview.chromium.org/1353193004/diff/80001/fpdfsdk/src/jsapi/fxjs_v8.cpp#newcode64 fpdfsdk/src/jsapi/fxjs_v8.cpp:64: fun->PrototypeTemplate()->SetInternalFieldCount(2); this shouldn't be required (setting it on the ...
4 years, 2 months ago (2015-09-30 08:30:20 UTC) #14
Tom Sepez
Ah, that does the trick. Jochen, please take a final look. Thanks!
4 years, 2 months ago (2015-09-30 17:17:25 UTC) #15
jochen (gone - plz use gerrit)
still lgtm
4 years, 2 months ago (2015-09-30 21:07:59 UTC) #16
Tom Sepez
Committed patchset #6 (id:100001) manually as 158e335717efba9dce3aa6f6d1e31ed884e1f59e (presubmit successful).
4 years, 2 months ago (2015-09-30 22:40:02 UTC) #17
phoglund_chromium
On 2015/09/30 22:40:02, Tom Sepez wrote: > Committed patchset #6 (id:100001) manually as > 158e335717efba9dce3aa6f6d1e31ed884e1f59e ...
4 years, 2 months ago (2015-10-01 12:16:42 UTC) #18
phoglund_chromium
4 years, 2 months ago (2015-10-01 12:17:05 UTC) #20
Tom Sepez
On 2015/10/01 12:17:05, phoglund_chrome wrote: Argh, yes obviously, static v8:Local<> won't cut it.
4 years, 2 months ago (2015-10-01 15:38:14 UTC) #21
jochen (gone - plz use gerrit)
4 years, 2 months ago (2015-10-01 15:40:04 UTC) #22
Message was sent while issue was closed.
On 2015/10/01 at 15:38:14, tsepez wrote:
> On 2015/10/01 12:17:05, phoglund_chrome wrote:
> 
> Argh, yes obviously, static v8:Local<> won't cut it.

Maybe just put the Global into FXJSPerIsolateData?

Powered by Google App Engine
This is Rietveld 408576698