Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Issue #2530: Add Autopush support components #2694

Merged
merged 7 commits into from May 29, 2019

Conversation

Projects
None yet
3 participants
@jonalmeida
Copy link
Member

commented Apr 9, 2019

Pull Request checklist

  • Quality: This PR builds and passes detekt/ktlint checks (A pre-push hook is recommended)
  • Tests: This PR includes thorough tests or an explanation of why it does not
  • Changelog: This PR includes a changelog entry or does not need one
  • Accessibility: The code in this PR follows accessibility best practices or does not include any user facing features

@jonalmeida jonalmeida requested a review from grigoryk Apr 9, 2019

@jonalmeida

This comment has been minimized.

Copy link
Member Author

commented Apr 10, 2019

Some comments from @grigoryk after an offline conversation:

  • Don't surface the errors for firebase. This isn't very useful for clients of the push feature to do anything.
  • Surface the subscription info to subscribers.
  • After some sleuthing in fennec, third party push subscribers only need the FCM registration token in order to do their thing.
    • Remove ThirdParty from PushType.
    • Surface a way for subscribers to read the registration token and listen to token updates.

For AbstractFirebasePushService:

  • Re-add forceRegistrationRenewal - this is for Feature Push finding out that the token is invalid on the device.
    • This is hard to do from our abstraction without finding a way to communicate to the service from the Feature bi-directionally.

@jonalmeida jonalmeida force-pushed the jonalmeida:feature-push branch from b38bf85 to bec9ef0 Apr 12, 2019

@codecov

This comment has been minimized.

Copy link

commented Apr 12, 2019

Codecov Report

❗️ No coverage uploaded for pull request base (master@87b3747). Click here to learn what that means.
The diff coverage is 88.88%.

Impacted file tree graph

@@            Coverage Diff            @@
##             master    #2694   +/-   ##
=========================================
  Coverage          ?   81.41%           
  Complexity        ?     3079           
=========================================
  Files             ?      398           
  Lines             ?    13546           
  Branches          ?     1923           
=========================================
  Hits              ?    11029           
  Misses            ?     1721           
  Partials          ?      796
Impacted Files Coverage Δ Complexity Δ
...a/mozilla/components/concept/push/PushProcessor.kt 100% <100%> (ø) 0 <0> (?)
...s/lib/push/firebase/AbstractFirebasePushService.kt 77.77% <77.77%> (ø) 4 <4> (?)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 87b3747...00a2e4d. Read the comment docs.

@jonalmeida jonalmeida force-pushed the jonalmeida:feature-push branch 5 times, most recently from c6bb03f to 70d7075 Apr 12, 2019

@jonalmeida jonalmeida force-pushed the jonalmeida:feature-push branch from 70d7075 to f5d319d Apr 29, 2019

@jonalmeida jonalmeida force-pushed the jonalmeida:feature-push branch 4 times, most recently from 2650023 to a4ed8e7 May 6, 2019

@jonalmeida jonalmeida changed the title New Component: Feature Push Issue #2530: Add Autopush support components May 11, 2019

@jonalmeida jonalmeida marked this pull request as ready for review May 11, 2019

@jonalmeida jonalmeida requested a review from mozilla-mobile/act as a code owner May 11, 2019

@grigoryk
Copy link
Contributor

left a comment

I think we can kill some complexity and abstraction layers here pretty easily. Specifically, the bus and the RustPushConnection class. It's fine if you'd want to do this in a follow-up, just make sure to file issues and outline the cleanup work in comments.

@jonalmeida jonalmeida force-pushed the jonalmeida:feature-push branch from a4ed8e7 to de79947 May 20, 2019

@jonalmeida jonalmeida requested a review from grigoryk May 20, 2019

@jonalmeida jonalmeida force-pushed the jonalmeida:feature-push branch from de79947 to 14fd18a May 20, 2019

@grigoryk
Copy link
Contributor

left a comment

You've certainly thought enough about this - let's land it, and iterate in the tree.

Great work!! 💯

@jonalmeida jonalmeida force-pushed the jonalmeida:feature-push branch 3 times, most recently from 00db80d to 3f93b4b May 29, 2019

@jonalmeida jonalmeida force-pushed the jonalmeida:feature-push branch from 3f93b4b to ce27321 May 29, 2019

jonalmeida added some commits May 29, 2019

@jonalmeida jonalmeida merged commit ca6dfbe into mozilla-mobile:master May 29, 2019

1 check passed

Taskcluster (pull_request) TaskGroup: success
Details

@jonalmeida jonalmeida deleted the jonalmeida:feature-push branch May 29, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.