Incorporate View.setSystemGestureExclusionRects code review feedback from #11441 #11804

shihaohong · 2019-08-31T05:53:40Z

A follow-up to #11441. Changes include:

Revert MethodChannel.MethodCallHandler definition into inline-class
Updated test names to itDoesSomethingWhenSomethingHappens format. (The new ones are a little long in its current state)
Remove unnecessary ResultsMock class
Denoted test execution steps with comments

…tureExclusionRects success case

matthew-carroll

Thanks for revisiting this. Just a few more comments for clarity in the tests. Also, I'm wondering if 2 of the tests might be able to collapse down to one.

matthew-carroll · 2019-09-03T20:17:51Z

shell/platform/android/test/io/flutter/embedding/engine/systemchannels/PlatformChannelTest.java

        platformChannel.setPlatformMessageHandler(platformMessageHandler);
+        Result result = mock(Result.class);
+
+        int top = 0;


Let's avoid placing the values in variables. I think it only makes it more difficult to see what's going into the rect. Generally we hard code input and output in tests to eliminate all possible logic in tests.

matthew-carroll · 2019-09-03T20:21:04Z

shell/platform/android/test/io/flutter/embedding/engine/systemchannels/PlatformChannelTest.java

+        int bottom = 250;
+        int left = 0;
+
+        JSONObject JsonRect = new JSONObject();


Please start with lowercase letters.

matthew-carroll · 2019-09-03T20:22:22Z

shell/platform/android/test/io/flutter/embedding/engine/systemchannels/PlatformChannelTest.java

+        JsonRect.put("right", right);
+        JsonRect.put("bottom", bottom);
+        JsonRect.put("left", left);
+        JSONArray inputRects = new JSONArray();


Since channels go 2 directions, it may not be clear what "input" means here. Perhaps jsonGestureInsetsFromPlatform would clarify?

I updated the variable name to jsonExclusionRectsFromPlatform in both instances you mentioned in this review

matthew-carroll · 2019-09-03T20:23:54Z

shell/platform/android/test/io/flutter/embedding/engine/systemchannels/PlatformChannelTest.java

+        platformChannel.setPlatformMessageHandler(platformMessageHandler);
+        Result result = mock(Result.class);

        int top = 0;


Let's remove these variables, too.

Looks like these variables should still be removed.

matthew-carroll · 2019-09-03T20:24:30Z

shell/platform/android/test/io/flutter/embedding/engine/systemchannels/PlatformChannelTest.java

-        ResultsMock resultsMock = mock(ResultsMock.class);
        JSONObject JsonRect = new JSONObject();
        JsonRect.put("top", top);
        JsonRect.put("right", right);


I can't comment on the right line, but let's consider an improved name for inputRects here as well.

matthew-carroll · 2019-09-03T20:25:15Z

shell/platform/android/test/io/flutter/embedding/engine/systemchannels/PlatformChannelTest.java

+    }
+
+    @Test
+    public void itProperlyDecodesGestureRectsWhenSettingSystemGestureExclusionRects() throws JSONException {


I'm curious, what's the difference in testing value between this test and the one before it? Are we testing different things?

I isolated the two because:

the first one tests for whether or not a result.success response is sent to the framework when properly formatted JSON data comes from the framework.

the second one tests for whether or not correctly formatted JSON data is properly transformed into ArrayList<Rect>, which is what View.setSystemGestureExclusionRects expects as its input. This ensures that we are feeding the Android API the correct data

The main idea for my isolation here was to distill two, separate expected outcomes from the same logical flow

…cation blocks

matthew-carroll · 2019-09-04T22:31:49Z

shell/platform/android/test/io/flutter/embedding/engine/systemchannels/PlatformChannelTest.java

-        MethodCall callGetSystemGestureExclusionRects = new MethodCall(
-            "SystemGestures.getSystemGestureExclusionRects",
-            null
+        int top = 0;


Here are a couple more variables we can remove.

matthew-carroll

LGTM. Thanks.

…eedback from flutter#11441 (flutter/engine#11804)

@keep

[email protected]:flutter/engine.git/compare/cd920398e40e...edee1fe git log cd92039..edee1fe --no-merges --oneline 2019-09-05 [email protected] Add @keep annotation (flutter/engine#11893) 2019-09-05 [email protected] Roll src/third_party/dart d0218d4771..be66176534 (11 commits) 2019-09-05 [email protected] Incorporate View.setSystemGestureExclusionRects code review feedback from #11441 (flutter/engine#11804) 2019-09-05 [email protected] Support build windows release/profile embedding builds (flutter/engine#11475) 2019-09-05 [email protected] Roll src/third_party/skia e784f75beb20..adecf4b6d5fe (6 commits) (flutter/engine#11900) 2019-09-05 [email protected] remove extra redundant channels setup in iOS embedding engine (flutter/engine#11886) 2019-09-05 [email protected] Revert "Add a BroadcastStream to FrameTiming (#11041)" (flutter/engine#11841) 2019-09-05 [email protected] Roll fuchsia/sdk/core/linux-amd64 from OmqaW... to Z7PSg... (flutter/engine#11898) 2019-09-05 [email protected] Roll fuchsia/sdk/core/mac-amd64 from S-f_C... to -AQkJ... (flutter/engine#11897) 2019-09-05 [email protected] Roll src/third_party/skia b0e2347fedfc..e784f75beb20 (1 commits) (flutter/engine#11896) 2019-09-05 [email protected] Roll src/third_party/dart 6eed35b60d..d0218d4771 (5 commits) 2019-09-05 [email protected] Roll src/third_party/skia 97218352addb..b0e2347fedfc (4 commits) (flutter/engine#11894) 2019-09-05 [email protected] Roll src/third_party/dart 67bb2b7819..6eed35b60d (18 commits) 2019-09-05 [email protected] Roll fuchsia/sdk/core/linux-amd64 from LKWtB... to OmqaW... (flutter/engine#11891) 2019-09-05 [email protected] Roll fuchsia/sdk/core/mac-amd64 from m-hNV... to S-f_C... (flutter/engine#11889) 2019-09-05 [email protected] Roll src/third_party/skia 166751ef578a..97218352addb (1 commits) (flutter/engine#11888) 2019-09-05 [email protected] Roll src/third_party/dart 08132661a6..67bb2b7819 (4 commits) 2019-09-04 [email protected] Finish plumbing message responses on method channels (flutter/engine#11877) 2019-09-04 [email protected] Roll src/third_party/dart c3db2e3ee0..08132661a6 (2 commits) 2019-09-04 [email protected] [Windows] Address #36422 by adding a context for async resource uploading (flutter/engine#11828) 2019-09-04 [email protected] Roll src/third_party/skia e7366841663b..166751ef578a (7 commits) (flutter/engine#11881) If this roll has caused a breakage, revert this CL and stop the roller using the controls here: https://autoroll.skia.org/r/flutter-engine-flutter-autoroll Please CC [email protected] on the revert to ensure that a human is aware of the problem. To report a problem with the AutoRoller itself, please file a bug: https://bugs.chromium.org/p/skia/issues/entry?template=Autoroller+Bug Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+/master/autoroll/README.md

Incorporate code review feedback from #11441

a0797ce

shihaohong added the platform-android label Aug 31, 2019

shihaohong requested review from matthew-carroll and mklim August 31, 2019 05:53

googlebot added cla: yes labels Aug 31, 2019

Improve decodeExclusionRects naming and javadoc

d201d4e

shihaohong mentioned this pull request Aug 31, 2019

Android 10+ View.getSystemGestureExclusionRects #11451

Merged

Split test for two behaviors into two separate tests for setSystemGes…

3130248

…tureExclusionRects success case

matthew-carroll reviewed Sep 3, 2019

View reviewed changes

shihaohong added 3 commits September 3, 2019 17:01

Code review feedback: Improved variable naming and usage

c879cbb

Merge conflict resolution with getSystemGestureExclusionRects PR

f42d5fe

Re-include accidentally removed comments on test execution and verifi…

1f7bb41

…cation blocks

shihaohong requested a review from matthew-carroll September 4, 2019 22:27

matthew-carroll reviewed Sep 4, 2019

View reviewed changes

Remove variables from tests

482a899

shihaohong requested a review from matthew-carroll September 5, 2019 19:45

matthew-carroll approved these changes Sep 5, 2019

View reviewed changes

shihaohong merged commit 14e4187 into flutter:master Sep 5, 2019

shihaohong deleted the set-exclusion-rects-cleanup branch September 5, 2019 19:53

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

14e4187 Incorporate View.setSystemGestureExclusionRects code review f…

abe9752

…eedback from flutter#11441 (flutter/engine#11804)

engine-flutter-autoroll mentioned this pull request Sep 5, 2019

Roll engine cd920398e40e..edee1fea5850 (21 commits) flutter/flutter#39936

Merged

shihaohong mentioned this pull request Apr 9, 2020

Revert getSystemGestureExclusionRects and setSystemGestureExclusionRects #17613

Merged

Incorporate View.setSystemGestureExclusionRects code review feedback from #11441 #11804

Incorporate View.setSystemGestureExclusionRects code review feedback from #11441 #11804

Uh oh!

Conversation

shihaohong commented Aug 31, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

matthew-carroll left a comment

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!

matthew-carroll left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

shihaohong commented Aug 31, 2019 •

edited

Loading