Add a BroadcastStream to FrameTiming #11041

liyuqian · 2019-08-15T22:53:11Z

Without this, developers have to override onReportTimings to listen for FrameTiming.
That can potentially break previous onReportTimings listeners if they forget to call
the old listener in their new callback.

This PR replaces the similar RP in the framework: flutter/flutter#38574

Once this PR landed, we'll have to create another framework PR to use the stream to replace
onReportTimings usages.

Once that's done, we can then propose the breaking change of removing the deprecated
onReportTimings.

devoncarew

My main comment (also in-line) is that we may want to change the name from:

Stream<FrameTiming> get frameTimingStream

to

Stream<FrameTiming> get frameTimings

devoncarew · 2019-08-16T15:53:41Z

lib/ui/window.dart

 typedef FrameCallback = void Function(Duration duration);

-/// Signature for [Window.onReportTimings].
+/// Signature for [Window._onReportTimings].


I'd have this continue point to onReportTimings. The TimingsCallback can be made private at the same time that onReportTimings is.

+1 We shouldn't be referencing private members in a doc comment. The doc comment should include that this way of doing it is deprecated and link to the new way.

Ok, I'll add ignore: deprecated usage... then. I previously changed onReportTiming to _onReportTimings mainly to silence the analyzer warnings.

devoncarew · 2019-08-16T15:55:31Z

lib/ui/window.dart

+  /// See also:
+  ///
+  ///  * [FrameTiming], the data event of this stream
+  Stream<FrameTiming> get frameTimingStream {


You might instead call this frameTimings; I don't think we need the return type (Stream) in the name.

devoncarew · 2019-08-16T15:58:34Z

lib/ui/window.dart

+
+  void _onFrameTimingListen() {
+    _internalSetOnReportTimings((List<FrameTiming> timings) {
+      timings.forEach(_frameTimingBroadcastController.add);


Is it more efficient to call timings.forEach(_frameTimingBroadcastController.add), or:

for (FrameTiming timing in timings) { _frameTimingBroadcastController.add(timing); }

I believe forEach is ok here since we are passing in a method directly and do not allocate a closure.

If I write a for ( in ) loop, the analyzer will complain... It forces me to write forEach.

goderbauer · 2019-08-16T16:40:58Z

lib/ui/window.dart

 typedef FrameCallback = void Function(Duration duration);

-/// Signature for [Window.onReportTimings].
+/// Signature for [Window._onReportTimings].


+1 We shouldn't be referencing private members in a doc comment. The doc comment should include that this way of doing it is deprecated and link to the new way.

goderbauer · 2019-08-16T16:45:40Z

lib/ui/window.dart

+  ///
+  /// If no one is listening to this stream, no additional work will be done.
+  /// Otherwise, Flutter spends less than 0.1ms every 1 second to report the
+  /// timings (measured on iPhone6S). The 0.1ms is about 0.6% of 16ms (frame


uber-nit: I believe the official spelling is "iPhone 6s"

goderbauer · 2019-08-16T16:46:09Z

lib/ui/window.dart

+  /// [FrameTiming.buildDuration] and [FrameTiming.rasterDuration]), or high
+  /// latencies (through [FrameTiming.totalSpan]).
+  ///
+  /// Unlike [Timeline], the timing information here is available in the release


drop the "the" before "release", "profile" and "debug"?

goderbauer · 2019-08-16T16:46:48Z

lib/ui/window.dart

+
+  void _onFrameTimingListen() {
+    _internalSetOnReportTimings((List<FrameTiming> timings) {
+      timings.forEach(_frameTimingBroadcastController.add);


I believe forEach is ok here since we are passing in a method directly and do not allocate a closure.

devoncarew

lgtm for my part

liyuqian · 2019-08-19T16:58:22Z

@goderbauer : any more suggestions? Otherwise I'll merge today.

goderbauer

LGTM

* 19327c8 Show all license diffs (flutter/engine#11034) * e97ed36 Add a BroadcastStream to FrameTiming (flutter/engine#11041) * 21ae926 [b/139487101] Dont present session twice (flutter/engine#11222)

Filed: flutter#38838 to track these usages. This was introduced by: flutter/engine#11041

This reverts commit e97ed36.

Filed: #38838 to track these usages. This was introduced by: flutter/engine#11041

This is the continuation of flutter/engine#11041 and #38574 This is not a breaking change as we're not removing `onReportTimings` API. We're simply removing the use of it in our framework.

Hixie · 2019-09-01T06:47:31Z

We try pretty hard not to use streams in Flutter, they have all kinds of problems. What was wrong with the old API?

This reverts commit e97ed36.

dnfield · 2019-09-03T23:11:36Z

@Hixie do you think we could get a section in the style guide on when and when not to use streams?

This reverts commit e97ed36. Reason for revert: Stream is considered a bad API: #11041 (comment)

Add a BroadcastStream to FrameTiming

cefdb13

googlebot added cla: yes labels Aug 15, 2019

liyuqian requested review from devoncarew and goderbauer August 15, 2019 22:53

liyuqian mentioned this pull request Aug 15, 2019

Add a broadcast stream for FrameTiming flutter/flutter#38574

Closed

Fix deprecated calls

855b274

liyuqian force-pushed the broadcast branch from 5f3f735 to 855b274 Compare August 16, 2019 00:34

devoncarew reviewed Aug 16, 2019

View reviewed changes

goderbauer reviewed Aug 16, 2019

View reviewed changes

More fixes

ae505ab

devoncarew approved these changes Aug 16, 2019

View reviewed changes

Fix web_ui

7440bfb

goderbauer approved these changes Aug 19, 2019

View reviewed changes

liyuqian merged commit e97ed36 into flutter:master Aug 19, 2019

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Aug 19, 2019

e97ed36 Add a BroadcastStream to FrameTiming (flutter/engine#11041)

eee7731

engine-flutter-autoroll mentioned this pull request Aug 19, 2019

Roll engine 7292d47e615a..e97ed36e580c (2 commits) flutter/flutter#38822

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Aug 19, 2019

e97ed36 Add a BroadcastStream to FrameTiming (flutter/engine#11041)

7cbaa82

engine-flutter-autoroll mentioned this pull request Aug 19, 2019

Roll engine 7292d47e615a..21ae92651a38 (3 commits) flutter/flutter#38828

Merged

iskakaushik pushed a commit to iskakaushik/flutter that referenced this pull request Aug 19, 2019

Fix analyzer issues for onReportTiming to frameTiming

502644a

Filed: flutter#38838 to track these usages. This was introduced by: flutter/engine#11041

iskakaushik mentioned this pull request Aug 19, 2019

Fix analyzer issues for onReportTiming to frameTiming flutter/flutter#38840

Merged

iskakaushik pushed a commit to iskakaushik/engine that referenced this pull request Aug 19, 2019

Revert "Add a BroadcastStream to FrameTiming (flutter#11041)"

fc6f72a

This reverts commit e97ed36.

iskakaushik added a commit to flutter/flutter that referenced this pull request Aug 19, 2019

Fix analyzer issues for onReportTiming to frameTiming (#38840)

d2bc74a

Filed: #38838 to track these usages. This was introduced by: flutter/engine#11041

This was referenced Aug 19, 2019

Add analyze to engine framework tests flutter/flutter#38848

Closed

Replace deprecated onReportTimings w/ frameTimings flutter/flutter#38861

Merged

Expose FrameTiming with multiple listeners instead of a simple callback flutter/flutter#39277

Closed

liyuqian mentioned this pull request Aug 29, 2019

Add multiple listeners capability to onReportTimings flutter/flutter#35133

Closed

liyuqian mentioned this pull request Sep 3, 2019

Revert "Replace deprecated onReportTimings w/ frameTimings" flutter/flutter#39778

Merged

liyuqian added a commit to liyuqian/engine that referenced this pull request Sep 3, 2019

Revert "Add a BroadcastStream to FrameTiming (flutter#11041)"

c74bff3

This reverts commit e97ed36.

liyuqian mentioned this pull request Sep 3, 2019

Revert "Add a BroadcastStream to FrameTiming (#11041)" #11841

Merged

liyuqian added a commit that referenced this pull request Sep 5, 2019

Revert "Add a BroadcastStream to FrameTiming (#11041)" (#11841)

4243324

This reverts commit e97ed36. Reason for revert: Stream is considered a bad API: #11041 (comment)

Add a BroadcastStream to FrameTiming #11041

Add a BroadcastStream to FrameTiming #11041

Uh oh!

Conversation

liyuqian commented Aug 15, 2019

Uh oh!

devoncarew left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

devoncarew left a comment

Choose a reason for hiding this comment

Uh oh!

liyuqian commented Aug 19, 2019

Uh oh!

goderbauer left a comment

Choose a reason for hiding this comment

Uh oh!

Hixie commented Sep 1, 2019

Uh oh!

dnfield commented Sep 3, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants