Use tabId as request code in media notification pending intent

Use tabId as request code in media notification pending intent

This makes the media nofication correctly reopen custom tabs, instead
of opening Chrome. The request code is needed to make the pending
intent unique, since Android doesn't look at the extras when comparing
intents. Without this Android will incorrectly merge this pending
intent with other pending intents with action ACTION_MAIN.

BUG=585395
Committed: https://crrev.com/b228999af8e0441607a20ee6f9cea54ca685eebe
Cr-Commit-Position: refs/heads/master@{#376767}

[aberent, 2016-02-17 17:57:00]

Description was changed from

==========
Use tabId as request code in media notification pending intent

This makes the media nofication correctly reopen custom tabs, instead
of opening Chrome. The request code is needed to make the pending
intent unique, since Android doesn't look at the extra's when comparing
intents. Without this Android will incorrectly merge this pending
intent with other pending intents with action ACTION_MAIN.

BUG=585395
==========

to

==========
Use tabId as request code in media notification pending intent

This makes the media nofication correctly reopen custom tabs, instead
of opening Chrome. The request code is needed to make the pending
intent unique, since Android doesn't look at the extras when comparing
intents. Without this Android will incorrectly merge this pending
intent with other pending intents with action ACTION_MAIN.

BUG=585395
==========

[aberent, 2016-02-17 17:58:15]

aberent@chromium.org changed reviewers:
+ mlamouri@chromium.org

[mlamouri (slow - plz ping), 2016-02-18 13:41:51]

mlamouri@chromium.org changed reviewers:
+ dfalcantara@chromium.org

[mlamouri (slow - plz ping), 2016-02-18 13:41:52]

+dfalcantara@

Dan, I think it would be great if Tab.createBringBackToFrontIntent() was
returning Intent that are different per Intent.filterEquals() (ie. different
data). Could we use an internal scheme like "tab" or "chrome-tab" to include the
tab id. Something like "tab://0". That way, we will not have to pass this key to
make bring back to front intents different for PendingIntent. I think the fact
that all these intents look the same to Android is a huge foot gun (and smoking
;)).

[gone, 2016-02-18 17:55:25]

On 2016/02/18 13:41:52, Mounir Lamouri (slow) wrote:
> +dfalcantara@
> 
> Dan, I think it would be great if Tab.createBringBackToFrontIntent() was
> returning Intent that are different per Intent.filterEquals() (ie. different
> data). Could we use an internal scheme like "tab" or "chrome-tab" to include
the
> tab id. Something like "tab://0". That way, we will not have to pass this key
to
> make bring back to front intents different for PendingIntent. I think the fact
> that all these intents look the same to Android is a huge foot gun (and
smoking
> ;)).

That TabOpenType.BRING_TAB_TO_FRONT extra was being used even before I started
mucking
with Tab#createBringTabToFrontIntent().  That said, wouldn't it make more sense
to use
a custom Intent Action instead of defining another scheme?  I completely lack
context
here on what that contentIntent contains, though, so I may be misgauging the
problem.

[mlamouri (slow - plz ping), 2016-02-22 15:39:56]

On 2016/02/18 at 17:55:25, dfalcantara wrote:
> On 2016/02/18 13:41:52, Mounir Lamouri (slow) wrote:
> > +dfalcantara@
> > 
> > Dan, I think it would be great if Tab.createBringBackToFrontIntent() was
> > returning Intent that are different per Intent.filterEquals() (ie. different
> > data). Could we use an internal scheme like "tab" or "chrome-tab" to include
the
> > tab id. Something like "tab://0". That way, we will not have to pass this
key to
> > make bring back to front intents different for PendingIntent. I think the
fact
> > that all these intents look the same to Android is a huge foot gun (and
smoking
> > ;)).
> 
> That TabOpenType.BRING_TAB_TO_FRONT extra was being used even before I started
mucking
> with Tab#createBringTabToFrontIntent().  That said, wouldn't it make more
sense to use
> a custom Intent Action instead of defining another scheme?  I completely lack
context
> here on what that contentIntent contains, though, so I may be misgauging the
problem.

The content intent is a PendingIntent that will be run when the notification is
clicked. The PendingIntent is created with an intent which is the bring back to
front intent.

I'm not sure if using a custom action intent would help given that we need the
bring back to front intents to be different from each other and the difference
has to come from the data and if I understand correctly, the data is the URL.

[aberent, 2016-02-22 16:19:40]

The CQ bit was checked by aberent@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-02-22 16:20:28]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1707783002/1
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1707783002/1

[aberent, 2016-02-22 16:23:02]

On 2016/02/22 15:39:56, Mounir Lamouri (slow) wrote:
> On 2016/02/18 at 17:55:25, dfalcantara wrote:
> > On 2016/02/18 13:41:52, Mounir Lamouri (slow) wrote:
> > > +dfalcantara@
> > > 
> > > Dan, I think it would be great if Tab.createBringBackToFrontIntent() was
> > > returning Intent that are different per Intent.filterEquals() (ie.
different
> > > data). Could we use an internal scheme like "tab" or "chrome-tab" to
include
> the
> > > tab id. Something like "tab://0". That way, we will not have to pass this
> key to
> > > make bring back to front intents different for PendingIntent. I think the
> fact
> > > that all these intents look the same to Android is a huge foot gun (and
> smoking
> > > ;)).
> > 
> > That TabOpenType.BRING_TAB_TO_FRONT extra was being used even before I
started
> mucking
> > with Tab#createBringTabToFrontIntent().  That said, wouldn't it make more
> sense to use
> > a custom Intent Action instead of defining another scheme?  I completely
lack
> context
> > here on what that contentIntent contains, though, so I may be misgauging the
> problem.
> 
> The content intent is a PendingIntent that will be run when the notification
is
> clicked. The PendingIntent is created with an intent which is the bring back
to
> front intent.
> 
> I'm not sure if using a custom action intent would help given that we need the
> bring back to front intents to be different from each other and the difference
> has to come from the data and if I understand correctly, the data is the URL.

While it would be good to have a long term solution to this, the bug is a
priority 1 bug, so we probably want to land a fix before the branch point on
Friday. Please comment on whether this fix should land.

[commit-bot: I haz the power, 2016-02-22 17:10:45]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-02-22 17:10:46]

Dry run: This issue passed the CQ dry run.

[gone, 2016-02-22 17:56:42]

Leaving the l-g-t-m to Mounir since he understands how the resultCode will be
handled, but if it's just reverting an accidental change I'd assume it was
alright.

Intents are checked for difference on both ACTION and DATA last I checked.  But
you should start a thread on a bug somewhere to continue the longer-term
discussion, if you're interested.

[mlamouri (slow - plz ping), 2016-02-22 18:47:46]

The CQ bit was checked by mlamouri@chromium.org

[mlamouri (slow - plz ping), 2016-02-22 18:47:47]

lgtm

[commit-bot: I haz the power, 2016-02-22 18:48:18]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1707783002/1
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1707783002/1

[commit-bot: I haz the power, 2016-02-22 18:54:57]

Description was changed from

==========
Use tabId as request code in media notification pending intent

This makes the media nofication correctly reopen custom tabs, instead
of opening Chrome. The request code is needed to make the pending
intent unique, since Android doesn't look at the extras when comparing
intents. Without this Android will incorrectly merge this pending
intent with other pending intents with action ACTION_MAIN.

BUG=585395
==========

to

==========
Use tabId as request code in media notification pending intent

This makes the media nofication correctly reopen custom tabs, instead
of opening Chrome. The request code is needed to make the pending
intent unique, since Android doesn't look at the extras when comparing
intents. Without this Android will incorrectly merge this pending
intent with other pending intents with action ACTION_MAIN.

BUG=585395
==========

[commit-bot: I haz the power, 2016-02-22 18:54:58]

Committed patchset #1 (id:1)

[commit-bot: I haz the power, 2016-02-22 18:55:59]

Description was changed from

==========
Use tabId as request code in media notification pending intent

This makes the media nofication correctly reopen custom tabs, instead
of opening Chrome. The request code is needed to make the pending
intent unique, since Android doesn't look at the extras when comparing
intents. Without this Android will incorrectly merge this pending
intent with other pending intents with action ACTION_MAIN.

BUG=585395
==========

to

==========
Use tabId as request code in media notification pending intent

This makes the media nofication correctly reopen custom tabs, instead
of opening Chrome. The request code is needed to make the pending
intent unique, since Android doesn't look at the extras when comparing
intents. Without this Android will incorrectly merge this pending
intent with other pending intents with action ACTION_MAIN.

BUG=585395

Committed: https://crrev.com/b228999af8e0441607a20ee6f9cea54ca685eebe
Cr-Commit-Position: refs/heads/master@{#376767}
==========

[commit-bot: I haz the power, 2016-02-22 18:56:00]

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