Fix code style issues in MacOS embedder #22270

knopp · 2020-11-03T17:27:28Z

Description

PR to fix code style issues introduced in #21525.

Related Issues

Tests

I added the following tests:

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. If not, delete the remainder of this section.
- I wrote a design doc: https://flutter.dev/go/template Replace this with a link to your design doc's short link
- I got input from the developer relations team, specifically from: Replace with the names of who gave advice
- I wrote a migration guide: https://flutter.dev/go/breaking-changes-template Replace this with a link to a pull request that adds the migration guide to https://flutter.dev/docs/release/breaking-changes

flutter-dashboard · 2020-11-03T17:27:31Z

It looks like this pull request may not have tests. Please make sure to add tests before merging. If you need an exemption to this rule, contact Hixie on the #hackers channel in Chat.

Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing.

iskakaushik · 2020-11-03T17:32:54Z

shell/platform/darwin/macos/BUILD.gn

  ]

  sources += _flutter_framework_headers

  deps = [
-    "//flutter/flow:flow",
    "//flutter/fml:fml",


I think we don't want to depend on fml as well.

Right. Removed.

FML is actually okay; we're going to need it for a11y support, so I've looked into it already, and it's generally very base-level, with minimal dependencies.

In this instance it was only used for single FML_DCHECK call, which I've replaced with NSAssert. I can put it back if FML_DCHECK is better.

stuartmorgan

Some small nits, but overall this looks great. Thanks so much for the quick follow up!

stuartmorgan · 2020-11-03T21:53:10Z

shell/platform/darwin/macos/framework/Source/FlutterResizeSynchronizer.h

+ * Returns whether the view should ensure surfaces with given size;
+ * This will be false during resizing for any size other than size specified
+ * during beginResize.
+ */
 - (bool)shouldEnsureSurfaceForSize:(CGSize)size;


stuartmorgan · 2020-11-03T21:54:17Z

shell/platform/darwin/macos/framework/Source/FlutterResizeSynchronizer.mm


 @interface FlutterResizeSynchronizer () {
-  uint32_t cookie;  // counter to detect stale callbacks
+  // counter to detect stale callbacks


Please use sentence style for all of these comments (capital letter to start, . at end)

stuartmorgan · 2020-11-03T21:55:02Z

shell/platform/darwin/macos/framework/Source/FlutterSurfaceManager.h

 #import <Cocoa/Cocoa.h>

 // Manages the IOSurfaces for FlutterView
 @interface FlutterSurfaceManager : NSObject

- (instancetype)initWithLayer:(CALayer*)layer openGLContext:(NSOpenGLContext*)opengLContext;
+- (instancetype)initWithLayer:(CALayer*)containingLayer
+                openGLContext:(NSOpenGLContext*)opengLContext;


These need nullability annotations.

stuartmorgan · 2020-11-03T21:55:23Z

shell/platform/darwin/macos/framework/Source/FlutterSurfaceManager.mm


 #include <OpenGL/gl.h>
+#import "flutter/shell/platform/darwin/macos/framework/Source/MacOSGLContextSwitch.h"


Blank line above this. I'm surprised clang-format isn't fixing this up.

stuartmorgan · 2020-11-03T21:57:46Z

shell/platform/darwin/macos/framework/Source/FlutterSurfaceManager.mm

-  kFront = 0,
-  kBack = 1,
+  kFrontBuffer = 0,
+  kBackBuffer = 1,
  kBufferCount,


How about kFlutterSurfaceManager{FrontBuffer,BackBuffer,BufferCount}

Sure. I looked at the constants elsewhere (i.e. FlutterTextInputPlugin) which don't seem to be prefixed, but I don't really mind.

Well, the style guide doesn't require prefixing for non-public constants. But my concern here is that it's much more likely that someone would accidentally not prefix a public constant (as happened with some very unfortunately generic constants in embedder.h, for instance) called kBufferCount than, say kClearClientMethod.

No argument here really, fixed in cfbe14bc6.

stuartmorgan · 2020-11-03T21:58:28Z

shell/platform/darwin/macos/framework/Source/FlutterView.h

@@ -20,7 +20,16 @@
 */
 @interface FlutterView : NSView

+/**
+ * Returns the OpenGL context of backing surface.


s/Returns the/The/; properties are generally documented as nouns rather than actions.

stuartmorgan · 2020-11-03T21:59:52Z

shell/platform/darwin/macos/framework/Source/MacOSGLContextSwitch.h

+  MacOSGLContextSwitch(MacOSGLContextSwitch&&) = delete;
+
+ private:
+  NSOpenGLContext* prev_;


previous_. They style guide very strongly discourages variable name abbreviations.

knopp · 2020-11-03T23:19:30Z

@stuartmorgan, these should be fixed now.

stuartmorgan · 2020-11-04T17:15:40Z

Thanks!

knopp added 2 commits November 3, 2020 18:20

Fix code-style issues

939fe89

Removes dependency on flow

dc2e5ab

knopp requested a review from stuartmorgan November 3, 2020 17:27

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

knopp mentioned this pull request Nov 3, 2020

Smooth window resizing on macOS #21525

Merged

12 tasks

iskakaushik reviewed Nov 3, 2020

View reviewed changes

knopp added 3 commits November 3, 2020 20:34

Add missing licenses

5c6cf0c

Remove dependency on fml

bc34af8

Rename constants

1b6ca20

knopp force-pushed the macos_embedder_code_style_fixes branch from 47972cc to 1b6ca20 Compare November 3, 2020 19:34

stuartmorgan requested changes Nov 3, 2020

View reviewed changes

More style fixes

cfbe14b

stuartmorgan approved these changes Nov 4, 2020

View reviewed changes

stuartmorgan 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 4, 2020

fluttergithubbot merged commit acc2466 into flutter:master Nov 4, 2020

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

Roll Engine from c7391d6b12c3 to bf259226b224 (16 revisions) flutter/flutter#69850

Merged

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

acc2466 Fix code style issues in MacOS embedder (flutter/engine#22270)

45c1f1f

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

Fix code style issues in MacOS embedder (flutter#22270)

e57ab05

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix code style issues in MacOS embedder #22270

Fix code style issues in MacOS embedder #22270

knopp commented Nov 3, 2020

flutter-dashboard bot commented Nov 3, 2020

iskakaushik Nov 3, 2020

knopp Nov 3, 2020

stuartmorgan Nov 3, 2020

knopp Nov 3, 2020

stuartmorgan left a comment

stuartmorgan Nov 3, 2020

stuartmorgan Nov 3, 2020

stuartmorgan Nov 3, 2020

stuartmorgan Nov 3, 2020

stuartmorgan Nov 3, 2020

knopp Nov 3, 2020

stuartmorgan Nov 4, 2020

knopp Nov 4, 2020

stuartmorgan Nov 3, 2020

stuartmorgan Nov 3, 2020

knopp commented Nov 3, 2020

stuartmorgan commented Nov 4, 2020


		#include <OpenGL/gl.h>
		#import "flutter/shell/platform/darwin/macos/framework/Source/MacOSGLContextSwitch.h"

Fix code style issues in MacOS embedder #22270

Fix code style issues in MacOS embedder #22270

Conversation

knopp commented Nov 3, 2020

Description

Related Issues

Tests

Checklist

Reviewer Checklist

Breaking Change

flutter-dashboard bot commented Nov 3, 2020

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

Choose a reason for hiding this comment

knopp commented Nov 3, 2020

stuartmorgan commented Nov 4, 2020