Skip to content

Commit 8b0b649

Browse files
yjbanovyjbanov
authored
improve surface state assert error messages (#16595)
1 parent c0549fb commit 8b0b649

File tree

3 files changed

+89
-39
lines changed

3 files changed

+89
-39
lines changed

‎lib/web_ui/lib/src/engine/surface/scene_builder.dart‎

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -41,7 +41,7 @@ class SurfaceSceneBuilder implements ui.SceneBuilder {
4141
// the live tree.
4242
if (surface.oldLayer != null) {
4343
assert(surface.oldLayer.runtimeType == surface.runtimeType);
44-
assert(surface.oldLayer.isActive);
44+
assert(debugAssertSurfaceState(surface.oldLayer, PersistedSurfaceState.active));
4545
surface.oldLayer.state = PersistedSurfaceState.pendingUpdate;
4646
}
4747
_adoptSurface(surface);
@@ -264,7 +264,9 @@ class SurfaceSceneBuilder implements ui.SceneBuilder {
264264
@override
265265
void addRetained(ui.EngineLayer retainedLayer) {
266266
final PersistedContainerSurface retainedSurface = retainedLayer;
267-
assert(retainedSurface.isActive || retainedSurface.isReleased);
267+
if (assertionsEnabled) {
268+
assert(debugAssertSurfaceState(retainedSurface, PersistedSurfaceState.active, PersistedSurfaceState.released));
269+
}
268270
retainedSurface.tryRetain();
269271
_adoptSurface(retainedSurface);
270272
}

‎lib/web_ui/lib/src/engine/surface/surface.dart‎

Lines changed: 69 additions & 37 deletions
Original file line numberDiff line numberDiff line change
@@ -77,7 +77,7 @@ void commitScene(PersistedScene scene) {
7777
if (_retainedSurfaces.isNotEmpty) {
7878
for (int i = 0; i < _retainedSurfaces.length; i++) {
7979
final PersistedSurface retainedSurface = _retainedSurfaces[i];
80-
assert(retainedSurface.isPendingRetention);
80+
assert(debugAssertSurfaceState(retainedSurface, PersistedSurfaceState.pendingRetention));
8181
retainedSurface.state = PersistedSurfaceState.active;
8282
}
8383
_retainedSurfaces = <PersistedSurface>[];
@@ -483,6 +483,39 @@ enum PersistedSurfaceState {
483483
released,
484484
}
485485

486+
class PersistedSurfaceException implements Exception {
487+
PersistedSurfaceException(this.surface, this.message);
488+
489+
final PersistedSurface surface;
490+
final String message;
491+
492+
@override
493+
String toString() {
494+
if (assertionsEnabled) {
495+
return '${surface.runtimeType}: $message';
496+
}
497+
return super.toString();
498+
}
499+
}
500+
501+
/// Verifies that the [surface] is in one of the valid states.
502+
///
503+
/// This function should be used inside an assertion expression.
504+
bool debugAssertSurfaceState(PersistedSurface surface, PersistedSurfaceState state1, [PersistedSurfaceState state2, PersistedSurfaceState state3]) {
505+
final List<PersistedSurfaceState> validStates = [ state1, state2, state3 ];
506+
507+
if (validStates.contains(surface.state)) {
508+
return true;
509+
}
510+
511+
throw PersistedSurfaceException(
512+
surface,
513+
'is in an unexpected state.\n'
514+
'Expected one of: ${validStates.whereType<PersistedSurfaceState>().join(', ')}\n'
515+
'But was: ${surface.state}',
516+
);
517+
}
518+
486519
/// A node in the tree built by [SceneBuilder] that contains information used to
487520
/// compute the fewest amount of mutations necessary to update the browser DOM.
488521
abstract class PersistedSurface implements ui.EngineLayer {
@@ -537,7 +570,7 @@ abstract class PersistedSurface implements ui.EngineLayer {
537570
/// reused before the request to retain it came in. In this case, the surface
538571
/// is [revive]d and rebuilt from scratch.
539572
void tryRetain() {
540-
assert(isActive || isReleased);
573+
assert(debugAssertSurfaceState(this, PersistedSurfaceState.active, PersistedSurfaceState.released));
541574
// Request that the layer is retained, but only if it's still active. It
542575
// could have been released.
543576
if (isActive) {
@@ -559,7 +592,7 @@ abstract class PersistedSurface implements ui.EngineLayer {
559592
@visibleForTesting
560593
@protected
561594
void revive() {
562-
assert(isReleased);
595+
assert(debugAssertSurfaceState(this, PersistedSurfaceState.released));
563596
state = PersistedSurfaceState.created;
564597
}
565598

@@ -636,7 +669,7 @@ abstract class PersistedSurface implements ui.EngineLayer {
636669
}
637670
}
638671
assert(rootElement == null);
639-
assert(isCreated);
672+
assert(debugAssertSurfaceState(this, PersistedSurfaceState.created));
640673
rootElement = createElement();
641674
applyWebkitClipFix(rootElement);
642675
if (_debugExplainSurfaceStats) {
@@ -656,7 +689,7 @@ abstract class PersistedSurface implements ui.EngineLayer {
656689
@mustCallSuper
657690
void adoptElements(covariant PersistedSurface oldSurface) {
658691
assert(oldSurface.rootElement != null);
659-
assert(oldSurface.isActive || oldSurface.isPendingUpdate);
692+
assert(debugAssertSurfaceState(oldSurface, PersistedSurfaceState.active, PersistedSurfaceState.pendingUpdate));
660693
assert(() {
661694
if (oldSurface.isPendingUpdate) {
662695
final PersistedContainerSurface self = this;
@@ -684,7 +717,8 @@ abstract class PersistedSurface implements ui.EngineLayer {
684717
void update(covariant PersistedSurface oldSurface) {
685718
assert(oldSurface != null);
686719
assert(!identical(oldSurface, this));
687-
assert(isCreated && (oldSurface.isPendingUpdate || oldSurface.isActive));
720+
assert(debugAssertSurfaceState(this, PersistedSurfaceState.created));
721+
assert(debugAssertSurfaceState(oldSurface, PersistedSurfaceState.active, PersistedSurfaceState.pendingUpdate));
688722

689723
adoptElements(oldSurface);
690724

@@ -729,7 +763,7 @@ abstract class PersistedSurface implements ui.EngineLayer {
729763
@protected
730764
@mustCallSuper
731765
void discard() {
732-
assert(isActive);
766+
assert(debugAssertSurfaceState(this, PersistedSurfaceState.active));
733767
assert(rootElement != null);
734768
// TODO(yjbanov): it may be wasteful to recursively disassemble the DOM tree
735769
// node by node. It should be sufficient to detach the root
@@ -919,8 +953,7 @@ abstract class PersistedContainerSurface extends PersistedSurface {
919953

920954
/// Adds a child to this container.
921955
void appendChild(PersistedSurface child) {
922-
assert(child.isCreated || child.isPendingRetention || child.isPendingUpdate,
923-
'Child is in incorrect state ${child.state}');
956+
assert(debugAssertSurfaceState(child, PersistedSurfaceState.created, PersistedSurfaceState.pendingRetention, PersistedSurfaceState.pendingUpdate));
924957
_children.add(child);
925958
child.parent = this;
926959
}
@@ -957,10 +990,10 @@ abstract class PersistedContainerSurface extends PersistedSurface {
957990
} else if (child is PersistedContainerSurface && child.oldLayer != null) {
958991
final PersistedSurface oldLayer = child.oldLayer;
959992
assert(oldLayer.rootElement != null);
960-
assert(oldLayer.isPendingUpdate);
993+
assert(debugAssertSurfaceState(oldLayer, PersistedSurfaceState.pendingUpdate));
961994
child.update(child.oldLayer);
962995
} else {
963-
assert(child.isCreated);
996+
assert(debugAssertSurfaceState(child, PersistedSurfaceState.created));
964997
assert(child.rootElement == null);
965998
child.build();
966999
}
@@ -984,10 +1017,10 @@ abstract class PersistedContainerSurface extends PersistedSurface {
9841017

9851018
@override
9861019
void update(PersistedContainerSurface oldSurface) {
987-
assert(oldSurface.isActive || oldSurface.isPendingUpdate);
1020+
assert(debugAssertSurfaceState(oldSurface, PersistedSurfaceState.active, PersistedSurfaceState.pendingUpdate));
9881021
assert(runtimeType == oldSurface.runtimeType);
9891022
super.update(oldSurface);
990-
assert(oldSurface.isReleased);
1023+
assert(debugAssertSurfaceState(oldSurface, PersistedSurfaceState.released));
9911024

9921025
if (oldSurface._children.isEmpty) {
9931026
_updateZeroToMany(oldSurface);
@@ -1019,8 +1052,7 @@ abstract class PersistedContainerSurface extends PersistedSurface {
10191052
}
10201053
for (int i = 0; i < _children.length; i++) {
10211054
final PersistedSurface newChild = _children[i];
1022-
assert(newChild.isActive || newChild.isPendingRetention,
1023-
'New child is in incorrect state ${newChild.state}');
1055+
assert(debugAssertSurfaceState(newChild, PersistedSurfaceState.active, PersistedSurfaceState.pendingRetention));
10241056
assert(newChild.rootElement != null);
10251057
assert(newChild.rootElement.parent == childContainer);
10261058
}
@@ -1043,17 +1075,17 @@ abstract class PersistedContainerSurface extends PersistedSurface {
10431075
final PersistedSurface newChild = _children[i];
10441076
if (newChild.isPendingRetention) {
10451077
newChild.retain();
1046-
assert(newChild.isPendingRetention);
1078+
assert(debugAssertSurfaceState(newChild, PersistedSurfaceState.pendingRetention));
10471079
} else if (newChild is PersistedContainerSurface &&
10481080
newChild.oldLayer != null) {
10491081
final PersistedContainerSurface oldLayer = newChild.oldLayer;
1050-
assert(oldLayer.isPendingUpdate);
1082+
assert(debugAssertSurfaceState(oldLayer, PersistedSurfaceState.pendingUpdate));
10511083
newChild.update(oldLayer);
1052-
assert(oldLayer.isReleased);
1053-
assert(newChild.isActive);
1084+
assert(debugAssertSurfaceState(oldLayer, PersistedSurfaceState.released));
1085+
assert(debugAssertSurfaceState(newChild, PersistedSurfaceState.active));
10541086
} else {
10551087
newChild.build();
1056-
assert(newChild.isActive);
1088+
assert(debugAssertSurfaceState(newChild, PersistedSurfaceState.active));
10571089
}
10581090
assert(newChild.rootElement != null);
10591091
containerElement.append(newChild.rootElement);
@@ -1093,14 +1125,14 @@ abstract class PersistedContainerSurface extends PersistedSurface {
10931125
newChild.retain();
10941126

10951127
_discardActiveChildren(oldSurface);
1096-
assert(newChild.isPendingRetention);
1128+
assert(debugAssertSurfaceState(newChild, PersistedSurfaceState.pendingRetention));
10971129
return;
10981130
}
10991131

11001132
// Updated child is moved to the correct location in the tree; all others
11011133
// are released.
11021134
if (newChild is PersistedContainerSurface && newChild.oldLayer != null) {
1103-
assert(newChild.oldLayer.isPendingUpdate);
1135+
assert(debugAssertSurfaceState(newChild.oldLayer, PersistedSurfaceState.pendingUpdate));
11041136
assert(newChild.rootElement == null);
11051137
assert(newChild.oldLayer.rootElement != null);
11061138

@@ -1113,12 +1145,12 @@ abstract class PersistedContainerSurface extends PersistedSurface {
11131145

11141146
newChild.update(oldLayer);
11151147
_discardActiveChildren(oldSurface);
1116-
assert(oldLayer.isReleased);
1117-
assert(newChild.isActive);
1148+
assert(debugAssertSurfaceState(oldLayer, PersistedSurfaceState.released));
1149+
assert(debugAssertSurfaceState(newChild, PersistedSurfaceState.active));
11181150
return;
11191151
}
11201152

1121-
assert(newChild.isCreated);
1153+
assert(debugAssertSurfaceState(newChild, PersistedSurfaceState.created));
11221154

11231155
PersistedSurface bestMatch;
11241156
double bestScore = 2.0;
@@ -1135,19 +1167,19 @@ abstract class PersistedContainerSurface extends PersistedSurface {
11351167
}
11361168

11371169
if (bestMatch != null) {
1138-
assert(bestMatch.isActive);
1170+
assert(debugAssertSurfaceState(bestMatch, PersistedSurfaceState.active));
11391171
newChild.update(bestMatch);
11401172

11411173
// Move the HTML node if necessary.
11421174
if (newChild.rootElement.parent != childContainer) {
11431175
childContainer.append(newChild.rootElement);
11441176
}
11451177

1146-
assert(bestMatch.isReleased);
1178+
assert(debugAssertSurfaceState(bestMatch, PersistedSurfaceState.released));
11471179
} else {
11481180
newChild.build();
11491181
childContainer.append(newChild.rootElement);
1150-
assert(newChild.isActive);
1182+
assert(debugAssertSurfaceState(newChild, PersistedSurfaceState.active));
11511183
}
11521184

11531185
// Child nodes that were not used this frame that are still active and not
@@ -1202,29 +1234,29 @@ abstract class PersistedContainerSurface extends PersistedSurface {
12021234
final PersistedSurface newChild = _children[bottomInNew];
12031235
if (newChild.isPendingRetention) {
12041236
newChild.retain();
1205-
assert(newChild.isPendingRetention);
1237+
assert(debugAssertSurfaceState(newChild, PersistedSurfaceState.pendingRetention));
12061238
} else if (newChild is PersistedContainerSurface &&
12071239
newChild.oldLayer != null) {
12081240
final PersistedContainerSurface oldLayer = newChild.oldLayer;
1209-
assert(oldLayer.isPendingUpdate);
1241+
assert(debugAssertSurfaceState(oldLayer, PersistedSurfaceState.pendingUpdate));
12101242
newChild.update(oldLayer);
1211-
assert(oldLayer.isReleased);
1212-
assert(newChild.isActive);
1243+
assert(debugAssertSurfaceState(oldLayer, PersistedSurfaceState.released));
1244+
assert(debugAssertSurfaceState(newChild, PersistedSurfaceState.active));
12131245
} else {
12141246
final PersistedSurface matchedOldChild = matches[newChild];
12151247
if (matchedOldChild != null) {
1216-
assert(matchedOldChild.isActive);
1248+
assert(debugAssertSurfaceState(matchedOldChild, PersistedSurfaceState.active));
12171249
newChild.update(matchedOldChild);
1218-
assert(matchedOldChild.isReleased);
1219-
assert(newChild.isActive);
1250+
assert(debugAssertSurfaceState(matchedOldChild, PersistedSurfaceState.released));
1251+
assert(debugAssertSurfaceState(newChild, PersistedSurfaceState.active));
12201252
} else {
12211253
newChild.build();
1222-
assert(newChild.isActive);
1254+
assert(debugAssertSurfaceState(newChild, PersistedSurfaceState.active));
12231255
}
12241256
}
12251257
insertDomNodeIfMoved(newChild);
12261258
assert(newChild.rootElement != null);
1227-
assert(newChild.isActive || newChild.isPendingRetention);
1259+
assert(debugAssertSurfaceState(newChild, PersistedSurfaceState.active, PersistedSurfaceState.pendingRetention));
12281260
nextSibling = newChild;
12291261
}
12301262

‎lib/web_ui/test/engine/surface/surface_test.dart‎

Lines changed: 16 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -15,6 +15,22 @@ void main() {
1515
SurfaceSceneBuilder.debugForgetFrameScene();
1616
});
1717

18+
test('debugAssertSurfaceState produces a human-readable message', () {
19+
final SceneBuilder builder = SceneBuilder();
20+
final PersistedOpacity opacityLayer = builder.pushOpacity(100);
21+
try {
22+
debugAssertSurfaceState(opacityLayer, PersistedSurfaceState.active, PersistedSurfaceState.pendingRetention);
23+
fail('Expected $PersistedSurfaceException');
24+
} on PersistedSurfaceException catch (exception) {
25+
expect(
26+
'$exception',
27+
'PersistedOpacity: is in an unexpected state.\n'
28+
'Expected one of: PersistedSurfaceState.active, PersistedSurfaceState.pendingRetention\n'
29+
'But was: PersistedSurfaceState.created',
30+
);
31+
}
32+
});
33+
1834
test('is created', () {
1935
final SceneBuilder builder = SceneBuilder();
2036
final PersistedOpacity opacityLayer = builder.pushOpacity(100);

0 commit comments

Comments
 (0)