Switch macOS embedding to proc table embedder API #21811

stuartmorgan · 2020-10-13T16:30:30Z

Description

Converts the macOS embedding to use the new proc table version of the embedding API, and adds one example unit test using it to demonstrate and validate the approach.

Related Issues

flutter/flutter#66935

Tests

I added the following tests: One example macOS engine test demonstrating mocking an embedder method.

Checklist

Before you create this PR confirm that it meets all requirements listed below by checking the relevant checkboxes ([x]). This will ensure a smooth and quick review process.

I read the contributor guide and followed the process outlined there for submitting PRs.
I signed the CLA.
I read and followed the C++, Objective-C, Java style guides for the engine.
I read the tree hygiene wiki page, which explains my responsibilities.
I updated/added relevant documentation.
All existing and new tests are passing.
I am willing to follow-up on review comments in a timely manner.

Reviewer Checklist

I have submitted any presubmit flakes in this PR using the engine presubmit flakes form before re-triggering the failure.

Breaking Change

Did any tests fail when you ran them? Please read handling breaking changes.

No, no existing tests failed, so this is not a breaking change.
Yes, this is a breaking change.

stuartmorgan · 2020-10-13T16:31:26Z

Let me know if you'd like me to split the macOS changes out into a separate PR; it's easily separable, but I thought it would be useful to have and end-to-end example for initial review.

chinmaygarde · 2020-10-13T16:52:28Z

Let me know if you'd like me to split the macOS changes out into a separate PR.

I'd prefer it if you don't mind. I'd be more comfortable evaluating the usability of the PR on its own if the changes for a specific embedder were separated out. You can even just rework this PR into two separate commits if thats easier.

stuartmorgan · 2020-10-13T17:18:30Z

Split out into #21813; I'll make this a draft for now and then rebase it once that one lands.

stuartmorgan · 2020-10-29T22:18:21Z

There's some minor unit test cleanup here; while adding the example test I found that the test file naming was inconsistent within this embedding, so I standardized it to match naming in the iOS embedding.

cbracken · 2020-10-31T06:11:39Z

shell/platform/darwin/macos/BUILD.gn

-  defines = [ "FLUTTER_FRAMEWORK" ]
+  defines = [
+    "FLUTTER_FRAMEWORK",
+    "FLUTTER_ENGINE_NO_PROTOTYPES",


Looks like this breaks FlutterEngine.mm due to a few symbols defined in the #infndef FLUTTER_ENGINE_NO_PROTOTYPES block added in #21813:

FlutterEngineCollectAOTData

FlutterEngineRunsAOTCompiledDartCode

FlutterEngineCreateAOTData

I apparently didn't rebuild locally after rebasing on master 😳 The code that's not compiling is new, so hadn't been updated to use the proc table.

cbracken

lgtm for the change modulo breakage w.r.t. #21813

…gine#21811)

* c597333 Roll Skia from f548a028ce70 to c21902c0d3cc (46 revisions) (flutter/engine#22224) * 37d766c Fix includes to start with shell (flutter/engine#22227) * 1ad6765 [web] Fixes canvas pixelation and overallocation due to transforms. (flutter/engine#22160) * 9f9fc1f Roll Skia from c21902c0d3cc to 9615bcf71f2a (1 revision) (flutter/engine#22226) * 0faa72e Roll Dart SDK from fed66f60a3bc to 25ef5dc559cf (1 revision) (flutter/engine#22225) * 172a393 Report image diff status for iOS scenario golden tests (flutter/engine#22230) * fddabca updating integration tests version. (flutter/engine#22235) * a36bcdc Roll Fuchsia Mac SDK from mhak7e_o6... to 8SkbMXJJ9... (flutter/engine#22231) * 6a331d3 Roll Skia from 9615bcf71f2a to d5e6368fffd0 (8 revisions) (flutter/engine#22234) * 9b34207 Fixing semantics borders on mobile web (flutter/engine#21856) * 4e9459e Refactored the FlutterEngine to make it easier to implement spawn functionality (flutter/engine#21890) * fa77e68 disable AppLifecycleTests (flutter/engine#22236) * 153775b update golden (flutter/engine#22247) * 50dbe85 [web] fix hot restart type error (flutter/engine#22248) * c8cf09a Roll Skia from d5e6368fffd0 to 7585a65ac709 (7 revisions) (flutter/engine#22237) * 68e2e46 Roll Fuchsia Mac SDK from 8SkbMXJJ9... to Pz4ZHZrUp... (flutter/engine#22246) * d3182bc Roll Dart SDK from 25ef5dc559cf to 5acb5fcf84cb (4 revisions) (flutter/engine#22243) * 9b4bb20 makes android semanticsnode to ignore hittest if it is not focusable (flutter/engine#22205) * 3c7a54e Roll Fuchsia Linux SDK from sNx8qabBn... to QqGvMWaYk... (flutter/engine#22244) * eea98b2 Roll Skia from 7585a65ac709 to dffd20efe95c (14 revisions) (flutter/engine#22250) * 46e3bba Defer Windows arrow key and delete handling (flutter/engine#22207) * 14437d6 fix _getArrayBuffer signature (flutter/engine#22251) * 8defec6 Fix nullability issue with Image.network (flutter/engine#22252) * 67d55ed Roll Dart SDK from 5acb5fcf84cb to a9d583383410 (4 revisions) (flutter/engine#22255) * 7c8f57c Report error when instantiating CanvasKit network image (flutter/engine#22159) * 9945db3 Remove the metrics task from cirrus. (flutter/engine#22240) * bd19181 Add braces on if statements to match linter style (flutter/engine#22130) * e9c62e7 do not print in _computePixelDensity (flutter/engine#22257) * 2617101 Roll Dart SDK from a9d583383410 to d2577410a501 (1 revision) (flutter/engine#22258) * 3d194fa Switch macOS embedding to proc table embedder API (flutter/engine#21811) * 31b6f0b Roll Fuchsia Mac SDK from Pz4ZHZrUp... to 6yEx5GNGG... (flutter/engine#22262) * 78a0181 Roll Fuchsia Linux SDK from QqGvMWaYk... to oLF1FW-gC... (flutter/engine#22264) * ccdb681 WeakPersistentHandle migration (flutter/engine#19843) * ce0a30c Roll Dart SDK from 52783837369d to b43baaaa477d (723 revisions) (flutter/engine#22265) * 59b01e0 [web] Fix repaint logic for cullrect,transform changes (flutter/engine#22273)

Converts the macOS embedding to use the new proc table version of the embedding API, and adds one example unit test using it to demonstrate and validate the approach.

stuartmorgan added affects: engine affects: desktop platform-macos labels Oct 13, 2020

stuartmorgan requested a review from chinmaygarde October 13, 2020 16:30

google-cla bot added the cla: yes label Oct 13, 2020

stuartmorgan changed the title ~~Add a proc table version of embedder API~~ Switch macOS embedding to proc table embedder API Oct 13, 2020

stuartmorgan marked this pull request as draft October 13, 2020 17:18

stuartmorgan added 7 commits October 29, 2020 15:12

Switch macOS to proc table

015cdfd

Standardize unit test file name for macOS to match iOS

f35bb14

Standardize test naming to be less redundant

baa598a

Fix overuse of ASSERT

cbfb035

PoC test, with supporting macro

08e701b

Remove cruft

dd5c644

License file updates

e982221

stuartmorgan removed the request for review from chinmaygarde October 29, 2020 22:13

stuartmorgan force-pushed the darwin-jump-table-tests branch from 1622295 to e982221 Compare October 29, 2020 22:16

stuartmorgan requested a review from cbracken October 29, 2020 22:17

stuartmorgan marked this pull request as ready for review October 29, 2020 22:17

chinmaygarde approved these changes Oct 30, 2020

View reviewed changes

cbracken reviewed Oct 31, 2020

View reviewed changes

cbracken approved these changes Oct 31, 2020

View reviewed changes

stuartmorgan added 3 commits November 2, 2020 09:17

Merge branch 'master' into darwin-jump-table-tests

96062b7

Update recently added code to use the proc table

5f9ff2d

Update function names in test

87d18d9

stuartmorgan merged commit 3d194fa into flutter:master Nov 3, 2020

stuartmorgan deleted the darwin-jump-table-tests branch November 3, 2020 05:44

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

Roll Engine from 346051939814 to 31b6f0b31a48 (31 revisions) flutter/flutter#69648

Closed

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

3d194fa Switch macOS embedding to proc table embedder API (flutter/en…

3464798

…gine#21811)

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

Roll Engine from 346051939814 to 78a018174088 (32 revisions) flutter/flutter#69657

Closed

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

3d194fa Switch macOS embedding to proc table embedder API (flutter/en…

b65af1d

…gine#21811)

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

Roll Engine from 346051939814 to ccdb6819ce53 (33 revisions) flutter/flutter#69672

Closed

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

3d194fa Switch macOS embedding to proc table embedder API (flutter/en…

987582c

…gine#21811)

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

Roll Engine from 346051939814 to ccdb6819ce53 (33 revisions) flutter/flutter#69692

Closed

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

3d194fa Switch macOS embedding to proc table embedder API (flutter/en…

78496e1

…gine#21811)

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

Roll Engine from 346051939814 to 59b01e0e5a3f (35 revisions) flutter/flutter#69709

Merged

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

3d194fa Switch macOS embedding to proc table embedder API (flutter/en…

cb804d9

…gine#21811)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Switch macOS embedding to proc table embedder API #21811

Switch macOS embedding to proc table embedder API #21811

stuartmorgan commented Oct 13, 2020 •

edited

Loading

stuartmorgan commented Oct 13, 2020

chinmaygarde commented Oct 13, 2020

stuartmorgan commented Oct 13, 2020

stuartmorgan commented Oct 29, 2020

cbracken Oct 31, 2020 •

edited

Loading

stuartmorgan Nov 2, 2020

cbracken left a comment

Switch macOS embedding to proc table embedder API #21811

Switch macOS embedding to proc table embedder API #21811

Conversation

stuartmorgan commented Oct 13, 2020 • edited Loading

Description

Related Issues

Tests

Checklist

Reviewer Checklist

Breaking Change

stuartmorgan commented Oct 13, 2020

chinmaygarde commented Oct 13, 2020

stuartmorgan commented Oct 13, 2020

stuartmorgan commented Oct 29, 2020

cbracken Oct 31, 2020 • edited Loading

Choose a reason for hiding this comment

stuartmorgan Nov 2, 2020

Choose a reason for hiding this comment

cbracken left a comment

Choose a reason for hiding this comment

stuartmorgan commented Oct 13, 2020 •

edited

Loading

cbracken Oct 31, 2020 •

edited

Loading