Remove code that implies external apps can open incognito tabs.

Remove code that implies external apps can open incognito tabs.

On Android, we restrict who can open incognito tabs to only
Chrome.  The logic to enforce this was spread across
IntentHandler#shouldIgnoreIntent and
ChromeTabbedActivity#processUrlViewIntent.  It was unclear and
confusing to read, so this removes the invalid paths and makes
it more explicit about what is allowed.

BUG=

Committed: https://crrev.com/1b2247e8e0a7277aa58ce4fcb242c7be2dafe564
Cr-Commit-Position: refs/heads/master@{#440918}

[Ted C, 2016-12-28 23:09:43]

tedchoc@chromium.org changed reviewers:
+ mariakhomenko@chromium.org

[Ted C, 2016-12-28 23:09:44]

PTAL

[Maria, 2016-12-29 00:14:56]

lgtm

https://codereview.chromium.org/2601183002/diff/1/chrome/android/java/src/org...
File
chrome/android/java/src/org/chromium/chrome/browser/ChromeTabbedActivity.java
(right):

https://codereview.chromium.org/2601183002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/ChromeTabbedActivity.java:849:
// Used by the Account management screen to open a new incognito tab.
do you want to add an assert here that externalAppId is the same as
getPackageName() just to clearly show what the expected invariant is?

[Ted C, 2016-12-29 00:22:24]

https://codereview.chromium.org/2601183002/diff/1/chrome/android/java/src/org...
File
chrome/android/java/src/org/chromium/chrome/browser/ChromeTabbedActivity.java
(right):

https://codereview.chromium.org/2601183002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/ChromeTabbedActivity.java:849:
// Used by the Account management screen to open a new incognito tab.
On 2016/12/29 00:14:56, Maria wrote:
> do you want to add an assert here that externalAppId is the same as
> getPackageName() just to clearly show what the expected invariant is?

Ahhh...I see that we set the package regardless in
shouldOverrideUrlLoadingInternal.  We also do it in the
LauncherShortcutActivity.

In this case, should I add an if clause at the top along the lines of

if (!TextUtils.equals(externalAppId, getPackageName())) {
    assert false : "";
    Log.e(TAG, "");
    return;
}

Then we can catch cases in code but also prevent any cases we missed from
allowing doors into this path?

I had that originally but didn't think we always set the externalAppId, but you
are certainly correct there.

[Maria, 2016-12-29 00:25:12]

On 2016/12/29 00:22:24, Ted C wrote:
>
https://codereview.chromium.org/2601183002/diff/1/chrome/android/java/src/org...
> File
> chrome/android/java/src/org/chromium/chrome/browser/ChromeTabbedActivity.java
> (right):
> 
>
https://codereview.chromium.org/2601183002/diff/1/chrome/android/java/src/org...
>
chrome/android/java/src/org/chromium/chrome/browser/ChromeTabbedActivity.java:849:
> // Used by the Account management screen to open a new incognito tab.
> On 2016/12/29 00:14:56, Maria wrote:
> > do you want to add an assert here that externalAppId is the same as
> > getPackageName() just to clearly show what the expected invariant is?
> 
> Ahhh...I see that we set the package regardless in
> shouldOverrideUrlLoadingInternal.  We also do it in the
> LauncherShortcutActivity.
> 
> In this case, should I add an if clause at the top along the lines of
> 
> if (!TextUtils.equals(externalAppId, getPackageName())) {
>     assert false : "";
>     Log.e(TAG, "");
>     return;
> }
> 
> Then we can catch cases in code but also prevent any cases we missed from
> allowing doors into this path?
> 
> I had that originally but didn't think we always set the externalAppId, but
you
> are certainly correct there.

Yeah, making that check in the code sounds good.

[Ted C, 2016-12-29 00:29:17]

On 2016/12/29 00:25:12, Maria wrote:
> On 2016/12/29 00:22:24, Ted C wrote:
> >
>
https://codereview.chromium.org/2601183002/diff/1/chrome/android/java/src/org...
> > File
> >
chrome/android/java/src/org/chromium/chrome/browser/ChromeTabbedActivity.java
> > (right):
> > 
> >
>
https://codereview.chromium.org/2601183002/diff/1/chrome/android/java/src/org...
> >
>
chrome/android/java/src/org/chromium/chrome/browser/ChromeTabbedActivity.java:849:
> > // Used by the Account management screen to open a new incognito tab.
> > On 2016/12/29 00:14:56, Maria wrote:
> > > do you want to add an assert here that externalAppId is the same as
> > > getPackageName() just to clearly show what the expected invariant is?
> > 
> > Ahhh...I see that we set the package regardless in
> > shouldOverrideUrlLoadingInternal.  We also do it in the
> > LauncherShortcutActivity.
> > 
> > In this case, should I add an if clause at the top along the lines of
> > 
> > if (!TextUtils.equals(externalAppId, getPackageName())) {
> >     assert false : "";
> >     Log.e(TAG, "");
> >     return;
> > }
> > 
> > Then we can catch cases in code but also prevent any cases we missed from
> > allowing doors into this path?
> > 
> > I had that originally but didn't think we always set the externalAppId, but
> you
> > are certainly correct there.
> 
> Yeah, making that check in the code sounds good.

Done

[Maria, 2016-12-29 00:37:17]

lgtm

[Ted C, 2016-12-29 00:39:06]

The CQ bit was checked by tedchoc@chromium.org

[commit-bot: I haz the power, 2016-12-29 00:39:22]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-29 01:10:09]

CQ is committing da patch.

Bot data: {"patchset_id": 20001, "attempt_start_ts": 1482971946895960,
"parent_rev": "c105348311c05f0d17f6cf68bd0aefbc1b28e05c", "commit_rev":
"cc967608e0987d8641e13fc8d772350cfda494e2"}

[commit-bot: I haz the power, 2016-12-29 01:11:35]

Description was changed from

==========
Remove code that implies external apps can open incognito tabs.

On Android, we restrict who can open incognito tabs to only
Chrome.  The logic to enforce this was spread across
IntentHandler#shouldIgnoreIntent and
ChromeTabbedActivity#processUrlViewIntent.  It was unclear and
confusing to read, so this removes the invalid paths and makes
it more explicit about what is allowed.

BUG=
==========

to

==========
Remove code that implies external apps can open incognito tabs.

On Android, we restrict who can open incognito tabs to only
Chrome.  The logic to enforce this was spread across
IntentHandler#shouldIgnoreIntent and
ChromeTabbedActivity#processUrlViewIntent.  It was unclear and
confusing to read, so this removes the invalid paths and makes
it more explicit about what is allowed.

BUG=

Review-Url: https://codereview.chromium.org/2601183002
==========

[commit-bot: I haz the power, 2016-12-29 01:11:36]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2017-01-02 15:51:08]

Description was changed from

==========
Remove code that implies external apps can open incognito tabs.

On Android, we restrict who can open incognito tabs to only
Chrome.  The logic to enforce this was spread across
IntentHandler#shouldIgnoreIntent and
ChromeTabbedActivity#processUrlViewIntent.  It was unclear and
confusing to read, so this removes the invalid paths and makes
it more explicit about what is allowed.

BUG=

Review-Url: https://codereview.chromium.org/2601183002
==========

to

==========
Remove code that implies external apps can open incognito tabs.

On Android, we restrict who can open incognito tabs to only
Chrome.  The logic to enforce this was spread across
IntentHandler#shouldIgnoreIntent and
ChromeTabbedActivity#processUrlViewIntent.  It was unclear and
confusing to read, so this removes the invalid paths and makes
it more explicit about what is allowed.

BUG=

Committed: https://crrev.com/1b2247e8e0a7277aa58ce4fcb242c7be2dafe564
Cr-Commit-Position: refs/heads/master@{#440918}
==========

[commit-bot: I haz the power, 2017-01-02 15:51:09]

Patchset 2 (id:??) landed as
https://crrev.com/1b2247e8e0a7277aa58ce4fcb242c7be2dafe564
Cr-Commit-Position: refs/heads/master@{#440918}