Mark forwarded user gestures as forwarded, and don't forward already forwarded user gestures.

Mark forwarded user gestures as forwarded, and don't forward already forwarded user gestures.

BUG=354217
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=267057

[Marijn Kruisselbrink, 2014-04-28 18:10:54]

(this depends on the blink changes in
https://codereview.chromium.org/258783004/)

[not at google - send to devlin, 2014-04-28 19:13:12]

lgtm, there have been a number of other tests added recently and I wonder if
they can be cleaned up. e.g. there was that test where a small setTimeout value
was used and it reset the token too often; can that test be changed to a smaller
setTimeout?

I hope that somewhere we ensure that the tokens are maintained during a closure
(i.e. that on getting a message you can call chrome.permissions.request(..) then
on that callback mediaGalleries.pick or whatever it's called).

[Marijn Kruisselbrink, 2014-04-29 17:24:22]

On 2014/04/28 19:13:12, kalman wrote:
> lgtm, there have been a number of other tests added recently and I wonder if
> they can be cleaned up. e.g. there was that test where a small setTimeout
value
> was used and it reset the token too often; can that test be changed to a
smaller
> setTimeout?
I'm not exactly sure what test you're referring to?

> I hope that somewhere we ensure that the tokens are maintained during a
closure
> (i.e. that on getting a message you can call chrome.permissions.request(..)
then
> on that callback mediaGalleries.pick or whatever it's called).
We have tests that ensure you have a user gesture on getting a message, and we
have tests that ensure that if you have a user gesture when calling
permissions.request() the callback also gets a user gesture on success. I'm not
sure if we need additional tests for the combination of these two.

[not at google - send to devlin, 2014-04-29 17:25:08]

On 2014/04/29 17:24:22, Marijn Kruisselbrink wrote:
> On 2014/04/28 19:13:12, kalman wrote:
> > lgtm, there have been a number of other tests added recently and I wonder if
> > they can be cleaned up. e.g. there was that test where a small setTimeout
> value
> > was used and it reset the token too often; can that test be changed to a
> smaller
> > setTimeout?
> I'm not exactly sure what test you're referring to?

an external contributor added them recently.

> 
> > I hope that somewhere we ensure that the tokens are maintained during a
> closure
> > (i.e. that on getting a message you can call chrome.permissions.request(..)
> then
> > on that callback mediaGalleries.pick or whatever it's called).
> We have tests that ensure you have a user gesture on getting a message, and we
> have tests that ensure that if you have a user gesture when calling
> permissions.request() the callback also gets a user gesture on success. I'm
not
> sure if we need additional tests for the combination of these two.

ok.

[Marijn Kruisselbrink, 2014-04-29 20:26:39]

The CQ bit was checked by mek@chromium.org

[commit-bot: I haz the power, 2014-04-29 20:43:13]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mek@chromium.org/256623008/20001

[commit-bot: I haz the power, 2014-04-30 03:07:29]

Change committed as 267057

[wjywbs, 2014-05-01 00:27:31]

This change broke keeping the user gesture in this case again:
window.postMessage from web page -> content script -> extension background page.
The user gesture can not pass from content script to extension background page
anymore because it's already forwarded once from web page to content script.

[Marijn Kruisselbrink, 2014-05-01 00:37:52]

On 2014/05/01 00:27:31, wjywbs wrote:
> This change broke keeping the user gesture in this case again:
> window.postMessage from web page -> content script -> extension background
page.
> The user gesture can not pass from content script to extension background page
> anymore because it's already forwarded once from web page to content script.
Hmm, true... I tried to think of valid usecases where forwarding a user gesture
twice would be meaningful, but I hadn't considered that particular situation.
externally_connectable might be a workaround, since it would allow the web page
to directly message the background page. Other than that I'm not quite sure what
exact usecases are enabled by this double forwarding?

[wjywbs, 2014-05-01 01:20:51]

On 2014/05/01 00:37:52, Marijn Kruisselbrink wrote:
> On 2014/05/01 00:27:31, wjywbs wrote:
> > This change broke keeping the user gesture in this case again:
> > window.postMessage from web page -> content script -> extension background
> page.
> > The user gesture can not pass from content script to extension background
page
> > anymore because it's already forwarded once from web page to content script.
> Hmm, true... I tried to think of valid usecases where forwarding a user
gesture
> twice would be meaningful, but I hadn't considered that particular situation.
> externally_connectable might be a workaround, since it would allow the web
page
> to directly message the background page. Other than that I'm not quite sure
what
> exact usecases are enabled by this double forwarding?

It can be used when an extension changes a web page into an app manager. When
the user clicks the remove button on the web page, it does window.postMessage
from web page -> content script -> extension background page.

An example is:
https://github.com/wjywbs/chrome-newtab-reloaded/blob/session/newtab_reloaded...
https://github.com/wjywbs/chrome-newtab-reloaded/tree/session

[not at google - send to devlin, 2014-05-01 15:59:19]

On 2014/05/01 01:20:51, wjywbs wrote:
> On 2014/05/01 00:37:52, Marijn Kruisselbrink wrote:
> > On 2014/05/01 00:27:31, wjywbs wrote:
> > > This change broke keeping the user gesture in this case again:
> > > window.postMessage from web page -> content script -> extension background
> > page.
> > > The user gesture can not pass from content script to extension background
> page
> > > anymore because it's already forwarded once from web page to content
script.
> > Hmm, true... I tried to think of valid usecases where forwarding a user
> gesture
> > twice would be meaningful, but I hadn't considered that particular
situation.
> > externally_connectable might be a workaround, since it would allow the web
> page
> > to directly message the background page. Other than that I'm not quite sure
> what
> > exact usecases are enabled by this double forwarding?
> 
> It can be used when an extension changes a web page into an app manager. When
> the user clicks the remove button on the web page, it does window.postMessage
> from web page -> content script -> extension background page.
> 
> An example is:
>
https://github.com/wjywbs/chrome-newtab-reloaded/blob/session/newtab_reloaded...
> https://github.com/wjywbs/chrome-newtab-reloaded/tree/session

Without reading your code in too much detail -- why do you need to postMessage
from the page to the content script? You should be able to listen to events from
the content script, or at least fire a CustomEvent to communicate with it.

[wjywbs, 2014-05-01 21:52:26]

On 2014/05/01 15:59:19, kalman wrote:
> Without reading your code in too much detail -- why do you need to postMessage
> from the page to the content script? You should be able to listen to events
from
> the content script, or at least fire a CustomEvent to communicate with it.

CustomEvent works. Thanks.

[Marijn Kruisselbrink, 2014-05-06 17:49:09]

A revert of this CL has been created in
https://codereview.chromium.org/261753019/ by mek@chromium.org.

The reason for reverting is: The blink-side change for this had the unintended
side effect of breaking websites,
so reverting this while we figure out a proper fix.

BUG=369963.