Skip to content
This repository has been archived by the owner on Feb 20, 2023. It is now read-only.

For #514 & #5742: Updates "launch links in private tab" functionality #5721

Merged
merged 2 commits into from
Oct 3, 2019

Conversation

sblatz
Copy link
Contributor

@sblatz sblatz commented Oct 1, 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
  • Screenshots: This PR includes screenshots or GIFs of the changes made or an explanation of why it does not
  • Accessibility: The code in this PR follows accessibility best practices or does not include any user facing features

After merge

  • Milestone: Make sure issues finished by this pull request are added to the milestone of the version currently in development.

To download an APK when reviewing a PR:

  1. click on Show All Checks,
  2. click Details next to "Taskcluster (pull_request)" after it appears and then finishes with a green checkmark,
  3. click on the "Fenix - assemble" task, then click "Run Artifacts".
  4. the APK links should be on the left side of the screen, named for each CPU architecture

@codecov-io
Copy link

codecov-io commented Oct 1, 2019

Codecov Report

❗ No coverage uploaded for pull request base (master@3be0695). Click here to learn what that means.
The diff coverage is 4.76%.

Impacted file tree graph

@@            Coverage Diff            @@
##             master    #5721   +/-   ##
=========================================
  Coverage          ?   14.43%           
  Complexity        ?      322           
=========================================
  Files             ?      256           
  Lines             ?    10500           
  Branches          ?     1530           
=========================================
  Hits              ?     1516           
  Misses            ?     8858           
  Partials          ?      126
Impacted Files Coverage Δ Complexity Δ
...ava/org/mozilla/fenix/settings/SettingsFragment.kt 0% <0%> (ø) 0 <0> (?)
...a/fenix/settings/DefaultBrowserSettingsFragment.kt 0% <0%> (ø) 0 <0> (?)
...pp/src/main/java/org/mozilla/fenix/HomeActivity.kt 8% <0%> (ø) 7 <0> (?)
.../src/main/java/org/mozilla/fenix/utils/Settings.kt 72.78% <100%> (ø) 15 <0> (?)
...n/java/org/mozilla/fenix/IntentReceiverActivity.kt 29.16% <12.5%> (ø) 4 <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 3be0695...3a73344. Read the comment docs.

@sblatz sblatz added pr:work-in-progress PRs that are not ready to be reviewed yet and are actively being worked on pr:do-not-land labels Oct 1, 2019
@sblatz sblatz force-pushed the mess-with-private-mode-intents branch 7 times, most recently from f1b6c16 to ec84796 Compare October 3, 2019 15:49
@sblatz sblatz removed pr:do-not-land pr:work-in-progress PRs that are not ready to be reviewed yet and are actively being worked on labels Oct 3, 2019
@sblatz sblatz force-pushed the mess-with-private-mode-intents branch from ec84796 to 2795a32 Compare October 3, 2019 15:59
@sblatz sblatz requested a review from a team October 3, 2019 16:00
@sblatz sblatz force-pushed the mess-with-private-mode-intents branch from 2795a32 to b1d65f3 Compare October 3, 2019 16:01
@sblatz sblatz changed the title For #514: Adds back actvitiy alias for launch links in private tab For #514 & #5742: Updates "launch links in private tab" functionality Oct 3, 2019
@sblatz sblatz force-pushed the mess-with-private-mode-intents branch from b1d65f3 to ae9e2ec Compare October 3, 2019 16:55
Copy link
Contributor

@severinrudie severinrudie left a comment

Choose a reason for hiding this comment

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

Heading to a meeting, I'll review more later today.

@@ -123,6 +123,54 @@
android:resource="@mipmap/ic_launcher" />
</activity>

<activity-alias
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: this would be a good place for a small comment explaining why we need this alias.

app/src/main/java/org/mozilla/fenix/HomeActivity.kt Outdated Show resolved Hide resolved
Comment on lines +50 to +55
if (didLaunchPrivateLink && Browsers.all(this).isDefaultBrowser) {
this.settings().openLinksInAPrivateTab = true
} else if (!Browsers.all(this).isDefaultBrowser) {
/* If the user has unset us as the default browser, unset alwaysOpenInPrivateMode */
this.settings().openLinksInAPrivateTab = false
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Double checking because I'm not sure: would this properly handle the following case?

  • User launches app in normal mode
  • App is default browser
  • User toggles openLinksInAPrivateTab
  • App is backgrounded (but not destroyed)
  • A third party link is opened

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I just tested and it opens the link in private mode as expected :)

Copy link
Contributor

@severinrudie severinrudie left a comment

Choose a reason for hiding this comment

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

I'm not particularly familiar with the Preference API, but this seems good to me.

android:paddingEnd="?android:attr/scrollbarSize">

<LinearLayout
android:id="@android:id/widget_frame"
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not clear on exactly how this works, would it still function as a FrameLayout? They're somewhat less expensive to inflate.

@sblatz sblatz force-pushed the mess-with-private-mode-intents branch from 0a74773 to 3a73344 Compare October 3, 2019 18:03
@sblatz sblatz merged commit 371e2ac into mozilla-mobile:master Oct 3, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants