Suppress JavaScript dialogs when a tab puts itself into fullscreen.

Suppress JavaScript dialogs when a tab puts itself into fullscreen.

BUG=550017
TEST=as in bug

[Avi (use Gerrit), 2016-08-01 16:40:04]

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

[commit-bot: I haz the power, 2016-08-01 16:40:14]

Dry run: 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-08-01 16:51:37]

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

[commit-bot: I haz the power, 2016-08-01 16:51:37]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Avi (use Gerrit), 2016-08-01 17:01:46]

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

[commit-bot: I haz the power, 2016-08-01 17:02:20]

Dry run: 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-08-01 17:55:55]

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

[commit-bot: I haz the power, 2016-08-01 17:55:56]

Dry run: This issue passed the CQ dry run.

[Avi (use Gerrit), 2016-08-01 18:06:53]

avi@chromium.org changed reviewers:
+ jam@chromium.org, meacer@chromium.org, mgiuca@chromium.org

[Avi (use Gerrit), 2016-08-01 18:06:54]

meacer, mgiuca: dialogs
jam: the content change and associated random files

[meacer, 2016-08-01 18:23:26]

LGTM.

Was there a discussion of this change outside the associated bug? I'm not sure
if the only decision was the one where I said "please go ahead and do this", if
so, a PSA could be useful :)

[Avi (use Gerrit), 2016-08-01 18:27:49]

On 2016/08/01 18:23:26, Mustafa Emre Acer wrote:
> Was there a discussion of this change outside the associated bug?

Not that I'm aware of. You kept referring to "the discussion" and I had no idea
what discussion you were referring to. There's a long-term discussion about
depowering alerts, but no one had been proposing removing alerts from fullscreen
pages except for you who kept asserting that someone had made that decision.

If you could refer me to who made that decision that would be great.

> I'm not sure
> if the only decision was the one where I said "please go ahead and do this",
if
> so, a PSA could be useful :)

Yes, you are the only person I'm aware of who said to do it.

Do you want to file the intent on blink-dev?

[meacer, 2016-08-01 18:37:12]

On 2016/08/01 18:27:49, Avi wrote:
> On 2016/08/01 18:23:26, Mustafa Emre Acer wrote:
> > Was there a discussion of this change outside the associated bug?
> 
> Not that I'm aware of. You kept referring to "the discussion" and I had no
idea
> what discussion you were referring to. There's a long-term discussion about
> depowering alerts, but no one had been proposing removing alerts from
fullscreen
> pages except for you who kept asserting that someone had made that decision.

Do you mean Matt, as I don't see any comments where I referred to "the
discussion"?
I suggested doing this in a comment, expecting it to be a controversial
proposal, but
it seemed to be accepted as the right way forward.

> 
> If you could refer me to who made that decision that would be great.
> 
> > I'm not sure
> > if the only decision was the one where I said "please go ahead and do this",
> if
> > so, a PSA could be useful :)
> 
> Yes, you are the only person I'm aware of who said to do it.
> 
> Do you want to file the intent on blink-dev?

Given that "the discussion" doesn't seem to exist, we should first drop view
restrictions on the bug and get some feedback. I can file an intent on blink-dev
if you
or Matt aren't super excited to do so.

[Avi (use Gerrit), 2016-08-01 18:59:23]

On 2016/08/01 18:37:12, Mustafa Emre Acer wrote:
> On 2016/08/01 18:27:49, Avi wrote:
> > On 2016/08/01 18:23:26, Mustafa Emre Acer wrote:
> > > Was there a discussion of this change outside the associated bug?
> > 
> > Not that I'm aware of. You kept referring to "the discussion" and I had no
> idea
> > what discussion you were referring to. There's a long-term discussion about
> > depowering alerts, but no one had been proposing removing alerts from
> fullscreen
> > pages except for you who kept asserting that someone had made that decision.
> 
> Do you mean Matt, as I don't see any comments where I referred to "the
> discussion"?
> I suggested doing this in a comment, expecting it to be a controversial
> proposal, but
> it seemed to be accepted as the right way forward.
> 
> > 
> > If you could refer me to who made that decision that would be great.
> > 
> > > I'm not sure
> > > if the only decision was the one where I said "please go ahead and do
this",
> > if
> > > so, a PSA could be useful :)
> > 
> > Yes, you are the only person I'm aware of who said to do it.
> > 
> > Do you want to file the intent on blink-dev?
> 
> Given that "the discussion" doesn't seem to exist, we should first drop view
> restrictions on the bug and get some feedback. I can file an intent on
blink-dev
> if you
> or Matt aren't super excited to do so.

I don't think we should drop restrictions on the bug yet; it's a security bug
after all.

The idea of dropping dialogs from fullscreens got decent support on the bug, but
we probably should get a wider airing of the issue without giving away the
security implications. (If we can do that...)

[Matt Giuca, 2016-08-02 01:42:10]

code lgtm (very neat, thankyou for doing this, I wouldn't have known to use this
existing infrastructure), but pending on the process discussion.

On 2016/08/01 18:59:23, Avi wrote:
> On 2016/08/01 18:37:12, Mustafa Emre Acer wrote:
> > On 2016/08/01 18:27:49, Avi wrote:
> > > On 2016/08/01 18:23:26, Mustafa Emre Acer wrote:
> > > > Was there a discussion of this change outside the associated bug?
> > > 
> > > Not that I'm aware of. You kept referring to "the discussion" and I had no
> > idea
> > > what discussion you were referring to. There's a long-term discussion
about
> > > depowering alerts, but no one had been proposing removing alerts from
> > fullscreen
> > > pages except for you who kept asserting that someone had made that
decision.
> > 
> > Do you mean Matt, as I don't see any comments where I referred to "the
> > discussion"?
> > I suggested doing this in a comment, expecting it to be a controversial
> > proposal, but
> > it seemed to be accepted as the right way forward.
> > 
> > > 
> > > If you could refer me to who made that decision that would be great.
> > > 
> > > > I'm not sure
> > > > if the only decision was the one where I said "please go ahead and do
> this",
> > > if
> > > > so, a PSA could be useful :)
> > > 
> > > Yes, you are the only person I'm aware of who said to do it.
> > > 
> > > Do you want to file the intent on blink-dev?
> > 
> > Given that "the discussion" doesn't seem to exist, we should first drop view
> > restrictions on the bug and get some feedback. I can file an intent on
> blink-dev
> > if you
> > or Matt aren't super excited to do so.
> 
> I don't think we should drop restrictions on the bug yet; it's a security bug
> after all.
> 
> The idea of dropping dialogs from fullscreens got decent support on the bug,
but
> we probably should get a wider airing of the issue without giving away the
> security implications. (If we can do that...)

I think discussing the security implications in public is necessary if we're to
have a public discussion about the merits of this change. (And by the way, this
CL is public.) I think sending an intent-to-deprecate to blink-dev is
appropriate before landing this.

I went to the specs:

https://www.w3.org/TR/html5/webappapis.html#user-prompts

alert:
3. Optionally, abort these steps.

confirm:
3. Optionally, return false and abort these steps.

prompt:
3. Optionally, return null and abort these steps.

So we are within spec if we do not show the dialog, for any reason, as long as
confirm() returns false and prompt() returns null (which I assume is what this
code naturally does when the suppress message flag is active).

[Matt Giuca, 2016-08-02 01:42:21]

https://codereview.chromium.org/2194243002/diff/20001/chrome/browser/ui/brows...
File chrome/browser/ui/browser.cc (right):

https://codereview.chromium.org/2194243002/diff/20001/chrome/browser/ui/brows...
chrome/browser/ui/browser.cc:1213: return
exclusive_access_manager_->fullscreen_controller()
optionally add a comment:
// Do not show dialogs if the browser is in fullscreen.

[Avi (use Gerrit), 2016-08-02 01:53:07]

On 2016/08/02 01:42:10, Matt Giuca wrote:
> So we are within spec if we do not show the dialog, for any reason

The spec follows browsers. As a specific example, the onbeforeunload change to
not show the page's string was against the spec before we made the change, and
then the spec was changed *afterwards* once three of four browsers did it that
way.

Don't use the spec as justification, just as backup.

[Matt Giuca, 2016-08-02 02:56:05]

On 2016/08/02 01:53:07, Avi wrote:
> On 2016/08/02 01:42:10, Matt Giuca wrote:
> > So we are within spec if we do not show the dialog, for any reason
> 
> The spec follows browsers. As a specific example, the onbeforeunload change to
> not show the page's string was against the spec before we made the change, and
> then the spec was changed *afterwards* once three of four browsers did it that
> way.
> 
> Don't use the spec as justification, just as backup.

Sure, but it's good that we are in line with what the spec currently says so we
don't have to change it further.

[Matt Giuca, 2016-08-02 03:34:56]

not lgtm (to cancel my previous lg); I thought some more about it and I think we
should tweak the behaviour. See comment.

https://codereview.chromium.org/2194243002/diff/20001/chrome/browser/ui/brows...
File chrome/browser/ui/browser.cc (right):

https://codereview.chromium.org/2194243002/diff/20001/chrome/browser/ui/brows...
chrome/browser/ui/browser.cc:1214: ->IsControllerInitiatedFullscreen();
Actually, I thought some more about this. I think we don't want this suppressed
in user-entered fullscreen (e.g., F11), only in site-entered fullscreen (HTML5
API).

Rationale is that sites have no control over the user-entered fullscreen so this
would immediately break all sites using alerts, if the user decides to use
fullscreen (e.g., using Reitveld in fullscreen, press Delete on a Patch Set and
there will be no confirm prompt, just silent failure). Whereas if we only block
prompts on HTML5 fullscreen, it only breaks a fraction of sites. Also there is
no spoofing possibility for user fullscreen.

So change IsControllerInitiatedFullscreen to IsTabFullscreen. See:
https://cs.chromium.org/chromium/src/chrome/browser/renderer_context_menu/ren...

(The name IsTabFullscreen is bad; I'll change it but after your CL to avoid
conflicts.)

[miu, 2016-08-02 03:53:16]

Perhaps crbug.com/456 is relevant here? Personally, I would love to see modal
dialogs from web pages eliminated completely once and for all...

[Avi (use Gerrit), 2016-08-02 03:53:31]

https://codereview.chromium.org/2194243002/diff/20001/chrome/browser/ui/brows...
File chrome/browser/ui/browser.cc (right):

https://codereview.chromium.org/2194243002/diff/20001/chrome/browser/ui/brows...
chrome/browser/ui/browser.cc:1213: return
exclusive_access_manager_->fullscreen_controller()
On 2016/08/02 01:42:20, Matt Giuca wrote:
> optionally add a comment:
> // Do not show dialogs if the browser is in fullscreen.

That seems obvious enough from the code, though.

https://codereview.chromium.org/2194243002/diff/20001/chrome/browser/ui/brows...
chrome/browser/ui/browser.cc:1214: ->IsControllerInitiatedFullscreen();
On 2016/08/02 03:34:56, Matt Giuca wrote:
> Actually, I thought some more about this. I think we don't want this
suppressed
> in user-entered fullscreen (e.g., F11), only in site-entered fullscreen (HTML5
> API).
> 
> Rationale is that sites have no control over the user-entered fullscreen so
this
> would immediately break all sites using alerts, if the user decides to use
> fullscreen (e.g., using Reitveld in fullscreen, press Delete on a Patch Set
and
> there will be no confirm prompt, just silent failure). Whereas if we only
block
> prompts on HTML5 fullscreen, it only breaks a fraction of sites. Also there is
> no spoofing possibility for user fullscreen.
> 
> So change IsControllerInitiatedFullscreen to IsTabFullscreen. See:
>
https://cs.chromium.org/chromium/src/chrome/browser/renderer_context_menu/ren...
> 
> (The name IsTabFullscreen is bad; I'll change it but after your CL to avoid
> conflicts.)

That is a very reasonable approach, and I was thinking about our possibilities
here, but got lost in the details of the fullscreen options.

First, I'd recommend you go ahead and rename it, and I'll rebase.

Second, this really helps us make the case publicly.

Do the renaming tomorrow morning and let me know the new name, and I'll adjust
this.

[Matt Giuca, 2016-08-02 04:23:50]

https://codereview.chromium.org/2194243002/diff/20001/chrome/browser/ui/brows...
File chrome/browser/ui/browser.cc (right):

https://codereview.chromium.org/2194243002/diff/20001/chrome/browser/ui/brows...
chrome/browser/ui/browser.cc:1213: return
exclusive_access_manager_->fullscreen_controller()
On 2016/08/02 03:53:30, Avi wrote:
> On 2016/08/02 01:42:20, Matt Giuca wrote:
> > optionally add a comment:
> > // Do not show dialogs if the browser is in fullscreen.
> 
> That seems obvious enough from the code, though.

Acknowledged, but see below because it's going to be less clear now.

https://codereview.chromium.org/2194243002/diff/20001/chrome/browser/ui/brows...
chrome/browser/ui/browser.cc:1214: ->IsControllerInitiatedFullscreen();
On 2016/08/02 03:53:30, Avi wrote:
> On 2016/08/02 03:34:56, Matt Giuca wrote:
> That is a very reasonable approach, and I was thinking about our possibilities
> here, but got lost in the details of the fullscreen options.
> 
> First, I'd recommend you go ahead and rename it, and I'll rebase.

Actually, I started doing this but I discovered that FullscreenController has a
fairly well-established terminology for "tab fullscreen" versus "browser
fullscreen" which I don't think we want to change:

https://cs.chromium.org/chromium/src/chrome/browser/ui/exclusive_access/fulls...

"Tab fullscreen" refers to any renderer-initiated fullscreen (Flash or HTML5
API) so renaming it to "IsHTMLFullscreen" would become a misnomer. Also the
terminology "Tab" is internally consistent throughout the many methods of
FullscreenController. So let's just keep it as-is. Maybe you can add a comment
here saying exactly what modes this captures.

(I'm not sure we need to worry about Flash fullscreen because we shouldn't be
getting alert() popups during fullscreen Flash, but if we did, I think it would
be safe to suppress them.)

> 
> Second, this really helps us make the case publicly.
> 
> Do the renaming tomorrow morning and let me know the new name, and I'll adjust
> this.

[Avi (use Gerrit), 2016-08-02 04:27:54]

https://codereview.chromium.org/2194243002/diff/20001/chrome/browser/ui/brows...
File chrome/browser/ui/browser.cc (right):

https://codereview.chromium.org/2194243002/diff/20001/chrome/browser/ui/brows...
chrome/browser/ui/browser.cc:1214: ->IsControllerInitiatedFullscreen();
On 2016/08/02 04:23:50, Matt Giuca wrote:
> On 2016/08/02 03:53:30, Avi wrote:
> > On 2016/08/02 03:34:56, Matt Giuca wrote:
> > That is a very reasonable approach, and I was thinking about our
possibilities
> > here, but got lost in the details of the fullscreen options.
> > 
> > First, I'd recommend you go ahead and rename it, and I'll rebase.
> 
> Actually, I started doing this but I discovered that FullscreenController has
a
> fairly well-established terminology for "tab fullscreen" versus "browser
> fullscreen" which I don't think we want to change:
> 
>
https://cs.chromium.org/chromium/src/chrome/browser/ui/exclusive_access/fulls...
> 
> "Tab fullscreen" refers to any renderer-initiated fullscreen (Flash or HTML5
> API) so renaming it to "IsHTMLFullscreen" would become a misnomer. Also the
> terminology "Tab" is internally consistent throughout the many methods of
> FullscreenController. So let's just keep it as-is. Maybe you can add a comment
> here saying exactly what modes this captures.
> 
> (I'm not sure we need to worry about Flash fullscreen because we shouldn't be
> getting alert() popups during fullscreen Flash, but if we did, I think it
would
> be safe to suppress them.)
> 
> > 
> > Second, this really helps us make the case publicly.
> > 
> > Do the renaming tomorrow morning and let me know the new name, and I'll
adjust
> > this.
> 

In that case, agreed, I'll clarify. How about:

// In the case where fullscreen was triggered by the web page
// ("tab fullscreen"), do not allow dialogs. This does not affect
// the case of user-initiated fullscreen ("browser fullscreen").

Is that accurate? (I'll switch to IsTabFullscreen with the next patchset too.)

[Matt Giuca, 2016-08-02 04:28:34]

On 2016/08/02 04:27:54, Avi wrote:
>
https://codereview.chromium.org/2194243002/diff/20001/chrome/browser/ui/brows...
> File chrome/browser/ui/browser.cc (right):
> 
>
https://codereview.chromium.org/2194243002/diff/20001/chrome/browser/ui/brows...
> chrome/browser/ui/browser.cc:1214: ->IsControllerInitiatedFullscreen();
> On 2016/08/02 04:23:50, Matt Giuca wrote:
> > On 2016/08/02 03:53:30, Avi wrote:
> > > On 2016/08/02 03:34:56, Matt Giuca wrote:
> > > That is a very reasonable approach, and I was thinking about our
> possibilities
> > > here, but got lost in the details of the fullscreen options.
> > > 
> > > First, I'd recommend you go ahead and rename it, and I'll rebase.
> > 
> > Actually, I started doing this but I discovered that FullscreenController
has
> a
> > fairly well-established terminology for "tab fullscreen" versus "browser
> > fullscreen" which I don't think we want to change:
> > 
> >
>
https://cs.chromium.org/chromium/src/chrome/browser/ui/exclusive_access/fulls...
> > 
> > "Tab fullscreen" refers to any renderer-initiated fullscreen (Flash or HTML5
> > API) so renaming it to "IsHTMLFullscreen" would become a misnomer. Also the
> > terminology "Tab" is internally consistent throughout the many methods of
> > FullscreenController. So let's just keep it as-is. Maybe you can add a
comment
> > here saying exactly what modes this captures.
> > 
> > (I'm not sure we need to worry about Flash fullscreen because we shouldn't
be
> > getting alert() popups during fullscreen Flash, but if we did, I think it
> would
> > be safe to suppress them.)
> > 
> > > 
> > > Second, this really helps us make the case publicly.
> > > 
> > > Do the renaming tomorrow morning and let me know the new name, and I'll
> adjust
> > > this.
> > 
> 
> In that case, agreed, I'll clarify. How about:
> 
> // In the case where fullscreen was triggered by the web page
> // ("tab fullscreen"), do not allow dialogs. This does not affect
> // the case of user-initiated fullscreen ("browser fullscreen").
> 
> Is that accurate? (I'll switch to IsTabFullscreen with the next patchset too.)

SGTM

[Matt Giuca, 2016-08-02 04:28:52]

https://codereview.chromium.org/2194243002/diff/20001/chrome/browser/ui/brows...
File chrome/browser/ui/browser.cc (right):

https://codereview.chromium.org/2194243002/diff/20001/chrome/browser/ui/brows...
chrome/browser/ui/browser.cc:1214: ->IsControllerInitiatedFullscreen();
On 2016/08/02 04:27:54, Avi wrote:
> On 2016/08/02 04:23:50, Matt Giuca wrote:
> > On 2016/08/02 03:53:30, Avi wrote:
> > > On 2016/08/02 03:34:56, Matt Giuca wrote:
> > > That is a very reasonable approach, and I was thinking about our
> possibilities
> > > here, but got lost in the details of the fullscreen options.
> > > 
> > > First, I'd recommend you go ahead and rename it, and I'll rebase.
> > 
> > Actually, I started doing this but I discovered that FullscreenController
has
> a
> > fairly well-established terminology for "tab fullscreen" versus "browser
> > fullscreen" which I don't think we want to change:
> > 
> >
>
https://cs.chromium.org/chromium/src/chrome/browser/ui/exclusive_access/fulls...
> > 
> > "Tab fullscreen" refers to any renderer-initiated fullscreen (Flash or HTML5
> > API) so renaming it to "IsHTMLFullscreen" would become a misnomer. Also the
> > terminology "Tab" is internally consistent throughout the many methods of
> > FullscreenController. So let's just keep it as-is. Maybe you can add a
comment
> > here saying exactly what modes this captures.
> > 
> > (I'm not sure we need to worry about Flash fullscreen because we shouldn't
be
> > getting alert() popups during fullscreen Flash, but if we did, I think it
> would
> > be safe to suppress them.)
> > 
> > > 
> > > Second, this really helps us make the case publicly.
> > > 
> > > Do the renaming tomorrow morning and let me know the new name, and I'll
> adjust
> > > this.
> > 
> 
> In that case, agreed, I'll clarify. How about:
> 
> // In the case where fullscreen was triggered by the web page
> // ("tab fullscreen"), do not allow dialogs. This does not affect
> // the case of user-initiated fullscreen ("browser fullscreen").
> 
> Is that accurate? (I'll switch to IsTabFullscreen with the next patchset too.)

SGTM.

[Avi (use Gerrit), 2016-08-02 15:38:37]

Description was changed from

==========
Suppress JavaScript dialogs while a tab is fullscreen.

BUG=550017
TEST=as in bug
==========

to

==========
Suppress JavaScript dialogs when a tab puts itself into fullscreen.

BUG=550017
TEST=as in bug
==========

[Avi (use Gerrit), 2016-08-02 15:41:25]

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

[commit-bot: I haz the power, 2016-08-02 15:41:44]

Dry run: 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-08-02 16:46:34]

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

[commit-bot: I haz the power, 2016-08-02 16:46:35]

Dry run: This issue passed the CQ dry run.

[Avi (use Gerrit), 2016-08-02 17:41:59]

Matt, PTAL.

[Matt Giuca, 2016-08-03 01:29:24]

lgtm. Thanks for doing this!

[Avi (use Gerrit), 2016-08-03 03:53:18]

On 2016/08/03 01:29:24, Matt Giuca wrote:
> lgtm. Thanks for doing this!

Are we doing a public discussion of this?

[Matt Giuca, 2016-08-03 04:23:23]

On 2016/08/03 03:53:18, Avi wrote:
> On 2016/08/03 01:29:24, Matt Giuca wrote:
> > lgtm. Thanks for doing this!
> 
> Are we doing a public discussion of this?

Yes, I think it at least makes sense to send an "Intent to deprecate and remove"
email to blink-dev:
http://www.chromium.org/blink
("Launch process: Deprecation")

In fact, we probably should be collecting data about this first, to confirm what
percentage of sites are using dialogs while in full screen.

(Perhaps we could add UMA to Browser::ShouldSuppressDialogs which logs whether
IsTabFullscreen() returns true for each activation of the prompt?)

[Avi (use Gerrit), 2016-08-10 16:42:37]

Where do we stand with the intent, etc? Mustafa, you said you would take this.

This is a security bug that's been sitting around for a while. Is that something
that might influence how we want to handle this intent?

[meacer, 2016-08-10 17:24:01]

On 2016/08/10 16:42:37, Avi wrote:
> Where do we stand with the intent, etc? Mustafa, you said you would take this.
> 
> This is a security bug that's been sitting around for a while. Is that
something
> that might influence how we want to handle this intent?

Hmm, I was waiting for this thread to conclude. I can send out one this week.
This being a security bug will be a strong case for the intent.

[Avi (use Gerrit), 2016-10-25 20:18:54]

On 2016/08/10 17:24:01, Mustafa Emre Acer wrote:
> On 2016/08/10 16:42:37, Avi wrote:
> > Where do we stand with the intent, etc? Mustafa, you said you would take
this.
> > 
> > This is a security bug that's been sitting around for a while. Is that
> something
> > that might influence how we want to handle this intent?
> 
> Hmm, I was waiting for this thread to conclude. I can send out one this week.
> This being a security bug will be a strong case for the intent.

This isn't going to happen; closing.