Fix scroll restoration when exiting fullscreen mode.

chrome/browser/ui/exclusive_access/fullscreen_controller.cc

Index: chrome/browser/ui/exclusive_access/fullscreen_controller.cc
diff --git a/chrome/browser/ui/exclusive_access/fullscreen_controller.cc b/chrome/browser/ui/exclusive_access/fullscreen_controller.cc
index 9f18156b6f09cca7b8ef2857e9901875a6db8cd8..c7f7f9a54678bc19de4b158f4f59b3e37a41352a 100644
--- a/chrome/browser/ui/exclusive_access/fullscreen_controller.cc
+++ b/chrome/browser/ui/exclusive_access/fullscreen_controller.cc
@@ -273,7 +273,8 @@ void FullscreenController::OnTabDetachedFromView(WebContents* old_contents) {
 
 void FullscreenController::OnTabClosing(WebContents* web_contents) {
   if (IsFullscreenForCapturedTab(web_contents)) {
-    web_contents->ExitFullscreen();
+    bool will_cause_resize = IsFullscreenCausedByTab();
+    web_contents->ExitFullscreen(will_cause_resize);

[Peter Kasting, 2016/01/05 20:34:26]

Nit: Inline:

    web_contents->ExitFullscreen(IsFullscreenCausedByTab());

Then remove the {} on the arms of this conditional.

[bokan, 2016/01/06 17:48:32]

Using the local was actually suggested by scheib@ in review above to improve
readability. I tend to agree since the param has a somewhat different meaning
that "fullscreen cased by tab". WDYT?

[Peter Kasting, 2016/01/06 20:00:28]

Readers who don't already know what ExitFullscreen()'s param means can simply
look at the param name in the symbol cross-reference.  Adding a temp isn't
necessary, and it both makes the code longer to scan, as well as making the
reader wonder why the temp is there, e.g. because the function takes a non-const
ref or something.

I'm very much not a fan of such temps for these reasons.

[scheib, 2016/01/06 21:21:01]

A compromise could be 
    web_contents->ExitFullscreen(
       /* will_cause_resize */ IsFullscreenCausedByTab());
which removes the temp but keeps the ability to read this non-trivial
logic transformation without tools.

[Peter Kasting, 2016/01/06 21:26:04]

All programmers should have an easy and near-instantaneous ability to look at
the function definition for a particular symbol.  Why would we be worried about
people who aren't using such tools?

More importantly, why is it critical for the reader here to know that the
meaning of this arg is "will_cause_resize"?  If I really need to understand the
intent of this arg in the call, simply naming it isn't going to help much -- I
still don't know precisely what "will cause resize" means, concretely, or why it
is true iff IsFullscreenCausedByTab().  I'm going to have to go read the
comments on, and likely definition of, the functions being called here anyway. 
Simply naming the param doesn't tell me much.

[bokan, 2016/01/06 21:40:57]

I'm personally one such person who doesn't have this ability in my tools. My
feeling is that many of our coding guidelines tend to favor readability for
bare-bones editing. e.g. tools exist to autowrap code but we still insist on 80
col line limits. 
I think it's a small reduction in cognitive burden and a signal to the reader
that the meaning of the parameter isn't semantically the same as
isFullscreenCausedByTab. At the least it might nudge a future refactorer to be
careful when making related changes and actually go and check what the meaning
of this is (by reading the definition). Anyway, hypotheticals aside, I don't
have strong feelings here; I'm happy to change this to whatever the reviewers
agree on :)

[Peter Kasting, 2016/01/06 21:56:41]

Why?  You don't have Chromium codesearch bookmarked?

I mean, to me this really does read like a chef saying they don't own any pots
or pans.  I'm kind of mystified why someone would try to do high-level software
development without that?

[scheib, 2016/01/06 22:06:56]

I don't think we should spend much time debating. My opinion hasn't shifted
much, but it's not strong enough to spend more time. I use tools such as this
code review tool, and code search and there's no magic that makes arguments
appear without me doing work and searching. I like being able to just read code
to understand it, just looking at it. This doesn't seem to be a code style issue
- so Peter please say "Just do it this way" or say "Bokan use your judement".

   } else {
     ExclusiveAccessControllerBase::OnTabClosing(web_contents);
   }
@@ -300,7 +301,8 @@ bool FullscreenController::HandleUserPressedEscape() {
   WebContents* const active_web_contents =
       exclusive_access_manager()->context()->GetActiveWebContents();
   if (IsFullscreenForCapturedTab(active_web_contents)) {
-    active_web_contents->ExitFullscreen();
+    bool will_cause_resize = IsFullscreenCausedByTab();
+    active_web_contents->ExitFullscreen(will_cause_resize);

[Peter Kasting, 2016/01/05 20:34:26]

Nit: Inline as above.

     return true;
   } else if (IsWindowFullscreenForTabOrPending()) {

[Peter Kasting, 2016/01/05 20:34:26]

Nit: No "else" after "return".

Can reverse this conditional to be able to remove {}:

  if (!IsWindowFullscreenForTabOrPending())
    return false;
  ExitExclusiveAccessIfNecessary();
  return true;

[bokan, 2016/01/06 17:48:32]

Done.

     ExitExclusiveAccessIfNecessary();
@@ -409,9 +411,10 @@ void FullscreenController::NotifyTabExclusiveAccessLost() {
     WebContents* web_contents = exclusive_access_tab();
     SetTabWithExclusiveAccess(nullptr);
     fullscreened_origin_ = GURL();
+    bool will_cause_resize = IsFullscreenCausedByTab();
     state_prior_to_tab_fullscreen_ = STATE_INVALID;
     tab_fullscreen_accepted_ = false;
-    web_contents->ExitFullscreen();
+    web_contents->ExitFullscreen(will_cause_resize);

[Peter Kasting, 2016/01/05 20:34:26]

Nit: Inline as above.

     exclusive_access_manager()->UpdateExclusiveAccessExitBubbleContent();
   }
 }