1182328 - FF41: Only add CATEGORY_BROWSABLE on "intent" and "android-app"

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

As per the discussion in bug 1182140 comment 5 - 7.

This was originally caused by the revision of bug 1168998 in bug 1168662, should undo the hack fix in bug 1100100, and fix bug 1175532.

Comment 1

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

The better solution is to only add CATEGORY_BROWSABLE on links clicked from web-content, which will be a follow-up in bug 1182140.

Comment 2

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

As for custom scheme, I wonder if (scheme != null) will work...

Building the patch on top of bug 1173200

Comment 3

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

(In reply to Michael Comella (:mcomella) from comment #2)
> Building the patch on top of bug 1173200

Actually, bad idea - this needs uplift.

Comment 4

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

(In reply to Michael Comella (:mcomella) from comment #2)
> As for custom scheme, I wonder if (scheme != null) will work...

This won't work for "file" schemes - we added the hack in bug 1100100 for this.

Comment 5

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

(In reply to Michael Comella (:mcomella) from comment #4)
> This won't work for "file" schemes - we added the hack in bug 1100100 for
> this.

To be more explicit, we should add Browsable for "file" schemes coming from web content, but because both code paths go through here, we can't be sure which is being used and we can't add Browsable.

Comment 6

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Attachment: MozReview Request: Bug 1182328 - Use Intent.parseUri for intent:// & android-app://. r=margaret

Bug 1182328 - Only add Browsable to apps with schemes that are not file. r=margaret

Comment 7

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

I need to test this with some URI schemes to ensure it does not regress anything.

Adding Browsable to only "intent" and "android-app" schemes would prevent more regressions, but it wouldn't cover custom schemes which are essentially "intent" schemes you don't have to register.

As it stands, this patch is actually just a more generic fix for bug 1175532, but not the way out of this mess I was hoping for.

Margaret, let me know if you have an opinion here.

Comment 8

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

(In reply to Michael Comella (:mcomella) from comment #7)
> I need to test this with some URI schemes to ensure it does not regress
> anything.

tel, mailto, my custom scheme, and intent schemes seem to work.

Comment 9

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Because it's too complicated to add CATEGORY_BROWSABLE to custom uri schemes, we're just going to add it to intent:// and android-app://. We'll ideally use bug 1182140 to add the custom uri schemes in 42.

Comment 10

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Comment on attachment 8631910 [details]
MozReview Request: Bug 1182328 - Use Intent.parseUri for intent:// & android-app://. r=margaret

Bug 1182328 - Use Intent.parseUri for intent:// & android-app://. r=margaret

We now specify no flags to Intent.parseUri so it can accept and parse arbitrary
URIs.

Comment 11

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Attachment: MozReview Request: Bug 1182328 - Only add CATEGORY_BROWSABLE to intent & android-app uris. r=margaret

Bug 1182328 - Only add CATEGORY_BROWSABLE to intent & android-app uris. r=margaret

Comment 12

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Reminder to update the flags on bug 1182328 when this lands.

Comment 13

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Reminder to update the flags on bug 1176018 when this lands (soft dep added).

Comment 14

[:Margaret Leibovic]

Comment on attachment 8631910 [details]
MozReview Request: Bug 1182328 - Use Intent.parseUri for intent:// & android-app://. r=margaret

https://reviewboard.mozilla.org/r/12957/#review11763

Ship It!

Comment 15

[:Margaret Leibovic]

Comment on attachment 8632376 [details]
MozReview Request: Bug 1182328 - Only add CATEGORY_BROWSABLE to intent & android-app uris. r=margaret

https://reviewboard.mozilla.org/r/13055/#review11765

Ship It!

Comment 16

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

url:        https://hg.mozilla.org/integration/fx-team/rev/2e02ac7f87ab3ccd6f810589123e1ea5b0bffa0c
changeset:  2e02ac7f87ab3ccd6f810589123e1ea5b0bffa0c
user:       Michael Comella <[e-mail]>
date:       Thu Jul 09 17:28:37 2015 -0700
description:
Bug 1182328 - Only add Browsable to apps with schemes that are not file. r=margaret

Comment 17

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

url:        https://hg.mozilla.org/integration/fx-team/rev/4463255d9bf2eff71d1a828a006804fc13276b06
changeset:  4463255d9bf2eff71d1a828a006804fc13276b06
user:       Michael Comella <[e-mail]>
date:       Mon Jul 13 17:14:41 2015 -0700
description:
Bug 1182328 - Backed out changeset 2e02ac7f87ab. r=me

Did not mean to land this WIP patch.

Comment 18

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Ignore comment 16 & 17 – I landed the wrong patch.

Also, these patches should land in 41, not fx-team.

Comment 19

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Comment on attachment 8632376 [details]
MozReview Request: Bug 1182328 - Only add CATEGORY_BROWSABLE to intent & android-app uris. r=margaret

This request is for both changesets in this bug.

Approval Request Comment
[Feature/regressing bug #]: Broken w/ several changes around intent handling – see bug 1182695 for the full details.

[User impact if declined]: Users will have several regressions in opening external apps including bug 1175532 and bug 1176018.

[Describe test coverage new/current, TreeHerder]: Tested locally.
[Risks and why]: Low – we are reverting some breaking changes to our original intent handling code (e.g. adding CATEGORY_BROWSABLE to all outgoing intents) and making changes only for links of "intent://" and "android-app://" which open native android apps and we didn't support before 41. If this functionality is broken, we're about in the same boat as before but since we heavily rely on a built-in Android method (in addition to some externally tested modifications from bug 1168998), I'm confident it will work as expected.

[String/UUID change made/needed]: None

Comment 20

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8632376 [details]
MozReview Request: Bug 1182328 - Only add CATEGORY_BROWSABLE to intent & android-app uris. r=margaret

Approved.

Comment 21

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/1e469c088347
https://hg.mozilla.org/releases/mozilla-aurora/rev/6c05327d7d59