Move common graphics utils to //flutter/common/graphics #22320

iskakaushik · 2020-11-05T15:41:31Z

Move common graphics utils to //flutter/common/graphics

This change creates a new source set //flutter/common/graphics
with minimal dependencies.

The intention with this target is to be later consumed in a
darwin common graphics target across the desktop boundary.

Also unifies some of the graphics utilities under the same folder.

iskakaushik · 2020-11-05T15:42:58Z

@stuartmorgan , I eventually plan on creating darwin/graphics like so: https://github.com/flutter/engine/pull/22303/files#diff-b8669fd3d05e9e8799cf1dd1b38549f32f06b79ab6b7ae326aeb8dc97bca9d4a. This will let us share a bunch of metal logic for example across ios/mac boundaries. And then use this target as a dependency for the mac desktop embedding.

stuartmorgan-g · 2020-11-05T17:31:37Z

graphics/BUILD.gn

+    "//flutter/shell/version:version",
+    "//third_party/rapidjson",
+    "//third_party/skia",
+  ]


Shouldn't this have public_configs = [ "//flutter:config" ] like most other targets?

stuartmorgan-g · 2020-11-05T17:36:45Z

graphics/persistent_cache.cc

@@ -2,7 +2,7 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.

-#include "flutter/shell/common/persistent_cache.h"
+#include "flutter/graphics/persistent_cache.h"


I'm not wild about having flutter/graphics have a dependency into flutter/shell (for version). I know version is essentially self-contained, but shell/* is exactly what we don't want embeddings built on embedder.h to have arbitrary dependencies into. That's the whole reason for this move, after all.

I think we need a clearer definition of what dependencies are allowable, and a directory structure that makes the line much crisper. (I really wish we had Chromium's tooling for whitelisting/blacklisting cross-directory dependencies set up.)

I agree, I was contemplating moving version out of shell to flutter root. Do you think that would make things easier to reason about?

I'm not sure how many things we want to move to the root; these two things are probably not going to be the last things we want to share.

I'm increasingly thinking we should instead have a new directory in shell, something like shell/embedding_utils (it's not a great name, but naming it is non-trivial, especially given the existing common, which now isn't), and make that the firewall, and put graphics and version in there, along with an explanatory README.

I'll workshop a name in the engine channel.

@stuartmorgan given that this will likely take some thinking. I'd like to not let this PR go stale. We can always move this folder to the right home once we decide on it. I've also added a warning message in the gn file to not add additional deps.

My concern is that this gives the appearance of being isolated from shell/ by moving out of shell, but since it actually does depend back into shell that's not actually the case.

Even if we don't think this is a final location, what's the argument for having the interim location be both top-level and having dependencies into shell?

I will move this into //flutter/shell/graphics/

What do you think of //flutter/common/graphics so we don't inflate the root? Maybe version can also go there as both of them don't seem to rely on shell to exist.

graphics/BUILD.gn

liyuqian

Moving gl_context_switch, texture, persistent_cache into graphics makes sense to me. @flar and I were also discussing where to put common graphics code shared between flow and lib/ui. I guess graphics would be a nice place once this lands.

iskakaushik · 2020-11-10T17:44:28Z

@stuartmorgan , @liyuqian I've moved it to //flutter/common/graphics. I will move version to //flutter/common in a follow-up commit.

stuartmorgan-g

LGTM once the header guards are fixed.

stuartmorgan-g · 2020-11-10T18:20:31Z

common/graphics/persistent_cache.h

@@ -2,8 +2,8 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.

-#ifndef FLUTTER_SHELL_COMMON_PERSISTENT_CACHE_H_
-#define FLUTTER_SHELL_COMMON_PERSISTENT_CACHE_H_
+#ifndef FLUTTER_GRAPHICS_PERSISTENT_CACHE_H_


All the header guards need updating again.

liyuqian

LGTM. BTW, some include needs to be reordered. For example

-#include "flutter/fml/logging.h"
 #include "flutter/common/graphics/persistent_cache.h"
+#include "flutter/fml/logging.h"

This change creates a new source set //flutter/common/graphics with minimal dependencies. The intention with this target is to be later consumed in a darwin common graphics target across the desktop boundary. Also unifies some of the graphics utilities under the same folder.

…ter/engine#22320)

* 03e750d Roll Fuchsia Linux SDK from g6EuxMthn... to DzZi2gPbF... (flutter/engine#22417) * 80e9a3f Roll Skia from 84d503b21322 to 5b8598952931 (7 revisions) (flutter/engine#22418) * 80cc0fa Roll Skia from 5b8598952931 to 02dd0ed8ce5e (1 revision) (flutter/engine#22419) * d14c4a7 Roll Skia from 02dd0ed8ce5e to fb5850f41043 (4 revisions) (flutter/engine#22420) * af185be Roll Skia from fb5850f41043 to 008d63e23dab (6 revisions) (flutter/engine#22421) * 1a13dac Simplify API for scheduling Skia object deletions (flutter/engine#22409) * 49299c3 Roll Skia from 008d63e23dab to 267826c86552 (4 revisions) (flutter/engine#22422) * 25e0829 Roll Fuchsia Mac SDK from w10eytxvc... to e-4Jm-yWa... (flutter/engine#22423) * 76e6158 Roll Skia from 267826c86552 to 88e8bb2fe2d5 (3 revisions) (flutter/engine#22424) * caf678d Move common graphics utils to //flutter/common/graphics (flutter/engine#22320)

iskakaushik requested review from stuartmorgan-g and liyuqian November 5, 2020 15:41

flutter-dashboard bot added platform-android platform-ios labels Nov 5, 2020

google-cla bot added the cla: yes label Nov 5, 2020

stuartmorgan-g suggested changes Nov 5, 2020

View reviewed changes

liyuqian reviewed Nov 5, 2020

View reviewed changes

iskakaushik force-pushed the core-graphics branch from deeb293 to 81d246d Compare November 10, 2020 17:42

iskakaushik changed the title ~~Move common graphics utils to //flutter/graphics~~ Move common graphics utils to //flutter/shell/common/graphics Nov 10, 2020

iskakaushik force-pushed the core-graphics branch from 81d246d to 1158ee9 Compare November 10, 2020 18:19

iskakaushik changed the title ~~Move common graphics utils to //flutter/shell/common/graphics~~ Move common graphics utils to //flutter/common/graphics Nov 10, 2020

stuartmorgan-g approved these changes Nov 10, 2020

View reviewed changes

iskakaushik force-pushed the core-graphics branch from 1158ee9 to 8a16d19 Compare November 10, 2020 18:45

liyuqian approved these changes Nov 10, 2020

View reviewed changes

iskakaushik force-pushed the core-graphics branch from 8a16d19 to 7980b4a Compare November 10, 2020 19:32

iskakaushik added the waiting for tree to go green This PR is approved and tested, but waiting for the tree to be green to land. label Nov 10, 2020

fluttergithubbot merged commit caf678d into flutter:master Nov 10, 2020

engine-flutter-autoroll mentioned this pull request Nov 10, 2020

Roll Engine from 4e0f7ab788bd to caf678d30fc0 (10 revisions) flutter/flutter#70231

Merged

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Nov 10, 2020

caf678d Move common graphics utils to //flutter/common/graphics (flut…

a2acc16

…ter/engine#22320)

chaselatta pushed a commit to chaselatta/engine that referenced this pull request Nov 30, 2020

Move common graphics utils to //flutter/common/graphics (flutter#22320)

cb9b682

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Move common graphics utils to //flutter/common/graphics #22320

Move common graphics utils to //flutter/common/graphics #22320

Uh oh!

iskakaushik commented Nov 5, 2020 •

edited

Loading

Uh oh!

iskakaushik commented Nov 5, 2020

Uh oh!

stuartmorgan-g Nov 5, 2020

Uh oh!

stuartmorgan-g Nov 5, 2020

Uh oh!

iskakaushik Nov 5, 2020

Uh oh!

stuartmorgan-g Nov 5, 2020

Uh oh!

iskakaushik Nov 6, 2020

Uh oh!

stuartmorgan-g Nov 9, 2020

Uh oh!

iskakaushik Nov 9, 2020

Uh oh!

liyuqian Nov 9, 2020

Uh oh!

Uh oh!

liyuqian left a comment

Uh oh!

iskakaushik commented Nov 10, 2020 •

edited

Loading

Uh oh!

stuartmorgan-g left a comment

Uh oh!

stuartmorgan-g Nov 10, 2020

Uh oh!

liyuqian left a comment

Uh oh!

Uh oh!

Move common graphics utils to //flutter/common/graphics #22320

Move common graphics utils to //flutter/common/graphics #22320

Uh oh!

Conversation

iskakaushik commented Nov 5, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

iskakaushik commented Nov 5, 2020

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!

Uh oh!

liyuqian left a comment

Choose a reason for hiding this comment

Uh oh!

iskakaushik commented Nov 10, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

stuartmorgan-g left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

liyuqian left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

iskakaushik commented Nov 5, 2020 •

edited

Loading

iskakaushik commented Nov 10, 2020 •

edited

Loading