Fix scroll restoration when exiting fullscreen mode.

Fix scroll restoration when exiting fullscreen mode.

There's two problems in Chrome/Blink that cause the existing scroll restoration
not to work on desktop platforms:

In Blink, when entering fullscreen mode, FullscreenController saves the current
scroll offset so that it can restore it later. However, this was done in
FullscreenController::didEnterFullScreen, which is called only after WebViewImpl
is resized, meaning that the scroll offset may already be wiped out.

In Chrome, we explicitly send a WasResized message to ensure that the renderer
is notified of the fullscreen change in cases where toggling fullscreen doesn't
actually change the renderer's size (e.g. going from tab fullscreen to browser
fullscreen). However, this means that the renderer gets two separate resize
events for the fullscreen toggle: one that toggles fullscreen and another for
the size change. Thus, Blink tries to restore the scroll offset before the
renderer is resized which might then clamp that scroll offset.

For the first issue, I've moved the scroll offset saving logic into
FullscreenController::enterFullScreenForElement which is called before sending
the request IPC to the browser.

For the second issue, I added a will_cause_resize parameter to
WebContents::ExitFullscreen that tells the method whether exiting fullscreen will
potentially cause a window resize. Based on this, WebContents can choose to
explicitly send the WasResized only in cases where it will not occur otherwise.
This ensures the Resize message includes both the fullscreen state change and
size change in one message.

BUG=142427
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation

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

[bokan, 2015-11-30 16:24:58]

bokan@chromium.org changed reviewers:
+ scheib@chromium.org

[bokan, 2015-11-30 16:24:59]

Hi Vincent,

I'm wondering if this is the right fix on the Chromium side? I tested using the
steps provided in https://codereview.chromium.org/928673002 and it seems to
maintain that fix. If this is fine, where should I put a test?

Thanks.

[scheib, 2015-12-01 00:30:18]

Tests aren't in a good position for this span of code logic. Browser tests have
historically been too flaky using the real browser window and OS calls.
Fullscreen logic in browser is tested in unit tests, but not with coverage of
the logic down to Blink. Perhaps ideally we would have layout tests with the
browser under test using fake browser windows and simulating the OS level
events. That would still have some limitations (are we simulating OS events
correctly) but fix robustness. It would require a moderate amount of work to
setup custom test harness hooks plumbed to fake browser window behavior. 

That is why historically you'll see manual testing called out on patches
affecting this type of code.

https://codereview.chromium.org/1488653002/diff/1/chrome/browser/ui/exclusive...
File chrome/browser/ui/exclusive_access/fullscreen_controller.cc (right):

https://codereview.chromium.org/1488653002/diff/1/chrome/browser/ui/exclusive...
chrome/browser/ui/exclusive_access/fullscreen_controller.cc:225: // Ensure web
contents exit fullscreen state by sending a resize message,
I'm a bit wondering why this isn't part of ::NotifyTabExclusiveAccessLost and
then ExitFullscreen calling ExitFullscreenMode. I haven't traced through yet to
see why the call is needed to be moved here instead of web_contents. Calling
from FullscreenController feels as if the implementation details of WebContents
are too exposed.

https://codereview.chromium.org/1488653002/diff/1/third_party/WebKit/Source/w...
File third_party/WebKit/Source/web/FullscreenController.cpp (right):

https://codereview.chromium.org/1488653002/diff/1/third_party/WebKit/Source/w...
third_party/WebKit/Source/web/FullscreenController.cpp:150: // We need to store
these values here rather than didEnterFullScreen since by the time the latter
Comments might as well wrap to 80 chars. (optional)

[bokan, 2015-12-01 20:46:10]

https://codereview.chromium.org/1488653002/diff/1/chrome/browser/ui/exclusive...
File chrome/browser/ui/exclusive_access/fullscreen_controller.cc (right):

https://codereview.chromium.org/1488653002/diff/1/chrome/browser/ui/exclusive...
chrome/browser/ui/exclusive_access/fullscreen_controller.cc:225: // Ensure web
contents exit fullscreen state by sending a resize message,
On 2015/12/01 00:30:18, scheib wrote:
> I'm a bit wondering why this isn't part of ::NotifyTabExclusiveAccessLost and
> then ExitFullscreen calling ExitFullscreenMode. I haven't traced through yet
to
> see why the call is needed to be moved here instead of web_contents. Calling
> from FullscreenController feels as if the implementation details of
WebContents
> are too exposed. 

I rarely dwell in these parts so it's entirely likely this code doesn't belong
here. The reason it was a problem before is that WasResized gets called even if
we're going from Tab Fullscreen -> Normal but that would happen before the
renderer bounds would change. So the renderer would get a resize IPC with
"is_fullscreen" toggled. Then it would get a second resize with the bounds
changed. This is a problem because the renderer then doesn't know when to
restore the pre-fullscreen state without resorting to some awkward bookkeeping .

I've uploaded a new patch that instead passes a parameter through to
WebContents::ExitFullscreen that tells it whether exiting fullscreen will cause
the bounds to change. That way WebContents can then decide whether or not to
send the WasResized. I'm happy to move it elsewhere if it makes more sense, any
suggestions?

https://codereview.chromium.org/1488653002/diff/1/third_party/WebKit/Source/w...
File third_party/WebKit/Source/web/FullscreenController.cpp (right):

https://codereview.chromium.org/1488653002/diff/1/third_party/WebKit/Source/w...
third_party/WebKit/Source/web/FullscreenController.cpp:150: // We need to store
these values here rather than didEnterFullScreen since by the time the latter
On 2015/12/01 00:30:18, scheib wrote:
> Comments might as well wrap to 80 chars. (optional)

Done.

[bokan, 2015-12-01 20:49:44]

On 2015/12/01 00:30:18, scheib wrote:
> Tests aren't in a good position for this span of code logic. Browser tests
have
> historically been too flaky using the real browser window and OS calls.
> Fullscreen logic in browser is tested in unit tests, but not with coverage of
> the logic down to Blink. Perhaps ideally we would have layout tests with the
> browser under test using fake browser windows and simulating the OS level
> events. That would still have some limitations (are we simulating OS events
> correctly) but fix robustness. It would require a moderate amount of work to
> setup custom test harness hooks plumbed to fake browser window behavior. 
> 
> That is why historically you'll see manual testing called out on patches
> affecting this type of code.

Hmm, Is it possible to write this as a unit test? Really, all I need to test is
that exiting a fullscreen tab that causes a size change sends one resize IPC
with both |fullscreen| and |bounds| modified. Doing a full integration style
test seems like it would be hard to write in a non-flaky way. Sorry if this
sounds idealistic, I have little experience at a level higher than Blink :/

> 
>
https://codereview.chromium.org/1488653002/diff/1/chrome/browser/ui/exclusive...
> File chrome/browser/ui/exclusive_access/fullscreen_controller.cc (right):
> 
>
https://codereview.chromium.org/1488653002/diff/1/chrome/browser/ui/exclusive...
> chrome/browser/ui/exclusive_access/fullscreen_controller.cc:225: // Ensure web
> contents exit fullscreen state by sending a resize message,
> I'm a bit wondering why this isn't part of ::NotifyTabExclusiveAccessLost and
> then ExitFullscreen calling ExitFullscreenMode. I haven't traced through yet
to
> see why the call is needed to be moved here instead of web_contents. Calling
> from FullscreenController feels as if the implementation details of
WebContents
> are too exposed.
> 
>
https://codereview.chromium.org/1488653002/diff/1/third_party/WebKit/Source/w...
> File third_party/WebKit/Source/web/FullscreenController.cpp (right):
> 
>
https://codereview.chromium.org/1488653002/diff/1/third_party/WebKit/Source/w...
> third_party/WebKit/Source/web/FullscreenController.cpp:150: // We need to
store
> these values here rather than didEnterFullScreen since by the time the latter
> Comments might as well wrap to 80 chars. (optional)

[bokan, 2015-12-02 20:11:19]

On 2015/12/01 20:49:44, bokan wrote:
> On 2015/12/01 00:30:18, scheib wrote:
> > Tests aren't in a good position for this span of code logic. Browser tests
> have
> > historically been too flaky using the real browser window and OS calls.
> > Fullscreen logic in browser is tested in unit tests, but not with coverage
of
> > the logic down to Blink. Perhaps ideally we would have layout tests with the
> > browser under test using fake browser windows and simulating the OS level
> > events. That would still have some limitations (are we simulating OS events
> > correctly) but fix robustness. It would require a moderate amount of work to
> > setup custom test harness hooks plumbed to fake browser window behavior. 
> > 
> > That is why historically you'll see manual testing called out on patches
> > affecting this type of code.
> 
> Hmm, Is it possible to write this as a unit test? Really, all I need to test
is
> that exiting a fullscreen tab that causes a size change sends one resize IPC
> with both |fullscreen| and |bounds| modified. Doing a full integration style
> test seems like it would be hard to write in a non-flaky way. Sorry if this
> sounds idealistic, I have little experience at a level higher than Blink :/
> 
> > 
> >
>
https://codereview.chromium.org/1488653002/diff/1/chrome/browser/ui/exclusive...
> > File chrome/browser/ui/exclusive_access/fullscreen_controller.cc (right):
> > 
> >
>
https://codereview.chromium.org/1488653002/diff/1/chrome/browser/ui/exclusive...
> > chrome/browser/ui/exclusive_access/fullscreen_controller.cc:225: // Ensure
web
> > contents exit fullscreen state by sending a resize message,
> > I'm a bit wondering why this isn't part of ::NotifyTabExclusiveAccessLost
and
> > then ExitFullscreen calling ExitFullscreenMode. I haven't traced through yet
> to
> > see why the call is needed to be moved here instead of web_contents. Calling
> > from FullscreenController feels as if the implementation details of
> WebContents
> > are too exposed.
> > 
> >
>
https://codereview.chromium.org/1488653002/diff/1/third_party/WebKit/Source/w...
> > File third_party/WebKit/Source/web/FullscreenController.cpp (right):
> > 
> >
>
https://codereview.chromium.org/1488653002/diff/1/third_party/WebKit/Source/w...
> > third_party/WebKit/Source/web/FullscreenController.cpp:150: // We need to
> store
> > these values here rather than didEnterFullScreen since by the time the
latter
> > Comments might as well wrap to 80 chars. (optional)

Actually, hang on before reviewing the update. It appears not to work on
Windows...sigh.

[bokan, 2015-12-02 23:26:33]

On 2015/12/02 20:11:19, bokan wrote:
> On 2015/12/01 20:49:44, bokan wrote:
> > On 2015/12/01 00:30:18, scheib wrote:
> > > Tests aren't in a good position for this span of code logic. Browser tests
> > have
> > > historically been too flaky using the real browser window and OS calls.
> > > Fullscreen logic in browser is tested in unit tests, but not with coverage
> of
> > > the logic down to Blink. Perhaps ideally we would have layout tests with
the
> > > browser under test using fake browser windows and simulating the OS level
> > > events. That would still have some limitations (are we simulating OS
events
> > > correctly) but fix robustness. It would require a moderate amount of work
to
> > > setup custom test harness hooks plumbed to fake browser window behavior. 
> > > 
> > > That is why historically you'll see manual testing called out on patches
> > > affecting this type of code.
> > 
> > Hmm, Is it possible to write this as a unit test? Really, all I need to test
> is
> > that exiting a fullscreen tab that causes a size change sends one resize IPC
> > with both |fullscreen| and |bounds| modified. Doing a full integration style
> > test seems like it would be hard to write in a non-flaky way. Sorry if this
> > sounds idealistic, I have little experience at a level higher than Blink :/
> > 
> > > 
> > >
> >
>
https://codereview.chromium.org/1488653002/diff/1/chrome/browser/ui/exclusive...
> > > File chrome/browser/ui/exclusive_access/fullscreen_controller.cc (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/1488653002/diff/1/chrome/browser/ui/exclusive...
> > > chrome/browser/ui/exclusive_access/fullscreen_controller.cc:225: // Ensure
> web
> > > contents exit fullscreen state by sending a resize message,
> > > I'm a bit wondering why this isn't part of ::NotifyTabExclusiveAccessLost
> and
> > > then ExitFullscreen calling ExitFullscreenMode. I haven't traced through
yet
> > to
> > > see why the call is needed to be moved here instead of web_contents.
Calling
> > > from FullscreenController feels as if the implementation details of
> > WebContents
> > > are too exposed.
> > > 
> > >
> >
>
https://codereview.chromium.org/1488653002/diff/1/third_party/WebKit/Source/w...
> > > File third_party/WebKit/Source/web/FullscreenController.cpp (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/1488653002/diff/1/third_party/WebKit/Source/w...
> > > third_party/WebKit/Source/web/FullscreenController.cpp:150: // We need to
> > store
> > > these values here rather than didEnterFullScreen since by the time the
> latter
> > > Comments might as well wrap to 80 chars. (optional)
> 
> Actually, hang on before reviewing the update. It appears not to work on
> Windows...sigh.

Never mind, review-away. I was accidentally launching an existing instance of
chrome as I forgot to pass --user-data-dir. It does work on Windows too.

[scheib, 2015-12-03 19:58:57]

OK, I like this direction. Check if you can unit test this with
src/chrome/browser/ui/exclusive_access/fullscreen_controller_state_unittest.cc
-- at quick skim I didn't see which web contents impl it would be using, but if
you can control and verify that mock/fake has the correct ExitFullscreen(bool
will_cause_resize) parameter I think we're good.

[bokan, 2015-12-04 21:29:23]

The CQ bit was checked by bokan@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2015-12-04 21:31:05]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1488653002/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1488653002/40001

[commit-bot: I haz the power, 2015-12-04 21:48:30]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-12-04 21:48:30]

Dry run: Try jobs failed on following builders:
  cast_shell_android on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_andr...)

[bokan, 2015-12-08 23:44:00]

Patchset #4 (id:60001) has been deleted

[bokan, 2015-12-08 23:44:26]

The CQ bit was checked by bokan@chromium.org to run a CQ dry run

[bokan, 2015-12-08 23:44:38]

Added a test, PTAL

[commit-bot: I haz the power, 2015-12-08 23:49:05]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1488653002/80001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1488653002/80001

[commit-bot: I haz the power, 2015-12-09 00:13:56]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-12-09 00:13:57]

Dry run: Try jobs failed on following builders:
  android_arm64_dbg_recipe on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/android_arm64_d...)
  android_chromium_gn_compile_dbg on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/android_chromiu...)
  android_chromium_gn_compile_rel on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/android_chromiu...)
  android_compile_dbg on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/android_compile...)
  cast_shell_android on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_andr...)
  cast_shell_linux on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
  linux_chromium_chromeos_ozone_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_chromeos_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_compile_dbg_32_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_compile_dbg_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_gn_chromeos_rel on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_gn_rel on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_gn_r...)
  mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[bokan, 2015-12-09 13:46:54]

The CQ bit was checked by bokan@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2015-12-09 13:48:43]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1488653002/100001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1488653002/100001

[commit-bot: I haz the power, 2015-12-09 14:11:16]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-12-09 14:11:17]

Dry run: Try jobs failed on following builders:
  android_chromium_gn_compile_rel on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/android_chromiu...)

[bokan, 2015-12-09 14:36:28]

The CQ bit was checked by bokan@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2015-12-09 14:37:26]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1488653002/120001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1488653002/120001

[bokan, 2015-12-09 15:34:33]

Description was changed from

==========
Fix scroll restoration when exiting fullscreen mode.

There's two problems in Chrome/Blink that cause the existing scroll restoration
not to work on desktop platforms:

In Blink, when entering fullscreen mode, FullscreenController saves the current
scroll offset so that it can restore it later. However, this was done in
FullscreenController::didEnterFullScreen, which is called only after WebViewImpl
is resized, meaning that the scroll offset may already be wiped out.

In Chrome, we explicitly send a WasResized message to ensure that the renderer
is notified of the fullscreen change in cases where toggling fullscreen doesn't
actually change the renderer's size (e.g. going from tab fullscreen to browser
fullscreen). However, this means that the renderer gets two separate resize
events for the fullscreen toggle: one that toggles fullscreen and another for
the size change. Thus, Blink tries to restore the scroll offset before the
renderer is resized which might then clamp that scroll offset.

For the first issue, I've moved the scroll offset saving logic into
FullscreenController::enterFullScreenForElement which is called before sending
the request IPC to the browser.

For the second issues, I've moved the explicit WasResized call to occur only
for the tab fullscreen to browser fullscreen case, which is already handled
separately in Chrome's FullscreenController.

BUG=142427
==========

to

==========
Fix scroll restoration when exiting fullscreen mode.

There's two problems in Chrome/Blink that cause the existing scroll restoration
not to work on desktop platforms:

In Blink, when entering fullscreen mode, FullscreenController saves the current
scroll offset so that it can restore it later. However, this was done in
FullscreenController::didEnterFullScreen, which is called only after WebViewImpl
is resized, meaning that the scroll offset may already be wiped out.

In Chrome, we explicitly send a WasResized message to ensure that the renderer
is notified of the fullscreen change in cases where toggling fullscreen doesn't
actually change the renderer's size (e.g. going from tab fullscreen to browser
fullscreen). However, this means that the renderer gets two separate resize
events for the fullscreen toggle: one that toggles fullscreen and another for
the size change. Thus, Blink tries to restore the scroll offset before the
renderer is resized which might then clamp that scroll offset.

For the first issue, I've moved the scroll offset saving logic into
FullscreenController::enterFullScreenForElement which is called before sending
the request IPC to the browser.

For the second issue, I added a will_cause_resize parameter to
WebContents::ExitFullscreen that tells the method whether exiting fullscreen
will
potentially cause a window resize. Based on this, WebContents can choose to
explicitly send the WasResized only in cases where it will not occur otherwise.

BUG=142427
==========

[commit-bot: I haz the power, 2015-12-09 16:53:16]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-12-09 16:53:17]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[bokan, 2015-12-09 20:21:18]

The CQ bit was checked by bokan@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2015-12-09 20:24:10]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1488653002/140001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1488653002/140001

[commit-bot: I haz the power, 2015-12-09 21:26:56]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-12-09 21:26:56]

Dry run: Try jobs failed on following builders:
  linux_chromium_gn_chromeos_rel on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[bokan, 2015-12-14 18:20:19]

Your calendar has you as back from OOO, PTAL if you have a chance.

[bokan, 2016-01-04 18:34:12]

Friendly post-holiday ping

[blink-reviews, 2016-01-04 20:34:59]

OK, see it and sorry for delay. It will be first code review I do after
triaging work items.

On Mon, Jan 4, 2016 at 10:34 AM, <bokan@chromium.org> wrote:

> Friendly post-holiday ping
>
> https://codereview.chromium.org/1488653002/
>

-- 
You received this message because you are subscribed to the Google Groups "Blink
Reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to blink-reviews+unsubscribe@chromium.org.

[chromium-reviews, 2016-01-04 20:35:00]

OK, see it and sorry for delay. It will be first code review I do after
triaging work items.

On Mon, Jan 4, 2016 at 10:34 AM, <bokan@chromium.org> wrote:

> Friendly post-holiday ping
>
> https://codereview.chromium.org/1488653002/
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[scheib, 2016-01-04 22:26:48]

LGTM with suggested comments:

https://codereview.chromium.org/1488653002/diff/140001/chrome/browser/ui/excl...
File chrome/browser/ui/exclusive_access/fullscreen_controller.cc (right):

https://codereview.chromium.org/1488653002/diff/140001/chrome/browser/ui/excl...
chrome/browser/ui/exclusive_access/fullscreen_controller.cc:275:
web_contents->ExitFullscreen(IsFullscreenCausedByTab());
For readability please use the pattern you used below, with the local variable
will_cause_resize.

https://codereview.chromium.org/1488653002/diff/140001/chrome/browser/ui/excl...
chrome/browser/ui/exclusive_access/fullscreen_controller.cc:302:
active_web_contents->ExitFullscreen(IsFullscreenCausedByTab());
For readability please use the pattern you used below, with the local variable
will_cause_resize.

https://codereview.chromium.org/1488653002/diff/140001/chrome/browser/ui/excl...
File chrome/browser/ui/exclusive_access/fullscreen_controller_state_unittest.cc
(right):

https://codereview.chromium.org/1488653002/diff/140001/chrome/browser/ui/excl...
chrome/browser/ui/exclusive_access/fullscreen_controller_state_unittest.cc:766:
// Tests that going from tab fullscreen -> browser fullscreen causes an explicit
Comment here and in body describes how ::WasResized should be called but doesn't
explain that it is the 2nd 'false' parameter to DidToggleFullscreenModeForTab
that will cause that to happen. Add brief note to that effect, probably just in
the body comments.

https://codereview.chromium.org/1488653002/diff/140001/content/browser/frame_...
File content/browser/frame_host/render_frame_host_impl.cc (right):

https://codereview.chromium.org/1488653002/diff/140001/content/browser/frame_...
content/browser/frame_host/render_frame_host_impl.cc:1660:
delegate_->ExitFullscreenMode(true);
/* will_cause_resize */ true

[bokan, 2016-01-05 15:37:11]

https://codereview.chromium.org/1488653002/diff/140001/chrome/browser/ui/excl...
File chrome/browser/ui/exclusive_access/fullscreen_controller.cc (right):

https://codereview.chromium.org/1488653002/diff/140001/chrome/browser/ui/excl...
chrome/browser/ui/exclusive_access/fullscreen_controller.cc:275:
web_contents->ExitFullscreen(IsFullscreenCausedByTab());
On 2016/01/04 22:26:48, scheib wrote:
> For readability please use the pattern you used below, with the local variable
> will_cause_resize.

Done.

https://codereview.chromium.org/1488653002/diff/140001/chrome/browser/ui/excl...
chrome/browser/ui/exclusive_access/fullscreen_controller.cc:302:
active_web_contents->ExitFullscreen(IsFullscreenCausedByTab());
On 2016/01/04 22:26:48, scheib wrote:
> For readability please use the pattern you used below, with the local variable
> will_cause_resize.

Done.

https://codereview.chromium.org/1488653002/diff/140001/chrome/browser/ui/excl...
File chrome/browser/ui/exclusive_access/fullscreen_controller_state_unittest.cc
(right):

https://codereview.chromium.org/1488653002/diff/140001/chrome/browser/ui/excl...
chrome/browser/ui/exclusive_access/fullscreen_controller_state_unittest.cc:766:
// Tests that going from tab fullscreen -> browser fullscreen causes an explicit
On 2016/01/04 22:26:48, scheib wrote:
> Comment here and in body describes how ::WasResized should be called but
doesn't
> explain that it is the 2nd 'false' parameter to DidToggleFullscreenModeForTab
> that will cause that to happen. Add brief note to that effect, probably just
in
> the body comments.

Done.

https://codereview.chromium.org/1488653002/diff/140001/content/browser/frame_...
File content/browser/frame_host/render_frame_host_impl.cc (right):

https://codereview.chromium.org/1488653002/diff/140001/content/browser/frame_...
content/browser/frame_host/render_frame_host_impl.cc:1660:
delegate_->ExitFullscreenMode(true);
On 2016/01/04 22:26:48, scheib wrote:
> /* will_cause_resize */ true

Done.

[bokan, 2016-01-05 15:38:10]

Patchset #8 (id:160001) has been deleted

[bokan, 2016-01-05 15:46:34]

bokan@chromium.org changed reviewers:
+ avi@chromium.org, lfg@chromium.org, pkasting@chromium.org, rockot@chromium.org

[bokan, 2016-01-05 15:46:34]

Adding OWNERs:

+lfg@ for guest_view_base.cc
+rockot@ for tab_capture_registry.cc
+pkasting@ for chrome/browser/ui/*
+avi@ for content/* and ui/*

[bokan, 2016-01-05 15:49:15]

Description was changed from

==========
Fix scroll restoration when exiting fullscreen mode.

There's two problems in Chrome/Blink that cause the existing scroll restoration
not to work on desktop platforms:

In Blink, when entering fullscreen mode, FullscreenController saves the current
scroll offset so that it can restore it later. However, this was done in
FullscreenController::didEnterFullScreen, which is called only after WebViewImpl
is resized, meaning that the scroll offset may already be wiped out.

In Chrome, we explicitly send a WasResized message to ensure that the renderer
is notified of the fullscreen change in cases where toggling fullscreen doesn't
actually change the renderer's size (e.g. going from tab fullscreen to browser
fullscreen). However, this means that the renderer gets two separate resize
events for the fullscreen toggle: one that toggles fullscreen and another for
the size change. Thus, Blink tries to restore the scroll offset before the
renderer is resized which might then clamp that scroll offset.

For the first issue, I've moved the scroll offset saving logic into
FullscreenController::enterFullScreenForElement which is called before sending
the request IPC to the browser.

For the second issue, I added a will_cause_resize parameter to
WebContents::ExitFullscreen that tells the method whether exiting fullscreen
will
potentially cause a window resize. Based on this, WebContents can choose to
explicitly send the WasResized only in cases where it will not occur otherwise.

BUG=142427
==========

to

==========
Fix scroll restoration when exiting fullscreen mode.

There's two problems in Chrome/Blink that cause the existing scroll restoration
not to work on desktop platforms:

In Blink, when entering fullscreen mode, FullscreenController saves the current
scroll offset so that it can restore it later. However, this was done in
FullscreenController::didEnterFullScreen, which is called only after WebViewImpl
is resized, meaning that the scroll offset may already be wiped out.

In Chrome, we explicitly send a WasResized message to ensure that the renderer
is notified of the fullscreen change in cases where toggling fullscreen doesn't
actually change the renderer's size (e.g. going from tab fullscreen to browser
fullscreen). However, this means that the renderer gets two separate resize
events for the fullscreen toggle: one that toggles fullscreen and another for
the size change. Thus, Blink tries to restore the scroll offset before the
renderer is resized which might then clamp that scroll offset.

For the first issue, I've moved the scroll offset saving logic into
FullscreenController::enterFullScreenForElement which is called before sending
the request IPC to the browser.

For the second issue, I added a will_cause_resize parameter to
WebContents::ExitFullscreen that tells the method whether exiting fullscreen
will
potentially cause a window resize. Based on this, WebContents can choose to
explicitly send the WasResized only in cases where it will not occur otherwise.
This ensures the Resize message includes both the fullscreen state change and
size change in one message.

BUG=142427
==========

[lfg, 2016-01-05 16:10:33]

guest_view_base.cc lgtm

[Avi (use Gerrit), 2016-01-05 16:17:57]

https://codereview.chromium.org/1488653002/diff/200001/chrome/browser/ui/excl...
File chrome/browser/ui/exclusive_access/fullscreen_controller_state_unittest.cc
(right):

https://codereview.chromium.org/1488653002/diff/200001/chrome/browser/ui/excl...
chrome/browser/ui/exclusive_access/fullscreen_controller_state_unittest.cc:762: 
nit: no need for a newline here

https://codereview.chromium.org/1488653002/diff/200001/chrome/browser/ui/excl...
chrome/browser/ui/exclusive_access/fullscreen_controller_state_unittest.cc:779:
// should call WasResized since the fullscreen change wont cause a size change
s/wont/won't/

https://codereview.chromium.org/1488653002/diff/200001/content/browser/frame_...
File content/browser/frame_host/render_frame_host_delegate.h (right):

https://codereview.chromium.org/1488653002/diff/200001/content/browser/frame_...
content/browser/frame_host/render_frame_host_delegate.h:156: //
|will_cause_resize| can be false in the case where we exit tab fullscreen
"can be false"? Please write a comment about when a parameter *will* or *will
not* be true or false, rather than when it might or might not.

Perhaps, "|will_cause_resize| will be false in the case..."

https://codereview.chromium.org/1488653002/diff/200001/content/browser/web_co...
File content/browser/web_contents/web_contents_impl.cc (right):

https://codereview.chromium.org/1488653002/diff/200001/content/browser/web_co...
content/browser/web_contents/web_contents_impl.cc:1688: // hence the explicit
call here is required.
Does this comment need a change? It talks about always sending a resize message,
while the block of code below is now guarded by an if().

https://codereview.chromium.org/1488653002/diff/200001/content/public/browser...
File content/public/browser/web_contents.h (right):

https://codereview.chromium.org/1488653002/diff/200001/content/public/browser...
content/public/browser/web_contents.h:672: // |will_cause_resize| can be false
in the case where we exit tab fullscreen
Same comment here. "will be false in the case"?

[Ken Rockot(use gerrit already), 2016-01-05 16:47:54]

tab_capture_registry.cc lgtm

[bokan, 2016-01-05 17:15:48]

https://codereview.chromium.org/1488653002/diff/200001/chrome/browser/ui/excl...
File chrome/browser/ui/exclusive_access/fullscreen_controller_state_unittest.cc
(right):

https://codereview.chromium.org/1488653002/diff/200001/chrome/browser/ui/excl...
chrome/browser/ui/exclusive_access/fullscreen_controller_state_unittest.cc:762: 
On 2016/01/05 16:17:57, Avi wrote:
> nit: no need for a newline here

Done.

https://codereview.chromium.org/1488653002/diff/200001/chrome/browser/ui/excl...
chrome/browser/ui/exclusive_access/fullscreen_controller_state_unittest.cc:779:
// should call WasResized since the fullscreen change wont cause a size change
On 2016/01/05 16:17:57, Avi wrote:
> s/wont/won't/

Done.

https://codereview.chromium.org/1488653002/diff/200001/content/browser/frame_...
File content/browser/frame_host/render_frame_host_delegate.h (right):

https://codereview.chromium.org/1488653002/diff/200001/content/browser/frame_...
content/browser/frame_host/render_frame_host_delegate.h:156: //
|will_cause_resize| can be false in the case where we exit tab fullscreen
On 2016/01/05 16:17:57, Avi wrote:
> "can be false"? Please write a comment about when a parameter *will* or *will
> not* be true or false, rather than when it might or might not.
> 
> Perhaps, "|will_cause_resize| will be false in the case..."

Thanks, I've reworded the whole thing to be more clear.

https://codereview.chromium.org/1488653002/diff/200001/content/browser/web_co...
File content/browser/web_contents/web_contents_impl.cc (right):

https://codereview.chromium.org/1488653002/diff/200001/content/browser/web_co...
content/browser/web_contents/web_contents_impl.cc:1688: // hence the explicit
call here is required.
On 2016/01/05 16:17:57, Avi wrote:
> Does this comment need a change? It talks about always sending a resize
message,
> while the block of code below is now guarded by an if().

I've made it clearer that this is only for the case where the view size doesn't
change.

https://codereview.chromium.org/1488653002/diff/200001/content/public/browser...
File content/public/browser/web_contents.h (right):

https://codereview.chromium.org/1488653002/diff/200001/content/public/browser...
content/public/browser/web_contents.h:672: // |will_cause_resize| can be false
in the case where we exit tab fullscreen
On 2016/01/05 16:17:57, Avi wrote:
> Same comment here. "will be false in the case"?

Done.

[Avi (use Gerrit), 2016-01-05 17:24:40]

Thanks! LGTM

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

LGTM

https://codereview.chromium.org/1488653002/diff/220001/chrome/browser/ui/excl...
File chrome/browser/ui/exclusive_access/fullscreen_controller.cc (right):

https://codereview.chromium.org/1488653002/diff/220001/chrome/browser/ui/excl...
chrome/browser/ui/exclusive_access/fullscreen_controller.cc:277:
web_contents->ExitFullscreen(will_cause_resize);
Nit: Inline:

    web_contents->ExitFullscreen(IsFullscreenCausedByTab());

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

https://codereview.chromium.org/1488653002/diff/220001/chrome/browser/ui/excl...
chrome/browser/ui/exclusive_access/fullscreen_controller.cc:305:
active_web_contents->ExitFullscreen(will_cause_resize);
Nit: Inline as above.

https://codereview.chromium.org/1488653002/diff/220001/chrome/browser/ui/excl...
chrome/browser/ui/exclusive_access/fullscreen_controller.cc:307: } else if
(IsWindowFullscreenForTabOrPending()) {
Nit: No "else" after "return".

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

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

https://codereview.chromium.org/1488653002/diff/220001/chrome/browser/ui/excl...
chrome/browser/ui/exclusive_access/fullscreen_controller.cc:417:
web_contents->ExitFullscreen(will_cause_resize);
Nit: Inline as above.

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

https://codereview.chromium.org/1488653002/diff/220001/chrome/browser/ui/excl...
File chrome/browser/ui/exclusive_access/fullscreen_controller.cc (right):

https://codereview.chromium.org/1488653002/diff/220001/chrome/browser/ui/excl...
chrome/browser/ui/exclusive_access/fullscreen_controller.cc:277:
web_contents->ExitFullscreen(will_cause_resize);
On 2016/01/05 20:34:26, Peter Kasting wrote:
> Nit: Inline:
> 
>     web_contents->ExitFullscreen(IsFullscreenCausedByTab());
> 
> Then remove the {} on the arms of this conditional.

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?

https://codereview.chromium.org/1488653002/diff/220001/chrome/browser/ui/excl...
chrome/browser/ui/exclusive_access/fullscreen_controller.cc:307: } else if
(IsWindowFullscreenForTabOrPending()) {
On 2016/01/05 20:34:26, Peter Kasting wrote:
> Nit: No "else" after "return".
> 
> Can reverse this conditional to be able to remove {}:
> 
>   if (!IsWindowFullscreenForTabOrPending())
>     return false;
>   ExitExclusiveAccessIfNecessary();
>   return true;

Done.

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

https://codereview.chromium.org/1488653002/diff/220001/chrome/browser/ui/excl...
File chrome/browser/ui/exclusive_access/fullscreen_controller.cc (right):

https://codereview.chromium.org/1488653002/diff/220001/chrome/browser/ui/excl...
chrome/browser/ui/exclusive_access/fullscreen_controller.cc:277:
web_contents->ExitFullscreen(will_cause_resize);
On 2016/01/06 17:48:32, bokan wrote:
> On 2016/01/05 20:34:26, Peter Kasting wrote:
> > Nit: Inline:
> > 
> >     web_contents->ExitFullscreen(IsFullscreenCausedByTab());
> > 
> > Then remove the {} on the arms of this conditional.
> 
> 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?

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]

https://codereview.chromium.org/1488653002/diff/220001/chrome/browser/ui/excl...
File chrome/browser/ui/exclusive_access/fullscreen_controller.cc (right):

https://codereview.chromium.org/1488653002/diff/220001/chrome/browser/ui/excl...
chrome/browser/ui/exclusive_access/fullscreen_controller.cc:277:
web_contents->ExitFullscreen(will_cause_resize);
On 2016/01/06 20:00:28, Peter Kasting wrote:
> On 2016/01/06 17:48:32, bokan wrote:
> > On 2016/01/05 20:34:26, Peter Kasting wrote:
> > > Nit: Inline:
> > > 
> > >     web_contents->ExitFullscreen(IsFullscreenCausedByTab());
> > > 
> > > Then remove the {} on the arms of this conditional.
> > 
> > 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?
> 
> 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.

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]

https://codereview.chromium.org/1488653002/diff/220001/chrome/browser/ui/excl...
File chrome/browser/ui/exclusive_access/fullscreen_controller.cc (right):

https://codereview.chromium.org/1488653002/diff/220001/chrome/browser/ui/excl...
chrome/browser/ui/exclusive_access/fullscreen_controller.cc:277:
web_contents->ExitFullscreen(will_cause_resize);
On 2016/01/06 21:21:01, scheib wrote:
> On 2016/01/06 20:00:28, Peter Kasting wrote:
> > On 2016/01/06 17:48:32, bokan wrote:
> > > On 2016/01/05 20:34:26, Peter Kasting wrote:
> > > > Nit: Inline:
> > > > 
> > > >     web_contents->ExitFullscreen(IsFullscreenCausedByTab());
> > > > 
> > > > Then remove the {} on the arms of this conditional.
> > > 
> > > 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?
> > 
> > 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.
> 
> 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.

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]

https://codereview.chromium.org/1488653002/diff/220001/chrome/browser/ui/excl...
File chrome/browser/ui/exclusive_access/fullscreen_controller.cc (right):

https://codereview.chromium.org/1488653002/diff/220001/chrome/browser/ui/excl...
chrome/browser/ui/exclusive_access/fullscreen_controller.cc:277:
web_contents->ExitFullscreen(will_cause_resize);
On 2016/01/06 21:26:04, Peter Kasting wrote:
> On 2016/01/06 21:21:01, scheib wrote:
> > On 2016/01/06 20:00:28, Peter Kasting wrote:
> > > On 2016/01/06 17:48:32, bokan wrote:
> > > > On 2016/01/05 20:34:26, Peter Kasting wrote:
> > > > > Nit: Inline:
> > > > > 
> > > > >     web_contents->ExitFullscreen(IsFullscreenCausedByTab());
> > > > > 
> > > > > Then remove the {} on the arms of this conditional.
> > > > 
> > > > 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?
> > > 
> > > 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.
> > 
> > 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.
> 
> 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?

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. 

> 
> 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.

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]

https://codereview.chromium.org/1488653002/diff/220001/chrome/browser/ui/excl...
File chrome/browser/ui/exclusive_access/fullscreen_controller.cc (right):

https://codereview.chromium.org/1488653002/diff/220001/chrome/browser/ui/excl...
chrome/browser/ui/exclusive_access/fullscreen_controller.cc:277:
web_contents->ExitFullscreen(will_cause_resize);
On 2016/01/06 21:40:57, bokan wrote:
> On 2016/01/06 21:26:04, Peter Kasting wrote:
> > On 2016/01/06 21:21:01, scheib wrote:
> > > On 2016/01/06 20:00:28, Peter Kasting wrote:
> > > > On 2016/01/06 17:48:32, bokan wrote:
> > > > > On 2016/01/05 20:34:26, Peter Kasting wrote:
> > > > > > Nit: Inline:
> > > > > > 
> > > > > >     web_contents->ExitFullscreen(IsFullscreenCausedByTab());
> > > > > > 
> > > > > > Then remove the {} on the arms of this conditional.
> > > > > 
> > > > > 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?
> > > > 
> > > > 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.
> > > 
> > > 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.
> > 
> > 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?
> 
> I'm personally one such person who doesn't have this ability in my tools.

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?

[bokan, 2016-01-06 22:01:17]

On 2016/01/06 21:56:41, Peter Kasting wrote:
>
https://codereview.chromium.org/1488653002/diff/220001/chrome/browser/ui/excl...
> File chrome/browser/ui/exclusive_access/fullscreen_controller.cc (right):
> 
>
https://codereview.chromium.org/1488653002/diff/220001/chrome/browser/ui/excl...
> chrome/browser/ui/exclusive_access/fullscreen_controller.cc:277:
> web_contents->ExitFullscreen(will_cause_resize);
> On 2016/01/06 21:40:57, bokan wrote:
> > On 2016/01/06 21:26:04, Peter Kasting wrote:
> > > On 2016/01/06 21:21:01, scheib wrote:
> > > > On 2016/01/06 20:00:28, Peter Kasting wrote:
> > > > > On 2016/01/06 17:48:32, bokan wrote:
> > > > > > On 2016/01/05 20:34:26, Peter Kasting wrote:
> > > > > > > Nit: Inline:
> > > > > > > 
> > > > > > >     web_contents->ExitFullscreen(IsFullscreenCausedByTab());
> > > > > > > 
> > > > > > > Then remove the {} on the arms of this conditional.
> > > > > > 
> > > > > > 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?
> > > > > 
> > > > > 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.
> > > > 
> > > > 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.
> > > 
> > > 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?
> > 
> > I'm personally one such person who doesn't have this ability in my tools.
> 
> 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?

That I have, I even have a keyboard shortcut to take me to the current line, but
that breaks me out of my editor and into a new workflow. I wouldn't call it
"near-instantaneous". I don't consider it a big-deal but I'd put it in the same
class of issues as things like line-break conventions - tiny conventions that
together aim to increase comprehension speed and reduce cognitive burden.

[Peter Kasting, 2016-01-06 22:05:09]

Well, this isn't something where the style guide has guidance, so I'm definitely
not going to block the review on it; you have my LGTM already.

My opinion is still that the code is more idiomatic and reads better without the
temp or the param name as a comment, and that adding it doesn't really give the
reader much information, certainly not enough if they're in the situation of
needing to know what the param means to begin with.  So I would remove it. 
Given that argument, use your best judgment as to how to proceed.

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

https://codereview.chromium.org/1488653002/diff/220001/chrome/browser/ui/excl...
File chrome/browser/ui/exclusive_access/fullscreen_controller.cc (right):

https://codereview.chromium.org/1488653002/diff/220001/chrome/browser/ui/excl...
chrome/browser/ui/exclusive_access/fullscreen_controller.cc:277:
web_contents->ExitFullscreen(will_cause_resize);
On 2016/01/06 21:56:41, Peter Kasting wrote:
> On 2016/01/06 21:40:57, bokan wrote:
> > On 2016/01/06 21:26:04, Peter Kasting wrote:
> > > On 2016/01/06 21:21:01, scheib wrote:
> > > > On 2016/01/06 20:00:28, Peter Kasting wrote:
> > > > > On 2016/01/06 17:48:32, bokan wrote:
> > > > > > On 2016/01/05 20:34:26, Peter Kasting wrote:
> > > > > > > Nit: Inline:
> > > > > > > 
> > > > > > >     web_contents->ExitFullscreen(IsFullscreenCausedByTab());
> > > > > > > 
> > > > > > > Then remove the {} on the arms of this conditional.
> > > > > > 
> > > > > > 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?
> > > > > 
> > > > > 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.
> > > > 
> > > > 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.
> > > 
> > > 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?
> > 
> > I'm personally one such person who doesn't have this ability in my tools.
> 
> 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?

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".

[Peter Kasting, 2016-01-06 22:11:06]

On 2016/01/06 22:06:56, scheib wrote:
> Peter please say "Just do it this way" or say "Bokan use your judement".

Already done :)

[bokan, 2016-01-06 22:16:55]

I think scheib@'s compromise strikes a good balance so I'll go with that :).
Thanks folks!

[bokan, 2016-01-06 22:17:05]

The CQ bit was checked by bokan@chromium.org

[bokan, 2016-01-06 22:17:05]

The patchset sent to the CQ was uploaded after l-g-t-m from scheib@chromium.org,
lfg@chromium.org, rockot@chromium.org, avi@chromium.org, pkasting@chromium.org
Link to the patchset: https://codereview.chromium.org/1488653002/#ps260001
(title: "Nit: Inlined arguments in fullscreen_controller")

[commit-bot: I haz the power, 2016-01-06 22:17:15]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1488653002/260001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1488653002/260001

[commit-bot: I haz the power, 2016-01-06 23:23:21]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-01-06 23:23:22]

Try jobs failed on following builders:
  linux_chromium_chromeos_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[bokan, 2016-01-27 18:27:24]

Description was changed from

==========
Fix scroll restoration when exiting fullscreen mode.

There's two problems in Chrome/Blink that cause the existing scroll restoration
not to work on desktop platforms:

In Blink, when entering fullscreen mode, FullscreenController saves the current
scroll offset so that it can restore it later. However, this was done in
FullscreenController::didEnterFullScreen, which is called only after WebViewImpl
is resized, meaning that the scroll offset may already be wiped out.

In Chrome, we explicitly send a WasResized message to ensure that the renderer
is notified of the fullscreen change in cases where toggling fullscreen doesn't
actually change the renderer's size (e.g. going from tab fullscreen to browser
fullscreen). However, this means that the renderer gets two separate resize
events for the fullscreen toggle: one that toggles fullscreen and another for
the size change. Thus, Blink tries to restore the scroll offset before the
renderer is resized which might then clamp that scroll offset.

For the first issue, I've moved the scroll offset saving logic into
FullscreenController::enterFullScreenForElement which is called before sending
the request IPC to the browser.

For the second issue, I added a will_cause_resize parameter to
WebContents::ExitFullscreen that tells the method whether exiting fullscreen
will
potentially cause a window resize. Based on this, WebContents can choose to
explicitly send the WasResized only in cases where it will not occur otherwise.
This ensures the Resize message includes both the fullscreen state change and
size change in one message.

BUG=142427
==========

to

==========
Fix scroll restoration when exiting fullscreen mode.

There's two problems in Chrome/Blink that cause the existing scroll restoration
not to work on desktop platforms:

In Blink, when entering fullscreen mode, FullscreenController saves the current
scroll offset so that it can restore it later. However, this was done in
FullscreenController::didEnterFullScreen, which is called only after WebViewImpl
is resized, meaning that the scroll offset may already be wiped out.

In Chrome, we explicitly send a WasResized message to ensure that the renderer
is notified of the fullscreen change in cases where toggling fullscreen doesn't
actually change the renderer's size (e.g. going from tab fullscreen to browser
fullscreen). However, this means that the renderer gets two separate resize
events for the fullscreen toggle: one that toggles fullscreen and another for
the size change. Thus, Blink tries to restore the scroll offset before the
renderer is resized which might then clamp that scroll offset.

For the first issue, I've moved the scroll offset saving logic into
FullscreenController::enterFullScreenForElement which is called before sending
the request IPC to the browser.

For the second issue, I added a will_cause_resize parameter to
WebContents::ExitFullscreen that tells the method whether exiting fullscreen
will
potentially cause a window resize. Based on this, WebContents can choose to
explicitly send the WasResized only in cases where it will not occur otherwise.
This ensures the Resize message includes both the fullscreen state change and
size change in one message.

BUG=142427
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
==========

[bokan, 2016-01-28 13:46:10]

The CQ bit was checked by bokan@chromium.org

[bokan, 2016-01-28 13:46:10]

The patchset sent to the CQ was uploaded after l-g-t-m from scheib@chromium.org,
avi@chromium.org, lfg@chromium.org, pkasting@chromium.org, rockot@chromium.org
Link to the patchset: https://codereview.chromium.org/1488653002/#ps340001
(title: "Accidentally made will_cause_resize always false in previous cleanup.
Fixed")

[commit-bot: I haz the power, 2016-01-28 13:46:53]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1488653002/340001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1488653002/340001

[commit-bot: I haz the power, 2016-01-28 16:23:27]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-01-28 16:23:28]

Try jobs failed on following builders:
  linux_chromium_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[bokan, 2016-01-28 16:24:05]

The CQ bit was checked by bokan@chromium.org

[commit-bot: I haz the power, 2016-01-28 16:25:15]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1488653002/340001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1488653002/340001

[commit-bot: I haz the power, 2016-01-28 19:49:56]

Committed patchset #16 (id:340001)

[commit-bot: I haz the power, 2016-01-28 19:50:56]

Description was changed from

==========
Fix scroll restoration when exiting fullscreen mode.

There's two problems in Chrome/Blink that cause the existing scroll restoration
not to work on desktop platforms:

In Blink, when entering fullscreen mode, FullscreenController saves the current
scroll offset so that it can restore it later. However, this was done in
FullscreenController::didEnterFullScreen, which is called only after WebViewImpl
is resized, meaning that the scroll offset may already be wiped out.

In Chrome, we explicitly send a WasResized message to ensure that the renderer
is notified of the fullscreen change in cases where toggling fullscreen doesn't
actually change the renderer's size (e.g. going from tab fullscreen to browser
fullscreen). However, this means that the renderer gets two separate resize
events for the fullscreen toggle: one that toggles fullscreen and another for
the size change. Thus, Blink tries to restore the scroll offset before the
renderer is resized which might then clamp that scroll offset.

For the first issue, I've moved the scroll offset saving logic into
FullscreenController::enterFullScreenForElement which is called before sending
the request IPC to the browser.

For the second issue, I added a will_cause_resize parameter to
WebContents::ExitFullscreen that tells the method whether exiting fullscreen
will
potentially cause a window resize. Based on this, WebContents can choose to
explicitly send the WasResized only in cases where it will not occur otherwise.
This ensures the Resize message includes both the fullscreen state change and
size change in one message.

BUG=142427
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Fix scroll restoration when exiting fullscreen mode.

There's two problems in Chrome/Blink that cause the existing scroll restoration
not to work on desktop platforms:

In Blink, when entering fullscreen mode, FullscreenController saves the current
scroll offset so that it can restore it later. However, this was done in
FullscreenController::didEnterFullScreen, which is called only after WebViewImpl
is resized, meaning that the scroll offset may already be wiped out.

In Chrome, we explicitly send a WasResized message to ensure that the renderer
is notified of the fullscreen change in cases where toggling fullscreen doesn't
actually change the renderer's size (e.g. going from tab fullscreen to browser
fullscreen). However, this means that the renderer gets two separate resize
events for the fullscreen toggle: one that toggles fullscreen and another for
the size change. Thus, Blink tries to restore the scroll offset before the
renderer is resized which might then clamp that scroll offset.

For the first issue, I've moved the scroll offset saving logic into
FullscreenController::enterFullScreenForElement which is called before sending
the request IPC to the browser.

For the second issue, I added a will_cause_resize parameter to
WebContents::ExitFullscreen that tells the method whether exiting fullscreen
will
potentially cause a window resize. Based on this, WebContents can choose to
explicitly send the WasResized only in cases where it will not occur otherwise.
This ensures the Resize message includes both the fullscreen state change and
size change in one message.

BUG=142427
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation

Committed: https://crrev.com/ece34a8262550f0c24f9883c5b4eb903b270aa17
Cr-Commit-Position: refs/heads/master@{#372146}
==========

[commit-bot: I haz the power, 2016-01-28 19:50:57]

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