[WebView] Add additionalHttpHeaders to WebContent.Url

[svenjacobs]

I have a use case where I need to pass additional HTTP headers to WebView.loadUrl().

While extending WebContent.Url I noticed that state.content is reassigned in several callback methods of WebViewClient, for example in shouldOverrideUrlLoading(). While additionalHttpHeaders are used for the initial loadUrl() call, this means that the state property additionalHttpHeaders is lost on reassignment. I'm not sure if this poses an issue and if so how to handle this situation.

[google-cla]

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

For more information, open the CLA check for this pull request.

[svenjacobs]

@bentrengrove Can you please have a look at this PR?

[bentrengrove]

I think if we were to support this you would have to ensure those additional headers weren't lost when the page was loaded and the state reassigned. For instance, you might need the headers passed across multiple redirects and the way it is right now they wouldn't be.

[svenjacobs]

I think if we were to support this you would have to ensure those additional headers weren't lost when the page was loaded and the state reassigned. For instance, you might need the headers passed across multiple redirects and the way it is right now they wouldn't be.

I can make sure that the headers are not lost by using the copy() operator on WebContent.Url. However whether WebView keeps using the headers passed to the initial loadUrl() call on redirects is implementation detail of the WebView and out of our control imho.

[svenjacobs]

Oh, I get it know. You mean when state.content is reassigned which triggers a new loadUrl()? I just pushed a commit ensuring that additionalHttpHeaders are not lost on reassignment. Please have a look.

[bentrengrove]

I don't think a mutable map is the solution here. Does webview actually return the headers it used in onPageFinished or anything like that where we update our state? We basically try to mirror the state of the webview with our state class, that is why it is reassigned all the time. If that was the case then we could store the headers again there.

[svenjacobs]

I don't think a mutable map is the solution here. Does webview actually return the headers it used in onPageFinished or anything like that where we update our state? We basically try to mirror the state of the webview with our state class, that is why it is reassigned all the time. If that was the case then we could store the headers again there.

I'm just passing a mutable map because the documentation of WebView's loadData() states that

Some older WebView implementations require additionalHttpHeaders to be mutable.

I don't think we have access to the actual headers that are used by WebView. The only data we get in onPageFinished are the WebView itself and the URL. I believe WebView is more or less a black box in that matter.

[svenjacobs]

Since the attribute is called additionalHttpHeaders I think it is fine that they don't match the actual headers which WebView is using because - as I assumed previously - we probably don't have access to the actual headers.

With the current implementation, additionalHttpHeaders are used for the initial and all subsequent loadUrl calls. Should the user however desire that subsequent requests use different headers, I guess we have to think of another solution then.

[svenjacobs]

Is there anything left to do?

[bentrengrove]

Yes, we would still need tests and documentation in order to merge this in. You would also need to run ./gradlew metalavaGenerateSignature to update the API signature.

[sampengilly]

Could probably make the header map non-nullable, default it to emptyMap() and apply it to loadUrl() if it's not empty.

[svenjacobs]

Could probably make the header map non-nullable, default it to emptyMap() and apply it to loadUrl() if it's not empty.

Sure, why not. I don't have a strong opinion on that one. What do you think @bentrengrove?

[svenjacobs]

I updated the documentation, metadata and added a test. Hope you like it 🤞🏼😃

PS: How can I get rid of the following error when calling any Gradle command?

> Failed to apply plugin 'com.vanniktech.maven.publish'.
   > The property SONATYPE_NEXUS_USERNAME was removed. Use mavenCentralUsername instead.

I had to temporarily comment out the com.vanniktech.maven.publish plugin in all build.gradle files.

[bentrengrove]

This all looks good to me and I like the emptyMap(). It just needs a rebase now as another WebView change was merged in ahead of it sorry.

As for the sonatype error, I am not actually sure! I am not seeing that here.

[svenjacobs]

Branch is now rebased on main.

As for the sonatype error, I am not actually sure! I am not seeing that here.

Is there anything else to do after checking out the project via Git? Maybe I'm missing some environment variables?

[bentrengrove]

Thanks for the rebase.

I can't spot anything environment variables in my setup at all, there shouldn't be anything you have to do

[svenjacobs]

Hm, it seems the test is flaky?

com.google.accompanist.web.WebTest > testAdditionalHttpHeaders[test(AVD) - 9] FAILED
androidx.compose.ui.test.junit4.android.ComposeNotIdleException: Idling resource timed out: possibly due to compose being busy.

[svenjacobs]

I pushed another commit, trying to work around the flaky test similar to testLoadingState().

[bentrengrove]

That test is marked as flaky with an issue for it to be fixed, as in it is currently broken. Copying that test is not a valid workaround sorry. You might just be able to increase the timeout.

[svenjacobs]

That test is marked as flaky with an issue for it to be fixed, as in it is currently broken. Copying that test is not a valid workaround sorry. You might just be able to increase the timeout.

I assumed both tests are flaky because they both use MockWebServer and the fix would be the same. I can remove the annotation, sure. But I'm unable to test whether the latest modifications fixed the test since I'm not allowed to run the GitHub workflows.

[bentrengrove]

Happy to rerun it for you but yeah my point is that I think it will still be flaky because that test is currently marked as flaky, it's not working around anything.

[svenjacobs]

Please rerun :) If it works now I will remove the annotation.

[svenjacobs]

I removed the annotation and increased the timeout. However the test that was failing for the latest run is testNavigatorCanGoForward(), unrelated to this PR.

[svenjacobs]

By the way, the test was / is "working on my computer" even before the latest modifications. So it's really hard for me to reproduce and fix this 😐

[svenjacobs]

Unfortunately the test is still failing 😞 Do you @bentrengrove have an idea? Because I've run out of ideas 🤷🏼‍♂️

[bentrengrove]

Maybe go back to the original version of the test when I approved it, add the flaky annotation and then I will have a look and see if I can fix both tests at the same time

[svenjacobs]

I reverted the test changes

[bentrengrove]

Thank you!