Skip to content
This repository was archived by the owner on Feb 25, 2025. It is now read-only.
Merged
Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
42 commits
Select commit Hold shift + click to select a range
65fb5d3
Switch ui.window.devicePixelRatio to browser dpi
ferhatb Mar 18, 2020
bd58d35
Rename debugOverrideDevicePixelRation to override. Add integration te…
ferhatb Mar 19, 2020
2165d86
Merge remote-tracking branch 'upstream/master' into devicepixels
ferhatb Mar 19, 2020
542be23
Update cirrus for new integration test
ferhatb Mar 19, 2020
8705651
Merge remote-tracking branch 'upstream/master' into devicepixels
ferhatb Mar 19, 2020
aab46b4
update canvas golden test root transform
ferhatb Mar 19, 2020
101083d
Update compositing golden test with dpr transform
ferhatb Mar 19, 2020
a328874
Address review comment
ferhatb Mar 19, 2020
3af31ef
remove print
ferhatb Mar 19, 2020
472615b
Merge remote-tracking branch 'upstream/master' into devicepixels
ferhatb Mar 20, 2020
489ad43
Merge remote-tracking branch 'upstream/master' into devicepixels
ferhatb Mar 20, 2020
7d31069
update test
ferhatb Mar 20, 2020
3ae5e40
Merge remote-tracking branch 'upstream/master' into devicepixels
ferhatb Mar 20, 2020
16a0f0f
Merge remote-tracking branch 'upstream/master' into devicepixels
ferhatb Mar 23, 2020
d8907b0
Merge remote-tracking branch 'upstream/master' into devicepixels
ferhatb Mar 23, 2020
a2aaafb
Addressed review comments/replyChannel
ferhatb Mar 23, 2020
19da8b6
Merge remote-tracking branch 'upstream/master' into devicepixels
ferhatb Mar 23, 2020
2b99962
Fix missing platform replymessage
ferhatb Mar 23, 2020
6ff40c2
Don't reply when callback is null
ferhatb Mar 23, 2020
8d087b4
Merge remote-tracking branch 'upstream/master' into devicepixels
ferhatb Mar 23, 2020
44d045c
Merge remote-tracking branch 'upstream/master' into devicepixels
ferhatb Mar 24, 2020
e456f3d
update test embedding for flutter framework tests
ferhatb Mar 24, 2020
c34111b
fix assert return
ferhatb Mar 24, 2020
47da9ce
CI test
ferhatb Mar 24, 2020
81fd917
CI test
ferhatb Mar 24, 2020
940e44d
CI test
ferhatb Mar 24, 2020
1d864dc
CI test
ferhatb Mar 24, 2020
2419fdb
CI Test
ferhatb Mar 24, 2020
d455d74
CI Test
ferhatb Mar 24, 2020
be2113d
Test CI
ferhatb Mar 24, 2020
6b227a7
Test CI
ferhatb Mar 24, 2020
5f21300
rename override to debugOverride
ferhatb Mar 24, 2020
2eaae7e
Revert to pre CI test code that was reviewed
ferhatb Mar 24, 2020
50f6c64
Merge remote-tracking branch 'upstream/master' into devicepixels
ferhatb Mar 24, 2020
158533c
fix typo and address review comment
ferhatb Mar 24, 2020
de3b6a7
Fix check for top level matrix
ferhatb Mar 25, 2020
fcffbec
Merge remote-tracking branch 'upstream/master' into devicepixels
ferhatb Mar 25, 2020
b710b87
Update assert for top level matrix to allow subclassing of window in …
ferhatb Mar 25, 2020
68e85ca
Merge remote-tracking branch 'upstream/master' into devicepixels
ferhatb Mar 25, 2020
01c5bf5
update assert to handle null _debugDevicePixels
ferhatb Mar 25, 2020
47a11e5
Restrict top level transform check
ferhatb Mar 25, 2020
ea15a3c
address review comments
ferhatb Mar 25, 2020
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions .cirrus.yml
Original file line number Diff line number Diff line change
Expand Up @@ -146,6 +146,7 @@ task:
- $FRAMEWORK_PATH/flutter/bin/flutter config --local-engine=host_debug_unopt --no-analytics --enable-web
- $FRAMEWORK_PATH/flutter/bin/flutter pub get --local-engine=host_debug_unopt
- $FRAMEWORK_PATH/flutter/bin/flutter drive -v --target=test_driver/text_editing_e2e.dart -d web-server --release --browser-name=chrome --local-engine=host_debug_unopt
- $FRAMEWORK_PATH/flutter/bin/flutter drive -v --target=test_driver/image_loading_e2e.dart -d web-server --release --browser-name=chrome --local-engine=host_debug_unopt
Comment thread
ferhatb marked this conversation as resolved.

- name: build_and_test_web_linux_firefox
compile_host_script: |
Expand Down
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
38 changes: 38 additions & 0 deletions e2etests/web/regular_integration_tests/lib/image_loading_main.dart
Original file line number Diff line number Diff line change
@@ -0,0 +1,38 @@
// Copyright 2013 The Flutter Authors. All rights reserved.
// Use of this source code is governed by a BSD-style license that can be
// found in the LICENSE file.

import 'package:flutter/material.dart';
import 'package:flutter/services.dart';

void main() {
runApp(MyApp());
}

class MyApp extends StatefulWidget {
@override
MyAppState createState() => MyAppState();
}

class MyAppState extends State<MyApp> {

@override
void initState() {
super.initState();
const MethodChannel channel =
OptionalMethodChannel('flutter/web_test_e2e', JSONMethodCodec());
channel.invokeMethod<void>(
'setDevicePixelRatio',
'1.5',
);
Comment thread
ferhatb marked this conversation as resolved.
Outdated
}

@override
Widget build(BuildContext context) {
return MaterialApp(
key: const Key('mainapp'),
title: 'Integration Test App',
home: Image.asset('assets/images/sample_image1.png')
);
}
}
4 changes: 4 additions & 0 deletions e2etests/web/regular_integration_tests/pubspec.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -16,3 +16,7 @@ dev_dependencies:
e2e: 0.2.4+4
http: 0.12.0+2
test: any

flutter:
assets:
- assets/images/
Original file line number Diff line number Diff line change
@@ -0,0 +1,24 @@
// Copyright 2013 The Flutter Authors. All rights reserved.
// Use of this source code is governed by a BSD-style license that can be
// found in the LICENSE file.

import 'dart:html' as html;
import 'package:flutter_test/flutter_test.dart';
import 'package:regular_integration_tests/image_loading_main.dart' as app;

import 'package:e2e/e2e.dart';

void main() {
E2EWidgetsFlutterBinding.ensureInitialized() as E2EWidgetsFlutterBinding;

testWidgets('Image loads asset variant based on device pixel ratio',
(WidgetTester tester) async {
app.main();
await tester.pumpAndSettle();
final html.ImageElement imageElement = html.querySelector('img') as html.ImageElement;
expect(imageElement.naturalWidth, 1.5 * 100);
expect(imageElement.naturalHeight, 1.5 * 100);
expect(imageElement.width, 100);
expect(imageElement.height, 100);
});
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
// Copyright 2013 The Flutter Authors. All rights reserved.
// Use of this source code is governed by a BSD-style license that can be
// found in the LICENSE file.

import 'dart:io';

import 'package:flutter_driver/flutter_driver.dart';

Future<void> main() async {
final FlutterDriver driver = await FlutterDriver.connect();

final String dataRequest =
await driver.requestData(null, timeout: const Duration(seconds: 1));
print('result $dataRequest');
Comment thread
ferhatb marked this conversation as resolved.
Outdated
await driver.close();

exit(dataRequest == 'pass' ? 0 : 1);
}
6 changes: 0 additions & 6 deletions lib/web_ui/lib/src/engine/dom_renderer.dart
Original file line number Diff line number Diff line change
Expand Up @@ -419,12 +419,6 @@ flt-glass-pane * {
// DOM tree.
setElementAttribute(_sceneHostElement, 'aria-hidden', 'true');

// We treat browser pixels as device pixels because pointer events,
// position, and sizes all use browser pixel as the unit (i.e. "px" in CSS).
// Therefore, as far as the framework is concerned the device pixel ratio
// is 1.0.
window.debugOverrideDevicePixelRatio(1.0);

if (html.window.visualViewport == null && isWebKit) {
// Safari sometimes gives us bogus innerWidth/innerHeight values when the
// page loads. When it changes the values to correct ones it does not
Expand Down
8 changes: 8 additions & 0 deletions lib/web_ui/lib/src/engine/surface/scene_builder.dart
Original file line number Diff line number Diff line change
Expand Up @@ -97,6 +97,14 @@ class SurfaceSceneBuilder implements ui.SceneBuilder {
if (matrix4.length != 16) {
throw ArgumentError('"matrix4" must have 16 entries.');
}
if (_surfaceStack.length == 1) {
// Top level transform contains view configuration to scale
// scene to devicepixelratio. Use identity instead since web browser
Comment thread
ferhatb marked this conversation as resolved.
Outdated
// renders using logical device pixels.
assert(matrix4[0] == EngineWindow.browserDevicePixelRatio &&
matrix4[5] == EngineWindow.browserDevicePixelRatio);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This assertion error would be visible to developers and Flutter contributors writing tests. It would be very confusing without a good error message. Consider moving the comment into the assertion error message.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removed the check for test environment since there are many SceneBuilder raw tests outside the context of an app that pushes root transform for now (we really need a ui.Window level debugDevicePixelRatio, i noticed some hacked in TestWindow.devicePixelRatioTestValue etc in the code base used for viewport_test).

matrix4 = Matrix4.identity().storage;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, now that I think about it, the correct fix would be to inject a transform that scales by 1/DPR. This way we don't have to rely on a particular framework behavior. To avoid extra div nodes we can do something like:

if (the scene contains a root transform) {
  reuse the root transform by scale it by 1/DPR.
} else {
  apply 1/DPR to the `<flt-scene>` element.
}

In most cases we'll hit the if branch, not the else branch. Also in most cases the root transform will be scale by DPR. So the resulting root transform will automatically become identity and we won't apply any transform at all.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The framework itself injects pushTransform using ViewConfiguration. Ideally this code would not live in framework but in engine. See view.dart:39. Or instead of calling pushTransform , framework would call SceneBuilder.setViewConfiguration in engine.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right, but you can use dart:ui without the Flutter framework where you can paint a scene without a root transform. This typically happens in tests and benchmarks. I don't expect there to be many apps that don't use the framework.

}
return _pushSurface(PersistedTransform(oldLayer, matrix4));
}

Expand Down
42 changes: 24 additions & 18 deletions lib/web_ui/lib/src/engine/window.dart
Original file line number Diff line number Diff line change
Expand Up @@ -15,19 +15,11 @@ class EngineWindow extends ui.Window {
}

@override
double get devicePixelRatio {
if (_debugDevicePixelRatio != null) {
return _debugDevicePixelRatio;
}
double get devicePixelRatio => _debugDevicePixelRatio != null
? _debugDevicePixelRatio
: browserDevicePixelRatio;

if (experimentalUseSkia) {
return browserDevicePixelRatio;
} else {
return 1.0;
}
}

/// Returns device pixel ratio returns by browser.
/// Returns device pixel ratio returned by browser.
static double get browserDevicePixelRatio {
double ratio = html.window.devicePixelRatio;
// Guard against WebOS returning 0.
Expand All @@ -36,12 +28,10 @@ class EngineWindow extends ui.Window {

/// Overrides the default device pixel ratio.
///
/// This is useful in tests to emulate screens of different dimensions.
void debugOverrideDevicePixelRatio(double value) {
assert(() {
_debugDevicePixelRatio = value;
return true;
}());
/// This is useful in tests to emulate screens of different dimensions
/// including integration tests.
void overrideDevicePixelRatio(double value) {
_debugDevicePixelRatio = value;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why this change? It's still used only in tests, so the "debug" prefix and assert seem appropriate.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is now also used in integration test (the one that loads an image at framework level).

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right, but that's still a test :)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do integration tests strip out asserts?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If they don't, we should add the option to enable them. dart2js supports asserts (we use dart2js for engine unit-test with assertions enabled).

/cc @nturgut

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My comment is non-blocking though. I don't think there's major risk. It just doesn't feel right for a method that's not annotated as "debug" to manipulate a "debug" field without an assertion.

}

double _debugDevicePixelRatio;
Expand Down Expand Up @@ -182,6 +172,9 @@ class EngineWindow extends ui.Window {
textEditing.channel.handleTextInput(data);
return;

case 'flutter/web_test_e2e':
_handleWebTestEnd2EndMessage(data);
return;
Comment thread
ferhatb marked this conversation as resolved.
Comment thread
ferhatb marked this conversation as resolved.
case 'flutter/platform_views':
if (experimentalUseSkia) {
rasterizer.viewEmbedder.handlePlatformViewCall(data, callback);
Expand Down Expand Up @@ -315,6 +308,19 @@ class EngineWindow extends ui.Window {
Rasterizer rasterizer = experimentalUseSkia ? Rasterizer(Surface()) : null;
}

void _handleWebTestEnd2EndMessage(ByteData data) {
const MethodCodec codec = JSONMethodCodec();
final MethodCall decoded = codec.decodeMethodCall(data);
final Map<String, dynamic> message = decoded.arguments;
double ratio = double.parse(decoded.arguments);
switch(decoded.method) {
case 'setDevicePixelRatio':
window.overrideDevicePixelRatio(ratio);
window.onMetricsChanged();
break;
}
}

/// The window singleton.
///
/// `dart:ui` window delegates to this value. However, this value has a wider
Expand Down
2 changes: 1 addition & 1 deletion lib/web_ui/lib/src/ui/test_embedding.dart
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,7 @@ Future<void> webOnlyInitializeTestDomRenderer({double devicePixelRatio = 3.0}) {
// The following parameters are hard-coded in Flutter's test embedder. Since
// we don't have an embedder yet this is the lowest-most layer we can put
// this stuff in.
engine.window.debugOverrideDevicePixelRatio(devicePixelRatio);
engine.window.overrideDevicePixelRatio(devicePixelRatio);
engine.window.webOnlyDebugPhysicalSizeOverride =
Size(800 * devicePixelRatio, 600 * devicePixelRatio);
webOnlyScheduleFrameCallback = () {};
Expand Down
3 changes: 3 additions & 0 deletions lib/web_ui/test/golden_tests/engine/canvas_golden_test.dart
Original file line number Diff line number Diff line change
Expand Up @@ -222,12 +222,15 @@ void main() async {
);

final SceneBuilder sb = SceneBuilder();
sb.pushTransform(Matrix4.diagonal3Values(EngineWindow.browserDevicePixelRatio,
EngineWindow.browserDevicePixelRatio, 1.0).storage);
sb.pushTransform(Matrix4.rotationZ(math.pi / 2).storage);
sb.pushOffset(0, -500);
sb.pushClipRect(canvasSize);
sb.pop();
sb.pop();
sb.pop();
sb.pop();
final SurfaceScene scene = sb.build();
final html.Element sceneElement = scene.webOnlyRootElement;

Expand Down
4 changes: 2 additions & 2 deletions lib/web_ui/test/path_test.dart
Original file line number Diff line number Diff line change
Expand Up @@ -259,7 +259,7 @@ void main() {
// Regression test for https://github.com/flutter/flutter/issues/44470
test('Should handle contains for devicepixelratio != 1.0', () {
js_util.setProperty(html.window, 'devicePixelRatio', 4.0);
window.debugOverrideDevicePixelRatio(4.0);
window.overrideDevicePixelRatio(4.0);
final path = Path()
..moveTo(50, 0)
..lineTo(100, 100)
Expand All @@ -268,7 +268,7 @@ void main() {
..close();
expect(path.contains(Offset(50, 50)), isTrue);
js_util.setProperty(html.window, 'devicePixelRatio', 1.0);
window.debugOverrideDevicePixelRatio(1.0);
window.overrideDevicePixelRatio(1.0);
// TODO: Investigate failure on CI. Locally this passes.
// [Exception... "Failure" nsresult: "0x80004005 (NS_ERROR_FAILURE)"
}, skip: browserEngine == BrowserEngine.firefox);
Expand Down