PlatformChannel concept added to support interop #8394

mravn-google · 2017-02-24T16:12:16Z

Matches flutter/engine#3446 on flutter/engine that introduces a FlutterChannel on the platform side.
Unit tests pending.
Ready for design review.

abarth · 2017-02-24T18:49:07Z

examples/platform_services/android/app/src/main/AndroidManifest.xml

@@ -10,7 +10,7 @@

    <application android:name="io.flutter.app.FlutterApplication" android:label="@string/app_name" >
        <activity
-            android:name=".ExampleActivity"
+            android:name="com.example.flutter.ExampleActivity"


Is there a reason we're using com.example.platformservices on line 3 and com.example.flutter here?

No. The original manifest package was wrong, and I was unaware of the . prefix shorthand. Fixed.

abarth · 2017-02-24T18:50:50Z

examples/platform_services/lib/main.dart

-        )
-      )
-    );
+    return new Material(child: new Center(child: new Column(


If you add trailing commas everywhere there are permitted and run dartfmt, you'll get code that matches https://github.com/flutter/flutter/wiki/Style-guide-for-Flutter-repo in this example.

abarth · 2017-02-24T18:51:08Z

examples/platform_services/lib/main.dart

-      _latitude = reply['latitude'].toDouble();
-      _longitude = reply['longitude'].toDouble();
+      _locationRequest =
+        new PlatformChannel('geo').invokeMethod('getLocation', 'network');


abarth · 2017-02-24T18:53:44Z

packages/flutter/lib/src/services/platform_messages.dart

+  /// * [String]s
+  /// * [List]s of supported values
+  /// * [Map]s from strings to supported values
+  static const MessageCodec json = const _JSONCodec();


Why not make _JSONCodec a public class?

abarth · 2017-02-24T18:54:30Z

packages/flutter/lib/src/services/platform_messages.dart

+  /// * [Uint8List]s, [Int32List]s, [Int64List]s, [Float64List]s
+  /// * [List]s of supported values
+  /// * [Map]s from supported values to supported values
+  static const MessageCodec standard = const _StandardCodec();


Same here.

The way you've structured this code, these two are special children. It would be better to structure things in a way that they're not special and someone could use a different codec (e.g., protobufs) easily.

abarth · 2017-02-24T18:55:57Z

packages/flutter/lib/src/services/platform_messages.dart

+  /// Encodes the specified method call in binary.
+  ///
+  /// The [name] of the method must be non-null. The [arguments] may be `null`.
+  ByteData encodeMethodCall(String name, dynamic arguments);


As in the Java code review, I think we should disentangle the methodcall-based and message-based codecs. If necessary, we can have separate PlatformChannel classes to drive them. For example, we could have a PlatformMessageChannel and a PlatformMethodChannel (or some other better naming scheme.

abarth · 2017-02-24T18:58:15Z

packages/flutter/lib/src/services/platform_messages.dart

+/// populate a [PlatformException].
+///
+/// All operations throw [FormatException], if conversion fails.
+abstract class MessageCodec {


I'd break this file up into three separate files:

One file that has the abstract classes for MessageCodec and MethodCodec.

Another file that defines concrete implementations of these codecs for "standard" and "JSON". This separation will prevent these codecs from being special children because (2) will be able to import (1) but (1) should not import (2).

A third file that contains the PlatformChannel class that has the pretty front-ends.

abarth · 2017-02-24T18:59:16Z

packages/flutter/lib/src/services/platform_messages.dart

+  }
+}
+
+class _WriteBuffer {


I'd move this out into foundation.

abarth · 2017-02-24T19:00:04Z

packages/flutter/lib/src/services/platform_messages.dart

+
+  void putInt32(int value) {
+    _fourBytes.setInt32(0, value);
+    buffer.addAll(_fourBytesAsList);


Is this really more efficient than doing the bit smashing manually?

Why not just get a ByteData view of buffer.buffer and use setInt32 directly?

Direct bit-smashing applied for ints.

For doubles, it seems I would need to create a new ByteData view instance of buffer.buffer for each double added, because the buffer is dynamically extended. Retained original approach for now.

abarth · 2017-02-24T19:04:00Z

packages/flutter/lib/src/services/platform_messages.dart

+  }
+}
+
+class _StandardCodec implements MessageCodec {


As requested in the Java review, please document this protocol. Here are examples where we've documented similar protocols:

https://github.com/flutter/engine/blob/master/lib/ui/text.dart#L184
https://github.com/flutter/engine/blob/master/lib/ui/painting.dart#L310

Doesn't have to be anything fancy. Just enough documentation so that someone could write, for example, a C# implementation of the protocol without having to reverse engineer the other implementations.

abarth · 2017-03-01T07:37:16Z

examples/platform_services/lib/main.dart

-          ]
-        )
-      )
+            new FutureBuilder<dynamic>(


abarth · 2017-03-01T07:38:40Z

examples/platform_services/lib/main.dart

    setState(() {
-      _latitude = reply['latitude'].toDouble();
-      _longitude = reply['longitude'].toDouble();
+      _locationRequest = new PlatformMethodChannel('geo').invokeMethod(


Is it recommend to make a new channel for each request? If so, I wonder if we can make it const constructible. No action needed. Just wondering aloud.

abarth · 2017-03-01T07:38:55Z

packages/flutter/lib/src/foundation/serialization.dart

@@ -0,0 +1,204 @@
+// Copyright 2015 The Chromium Authors. All rights reserved.


abarth · 2017-03-01T07:40:44Z

packages/flutter/lib/src/foundation/serialization.dart

+    if (Endianness.HOST_ENDIAN == Endianness.BIG_ENDIAN) {
+      _buffer.addAll(list.buffer.asUint8List(list.offsetInBytes, 4 * list.length));
+    } else {
+      for (final int value in list) {


We don't usually put final here but we probably should.

abarth · 2017-03-01T07:42:08Z

packages/flutter/lib/src/services/message_codec.dart

+
+
+/// Thrown to indicate that a platform interaction resulted in an error.
+class PlatformException implements Exception {


Why have a PlatformException that's different from FlutterException?

I see. These are errors reported from the other side of the channel. Would you be willing to add a bit more to the dartdoc explaining that? I thought these were errors in the mechanism itself rather than errors from the platform-specific implementation on the other side.

abarth · 2017-03-01T07:42:37Z

packages/flutter/lib/src/services/message_codec.dart

+/// A message encoding/decoding mechanism.
+///
+/// Both operations throw [FormatException], if conversion fails.
+abstract class MessageCodec<T> {


mravn-google · 2017-03-01T07:44:50Z

packages/flutter/lib/src/services/message_codecs.dart

+  // * Strings are encoded using their UTF-8 representation. First the length
+  //   of that in bytes is encoded using the expanding format, then follows the
+  //   UTF-8 encoding itself.
+  // * Uint8Lists, Int32Lists, Int64Lists, and Float64Lists are encoded by first


Document alignment and byte ordering.

abarth · 2017-03-01T07:45:38Z

packages/flutter/lib/src/services/message_codecs.dart

+  //   type byte (Lists are assumed to be heterogeneous).
+  // * Maps are encoded by first encoding their length in the expanding format,
+  //   then follows the recursive encoding of each key/value pair, including the
+  //   type byte for both (Maps are assumed to be heterogeneous).


Perfect. Thank you.

abarth · 2017-03-01T07:46:21Z

packages/flutter/lib/src/services/message_codecs.dart

+  ByteData encodeMessage(dynamic message) {
+    if (message == null) {
+      return null;
+    }


https://github.com/flutter/flutter/wiki/Style-guide-for-Flutter-repo#avoid-using-braces-for-one-line-long-control-structure-statements

abarth · 2017-03-01T07:47:01Z

packages/flutter/lib/src/services/platform_channel.dart

@@ -0,0 +1,198 @@
+// Copyright 2016 The Chromium Authors. All rights reserved.


abarth · 2017-03-01T07:48:15Z

packages/flutter/lib/src/services/platform_channel.dart

+  PlatformMethodChannel(this.name, [this.codec = const StandardMethodCodec()]) {
+    assert(name != null);
+    assert(codec != null);
+  }


Looks like it could be const constructible except for the asserts.

abarth · 2017-03-01T07:48:45Z

packages/flutter/lib/src/services/platform_channel.dart

+        onListen: () async {
+          PlatformMessages.setBinaryMessageHandler(
+              name,
+                  (ByteData reply) async {


We'd usually combine this line and the previous one.

abarth · 2017-03-01T07:49:41Z

packages/flutter/lib/src/services/platform_channel.dart

+            controller.addError(e);
+          }
+        }, onCancel: () async {
+      PlatformMessages.setBinaryMessageHandler(name, null);


This should be indented to match line 181, right?

Argh - the indentation is completely garbled in this class! Appears my IntelliJ does that on copy/paste. I moved nicely formatted code from PlatformMessages...

abarth · 2017-03-01T07:51:15Z

packages/flutter/lib/src/widgets/async.dart

+  T get requireData {
+    if (hasData)
+      return data;
+    else if (hasError)


I don't see it explicitly in the style guide, but generally we skip the else after return or throw statements. (Also applies to line 216

abarth · 2017-03-01T07:51:40Z

abarth · 2017-03-01T07:53:12Z

If I were writing this patch, I'd smoke test each type of channel against a mock implementation. (I wouldn't try to hit every type of encoded message, but I'd just makes sure messages and errors can get through as expected.)

This looks great. Just some trivial nits.

mravn-google · 2017-03-01T07:54:39Z

Thanks a lot Adam. Smoke test is in the works. Could I get you to take a look at the Android side too: flutter/engine#3446 Mikkel

…

On Wed, Mar 1, 2017 at 8:53 AM, Adam Barth ***@***.***> wrote: If I were writing this patch, I'd smoke test each type of channel against a mock implementation. (I wouldn't try to hit every type of encoded message, but I'd just makes sure messages and errors can get through as expected.) This looks great. Just some trivial nits. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#8394 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AV33XQ6jOpfecEW8OKv5aj1qJYew1iw9ks5rhSPtgaJpZM4MLYYl> .

-- Mikkel Nygaard Ravn Software Engineer

…example

Hixie · 2017-03-01T19:00:30Z

packages/flutter/lib/src/foundation/serialization.dart

+  ByteData _eightBytes;
+  Uint8List _eightBytesAsList;
+
+  WriteBuffer() {


nit: here and in the class below, please move the constructor to above the fields. We put constructors first so that someone quickly glancing at the code can quickly tell if the class has an implicit constructor or explicit constructors.

Hixie · 2017-03-01T19:05:01Z

packages/flutter/lib/src/services/message_codecs.dart

+  }
+
+  static void _writeSize(WriteBuffer buffer, int value) {
+    assert(0 <= value && value < 0xffffffff);


why not <= 0xffffffff ?

I guess it's so that you can expand the format later to store even bigger ints?

your decoder just reads 0xffffffff as that, though.

Expanding the format further was not the intention. I just had myself confused, that's all. Fix upcoming.

Hixie · 2017-03-01T19:07:53Z

packages/flutter/lib/src/services/message_codecs.dart

+        buffer.putUint8(_kLargeInt);
+        final List<int> hex = UTF8.encoder.convert(value.toRadixString(16));
+        _writeSize(buffer, hex.length);
+        buffer.putUint8List(hex);


haha, that escalated fast. :-P

I agree. It's a temporary solution. Make it work, make it right, make it fast...

Hixie · 2017-03-01T19:11:25Z

packages/flutter/lib/src/foundation/serialization.dart

+      list = data.buffer.asInt64List(data.offsetInBytes + position, length);
+    } else {
+      final ByteData invertedData = new ByteData(8 * length);
+      for (int i = 0; i < length; i++) {


nit: no braces around single-line blocks

Hixie · 2017-03-01T19:12:20Z

packages/flutter/lib/src/foundation/serialization.dart

+
+  void _alignTo(int alignment) {
+    final int mod = position % alignment;
+    if (mod != 0) {


nit: similar here and elsewhere

Hixie · 2017-03-01T19:17:03Z

packages/flutter/lib/src/services/platform_messages.dart

@@ -1,4 +1,4 @@
-// Copyright 2016 The Chromium Authors. All rights reserved.
+// Copyright 2017 The Chromium Authors. All rights reserved.


nit: (don't fix this, but for future knowledge): don't change the copyright dates.

Hixie · 2017-03-01T19:17:29Z

packages/flutter/lib/src/services/platform_messages.dart

@@ -104,6 +103,8 @@ class PlatformMessages {
  ///
  /// Returns a [Future] which completes to the received response, decoded as a
  /// UTF-8 string, or to an error, if the decoding fails.
+  ///
+  /// Deprecated, use [PlatformMessageChannel.send] instead.


Mark the deprecated methods using @deprecated?

(or just remove them)

Yes, that's the plan. Just wanted to land this PR first, then I'll remove uses of these methods.

Hixie · 2017-03-01T19:18:26Z

This is great. Thanks so much for refactoring all this into this much saner mechanism.

googlebot added the cla: yes label Feb 24, 2017

mravn-google requested review from abarth, Hixie, szakarias, mit-mit and jakobr-google February 24, 2017 16:16

mravn-google mentioned this pull request Feb 24, 2017

FlutterChannel concept added to FlutterView to support interop flutter/engine#3446

Merged

abarth reviewed Feb 24, 2017

View reviewed changes

abarth reviewed Mar 1, 2017

View reviewed changes

examples/platform_services/lib/main.dart

]

)

)

new FutureBuilder<dynamic>(

Copy link

Contributor

abarth Mar 1, 2017

Choose a reason for hiding this comment

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

Nice.

abarth reviewed Mar 1, 2017

View reviewed changes

mravn-google commented Mar 1, 2017

View reviewed changes

abarth reviewed Mar 1, 2017

View reviewed changes

mravn-google added 14 commits March 1, 2017 14:18

PlatformChannel concept with std and json codecs

5b871e8

Support for typed_data lists in std codec. Updated platform_messages …

c17fad9

…example

Incorporated review suggestions

b5ceabd

Incorporated review suggestions

7d1a8a8

Deprecated superseded methods

909560e

Incorporated review suggestions

cde05b5

Implemented review suggestions

d469ca2

Split json and std message and method codecs

06e6c11

Handled byte ordering issue with Int32List, Int64List, and Float64List

99c34fe

Updated dartdoc

8c62928

Implemented review suggestions

c0b38ef

Added more dartdoc

fea56c8

Temporarily removed API deprecations to make travis happy

8a5ed71

Engine roll

62b65f2

mravn-google merged commit 390993d into flutter:master Mar 1, 2017

Hixie reviewed Mar 1, 2017

View reviewed changes

mravn-google mentioned this pull request Mar 1, 2017

PlatformMessageChannel: code cleanup, bugfixes, and unit tests #8509

Merged

github-actions bot locked as resolved and limited conversation to collaborators Aug 14, 2021

		@@ -0,0 +1,204 @@
		// Copyright 2015 The Chromium Authors. All rights reserved.



		/// Thrown to indicate that a platform interaction resulted in an error.
		class PlatformException implements Exception {

		@@ -0,0 +1,198 @@
		// Copyright 2016 The Chromium Authors. All rights reserved.

		@@ -1,4 +1,4 @@
		// Copyright 2016 The Chromium Authors. All rights reserved.
		// Copyright 2017 The Chromium Authors. All rights reserved.

PlatformChannel concept added to support interop #8394

PlatformChannel concept added to support interop #8394

Uh oh!

Conversation

mravn-google commented Feb 24, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

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!

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!

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!

abarth commented Mar 1, 2017

mravn-google commented Feb 24, 2017 •

edited

Loading