Skip to content

Conversation

@hnljp
Copy link
Contributor

@hnljp hnljp commented Jul 12, 2018

support for custom tabs was added to stable Firefox in version 57.0

fixes: #368
Signed-off-by: Henning Nielsen Lund henning.n.lund@jp.dk

support for custom tabs was added to stable Firefox in version 57.0

fixes: openid#368
Signed-off-by: Henning Nielsen Lund <henning.n.lund@jp.dk>
@codecov-io
Copy link

codecov-io commented Jul 12, 2018

Codecov Report

Merging #369 into master will decrease coverage by 0.17%.
The diff coverage is 65%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master     #369      +/-   ##
============================================
- Coverage     82.93%   82.76%   -0.18%     
  Complexity      456      456              
============================================
  Files            41       41              
  Lines          2233     2251      +18     
  Branches        217      220       +3     
============================================
+ Hits           1852     1863      +11     
- Misses          298      302       +4     
- Partials         83       86       +3
Impacted Files Coverage Δ Complexity Δ
...a/net/openid/appauth/browser/BrowserBlacklist.java 100% <ø> (ø) 4 <0> (ø) ⬇️
...rary/java/net/openid/appauth/browser/Browsers.java 100% <100%> (ø) 0 <0> (ø) ⬇️
...penid/appauth/browser/VersionedBrowserMatcher.java 85.71% <100%> (+1.5%) 7 <0> (ø) ⬇️
...va/net/openid/appauth/browser/BrowserSelector.java 78.78% <53.33%> (-8.01%) 17 <0> (ø)

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 51c5b26...57d183a. Read the comment docs.

@hnljp hnljp mentioned this pull request Jul 13, 2018
hnljp added 3 commits July 16, 2018 00:19
a possible default browser of the user is respected.

fixes: openid#372
Signed-off-by: Henning Nielsen Lund <henning.n.lund@jp.dk>
The line needed to get reduced to no more than 100 characters.

Signed-off-by: Henning Nielsen Lund <henning.n.lund@jp.dk>
Corrected indentation accoring to travis-ci.

Signed-off-by: Henning Nielsen Lund <henning.n.lund@jp.dk>
@iainmcgin
Copy link
Member

If you can separate the commits so that the firefox custom tab definitions can be pulled in separately from the rest, I can merge that piece at least. The default browser behavior needs some more discussion and investigation.

@hnljp
Copy link
Contributor Author

hnljp commented Jul 16, 2018

The ordered list is always returning Chrome as the first entry, no matter what...
This is an example, of a phone with Android 8.0, where Firefox is set as the default browser: 07-14 22:15:15.714 12422-12909/? D/BrowserSelector: resolvedActivityList: [ResolveInfo{3be23c com.android.chrome/com.google.android.apps.chrome.Main m=0x108000}, ResolveInfo{ed5cbc5 org.mozilla.firefox/.App m=0x108000}]
I do not know what their criteria is for the order, but it is not returning the default browser first.
The same is also shown on Android 7.0.

@hnljp
Copy link
Contributor Author

hnljp commented Jul 16, 2018

it could be changed if the flag used by queryIntentActivities was changed.

@hnljp
Copy link
Contributor Author

hnljp commented Jul 18, 2018

I have tried to look more into it.
The problem is that the PackageManager.MATCH_ALL flag is making the result unsorted.
https://developer.android.com/reference/android/content/pm/PackageManager#MATCH_ALL
But the problem is that removing MATCH_ALL, will make queryIntentActivities only return the default browser on Android >= 7.0.

Looking at the code generating the list returned by queryIntentActivities:
https://android.googlesource.com/platform/frameworks/base.git/+/master/services/core/java/com/android/server/pm/PackageManagerService.java#7801
That is using the list generated as "an unsorted list":
https://android.googlesource.com/platform/frameworks/base.git/+/master/services/core/java/com/android/server/pm/PackageManagerService.java#7731
That is why there is a need to get the package name of the default browser and the list using MATCH_ALL is needed, in case the default browser is not compatible with Custom Tabs.

@iainmcgin
Copy link
Member

OK, thanks for investigating further. Could you still separate this change into two pull requests, one for adding the additional constants related to Firefox, and the other for the default browser selection?

@hnljp hnljp mentioned this pull request Jul 20, 2018
@hnljp
Copy link
Contributor Author

hnljp commented Jul 20, 2018

Have made a new pull request only including the Firefox support.
Better to separate the two issues, as stated by iainmcgin
#378

hnljp added a commit to hnljp/AppAuth-Android that referenced this pull request Jul 20, 2018
a possible default browser of the user is respected.

fixes: openid#372

split from openid#369

Signed-off-by: Henning Nielsen Lund <henning.n.lund@jp.dk>
@hnljp
Copy link
Contributor Author

hnljp commented Jul 20, 2018

Have created a new pull request regarding using the default browser, if it is compatible.
#379

@hnljp hnljp closed this Jul 20, 2018
hnljp added a commit to hnljp/AppAuth-Android that referenced this pull request Jul 27, 2018
a possible default browser of the user is respected.

fixes: openid#372

split from openid#369

Signed-off-by: Henning Nielsen Lund <henning.n.lund@jp.dk>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Support for custom tabs in Firefox

5 participants