Enable setting large WebView content on Windows #4062
Conversation
* extended webview example app
|
@freakboy3742 Please review this PR |
johnzhou721
left a comment
johnzhou721
left a comment
There was a problem hiding this comment.
2 small notes, otherwise this is a great feature.
| if self._large_content_filepath and os.path.exists( | ||
| self._large_content_filepath | ||
| ): | ||
| os.remove(self._large_content_filepath) |
There was a problem hiding this comment.
I'm not sure of the goals of WebView.... but navigating back to the previous page is an usual use case for a web view widget. As written, if you navigate to another page from the static content loaded, and you navigate back, it'll probably error. IMO it would be safer to defer removing the large content file when the widget is discussed.
There was a problem hiding this comment.
Good point. I could use a static file name or a list of uuid. Then, the temporary file(s) could be deleted in the WebView destructor
There was a problem hiding this comment.
The purpose of a cache folder is that you shouldn't need to do this sort of cleanup; it should be an automated operation that happens independent of widget lifecycle.
| @@ -0,0 +1 @@ | |||
| The 2MB limit for setting static WebView content has been removed for Windows | |||
There was a problem hiding this comment.
2 cosmetic things:
- missing period at end
- IMO it'll be clearer to say "2MB size limit" not just "2MB limit".
- This shouldn't be a
miscnote -- it has user impact. I believebugfixwould be appropriate.
|
Would this feature also make sense for other platforms, even when there is no documented upper limit? |
|
@t-arn The way you test for that is by making a non platform-specific test, i.e. removing the branch of line 210 of your testbed change. And then we see if it fails on other platforms... if it does but you feel it will be scope creep if worked on in this PR, add a property to the probes and use that to exclude platforms, instead of platform-specific checks in the testbed. If you want to exclude platforms stuff, instead of a probe you can also use a probe-specific method to perform an operation specific to the platform, and pytest.skip on others. That makes it easier to find missing functionalities through test probes. |
This isn't a "feature" - it's a bug fix for a specific detail/limitation of the Winforms implementation of WebView. If a similar limitation exists for other platforms, then yes - that should be resolved; any test that you add for this feature should be in the testbed, which means any platform-specific limitation will be exposed. |
freakboy3742
left a comment
freakboy3742
left a comment
There was a problem hiding this comment.
Broadly makes sense; a couple of comments about cache strategy inline.
| self.native.NavigateToString(content) | ||
| if len(content) > 1500000: | ||
| # according to the Microsoft documentation, the max content size is | ||
| # 2 MB, but in fact, the limit seems to be at 1.5 MB |
There was a problem hiding this comment.
is it literally 1.5MB, or is this a Megabyte vs Mebibyte discrepancy?
| # according to the Microsoft documentation, the max content size is | ||
| # 2 MB, but in fact, the limit seems to be at 1.5 MB | ||
| file_name = str(uuid.uuid4()) + ".html" | ||
| self._large_content_filepath = toga.App.app.paths.cache / file_name |
There was a problem hiding this comment.
Given that a root_url is a required part of the set_content() call, that would seem to be the appropriate cache key. I'd also suggest that the base cache path isn't an appropriate directory location - we should be putting webview cache content into some sort of internal Toga directory (e.g., paths.cache / "toga/webview"). There's also a need to differentiate this widget from others - if I have 2 web views ,both displaying "index.html", I don't want them to collide in the cache.
| if self._large_content_filepath and os.path.exists( | ||
| self._large_content_filepath | ||
| ): | ||
| os.remove(self._large_content_filepath) |
There was a problem hiding this comment.
The purpose of a cache folder is that you shouldn't need to do this sort of cleanup; it should be an automated operation that happens independent of widget lifecycle.
3 participants
This PR adds a workaround for Windows, so that WebView content > 2MB can still be displayed without causing a WebView2 error: The content is written to a temporary file in the 'Cache' directory. This file is deleted when the page load is finished.
Although the Microsoft documentation says that the limit is at 2 MB, the tests have revealed that the limit actually is at 1.5 MB
PR Checklist: