[web] Use platform detection from Flutter web engine. #147346
Conversation
github-actions
bot
added
the
framework
github-actions
bot
added
the
engine
This PR moves the core of `browser_detection.dart` to a location in `dart:ui_web` so it can be used by apps and plugins. In order for the code to be a little bit tidier in ui_web, it's encapsulated in a singleton instance that can be accessed through `BrowserDetection.instance` or a top level global `browser` in `dart:ui_web`. ## Issues * Needed to fix: flutter/flutter#128943 * Needed to land: flutter/flutter#147346 ## Tests Updated affected tests. Mostly the update was to call the methods from `web_ui.browser.methodName` rather than a global scope. Also split the tests for this module in two files: * `engine_browser_detect_test.dart` - with the tests specific to the engine (capability detection, etc...) * `browser_detect_test.dart` - only the tests pertaining to the "core" of the library. [C++, Objective-C, Java style guides]: https://github.com/flutter/engine/blob/main/CONTRIBUTING.md#style
ditman
force-pushed
the
use-engine-browser-detection
branch
from
7964ddb
to
b279670
Compare
github-actions
bot
removed
the
engine
|
Rebased with latest master, marking as ready for review! |
|
Rebasing again to give FROB a chance to pick up the engine-related changes as well. |
This comment was marked as resolved.
This comment was marked as resolved.
|
is it possible to write a test that just spot checks that it's actually using the engine values and not just coincidentally implementing the same algorithm? |
@Hixie hmm... not sure I can inspect directly that the code I'm using is from the engine, but I think I can mock something wacky in the engine, then ensure that the framework returns the same. Thanks! |
|
Tweaked the |
| final platform.TargetPlatform? _testPlatform = () { | ||
| platform.TargetPlatform? result; | ||
| // The TargetPlatform used ON WEB TESTS, unless overridden. | ||
| // | ||
| // Respects the `ui_web.browser.debugOperatingSystemOverride` value (when set). | ||
| platform.TargetPlatform? get _testPlatform { | ||
| platform.TargetPlatform? testPlatform; |
There was a problem hiding this comment.
Note that _testPlatform used to be a final variable computed from an IIFE, but it needs to be a getter so it can be overridden as needed from tests. The getter ends up as an empty function when asserts are disabled, so I hope this doesn't cause a huge performance hit.
There was a problem hiding this comment.
Maybe @pragma('dart2js:tryInline') could help?
There was a problem hiding this comment.
Brought this up with the dart2js people, and they confirmed that the annotation is not needed; @biggs0125 said:
Dart2js should do a pretty good job of inlining that code when assertions are disabled, even without the annotation. We should be able to tell that (1) the code has no side effects and (2) the code always returns null. So it should just inline a null into the callsite.
(I hope it doesn't make debug mode too miserable! In that case, I'd walk back the changes that I added just for testing, and find another way)
* Extract the OperatingSystem -> TargetPlatform conversion to a function. * Make the switch expression exhaustive. * Add a special section to the _testPlatform getter so it takes into account the web_ui overrides. * Added browser-only unit-test.
ditman
force-pushed
the
use-engine-browser-detection
branch
from
4df122d
to
506e055
Compare
Important
Requires the following engine PR:
This PR refactors Flutter
foundation's libraryplatformfor the web with the same code we use to detect platforms in the engine.Issues
Testing
Demo app deployed here:
Pre-launch Checklist
///).If you need help, consider asking for advice on the #hackers-new channel on Discord.