Skip to content
This repository was archived by the owner on Nov 1, 2022. It is now read-only.

Provide request interceptor to automatically navigate into PWAs #7367

Merged
merged 1 commit into from
Jul 8, 2020
Merged

Provide request interceptor to automatically navigate into PWAs #7367

merged 1 commit into from
Jul 8, 2020

Conversation

@TitanNano
Copy link
Contributor

@TitanNano TitanNano commented Jun 12, 2020

For #7366 and mozilla-mobile/fenix#5772

  • a WebAppInterceptor is needed to redirect to a separate WebApp
    activity
  • the manifest dao currently reports a web app as installed when it was
    last used before the deadline / expiration time. This is the oposite
    of what it's supposed to do.
  • The intent extension should also hold an override URL so a WebApp
    intent can be launched with a deeplink.
  • The WebAppIntentProcessor currently launches new sessions for each
    intent. This will break user expectations when they start to be
    switched to a already running WebApp activity but loose their session.

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

After merge

  • Milestone: Make sure issues closed by this pull request are added to the milestone of the version currently in development.
  • Breaking Changes: If this is a breaking change, please push a draft PR on Reference Browser to address the breaking issues.

@TitanNano TitanNano changed the title Automatically switch to PWA when web app is installed #7366 Automatically switch to PWA when web app is installed Jun 12, 2020
@TitanNano TitanNano mentioned this pull request Jun 12, 2020
Closed
4 tasks
@codecov
Copy link

codecov bot commented Jun 12, 2020 •
edited
Loading

Codecov Report

Merging #7367 into master will decrease coverage by 0.72%.
The diff coverage is 29.03%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master    #7367      +/-   ##
============================================
- Coverage     77.76%   77.03%   -0.73%     
+ Complexity     5045     4815     -230     
============================================
  Files           647      660      +13     
  Lines         24170    23683     -487     
  Branches       3538     3426     -112     
============================================
- Hits          18795    18244     -551     
- Misses         3903     4036     +133     
+ Partials       1472     1403      -69
Impacted Files Coverage Δ Complexity Δ
...omponents/feature/app/links/AppLinksInterceptor.kt 76.31% <ø> (ø) 16.00 <0.00> (ø)
...ozilla/components/feature/pwa/WebAppInterceptor.kt 0.00% <0.00%> (ø) 0.00 <0.00> (?)
...nents/concept/engine/request/RequestInterceptor.kt 78.94% <50.00%> (-3.41%) 0.00 <0.00> (ø)
.../java/mozilla/components/feature/pwa/ext/Intent.kt 60.00% <50.00%> (-6.67%) 0.00 <0.00> (ø)
...onents/feature/pwa/intent/WebAppIntentProcessor.kt 69.56% <53.84%> (-23.77%) 9.00 <2.00> (+2.00) ⬇️
...feature/customtabs/store/CustomTabsServiceStore.kt 0.00% <0.00%> (-100.00%) 0.00% <0.00%> (-2.00%)
...ts/feature/pwa/feature/WebAppHideToolbarFeature.kt 61.11% <0.00%> (-27.36%) 8.00% <0.00%> (-7.00%)
...onents/feature/customtabs/verify/OriginVerifier.kt 45.28% <0.00%> (-17.88%) 5.00% <0.00%> (ø%)
...omponents/browser/session/engine/EngineObserver.kt 71.57% <0.00%> (-11.03%) 52.00% <0.00%> (+2.00%) ⬇️
...ponents/service/fxa/sync/WorkManagerSyncManager.kt 0.00% <0.00%> (-10.29%) 0.00% <0.00%> (ø%)
... and 129 more

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 b458044...2f99e0a. Read the comment docs.

@TitanNano TitanNano changed the title Automatically switch to PWA when web app is installed Provide request interceptor to automatically navigate into PWAs Jun 12, 2020
@NotWoods NotWoods self-assigned this Jun 13, 2020
NotWoods
NotWoods suggested changes Jun 13, 2020
View reviewed changes
Copy link
Contributor

@NotWoods NotWoods left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks so much for working on another big PWA feature! This is also laying groundwork for other PWA & custom tabs improvements, which is great.

I like the overall direction but I left some comments about the interceptor and some general nits.

@TitanNano TitanNano force-pushed the issues/7366 branch 4 times, most recently from d7cfed7 to adb65db Compare June 18, 2020 22:53
@NotWoods
Copy link
Contributor

NotWoods commented Jun 23, 2020 •
edited
Loading

Because there's still some UI needed to make this a good user experience, here's what I propose moving forward:

Let's merge in this PR for Android Components, but close the Fenix work for now. We can have this working in sample browser (and maybe Reference Browser) to test with.

We'll need to try and figure out a good UI for this feature. One option is to make it a user toggle, another is to display a list of choices when the link is opened. We'll also need to make this work alongside app links. (ie, what happens if both "Open in app" and "Open in PWA" are turned on?)

Unfortunately my communication was poor in the original issue and we'll still need the UI. I think it would be jarring to always move the user to the PWA, especially since we can't accurately determine when they are removed.

@TitanNano
Copy link
Contributor Author

TitanNano commented Jun 23, 2020

(ie, what happens if both "Open in app" and "Open in PWA" are turned on?)

@NotWoods Right now app links take precedence over web apps.

@rocketsroger
Copy link
Contributor

rocketsroger commented Jun 25, 2020

@TitanNano, make sure tests are fixed since you changed the onLoadRequest interface. Also, please add tests for your component. Take a look at AppLinksInterceptorTest. Thanks,

@mcomella mcomella requested a review from MarcLeclair June 26, 2020 23:03
@mcomella
Copy link
Contributor

mcomella commented Jun 26, 2020 •
edited
Loading

@MarcLeclair Can you give this a look over from a perf perspective while I'm OOO? I don't think we need formal measurement but ensure nothing looks suspicious. See #7367 (comment) for a previous concern.

@MarcLeclair
Copy link
Contributor

MarcLeclair commented Jul 5, 2020

@mcomella (I'm late to answer, forgot to do so) but nothing jumps out of the ordinary in terms of performance. Unless we need those things right away to populate our UI on MAIN start up (or even VIEW), then it could delay but it doesn't seem like this is the case so I think we're good.

@TitanNano TitanNano force-pushed the issues/7366 branch 4 times, most recently from cf73c3d to a8c542b Compare July 6, 2020 23:48
@TitanNano
Provide request interceptor to automatically navigate into PWAs mozil…
2f99e0a 
…la-mobile#7366

- a WebAppInterceptor is needed to redirect to a separate WebApp
  activity
- the manifest dao currently reports a web app as installed when it was
  last used before the deadline / expiration time. This is the oposite
  of what it's supposed to do.
- The intent extension should also hold an override URL so a WebApp
  intent can be launched with a deeplink.
- The WebAppIntentProcessor currently launches new sessions for each
  intent. This will break user expectations when they start to be
  switched to a already running WebApp activity but loose their session.
- we need a cache for currently installed web app scopes, so we don't
  have to do blocking IO during page load.
@TitanNano
Copy link
Contributor Author

TitanNano commented Jul 7, 2020

@rocketsroger I added new tests and everything else also passes now.

@rocketsroger
Copy link
Contributor

rocketsroger commented Jul 7, 2020

@rocketsroger I added new tests and everything else also passes now.

@TitanNano I have couple of concerns:

  1. This should be disabled until UX is ready.
  2. Have you tested with Fenix? Currently Fenix does not intercept any app initiated request.

    android-components/components/concept/engine/src/main/java/mozilla/components/concept/engine/request/RequestInterceptor.kt

    Line 96 in f70216b

    fun interceptsAppInitiatedRequests() = false
    .

@TitanNano
Copy link
Contributor Author

TitanNano commented Jul 7, 2020

@rocketsroger I added new tests and everything else also passes now.

@TitanNano I have couple of concerns:

1. This should be disabled until UX is ready.

2. Have you tested with Fenix? Currently Fenix does not intercept any app initiated request.  https://github.com/mozilla-mobile/android-components/blob/f70216b224852172f9f781b905f94f8e2c0b4d9f/components/concept/engine/src/main/java/mozilla/components/concept/engine/request/RequestInterceptor.kt#L96
   .

@rocketsroger

  1. Where would you like to disable this? If I understood @NotWoods correctly Provide request interceptor to automatically navigate into PWAs #7367 (comment), he'd like to have it in sample browser. For Fenix this has to be integrated first before it will do anything.
  2. a. There is a patch for Fenix Automatically switch to PWA as long as web app is installed fenix#5772 which worked with a previous iteration of this patch, but I will have to make some slight adjustments to it, to account for the most recent modifications. @NotWoods wanted to put the Fenix patch on hold until UX is ready though, so I haven't put more effort into it for now.
    b. Regarding the app initiated requests: The WebAppInterceptor doesn't want to intercept app initiated requests, so it's perfectly fine that Fenix doesn't intercept them. That's also the reason I filtered them out in the SampleRequestInterceptor https://github.com/mozilla-mobile/android-components/pull/7367/files#diff-511c28810156a10021024868446d5f52R34.

@mcomella mcomella removed their request for review July 7, 2020 22:09
@mcomella
Copy link
Contributor

mcomella commented Jul 7, 2020

Removing self as reviewer as I defer to MarcLeclair from #7367 (comment)

@rocketsroger
Copy link
Contributor

rocketsroger commented Jul 7, 2020 •
edited
Loading

@rocketsroger

1. Where would you like to disable this? If I understood @NotWoods correctly [#7367 (comment)](https://github.com/mozilla-mobile/android-components/pull/7367#issuecomment-648267039), he'd like to have it in sample browser. For Fenix this has to be integrated first before it will do anything.

2. a. There is a patch for Fenix [mozilla-mobile/fenix#5772](https://github.com/mozilla-mobile/fenix/issues/5772) which worked with a previous iteration of this patch, but I will have to make some slight adjustments to it, to account for the most recent modifications. @NotWoods wanted to put the Fenix patch on hold until UX is ready though, so I haven't put more effort into it for now.
   b. Regarding the app initiated requests: The `WebAppInterceptor` doesn't want to intercept app initiated requests, so it's perfectly fine that Fenix doesn't intercept them. That's also the reason I filtered them out in the `SampleRequestInterceptor` https://github.com/mozilla-mobile/android-components/pull/7367/files#diff-511c28810156a10021024868446d5f52R34.

Ah I see. I'm good with the change then. Since this is only available in the sample browser.

@TitanNano
Copy link
Contributor Author

TitanNano commented Jul 8, 2020

@rocketsroger @NotWoods who will give the final approval? 😄

@rocketsroger
Copy link
Contributor

rocketsroger commented Jul 8, 2020

@NotWoods I'm good with the change from my end. But I'll defer to your approval since you have a outstanding change request on the review. ✅

NotWoods
NotWoods approved these changes Jul 8, 2020
View reviewed changes
Copy link
Contributor

@NotWoods NotWoods left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good! Let's start testing with Sample Browser & Reference Browser

@NotWoods
Copy link
Contributor

NotWoods commented Jul 8, 2020

bors r+

@bors
Copy link

bors bot commented Jul 8, 2020

Build succeeded:

@bors bors bot merged commit 1df4738 into mozilla-mobile:master Jul 8, 2020
@TitanNano TitanNano deleted the issues/7366 branch July 9, 2020 07:18
@csadilek csadilek mentioned this pull request Oct 13, 2022
Closed
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@rocketsroger rocketsroger Awaiting requested review from rocketsroger

@MarcLeclair MarcLeclair Awaiting requested review from MarcLeclair

3 more reviewers

@mcomella mcomella mcomella left review comments

@acreskeyMoz acreskeyMoz acreskeyMoz left review comments

@NotWoods NotWoods NotWoods approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@NotWoods NotWoods

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@TitanNano @NotWoods @rocketsroger @mcomella @MarcLeclair @acreskeyMoz