Skip to content

Conversation

@nan-li
Copy link
Contributor

@nan-li nan-li commented Oct 13, 2021

See Github Issue #1453


This change is Reviewable

Copy link
Member

@jkasten2 jkasten2 left a comment

Choose a reason for hiding this comment

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

Reviewed 1 of 1 files at r1, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @nan-li)


OneSignalSDK/onesignal/src/main/java/com/onesignal/WebViewManager.java, line 150 at r1 (raw file):

                public void run() {
                    // This try-catch is added to address this error when OSWebView is created
                    // MissingWebViewPackageException: Failed to load WebView provider: No WebView installed

Comment could be simplified to:

// Handles exception  "MissingWebViewPackageException: Failed to load WebView provider: No WebView installed"

OneSignalSDK/onesignal/src/main/java/com/onesignal/WebViewManager.java, line 154 at r1 (raw file):

                        webViewManager.setupWebView(currentActivity, base64Str);
                    } catch (Exception e) {
                        if (e.getMessage() != null && e.getMessage().contains("WebView")) {

Would be good to callout here that MissingWebViewPackageException isn't public so checking the message of the exception to only catch that specific one.


OneSignalSDK/onesignal/src/main/java/com/onesignal/WebViewManager.java, line 156 at r1 (raw file):

                        if (e.getMessage() != null && e.getMessage().contains("WebView")) {
                            OneSignal.Log(OneSignal.LOG_LEVEL.ERROR, "Error setting up WebView: ", e);
                            e.printStackTrace();

We can omit this line, since we are using OneSignal.Log above to print the message.

@nan-li nan-li force-pushed the fix/catch-no-WebView-installed branch from 1fa3531 to 3cf992d Compare October 15, 2021 19:18
Copy link
Contributor Author

@nan-li nan-li left a comment

Choose a reason for hiding this comment

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

Reviewable status: 0 of 1 files reviewed, 3 unresolved discussions (waiting on @jkasten2)


OneSignalSDK/onesignal/src/main/java/com/onesignal/WebViewManager.java, line 150 at r1 (raw file):

Previously, jkasten2 (Josh Kasten) wrote…

Comment could be simplified to:

// Handles exception  "MissingWebViewPackageException: Failed to load WebView provider: No WebView installed"

Done.


OneSignalSDK/onesignal/src/main/java/com/onesignal/WebViewManager.java, line 154 at r1 (raw file):

Previously, jkasten2 (Josh Kasten) wrote…

Would be good to callout here that MissingWebViewPackageException isn't public so checking the message of the exception to only catch that specific one.

Done.


OneSignalSDK/onesignal/src/main/java/com/onesignal/WebViewManager.java, line 156 at r1 (raw file):

Previously, jkasten2 (Josh Kasten) wrote…

We can omit this line, since we are using OneSignal.Log above to print the message.

Done.

@nan-li nan-li requested a review from jkasten2 October 19, 2021 21:17
Copy link
Member

@jkasten2 jkasten2 left a comment

Choose a reason for hiding this comment

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

Reviewed 1 of 1 files at r2, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @nan-li)

@nan-li nan-li merged commit 70b6297 into main Oct 22, 2021
@nan-li nan-li deleted the fix/catch-no-WebView-installed branch October 22, 2021 18:34
@jkasten2 jkasten2 mentioned this pull request Nov 5, 2021
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.

3 participants