Refactor away the Browser* dependency in exclusive_access

Refactor away the Browser* dependency in exclusive_access

This change is needed to support the use of the exclusive
access bubble from extensions. There are two scenarios
under which the bubble will be used to:
- Display the bubble when keyboard lock is in effect.
- Display the fullscreen/mouse lock UI from Hosted app (not
  hooked up in this CL).

Exclusive access controllers such as fullscreen and mouse
lock are updated to no longer access Browser* object
directly, but instead use new ExclusiveAccessContext and
 ExclusiveAccessBubbleViewsContext interfaces. These are
implemented by BrowserView and will in the future be
implemented by NativeAppWindow.

Committed: https://crrev.com/39b6b787e7c4cbcecf1c9be40e9f47b76cd3c14b
Cr-Commit-Position: refs/heads/master@{#318965}

[Sriram, 2015-01-31 00:23:19]

sriramsr@chromium.org changed reviewers:
+ miu@chromium.org, scheib@chromium.org, sky@chromium.org

[chromium-reviews, 2015-02-03 01:37:09]

Ping.
On Jan 30, 2015 4:25 PM, <sriramsr@chromium.org> wrote:

> Reviewers: miu, scheib OOO until Feb 6th, sky,
>
> Description:
> Refactor Browser* dependency from exclusive_access.
>
> Refactoring so hide Browser* dependency in exclusive_access* so that
> exclusive
> access bubble can be used from AppWindow.
>
> Sky for owner review for all except exclusive_access/*
>
>
> BUG=166928
>
> Please review this at https://codereview.chromium.org/877413004/
>
> Base URL: https://chromium.googlesource.com/chromium/src.git@master
>
> Affected files (+507, -168 lines):
>   M chrome/browser/ui/browser.cc
>   M chrome/browser/ui/cocoa/browser_window_controller_private.mm
>   M chrome/browser/ui/cocoa/exclusive_access_bubble_window_controller.h
>   M chrome/browser/ui/cocoa/exclusive_access_bubble_window_controller.mm
>   M chrome/browser/ui/cocoa/exclusive_access_bubble_window_
> controller_unittest.mm
>   M chrome/browser/ui/exclusive_access/exclusive_access_bubble.h
>   M chrome/browser/ui/exclusive_access/exclusive_access_bubble.cc
>   A chrome/browser/ui/exclusive_access/exclusive_access_context.h
>   A chrome/browser/ui/exclusive_access/exclusive_access_context_browser.h
>   A chrome/browser/ui/exclusive_access/exclusive_access_context_browser.cc
>   M chrome/browser/ui/exclusive_access/exclusive_access_controller_base.h
>   M chrome/browser/ui/exclusive_access/exclusive_access_controller_base.cc
>   M chrome/browser/ui/exclusive_access/exclusive_access_manager.h
>   M chrome/browser/ui/exclusive_access/exclusive_access_manager.cc
>   M chrome/browser/ui/exclusive_access/fullscreen_controller.h
>   M chrome/browser/ui/exclusive_access/fullscreen_controller.cc
>   M chrome/browser/ui/exclusive_access/mouse_lock_controller.h
>   M chrome/browser/ui/exclusive_access/mouse_lock_controller.cc
>   M chrome/browser/ui/views/exclusive_access_bubble_views.h
>   M chrome/browser/ui/views/exclusive_access_bubble_views.cc
>   A chrome/browser/ui/views/exclusive_access_bubble_views_context.h
>   A chrome/browser/ui/views/exclusive_access_bubble_views_
> context_browser.h
>   A chrome/browser/ui/views/exclusive_access_bubble_views_
> context_browser.cc
>   M chrome/browser/ui/views/frame/browser_view.cc
>   M chrome/chrome_browser_ui.gypi
>
>
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[miu, 2015-02-03 05:09:39]

I like the idea of accessing Browser or AppWindow via ExclusiveAccessManager,
but I don't see the need for the separate ExclusiveAccessContextBrowser
class/object.  It adds all kinds of extra indirection throughout the code.  Can
you just merge ExclusiveAccessContextBrowser into ExclusiveAccessManager?  For
the AppWindow case, you could provide a second constructor in
ExclusiveAccessManager that constructs for an AppWindow* instead of a Browser*. 
Alternatively, perhaps you should define a common interface that both Browser
and AppWindow implement?  Also, is there a DEPS issue: Meaning, can code in
src/extensions/browser depend on code in src/chrome/browser/ui?

Let's chat about this tomorrow.

[scheib, 2015-02-10 00:16:43]

Some level of indirection is warranted. This patch's ExclusiveAccessContext
seems fine, but an interface that Browser and AppWindow implement seems cleaner.

Either would need to be located at a lower level that both can access. Likely in
src/ui/, possibly src/ui/base. Please discuss with sky@.

By the time we move the interface to a lower level, it seems likely that we
should move more of chrome/browser/ui/exclusive_access. However fullscreen and
mouse_lock code there is only used currently by chrome/browser.

[scheib, 2015-02-10 23:12:03]

I think you should put this patch on hold until you have a larger patch that
builds up what is necessary for keyboard lock in AppWindow and Browser. 

I made some comments inline noticing some issues with separating out concerns.
The fullscreen and mouse lock are I think fairly tied to Profile which will be
an issue. And it doesn't make sense to have AppWindow deal with these fullscreen
concerns as it's not using the fullscreen controller.

https://codereview.chromium.org/877413004/diff/40001/chrome/browser/ui/exclus...
File chrome/browser/ui/exclusive_access/exclusive_access_context.h (right):

https://codereview.chromium.org/877413004/diff/40001/chrome/browser/ui/exclus...
chrome/browser/ui/exclusive_access/exclusive_access_context.h:19: class
ExclusiveAccessContext {
I think I'm preferring the Delegate pattern where this interface is used as a
base class for Browser and AppWindow instead of having
ExclusiveAccessContextBrowser adapters.

https://codereview.chromium.org/877413004/diff/40001/chrome/browser/ui/exclus...
chrome/browser/ui/exclusive_access/exclusive_access_context.h:24: virtual
Profile* GetProfile() = 0;
Profile* won't work in extensions, it will need to be BrowserContext. But that
doesn't have GetHostContentSettingsmap or GetPrefs.

https://codereview.chromium.org/877413004/diff/40001/chrome/browser/ui/exclus...
chrome/browser/ui/exclusive_access/exclusive_access_context.h:28: virtual bool
IsFullscreen() = 0;
Most of these methods so far are Fullscreen related. Those won't be used in
AppWindow AFAIK because ExclusiveAccessController isn't used there at all.

What's the plan with keyboard? Will members such as this be necessary for
keyboard?

It appears no methods are specific to mouse lock.

So then I wonder if either the fullscreen related bits should be pulled out to
another interface and just not implemented at all by AppWindow, or if all these
methods should have default impl with NOT_IMPLEMENTED.

[Sriram, 2015-02-10 23:44:39]

But I cannot use it in AppWindow unless I move the code to component which
should not depend on Browser... so will have to get this in and move it before I
can actually use it.

Also Ben mentioned that he had a scenario to use FullscreenController from
AppWindow too but I am not sure of the scenario. So I wanted to make sure that
he can hook it up if needed (i.e. implement the objects if needed).

On 2015/02/10 23:12:03, scheib wrote:
> I think you should put this patch on hold until you have a larger patch that
> builds up what is necessary for keyboard lock in AppWindow and Browser. 
> 
> I made some comments inline noticing some issues with separating out concerns.
> The fullscreen and mouse lock are I think fairly tied to Profile which will be
> an issue. And it doesn't make sense to have AppWindow deal with these
fullscreen
> concerns as it's not using the fullscreen controller.
> 
>
https://codereview.chromium.org/877413004/diff/40001/chrome/browser/ui/exclus...
> File chrome/browser/ui/exclusive_access/exclusive_access_context.h (right):
> 
>
https://codereview.chromium.org/877413004/diff/40001/chrome/browser/ui/exclus...
> chrome/browser/ui/exclusive_access/exclusive_access_context.h:19: class
> ExclusiveAccessContext {
> I think I'm preferring the Delegate pattern where this interface is used as a
> base class for Browser and AppWindow instead of having
> ExclusiveAccessContextBrowser adapters.
> 
>
https://codereview.chromium.org/877413004/diff/40001/chrome/browser/ui/exclus...
> chrome/browser/ui/exclusive_access/exclusive_access_context.h:24: virtual
> Profile* GetProfile() = 0;
> Profile* won't work in extensions, it will need to be BrowserContext. But that
> doesn't have GetHostContentSettingsmap or GetPrefs.
> 
>
https://codereview.chromium.org/877413004/diff/40001/chrome/browser/ui/exclus...
> chrome/browser/ui/exclusive_access/exclusive_access_context.h:28: virtual bool
> IsFullscreen() = 0;
> Most of these methods so far are Fullscreen related. Those won't be used in
> AppWindow AFAIK because ExclusiveAccessController isn't used there at all.
> 
> What's the plan with keyboard? Will members such as this be necessary for
> keyboard?
> 
> It appears no methods are specific to mouse lock.
> 
> So then I wonder if either the fullscreen related bits should be pulled out to
> another interface and just not implemented at all by AppWindow, or if all
these
> methods should have default impl with NOT_IMPLEMENTED.

[Sriram, 2015-02-10 23:44:46]

https://codereview.chromium.org/877413004/diff/40001/chrome/browser/ui/exclus...
File chrome/browser/ui/exclusive_access/exclusive_access_context.h (right):

https://codereview.chromium.org/877413004/diff/40001/chrome/browser/ui/exclus...
chrome/browser/ui/exclusive_access/exclusive_access_context.h:19: class
ExclusiveAccessContext {
Yuri had same suggestion. It would be BrowserWindow and AppWindow. I will do
that.

On 2015/02/10 23:12:03, scheib wrote:
> I think I'm preferring the Delegate pattern where this interface is used as a
> base class for Browser and AppWindow instead of having
> ExclusiveAccessContextBrowser adapters.

https://codereview.chromium.org/877413004/diff/40001/chrome/browser/ui/exclus...
chrome/browser/ui/exclusive_access/exclusive_access_context.h:24: virtual
Profile* GetProfile() = 0;
I thought I could go from BrowserContext to Profile using
Profile::FromBrowserContext() but that was before I realized that AppWindow
cannot depend on chrome code. Ben? 

On 2015/02/10 23:12:03, scheib wrote:
> Profile* won't work in extensions, it will need to be BrowserContext. But that
> doesn't have GetHostContentSettingsmap or GetPrefs.

https://codereview.chromium.org/877413004/diff/40001/chrome/browser/ui/exclus...
chrome/browser/ui/exclusive_access/exclusive_access_context.h:28: virtual bool
IsFullscreen() = 0;
I was planning to use ExclusiveAccessController as such but not hook-up calls
needed to actually enable fullscreen/mouselock controller. There was a some
scenarios under which AppWindow wanted to use them (once again Ben?), and when
they are hooked up it will behave correctly with keyboard lock. 

On 2015/02/10 23:12:03, scheib wrote:
> Most of these methods so far are Fullscreen related. Those won't be used in
> AppWindow AFAIK because ExclusiveAccessController isn't used there at all.
> 
> What's the plan with keyboard? Will members such as this be necessary for
> keyboard?
> 
> It appears no methods are specific to mouse lock.
> 
> So then I wonder if either the fullscreen related bits should be pulled out to
> another interface and just not implemented at all by AppWindow, or if all
these
> methods should have default impl with NOT_IMPLEMENTED.

[scheib, 2015-02-11 00:05:55]

On 2015/02/10 23:44:39, Sriram wrote:
> But I cannot use it in AppWindow unless I move the code to component which
> should not depend on Browser... so will have to get this in and move it before
I
> can actually use it.
> 

Create a larger patch that does the move and uses it in AppWindow. That patch is
to sort out all the details. When landing please keep them split up. This is
often referred to as 'pipelining'. If you haven't had multiple dependent patches
in progress before I suggest asking office mates to help with the skill. I can
help remotely if needed, it's just more communication work.

https://codereview.chromium.org/877413004/diff/40001/chrome/browser/ui/exclus...
File chrome/browser/ui/exclusive_access/exclusive_access_context.h (right):

https://codereview.chromium.org/877413004/diff/40001/chrome/browser/ui/exclus...
chrome/browser/ui/exclusive_access/exclusive_access_context.h:24: virtual
Profile* GetProfile() = 0;
On 2015/02/10 23:44:46, Sriram wrote:
> I thought I could go from BrowserContext to Profile using
> Profile::FromBrowserContext() but that was before I realized that AppWindow
> cannot depend on chrome code. Ben? 
> 
> On 2015/02/10 23:12:03, scheib wrote:
> > Profile* won't work in extensions, it will need to be BrowserContext. But
that
> > doesn't have GetHostContentSettingsmap or GetPrefs.
> 

Ah, yes, so long as the code accessing is in chrome, you can get back to
Profile. This implies that the EACFullscreen and MouseLock classes remain in
chrome/browser

[miu, 2015-02-11 00:36:59]

On 2015/02/11 00:05:55, scheib wrote:
> On 2015/02/10 23:44:39, Sriram wrote:
> Create a larger patch that does the move and uses it in AppWindow. That patch
is
> to sort out all the details. When landing please keep them split up. This is
> often referred to as 'pipelining'. If you haven't had multiple dependent
patches
> in progress before I suggest asking office mates to help with the skill. I can
> help remotely if needed, it's just more communication work.

+1.  Others may have better strategies, but I have gotten REALLY good at
hand-editing diffs (with the help of emacs diff-mode).  Essentially, you're
prototyping all the way to the end goal, to work out ALL the potential design
issues.  Then, it's a lot easier to go through each code review and explain why
things are arranged/designed a certain way.

[Sriram, 2015-02-25 00:41:23]

sriramsr@chromium.org changed reviewers:
+ benwells@chromium.org

[Sriram, 2015-02-25 00:41:23]

Turns out that I needed the exit bubble/controller from
chrome/browser/ui/views/apps and do there was no dependency issue. I hacked up
to use mouse controller UI flow to handle keyboard lock from web app and it
worked fine. Here are the CLs if you are interested:

Integration with extension: https://codereview.chromium.org/949553002/

The full CL with some hacks for quick testing (including chromoting webapp
change):
https://codereview.chromium.org/944123003/

https://codereview.chromium.org/877413004/diff/40001/chrome/browser/ui/exclus...
File chrome/browser/ui/exclusive_access/exclusive_access_context.h (right):

https://codereview.chromium.org/877413004/diff/40001/chrome/browser/ui/exclus...
chrome/browser/ui/exclusive_access/exclusive_access_context.h:24: virtual
Profile* GetProfile() = 0;
On 2015/02/11 00:05:55, scheib wrote:
> On 2015/02/10 23:44:46, Sriram wrote:
> > I thought I could go from BrowserContext to Profile using
> > Profile::FromBrowserContext() but that was before I realized that AppWindow
> > cannot depend on chrome code. Ben? 
> > 
> > On 2015/02/10 23:12:03, scheib wrote:
> > > Profile* won't work in extensions, it will need to be BrowserContext. But
> that
> > > doesn't have GetHostContentSettingsmap or GetPrefs.
> > 
> 
> Ah, yes, so long as the code accessing is in chrome, you can get back to
> Profile. This implies that the EACFullscreen and MouseLock classes remain in
> chrome/browser

Acknowledged.

[Sriram, 2015-02-25 00:51:08]

This is the CL that is to be reviewed and others are just to show end-to-end
usage (but hacked up).


On 2015/02/25 00:41:23, Sriram wrote:
> Turns out that I needed the exit bubble/controller from
> chrome/browser/ui/views/apps and do there was no dependency issue. I hacked up
> to use mouse controller UI flow to handle keyboard lock from web app and it
> worked fine. Here are the CLs if you are interested:
> 
> Integration with extension: https://codereview.chromium.org/949553002/
> 
> The full CL with some hacks for quick testing (including chromoting webapp
> change):
> https://codereview.chromium.org/944123003/
> 
>
https://codereview.chromium.org/877413004/diff/40001/chrome/browser/ui/exclus...
> File chrome/browser/ui/exclusive_access/exclusive_access_context.h (right):
> 
>
https://codereview.chromium.org/877413004/diff/40001/chrome/browser/ui/exclus...
> chrome/browser/ui/exclusive_access/exclusive_access_context.h:24: virtual
> Profile* GetProfile() = 0;
> On 2015/02/11 00:05:55, scheib wrote:
> > On 2015/02/10 23:44:46, Sriram wrote:
> > > I thought I could go from BrowserContext to Profile using
> > > Profile::FromBrowserContext() but that was before I realized that
AppWindow
> > > cannot depend on chrome code. Ben? 
> > > 
> > > On 2015/02/10 23:12:03, scheib wrote:
> > > > Profile* won't work in extensions, it will need to be BrowserContext.
But
> > that
> > > > doesn't have GetHostContentSettingsmap or GetPrefs.
> > > 
> > 
> > Ah, yes, so long as the code accessing is in chrome, you can get back to
> > Profile. This implies that the EACFullscreen and MouseLock classes remain in
> > chrome/browser
> 
> Acknowledged.

[Sriram, 2015-02-25 17:55:01]

On 2015/02/25 00:52:16, Sriram wrote:

Update since last review:
1. Removed separate objects that where implementing the new interfaces.
2. Side effect of 1 is use of raw pointer instead of scoped_ptr.
3. Removed reference to sky from description but his LGTM is still needed for
OWNERS.

[scheib, 2015-02-25 19:30:13]

In change description:

> This change is needed to support the use of the exclusive 
> access bubble from extensions. There are two scenarios 
> under which the bubble will be used:
> - Display when keyboard lock is in effect.
> - Hosted app

What does "Hosted app" mean? Does it mean keyboard lock from hosted app?

> 
> Exclusive access controllers like fullscreen and mouse lock 
> controller uses Browser* object directly to determine the 
> state of the window and also control the exit bubble. Added 
> a new interface ExclusiveAccessContext to hide this 
> interaction. This interface is implemented by BrowserView. 

This paragraph is mixing previous and post state of change.

reword to describe what this change does and the state afterwards. If referring
to previous state make it clear that it is past tense.
e.g.:

Exclusive access controllers such as fullscreen and mouse lock
are updated to no longer access Browser* object directly, but instead use new
ExclusiveAccessContext and ExclusiveAccessBubbleViewsContext interfaces. These
are implemented by BrowserView and will in the future be implemented by
NativeAppWindow.

https://codereview.chromium.org/877413004/diff/80001/chrome/browser/ui/exclus...
File chrome/browser/ui/exclusive_access/exclusive_access_bubble.cc (right):

https://codereview.chromium.org/877413004/diff/80001/chrome/browser/ui/exclus...
chrome/browser/ui/exclusive_access/exclusive_access_bubble.cc:117: void
ExclusiveAccessBubble::ToggleFullscreen() {
While here, would you fix this too please: Rename
ExclusiveAccessBubble::ToggleFullscreen ->
ExclusiveAccessBubble::ExitExclusiveAccess

https://codereview.chromium.org/877413004/diff/80001/chrome/browser/ui/exclus...
File chrome/browser/ui/exclusive_access/exclusive_access_context.h (right):

https://codereview.chromium.org/877413004/diff/80001/chrome/browser/ui/exclus...
chrome/browser/ui/exclusive_access/exclusive_access_context.h:30: // Returns
true if the window host the exclusive access bubble is fullscreen.
This comment needs fixing. BrowserWindow::IsFullscreenWithToolbar documents
generally what this is, though even that could be clearer. The other comments in
this interface also reference this concept a bit. I suggest:

Returns true if the window is fullscreen with additional UI elements. See
EnterFullscreen |with_toolbar|.

https://codereview.chromium.org/877413004/diff/80001/chrome/browser/ui/exclus...
chrome/browser/ui/exclusive_access/exclusive_access_context.h:33: // Methods
that change fullscreen state.
"Methods that change fullscreen state" -> "Enter fullscreen and update exit
bubble."

https://codereview.chromium.org/877413004/diff/80001/chrome/browser/ui/exclus...
chrome/browser/ui/exclusive_access/exclusive_access_context.h:52: virtual bool
SupportsFullscreenWithToolbar() const;
Move Supports____ and Update___ just above IsFullscreenWithToolbar() (and move
all impl files to maintain consistent ordering...)

https://codereview.chromium.org/877413004/diff/80001/chrome/browser/ui/exclus...
chrome/browser/ui/exclusive_access/exclusive_access_context.h:59: // Sets state
for entering or exiting Win8 Metro snap mode.
Wrap in #if defined(OS_WIN) as is done elsewhere to make it clear this is only
Windows.

https://codereview.chromium.org/877413004/diff/80001/chrome/browser/ui/exclus...
chrome/browser/ui/exclusive_access/exclusive_access_context.h:66: virtual void
UpdateDownloadShelf(bool unhide);
The naming is and readability suffers (have to look at declaration to understand
what Update(false) means. Split to two methods
HideDownloadShelf
UnhideDownloadShelf

https://codereview.chromium.org/877413004/diff/80001/chrome/browser/ui/exclus...
File chrome/browser/ui/exclusive_access/fullscreen_controller.cc (right):

https://codereview.chromium.org/877413004/diff/80001/chrome/browser/ui/exclus...
chrome/browser/ui/exclusive_access/fullscreen_controller.cc:355: // Do not store
preference on file:// URLs, they don't have a clean
Leave the blank line.

https://codereview.chromium.org/877413004/diff/80001/chrome/browser/ui/views/...
File chrome/browser/ui/views/exclusive_access_bubble_views.cc (right):

https://codereview.chromium.org/877413004/diff/80001/chrome/browser/ui/views/...
chrome/browser/ui/views/exclusive_access_bubble_views.cc:275: &accelerator);
Why drop the DCHECK?

[Sriram, 2015-02-25 21:53:27]

https://codereview.chromium.org/877413004/diff/80001/chrome/browser/ui/exclus...
File chrome/browser/ui/exclusive_access/exclusive_access_bubble.cc (right):

https://codereview.chromium.org/877413004/diff/80001/chrome/browser/ui/exclus...
chrome/browser/ui/exclusive_access/exclusive_access_bubble.cc:117: void
ExclusiveAccessBubble::ToggleFullscreen() {
On 2015/02/25 19:30:13, scheib wrote:
> While here, would you fix this too please: Rename
> ExclusiveAccessBubble::ToggleFullscreen ->
> ExclusiveAccessBubble::ExitExclusiveAccess

Actually this is explicitly dealing with fullscreen and not exiting exclusive
access so leaving this one as such.

https://codereview.chromium.org/877413004/diff/80001/chrome/browser/ui/exclus...
File chrome/browser/ui/exclusive_access/exclusive_access_context.h (right):

https://codereview.chromium.org/877413004/diff/80001/chrome/browser/ui/exclus...
chrome/browser/ui/exclusive_access/exclusive_access_context.h:30: // Returns
true if the window host the exclusive access bubble is fullscreen.
On 2015/02/25 19:30:13, scheib wrote:
> This comment needs fixing. BrowserWindow::IsFullscreenWithToolbar documents
> generally what this is, though even that could be clearer. The other comments
in
> this interface also reference this concept a bit. I suggest:
> 
> Returns true if the window is fullscreen with additional UI elements. See
> EnterFullscreen |with_toolbar|.

Done.

https://codereview.chromium.org/877413004/diff/80001/chrome/browser/ui/exclus...
chrome/browser/ui/exclusive_access/exclusive_access_context.h:33: // Methods
that change fullscreen state.
On 2015/02/25 19:30:13, scheib wrote:
> "Methods that change fullscreen state" -> "Enter fullscreen and update exit
> bubble."

Done.

https://codereview.chromium.org/877413004/diff/80001/chrome/browser/ui/exclus...
chrome/browser/ui/exclusive_access/exclusive_access_context.h:52: virtual bool
SupportsFullscreenWithToolbar() const;
On 2015/02/25 19:30:13, scheib wrote:
> Move Supports____ and Update___ just above IsFullscreenWithToolbar() (and move
> all impl files to maintain consistent ordering...)

Done.

https://codereview.chromium.org/877413004/diff/80001/chrome/browser/ui/exclus...
chrome/browser/ui/exclusive_access/exclusive_access_context.h:59: // Sets state
for entering or exiting Win8 Metro snap mode.
On 2015/02/25 19:30:13, scheib wrote:
> Wrap in #if defined(OS_WIN) as is done elsewhere to make it clear this is only
> Windows.

Done.

https://codereview.chromium.org/877413004/diff/80001/chrome/browser/ui/exclus...
chrome/browser/ui/exclusive_access/exclusive_access_context.h:66: virtual void
UpdateDownloadShelf(bool unhide);
On 2015/02/25 19:30:13, scheib wrote:
> The naming is and readability suffers (have to look at declaration to
understand
> what Update(false) means. Split to two methods
> HideDownloadShelf
> UnhideDownloadShelf

Done.

https://codereview.chromium.org/877413004/diff/80001/chrome/browser/ui/exclus...
File chrome/browser/ui/exclusive_access/fullscreen_controller.cc (right):

https://codereview.chromium.org/877413004/diff/80001/chrome/browser/ui/exclus...
chrome/browser/ui/exclusive_access/fullscreen_controller.cc:355: // Do not store
preference on file:// URLs, they don't have a clean
On 2015/02/25 19:30:13, scheib wrote:
> Leave the blank line.

Done.

https://codereview.chromium.org/877413004/diff/80001/chrome/browser/ui/views/...
File chrome/browser/ui/views/exclusive_access_bubble_views.cc (right):

https://codereview.chromium.org/877413004/diff/80001/chrome/browser/ui/views/...
chrome/browser/ui/views/exclusive_access_bubble_views.cc:275: &accelerator);
On 2015/02/25 19:30:13, scheib wrote:
> Why drop the DCHECK?

Updated the comments to reflect this. There is no accelerator for packaged app
to exit fullscreen.

[scheib, 2015-02-25 22:31:51]

lgtm with a few changes:
Fix ToggleFullscreen() as per comment.
Fix the change description line wrapping.

https://codereview.chromium.org/877413004/diff/80001/chrome/browser/ui/exclus...
File chrome/browser/ui/exclusive_access/exclusive_access_bubble.cc (right):

https://codereview.chromium.org/877413004/diff/80001/chrome/browser/ui/exclus...
chrome/browser/ui/exclusive_access/exclusive_access_bubble.cc:117: void
ExclusiveAccessBubble::ToggleFullscreen() {
On 2015/02/25 21:53:27, Sriram wrote:
> On 2015/02/25 19:30:13, scheib wrote:
> > While here, would you fix this too please: Rename
> > ExclusiveAccessBubble::ToggleFullscreen ->
> > ExclusiveAccessBubble::ExitExclusiveAccess
> 
> Actually this is explicitly dealing with fullscreen and not exiting exclusive
> access so leaving this one as such.

It shouldn't be "Toggle", it is definitely not Toggling.

The only impl is Fullscreen now, but for keyboard lock without pointer lock
won't we want it to exit keyboard lock?

At this point in code I think the abstracted concept ExitExclusiveAccess is
fitting.

https://codereview.chromium.org/877413004/diff/100001/chrome/browser/ui/views...
File chrome/browser/ui/views/exclusive_access_bubble_views.cc (right):

https://codereview.chromium.org/877413004/diff/100001/chrome/browser/ui/views...
chrome/browser/ui/views/exclusive_access_bubble_views.cc:274: // is accelerator
defined to escape out of fullscreen for packaged apps.
Thanks for explanation. You intended "is no accelerator".

Hmm, there is an ESC default unless app.window.fullscreen.overrideEsc is added.
So this comment is sort of true but also confusing.

If you hit the DCHECK with other code in this patch then it's fine to just drop
it without comment in the code, the current comment will be correct "... if it
is defined". 

If this patch isn't hitting the DCHECK, wait until the patch that does hit it to
remove it.

[Sriram, 2015-02-25 23:15:59]

https://codereview.chromium.org/877413004/diff/80001/chrome/browser/ui/exclus...
File chrome/browser/ui/exclusive_access/exclusive_access_bubble.cc (right):

https://codereview.chromium.org/877413004/diff/80001/chrome/browser/ui/exclus...
chrome/browser/ui/exclusive_access/exclusive_access_bubble.cc:117: void
ExclusiveAccessBubble::ToggleFullscreen() {
On 2015/02/25 22:31:51, scheib wrote:
> On 2015/02/25 21:53:27, Sriram wrote:
> > On 2015/02/25 19:30:13, scheib wrote:
> > > While here, would you fix this too please: Rename
> > > ExclusiveAccessBubble::ToggleFullscreen ->
> > > ExclusiveAccessBubble::ExitExclusiveAccess
> > 
> > Actually this is explicitly dealing with fullscreen and not exiting
exclusive
> > access so leaving this one as such.
> 
> It shouldn't be "Toggle", it is definitely not Toggling.
> 
> The only impl is Fullscreen now, but for keyboard lock without pointer lock
> won't we want it to exit keyboard lock?
> 
> At this point in code I think the abstracted concept ExitExclusiveAccess is
> fitting.

Done.

https://codereview.chromium.org/877413004/diff/100001/chrome/browser/ui/views...
File chrome/browser/ui/views/exclusive_access_bubble_views.cc (right):

https://codereview.chromium.org/877413004/diff/100001/chrome/browser/ui/views...
chrome/browser/ui/views/exclusive_access_bubble_views.cc:274: // is accelerator
defined to escape out of fullscreen for packaged apps.
On 2015/02/25 22:31:51, scheib wrote:
> Thanks for explanation. You intended "is no accelerator".
> 
> Hmm, there is an ESC default unless app.window.fullscreen.overrideEsc is
added.
> So this comment is sort of true but also confusing.
> 
> If you hit the DCHECK with other code in this patch then it's fine to just
drop
> it without comment in the code, the current comment will be correct "... if it
> is defined". 
> 
> If this patch isn't hitting the DCHECK, wait until the patch that does hit it
to
> remove it.

This is not hit in current patch but with patch that integrates the change with
apps and running with app that actually overrides ESC. I have reverted this for
now.

[Sriram, 2015-02-25 23:16:38]

On 2015/02/25 23:15:59, Sriram wrote:
>
https://codereview.chromium.org/877413004/diff/80001/chrome/browser/ui/exclus...
> File chrome/browser/ui/exclusive_access/exclusive_access_bubble.cc (right):
> 
>
https://codereview.chromium.org/877413004/diff/80001/chrome/browser/ui/exclus...
> chrome/browser/ui/exclusive_access/exclusive_access_bubble.cc:117: void
> ExclusiveAccessBubble::ToggleFullscreen() {
> On 2015/02/25 22:31:51, scheib wrote:
> > On 2015/02/25 21:53:27, Sriram wrote:
> > > On 2015/02/25 19:30:13, scheib wrote:
> > > > While here, would you fix this too please: Rename
> > > > ExclusiveAccessBubble::ToggleFullscreen ->
> > > > ExclusiveAccessBubble::ExitExclusiveAccess
> > > 
> > > Actually this is explicitly dealing with fullscreen and not exiting
> exclusive
> > > access so leaving this one as such.
> > 
> > It shouldn't be "Toggle", it is definitely not Toggling.
> > 
> > The only impl is Fullscreen now, but for keyboard lock without pointer lock
> > won't we want it to exit keyboard lock?
> > 
> > At this point in code I think the abstracted concept ExitExclusiveAccess is
> > fitting.
> 
> Done.
> 
>
https://codereview.chromium.org/877413004/diff/100001/chrome/browser/ui/views...
> File chrome/browser/ui/views/exclusive_access_bubble_views.cc (right):
> 
>
https://codereview.chromium.org/877413004/diff/100001/chrome/browser/ui/views...
> chrome/browser/ui/views/exclusive_access_bubble_views.cc:274: // is
accelerator
> defined to escape out of fullscreen for packaged apps.
> On 2015/02/25 22:31:51, scheib wrote:
> > Thanks for explanation. You intended "is no accelerator".
> > 
> > Hmm, there is an ESC default unless app.window.fullscreen.overrideEsc is
> added.
> > So this comment is sort of true but also confusing.
> > 
> > If you hit the DCHECK with other code in this patch then it's fine to just
> drop
> > it without comment in the code, the current comment will be correct "... if
it
> > is defined". 
> > 
> > If this patch isn't hitting the DCHECK, wait until the patch that does hit
it
> to
> > remove it.
> 
> This is not hit in current patch but with patch that integrates the change
with
> apps and running with app that actually overrides ESC. I have reverted this
for
> now.

Sky - can you take a look at the CL for OWNERS for everything but
exclusive_access/*? Thanks.

[sky, 2015-02-26 18:29:20]

https://codereview.chromium.org/877413004/diff/120001/chrome/browser/ui/brows...
File chrome/browser/ui/browser_window.h (right):

https://codereview.chromium.org/877413004/diff/120001/chrome/browser/ui/brows...
chrome/browser/ui/browser_window.h:400: virtual ExclusiveAccessContext*
GetExclusiveAccessContext() = 0;
Does BrowserWindowCocoa implement this?

https://codereview.chromium.org/877413004/diff/120001/chrome/browser/ui/views...
File chrome/browser/ui/views/exclusive_access_bubble_views_context.h (right):

https://codereview.chromium.org/877413004/diff/120001/chrome/browser/ui/views...
chrome/browser/ui/views/exclusive_access_bubble_views_context.h:26: virtual
views::Widget* GetWidget() = 0;
The names here are too generic and easily conflict with other things BrowserView
has. Either change the names to be more explicit, of make BrowserView have an
object that implements the interface but does not directly implement it.

[miu, 2015-02-26 20:07:58]

https://codereview.chromium.org/877413004/diff/120001/chrome/browser/ui/exclu...
File chrome/browser/ui/exclusive_access/exclusive_access_bubble.h (right):

https://codereview.chromium.org/877413004/diff/120001/chrome/browser/ui/exclu...
chrome/browser/ui/exclusive_access/exclusive_access_bubble.h:89:
ExclusiveAccessManager* manager_;
nit: ExclusiveAccessManager* const manager_;

Since this member is never re-assigned after construction.

https://codereview.chromium.org/877413004/diff/120001/chrome/browser/ui/exclu...
chrome/browser/ui/exclusive_access/exclusive_access_bubble.h:92: GURL url_;
ditto: const GURL url_;

https://codereview.chromium.org/877413004/diff/120001/chrome/browser/ui/exclu...
chrome/browser/ui/exclusive_access/exclusive_access_bubble.h:95:
ExclusiveAccessBubbleType bubble_type_;
ditto: const EABT bubble_type_;

https://codereview.chromium.org/877413004/diff/120001/chrome/browser/ui/exclu...
File chrome/browser/ui/exclusive_access/exclusive_access_context.h (right):

https://codereview.chromium.org/877413004/diff/120001/chrome/browser/ui/exclu...
chrome/browser/ui/exclusive_access/exclusive_access_context.h:4: #ifndef
CHROME_BROWSER_UI_EXCLUSIVE_ACCESS_EXCLUSIVE_ACCESS_CONTEXT_H_
Please add a newline above this line.

https://codereview.chromium.org/877413004/diff/120001/chrome/browser/ui/exclu...
chrome/browser/ui/exclusive_access/exclusive_access_context.h:37: // Returns
true if the window is fullscreen with additional UI elements. See
Please add a newline above this line.

https://codereview.chromium.org/877413004/diff/120001/chrome/browser/ui/exclu...
chrome/browser/ui/exclusive_access/exclusive_access_context.h:68: virtual void
UnhideDownloadShelf();
The hide/unhide download shelf methods are an abstraction violation.  Meaning,
the concept only applies to the Browser and not a platform app window.  So,
please add a TODO comment here to remove them in a later change.  Ideally, the
BrowserView should hide its download shelf widget when it is instructed to enter
fullscreen mode.

https://codereview.chromium.org/877413004/diff/120001/chrome/browser/ui/exclu...
File chrome/browser/ui/exclusive_access/exclusive_access_manager.h (right):

https://codereview.chromium.org/877413004/diff/120001/chrome/browser/ui/exclu...
chrome/browser/ui/exclusive_access/exclusive_access_manager.h:39:
ExclusiveAccessContext* exclusive_access_context() {
Method should be const.

https://codereview.chromium.org/877413004/diff/120001/chrome/browser/ui/exclu...
chrome/browser/ui/exclusive_access/exclusive_access_manager.h:39:
ExclusiveAccessContext* exclusive_access_context() {
This seems to be causing a ton of 3+ line statements, especially in
fullscreen_controller.cc.  Example:

  WebContents* const active_web_contents = exclusive_access_manager()
                                               ->exclusive_access_context()
                                               ->GetActiveWebContents();

Consider renaming this method to just context() to simplify all code that gets
the ExclusiveAccessContext*.  The above example would become:

  WebContents* const active_web_contents =
      exclusive_access_manager()->context()->GetActiveWebContents();

https://codereview.chromium.org/877413004/diff/120001/chrome/browser/ui/exclu...
chrome/browser/ui/exclusive_access/exclusive_access_manager.h:68:
ExclusiveAccessContext* exclusive_access_context_;
ExclusiveAccessContext* const ...;

https://codereview.chromium.org/877413004/diff/120001/chrome/browser/ui/views...
File chrome/browser/ui/views/exclusive_access_bubble_views.h (right):

https://codereview.chromium.org/877413004/diff/120001/chrome/browser/ui/views...
chrome/browser/ui/views/exclusive_access_bubble_views.h:85:
ExclusiveAccessBubbleViewsContext* bubble_view_context_;
const

[Sriram, 2015-02-26 23:58:03]

https://codereview.chromium.org/877413004/diff/120001/chrome/browser/ui/brows...
File chrome/browser/ui/browser_window.h (right):

https://codereview.chromium.org/877413004/diff/120001/chrome/browser/ui/brows...
chrome/browser/ui/browser_window.h:400: virtual ExclusiveAccessContext*
GetExclusiveAccessContext() = 0;
On 2015/02/26 18:29:20, sky wrote:
> Does BrowserWindowCocoa implement this?

Done.

https://codereview.chromium.org/877413004/diff/120001/chrome/browser/ui/exclu...
File chrome/browser/ui/exclusive_access/exclusive_access_bubble.h (right):

https://codereview.chromium.org/877413004/diff/120001/chrome/browser/ui/exclu...
chrome/browser/ui/exclusive_access/exclusive_access_bubble.h:89:
ExclusiveAccessManager* manager_;
On 2015/02/26 20:07:56, miu wrote:
> nit: ExclusiveAccessManager* const manager_;
> 
> Since this member is never re-assigned after construction.

Done.

https://codereview.chromium.org/877413004/diff/120001/chrome/browser/ui/exclu...
chrome/browser/ui/exclusive_access/exclusive_access_bubble.h:92: GURL url_;
On 2015/02/26 20:07:56, miu wrote:
> ditto: const GURL url_;

This is not changed in this class but is modified from
exclusive_access_bubble_views.

https://codereview.chromium.org/877413004/diff/120001/chrome/browser/ui/exclu...
chrome/browser/ui/exclusive_access/exclusive_access_bubble.h:95:
ExclusiveAccessBubbleType bubble_type_;
On 2015/02/26 20:07:56, miu wrote:
> ditto: const EABT bubble_type_;

Same as url_

https://codereview.chromium.org/877413004/diff/120001/chrome/browser/ui/exclu...
File chrome/browser/ui/exclusive_access/exclusive_access_context.h (right):

https://codereview.chromium.org/877413004/diff/120001/chrome/browser/ui/exclu...
chrome/browser/ui/exclusive_access/exclusive_access_context.h:4: #ifndef
CHROME_BROWSER_UI_EXCLUSIVE_ACCESS_EXCLUSIVE_ACCESS_CONTEXT_H_
On 2015/02/26 20:07:57, miu wrote:
> Please add a newline above this line.

Done.

https://codereview.chromium.org/877413004/diff/120001/chrome/browser/ui/exclu...
chrome/browser/ui/exclusive_access/exclusive_access_context.h:37: // Returns
true if the window is fullscreen with additional UI elements. See
On 2015/02/26 20:07:56, miu wrote:
> Please add a newline above this line.

Done.

https://codereview.chromium.org/877413004/diff/120001/chrome/browser/ui/exclu...
chrome/browser/ui/exclusive_access/exclusive_access_context.h:68: virtual void
UnhideDownloadShelf();
On 2015/02/26 20:07:57, miu wrote:
> The hide/unhide download shelf methods are an abstraction violation.  Meaning,
> the concept only applies to the Browser and not a platform app window.  So,
> please add a TODO comment here to remove them in a later change.  Ideally, the
> BrowserView should hide its download shelf widget when it is instructed to
enter
> fullscreen mode.

Done.

https://codereview.chromium.org/877413004/diff/120001/chrome/browser/ui/exclu...
File chrome/browser/ui/exclusive_access/exclusive_access_manager.h (right):

https://codereview.chromium.org/877413004/diff/120001/chrome/browser/ui/exclu...
chrome/browser/ui/exclusive_access/exclusive_access_manager.h:39:
ExclusiveAccessContext* exclusive_access_context() {
On 2015/02/26 20:07:57, miu wrote:
> This seems to be causing a ton of 3+ line statements, especially in
> fullscreen_controller.cc.  Example:
> 
>   WebContents* const active_web_contents = exclusive_access_manager()
>                                                ->exclusive_access_context()
>                                                ->GetActiveWebContents();
> 
> Consider renaming this method to just context() to simplify all code that gets
> the ExclusiveAccessContext*.  The above example would become:
> 
>   WebContents* const active_web_contents =
>       exclusive_access_manager()->context()->GetActiveWebContents();

Done.

https://codereview.chromium.org/877413004/diff/120001/chrome/browser/ui/exclu...
chrome/browser/ui/exclusive_access/exclusive_access_manager.h:39:
ExclusiveAccessContext* exclusive_access_context() {
On 2015/02/26 20:07:57, miu wrote:
> Method should be const.

Done.

https://codereview.chromium.org/877413004/diff/120001/chrome/browser/ui/exclu...
chrome/browser/ui/exclusive_access/exclusive_access_manager.h:68:
ExclusiveAccessContext* exclusive_access_context_;
On 2015/02/26 20:07:57, miu wrote:
> ExclusiveAccessContext* const ...;

Done.

https://codereview.chromium.org/877413004/diff/120001/chrome/browser/ui/views...
File chrome/browser/ui/views/exclusive_access_bubble_views.h (right):

https://codereview.chromium.org/877413004/diff/120001/chrome/browser/ui/views...
chrome/browser/ui/views/exclusive_access_bubble_views.h:85:
ExclusiveAccessBubbleViewsContext* bubble_view_context_;
On 2015/02/26 20:07:57, miu wrote:
> const

Done.

https://codereview.chromium.org/877413004/diff/120001/chrome/browser/ui/views...
File chrome/browser/ui/views/exclusive_access_bubble_views_context.h (right):

https://codereview.chromium.org/877413004/diff/120001/chrome/browser/ui/views...
chrome/browser/ui/views/exclusive_access_bubble_views_context.h:26: virtual
views::Widget* GetWidget() = 0;
On 2015/02/26 18:29:20, sky wrote:
> The names here are too generic and easily conflict with other things
BrowserView
> has. Either change the names to be more explicit, of make BrowserView have an
> object that implements the interface but does not directly implement it.

Done.

[miu, 2015-02-27 00:38:11]

lgtm

[sky, 2015-02-27 15:57:18]

LGTM

[Sriram, 2015-03-03 19:28:16]

On 2015/02/27 15:57:18, sky wrote:
> LGTM

I had to update the patch with following changes due to unit test failures:
1. Implement ExclusiveAccessContext in BrowserWindowCocoa so it can return
pointer to itself in Cocoa.
2. Implement ExclusiveAccessContext in test to be returned as part of
GetExclusiveAccessContext() in test BrowserWindow class.

Scott - could you PTAL? Thanks!

[sky, 2015-03-03 20:16:03]

SLGTM

[Sriram, 2015-03-03 21:07:21]

The CQ bit was checked by sriramsr@chromium.org

[Sriram, 2015-03-03 21:07:22]

The patchset sent to the CQ was uploaded after l-g-t-m from scheib@chromium.org,
miu@chromium.org, sky@chromium.org
Link to the patchset: https://codereview.chromium.org/877413004/#ps220001
(title: "Fix broken mac change")

[commit-bot: I haz the power, 2015-03-03 21:08:35]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/877413004/220001

[commit-bot: I haz the power, 2015-03-04 00:03:41]

Committed patchset #12 (id:220001)

[commit-bot: I haz the power, 2015-03-04 00:05:22]

Patchset 12 (id:??) landed as
https://crrev.com/39b6b787e7c4cbcecf1c9be40e9f47b76cd3c14b
Cr-Commit-Position: refs/heads/master@{#318965}