fix(android): remove usage of getResourceAsStream() #11058

garymathews · 2019-07-17T23:57:59Z

Summary:

Remove usage of getResourceAsStream() as this method has high overhead
Utilize getResource() or getAsset() where appropriate

Test 1:
(This verifies we can still read our "build.properties" file.)

Create a default Classic Titanium app.
Build and run on Android.
Look for a log message like the below on startup. Verify that the "Powered by Titanium" version is 8.3.0 and not 1.0.0
<YourAppName> <YourAppVersion> (Powered by Titanium 8.3.0.2d9d45bfc7)

Test 2:
(This verifies that our WebView JS bindings still work.)

Build and run the below code on Android.
Wait for the WebView to load the appcelerator website.
Verify the following gets logged.
@@@ document.title: Home - Appcelerator | The Mobile First Platform

var lastLoadedTitle = null;
var window = Ti.UI.createWindow();
var webView = Ti.UI.createWebView({
	url: "https://appcelerator.com",
});
webView.addEventListener("load", function(e) {
	var loadedTitle = webView.evalJS("document.title;");
	if (loadedTitle !== lastLoadedTitle) {
		lastLoadedTitle = loadedTitle;
		Ti.API.info("@@@ document.title: " + loadedTitle);
	}
});
window.add(webView);
window.open();

Test 3:
(This verifies that our WebView JS bindings still work.)

Build and run the below code on Android.
Verify that you see a WebView which counts-down from 5 seconds.
Verify that a DOMCountentLoaded toast message appears at the bottom of the screen when the countdown reaches zero.

var htmlText =
		'<!DOCTYPE html>' +
		'<html>' +
		'	<head>' +
		'		<meta name="viewport" content="width=device-width, initial-scale=1.0">' +
		'	</head>' +
		'	<body>' +
		'		<p>HTML JavaScript Interop Test</p>' +
		'		<p id="label"></p>' +
		'	</body>' +
		'	<script>' +
		'		document.addEventListener("DOMContentLoaded", function(e) {' +
		'			Ti.API.info("@@@ Firing events from HTML");' +
		'			Ti.App.fireEvent("app:htmlLoaded", { isRequestingStartTimer: true });' +
		'			Ti.App.fireEvent("app:htmlRequestingShowToast", { message: "DOMContentLoaded" });' +
		'		});' +
		'		function updateLabelTo(x) {' +
		'			document.getElementById("label").innerHTML = x;' +
		'		}' +
		'	</script>' +
		'</html>';

var window = Ti.UI.createWindow();
var webView = Ti.UI.createWebView({
	html: htmlText,
});
window.add(webView);
window.open();

var timerId;
var timerCount;
function updateWebView() {
	timerCount = 5;
	function onUpdate() {
		if (timerCount > 0) {
			Ti.API.info("@@@ Calling evalJS()");
			webView.evalJS("updateLabelTo('Reload in:  " + timerCount + "')", function() {});
			timerCount--;
		} else {
			Ti.API.info("@@@ Reloading WebView");
			clearInterval(timerId);
			timerId = null;
			timerCount = 5;
			webView.html = htmlText;
		}
	}
	timerId = setInterval(function(e) {
		onUpdate();
	}, 1000);
	onUpdate();
}

Ti.App.addEventListener("app:htmlLoaded", function(e) {
	Ti.API.info("@@@ Received event 'htmlLoaded'");
	if (e.isRequestingStartTimer) {
		updateWebView();
	}
});

Ti.App.addEventListener("app:htmlRequestingShowToast", function(e) {
	if (Ti.App.Android || Ti.App.Windows) {
		if (e.message) {
			Ti.UI.createNotification({
				message: e.message,
				duration: Ti.UI.NOTIFICATION_DURATION_SHORT,
			}).show();
		}
	}
});

Test 4
(This verifies our TableView icons still work.)

const win = Ti.UI.createWindow({
    backgroundColor: 'gray'
});
const table = Ti.UI.createTableView({
    data: [
        {
            title: 'Apples',
            hasCheck: true
        },
        {
            title: 'Bananas',
            hasChild: true
        },
        {
            title: 'Carrots'
        },
        {
            title: 'Potatoes'
        }
    ]
});
win.add(table);
win.open();

android/modules/ui/src/java/ti/modules/titanium/ui/widget/tableview/TiBaseTableViewItem.java

android/build/common.xml

android/modules/ui/src/java/ti/modules/titanium/ui/widget/webview/TiWebViewBinding.java

jquick-axway · 2019-07-18T02:03:41Z

I've updated the PR's summary with test code that will test your changes.

I think the code which loads "ti:" URL schemes might be dead code. We only use a "ti:" scheme when assigning a "sourceURL" path to our JavaScript files. I've never seen us do this with drawables/images before. I think it's okay to keep your code changes just-in-case though.

garymathews · 2019-07-25T22:49:41Z

@jquick-axway Updated PR, thanks for the tests!

sgtcoolguy · 2019-07-26T19:33:16Z

@garymathews Got any timing data? Just curious, since you've made such a big perf boost in the snapshot stuff you've done...

jquick-axway

CR: Pass

jquick-axway · 2019-07-27T05:21:23Z

I just took some benchmarks.

Since every app is getting burned by us reading "build.properties" on startup, this is what I measured. I measured the old getResourceAsStream("build.properties") method call and this PR's KrollAssetHelper.openAsset("build.properties") method call that replaces it (which internally calls Google's openAsset() method).

Nexus 4:

Before PR: 300 ms
After PR: 0.678 ms
Result: 442.5x faster

Pixel 2:

Before PR: 16 ms
After PR: 0.180 ms
Result: 88.9x faster

It's so fast, I had to time it in nanoseconds to measure it properly. :)

The key thing here is this PR shaves off 299.4ms from the app startup time on our Nexus 4 (our slowest device). Gary's snapshot PR shaves off an additional 3057ms from the Nexus 4 startup time. Every bit helps. This is looking great and our launch times now feel on-par with Google's own major apps on the same device.

keerthi1032 · 2019-08-28T17:34:16Z

@garymathews I don't see the fix with sdk in this PR .with the testcase 1 im seeing powered by 1.0.n/a not 8.3 .for test case 2 &3 throws following error and for 4 table view appears different than GA.Can you please take a look

android/build/common.xml

android/titanium/src/java/org/appcelerator/titanium/TiApplication.java

sgtcoolguy

Looks like there's 2 changes that @jquick-axway mentions that need to be addressed before this can be FR'd

keerthi1032 · 2019-09-18T17:25:09Z

@garymathews I'm still seeing the same error as before #11058 (comment) can you please look at it

build · 2019-10-15T23:16:42Z

	Warnings
⚠️	Commit 12362357152ec21e59d3a74fbfd2dd8e10791a85 has a message "fix(android): TableView item image size" giving 1 errors: subject must not be sentence-case, start-case, pascal-case, upper-case

	Messages
📖	👍 Hey!, You deleted more code than you added. That's awesome!
📖	💾 Here's the generated SDK zipfile.
📖	✅ All tests are passing Nice one! All 4407 tests are passing. (There are 479 skipped tests not included in that total)
📖	🚨 This PR has one or more commits with warnings/errors for commit messages not matching our configuration. You may want to squash merge this PR and edit the message to match our conventions, or ask the original developer to modify their history.

Generated by 🚫 dangerJS against 1236235

ssekhri · 2019-10-21T21:10:26Z

@garymathews there is an issue with the PR related to tableviewRow height. Any tableview row with icons is shown with more height and also the icons are displayed quite big. Use the test code from Test4 in this PR.
Tested on:
Mac OS: 10.14.5
SDK: 8.3.0.v20191016113141
Appc CLI: 7.1.1
JDK: 1.8.0_162
Node: 10.5.0
Device: Nexus 4

garymathews · 2019-10-25T21:46:59Z

@ssekhri Pushed a fix

ssekhri · 2019-10-28T17:36:52Z

FR Passed.
Verified on:
Mac OS: 10.14.5
SDK: 8.3.0.v20191025142840
Appc CLI: 7.1.1
JDK: 1.8.0_162
Node: 10.5.0
Studio: 5.1.4.201909061933
Device: Nexus4(v5.1.1) device, Pixel3(v10.0) emulator

garymathews added android improvement no tests labels Jul 17, 2019

garymathews added this to the 8.3.0 milestone Jul 17, 2019

garymathews requested a review from jquick-axway July 17, 2019 23:57

build requested a review from a team July 18, 2019 00:20

jquick-axway reviewed Jul 18, 2019

View reviewed changes

jquick-axway mentioned this pull request Jul 18, 2019

build(android): include common bundle in snapshot #11054

Merged

garymathews force-pushed the TIMOB-26910 branch 2 times, most recently from b6e8ad8 to 61caa1b Compare July 25, 2019 21:06

jquick-axway approved these changes Jul 27, 2019

View reviewed changes

jquick-axway added the in-qe-testing 🕵 label Jul 27, 2019

garymathews mentioned this pull request Aug 8, 2019

refactor(android): asset handling #11045

Merged

garymathews force-pushed the TIMOB-26910 branch from 2e4c5e3 to 7ef062b Compare August 14, 2019 18:16

garymathews changed the title ~~fix(android): remove usage of getResourceAsStream()~~ fix(android)(8_3_X): remove usage of getResourceAsStream() Aug 14, 2019

garymathews changed the base branch from master to 8_3_X August 14, 2019 18:16

garymathews force-pushed the TIMOB-26910 branch from 3200e59 to fbcc0a7 Compare August 20, 2019 20:54

jquick-axway reviewed Aug 28, 2019

View reviewed changes

android/build/common.xml Outdated Show resolved Hide resolved

jquick-axway reviewed Sep 4, 2019

View reviewed changes

android/titanium/src/java/org/appcelerator/titanium/TiApplication.java Show resolved Hide resolved

sgtcoolguy changed the base branch from 8_3_X to master September 9, 2019 18:01

sgtcoolguy force-pushed the TIMOB-26910 branch from fde7bde to ab59314 Compare September 9, 2019 18:53

sgtcoolguy changed the title ~~fix(android)(8_3_X): remove usage of getResourceAsStream()~~ fix(android): remove usage of getResourceAsStream() Sep 9, 2019

sgtcoolguy self-requested a review September 10, 2019 13:46

sgtcoolguy removed the in-qe-testing 🕵 label Sep 10, 2019

sgtcoolguy suggested changes Sep 10, 2019

View reviewed changes

garymathews added the in-qe-testing 🕵 label Sep 11, 2019

sgtcoolguy approved these changes Sep 17, 2019

View reviewed changes

garymathews force-pushed the TIMOB-26910 branch 2 times, most recently from 09822d4 to ffb2d4c Compare October 15, 2019 22:36

garymathews force-pushed the TIMOB-26910 branch 2 times, most recently from 779d4ce to 81ce27c Compare October 16, 2019 18:23

garymathews force-pushed the TIMOB-26910 branch 2 times, most recently from 10db315 to f934825 Compare October 25, 2019 21:18

Gary Mathews added 2 commits October 25, 2019 14:19

fix(android): remove usage of getResourceAsStream()

fb61be3

fix(android): TableView item image size

1236235

garymathews force-pushed the TIMOB-26910 branch from f934825 to 1236235 Compare October 25, 2019 21:19

ssekhri merged commit a4c2b7a into tidev:master Oct 28, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(android): remove usage of getResourceAsStream() #11058

fix(android): remove usage of getResourceAsStream() #11058

garymathews commented Jul 17, 2019 •

edited

jquick-axway commented Jul 18, 2019 •

edited

garymathews commented Jul 25, 2019 •

edited

sgtcoolguy commented Jul 26, 2019

jquick-axway left a comment

jquick-axway commented Jul 27, 2019 •

edited

keerthi1032 commented Aug 28, 2019

sgtcoolguy left a comment

keerthi1032 commented Sep 18, 2019

build commented Oct 15, 2019 •

edited

ssekhri commented Oct 21, 2019

garymathews commented Oct 25, 2019

ssekhri commented Oct 28, 2019

fix(android): remove usage of getResourceAsStream() #11058

fix(android): remove usage of getResourceAsStream() #11058

Conversation

garymathews commented Jul 17, 2019 • edited

jquick-axway commented Jul 18, 2019 • edited

garymathews commented Jul 25, 2019 • edited

sgtcoolguy commented Jul 26, 2019

jquick-axway left a comment

Choose a reason for hiding this comment

jquick-axway commented Jul 27, 2019 • edited

keerthi1032 commented Aug 28, 2019

sgtcoolguy left a comment

Choose a reason for hiding this comment

keerthi1032 commented Sep 18, 2019

build commented Oct 15, 2019 • edited

ssekhri commented Oct 21, 2019

garymathews commented Oct 25, 2019

ssekhri commented Oct 28, 2019

garymathews commented Jul 17, 2019 •

edited

jquick-axway commented Jul 18, 2019 •

edited

garymathews commented Jul 25, 2019 •

edited

jquick-axway commented Jul 27, 2019 •

edited

build commented Oct 15, 2019 •

edited