[Fireperf][AASA] `FirstDrawDoneListener` #4041

leotianlizhan · 2022-08-29T23:10:07Z

b/243538554

github-actions · 2022-08-29T23:18:26Z

Unit Test Results

973 tests +4 973 ✔️ +4 1m 42s ⏱️ +30s
  51 suites +1     0 💤 ±0
  51 files +1     0 ❌ ±0

Results for commit 95990a6. ± Comparison against base commit 3f13291.

♻️ This comment has been updated with latest results.

google-oss-bot · 2022-08-29T23:19:48Z

Coverage Report ¹

Affected Products

firebase-perf
Overall coverage changed from 71.37% (53c0e1d) to 71.43% (d158f9f) by +0.06%.
Filename Base (53c0e1d) Merge (d158f9f) Diff
FirstDrawDoneListener.java ? 85.19% ?

Test Logs

Notes

Commit (d158f9f) is created by Prow via merging PR base commit (53c0e1d) and head commit (95990a6).
Run gradle <product>:checkCoverage to produce HTML coverage reports locally. After gradle commands finished, report files can be found under <product-build-dir>/reports/jacoco/.

https://storage.googleapis.com/firebase-sdk-metric-reports/kqod0FVw1S.html

google-oss-bot · 2022-08-29T23:25:42Z

Size Report ¹

Affected Products

firebase-perf
Type Base (53c0e1d) Merge (d158f9f) Diff
aar 311 kB 313 kB +2.42 kB (+0.8%)
apk (release) 2.48 MB 2.48 MB +428 B (+0.0%)

Test Logs

Notes

Commit (d158f9f) is created by Prow via merging PR base commit (53c0e1d) and head commit (95990a6).

https://storage.googleapis.com/firebase-sdk-metric-reports/Izw8s6vrk9.html

raymondlam · 2022-08-30T18:40:43Z

firebase-perf/src/main/java/com/google/firebase/perf/util/FirstDrawDoneListener.java

+    // Handle bug prior to API 26 where OnDrawListener from the floating ViewTreeObserver is not
+    // merged into the real ViewTreeObserver.
+    // https://android.googlesource.com/platform/frameworks/base/+/9f8ec54244a5e0343b9748db3329733f259604f3
+    if (Build.VERSION.SDK_INT >= 26


nit: I recommend flipping the cases around so we are checking for special cases first then falling through to the default. This also moves the comment to be close to the specific case it's referring to.

if (Build.VERSION.SDK_INT < 26 && (!view.getViewTreeObserver().isAlive() || view.getWindowToken() == null) { view.addOnAttachStateChangeListener(...) return; } view.getViewTreeObserver()....

raymondlam · 2022-08-30T18:59:16Z

firebase-perf/src/test/java/com/google/firebase/perf/util/FirstDrawDoneListenerTest.java

+    assertThat(listOfListeners).isNotNull();
+    assertThat(listOfListeners.size()).isEqualTo(1);
+
+    // OnDrawListener is not registered, it is delayed for later


Can we test that it is registered later on a state change?

I don't see a way to do it because View class doesn't have public View.dispatchAttachView() unlike the other listeners we have for ViewTreeObserver (ViewTreeObserver.dispatchOnDraw() and ViewTreeObserver.dispatchOnGlobalLayout()), and although there's a private View.dispatchAttachView() it uses parameter classes that are also not visible to us.

+1 to Raymond's ask. Currently, the test does not do much validate the listener registration for API < 26.

Figured out a way to do it through reflection, PTAL @visumickey @raymondlam

raymondlam · 2022-08-30T19:01:46Z

firebase-perf/src/test/java/com/google/firebase/perf/util/FirstDrawDoneListenerTest.java

+  }
+
+  @Test
+  @Config(sdk = 26)


Can we have a test for un-registering itself in Config(sdk=25) for both OnAttachStateChangeListener and OnDrawListener?

I only added @Config(sdk = 26) for this test because it uses reflection, so I don't know if in the future Android will change field names or refactor ViewTreeObserver.

The < API 26 bug is that adding OnDrawListeners to ViewTreeObserver doesn't work, it has nothing to do with removing or the listener itself, so I don't think we need to test unregistering for 2 different versions.

For the < API 26 bug, we should be making sure that we have a test to make sure that if we do register a View.OnAttachStateChangeListener, that it is unregistered once onViewAttachedToWindow is called. Otherwise, we could change the code to leave it registered and the tests would still pass.

Ah, I see what you mean. That is difficult for the same reason as the other comment: View class doesn't have public, easy to use dispatchXYZ() methods like ViewTreeObserver does.

Could you use a FakeView or mock the View object? This way you have access to the listeners and can trigger the interfaces directly.

Added a check at the last line of registerForNextDraw_delaysAddingListenerForAPIsBelow26().

leotianlizhan · 2022-08-30T22:40:44Z

/test smoke-tests

visumickey · 2022-08-31T12:19:17Z

firebase-perf/src/main/java/com/google/firebase/perf/util/FirstDrawDoneListener.java

+
+  @Override
+  public void onDraw() {
+    // Prevent 2nd draw and on from triggering onDraw


nit: Fix comment. The comment is a bit hard to parse.

reworded, PTAL @visumickey

visumickey · 2022-08-31T12:38:25Z

firebase-perf/src/main/java/com/google/firebase/perf/util/FirstDrawDoneListener.java

+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+package com.google.firebase.perf.util;


nit: Is there a better package where we drop this in? Util is too open and overtime this has becomes a drop off of different kind of classes. Since this is in the context of app start measurements, can we find a better place for this class? If it is hard to find, let catch up offline on the package structure.

I don't know where else to drop this in. I don't want to drop it in /metrics which is where AppStartTrace and other Trace metrics live in, because that would allow AppStartTrace access to some package-private methods that should not be allowed to use apart from testing. I only want AppStartTrace (the consumer of FirstDrawDoneListener) to use public static factory to register and create FirstDrawDoneListener.

firebase-perf/src/test/java/com/google/firebase/perf/util/FirstDrawDoneListenerTest.java

visumickey · 2022-08-31T12:57:06Z

firebase-perf/src/test/java/com/google/firebase/perf/util/FirstDrawDoneListenerTest.java

+    assertThat(listOfListeners).isNotNull();
+    assertThat(listOfListeners.size()).isEqualTo(1);
+
+    // OnDrawListener is not registered, it is delayed for later


+1 to Raymond's ask. Currently, the test does not do much validate the listener registration for API < 26.

leotianlizhan · 2022-09-02T17:34:07Z

/test check-coverage-changed

* WIP * handler * handle bug prior to api 26 * tests * copyright * improve tests * comment fix * improve tests * comment * backport View.isAttachedToWindow() * improved registerForNextDraw_delaysAddingListenerForAPIsBelow26 * remove LayoutChangeListener * add check to mmake sure OnAttachStateChangeListener removes itself * mcreate helper isAliveAndAttached * improve test with Robolectric ShadowLooper

leotianlizhan added 3 commits August 25, 2022 14:56

WIP

9a4596b

handler

9f2e9e5

handle bug prior to api 26

dd046aa

google-oss-bot added the size/M label Aug 29, 2022

tests

facf87a

google-oss-bot added size/L and removed size/M labels Aug 30, 2022

leotianlizhan changed the title ~~[Fireperf][AASA] FirstDrawDownListener~~ [Fireperf][AASA] FirstDrawDoneListener Aug 30, 2022

leotianlizhan added 3 commits August 30, 2022 11:03

copyright

449f611

improve tests

5025305

comment fix

a0e8b48

leotianlizhan requested review from raymondlam and visumickey August 30, 2022 18:15

raymondlam approved these changes Aug 30, 2022

View reviewed changes

improve tests

9c30ce7

visumickey requested a review from jeremyjiang-dev August 30, 2022 21:37

leotianlizhan added 2 commits August 30, 2022 14:47

comment

4e7792b

backport View.isAttachedToWindow()

40cfc29

visumickey approved these changes Aug 31, 2022

View reviewed changes

leotianlizhan added 4 commits August 31, 2022 18:26

improved registerForNextDraw_delaysAddingListenerForAPIsBelow26

59a55dd

remove LayoutChangeListener

4ca7a91

add check to mmake sure OnAttachStateChangeListener removes itself

c5772b1

mcreate helper isAliveAndAttached

3098287

jeremyjiang-dev approved these changes Sep 1, 2022

View reviewed changes

improve test with Robolectric ShadowLooper

95990a6

leotianlizhan merged commit ffd44ee into master Sep 2, 2022

leotianlizhan deleted the ondrawlistener branch September 2, 2022 18:58

firebase locked and limited conversation to collaborators Oct 3, 2022

[Fireperf][AASA] FirstDrawDoneListener #4041

[Fireperf][AASA] FirstDrawDoneListener #4041

Uh oh!

Conversation

leotianlizhan commented Aug 29, 2022

Uh oh!

github-actions bot commented Aug 29, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Unit Test Results

Uh oh!

google-oss-bot commented Aug 29, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Coverage Report 1

Affected Products

firebase-perf

Test Logs

Notes

Uh oh!

google-oss-bot commented Aug 29, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Size Report 1

Affected Products

firebase-perf

Test Logs

Notes

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!

leotianlizhan Sep 1, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

leotianlizhan commented Aug 30, 2022

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!

Choose a reason for hiding this comment

Uh oh!

leotianlizhan commented Sep 2, 2022

Uh oh!

Uh oh!

[Fireperf][AASA] `FirstDrawDoneListener` #4041

[Fireperf][AASA] `FirstDrawDoneListener` #4041

github-actions bot commented Aug 29, 2022 •

edited

Loading

google-oss-bot commented Aug 29, 2022 •

edited

Loading

Coverage Report ¹

`firebase-perf`

google-oss-bot commented Aug 29, 2022 •

edited

Loading

Size Report ¹

`firebase-perf`

leotianlizhan Sep 1, 2022 •

edited

Loading