Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[TIMOB-26038] Use JavaScriptCore framework API to implement proxies #10381

Merged
merged 22 commits into from Apr 8, 2019

Conversation

sgtcoolguy
Copy link
Contributor

@sgtcoolguy sgtcoolguy commented Oct 16, 2018

JIRA:

Description:
This is a technical spike that extends, cleans up, and formalizes what I was looking at in #10069

Basically, for some time now, Apple has had a supported mechanism for exposing native Obj-C classes/proxies to JavaScriptCore. It involves using protocols that extend from JSExport and then implementing those protocols in your classes. JavaScriptCore handles the grunt work of hooking up the methods/properties and converting types for you.

This PR begins migrating some of our proxies over using this mechanism and includes macros to simplify doing so, methods to handle dealing with conversions between new and old proxy types.

Some of the lessons learned:

  • This simplifies the code quite a bit, but technically we could improve performance even more for constant properties by using the C API to assigns static values. (This and our old way basically use getters to wrap method calls to grab the property values).
  • Ti.Buffer is special and allows indexed access to the contents. The new API has no means of doing this.
  • When we expect to receive an "old-style" proxy, I had to typically hack the API a little to expect either id or JSValue* so that I can convert it. The JSC API doesn't know how to unwrap the old style proxies automatically itself. But as we move more proxies to the "new" style, we could do less of this and actually specify proper types.

@sgtcoolguy
Copy link
Contributor Author

Note that I spent a lot of time/effort in making the commits/history super clean here. If we do merge this, it should be merged as "rebase and merge". Also, it should make it easier to review changes. The first commit lays all the ground work for hooking the new style proxies and uses them for a few core modules. The following commits are for converting other top-level modules over one by one.

@sgtcoolguy sgtcoolguy changed the title Use JavaScriptCore framework API to implement proxies [TIMOB-26038] Use JavaScriptCore framework API to implement proxies Oct 17, 2018
@sgtcoolguy
Copy link
Contributor Author

sgtcoolguy commented Oct 17, 2018

Interesting to note that the test for expecting a native error to be thrown when trying to set Ti.Geolocation.accuracy to null is failing because it no longer throws an Error! I'm curious how it's handling trying to convert null over under the hood (since the method/property is assigned the type CLLocationAccuracy which is a double).

Update: Turns out it treats null as 0, which I guess makes sense as Number(null) == 0.
And more fun: setting to 'abc' converts to NaN in JS and Obj-c worlds. So this still doesn't throw an Error unless we specifically do some checking natively ourselves here that !isnan(value); (https://stackoverflow.com/questions/719417/determine-if-nsnumber-is-nan)

So, I guess lesson to take away here is: moving this way leads to more liberal type coercion that matches JS spec (so is probably more correct) - so should probably also involve additional checking natively by us to throw exceptions if there's specific oddball values we don't want to allow like NaN, or values outside enumerated values, or limiting to integers/positive/negative, etc.

@sgtcoolguy sgtcoolguy changed the base branch from next to master October 30, 2018 18:54
@sgtcoolguy sgtcoolguy force-pushed the ios-objc-api branch 2 times, most recently from ebfbdf8 to a695e7e Compare October 31, 2018 20:53
@janvennemann
Copy link
Contributor

@sgtcoolguy What's the plan here on how much you want to move over to JSC? All of it immediately? Just a bunch of our proxies at first that are easiest to convert? Will we be able to completely replace Kroll with this?

I also started some experimental work with a new Hyperloop version that tries to use JSC as much as possible and integrates our metabase to workaround the runtime restriction of JSC. Would be a perfect complement to this.

@sgtcoolguy
Copy link
Contributor Author

@janvennemann My initial goal was to explore this as a tech spike, and I think the earlier PR and Han's feedback sealed that this looked like a promising direction to go.

So for now, I'm iterating on this and attempting to get a good subset of the proxies moved over and still have the unit tests pass (and a nice clean commit history). I think once I've got a substantial set migrated (say, the modules already done above, plus Ti.Blob and Ti.Filesystem?) and the tests all pass, I'll put this PR up for merge review.

It's really up to us, @vijaysingh-axway and @mukherjee2 to decide on the sort of timeframe for this. My goal was to allow us to move this way "silently" (i.e. without impacting native modules or introducing any noticeable changes). If we can do so, then the first chunk might be able to land in 8.0.0, and we could continue to migrate more as we went. I think there's just so many APIs, it's not entirely feasible to move everything over at once.

@janvennemann
Copy link
Contributor

Partly moving over to this for 8.0.0 sounds good. Maybe we can even deprecate the "old" way of creating modules and proxies in favor of the new JSC way with 8.0.0.

@sgtcoolguy
Copy link
Contributor Author

So, this PR is growing quite a bit - even if I'm trying to be good about structuring the commits. I think it'd be good to get view from @janvennemann and @vijaysingh-axway now and try and get this chunk in.

The only thing I worry about is the refactoring obviously changes the type hierarchy of these proxies. I think generally that should be ok as move native modules don't actually use any of these types, but TiBlob/TiFile might be used by them. So perhaps I should hold onto my TiBlob conversion here and we do that later? Or maybe, we say "oh well" and merge it in to 8.0.0 but don't bump the module api version because it's probably pretty unlikely many (or possibly any) native modules use TiBlob?

sgtcoolguy referenced this pull request Nov 6, 2018
…r proxies

* Update docs to fix incorrect info regarding properties that should be read-only, or incorrect types. Also update example code in docs.
* Remove methods from Ti.Calendar.Calendar that were deprecated in 7.0.0. This includes #getEventsInDate(), despite Android/docs not noting it as deprecated - iOS did report it as deprecated.
* Remove unused Ti.Calendar.Reminder type from iOS
* Return begin/end properties as Date objects on Ti.Calendar.Event, and Ti.Calendar.RecurrenceRule.end's endDate property. Docs/Android used Date, but iOS used String.
Copy link
Contributor

@janvennemann janvennemann left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Alright, i added a few notes, suggestions and questions. Man, this thing is a beast!

iphone/TitaniumKit/TitaniumKit/Sources/API/KrollBridge.h Outdated Show resolved Hide resolved
iphone/Classes/TiNetworkHTTPClientProxy.h Outdated Show resolved Hide resolved
iphone/Classes/TiNetworkHTTPClientProxy.h Outdated Show resolved Hide resolved
iphone/Classes/TiMediaItem.h Outdated Show resolved Hide resolved
iphone/TitaniumKit/TitaniumKit/Sources/API/KrollBridge.m Outdated Show resolved Hide resolved
iphone/TitaniumKit/TitaniumKit/Sources/API/KrollBridge.m Outdated Show resolved Hide resolved
iphone/TitaniumKit/TitaniumKit/Sources/API/ObjcProxy.h Outdated Show resolved Hide resolved
iphone/TitaniumKit/TitaniumKit/Sources/API/ObjcProxy.m Outdated Show resolved Hide resolved
Copy link
Contributor

@janvennemann janvennemann left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added another round of feedback

iphone/Classes/CalendarModule.m Outdated Show resolved Hide resolved
iphone/Classes/CodecModule.m Show resolved Hide resolved
// Need to use JSManagedValue here!
JSManagedValue *managedValue = [JSManagedValue managedValueWithValue:callback andOwner:self];
[callback.context.virtualMachine addManagedReference:managedValue withOwner:self];
[singleHeading addObject:managedValue];
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Calling managedValueWithValue:andOwner: will already register the object with the virtual machine so explicitly calling addManagedReference:withOwner: is unnecessary.

Have you checked that you really need managed values here? Adding the callback to an array should retain it and keep it's JSValueRef from beeing GC'ed

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's entirely possible we don't need a managed value here...

iphone/Classes/GestureModule.h Outdated Show resolved Hide resolved
iphone/Classes/PlatformModule.m Outdated Show resolved Hide resolved
iphone/Classes/TiCalendarAlert.h Outdated Show resolved Hide resolved
…to JavaScriptCore Obj-c based API for proxies

Use a base ObjcProxy class for all new-style proxies
* Add helper methods for wrapping/unwrapping 'old style' proxies
* Add helper methods around events/listeners to base proxy class.
* Also add macros to make life simpler for declaring properties with deprecated getter/setter accessor methods
…ew JavaScriptCore Obj-c based API for proxies
…ityIndicatorStyleProxy.m, TiUIiOSLivePhoto.m
…ased API for proxies

* Update some incorrect docs for the API
* Enable lastGeolocation property test
…I for proxies

- Most of the Ti.Blob suite passes now (#toString() override doesn't work)
- Avoid Ti.Blob#toString(), use Ti.Blob.text in test suite
…-c based API for proxies

* Update docs to fix incorrect info regarding properties that should be read-only, or incorrect types. Also update example code in docs.
* Remove methods from Ti.Calendar.Calendar that were deprecated in 7.0.0. This includes #getEventsInDate(), despite Android/docs not noting it as deprecated - iOS did report it as deprecated.
* Remove unused Ti.Calendar.Reminder type from iOS
* Return begin/end properties as Date objects on Ti.Calendar.Event, and Ti.Calendar.RecurrenceRule.end's endDate property. Docs/Android used Date, but iOS used String.
…es/defaults on JSValue* object arguments to methods
…e error on the JSContext* exception property and return nil immediately.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants