Fix scroll restoration when exiting fullscreen mode.

chrome/browser/ui/exclusive_access/fullscreen_controller.cc

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "chrome/browser/ui/exclusive_access/fullscreen_controller.h"
 
 #include "base/bind.h"
 #include "base/command_line.h"
 #include "base/location.h"
 #include "base/single_thread_task_runner.h"
@@ skipping 255 matching lines @@
 
   content::RenderWidgetHostView* const current_fs_view =
       old_contents->GetFullscreenRenderWidgetHostView();
   if (current_fs_view)
     current_fs_view->SetSize(old_contents->GetPreferredSize());
   ResizeWebContents(old_contents, old_contents->GetPreferredSize());
 }
 
 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);
   }
 }
 
 void FullscreenController::WindowFullscreenStateChanged() {
   reentrant_window_state_change_call_check_ = true;
   ExclusiveAccessContext* const exclusive_access_context =
       exclusive_access_manager()->context();
   bool exiting_fullscreen = !exclusive_access_context->IsFullscreen();
 
   PostFullscreenChangeNotification(!exiting_fullscreen);
   if (exiting_fullscreen) {
     toggled_into_fullscreen_ = false;
     extension_caused_fullscreen_ = GURL();
     NotifyTabExclusiveAccessLost();
     exclusive_access_context->UnhideDownloadShelf();
   } else {
     exclusive_access_context->HideDownloadShelf();
   }
 }
 
 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();
     return true;
   }
 
   return false;
 }
 
 void FullscreenController::ExitExclusiveAccessToPreviousState() {
   if (IsWindowFullscreenForTabOrPending())
     ExitFullscreenModeForTab(exclusive_access_tab());
@@ skipping 86 matching lines @@
       chrome::NOTIFICATION_FULLSCREEN_CHANGED,
       content::Source<FullscreenController>(this),
       content::Details<bool>(&is_fullscreen));
 }
 
 void FullscreenController::NotifyTabExclusiveAccessLost() {
   if (exclusive_access_tab()) {
     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();
   }
 }
 
 void FullscreenController::ToggleFullscreenModeInternal(
     FullscreenInternalOption option) {
 #if defined(OS_WIN)
   // When in Metro snap mode, toggling in and out of fullscreen is prevented.
   if (IsInMetroSnapMode())
     return;
@@ skipping 172 matching lines @@
     return fullscreened_origin_;
 
   return exclusive_access_tab()->GetLastCommittedURL();
 }
 
 GURL FullscreenController::GetEmbeddingOrigin() const {
   DCHECK(exclusive_access_tab());
 
   return exclusive_access_tab()->GetLastCommittedURL();
 }