1347583 - Allow third-party apps to open a tab in private browsing mode

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, enhancement)

Description

[Sebastian Kaspari (:sebastian; :pocmo)]

We were considering opening a private tab from Firefox Focus when switching to Fennec. However that's not possible currently.

Focus issue:
https://github.com/mozilla-mobile/focus-android/issues/195

Comment 1

[Sebastian Kaspari (:sebastian; :pocmo)]

Gautam: This would be a great bug for you to pick up. Follow the incoming intent from BrowserApp/GeckoApp to Tabs.java - we already have an internal flag for creating a private tab - if we additional look at a flag inside the Intent then we could open private tabs from Intents.

I can write a bit more about this - just let me know.

Comment 2

[Gautam Prajapati]

Sebastian, that sounds like an interesting one. Thanks for flagging me on this. 

Currently, I'm working on Bug 1337078, I think I'd finish that in a day or two, then I'll look into this.
Is that fine?

Comment 3

[Gautam Prajapati]

(In reply to Sebastian Kaspari (:sebastian) from comment #1)
> Follow the incoming intent from BrowserApp/GeckoApp to Tabs.java - we already have an internal
> flag for creating a private tab - if we additional look at a flag inside the
> Intent then we could open private tabs from Intents.
> 
> I can write a bit more about this - just let me know.

Hi Sebastian!

I think it would be helpful if you elaborate a bit more on this.
I have looked through the intent handling in BrowserApp/GeckoApp. 

Will I need to change something in focus-android as well or we will handle this on fennec side only? 

I'm not a user of focus right now, How exactly will the intent be launched from there?

I went through the discussion on Github issue thread. 
I have the same doubt, why do we want to copy an URL from focus and load it in fennec's private browsing?
Why would we need an intent if we're copying pasting URL from one browser to another? 

I may have missed something so I'd like to know more on this :)

Comment 4

[Sebastian Kaspari (:sebastian; :pocmo)]

One of the use cases is that we want to open a private tab from Focus. But this should be a generic way of opening private tabs for third-party apps.

(In reply to Gautam Prajapati from comment #3)
> Will I need to change something in focus-android as well or we will handle
> this on fennec side only? 

This bug is only about the changes in Fennec.

> I'm not a user of focus right now, How exactly will the intent be launched
> from there?

We would need to introduce an Intent extra that allows an app to say "please open this in private browsing".

An example of such an extra is EXTRA_APPLICATION_ID which allows a third-party app to open URLs always in the same tab:
https://developer.android.com/reference/android/provider/Browser.html#EXTRA_APPLICATION_ID

Or EXTRA_CREATE_NEW_TAB which will always open a new tab:
https://developer.android.com/reference/android/provider/Browser.html#EXTRA_CREATE_NEW_TAB

In our case we'd introduce a new extra (maybe "private_browsing") that can be true or false and will control whether we create a private tab.


An URL that is opened via an ACTION_VIEW intent is passed from GeckoApp to Tabs.loadUrlWithIntentExtras():
https://dxr.mozilla.org/mozilla-central/source/mobile/android/base/java/org/mozilla/gecko/GeckoApp.java#2194

Following the code from loadUrlWithIntentExtras() you end up in loadUrl():
https://dxr.mozilla.org/mozilla-central/source/mobile/android/base/java/org/mozilla/gecko/Tabs.java#966

In loadUrl() we already open a private tab if an internal flag is set. Additionally we would need to look into the intent to see if the extra is set:
https://dxr.mozilla.org/mozilla-central/source/mobile/android/base/java/org/mozilla/gecko/Tabs.java#975

After that we should be able to open private tabs from outside the browser. We might need to check if the browser switches to the private tab automatically (even if you are looking at non-private tabs) or if this is something we need to add too.

> Why would we need an intent if we're copying pasting URL from one browser to
> another? 

In this case we are not copying an URL. Instead Focus is launching an intent to open a (private) tab in Firefox.

Comment 5

[Gautam Prajapati]

Attachment: Bug 1347583 - Allow third-party apps to open a tab in private browsing mode;

Review commit: https://reviewboard.mozilla.org/r/132428/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/132428/

Comment 6

[Gautam Prajapati]

(In reply to Gautam Prajapati from comment #5)
> Created attachment 8860418 [details]
> Bug 1347583 - Allow third-party apps to open a tab in private browsing mode;
> 
> Review commit: https://reviewboard.mozilla.org/r/132428/diff/#index_header
> See other reviews: https://reviewboard.mozilla.org/r/132428/

So I added an intent extra 'is_private_tab' which adds the private tab flag if it's true. This should allow every third party app to open a private tab. 

Please review, let me know if changes are required. I tested by opening an ACTION_VIEW type intent using focus-android, it works fine. :)

Comment 7

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8860418 [details]
Bug 1347583 - Allow third-party apps to open a tab in private browsing mode;

https://reviewboard.mozilla.org/r/132428/#review135314

::: mobile/android/base/java/org/mozilla/gecko/GeckoApp.java:161
(Diff revision 1)
>  
>      public static final String INTENT_REGISTER_STUMBLER_LISTENER = "org.mozilla.gecko.STUMBLER_REGISTER_LOCAL_LISTENER";
>  
>      public static final String EXTRA_STATE_BUNDLE          = "stateBundle";
>  
> +    public static final String PRIVATE_TAB_INTENT_EXTRA    = "is_private_tab";

I'd remove "is_" here. At the time the intent is created it is not a private tab yet.

::: mobile/android/base/java/org/mozilla/gecko/GeckoApp.java:2213
(Diff revision 1)
> +                    final boolean isTabPrivate = intent.getBooleanExtra(PRIVATE_TAB_INTENT_EXTRA, false);
>                      int flags = Tabs.LOADURL_NEW_TAB | Tabs.LOADURL_USER_ENTERED | Tabs.LOADURL_EXTERNAL;
>                      if (isFirstTab) {
>                          flags |= Tabs.LOADURL_FIRST_AFTER_ACTIVITY_UNHIDDEN;
>                      }
> +                    if (isTabPrivate) {
> +                        flags |= Tabs.LOADURL_PRIVATE;
> +                    }

Let's move this into loadUrl():
https://dxr.mozilla.org/mozilla-central/source/mobile/android/base/java/org/mozilla/gecko/Tabs.java#975

The intent is passed all the way to loadUrl() and that's where we read other extras too.

Comment 8

[Gautam Prajapati]

Comment on attachment 8860418 [details]
Bug 1347583 - Allow third-party apps to open a tab in private browsing mode;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/132428/diff/1-2/

Comment 9

[Gautam Prajapati]

(In reply to Gautam Prajapati from comment #8)
> Comment on attachment 8860418 [details]
> Bug 1347583 - Allow third-party apps to open a tab in private browsing mode;
> 
> Review request updated; see interdiff:
> https://reviewboard.mozilla.org/r/132428/diff/1-2/

Sebastian, drawing your attention to this one as well. I updated the patch, review whenever you have time! :)

Comment 10

[Sebastian Kaspari (:sebastian; :pocmo)]

The review flag is sufficient. I'll have a look! :)

Comment 11

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8860418 [details]
Bug 1347583 - Allow third-party apps to open a tab in private browsing mode;

https://reviewboard.mozilla.org/r/132428/#review135680

::: mobile/android/base/java/org/mozilla/gecko/Tabs.java:976
(Diff revision 2)
> +        if (intent.getBooleanExtra(PRIVATE_TAB_INTENT_EXTRA, false) != null) {
> +            isPrivate = intent.getBooleanExtra(PRIVATE_TAB_INTENT_EXTRA, false);
> +        }

The null check is not needed. getBooleanExtra() will either return the value from the intent or the default you specified. But never null.

In this case you can just do:

> boolean isPrivate = (flags & LOADURL_PRIVATE) != 0
>        || intent.getBooleanExtra(PRIVATE_TAB_INTENT_EXTRA, false);

However there's the possibility that intent is null. So let's guard against that:

> boolean isPrivate = (flags & LOADURL_PRIVATE) != 0
>        || (intent != null &&  intent.getBooleanExtra(PRIVATE_TAB_INTENT_EXTRA, false));

Comment 12

[Gautam Prajapati]

Comment on attachment 8860418 [details]
Bug 1347583 - Allow third-party apps to open a tab in private browsing mode;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/132428/diff/2-3/

Comment 13

[Gautam Prajapati]

(In reply to Sebastian Kaspari (:sebastian) from comment #10)
> The review flag is sufficient. I'll have a look! :)

Will keep that in mind :)

Thanks for reviewing, I have updated the patch. Let me know if some more changes are required.

One more thing, how will a third party app know that fennec provides such a flag? 
I mean we know because we wrote the code, but for a normal third party, 
is there any public documentation keeping a track of such flags?

Comment 14

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8860418 [details]
Bug 1347583 - Allow third-party apps to open a tab in private browsing mode;

https://reviewboard.mozilla.org/r/132428/#review135792

Looks good! Thank you! :)

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/4b90d988f654
Allow third-party apps to open a tab in private browsing mode; r=sebastian

Comment 16

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/4b90d988f654