Adding a FidListener that propagates fid changes to the clients. #2195

ankitaj224 · 2020-11-23T17:59:18Z

No description provided.

google-oss-bot · 2020-11-23T18:11:54Z

Coverage Report

Affected SDKs

firebase-installations

SDK overall coverage changed from 58.77% (4468726) to 60.38% (65007112) by +1.61%.

Filename Base (4468726) Head (65007112) Diff

AutoValue_InstallationResponse.java 42.86% 58.93% +16.07%

FirebaseInstallations.java 97.88% 98.04% +0.16%

Test Logs

Notes

HTML coverage reports can be produced locally with ./gradlew <product>:checkCoverage.
Report files are located at <product-build-dir>/reports/jacoco/.

Head commit (65007112) is created by Prow via merging commits: 4468726 0e45a10.

google-oss-bot · 2020-11-23T18:12:13Z

Binary Size Report

Affected SDKs

firebase-installations

Type Base (4468726) Head (65007112) Diff

aar 55.4 kB 56.5 kB +1.12 kB (+2.0%)

apk (aggressive) 79.4 kB 79.4 kB -4 B (-0.0%)

apk (release) 656 kB 657 kB +432 B (+0.1%)
firebase-installations-interop

Type Base (4468726) Head (65007112) Diff

aar 7.51 kB 8.04 kB +532 B (+7.1%)

apk (release) 607 kB 607 kB +176 B (+0.0%)

Test Logs

Notes

Head commit (65007112) is created by Prow via merging commits: 4468726 0e45a10.

...ations-interop/src/main/java/com/google/firebase/installations/FirebaseInstallationsApi.java

...tallations-interop/src/main/java/com/google/firebase/installations/internal/FidListener.java

...ase-installations/src/main/java/com/google/firebase/installations/FirebaseInstallations.java

...ations-interop/src/main/java/com/google/firebase/installations/FirebaseInstallationsApi.java

…o fisHook

firebase-installations-interop/firebase-installations-interop.gradle

...ions-interop/src/main/java/com/google/firebase/installations/internal/HandleFidListener.java

vkryachko · 2020-11-25T14:10:58Z

...ions-interop/src/main/java/com/google/firebase/installations/internal/HandleFidListener.java

+   *
+   * @hide
+   */
+  void unregister(FidListener fidListener);


The handle is supposed to represent a registration so it shouldn't take a listener as a parameter. For an example see

firebase-android-sdk/firebase-firestore/src/main/java/com/google/firebase/firestore/ListenerRegistration.java

Lines 17 to 25 in c7d3073

/** Represents a listener that can be removed by calling {@link #remove()}. */

public interface ListenerRegistration {

/**

* Removes the listener being tracked by this {@code ListenerRegistration}. After the initial

* call, subsequent calls have no effect.

*/

void remove();

}

PTAL. Thanks

...ase-installations/src/main/java/com/google/firebase/installations/FirebaseInstallations.java

vkryachko · 2020-11-25T14:15:32Z

...installations/src/test/java/com/google/firebase/installations/FirebaseInstallationsTest.java

+
+    FakeFidListener fidListener = new FakeFidListener();
+    // Register the FidListener
+    firebaseInstallations.registerFidListener(fidListener);


Please consider adding a test for removing a registration as well.

Updated this test to check the removing registration code as well. PTAL. Thanks

…o fisHook

google-oss-bot · 2020-11-30T18:05:09Z

The public api surface has changed for the subproject firebase-installations:
error: Added method com.google.firebase.installations.FirebaseInstallations.registerFidListener(com.google.firebase.installations.internal.FidListener) [AddedMethod]

Please update the api.txt files for the subprojects being affected by this change by running ./gradlew ${subproject}:generateApiTxtFile. Also perform a major/minor bump accordingly.

google-oss-bot · 2020-11-30T18:21:14Z

The public api surface has changed for the subproject firebase-installations:
error: Added method com.google.firebase.installations.FirebaseInstallations.registerFidListener(com.google.firebase.installations.internal.FidListener) [AddedMethod]

Please update the api.txt files for the subprojects being affected by this change by running ./gradlew ${subproject}:generateApiTxtFile. Also perform a major/minor bump accordingly.

vkryachko · 2020-11-30T19:57:27Z

CI failures look legit, see: https://gist.github.com/vkryachko/d1cd35daaf62a63ff7aff9a94e1a28e4 regarding GuardedBy

ankitaj224 · 2020-12-01T00:29:47Z

CI failures look legit, see: https://gist.github.com/vkryachko/d1cd35daaf62a63ff7aff9a94e1a28e4 regarding GuardedBy

Thanks for pointing this out, Vlad. Fixed it.

Just curious: how did you generate this?

vkryachko · 2020-12-01T16:51:34Z

@ankitaj224 No worries, I did not generate it, it was in the build log of the check-changed presubmit:
https://android-ci.firebaseopensource.com/view/gcs/android-ci/pr-logs/pull/firebase_firebase-android-sdk/2195/check-changed/1333493750147584001/

vkryachko · 2020-12-01T16:55:00Z

...ase-installations/src/main/java/com/google/firebase/installations/FirebaseInstallations.java

-          fidListeners.remove(fidListener);
+      public void unregister() {
+        synchronized (FirebaseInstallations.this) {
+          if (fidListeners.contains(listener)) {


nit: consider removing the contains check as remove handles it internally.

Thanks. Done

…o fisHook

google-oss-bot · 2020-12-01T22:47:03Z

@ankitaj224: The following test failed, say /retest to rerun them all:

Test name	Commit	Details	Rerun command
smoke-tests	`78d2468`	link	`/test smoke-tests`

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

Adding a FidListener that propagates fid changes to the clients.

ada672b

ankitaj224 requested review from vkryachko and andirayo November 23, 2020 17:59

googlebot added the cla: yes Override cla label Nov 23, 2020

google-oss-bot added the size/L label Nov 23, 2020

vkryachko reviewed Nov 23, 2020

View reviewed changes

ankitaj224 added 3 commits November 23, 2020 11:00

Merge branch 'master' of github.com:firebase/firebase-android-sdk int…

f7fb021

…o fisHook

Addressing vlad's comments.

6c5a1eb

Adding unregister method for listeners

28e81ab

ankitaj224 requested a review from vkryachko November 25, 2020 07:17

vkryachko reviewed Nov 25, 2020

View reviewed changes

ankitaj224 added 2 commits November 30, 2020 09:47

Merge branch 'master' of github.com:firebase/firebase-android-sdk int…

7a83a77

…o fisHook

Fixing the FidListenerHandle

43ed01b

Adding test to validate unregistering the listener

56c7b85

ankitaj224 requested a review from vkryachko November 30, 2020 18:15

Fixing api.txt

c276010

ankitaj224 added 2 commits November 30, 2020 15:10

Fixing gradle errors

16cc4d1

Minor fix

3d5b112

vkryachko approved these changes Dec 1, 2020

View reviewed changes

ankitaj224 added 2 commits December 1, 2020 14:13

Fixing minor nit

0e45a10

Merge branch 'master' of github.com:firebase/firebase-android-sdk int…

78d2468

…o fisHook

ankitaj224 merged commit ddccf8e into master Dec 1, 2020

firebase locked and limited conversation to collaborators Jan 1, 2021

kaibolay deleted the fisHook branch September 15, 2022 13:32

	/** Represents a listener that can be removed by calling {@link #remove()}. */
	public interface ListenerRegistration {

	/**
	* Removes the listener being tracked by this {@code ListenerRegistration}. After the initial
	* call, subsequent calls have no effect.
	*/
	void remove();
	}

Adding a FidListener that propagates fid changes to the clients. #2195

Adding a FidListener that propagates fid changes to the clients. #2195

Uh oh!

Conversation

ankitaj224 commented Nov 23, 2020

Uh oh!

google-oss-bot commented Nov 23, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Coverage Report

Affected SDKs

firebase-installations

Test Logs

Notes

Uh oh!

google-oss-bot commented Nov 23, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Binary Size Report

Affected SDKs

firebase-installations

firebase-installations-interop

Test Logs

Notes

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

vkryachko Nov 25, 2020

Choose a reason for hiding this comment

Uh oh!

ankitaj224 Nov 30, 2020

Choose a reason for hiding this comment

Uh oh!

Uh oh!

vkryachko Nov 25, 2020

Choose a reason for hiding this comment

Uh oh!

ankitaj224 Nov 30, 2020

Choose a reason for hiding this comment

Uh oh!

google-oss-bot commented Nov 30, 2020

Uh oh!

google-oss-bot commented Nov 30, 2020

Uh oh!

vkryachko commented Nov 30, 2020

Uh oh!

ankitaj224 commented Dec 1, 2020

Uh oh!

vkryachko commented Dec 1, 2020

Uh oh!

vkryachko Dec 1, 2020

Choose a reason for hiding this comment

Uh oh!

ankitaj224 Dec 1, 2020

Choose a reason for hiding this comment

Uh oh!

google-oss-bot commented Dec 1, 2020

Uh oh!

Uh oh!

google-oss-bot commented Nov 23, 2020 •

edited

Loading

`firebase-installations`

google-oss-bot commented Nov 23, 2020 •

edited

Loading

`firebase-installations`

`firebase-installations-interop`