Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

expose max depth on Window #10414

Merged
merged 6 commits into from Aug 3, 2019

Conversation

@dnfield
Copy link
Member

commented Aug 1, 2019

Exposes max depth on Window. Currently, this value is dropped when updating the viewport metrics.

Also changes the C++ side default to the value of dart:core's double.maxFinite, which will allow platforms that do not expose this concept to work a bit better with code that wants to check it.

Fixes flutter/flutter#33853

This will break framework tests, which rely on being able to implement Window. It will require a manual roll.

@dnfield dnfield requested a review from mklim Aug 1, 2019
@googlebot googlebot added the cla: yes label Aug 1, 2019
@mklim
mklim approved these changes Aug 1, 2019
Copy link
Member

left a comment

LGTM

// Platforms that do not explicitly set a depth will use this value, which
// avoids the need to special case logic that wants to check the max depth on
// the Dart side.
static const double kUnsetDepth = 1.7976931348623157e+308;

This comment has been minimized.

Copy link
@mklim

mklim Aug 1, 2019

Member

(Nothing to resolve) I'm a little concerned about if this could ever be potentially larger than std::numeric_limits<double>::max(), and what would happen if it was. I'm not sure if that's an actual practical problem that we would expect to see sometimes or not though. From some cursory investigation it looks like it should be fine.

This comment has been minimized.

Copy link
@dnfield

dnfield Aug 1, 2019

Author Member

Would that not result in a compilation error?

This number is actually smaller than what a 64 bit double should be able to hold from what I can tell.

This comment has been minimized.

Copy link
@mklim

mklim Aug 1, 2019

Member

That would make sense to me, but I think it's undefined behavior and compiler dependent. In some cases I'm pretty sure it'll get silently truncated to whatever will fit. From what I could track down this should be fine going by the IEEE 754 standard for 64 bit types, which is "usually" how double is implemented in C++. So it's most likely good.

Edit: this sums it up well: https://stackoverflow.com/a/34295021.

/// The physical depth is the maximum elevation that the Window allows.
///
/// Physical layers drawn at or above this elevation will have their elevation
/// clamped to this value.

This comment has been minimized.

Copy link
@mklim

mklim Aug 1, 2019

Member

nit: Do you think it's worth mentioning that elevation is cumulative, and that's what's going to be considered instead of what's actually set on a particular entity?

This comment has been minimized.

Copy link
@dnfield

dnfield Aug 1, 2019

Author Member

Added a line - does that sound right?

This comment has been minimized.

Copy link
@mklim

mklim Aug 1, 2019

Member

LG, thanks!

@dnfield dnfield requested a review from Piinks Aug 1, 2019
@Piinks
Piinks approved these changes Aug 1, 2019
Copy link
Contributor

left a comment

LGTM!

@dnfield dnfield referenced this pull request Aug 1, 2019
8 of 9 tasks complete
@dnfield

This comment has been minimized.

Copy link
Member Author

commented Aug 1, 2019

If flutter/flutter#37417 can land, I'd like to land this after that one, so it won't require a manual roll.

/// do not specify a maximum elevation.
double get physicalDepth => _physicalDepth;
double _physicalDepth = double.maxFinite;

This comment has been minimized.

Copy link
@Hixie

Hixie Aug 1, 2019

Contributor

is it worth mentioning that this only does anything on fuchsia today? I can imagine an Android developer arriving here and being very confused.

This comment has been minimized.

Copy link
@dnfield

dnfield Aug 1, 2019

Author Member

Sure

dnfield added 4 commits Aug 1, 2019
doc
@dnfield dnfield merged commit 63b253d into flutter:master Aug 3, 2019
21 checks passed
21 checks passed
WIP Ready for review
Details
build_and_test_android_profile_app Task Summary
Details
build_and_test_android_profile_app
Details
build_and_test_android_unopt_debug Task Summary
Details
build_and_test_android_unopt_debug
Details
build_and_test_linux_opt_profile Task Summary
Details
build_and_test_linux_opt_profile
Details
build_and_test_linux_opt_release Task Summary
Details
build_and_test_linux_opt_release
Details
build_and_test_linux_unopt_debug Task Summary
Details
build_and_test_linux_unopt_debug
Details
build_fuchsia_artifacts Task Summary
Details
build_fuchsia_artifacts
Details
build_windows_opt_debug Task Summary
Details
build_windows_opt_debug
Details
build_windows_unopt_debug Task Summary
Details
build_windows_unopt_debug
Details
cla/google All necessary CLAs are signed
format_and_dart_test Task Summary
Details
format_and_dart_test
Details
luci-engine
Details
@dnfield dnfield deleted the dnfield:expose_z branch Aug 3, 2019
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Aug 3, 2019
engine-flutter-autoroll added a commit to flutter/flutter that referenced this pull request Aug 3, 2019
git@github.com:flutter/engine.git/compare/960501721b23...63b253d907b9

git log 960501721b23..63b253d907b9 --no-merges --oneline
2019-08-03 dnfield@google.com expose max depth on Window (flutter/engine#10414)

The AutoRoll server is located here: https://autoroll.skia.org/r/flutter-engine-flutter-autoroll

Documentation for the AutoRoller is here:
https://skia.googlesource.com/buildbot/+/master/autoroll/README.md

If the roll is causing failures, please contact the current sheriff (franciscojma@google.com), and stop
the roller if necessary.
@Piinks Piinks referenced this pull request Aug 6, 2019
7 of 8 tasks complete
cfontas added a commit to cfontas/engine that referenced this pull request Aug 8, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants
You can’t perform that action at this time.