Skip to content
This repository was archived by the owner on Feb 25, 2025. It is now read-only.

Conversation

yjbanov
Copy link
Contributor

@yjbanov yjbanov commented Feb 13, 2020

No description provided.

@yjbanov yjbanov requested a review from mdebbar February 13, 2020 21:21
@auto-assign auto-assign bot requested a review from iskakaushik February 13, 2020 21:21
@yjbanov yjbanov force-pushed the better-surface-state-info branch from 644d3db to ff81113 Compare February 13, 2020 21:31
/// Verifies that the [surface] is in one of the valid states.
///
/// This function should be used inside an assertion expression.
bool debugAssertSurfaceState(PersistedSurface surface, PersistedSurfaceState state1, [PersistedSurfaceState state2, PersistedSurfaceState state3]) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm curious why you chose this signature vs passing an array of states.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Which one is better

This?

assert(debugAssertSurfaceState(oldSurface, PersistedSurfaceState.active, PersistedSurfaceState.pendingUpdate));

Or this?

assert(debugAssertSurfaceState(oldSurface, const <PersistedSurfaceState>[PersistedSurfaceState.active, PersistedSurfaceState.pendingUpdate]))

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah right. Explicit types :(

bool debugAssertSurfaceState(PersistedSurface surface, PersistedSurfaceState state1, [PersistedSurfaceState state2, PersistedSurfaceState state3]) {
final List<PersistedSurfaceState> validStates = [ state1, state2, state3 ];

if (validStates.contains(surface.state)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Is surface.state nullable? If it's null, it could mistakenly pass this check.

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 is not nullable. We have an assert for that.

Copy link
Contributor

Choose a reason for hiding this comment

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

I can't wait for Null Safety!

@yjbanov
Copy link
Contributor Author

yjbanov commented Feb 14, 2020

Build redness is Fuchsia infra flakiness

@yjbanov yjbanov merged commit 8b0b649 into flutter:master Feb 14, 2020
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Feb 15, 2020
jason-simmons pushed a commit to flutter/flutter that referenced this pull request Feb 18, 2020
* c0549fb Roll buildroot. (flutter/engine#16613)

* 8b0b649 improve surface state assert error messages (flutter/engine#16595)

* cd77e78 Fix drawRRect failure when shader is specified (flutter/engine#16601)

* fe63094 [web] Handle alignment correctly in Paragraph.getPositionForOffset (flutter/engine#16569)

* 65d1126 [web] Fixing launching Safari. This should solve the LUCI issue (flutter/engine#16590)

* f88f7df [web] Unskip tests that are already passing in Safari (flutter/engine#16567)

* 594f660 [shell tests] Integrate Vulkan with Shell Tests

* 400ed7c Revert "[shell tests] Integrate Vulkan with Shell Tests"

* 15e7f51 Implement Path extractPath, tangent APIs (flutter/engine#16599)

* 4941ff7 Remove usage of Dart_AllocateWithNativeFields from tonic (flutter/engine#16588)

* bb01cb7 Roll fuchsia/sdk/core/linux-amd64 from Bmq1m... to J-_s6... (flutter/engine#16592)
NoamDev pushed a commit to NoamDev/engine that referenced this pull request Feb 27, 2020
NoamDev added a commit to NoamDev/engine that referenced this pull request Feb 27, 2020
@yjbanov yjbanov deleted the better-surface-state-info branch June 22, 2021 21:19
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants