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-4706: Android: Cannot debug an application while using require('some_js_module') #379

Merged
merged 6 commits into from Aug 26, 2011

Conversation

marshall
Copy link
Contributor

http://jira.appcelerator.org/browse/TIMOB-4706

This also has a Rhino component / pull request:
https://github.com/appcelerator/rhino_titanium/pull/3

Repro steps are in the ticket

while in a thread associated with a context. some code cleanup in
TitaniumModule. TIMOB-4706
@nataliehuynh
Copy link
Contributor

Tested with 1.8.0.1761813, no longer encountering crash with debug. Request accepted

@billdawson
Copy link
Contributor

Code review and functional test Accepted. Was able to re-create failcase from the JIRA ticket on master, then with this branch there was no failure.

// NOTE: commonjs modules load absolute to root in Titanium
String fileUrl = "app://"+path+".js";
// NOTE: CommonJS modules load absolute to app:// in Titanium
String fileUrl = "app://" + path + ".js";
Copy link
Contributor

Choose a reason for hiding this comment

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

Should use StringBuilder for more than 1 concat. Also, don't we have a constant for "app://" somewhere?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, we have it in constant form -- I'll use that + StringBuilder

…serAgent

and require. fixed a class cast exception bug for module exports in the
process, thank you unit tests! TIMOB-4706
@billdawson
Copy link
Contributor

Code re-review and functional re-test Accepted. Functional test via the original failcase in JIRA item, as well as the drillbit tests.

// uri is optional but we point it to where we loaded it
proxy.setProperty("uri",fileUrl);
return proxy;
Log.d(LCAT, "Attempting to include JS module: " + tbf.nativePath());
Copy link
Contributor

Choose a reason for hiding this comment

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

This log statement should be upgraded to INFO or wrapped in if (DBG){}

@billdawson
Copy link
Contributor

Accepted based on another code review and functional test, the latter particularly because Marshall needed to resolve a merge conflict based on a binary file (the js.jar). To be safe I also re-did the functional test of TIMOB-3756 to make sure the merge went "backwards" successfully as well.

@donthorp
Copy link
Contributor

Code Reviewed. Request Accepted.

donthorp added a commit that referenced this pull request Aug 26, 2011
TIMOB-4706: Android: Cannot debug an application while using require('some_js_module')
@donthorp donthorp merged commit ba166dd into tidev:master Aug 26, 2011
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants