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.

Sign up for GitHub

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

Jump to bottom

Add bottom view inset to FlutterWindowMetricsEvent #26442

Merged
merged 1 commit into from Jun 3, 2021
Merged

Add bottom view inset to FlutterWindowMetricsEvent #26442

merged 1 commit into from Jun 3, 2021

Conversation

sagallea
Copy link
Contributor

This is needed to propagate inset values through Flutter.

Bug: b/181683254

@flutter-dashboard flutter-dashboard bot added the embedder Related to the embedder API label May 26, 2021
@flutter-dashboard
Copy link

It looks like this pull request may not have tests. Please make sure to add tests before merging. If you need an exemption to this rule, contact Hixie on the #hackers channel in Chat.

If you are not sure if you need tests, consider this rule of thumb: the purpose of a test is to make sure someone doesn't accidentally revert the fix. Ask yourself, is there anything in your PR that you feel it is important we not accidentally revert back to how it was before your fix?

Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing.

@google-cla google-cla bot added the cla: yes label May 26, 2021
dnicoara
dnicoara suggested changes May 27, 2021
View reviewed changes
Copy link
Contributor

@dnicoara dnicoara left a comment

Choose a reason for hiding this comment

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

@chinmaygarde can help with the review approvals.

shell/platform/embedder/embedder.h
@@ -572,6 +572,8 @@ typedef struct {
size_t left;
/// Vertical physical location of the top of the window on the screen.
size_t top;
/// Bottom inset of window.
double physical_view_inset_bottom;
Copy link
Contributor

Choose a reason for hiding this comment

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

I suggest adding all 4 inset parameters (top, left, bottom, right) while you're at it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

chinmaygarde
chinmaygarde reviewed May 27, 2021
View reviewed changes
Copy link
Member

@chinmaygarde chinmaygarde left a comment

Choose a reason for hiding this comment

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

can help with the review approvals.

Sure thing. Especially for changes to the public embedder API, it'd be good to be thorough.

shell/common/engine.cc Outdated
@@ -289,7 +289,15 @@ void Engine::SetViewportMetrics(const ViewportMetrics& metrics) {
bool dimensions_changed =
viewport_metrics_.physical_height != metrics.physical_height ||
viewport_metrics_.physical_width != metrics.physical_width ||
viewport_metrics_.device_pixel_ratio != metrics.device_pixel_ratio;
viewport_metrics_.device_pixel_ratio != metrics.device_pixel_ratio ||
viewport_metrics_.physical_view_inset_top !=
Copy link
Member

Choose a reason for hiding this comment

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

Is this necessary? The dimensions of the view itself aren't changing. It's only that the application now has to account for additional insets like notches and such. The new metrics will get delivered to the application no matter what.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It wasn't needed. Removed.

shell/platform/embedder/embedder.cc
@@ -1388,6 +1388,14 @@ FlutterEngineResult FlutterEngineSendWindowMetricsEvent(
metrics.physical_width = SAFE_ACCESS(flutter_metrics, width, 0.0);
metrics.physical_height = SAFE_ACCESS(flutter_metrics, height, 0.0);
metrics.device_pixel_ratio = SAFE_ACCESS(flutter_metrics, pixel_ratio, 1.0);
metrics.physical_view_inset_top =
Copy link
Member

Choose a reason for hiding this comment

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

I realize this is just plumbing things through, but, you could add a test for this in the embedder_unittests harness. Any of the tests in embedder_unittests.cc could be a fine starting point.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

@sagallea sagallea force-pushed the insets branch 2 times, most recently from e9fe2e5 to befaf20 Compare May 27, 2021 21:29
chinmaygarde
chinmaygarde approved these changes Jun 1, 2021
View reviewed changes
Copy link
Member

@chinmaygarde chinmaygarde left a comment

Choose a reason for hiding this comment

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

I don't think this is tested meaningfully, but neither was the old payload. If you want to land this, go for it and I'll add another test for the entire payload. Thanks!

shell/platform/embedder/tests/embedder_unittests.cc
@@ -984,6 +992,10 @@ TEST_F(EmbedderTest, VerifyB143464703WithSoftwareBackend) {
event.width = 1024;
event.height = 600;
event.pixel_ratio = 1.0;
event.physical_view_inset_top = 0.0;
Copy link
Member

Choose a reason for hiding this comment

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

It isn't asserted that these values are actually checked in Dart code. I mean, wouldn't this test pass no matter what values you specified here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

True, I'll add a better test in this change.

Copy link
Contributor

Choose a reason for hiding this comment

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

You should also add a test with non-zero values.

@chinmaygarde chinmaygarde added the waiting for tree to go green This PR is approved and tested, but waiting for the tree to be green to land. label Jun 2, 2021
@fluttergithubbot
Copy link
Contributor

This pull request is not suitable for automatic merging in its current state.

  • The status or check suite Linux Fuchsia has failed. Please fix the issues identified (or deflake) before re-applying this label.

@fluttergithubbot fluttergithubbot removed the waiting for tree to go green This PR is approved and tested, but waiting for the tree to be green to land. label Jun 2, 2021
@iskakaushik
Copy link
Contributor

Hi, the test failure has been fixed ToT, could you please rebase. Thanks.

@sagallea
Add physical view insets to FlutterWindowMetricsEvent
c55c41d 
This is needed to propagate inset values through Flutter.

Bug: b/181683254
@chinmaygarde chinmaygarde merged commit 762af4d into flutter:master Jun 3, 2021
@sagallea sagallea deleted the insets branch June 3, 2021 21:39
@engine-flutter-autoroll engine-flutter-autoroll mentioned this pull request Jun 4, 2021
Merged
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jun 4, 2021
@engine-flutter-autoroll
762af4d Add physical view insets to FlutterWindowMetricsEvent (flutte…
d329870 
…r/engine#26442)
naudzghebre pushed a commit to naudzghebre/engine that referenced this pull request Sep 2, 2021
@sagallea
Add physical view insets to FlutterWindowMetricsEvent (flutter#26442)
eee06d5 
This is needed to propagate inset values through Flutter.

Bug: b/181683254
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dnicoara dnicoara dnicoara requested changes

@chinmaygarde chinmaygarde chinmaygarde approved these changes

Assignees
No one assigned
Labels
cla: yes embedder Related to the embedder API
Projects
None yet
Milestone
No milestone
5 participants
@sagallea @fluttergithubbot @iskakaushik @chinmaygarde @dnicoara