Refactor fullscreen_controller.

Refactor fullscreen_controller.

Split FullscreenController to MouseController (and leave fullscreen related parts in original class). ExclusiveAccessManager is responsible for coordinating the different exclusive access controllers.

BUG=166928
TEST=Requires MANUAL testing of fullscreen and pointer/mouse lock is required as tests are disabled for being flaky

Committed: https://crrev.com/a41db560b2f4f86d1b8b05bfbad6b0684314662a
Cr-Commit-Position: refs/heads/master@{#313159}

[Sriram, 2015-01-14 17:36:51]

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

[Sriram, 2015-01-14 17:37:19]

I have not finished with the CL (even got tests to pass) but would appreciate
early feedback on the refactor. Can you take a look?

[scheib, 2015-01-14 22:37:59]

Looks like the right direction to me. When you get into testing, there are some
automated tests but manual testing is definitely warranted.
http://scheib.github.io/HTMLMisc/PointerLockAndFullscreen-NoPrefix.html may help
with that.

https://codereview.chromium.org/836933005/diff/1/chrome/browser/ui/exclusive_...
File chrome/browser/ui/exclusive_access/exclusive_access_controller_base.cc
(right):

https://codereview.chromium.org/836933005/diff/1/chrome/browser/ui/exclusive_...
chrome/browser/ui/exclusive_access/exclusive_access_controller_base.cc:51: //
Derived class will to actually implement if necessary.
"will have to implement if"

https://codereview.chromium.org/836933005/diff/1/chrome/browser/ui/exclusive_...
chrome/browser/ui/exclusive_access/exclusive_access_controller_base.cc:106:
manager_->UpdateExclusiveAccessExitBubbleContent();
For any Base method that just calls to a manager method, I think we should have
subclasses call the GetManager()->Method directly. Simplify Base and indirection
as much as possible.

https://codereview.chromium.org/836933005/diff/1/chrome/browser/ui/exclusive_...
File chrome/browser/ui/exclusive_access/exclusive_access_controller_base.h
(right):

https://codereview.chromium.org/836933005/diff/1/chrome/browser/ui/exclusive_...
chrome/browser/ui/exclusive_access/exclusive_access_controller_base.h:24: //
Base class for different exclusive_access controller class that implements
Later readers will be fairly confused by what "Exclusive Access" is all about.
Add more text here (or refer to text at e.g. ExclusiveAccessControllerManager)
to describe what this is all about, it's OK to mention the Fullscreen and
MouseLock components explicitly as the understanding will have more value than
abstraction.

Also, s/class/classes/

https://codereview.chromium.org/836933005/diff/1/chrome/browser/ui/exclusive_...
chrome/browser/ui/exclusive_access/exclusive_access_controller_base.h:56: //
content::NotificationObserver:
Might as well explain why:
content:NotificationObserver:
Detect page navigation and exits exclusive access.

https://codereview.chromium.org/836933005/diff/1/chrome/browser/ui/exclusive_...
File chrome/browser/ui/exclusive_access/exclusive_access_controller_manager.h
(right):

https://codereview.chromium.org/836933005/diff/1/chrome/browser/ui/exclusive_...
chrome/browser/ui/exclusive_access/exclusive_access_controller_manager.h:21: //
This class coordiates different exclusive access controllers and updates
Define "Exclusive access" as if you've just run into a SWE with no clue what
that term means, OK to call out Fullscreen and MouseLock as examples.

https://codereview.chromium.org/836933005/diff/1/chrome/browser/ui/exclusive_...
chrome/browser/ui/exclusive_access/exclusive_access_controller_manager.h:23:
class ExclusiveAccessControllerManager {
Maybe just ExclusiveAccessManager

https://codereview.chromium.org/836933005/diff/1/chrome/browser/ui/exclusive_...
File chrome/browser/ui/exclusive_access/fullscreen_controller.h (right):

https://codereview.chromium.org/836933005/diff/1/chrome/browser/ui/exclusive_...
chrome/browser/ui/exclusive_access/fullscreen_controller.h:34: // fullscreen
and/or mouse lock, since the assumption is that the plugin handles
remove mouse lock reference. I haven't looked yet - is this handled in
Mouse...Controller?

[Sriram, 2015-01-15 00:34:28]

https://codereview.chromium.org/836933005/diff/1/chrome/browser/ui/exclusive_...
File chrome/browser/ui/exclusive_access/exclusive_access_controller_base.cc
(right):

https://codereview.chromium.org/836933005/diff/1/chrome/browser/ui/exclusive_...
chrome/browser/ui/exclusive_access/exclusive_access_controller_base.cc:51: //
Derived class will to actually implement if necessary.
On 2015/01/14 22:37:59, scheib wrote:
> "will have to implement if"

Done.

https://codereview.chromium.org/836933005/diff/1/chrome/browser/ui/exclusive_...
chrome/browser/ui/exclusive_access/exclusive_access_controller_base.cc:106:
manager_->UpdateExclusiveAccessExitBubbleContent();
On 2015/01/14 22:37:59, scheib wrote:
> For any Base method that just calls to a manager method, I think we should
have
> subclasses call the GetManager()->Method directly. Simplify Base and
indirection
> as much as possible.

Done.

https://codereview.chromium.org/836933005/diff/1/chrome/browser/ui/exclusive_...
File chrome/browser/ui/exclusive_access/exclusive_access_controller_base.h
(right):

https://codereview.chromium.org/836933005/diff/1/chrome/browser/ui/exclusive_...
chrome/browser/ui/exclusive_access/exclusive_access_controller_base.h:24: //
Base class for different exclusive_access controller class that implements
On 2015/01/14 22:37:59, scheib wrote:
> Later readers will be fairly confused by what "Exclusive Access" is all about.
> Add more text here (or refer to text at e.g. ExclusiveAccessControllerManager)
> to describe what this is all about, it's OK to mention the Fullscreen and
> MouseLock components explicitly as the understanding will have more value than
> abstraction.
> 
> Also, s/class/classes/

Done.

https://codereview.chromium.org/836933005/diff/1/chrome/browser/ui/exclusive_...
chrome/browser/ui/exclusive_access/exclusive_access_controller_base.h:56: //
content::NotificationObserver:
On 2015/01/14 22:37:59, scheib wrote:
> Might as well explain why:
> content:NotificationObserver:
> Detect page navigation and exits exclusive access.

Done.

https://codereview.chromium.org/836933005/diff/1/chrome/browser/ui/exclusive_...
File chrome/browser/ui/exclusive_access/exclusive_access_controller_manager.h
(right):

https://codereview.chromium.org/836933005/diff/1/chrome/browser/ui/exclusive_...
chrome/browser/ui/exclusive_access/exclusive_access_controller_manager.h:21: //
This class coordiates different exclusive access controllers and updates
On 2015/01/14 22:37:59, scheib wrote:
> Define "Exclusive access" as if you've just run into a SWE with no clue what
> that term means, OK to call out Fullscreen and MouseLock as examples. 

Done.

https://codereview.chromium.org/836933005/diff/1/chrome/browser/ui/exclusive_...
chrome/browser/ui/exclusive_access/exclusive_access_controller_manager.h:23:
class ExclusiveAccessControllerManager {
On 2015/01/14 22:37:59, scheib wrote:
> Maybe just ExclusiveAccessManager

Done.

https://codereview.chromium.org/836933005/diff/1/chrome/browser/ui/exclusive_...
File chrome/browser/ui/exclusive_access/fullscreen_controller.h (right):

https://codereview.chromium.org/836933005/diff/1/chrome/browser/ui/exclusive_...
chrome/browser/ui/exclusive_access/fullscreen_controller.h:34: // fullscreen
and/or mouse lock, since the assumption is that the plugin handles
Done.

Yes, it is handled as IsPrivilegedFullscreenForTab() is checked from
MouseLockController. This and UnlockMouse() are two places where
MouseLockController checks state of FullscreenController - which can be cleaned
up by moving part of the functionality to the ExclusiveAccessManager.  

On 2015/01/14 22:37:59, scheib wrote:
> remove mouse lock reference. I haven't looked yet - is this handled in
> Mouse...Controller?

[miu, 2015-01-16 22:17:39]

Great job on this refactoring!  :)  Comments:

In the change description, I think you meant to refer to ExclusiveAccessManager
(not ExclusiveAccessControllerManager).

https://codereview.chromium.org/836933005/diff/80001/chrome/browser/ui/browser.h
File chrome/browser/ui/browser.h (right):

https://codereview.chromium.org/836933005/diff/80001/chrome/browser/ui/browse...
chrome/browser/ui/browser.h:478: ExclusiveAccessManager*
GetExclusiveAccessManager() const {
style: Inlined accessor should be named exclusive_access_manager().

https://codereview.chromium.org/836933005/diff/80001/chrome/browser/ui/browse...
chrome/browser/ui/browser.h:956: scoped_ptr<ExclusiveAccessManager>
exclusive_access_manager_;
nit: Let's get rid of the extra indirection and heap allocation.  This, here,
should be just:

  ExclusiveAccessManager exclusive_access_manager_;

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

https://codereview.chromium.org/836933005/diff/80001/chrome/browser/ui/exclus...
chrome/browser/ui/exclusive_access/exclusive_access_controller_base.cc:55:
content::WebContents* web_contents) {
nit: s/content::WebContents/WebContents/ and then this all fits on one line.

https://codereview.chromium.org/836933005/diff/80001/chrome/browser/ui/exclus...
chrome/browser/ui/exclusive_access/exclusive_access_controller_base.cc:61: // on
it to call NotifyTabOfExitIfNecessary(), because at that point
Adjust comment to account for method rename.

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

https://codereview.chromium.org/836933005/diff/80001/chrome/browser/ui/exclus...
chrome/browser/ui/exclusive_access/exclusive_access_controller_base.h:39: //
Control behavior when escape is pressed.
Add to comment, something like: Returns true if any action was taken.

https://codereview.chromium.org/836933005/diff/80001/chrome/browser/ui/exclus...
chrome/browser/ui/exclusive_access/exclusive_access_controller_base.h:52:
virtual void OnTabDeactivated(content::WebContents* web_contents);
style: These three should be:

  void Foo(...) override;

https://codereview.chromium.org/836933005/diff/80001/chrome/browser/ui/exclus...
chrome/browser/ui/exclusive_access/exclusive_access_controller_base.h:65:
Browser* GetBrowser() const;
IMO, you should inline these getters since they just return the pointers.

https://codereview.chromium.org/836933005/diff/80001/chrome/browser/ui/exclus...
chrome/browser/ui/exclusive_access/exclusive_access_controller_base.h:79:
ExclusiveAccessManager* manager_;
This can be const too.

https://codereview.chromium.org/836933005/diff/80001/chrome/browser/ui/exclus...
chrome/browser/ui/exclusive_access/exclusive_access_controller_base.h:81:
BrowserWindow* const window_;
Is the window_ member necessary?  Code can always call browser_->window().

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

https://codereview.chromium.org/836933005/diff/80001/chrome/browser/ui/exclus...
chrome/browser/ui/exclusive_access/exclusive_access_manager.h:5: #ifndef
CHROME_BROWSER_UI_EXCLUSIVE_ACCESS_EXCLUSIVE_ACCESS_MANAGER_H_
Your git cl --similarity=X setting might be too low.

You can experiment by looking at the output of:

git diff --stat -M10% origin/master
git diff --stat -M20% origin/master
git diff --stat -M30% origin/master
...and so on...

until you still retain the refactored code history, but minimize the
"ridiculous" file rename/edit detections.

https://codereview.chromium.org/836933005/diff/80001/chrome/browser/ui/exclus...
chrome/browser/ui/exclusive_access/exclusive_access_manager.h:29:
FullscreenController* GetFullscreenController();
Trivial accessors should be inlined:

  FullscreenController* fullscreen_controller() const {
    return &fullscreen_controller_;
  }
  ...and for MouseLockController...

https://codereview.chromium.org/836933005/diff/80001/chrome/browser/ui/exclus...
chrome/browser/ui/exclusive_access/exclusive_access_manager.h:36: // Callbacks
////////////////////////////////////////////////////////////////
nit: Add newline above this line.

https://codereview.chromium.org/836933005/diff/80001/chrome/browser/ui/exclus...
chrome/browser/ui/exclusive_access/exclusive_access_manager.h:55: Browser*
browser_;
nit: Browser* const browser_;

https://codereview.chromium.org/836933005/diff/80001/chrome/browser/ui/exclus...
chrome/browser/ui/exclusive_access/exclusive_access_manager.h:56: BrowserWindow*
window_;
You can remove this: It is unused.  Also, the forward-declare at the top of the
file can then be removed.

https://codereview.chromium.org/836933005/diff/80001/chrome/browser/ui/exclus...
chrome/browser/ui/exclusive_access/exclusive_access_manager.h:57:
scoped_ptr<FullscreenController> fullscreen_controller_;
Remove extra indirection and heap objects:

  FullscreenController fullscreen_controller_;
  MouseLockController mouse_lock_controller_;

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

https://codereview.chromium.org/836933005/diff/80001/chrome/browser/ui/exclus...
chrome/browser/ui/exclusive_access/fullscreen_controller.h:52: explicit
FullscreenController(ExclusiveAccessManager* manager,
Don't need explicit keyword anymore.

https://codereview.chromium.org/836933005/diff/80001/chrome/browser/ui/exclus...
chrome/browser/ui/exclusive_access/fullscreen_controller.h:63: bool
IsPrivilegedFullscreenForTab() const;
Needs comment.  Might be worth mentioning this is called to determine whether
Flash is providing the "exit from fullscreen" messaging.

https://codereview.chromium.org/836933005/diff/80001/chrome/browser/ui/exclus...
chrome/browser/ui/exclusive_access/fullscreen_controller.h:87: // Returns true
if controller has entered fullscrene mode.
/fullscrene/fullscreen/

https://codereview.chromium.org/836933005/diff/80001/chrome/browser/ui/exclus...
chrome/browser/ui/exclusive_access/fullscreen_controller.h:168: void
UnlockMouse();
Should UnlockMouse() be removed?

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

https://codereview.chromium.org/836933005/diff/80001/chrome/browser/ui/exclus...
chrome/browser/ui/exclusive_access/mouse_lock_controller.h:14: explicit
MouseLockController(ExclusiveAccessManager* manager,
no explicit needed for 2-arg ctor

https://codereview.chromium.org/836933005/diff/80001/chrome/browser/ui/views/...
File chrome/browser/ui/views/location_bar/zoom_bubble_view.h (left):

https://codereview.chromium.org/836933005/diff/80001/chrome/browser/ui/views/...
chrome/browser/ui/views/location_bar/zoom_bubble_view.h:79:
FullscreenController* fullscreen_controller);
Nice catch of unused arg!  :)

https://codereview.chromium.org/836933005/diff/80001/chrome/browser/ui/views/...
File chrome/browser/ui/views/passwords/manage_passwords_bubble_view.cc (right):

https://codereview.chromium.org/836933005/diff/80001/chrome/browser/ui/views/...
chrome/browser/ui/views/passwords/manage_passwords_bubble_view.cc:842:
DCHECK(browser->GetExclusiveAccessManager()->GetFullscreenController());
This DCHECK can be removed since there can never be a null pointer to
ExclusiveAccessManager nor FullscreenController if you have a valid Browser
object.

[Sriram, 2015-01-21 01:38:57]

https://codereview.chromium.org/836933005/diff/80001/chrome/browser/ui/browser.h
File chrome/browser/ui/browser.h (right):

https://codereview.chromium.org/836933005/diff/80001/chrome/browser/ui/browse...
chrome/browser/ui/browser.h:478: ExclusiveAccessManager*
GetExclusiveAccessManager() const {
On 2015/01/16 22:17:38, miu wrote:
> style: Inlined accessor should be named exclusive_access_manager().

Done.

https://codereview.chromium.org/836933005/diff/80001/chrome/browser/ui/browse...
chrome/browser/ui/browser.h:956: scoped_ptr<ExclusiveAccessManager>
exclusive_access_manager_;
On 2015/01/16 22:17:38, miu wrote:
> nit: Let's get rid of the extra indirection and heap allocation.  This, here,
> should be just:
> 
>   ExclusiveAccessManager exclusive_access_manager_;

Browser object construction is complex with some members like window not being
fully constructed. I think passing that to constructor is asking for trouble and
so I am leaning on leaving it be as heap member.

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

https://codereview.chromium.org/836933005/diff/80001/chrome/browser/ui/exclus...
chrome/browser/ui/exclusive_access/exclusive_access_controller_base.cc:55:
content::WebContents* web_contents) {
On 2015/01/16 22:17:38, miu wrote:
> nit: s/content::WebContents/WebContents/ and then this all fits on one line.

Done.

https://codereview.chromium.org/836933005/diff/80001/chrome/browser/ui/exclus...
chrome/browser/ui/exclusive_access/exclusive_access_controller_base.cc:61: // on
it to call NotifyTabOfExitIfNecessary(), because at that point
On 2015/01/16 22:17:38, miu wrote:
> Adjust comment to account for method rename.

Done.

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

https://codereview.chromium.org/836933005/diff/80001/chrome/browser/ui/exclus...
chrome/browser/ui/exclusive_access/exclusive_access_controller_base.h:39: //
Control behavior when escape is pressed.
On 2015/01/16 22:17:38, miu wrote:
> Add to comment, something like: Returns true if any action was taken.

Done.

https://codereview.chromium.org/836933005/diff/80001/chrome/browser/ui/exclus...
chrome/browser/ui/exclusive_access/exclusive_access_controller_base.h:52:
virtual void OnTabDeactivated(content::WebContents* web_contents);
On 2015/01/16 22:17:38, miu wrote:
> style: These three should be:
> 
>   void Foo(...) override;

They are not callbacks but actual function calls. So updated comments to reflect
this.

https://codereview.chromium.org/836933005/diff/80001/chrome/browser/ui/exclus...
chrome/browser/ui/exclusive_access/exclusive_access_controller_base.h:65:
Browser* GetBrowser() const;
On 2015/01/16 22:17:38, miu wrote:
> IMO, you should inline these getters since they just return the pointers.

Done.

https://codereview.chromium.org/836933005/diff/80001/chrome/browser/ui/exclus...
chrome/browser/ui/exclusive_access/exclusive_access_controller_base.h:79:
ExclusiveAccessManager* manager_;
On 2015/01/16 22:17:38, miu wrote:
> This can be const too.

Done.

https://codereview.chromium.org/836933005/diff/80001/chrome/browser/ui/exclus...
chrome/browser/ui/exclusive_access/exclusive_access_controller_base.h:81:
BrowserWindow* const window_;
On 2015/01/16 22:17:38, miu wrote:
> Is the window_ member necessary?  Code can always call browser_->window().

Done.

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

https://codereview.chromium.org/836933005/diff/80001/chrome/browser/ui/exclus...
chrome/browser/ui/exclusive_access/exclusive_access_manager.h:5: #ifndef
CHROME_BROWSER_UI_EXCLUSIVE_ACCESS_EXCLUSIVE_ACCESS_MANAGER_H_
I used 10% but tried a few value and even at 11% mouse_lock_controller is not
detected to be split from fullscreen_controller which I think is nice to have
history across. Is there any way to explicitly set L-side file? 

On 2015/01/16 22:17:39, miu wrote:
> Your git cl --similarity=X setting might be too low.
> 
> You can experiment by looking at the output of:
> 
> git diff --stat -M10% origin/master
> git diff --stat -M20% origin/master
> git diff --stat -M30% origin/master
> ...and so on...
> 
> until you still retain the refactored code history, but minimize the
> "ridiculous" file rename/edit detections.

https://codereview.chromium.org/836933005/diff/80001/chrome/browser/ui/exclus...
chrome/browser/ui/exclusive_access/exclusive_access_manager.h:29:
FullscreenController* GetFullscreenController();
On 2015/01/16 22:17:39, miu wrote:
> Trivial accessors should be inlined:
> 
>   FullscreenController* fullscreen_controller() const {
>     return &fullscreen_controller_;
>   }
>   ...and for MouseLockController...

Done.

https://codereview.chromium.org/836933005/diff/80001/chrome/browser/ui/exclus...
chrome/browser/ui/exclusive_access/exclusive_access_manager.h:36: // Callbacks
////////////////////////////////////////////////////////////////
On 2015/01/16 22:17:38, miu wrote:
> nit: Add newline above this line.

Done.

https://codereview.chromium.org/836933005/diff/80001/chrome/browser/ui/exclus...
chrome/browser/ui/exclusive_access/exclusive_access_manager.h:55: Browser*
browser_;
On 2015/01/16 22:17:38, miu wrote:
> nit: Browser* const browser_;

Done.

https://codereview.chromium.org/836933005/diff/80001/chrome/browser/ui/exclus...
chrome/browser/ui/exclusive_access/exclusive_access_manager.h:56: BrowserWindow*
window_;
On 2015/01/16 22:17:38, miu wrote:
> You can remove this: It is unused.  Also, the forward-declare at the top of
the
> file can then be removed.

Done.

https://codereview.chromium.org/836933005/diff/80001/chrome/browser/ui/exclus...
chrome/browser/ui/exclusive_access/exclusive_access_manager.h:57:
scoped_ptr<FullscreenController> fullscreen_controller_;
On 2015/01/16 22:17:38, miu wrote:
> Remove extra indirection and heap objects:
> 
>   FullscreenController fullscreen_controller_;
>   MouseLockController mouse_lock_controller_;

Done.

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

https://codereview.chromium.org/836933005/diff/80001/chrome/browser/ui/exclus...
chrome/browser/ui/exclusive_access/fullscreen_controller.h:52: explicit
FullscreenController(ExclusiveAccessManager* manager,
On 2015/01/16 22:17:39, miu wrote:
> Don't need explicit keyword anymore.

Done.

https://codereview.chromium.org/836933005/diff/80001/chrome/browser/ui/exclus...
chrome/browser/ui/exclusive_access/fullscreen_controller.h:63: bool
IsPrivilegedFullscreenForTab() const;
On 2015/01/16 22:17:39, miu wrote:
> Needs comment.  Might be worth mentioning this is called to determine whether
> Flash is providing the "exit from fullscreen" messaging.

Done.

https://codereview.chromium.org/836933005/diff/80001/chrome/browser/ui/exclus...
chrome/browser/ui/exclusive_access/fullscreen_controller.h:87: // Returns true
if controller has entered fullscrene mode.
On 2015/01/16 22:17:39, miu wrote:
> /fullscrene/fullscreen/

Done.

https://codereview.chromium.org/836933005/diff/80001/chrome/browser/ui/exclus...
chrome/browser/ui/exclusive_access/fullscreen_controller.h:168: void
UnlockMouse();
On 2015/01/16 22:17:39, miu wrote:
> Should UnlockMouse() be removed?

Done.

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

https://codereview.chromium.org/836933005/diff/80001/chrome/browser/ui/exclus...
chrome/browser/ui/exclusive_access/mouse_lock_controller.h:14: explicit
MouseLockController(ExclusiveAccessManager* manager,
On 2015/01/16 22:17:39, miu wrote:
> no explicit needed for 2-arg ctor

Done.

https://codereview.chromium.org/836933005/diff/80001/chrome/browser/ui/views/...
File chrome/browser/ui/views/location_bar/zoom_bubble_view.h (left):

https://codereview.chromium.org/836933005/diff/80001/chrome/browser/ui/views/...
chrome/browser/ui/views/location_bar/zoom_bubble_view.h:79:
FullscreenController* fullscreen_controller);
On 2015/01/16 22:17:39, miu wrote:
> Nice catch of unused arg!  :)

Acknowledged.

https://codereview.chromium.org/836933005/diff/80001/chrome/browser/ui/views/...
File chrome/browser/ui/views/passwords/manage_passwords_bubble_view.cc (right):

https://codereview.chromium.org/836933005/diff/80001/chrome/browser/ui/views/...
chrome/browser/ui/views/passwords/manage_passwords_bubble_view.cc:842:
DCHECK(browser->GetExclusiveAccessManager()->GetFullscreenController());
On 2015/01/16 22:17:39, miu wrote:
> This DCHECK can be removed since there can never be a null pointer to
> ExclusiveAccessManager nor FullscreenController if you have a valid Browser
> object.

Done.

[scheib, 2015-01-21 16:45:43]

> I have not finished with the CL (even got tests to pass) but would
appreciate
early feedback on the refactor.
Speak up when you want a full review.

On Tue, Jan 20, 2015 at 5:38 PM, <sriramsr@chromium.org> wrote:

>
> https://codereview.chromium.org/836933005/diff/80001/
> chrome/browser/ui/browser.h
> File chrome/browser/ui/browser.h (right):
>
> https://codereview.chromium.org/836933005/diff/80001/
> chrome/browser/ui/browser.h#newcode478
> chrome/browser/ui/browser.h:478: ExclusiveAccessManager*
> GetExclusiveAccessManager() const {
> On 2015/01/16 22:17:38, miu wrote:
>
>> style: Inlined accessor should be named exclusive_access_manager().
>>
>
> Done.
>
> https://codereview.chromium.org/836933005/diff/80001/
> chrome/browser/ui/browser.h#newcode956
> chrome/browser/ui/browser.h:956: scoped_ptr<ExclusiveAccessManager>
> exclusive_access_manager_;
> On 2015/01/16 22:17:38, miu wrote:
>
>> nit: Let's get rid of the extra indirection and heap allocation.
>>
> This, here,
>
>> should be just:
>>
>
>     ExclusiveAccessManager exclusive_access_manager_;
>>
>
> Browser object construction is complex with some members like window not
> being fully constructed. I think passing that to constructor is asking
> for trouble and so I am leaning on leaving it be as heap member.
>
> https://codereview.chromium.org/836933005/diff/80001/
> chrome/browser/ui/exclusive_access/exclusive_access_controller_base.cc
> File
> chrome/browser/ui/exclusive_access/exclusive_access_controller_base.cc
> (right):
>
> https://codereview.chromium.org/836933005/diff/80001/
> chrome/browser/ui/exclusive_access/exclusive_access_
> controller_base.cc#newcode55
> chrome/browser/ui/exclusive_access/exclusive_access_controller_base.cc:55:
> content::WebContents* web_contents) {
> On 2015/01/16 22:17:38, miu wrote:
>
>> nit: s/content::WebContents/WebContents/ and then this all fits on one
>>
> line.
>
> Done.
>
> https://codereview.chromium.org/836933005/diff/80001/
> chrome/browser/ui/exclusive_access/exclusive_access_
> controller_base.cc#newcode61
> chrome/browser/ui/exclusive_access/exclusive_access_controller_base.cc:61:
> // on it to call NotifyTabOfExitIfNecessary(), because at that point
> On 2015/01/16 22:17:38, miu wrote:
>
>> Adjust comment to account for method rename.
>>
>
> Done.
>
> https://codereview.chromium.org/836933005/diff/80001/
> chrome/browser/ui/exclusive_access/exclusive_access_controller_base.h
> File
> chrome/browser/ui/exclusive_access/exclusive_access_controller_base.h
> (right):
>
> https://codereview.chromium.org/836933005/diff/80001/
> chrome/browser/ui/exclusive_access/exclusive_access_
> controller_base.h#newcode39
> chrome/browser/ui/exclusive_access/exclusive_access_controller_base.h:39:
> // Control behavior when escape is pressed.
> On 2015/01/16 22:17:38, miu wrote:
>
>> Add to comment, something like: Returns true if any action was taken.
>>
>
> Done.
>
> https://codereview.chromium.org/836933005/diff/80001/
> chrome/browser/ui/exclusive_access/exclusive_access_
> controller_base.h#newcode52
> chrome/browser/ui/exclusive_access/exclusive_access_controller_base.h:52:
> virtual void OnTabDeactivated(content::WebContents* web_contents);
> On 2015/01/16 22:17:38, miu wrote:
>
>> style: These three should be:
>>
>
>     void Foo(...) override;
>>
>
> They are not callbacks but actual function calls. So updated comments to
> reflect this.
>
> https://codereview.chromium.org/836933005/diff/80001/
> chrome/browser/ui/exclusive_access/exclusive_access_
> controller_base.h#newcode65
> chrome/browser/ui/exclusive_access/exclusive_access_controller_base.h:65:
> Browser* GetBrowser() const;
> On 2015/01/16 22:17:38, miu wrote:
>
>> IMO, you should inline these getters since they just return the
>>
> pointers.
>
> Done.
>
> https://codereview.chromium.org/836933005/diff/80001/
> chrome/browser/ui/exclusive_access/exclusive_access_
> controller_base.h#newcode79
> chrome/browser/ui/exclusive_access/exclusive_access_controller_base.h:79:
> ExclusiveAccessManager* manager_;
> On 2015/01/16 22:17:38, miu wrote:
>
>> This can be const too.
>>
>
> Done.
>
> https://codereview.chromium.org/836933005/diff/80001/
> chrome/browser/ui/exclusive_access/exclusive_access_
> controller_base.h#newcode81
> chrome/browser/ui/exclusive_access/exclusive_access_controller_base.h:81:
> BrowserWindow* const window_;
> On 2015/01/16 22:17:38, miu wrote:
>
>> Is the window_ member necessary?  Code can always call
>>
> browser_->window().
>
> Done.
>
> https://codereview.chromium.org/836933005/diff/80001/
> chrome/browser/ui/exclusive_access/exclusive_access_manager.h
> File chrome/browser/ui/exclusive_access/exclusive_access_manager.h
> (right):
>
> https://codereview.chromium.org/836933005/diff/80001/
> chrome/browser/ui/exclusive_access/exclusive_access_manager.h#newcode5
> chrome/browser/ui/exclusive_access/exclusive_access_manager.h:5: #ifndef
> CHROME_BROWSER_UI_EXCLUSIVE_ACCESS_EXCLUSIVE_ACCESS_MANAGER_H_
> I used 10% but tried a few value and even at 11% mouse_lock_controller
> is not detected to be split from fullscreen_controller which I think is
> nice to have history across. Is there any way to explicitly set L-side
> file?
>
> On 2015/01/16 22:17:39, miu wrote:
>
>> Your git cl --similarity=X setting might be too low.
>>
>
>  You can experiment by looking at the output of:
>>
>
>  git diff --stat -M10% origin/master
>> git diff --stat -M20% origin/master
>> git diff --stat -M30% origin/master
>> ...and so on...
>>
>
>  until you still retain the refactored code history, but minimize the
>> "ridiculous" file rename/edit detections.
>>
>
> https://codereview.chromium.org/836933005/diff/80001/
> chrome/browser/ui/exclusive_access/exclusive_access_manager.h#newcode29
> chrome/browser/ui/exclusive_access/exclusive_access_manager.h:29:
> FullscreenController* GetFullscreenController();
> On 2015/01/16 22:17:39, miu wrote:
>
>> Trivial accessors should be inlined:
>>
>
>     FullscreenController* fullscreen_controller() const {
>>      return &fullscreen_controller_;
>>    }
>>    ...and for MouseLockController...
>>
>
> Done.
>
> https://codereview.chromium.org/836933005/diff/80001/
> chrome/browser/ui/exclusive_access/exclusive_access_manager.h#newcode36
> chrome/browser/ui/exclusive_access/exclusive_access_manager.h:36: //
> Callbacks
> ////////////////////////////////////////////////////////////////
> On 2015/01/16 22:17:38, miu wrote:
>
>> nit: Add newline above this line.
>>
>
> Done.
>
> https://codereview.chromium.org/836933005/diff/80001/
> chrome/browser/ui/exclusive_access/exclusive_access_manager.h#newcode55
> chrome/browser/ui/exclusive_access/exclusive_access_manager.h:55:
> Browser* browser_;
> On 2015/01/16 22:17:38, miu wrote:
>
>> nit: Browser* const browser_;
>>
>
> Done.
>
> https://codereview.chromium.org/836933005/diff/80001/
> chrome/browser/ui/exclusive_access/exclusive_access_manager.h#newcode56
> chrome/browser/ui/exclusive_access/exclusive_access_manager.h:56:
> BrowserWindow* window_;
> On 2015/01/16 22:17:38, miu wrote:
>
>> You can remove this: It is unused.  Also, the forward-declare at the
>>
> top of the
>
>> file can then be removed.
>>
>
> Done.
>
> https://codereview.chromium.org/836933005/diff/80001/
> chrome/browser/ui/exclusive_access/exclusive_access_manager.h#newcode57
> chrome/browser/ui/exclusive_access/exclusive_access_manager.h:57:
> scoped_ptr<FullscreenController> fullscreen_controller_;
> On 2015/01/16 22:17:38, miu wrote:
>
>> Remove extra indirection and heap objects:
>>
>
>     FullscreenController fullscreen_controller_;
>>    MouseLockController mouse_lock_controller_;
>>
>
> Done.
>
> https://codereview.chromium.org/836933005/diff/80001/
> chrome/browser/ui/exclusive_access/fullscreen_controller.h
> File chrome/browser/ui/exclusive_access/fullscreen_controller.h (right):
>
> https://codereview.chromium.org/836933005/diff/80001/
> chrome/browser/ui/exclusive_access/fullscreen_controller.h#newcode52
> chrome/browser/ui/exclusive_access/fullscreen_controller.h:52: explicit
> FullscreenController(ExclusiveAccessManager* manager,
> On 2015/01/16 22:17:39, miu wrote:
>
>> Don't need explicit keyword anymore.
>>
>
> Done.
>
> https://codereview.chromium.org/836933005/diff/80001/
> chrome/browser/ui/exclusive_access/fullscreen_controller.h#newcode63
> chrome/browser/ui/exclusive_access/fullscreen_controller.h:63: bool
> IsPrivilegedFullscreenForTab() const;
> On 2015/01/16 22:17:39, miu wrote:
>
>> Needs comment.  Might be worth mentioning this is called to determine
>>
> whether
>
>> Flash is providing the "exit from fullscreen" messaging.
>>
>
> Done.
>
> https://codereview.chromium.org/836933005/diff/80001/
> chrome/browser/ui/exclusive_access/fullscreen_controller.h#newcode87
> chrome/browser/ui/exclusive_access/fullscreen_controller.h:87: //
> Returns true if controller has entered fullscrene mode.
> On 2015/01/16 22:17:39, miu wrote:
>
>> /fullscrene/fullscreen/
>>
>
> Done.
>
> https://codereview.chromium.org/836933005/diff/80001/
> chrome/browser/ui/exclusive_access/fullscreen_controller.h#newcode168
> chrome/browser/ui/exclusive_access/fullscreen_controller.h:168: void
> UnlockMouse();
> On 2015/01/16 22:17:39, miu wrote:
>
>> Should UnlockMouse() be removed?
>>
>
> Done.
>
> https://codereview.chromium.org/836933005/diff/80001/
> chrome/browser/ui/exclusive_access/mouse_lock_controller.h
> File chrome/browser/ui/exclusive_access/mouse_lock_controller.h (right):
>
> https://codereview.chromium.org/836933005/diff/80001/
> chrome/browser/ui/exclusive_access/mouse_lock_controller.h#newcode14
> chrome/browser/ui/exclusive_access/mouse_lock_controller.h:14: explicit
> MouseLockController(ExclusiveAccessManager* manager,
> On 2015/01/16 22:17:39, miu wrote:
>
>> no explicit needed for 2-arg ctor
>>
>
> Done.
>
> https://codereview.chromium.org/836933005/diff/80001/
> chrome/browser/ui/views/location_bar/zoom_bubble_view.h
> File chrome/browser/ui/views/location_bar/zoom_bubble_view.h (left):
>
> https://codereview.chromium.org/836933005/diff/80001/
> chrome/browser/ui/views/location_bar/zoom_bubble_view.h#oldcode79
> chrome/browser/ui/views/location_bar/zoom_bubble_view.h:79:
> FullscreenController* fullscreen_controller);
> On 2015/01/16 22:17:39, miu wrote:
>
>> Nice catch of unused arg!  :)
>>
>
> Acknowledged.
>
> https://codereview.chromium.org/836933005/diff/80001/
> chrome/browser/ui/views/passwords/manage_passwords_bubble_view.cc
> File chrome/browser/ui/views/passwords/manage_passwords_bubble_view.cc
> (right):
>
> https://codereview.chromium.org/836933005/diff/80001/
> chrome/browser/ui/views/passwords/manage_passwords_
> bubble_view.cc#newcode842
> chrome/browser/ui/views/passwords/manage_passwords_bubble_view.cc:842:
> DCHECK(browser->GetExclusiveAccessManager()->GetFullscreenController());
> On 2015/01/16 22:17:39, miu wrote:
>
>> This DCHECK can be removed since there can never be a null pointer to
>> ExclusiveAccessManager nor FullscreenController if you have a valid
>>
> Browser
>
>> object.
>>
>
> Done.
>
> https://codereview.chromium.org/836933005/
>

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

[Sriram, 2015-01-21 18:25:35]

Hi, 
 This is the actual refactor before new lock controller for keyboard is added. 

Yuri and Vincent - the change is now ready for full review (I have done a manual
pass as well). 

Scott - Yuri/Vincent are not owner of files outside of exclusive_access/* and
would be great if you can review at least change there.

Thanks,
Sriram

[miu, 2015-01-22 00:47:15]

lgtm, assuming trybots go green after you rebase.

Also, please run all the unit tests in the chrome/browser/ui/exclusive_access
dir (using the --gtest_also_run_disabled_tests command-line arg).  IIRC, some
are compiled into unit_tests, some in browser_tests, and some in
interactive_ui_tests.  Ensure that the same tests pass (and the same failed ones
fail for the same reason), compared to a tip-of-trunk build on your desktop. 
Also, some will fail due to flakiness, so if they pass when you re-run them,
you're all good.

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

https://codereview.chromium.org/836933005/diff/80001/chrome/browser/ui/exclus...
chrome/browser/ui/exclusive_access/exclusive_access_controller_base.h:52:
virtual void OnTabDeactivated(content::WebContents* web_contents);
On 2015/01/21 01:38:56, Sriram wrote:
> On 2015/01/16 22:17:38, miu wrote:
> > style: These three should be:
> > 
> >   void Foo(...) override;
> 
> They are not callbacks but actual function calls. So updated comments to
reflect
> this.

OIC, these are not overriding a virtual method from a parent class here.

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

https://codereview.chromium.org/836933005/diff/80001/chrome/browser/ui/exclus...
chrome/browser/ui/exclusive_access/exclusive_access_manager.h:5: #ifndef
CHROME_BROWSER_UI_EXCLUSIVE_ACCESS_EXCLUSIVE_ACCESS_MANAGER_H_
On 2015/01/21 01:38:57, Sriram wrote:
> I used 10% but tried a few value and even at 11% mouse_lock_controller is not
> detected to be split from fullscreen_controller which I think is nice to have
> history across. Is there any way to explicitly set L-side file? 

Not that I know of.  Yeah, just prefer retaining history at all costs; even if
that means new files are based on "bogus" LHS files.

[scheib, 2015-01-22 22:54:04]

Please call out in the change description TEST= line that manual testing of
fullscreen and mouselock are required.

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

https://codereview.chromium.org/836933005/diff/120001/chrome/browser/ui/exclu...
chrome/browser/ui/exclusive_access/fullscreen_controller.h:159: void
NotifyTabOfExclusiveAccessChange() override;
Rename to e.g. NotifyTabExclusiveAccessLost

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

https://codereview.chromium.org/836933005/diff/120001/chrome/browser/ui/exclu...
chrome/browser/ui/exclusive_access/mouse_lock_controller.h:52: void
NotifyTabOfExclusiveAccessChange() override;
Rename to e.g. NotifyTabExclusiveAccessLost.

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

https://codereview.chromium.org/836933005/diff/140001/chrome/browser/ui/brows...
chrome/browser/ui/browser.cc:1185: // Escape exits tabbed fullscreen mode.
and mouse lock, and possibly others.

https://codereview.chromium.org/836933005/diff/140001/chrome/browser/ui/exclu...
File chrome/browser/ui/exclusive_access/exclusive_access_controller_base.cc
(right):

https://codereview.chromium.org/836933005/diff/140001/chrome/browser/ui/exclu...
chrome/browser/ui/exclusive_access/exclusive_access_controller_base.cc:77:
tab_with_exclusive_access_ = tab;
Would you add 
// Tab should never be replaced with another tab, or
UpdateNotificationRegistrations would need updating.
DCHECK(tab_with_exclusive_access_ == tab || tab_with_exclusive_access_ ==
nullptr || tab == nullptr);

https://codereview.chromium.org/836933005/diff/140001/chrome/browser/ui/exclu...
File chrome/browser/ui/exclusive_access/exclusive_access_controller_base.h
(right):

https://codereview.chromium.org/836933005/diff/140001/chrome/browser/ui/exclu...
chrome/browser/ui/exclusive_access/exclusive_access_controller_base.h:24: // The
base class for the different exclusive access controller like the
controller -> controllers

https://codereview.chromium.org/836933005/diff/140001/chrome/browser/ui/exclu...
chrome/browser/ui/exclusive_access/exclusive_access_controller_base.h:48: //
Return true if bubble content should be updated.
Return -> Returns
 
http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Function_Comm...
"These comments should be descriptive ("Opens the file") rather than imperative
("Open the file"); "
Probably prefix with explanation as well. e.g.
// Called to indicate that the permission for this exclusive access has been
granted. Returns true of the bubble content should be updated.

https://codereview.chromium.org/836933005/diff/140001/chrome/browser/ui/exclu...
chrome/browser/ui/exclusive_access/exclusive_access_controller_base.h:76: //
Make the current tab exit exclusive access mode if it is in it.
(Descriptive vs Imperative)
// Exits exclusive access mode for the tab if currently exclusive.

https://codereview.chromium.org/836933005/diff/140001/chrome/browser/ui/exclu...
chrome/browser/ui/exclusive_access/exclusive_access_controller_base.h:81:
virtual void NotifyTabOfExclusiveAccessChange() = 0;
The implementations are for exiting exclusive access. Rename to e.g.
NotifyTabExclusiveAccessLost.

https://codereview.chromium.org/836933005/diff/140001/chrome/browser/ui/exclu...
File chrome/browser/ui/exclusive_access/exclusive_access_manager.cc (right):

https://codereview.chromium.org/836933005/diff/140001/chrome/browser/ui/exclu...
chrome/browser/ui/exclusive_access/exclusive_access_manager.cc:33: if
(mouse_lock_controller_.IsMouseLockSilentlyAccepted())
There could be a bug here.
1. Clear content settings allowing fullscreen for site.
2. Enter mouse lock
3. Page exits mouse lock via document.exitPointerLock (NOT by ESC).
   http://scheib.github.io/HTMLMisc/PointerLockAndFullscreen.html
   has "Toggle Pointer Lock" button
4. Page requests fullscreen

Expected (and current on stable): Fullscreen bubble shown.

Risk: Logic elsewhere won't cause that, and IsMouseLockSilentlyAccepted() true
here will have no bubble shown.

Verify manually. Better yet, update this logic so by inspection we're certain we
won't encounter this.
if (mouse_lock_controller_.IsMouseLockSilentlyAccepted() &&
    (!fullscreen_controller_.IsWindowFullscreenForTabOrPending() ||
     fullscreen_controller_.IsUserAcceptedFullscreen()))
    
If making the logic change, verify automated tests and also manually that
pressing Toggle Pointer Lock repeatedly on
http://scheib.github.io/HTMLMisc/PointerLockAndFullscreen.html doesn't show the
bubble (use space bar to press button when in pointer lock).

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

https://codereview.chromium.org/836933005/diff/140001/chrome/browser/ui/exclu...
chrome/browser/ui/exclusive_access/exclusive_access_manager.h:23: // mouse lock)
which are each handled by respective controller. It also update
update -> updates

[Sriram, 2015-01-23 00:24:03]

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

[Sriram, 2015-01-23 07:13:43]

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

https://codereview.chromium.org/836933005/diff/140001/chrome/browser/ui/brows...
chrome/browser/ui/browser.cc:1185: // Escape exits tabbed fullscreen mode.
On 2015/01/22 22:54:03, scheib wrote:
> and mouse lock, and possibly others. 

Done.

https://codereview.chromium.org/836933005/diff/140001/chrome/browser/ui/exclu...
File chrome/browser/ui/exclusive_access/exclusive_access_controller_base.cc
(right):

https://codereview.chromium.org/836933005/diff/140001/chrome/browser/ui/exclu...
chrome/browser/ui/exclusive_access/exclusive_access_controller_base.cc:77:
tab_with_exclusive_access_ = tab;
On 2015/01/22 22:54:04, scheib wrote:
> Would you add 
> // Tab should never be replaced with another tab, or
> UpdateNotificationRegistrations would need updating.
> DCHECK(tab_with_exclusive_access_ == tab || tab_with_exclusive_access_ ==
> nullptr || tab == nullptr);

Done.

https://codereview.chromium.org/836933005/diff/140001/chrome/browser/ui/exclu...
File chrome/browser/ui/exclusive_access/exclusive_access_controller_base.h
(right):

https://codereview.chromium.org/836933005/diff/140001/chrome/browser/ui/exclu...
chrome/browser/ui/exclusive_access/exclusive_access_controller_base.h:24: // The
base class for the different exclusive access controller like the
On 2015/01/22 22:54:04, scheib wrote:
> controller -> controllers

Done.

https://codereview.chromium.org/836933005/diff/140001/chrome/browser/ui/exclu...
chrome/browser/ui/exclusive_access/exclusive_access_controller_base.h:48: //
Return true if bubble content should be updated.
On 2015/01/22 22:54:04, scheib wrote:
> Return -> Returns
>  
>
http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Function_Comm...
> "These comments should be descriptive ("Opens the file") rather than
imperative
> ("Open the file"); "
> Probably prefix with explanation as well. e.g.
> // Called to indicate that the permission for this exclusive access has been
> granted. Returns true of the bubble content should be updated.

Done.

https://codereview.chromium.org/836933005/diff/140001/chrome/browser/ui/exclu...
chrome/browser/ui/exclusive_access/exclusive_access_controller_base.h:76: //
Make the current tab exit exclusive access mode if it is in it.
On 2015/01/22 22:54:04, scheib wrote:
> (Descriptive vs Imperative)
> // Exits exclusive access mode for the tab if currently exclusive.

Done.

https://codereview.chromium.org/836933005/diff/140001/chrome/browser/ui/exclu...
chrome/browser/ui/exclusive_access/exclusive_access_controller_base.h:81:
virtual void NotifyTabOfExclusiveAccessChange() = 0;
On 2015/01/22 22:54:04, scheib wrote:
> The implementations are for exiting exclusive access. Rename to e.g.
> NotifyTabExclusiveAccessLost.

Done.

https://codereview.chromium.org/836933005/diff/140001/chrome/browser/ui/exclu...
File chrome/browser/ui/exclusive_access/exclusive_access_manager.cc (right):

https://codereview.chromium.org/836933005/diff/140001/chrome/browser/ui/exclu...
chrome/browser/ui/exclusive_access/exclusive_access_manager.cc:33: if
(mouse_lock_controller_.IsMouseLockSilentlyAccepted())
On 2015/01/22 22:54:04, scheib wrote:
> There could be a bug here.
> 1. Clear content settings allowing fullscreen for site.
> 2. Enter mouse lock
> 3. Page exits mouse lock via document.exitPointerLock (NOT by ESC).
>    http://scheib.github.io/HTMLMisc/PointerLockAndFullscreen.html
>    has "Toggle Pointer Lock" button
> 4. Page requests fullscreen
> 
> Expected (and current on stable): Fullscreen bubble shown.
> 
> Risk: Logic elsewhere won't cause that, and IsMouseLockSilentlyAccepted() true
> here will have no bubble shown.
> 
> Verify manually. Better yet, update this logic so by inspection we're certain
we
> won't encounter this.
Great catch and verified that this repros in TOT chrome build as well as with my
change (as both are quite similar modulo where the function is called from). 

The repro steps needs to be updated though:
1. Clear settings
2. Enter mouse lock by accepting prompt.
3. Page exits mouse lock and then re-acquire by clicking "toggle lock".
4. Page requests fullscreen.

Expected: Fullscreen bubble shown but actually none shown. I have fixed this.



> if (mouse_lock_controller_.IsMouseLockSilentlyAccepted() &&
>     (!fullscreen_controller_.IsWindowFullscreenForTabOrPending() ||
>      fullscreen_controller_.IsUserAcceptedFullscreen()))
>     
> If making the logic change, verify automated tests and also manually that
> pressing Toggle Pointer Lock repeatedly on
> http://scheib.github.io/HTMLMisc/PointerLockAndFullscreen.html doesn't show
the
> bubble (use space bar to press button when in pointer lock).

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

https://codereview.chromium.org/836933005/diff/140001/chrome/browser/ui/exclu...
chrome/browser/ui/exclusive_access/exclusive_access_manager.h:23: // mouse lock)
which are each handled by respective controller. It also update
On 2015/01/22 22:54:04, scheib wrote:
> update -> updates

Done.

[scheib, 2015-01-23 16:42:32]

lgtm

https://codereview.chromium.org/836933005/diff/140001/chrome/browser/ui/exclu...
File chrome/browser/ui/exclusive_access/exclusive_access_manager.cc (right):

https://codereview.chromium.org/836933005/diff/140001/chrome/browser/ui/exclu...
chrome/browser/ui/exclusive_access/exclusive_access_manager.cc:33: if
(mouse_lock_controller_.IsMouseLockSilentlyAccepted())
On 2015/01/23 07:13:43, Sriram wrote:
> On 2015/01/22 22:54:04, scheib wrote:
> > There could be a bug here.
> > 1. Clear content settings allowing fullscreen for site.
> > 2. Enter mouse lock
> > 3. Page exits mouse lock via document.exitPointerLock (NOT by ESC).
> >    http://scheib.github.io/HTMLMisc/PointerLockAndFullscreen.html
> >    has "Toggle Pointer Lock" button
> > 4. Page requests fullscreen
> > 
> > Expected (and current on stable): Fullscreen bubble shown.
> > 
> > Risk: Logic elsewhere won't cause that, and IsMouseLockSilentlyAccepted()
true
> > here will have no bubble shown.
> > 
> > Verify manually. Better yet, update this logic so by inspection we're
certain
> we
> > won't encounter this.
> Great catch and verified that this repros in TOT chrome build as well as with
my
> change (as both are quite similar modulo where the function is called from). 
> 
> The repro steps needs to be updated though:
> 1. Clear settings
> 2. Enter mouse lock by accepting prompt.
> 3. Page exits mouse lock and then re-acquire by clicking "toggle lock".
> 4. Page requests fullscreen.
> 
> Expected: Fullscreen bubble shown but actually none shown. I have fixed this.
> 
> 
> 
> > if (mouse_lock_controller_.IsMouseLockSilentlyAccepted() &&
> >     (!fullscreen_controller_.IsWindowFullscreenForTabOrPending() ||
> >      fullscreen_controller_.IsUserAcceptedFullscreen()))
> >     
> > If making the logic change, verify automated tests and also manually that
> > pressing Toggle Pointer Lock repeatedly on
> > http://scheib.github.io/HTMLMisc/PointerLockAndFullscreen.html doesn't show
> the
> > bubble (use space bar to press button when in pointer lock).
> 

http://images.clipartpanda.com/teamwork-funny-meme2.jpg

[sky, 2015-01-26 15:39:29]

LGTM

[Sriram, 2015-01-26 21:47:57]

The CQ bit was checked by sriramsr@chromium.org

[commit-bot: I haz the power, 2015-01-26 21:48:39]

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

[commit-bot: I haz the power, 2015-01-26 22:45:35]

Committed patchset #10 (id:180001)

[commit-bot: I haz the power, 2015-01-26 22:47:01]

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