Authenticated WebView improvement [Part 1] #13468

hichamboushaba · 2025-02-05T11:23:37Z

Part of: #13467

Tip

Excluding the commit f0ce7cd when reviewing would make the review easier, this commit just renames few classes.

Description

This PR is a preparation for the next PR that implements the improvements suggested in pe5sF9-3QB-p2, it handles the following changes:

It simplifies the code of the WCWebView composable, and make some changes to make it more use better Compose idiomatic approaches.
It removes some unused code (more details in the comments below)
It allows setting a custom WebViewClient, this turned out to be not needed for this feature, but I believe it can be useful for other features.
Rename the various classes related to the Authenticated WebView from WPComWebView... to AuthenticatedWebView... to align better with the new roles.

Steps to reproduce

This PR shouldn't introduce any behavior changes (except one for Blaze campaign details that I will explain below), so just testing few areas that use the WCWebView and the AuthenticatedWebView, for example:

Order card reader from the Payments hub.
Theme settings for Atomic websites.
Analytics reports for Atomic websites.

Testing information

Confirm that there are no regressions.

The tests that have been performed

^

I have considered if this change warrants release notes and have added them to RELEASE-NOTES.txt if necessary. Use the "[Internal]" label for non-user-facing changes.

Reviewer (or Author, in the case of optional code reviews):

Please make sure these conditions are met before approving the PR, or request changes if the PR needs improvement:

The PR is small and has a clear, single focus, or a valid explanation is provided in the description. If needed, please request to split it into smaller PRs.
Ensure Adequate Unit Test Coverage: The changes are reasonably covered by unit tests or an explanation is provided in the PR description.
Manual Testing: The author listed all the tests they ran, including smoke tests when needed (e.g., for refactorings). The reviewer confirmed that the PR works as expected on big (tablet) and small (phone) in case of UI changes, and no regressions are added.

dangermattic · 2025-02-05T11:24:24Z

	4 Warnings
⚠️	View files have been modified, but no screenshot or video is included in the pull request. Consider adding some for clarity.
⚠️	This PR is larger than 300 lines of changes. Please consider splitting it into smaller PRs for easier and faster reviews.
⚠️	Class `WCWebChromeClient` is missing tests, but `unit-tests-exemption` label was set to ignore this.
⚠️	Class `WCWebViewClient` is missing tests, but `unit-tests-exemption` label was set to ignore this.

Generated by 🚫 Danger

wpmobilebot · 2025-02-05T11:28:46Z

📲 You can test the changes from this Pull Request in WooCommerce-Wear Android by scanning the QR code below to install the corresponding build.

	App Name	WooCommerce-Wear Android
	Platform	⌚️ Wear OS
	Flavor	Jalapeno
	Build Type	Debug
	Commit	`9a4f298`
	Direct Download	`woocommerce-wear-prototype-build-pr13468-9a4f298.apk`

wpmobilebot · 2025-02-05T11:41:48Z

📲 You can test the changes from this Pull Request in WooCommerce Android by scanning the QR code below to install the corresponding build.

	App Name	WooCommerce Android
	Platform	📱 Mobile
	Flavor	Jalapeno
	Build Type	Debug
	Commit	`9a4f298`
	Direct Download	`woocommerce-prototype-build-pr13468-9a4f298.apk`

hichamboushaba · 2025-02-06T14:29:15Z

.../src/main/kotlin/com/woocommerce/android/ui/blaze/detail/BlazeCampaignDetailWebViewScreen.kt

@@ -32,7 +32,6 @@ fun BlazeCampaignDetailWebViewScreen(
            userAgent = userAgent,
            wpComAuthenticator = wpComAuthenticator,
            onUrlLoaded = onUrlLoaded,
-            clearCache = true,


clearing cache was enabled here for Blaze campaign details, I don't think this was intentional, so I removed it. The feature works without issues without this, but if I'm missing something, I can bring it back (cc @JorgeMucientes).

I also tested this and found no issues. I see no reason to clear the cache in this case, so it looks good to me.

hichamboushaba · 2025-02-06T14:30:36Z

WooCommerce/src/main/kotlin/com/woocommerce/android/ui/compose/component/web/WCWebView.kt

+    }
+
+    webView?.let { webView ->
+        LaunchedEffect(url) {


With using a LaunchedEffect for this, we simplified the code quite a bit as we don't need to check what was the last requested URL.
This also will help in the next PR, as we need to make authenticateAndLoadUrl a suspendable function.

hichamboushaba · 2025-02-06T14:32:24Z

WooCommerce/src/main/kotlin/com/woocommerce/android/ui/compose/component/web/WCWebView.kt

+
+    Box(modifier = modifier) {
+        val webViewAlpha by remember {
+            derivedStateOf {


Using derivedStateOf for this feels more Compose idiomatic than the local functions we were using before, and should be better for performance, as before we were recalculating the alpha on each recomposition even if it didn't change.

irfano

I tested web views on various screens and couldn't find any issues. Organized commits help a lot with the review, thank you for that.

I noticed an issue, but I’m unsure if it was introduced in this PR, and I couldn’t find an error log at first glance. When the Themes screen is open, and I put the app in the background and relaunch it, the app doesn’t start on the Themes screen. Instead, it shows the main screen. I also tested this in trunk, and it resumes from the Themes screen there. Could you check this, please?

Screen_recording_20250208_173756.webm

WooCommerce/src/main/kotlin/com/woocommerce/android/ui/compose/component/web/WCWebView.kt

irfano · 2025-02-08T14:18:01Z

.../src/main/kotlin/com/woocommerce/android/ui/blaze/detail/BlazeCampaignDetailWebViewScreen.kt

@@ -32,7 +32,6 @@ fun BlazeCampaignDetailWebViewScreen(
            userAgent = userAgent,
            wpComAuthenticator = wpComAuthenticator,
            onUrlLoaded = onUrlLoaded,
-            clearCache = true,


I also tested this and found no issues. I see no reason to clear the cache in this case, so it looks good to me.

hichamboushaba · 2025-02-10T09:46:59Z

I noticed an issue, but I’m unsure if it was introduced in this PR, and I couldn’t find an error log at first glance. When the Themes screen is open, and I put the app in the background and relaunch it, the app doesn’t start on the Themes screen. Instead, it shows the main screen. I also tested this in trunk, and it resumes from the Themes screen there. Could you check this, please?

I can't reproduce this issue using this branch, and this PR didn't make any changes to this part, I modified only the handling of the theme preview logic, which is in ThemePreviewFragment, while this issue from your video is in the ThemePickerFragment.

From the look of your video, it seems the "recent apps" repasses the Intent and creates a new task, which removes the AppSettingsActivity from the backstack, I'm not sure why it happens in this case, but it might be a good idea to force close the app, then launch it from the launcher directly, and then re-test, and see if the issue happens?

irfano

I wiped the emulator’s data and performed a cold boot, but the issue persisted. I then created a new emulator, and the issue is no longer reproducible. That’s strange, it must be related to the emulator itself. In any case, everything looks good. 👍🏻

codecov-commenter · 2025-02-10T17:05:02Z

Codecov Report

Attention: Patch coverage is 6.57895% with 71 lines in your changes missing coverage. Please review.

Project coverage is 37.89%. Comparing base (3f3a643) to head (9a4f298).
Report is 123 commits behind head on trunk.

Files with missing lines	Patch %	Lines
...erce/android/ui/compose/component/web/WCWebView.kt	0.00%	27 Missing ⚠️
...ndroid/ui/compose/component/web/WCWebViewClient.kt	0.00%	15 Missing ⚠️
...ocommerce/android/ui/themes/ThemePreviewWebView.kt	0.00%	9 Missing ⚠️
...roid/ui/compose/component/web/WCWebViewSettings.kt	0.00%	8 Missing ⚠️
...ui/common/webview/AuthenticatedWebViewViewModel.kt	0.00%	3 Missing ⚠️
...android/ui/compose/component/web/WCWebViewEvent.kt	0.00%	3 Missing ⚠️
.../android/ui/common/webview/WebViewAuthenticator.kt	0.00%	1 Missing ⚠️
...roid/ui/compose/component/web/WCWebChromeClient.kt	0.00%	1 Missing ⚠️
...oid/ui/google/webview/GoogleAdsWebViewViewModel.kt	0.00%	1 Missing ⚠️
...n/accountmismatch/AccountMismatchErrorViewModel.kt	0.00%	1 Missing ⚠️
... and 2 more

Additional details and impacted files

@@            Coverage Diff            @@
##              trunk   #13468   +/-   ##
=========================================
  Coverage     37.88%   37.89%           
- Complexity     8956     8957    +1     
=========================================
  Files          2050     2053    +3     
  Lines        112105   112092   -13     
  Branches      14173    14162   -11     
=========================================
+ Hits          42474    42477    +3     
+ Misses        65751    65736   -15     
+ Partials       3880     3879    -1

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

hichamboushaba added 8 commits February 5, 2025 11:22

Add ability to set a custom webview client to the Composable WebView

28da13d

Improve logic for loading the URL

fc42030

Use a more idiomatic Compose approach for updating the alpha

640e8db

Remove unused argument

5a653a1

Introduce WCWebViewSettings to improve the API

15542bd

Update WCWebView to get rid of clearCache argument

0eab82f

Update ThemePreview to use the default WCWebView implementation

1847c8b

Rename classes

88ee6a7

hichamboushaba added the type: task An internally driven task. label Feb 5, 2025

Move interface to its own file

0bcec8a

hichamboushaba added the unit-tests-exemption label Feb 6, 2025

hichamboushaba marked this pull request as ready for review February 6, 2025 14:13

hichamboushaba requested review from irfano and JorgeMucientes February 6, 2025 14:13

hichamboushaba commented Feb 6, 2025

View reviewed changes

hichamboushaba mentioned this pull request Feb 6, 2025

Authenticated WebView improvement [Part 2] #13487

Merged

4 tasks

hichamboushaba added 3 commits February 6, 2025 16:21

Rename the authenticated WebView

f0ce7cd

Remove unused class

4075361

Remove unused import

9a4f298

irfano self-assigned this Feb 6, 2025

irfano reviewed Feb 8, 2025

View reviewed changes

hichamboushaba requested a review from irfano February 10, 2025 10:05

irfano approved these changes Feb 10, 2025

View reviewed changes

irfano enabled auto-merge February 10, 2025 17:03

irfano added this to the 21.8 milestone Feb 10, 2025

irfano merged commit b1a251e into trunk Feb 10, 2025
16 checks passed

irfano deleted the issue/13467-improve-authenticated-webview-1 branch February 10, 2025 17:04

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Authenticated WebView improvement [Part 1] #13468

Authenticated WebView improvement [Part 1] #13468

hichamboushaba commented Feb 5, 2025 •

edited

Loading

dangermattic commented Feb 5, 2025 •

edited

Loading

wpmobilebot commented Feb 5, 2025 •

edited

Loading

wpmobilebot commented Feb 5, 2025 •

edited

Loading

hichamboushaba Feb 6, 2025

irfano Feb 8, 2025

hichamboushaba Feb 6, 2025

hichamboushaba Feb 6, 2025

irfano left a comment

irfano Feb 8, 2025

hichamboushaba commented Feb 10, 2025

irfano left a comment

codecov-commenter commented Feb 10, 2025

Authenticated WebView improvement [Part 1] #13468

Authenticated WebView improvement [Part 1] #13468

Conversation

hichamboushaba commented Feb 5, 2025 • edited Loading

Description

Steps to reproduce

Testing information

The tests that have been performed

Reviewer (or Author, in the case of optional code reviews):

dangermattic commented Feb 5, 2025 • edited Loading

wpmobilebot commented Feb 5, 2025 • edited Loading

wpmobilebot commented Feb 5, 2025 • edited Loading

hichamboushaba Feb 6, 2025

Choose a reason for hiding this comment

irfano Feb 8, 2025

Choose a reason for hiding this comment

hichamboushaba Feb 6, 2025

Choose a reason for hiding this comment

hichamboushaba Feb 6, 2025

Choose a reason for hiding this comment

irfano left a comment

Choose a reason for hiding this comment

irfano Feb 8, 2025

Choose a reason for hiding this comment

hichamboushaba commented Feb 10, 2025

irfano left a comment

Choose a reason for hiding this comment

codecov-commenter commented Feb 10, 2025

Codecov Report

hichamboushaba commented Feb 5, 2025 •

edited

Loading

dangermattic commented Feb 5, 2025 •

edited

Loading

wpmobilebot commented Feb 5, 2025 •

edited

Loading

wpmobilebot commented Feb 5, 2025 •

edited

Loading