[widget audit] toga.WebView

[freakboy3742]

Audit of the WebView widget.

Includes a number of backwards incompatible cleanups:

• Removed the on_key_down handler; there are better places to catch key input if it is needed
• Removed the get_dom() method; this wasn't working on any platform since the move to WKWebView, and doesn't appear to be easily implemented using native APIs.
• Modified evaluate_javascript() to return an AsyncResult object, and accept an on_result callback. This makes javascript evaluation similar to dialog handling - in a synchronous context you can throw the javascript over the wall and get a functional callback; in an async context, you can await the result.
• Removed the invoke_javascript() method. It's now satisfied by evaluate_javascript
• Explicitly deprecated file:// URLs, since WKWebview makes them effectively impossible anyway.

Audit checklist

• Core tests ported to pytest
• Core tests at 100% coverage
• Core docstrings complete, and in Sphinx format
• Documentation complete and consistently formatted
• cocoa backend at 100% coverage
• winforms backend at 100% coverage
• gtk backend at 100% coverage
• iOS backend at 100% coverage
• android backend at 100% coverage
• Widget support table updated
• Widget Issue backlog reviewed

[mhsmith]

The Windows tests are hanging in CI, which I've seen locally as well.

And there's one failure on Android which I haven't seen locally:

I/python.stdout: =================================== FAILURES ===================================
I/python.stdout: _____________________________ test_static_content ______________________________
I/python.stdout: Traceback (most recent call last):
I/python.stdout:   File "/data/data/org.beeware.toga.testbed/files/chaquopy/AssetFinder/app/tests/widgets/test_webview.py", line 110, in test_static_content
I/python.stdout:     await assert_content_change(
I/python.stdout:   File "/data/data/org.beeware.toga.testbed/files/chaquopy/AssetFinder/app/tests/widgets/test_webview.py", line 40, in assert_content_change
I/python.stdout:     assert new_url == url
I/python.stdout:            ^^^^^^^^^^^^^^
I/python.stdout: AssertionError
I/python.stdout: ----------------------------- Captured stderr call -----------------------------
I/python.stdout: [INFO:CONSOLE(1)] "Uncaught TypeError: Cannot read property 'innerHTML' of null", source: https://example.com/ (1)
I/python.stdout: s_glBindAttribLocation: bind attrib 0 name inPosition
I/python.stdout: s_glBindAttribLocation: bind attrib 1 name inColor
I/python.stdout: s_glBindAttribLocation: bind attrib 2 name inTextureCoords
I/python.stdout: s_glBindAttribLocation: bind attrib 0 name position
I/python.stdout: s_glBindAttribLocation: bind attrib 1 name localCoord
I/python.stdout: =========================== short test summary info ============================
I/python.stdout: FAILED tests/widgets/test_webview.py::test_static_content - AssertionError
I/python.stdout: ======= 1 failed, 173 passed, 7 skipped, 1 xfailed in 739.76s (0:12:19) ========```

I'll look into both of these tomorrow, but meanwhile any comments on my last few commits would be welcome.

[freakboy3742]

@mhsmith I've taken a look a the recent changes, merged with main, and pushed a couple of minor docs tweaks. On the whole, there's nothing alarming here. The changes all made sense... eventually. It took me a while to work out what was going on with the @requires_corewebview2 handler, so it might be worth dropping an implementation note block explaining the general approach for future archaeologists.

My only questions of substance relate to that decorator:

1. Is there a rationale around the methods that don't have the @requires_corewebview2 handler? Naïvely, it would seem that every method that touches the native widget should need this decorator, but based on the current implementation, it would seem that you can get the user agent, the current URL, or evaluate JavaScript without the native widget having completed initialisation. get_url() has a safety catch recognizing this; it would seem that the the other methods also need this. Could/should this be part of the requires decorator (or a second "getter" version of that decorator)? I'd also say there's an argument to be made that the safety catch should raise an exception, rather than returning a "neutral" result.
2. Are we going to get reliable coverage from this approach? Under test conditions, it would seem possible that a slow machine will never trigger the "do immediately" branch; and a fast machine will never trigger the "do deferred" branch. Do you think this is a risk in practice?

[mhsmith]

It appears our editors are fighting with each other over the line length. Can we standardize on 88 columns for both docstrings and code, as we already have in Briefcase (beeware/briefcase#1200)?

[mhsmith]

It took me a while to work out what was going on with the @requires_corewebview2 handler, so it might be worth dropping an implementation note block explaining the general approach for future archaeologists.

OK, done.

it would seem that you can get the user agent, the current URL, or evaluate JavaScript without the native widget having completed initialisation. get_url() has a safety catch recognizing this; it would seem that the the other methods also need this.

You're right about evaluate_javascript, and this was actually the cause of the hangs in CI: the uninitialized native widget ignored the call, so the future was never completed. I've fixed this, and added timeouts around all the async calls. (The assert new_url == url failure on Android above was a result of the existing 2-second timeout in assert_content_change.)

get_url can be covered by a general statement that setting the url property isn't guaranteed to take immediate effect, so I've added that to the docs.

That just leaves get_user_agent, which is probably the least important method in the API. We agreed it was OK for it to return an empty string when initialization is incomplete, as long as that's documented.

Are we going to get reliable coverage from this approach? Under test conditions, it would seem possible that a slow machine will never trigger the "do immediately" branch; and a fast machine will never trigger the "do deferred" branch. Do you think this is a risk in practice?

The "deferred" branch will always be run by the constructor setting the initial values of url and user_agent. It's impossible for us to have received the CoreWebView2InitializationCompleted event at that point, because we haven't returned to the event loop since initialization was requested.

The "immediate" branch will be run by those tests which involve load_url. That method always blocks until initialization is complete, so anything that happens after that will always take the immediate branch.

[mhsmith]

Based on the log timestamps, which you can display with shift-T, it takes about 30 seconds for the WebView to initialize for the first time on Windows, so I've changed the widget fixture to make sure that's complete before the body of the test begins.

On Android, CI is still getting intermittent hangs associated with the message "Cannot read property 'innerHTML' of null". I can't reproduce this locally, and making the timeout longer doesn't do anything except make the test hang for longer before it times out.

[mhsmith]

All the tests are now passing, but I've just kicked off a re-run to be extra careful. Assuming that passes too, I'm OK for this to be merged.

[freakboy3742]

Looks good to me. Nice work hunting the CI failures - they're painful to find at the best of times, and the 24 minute runtime for Android tests doesn't help.

[mhsmith]

FYI: there's still one intermittent failure in the merged version:

  File "/Users/runner/Library/Developer/CoreSimulator/Devices/06462ED6-C06F-4B73-86B6-1AB8482E6DD4/data/Containers/Bundle/Application/F79F4378-878E-4065-A59E-EFBA9586080B/Toga Testbed.app/app/tests/widgets/test_webview.py", line 112, in test_set_url
    await assert_content_change(
  File "/Users/runner/Library/Developer/CoreSimulator/Devices/06462ED6-C06F-4B73-86B6-1AB8482E6DD4/data/Containers/Bundle/Application/F79F4378-878E-4065-A59E-EFBA9586080B/Toga Testbed.app/app/tests/widgets/test_webview.py", line 53, in assert_content_change
    new_content = await get_content(widget)
  File "/Users/runner/Library/Developer/CoreSimulator/Devices/06462ED6-C06F-4B73-86B6-1AB8482E6DD4/data/Containers/Bundle/Application/F79F4378-878E-4065-A59E-EFBA9586080B/Toga Testbed.app/app/tests/widgets/test_webview.py", line 25, in get_content
    return await wait_for(
  File "/Users/runner/Library/Developer/CoreSimulator/Devices/06462ED6-C06F-4B73-86B6-1AB8482E6DD4/data/Containers/Bundle/Application/F79F4378-878E-4065-A59E-EFBA9586080B/Toga Testbed.app/python-stdlib/asyncio/tasks.py", line 458, in wait_for
    raise exceptions.TimeoutError() from exc
asyncio.exceptions.TimeoutError

This is because get_content catches TimeoutError, but it should be asyncio.TimeoutError, which is a different class in Python 3.10 and older. I'll fix this in #1951.