-
Notifications
You must be signed in to change notification settings - Fork 1.3k
FNX-14508 ⁃ For #11903: Fix private custom tabs #13327
FNX-14508 ⁃ For #11903: Fix private custom tabs #13327
Conversation
Codecov Report
@@ Coverage Diff @@
## master #13327 +/- ##
============================================
- Coverage 28.48% 28.48% -0.01%
- Complexity 1046 1047 +1
============================================
Files 420 420
Lines 16968 16970 +2
Branches 2200 2202 +2
============================================
Hits 4834 4834
- Misses 11786 11788 +2
Partials 348 348
Continue to review full report at Codecov.
|
components.analytics.metrics.track(Event.OpenedLink(Event.OpenedLink.Mode.NORMAL)) | ||
} | ||
|
||
val processor = getIntentProcessors(private).firstOrNull { it.process(intent) } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So AFAICT this getIntentProcessors
will also process externalAppIntentProcessors
and others. Before even though it wasn't working (lol) it was attempting to just add this extra on the custom tab and intent processors. Will there be any bad side effects from adding this extra to other intent entry points?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There shouldn't be side effects when we add extra intents. If we want to be super safe we can set the boolean on both this.intent
(before) and the home activity intent (now).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry that was a confusing comment. I meant as an example - now the TWA/PWA externalAppIntentProcessors will also be attaching PRIVATE_BROWSING_MODE
, right? Does that mean PWAs will also be attempting to present / be in PBM?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Honestly we wanted it to work that way in the first place, so if they do present in PBM that fixes another bug :)
We can do another pass in the future to re-evaulate what data is getting passed in intents like this.
Pull Request checklist
After merge
To download an APK when reviewing a PR: