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-25859] Android: Delay WebView Ti.App.fireEvent() to be fired after page load #9931

Closed
wants to merge 3 commits into from

Conversation

jquick-axway
Copy link
Contributor

JIRA: https://jira.appcelerator.org/browse/TIMOB-25859

Summary:

  • Events fired from HTML are now queued while the page is loading and are fired when page finishes loading. This is needed because evalJS() will fail if page hasn't finished loading.
  • Updated evalJS() documentation to state that it'll only work after the "load" event.
  • Added WebView unit test "html-interop".

Test:

  1. Set up "tiapp.xml" property "run-on-main-thread" to true.
  2. Build and run "WebViewInteropTest.js" attached to TIMOB-25859 on Android.
  3. Verify that you see a countdown in the WebView, starting from 5.
  4. Verify that a toast message appears when it counts to zero, the web page reloads, and the countdown resets back to 5.
  5. Look at the Android log and verify that no Timeout waiting to evaluate JS warning messages appear.

- Updated EvalJS() documentation to indicate it can't be called reliably until after "load" event.
}

// Queue event and fire it, if ready.
synchronized (this)
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think the synchronized is necessary?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This method can be called by the WebView's JavaScript thread (via @JavaScriptInterface APIs) or the main UI thread (via resumeFiringEvents() calls). So, the synchronized is needed. And we shouldn't invoke a listener within a synchronized block to avoid potential deadlock.

} catch (JSONException e) {
Log.e(TAG, "Error parsing event JSON", e);
}
}

public void fireQueuedEvents()
{
while (true) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe just use the Iterator like this:

if (!isEventFiringSuspended) {
	for (final Event event : this.eventQueue) {
		module.fireEvent(event.name, event.properties);
		this.eventQueue.remove();
	}
}

@lokeshchdhry
Copy link
Contributor

lokeshchdhry commented May 1, 2018

@jquick-axway , I am seeing an issue where if I have soasta enabled then I do see the Timeout waiting to evaluate JS warning, not otherwise on a Nexus 6P running android 8.0.

Full log: https://gist.github.com/lokeshchdhry/b0e1755bcccdf77e2dc8f2d8422557a0

@jquick-axway
Copy link
Contributor Author

@lokeshchdhry, we still have a timeout for evalJS() which will be triggered if the WebView doesn't respond in time. I know this will still happen if the HTML is super huge. It can also happen if the UI thread is very busy. Perhaps adding soasta has added enough overhead on app startup that it's not responding in time on a slow device.

@sgtcoolguy sgtcoolguy modified the milestones: 7.2.0, 7.3.0 May 16, 2018
@keerthi1032
Copy link
Contributor

Waiting for resolving conflicts

@jquick-axway
Copy link
Contributor Author

This PR may be obsolete due to our new async WebView.evalJS() feature here: #9889

I'll re-test this with our new async API. If it works with that, then I'll close out this PR.

@jquick-axway
Copy link
Contributor Author

I've confirmed that the async version of WebView.evalJS() does not have this issue. Only the blocking version does. So, I don't think this is worth fixing since the blocking version is inferior and can still have timeout issues if the webpage is too large or if it's a slow device.

Closing this PR.

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

7 participants