Chromium Code Reviews
Help | Chromium Project | Gerrit Changes | Sign in
(62)

Issue 2194243002: Suppress JavaScript dialogs when a tab puts itself into fullscreen. (Closed)

Created:
2 years, 7 months ago by Avi (use Gerrit)
Modified:
2 years, 4 months ago
Reviewers:
jam, Matt Giuca, meacer
CC:
chromium-reviews, extensions-reviews_chromium.org, cbentzel+watch_chromium.org, creis+watch_chromium.org, tburkard+watch_chromium.org, tfarina, nasko+codewatch_chromium.org, jam, gavinp+prer_chromium.org, darin-cc_chromium.org, chromium-apps-reviews_chromium.org, miu+watch_chromium.org
Base URL:
https://chromium.googlesource.com/chromium/src.git@master
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

Suppress JavaScript dialogs when a tab puts itself into fullscreen. BUG=550017 TEST=as in bug

Patch Set 1 #

Patch Set 2 : content:: #

Total comments: 8

Patch Set 3 : switch fullscreen call #

Unified diffs Side-by-side diffs Delta from patch set Stats (+44 lines, -24 lines) Patch
M apps/custom_launcher_page_contents.h View 1 chunk +2 lines, -1 line 0 comments Download
M apps/custom_launcher_page_contents.cc View 1 chunk +2 lines, -1 line 0 comments Download
M chrome/browser/background/background_contents.h View 1 chunk +2 lines, -1 line 0 comments Download
M chrome/browser/background/background_contents.cc View 1 chunk +2 lines, -1 line 0 comments Download
M chrome/browser/extensions/api/tab_capture/offscreen_tab.h View 1 chunk +2 lines, -1 line 0 comments Download
M chrome/browser/extensions/api/tab_capture/offscreen_tab.cc View 1 chunk +2 lines, -1 line 0 comments Download
M chrome/browser/prerender/prerender_contents.cc View 1 chunk +1 line, -1 line 0 comments Download
M chrome/browser/prerender/prerender_manager.cc View 1 chunk +1 line, -1 line 0 comments Download
M chrome/browser/ui/browser.h View 1 1 chunk +2 lines, -0 lines 0 comments Download
M chrome/browser/ui/browser.cc View 1 2 1 chunk +10 lines, -0 lines 0 comments Download
M chrome/browser/ui/fast_unload_controller.cc View 1 chunk +2 lines, -1 line 0 comments Download
M content/browser/web_contents/web_contents_impl.cc View 1 2 2 chunks +7 lines, -9 lines 0 comments Download
M content/public/browser/web_contents_delegate.h View 1 chunk +4 lines, -3 lines 0 comments Download
M content/public/browser/web_contents_delegate.cc View 1 chunk +2 lines, -1 line 0 comments Download
M extensions/browser/app_window/app_window.h View 1 chunk +2 lines, -1 line 0 comments Download
M extensions/browser/app_window/app_window.cc View 1 chunk +1 line, -1 line 0 comments Download

Messages

Total messages: 37 (14 generated)
Avi (use Gerrit)
meacer, mgiuca: dialogs jam: the content change and associated random files
2 years, 7 months ago (2016-08-01 18:06:54 UTC) #10
meacer
LGTM. Was there a discussion of this change outside the associated bug? I'm not sure ...
2 years, 7 months ago (2016-08-01 18:23:26 UTC) #11
Avi (use Gerrit)
On 2016/08/01 18:23:26, Mustafa Emre Acer wrote: > Was there a discussion of this change ...
2 years, 7 months ago (2016-08-01 18:27:49 UTC) #12
meacer
On 2016/08/01 18:27:49, Avi wrote: > On 2016/08/01 18:23:26, Mustafa Emre Acer wrote: > > ...
2 years, 7 months ago (2016-08-01 18:37:12 UTC) #13
Avi (use Gerrit)
On 2016/08/01 18:37:12, Mustafa Emre Acer wrote: > On 2016/08/01 18:27:49, Avi wrote: > > ...
2 years, 7 months ago (2016-08-01 18:59:23 UTC) #14
Matt Giuca
code lgtm (very neat, thankyou for doing this, I wouldn't have known to use this ...
2 years, 7 months ago (2016-08-02 01:42:10 UTC) #15
Matt Giuca
https://codereview.chromium.org/2194243002/diff/20001/chrome/browser/ui/browser.cc File chrome/browser/ui/browser.cc (right): https://codereview.chromium.org/2194243002/diff/20001/chrome/browser/ui/browser.cc#newcode1213 chrome/browser/ui/browser.cc:1213: return exclusive_access_manager_->fullscreen_controller() optionally add a comment: // Do not ...
2 years, 7 months ago (2016-08-02 01:42:21 UTC) #16
Avi (use Gerrit)
On 2016/08/02 01:42:10, Matt Giuca wrote: > So we are within spec if we do ...
2 years, 7 months ago (2016-08-02 01:53:07 UTC) #17
Matt Giuca
On 2016/08/02 01:53:07, Avi wrote: > On 2016/08/02 01:42:10, Matt Giuca wrote: > > So ...
2 years, 7 months ago (2016-08-02 02:56:05 UTC) #18
Matt Giuca
not lgtm (to cancel my previous lg); I thought some more about it and I ...
2 years, 7 months ago (2016-08-02 03:34:56 UTC) #19
miu
Perhaps crbug.com/456 is relevant here? Personally, I would love to see modal dialogs from web ...
2 years, 7 months ago (2016-08-02 03:53:16 UTC) #20
Avi (use Gerrit)
https://codereview.chromium.org/2194243002/diff/20001/chrome/browser/ui/browser.cc File chrome/browser/ui/browser.cc (right): https://codereview.chromium.org/2194243002/diff/20001/chrome/browser/ui/browser.cc#newcode1213 chrome/browser/ui/browser.cc:1213: return exclusive_access_manager_->fullscreen_controller() On 2016/08/02 01:42:20, Matt Giuca wrote: > ...
2 years, 7 months ago (2016-08-02 03:53:31 UTC) #21
Matt Giuca
https://codereview.chromium.org/2194243002/diff/20001/chrome/browser/ui/browser.cc File chrome/browser/ui/browser.cc (right): https://codereview.chromium.org/2194243002/diff/20001/chrome/browser/ui/browser.cc#newcode1213 chrome/browser/ui/browser.cc:1213: return exclusive_access_manager_->fullscreen_controller() On 2016/08/02 03:53:30, Avi wrote: > On ...
2 years, 7 months ago (2016-08-02 04:23:50 UTC) #22
Avi (use Gerrit)
https://codereview.chromium.org/2194243002/diff/20001/chrome/browser/ui/browser.cc File chrome/browser/ui/browser.cc (right): https://codereview.chromium.org/2194243002/diff/20001/chrome/browser/ui/browser.cc#newcode1214 chrome/browser/ui/browser.cc:1214: ->IsControllerInitiatedFullscreen(); On 2016/08/02 04:23:50, Matt Giuca wrote: > On ...
2 years, 7 months ago (2016-08-02 04:27:54 UTC) #23
Matt Giuca
On 2016/08/02 04:27:54, Avi wrote: > https://codereview.chromium.org/2194243002/diff/20001/chrome/browser/ui/browser.cc > File chrome/browser/ui/browser.cc (right): > > https://codereview.chromium.org/2194243002/diff/20001/chrome/browser/ui/browser.cc#newcode1214 > ...
2 years, 7 months ago (2016-08-02 04:28:34 UTC) #24
Matt Giuca
https://codereview.chromium.org/2194243002/diff/20001/chrome/browser/ui/browser.cc File chrome/browser/ui/browser.cc (right): https://codereview.chromium.org/2194243002/diff/20001/chrome/browser/ui/browser.cc#newcode1214 chrome/browser/ui/browser.cc:1214: ->IsControllerInitiatedFullscreen(); On 2016/08/02 04:27:54, Avi wrote: > On 2016/08/02 ...
2 years, 7 months ago (2016-08-02 04:28:52 UTC) #25
Avi (use Gerrit)
Matt, PTAL.
2 years, 7 months ago (2016-08-02 17:41:59 UTC) #31
Matt Giuca
lgtm. Thanks for doing this!
2 years, 7 months ago (2016-08-03 01:29:24 UTC) #32
Avi (use Gerrit)
On 2016/08/03 01:29:24, Matt Giuca wrote: > lgtm. Thanks for doing this! Are we doing ...
2 years, 7 months ago (2016-08-03 03:53:18 UTC) #33
Matt Giuca
On 2016/08/03 03:53:18, Avi wrote: > On 2016/08/03 01:29:24, Matt Giuca wrote: > > lgtm. ...
2 years, 7 months ago (2016-08-03 04:23:23 UTC) #34
Avi (use Gerrit)
Where do we stand with the intent, etc? Mustafa, you said you would take this. ...
2 years, 7 months ago (2016-08-10 16:42:37 UTC) #35
meacer
On 2016/08/10 16:42:37, Avi wrote: > Where do we stand with the intent, etc? Mustafa, ...
2 years, 7 months ago (2016-08-10 17:24:01 UTC) #36
Avi (use Gerrit)
2 years, 4 months ago (2016-10-25 20:18:54 UTC) #37
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.

Powered by Google App Engine
This is Rietveld 408576698