Initial import of GLFW Linux shell from FDE #8159

stuartmorgan · 2019-03-13T21:27:35Z

The initial commit in this PR is the flutter-desktop-embedding Linux implementation essentially as-is, but integrated into the engine build. The main things that need review there are:

GN files, which are substantially changed from FDE
The basic file location (there was no clear consensus in previous discussion, so I used cpp as a straw man for the location of this code, which will be shared between Linux and Windows when I do the Windows migration shortly)

The second commit switches from jsoncpp to RapidJSON, which could also use review. @franciscojma86 please take a look at that part too.

The last two commits are mechanical changes to stop using Embedder in prefixes (and in code and comments in general, to avoid confusion), and to reformat the files with Flutter's format.

Changes include: - File structure - Header guards - Include paths - Namespaces - Integration with the engine's GN build Doesn't yet build due to use of jsoncpp.

RapidJSON is already in use in the engine, so rather than introduce a second JSON library, converts all JSON handling to rapidjson. More error-handling is needed in the text code, but since there's a pending restructuring of that code full error handling is left for a follow-up.

stuartmorgan · 2019-03-13T21:27:43Z

I expect that landing this will be blocked on adding a script to apt-get the necessary dependencies and running that on the build bots; I'll put that together now, but wanted to get this under review in parallel.

chinmaygarde · 2019-03-13T21:31:24Z

Exciting.

chinmaygarde

Publishing comments about BUILD system and source code organization while I do a pass over the source.

chinmaygarde · 2019-03-13T22:15:07Z

shell/platform/cpp/BUILD.gn

+# Use of this source code is governed by a BSD-style license that can be
+# found in the LICENSE file.
+
+import("$flutter_root/common/config.gni")


The convention followed is //flutter/shell/platform/<name_of_platform>. Since cpp is not a platform, this location does not make sense. Please move this to platform/linux. If this is going to be a thing on Windows too, platform/glfw with its own subdirectory for linux and Windows specific files (if any).

Moved the code into a combination of flutter/glfw and flutter/common/cpp per discussion.

chinmaygarde · 2019-03-13T22:19:56Z

shell/platform/cpp/BUILD.gn

+source_set("common_cpp_library_headers") {
+  public = _public_headers
+
+  defines = _common_defines


Please use gn configs for this purpose (see gn help config).

chinmaygarde · 2019-03-13T22:21:06Z

shell/platform/cpp/BUILD.gn

+}
+
+source_set("common_cpp") {
+  public = [


Ditto about the use of configs. Use a public and private config and targets can selectively import. For the SDK export, you can use a variable that both that copy target and the configs reference.

Per offline discussion, I'm not clear on what to change here, and the decisions was to revisit this in a follow-up.

chinmaygarde · 2019-03-13T22:22:44Z

common/config.gni

@@ -16,6 +16,9 @@ declare_args() {

  # The runtime mode ("debug", "profile", "release", "dynamic_profile", or "dynamic_release")
  flutter_runtime_mode = "debug"
+
+  # Whether or not to use the GLFW desktop shell implementation.
+  use_glfw_desktop = is_linux || is_win


This variable is not common to all targets. Please move this into a new config.gni file in platform/foo with its own declare_args block.

I ended up removing this in the re-org; the reason it was here already (even though turning it off wouldn't work yet) was to help enforce a separation between code that would be used for any C++ implementation and code that's GLFW-specific. With the reorg the file structure does that even more strongly, so the variable isn't needed.

shell/platform/BUILD.gn

chinmaygarde · 2019-03-13T22:26:59Z

shell/platform/cpp/client_wrapper/BUILD.gn

+  ]
+}
+
+copy("_publish_includes") {


Not sure why the target names have the _ prefix. If you meant to restrict visibility, use visibility = [ ":*" ] (see gn help visibility) and get rid of the prefix. That will prevent incorrect use by GN.

Yep, I misread the GN docs and thought the _ applied to targets as well as variables. Fixed.

chinmaygarde

Another take with minor nits about convention and such.

chinmaygarde · 2019-03-13T22:33:14Z

shell/platform/cpp/client_wrapper/include/flutter/basic_message_channel.h

+  BasicMessageChannel(BinaryMessenger* messenger,
+                      const std::string& name,
+                      const MessageCodec<T>* codec)
+      : messenger_(messenger), name_(name), codec_(codec) {}


The engine uses the = default convention.

I assume you meant this for the next line, since this constructor takes arguments and thus isn't a default constructor.

Done for all default constructors/destructors that weren't already using default.

chinmaygarde · 2019-03-13T22:33:49Z

shell/platform/cpp/client_wrapper/include/flutter/basic_message_channel.h

+  ~BasicMessageChannel() {}
+
+  // Prevent copying.
+  BasicMessageChannel(BasicMessageChannel const&) = delete;


I suggest adding a macro.h file with these decorations for reuse. Seems like boilerplate that is repeated elsewhere.

The DISALLOW_COPY-style macro you are describing is no longer allowed by Google style; explicit deletion is the preferred approach. See
https://google.github.io/styleguide/cppguide.html#Copyable_Movable_Types
and
https://google.github.io/styleguide/cppguide.html#Preprocessor_Macros

chinmaygarde · 2019-03-13T22:35:44Z

shell/platform/cpp/client_wrapper/include/flutter/glfw/flutter_window_controller.h

+#include <vector>
+
+#ifdef FLUTTER_DESKTOP_LIBRARY
+#include "flutter/shell/platform/cpp/public/flutter_glfw.h"


Please modify the header search paths in the GN rule to not have to do this.

chinmaygarde · 2019-03-13T22:38:20Z

shell/platform/cpp/client_wrapper/include/flutter/json_method_codec.h

+  JsonMethodCodec() = default;
+
+  // MethodCodec:
+  std::unique_ptr<MethodCall<JsonValueType>> DecodeMethodCallInternal(


By convention, every overridden method name has a comment right above it listing the superclass that specifies the virtual method. See files like this as an example. Also a single empty newline after each method definition.

Done everywhere.

chinmaygarde · 2019-03-13T22:40:08Z

shell/platform/cpp/client_wrapper/glfw/flutter_window_controller.cc

+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#ifdef FLUTTER_DESKTOP_LIBRARY


Please modify the header search paths to make this consistent.

chinmaygarde · 2019-03-13T22:40:46Z

shell/platform/cpp/client_wrapper/README

@@ -0,0 +1,9 @@
+This code is intended to be built into plugins and applications to provide


I didn't see a unittest target for this. If there are not currently available, please create a stub target so folks know where to begin.

So far FDE didn't have any tests :( Added test targets for the client wrappers, some infrastructure for writing non-trivial tests, and a couple of starter tests. It'll need a bunch of test coverage as follow-up.

chinmaygarde · 2019-03-13T22:41:16Z

shell/platform/cpp/client_wrapper/engine_method_result.cc

+
+ReplyManager::ReplyManager(BinaryReply reply_handler)
+    : reply_handler_(std::move(reply_handler)) {
+  if (!reply_handler_) {


Can this just be an assertion?

stuartmorgan

All comments from the last round should be addressed; PTAL

stuartmorgan · 2019-03-13T23:46:45Z

shell/platform/cpp/client_wrapper/include/flutter/basic_message_channel.h

+  ~BasicMessageChannel() {}
+
+  // Prevent copying.
+  BasicMessageChannel(BasicMessageChannel const&) = delete;


The DISALLOW_COPY-style macro you are describing is no longer allowed by Google style; explicit deletion is the preferred approach. See
https://google.github.io/styleguide/cppguide.html#Copyable_Movable_Types
and
https://google.github.io/styleguide/cppguide.html#Preprocessor_Macros

stuartmorgan · 2019-03-14T19:35:54Z

shell/platform/cpp/BUILD.gn

+source_set("common_cpp_library_headers") {
+  public = _public_headers
+
+  defines = _common_defines


stuartmorgan · 2019-03-14T19:40:49Z

shell/platform/cpp/client_wrapper/BUILD.gn

+  ]
+}
+
+copy("_publish_includes") {


Yep, I misread the GN docs and thought the _ applied to targets as well as variables. Fixed.

stuartmorgan · 2019-03-14T19:46:48Z

shell/platform/cpp/client_wrapper/engine_method_result.cc

+
+ReplyManager::ReplyManager(BinaryReply reply_handler)
+    : reply_handler_(std::move(reply_handler)) {
+  if (!reply_handler_) {


stuartmorgan · 2019-03-14T19:57:42Z

shell/platform/cpp/client_wrapper/include/flutter/basic_message_channel.h

+  BasicMessageChannel(BinaryMessenger* messenger,
+                      const std::string& name,
+                      const MessageCodec<T>* codec)
+      : messenger_(messenger), name_(name), codec_(codec) {}


I assume you meant this for the next line, since this constructor takes arguments and thus isn't a default constructor.

Done for all default constructors/destructors that weren't already using default.

stuartmorgan · 2019-03-14T21:33:44Z

shell/platform/cpp/BUILD.gn

+}
+
+source_set("common_cpp") {
+  public = [


Per offline discussion, I'm not clear on what to change here, and the decisions was to revisit this in a follow-up.

stuartmorgan · 2019-03-15T02:39:01Z

shell/platform/cpp/BUILD.gn

+# Use of this source code is governed by a BSD-style license that can be
+# found in the LICENSE file.
+
+import("$flutter_root/common/config.gni")


Moved the code into a combination of flutter/glfw and flutter/common/cpp per discussion.

stuartmorgan · 2019-03-15T02:39:45Z

shell/platform/cpp/client_wrapper/include/flutter/glfw/flutter_window_controller.h

+#include <vector>
+
+#ifdef FLUTTER_DESKTOP_LIBRARY
+#include "flutter/shell/platform/cpp/public/flutter_glfw.h"


shell/platform/BUILD.gn

stuartmorgan · 2019-03-19T20:53:36Z

shell/platform/cpp/client_wrapper/README

@@ -0,0 +1,9 @@
+This code is intended to be built into plugins and applications to provide


So far FDE didn't have any tests :( Added test targets for the client wrappers, some infrastructure for writing non-trivial tests, and a couple of starter tests. It'll need a bunch of test coverage as follow-up.

chinmaygarde

Admittedly, this PR is a bit too large for me to review thoroughly (and I am sure it is unwieldy for you to have it one another branch while under review). I think the structure is fine and I have done my best to go over everything. Let's land this ASAP and tinker on it once it is in the tree. LGTM. Thanks!

)

flutter/engine@d8c89e2...188adf7 git log d8c89e2..188adf7 --no-merges --oneline 188adf7 Removed Activity reference from AccessibilityBridge by using a View for insets instead of the Activity (#18115) (flutter/engine#8231) d452dd5 Initial import of GLFW Linux shell from FDE (flutter/engine#8159) 5120045 Remove jsoncpp from desktop Linux shell setup (flutter/engine#8230) The AutoRoll server is located here: https://autoroll.skia.org/r/flutter-engine-flutter-autoroll Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+/master/autoroll/README.md If the roll is causing failures, please contact the current sheriff (liyuqian@google.com), and stop the roller if necessary.

Changes include: - File structure - Header guards - Include paths - Namespaces - Integration with the engine's GN build - Conversion from jsoncpp to rapidjson - Style and clang-format adjustment to match engine repository

This reverts commit f0ef655.

stuartmorgan added 4 commits March 13, 2019 17:13

Initial import of Linux shell from flutter-desktop-embedding repository.

ddede28

Changes include: - File structure - Header guards - Include paths - Namespaces - Integration with the engine's GN build Doesn't yet build due to use of jsoncpp.

Remove use of 'embedder' in names/comments

f3f2873

clang-format and GN format

d1ba818

stuartmorgan requested a review from chinmaygarde March 13, 2019 21:27

googlebot added the cla: yes label Mar 13, 2019

chinmaygarde reviewed Mar 13, 2019

View reviewed changes

stuartmorgan added 5 commits March 13, 2019 22:58

Sync with recent FDE change

63f1b81

Review feedback: simple style fixes

82bef49

Reorganize files into platform/glfw and platform/common/cpp

a090f79

Add unit test targets for the client wrappers

efe922e

Remove BUILD deps to the new targets for now

a3d8a01

stuartmorgan commented Mar 19, 2019

View reviewed changes

chinmaygarde approved these changes Mar 20, 2019

View reviewed changes

stuartmorgan added 3 commits March 20, 2019 14:03

Merge branch 'master' into linux-shell-import

1cfdbdb

clang-format

2b4fdf2

Update license file list

cc758fb

stuartmorgan merged commit d452dd5 into flutter:master Mar 20, 2019

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

d452dd5 Initial import of GLFW Linux shell from FDE (flutter/engine#8159

33088bd

)

engine-flutter-autoroll mentioned this pull request Mar 21, 2019

Roll engine d8c89e2bfe9b..188adf7e5ec5 (3 commits) flutter/flutter#29709

Merged

RBogie added a commit to RBogie/flutter-engine that referenced this pull request Apr 8, 2019

Revert "Initial import of GLFW Linux shell from FDE (flutter#8159)"

965895c

This reverts commit f0ef655.

RBogie added a commit to RBogie/flutter-engine that referenced this pull request Apr 8, 2019

Revert "Initial import of GLFW Linux shell from FDE (flutter#8159)"

6bf69ac

This reverts commit f0ef655.

		@@ -0,0 +1,9 @@
		This code is intended to be built into plugins and applications to provide

Initial import of GLFW Linux shell from FDE #8159

Initial import of GLFW Linux shell from FDE #8159

Conversation

stuartmorgan commented Mar 13, 2019

stuartmorgan commented Mar 13, 2019

chinmaygarde commented Mar 13, 2019

chinmaygarde left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chinmaygarde left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stuartmorgan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chinmaygarde left a comment • edited Loading

Choose a reason for hiding this comment

chinmaygarde left a comment •

edited

Loading