Replace hide-scrollbars flag with a web setting.

Replace hide-scrollbars flag with a web setting.

Instead of the composited/non-composited distinction, we now enforce this purely within in Blink in three places: PaintLayerScrollableArea, FrameView, VisualViewport.

Also updates the use of the flag in chromecast/.

BUG=639806
,internal b/29253042
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2

Committed: https://crrev.com/61ff9143240cc8bc02688b5d84cc939cb14e7685
Cr-Commit-Position: refs/heads/master@{#420766}

[Eric Seckler, 2016-09-14 09:35:04]

Description was changed from

==========
Replace hide-scrollbars flag with a web setting.

BUG=639806
==========

to

==========
Replace hide-scrollbars flag with a web setting.

BUG=639806
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

[Eric Seckler, 2016-09-14 09:35:44]

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

[commit-bot: I haz the power, 2016-09-14 09:36:05]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-14 09:56:00]

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

[commit-bot: I haz the power, 2016-09-14 09:56:01]

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

[Eric Seckler, 2016-09-14 16:05:20]

Description was changed from

==========
Replace hide-scrollbars flag with a web setting.

BUG=639806
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

to

==========
Replace hide-scrollbars flag with a web setting.

BUG=639806,internal b/29253042
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

[Eric Seckler, 2016-09-14 16:06:01]

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

[commit-bot: I haz the power, 2016-09-14 16:06:20]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-14 16:14:25]

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

[commit-bot: I haz the power, 2016-09-14 16:14:26]

Dry run: Try jobs failed on following builders:
  cast_shell_android on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...)
  cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)

[Eric Seckler, 2016-09-14 16:21:03]

Patchset #2 (id:20001) has been deleted

[Eric Seckler, 2016-09-14 16:22:10]

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

[commit-bot: I haz the power, 2016-09-14 16:22:33]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Eric Seckler, 2016-09-14 16:57:49]

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

[commit-bot: I haz the power, 2016-09-14 16:58:09]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Eric Seckler, 2016-09-14 17:02:48]

Description was changed from

==========
Replace hide-scrollbars flag with a web setting.

BUG=639806,internal b/29253042
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

to

==========
Replace hide-scrollbars flag with a web setting.

Instead of the composited/non-composited distinction, we now enforce this purely
within in Blink in three places: PaintLayerScrollableArea, FrameView,
VisualViewport.

Also updates the use of the flag in chromecast/.

BUG=639806,internal b/29253042
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

[Eric Seckler, 2016-09-14 17:04:20]

eseckler@chromium.org changed reviewers:
+ skobes@chromium.org

[Eric Seckler, 2016-09-14 17:04:21]

Hi Steve, PTAL if overall approach is fine. Thanks! :)

[commit-bot: I haz the power, 2016-09-14 18:36:03]

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

[commit-bot: I haz the power, 2016-09-14 18:36:05]

Dry run: This issue passed the CQ dry run.

[skobes, 2016-09-16 01:07:26]

Does this need to be a setting?  The bug says "within specific WebContents" but
as long as it is per-renderer and doesn't change dynamically it can be a
RuntimeEnabledFeature.

https://codereview.chromium.org/2339873002/diff/60001/content/public/common/w...
File content/public/common/web_preferences.h (right):

https://codereview.chromium.org/2339873002/diff/60001/content/public/common/w...
content/public/common/web_preferences.h:130: bool hide_scrollbars;
Where is this consumed?

[Eric Seckler, 2016-09-16 07:54:22]

On 2016/09/16 01:07:26, skobes wrote:
> Does this need to be a setting?  The bug says "within specific WebContents"
but
> as long as it is per-renderer and doesn't change dynamically it can be a
> RuntimeEnabledFeature.

I'd like a setting per-WebContents (i.e. per RenderView). It wouldn't have to
support dynamic enable/disable. AFAICT a REF is static per renderer process,
thus potentially shared by multiple views?

https://codereview.chromium.org/2339873002/diff/60001/content/public/common/w...
File content/public/common/web_preferences.h (right):

https://codereview.chromium.org/2339873002/diff/60001/content/public/common/w...
content/public/common/web_preferences.h:130: bool hide_scrollbars;
On 2016/09/16 01:07:26, skobes wrote:
> Where is this consumed?

Thanks, was missing the plumbing in RenderViewImpl and the
IPC_STRUCT_TRAITS_MEMBER.

[Eric Seckler, 2016-09-16 08:21:08]

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

[commit-bot: I haz the power, 2016-09-16 08:21:16]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-16 09:56:29]

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

[commit-bot: I haz the power, 2016-09-16 09:56:30]

Dry run: This issue passed the CQ dry run.

[skobes, 2016-09-16 17:28:40]

lgtm

On 2016/09/16 07:54:22, Eric Seckler wrote:
> I'd like a setting per-WebContents (i.e. per RenderView). It wouldn't have to
> support dynamic enable/disable. AFAICT a REF is static per renderer process,
> thus potentially shared by multiple views?

Ah, right... you do need a setting then.

[Eric Seckler, 2016-09-17 07:17:23]

eseckler@chromium.org changed reviewers:
+ dcheng@chromium.org, halliwell@chromium.org, piman@chromium.org,
sky@chromium.org

[Eric Seckler, 2016-09-17 07:17:24]

+sky@ for ui/native_theme, WebKit/platform, WebKit/web.
+piman@ for content/
+halliwell@ for chromecast/
+dcheng@ for security review of
content/public/common/common_param_traits_macros.h

Thanks everyone!

[halliwell, 2016-09-17 16:06:36]

On 2016/09/17 07:17:24, Eric Seckler wrote:
> +sky@ for ui/native_theme, WebKit/platform, WebKit/web.
> +piman@ for content/
> +halliwell@ for chromecast/
> +dcheng@ for security review of
> content/public/common/common_param_traits_macros.h
> 
> Thanks everyone!

chromecast/ lgtm

[sky, 2016-09-18 16:12:08]

ui/native_theme LGTM. I'm not a WebKit owner though, you need to get a local
owner for the WebKit changes.

[Eric Seckler, 2016-09-19 08:51:45]

eseckler@chromium.org changed reviewers:
+ pdr@chromium.org

[Eric Seckler, 2016-09-19 08:51:46]

On 2016/09/18 16:12:08, sky wrote:
> ui/native_theme LGTM. I'm not a WebKit owner though, you need to get a local
> owner for the WebKit changes.

Makes sense, +pdr@ for Webkit. Thanks! :)

[dcheng, 2016-09-19 10:04:47]

ipc changes lgtm but one blink question

https://codereview.chromium.org/2339873002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/paint/PaintLayerScrollableArea.cpp (right):

https://codereview.chromium.org/2339873002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/paint/PaintLayerScrollableArea.cpp:1054: if
(box().frame()->settings() && box().frame()->settings()->hideScrollbars())
Is this null check of settings() required? We do call this during document
shutdown, but at that point, I think the Settings object should still be
non-null.

[skobes, 2016-09-19 18:37:22]

https://codereview.chromium.org/2339873002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/paint/PaintLayerScrollableArea.cpp (right):

https://codereview.chromium.org/2339873002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/paint/PaintLayerScrollableArea.cpp:1054: if
(box().frame()->settings() && box().frame()->settings()->hideScrollbars())
On 2016/09/19 10:04:47, dcheng wrote:
> Is this null check of settings() required? We do call this during document
> shutdown, but at that point, I think the Settings object should still be
> non-null.

The question is whether Frame::m_host can ever be null at this point.  I am not
quite sure, but we do these checks in a lot of places and Frame::settings() has
the comment "can be null".

[dcheng, 2016-09-19 19:25:12]

On 2016/09/19 18:37:22, skobes wrote:
>
https://codereview.chromium.org/2339873002/diff/80001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/core/paint/PaintLayerScrollableArea.cpp
(right):
> 
>
https://codereview.chromium.org/2339873002/diff/80001/third_party/WebKit/Sour...
> third_party/WebKit/Source/core/paint/PaintLayerScrollableArea.cpp:1054: if
> (box().frame()->settings() && box().frame()->settings()->hideScrollbars())
> On 2016/09/19 10:04:47, dcheng wrote:
> > Is this null check of settings() required? We do call this during document
> > shutdown, but at that point, I think the Settings object should still be
> > non-null.
> 
> The question is whether Frame::m_host can ever be null at this point.  I am
not
> quite sure, but we do these checks in a lot of places and Frame::settings()
has
> the comment "can be null".

Right, but I think that in this case, it can't be: settings() will start
returning null when a frame is detached. However, we shouldn't be running layout
on a detached frames (and we clean this up before we null out that pointer), and
thus, we (should) never hit this code.

[skobes, 2016-09-19 19:31:20]

https://codereview.chromium.org/2339873002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/paint/PaintLayerScrollableArea.cpp (right):

https://codereview.chromium.org/2339873002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/paint/PaintLayerScrollableArea.cpp:1054: if
(box().frame()->settings() && box().frame()->settings()->hideScrollbars())
On 2016/09/19 18:37:22, skobes wrote:
> The question is whether Frame::m_host can ever be null at this point.  I am
not
> quite sure, but we do these checks in a lot of places and Frame::settings()
has
> the comment "can be null".

Ok, let's change the null checks to DCHECKs.

[piman, 2016-09-19 20:21:57]

lgtm

[pdr., 2016-09-19 20:27:33]

On 2016/09/19 at 20:21:57, piman wrote:
> lgtm

LGTM

[Eric Seckler, 2016-09-22 10:11:11]

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

[commit-bot: I haz the power, 2016-09-22 10:11:25]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-22 10:14:01]

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

[commit-bot: I haz the power, 2016-09-22 10:14:02]

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

[Eric Seckler, 2016-09-22 10:14:20]

Patchset #5 (id:100001) has been deleted

[Eric Seckler, 2016-09-22 10:40:37]

Patchset #5 (id:120001) has been deleted

[Eric Seckler, 2016-09-22 10:50:11]

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

[commit-bot: I haz the power, 2016-09-22 10:50:17]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Eric Seckler, 2016-09-22 12:41:56]

https://codereview.chromium.org/2339873002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/paint/PaintLayerScrollableArea.cpp (right):

https://codereview.chromium.org/2339873002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/paint/PaintLayerScrollableArea.cpp:1054: if
(box().frame()->settings() && box().frame()->settings()->hideScrollbars())
On 2016/09/19 19:31:20, skobes wrote:
> On 2016/09/19 18:37:22, skobes wrote:
> > The question is whether Frame::m_host can ever be null at this point.  I am
> not
> > quite sure, but we do these checks in a lot of places and Frame::settings()
> has
> > the comment "can be null".
> 
> Ok, let's change the null checks to DCHECKs.

Done. I also changed this in FrameView, assuming it's the same reasoning there.
Does that make sense?

[commit-bot: I haz the power, 2016-09-22 13:05:27]

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

[commit-bot: I haz the power, 2016-09-22 13:05:29]

Dry run: This issue passed the CQ dry run.

[skobes, 2016-09-22 14:12:45]

On 2016/09/22 12:41:56, Eric Seckler wrote:
>
https://codereview.chromium.org/2339873002/diff/80001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/core/paint/PaintLayerScrollableArea.cpp
(right):
> 
>
https://codereview.chromium.org/2339873002/diff/80001/third_party/WebKit/Sour...
> third_party/WebKit/Source/core/paint/PaintLayerScrollableArea.cpp:1054: if
> (box().frame()->settings() && box().frame()->settings()->hideScrollbars())
> On 2016/09/19 19:31:20, skobes wrote:
> > On 2016/09/19 18:37:22, skobes wrote:
> > > The question is whether Frame::m_host can ever be null at this point.  I
am
> > not
> > > quite sure, but we do these checks in a lot of places and
Frame::settings()
> > has
> > > the comment "can be null".
> > 
> > Ok, let's change the null checks to DCHECKs.
> 
> Done. I also changed this in FrameView, assuming it's the same reasoning
there.
> Does that make sense?

dcheng does that seem right?  It makes me a little nervous since we have null
checks on almost every other use of settings() in FrameView.

[Eric Seckler, 2016-09-22 14:26:03]

On 2016/09/22 14:12:45, skobes wrote:
> On 2016/09/22 12:41:56, Eric Seckler wrote:
> >
>
https://codereview.chromium.org/2339873002/diff/80001/third_party/WebKit/Sour...
> > File third_party/WebKit/Source/core/paint/PaintLayerScrollableArea.cpp
> (right):
> > 
> >
>
https://codereview.chromium.org/2339873002/diff/80001/third_party/WebKit/Sour...
> > third_party/WebKit/Source/core/paint/PaintLayerScrollableArea.cpp:1054: if
> > (box().frame()->settings() && box().frame()->settings()->hideScrollbars())
> > On 2016/09/19 19:31:20, skobes wrote:
> > > On 2016/09/19 18:37:22, skobes wrote:
> > > > The question is whether Frame::m_host can ever be null at this point.  I
> am
> > > not
> > > > quite sure, but we do these checks in a lot of places and
> Frame::settings()
> > > has
> > > > the comment "can be null".
> > > 
> > > Ok, let's change the null checks to DCHECKs.
> > 
> > Done. I also changed this in FrameView, assuming it's the same reasoning
> there.
> > Does that make sense?
> 
> dcheng does that seem right?  It makes me a little nervous since we have null
> checks on almost every other use of settings() in FrameView.

I'm not sure what the best practice for this is anyway. As Steve mentioned,
settings() doesn't really specify when it's safe to assume that it's not null,
so skipping the check (based on impl detail knowledge) feels a bit like a breach
of modularity. Maybe it's cleaner to add a private helper method
shouldHideScrollbars() to both FrameView and PaintLayerScrollableArea, which
also checks for null, and then not having to worry about where it's invoked
from...?

[skobes, 2016-09-22 21:04:03]

On 2016/09/22 14:26:03, Eric Seckler wrote:
> On 2016/09/22 14:12:45, skobes wrote:
> > On 2016/09/22 12:41:56, Eric Seckler wrote:
> > >
> >
>
https://codereview.chromium.org/2339873002/diff/80001/third_party/WebKit/Sour...
> > > File third_party/WebKit/Source/core/paint/PaintLayerScrollableArea.cpp
> > (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/2339873002/diff/80001/third_party/WebKit/Sour...
> > > third_party/WebKit/Source/core/paint/PaintLayerScrollableArea.cpp:1054: if
> > > (box().frame()->settings() && box().frame()->settings()->hideScrollbars())
> > > On 2016/09/19 19:31:20, skobes wrote:
> > > > On 2016/09/19 18:37:22, skobes wrote:
> > > > > The question is whether Frame::m_host can ever be null at this point. 
I
> > am
> > > > not
> > > > > quite sure, but we do these checks in a lot of places and
> > Frame::settings()
> > > > has
> > > > > the comment "can be null".
> > > > 
> > > > Ok, let's change the null checks to DCHECKs.
> > > 
> > > Done. I also changed this in FrameView, assuming it's the same reasoning
> > there.
> > > Does that make sense?
> > 
> > dcheng does that seem right?  It makes me a little nervous since we have
null
> > checks on almost every other use of settings() in FrameView.
> 
> I'm not sure what the best practice for this is anyway. As Steve mentioned,
> settings() doesn't really specify when it's safe to assume that it's not null,
> so skipping the check (based on impl detail knowledge) feels a bit like a
breach
> of modularity. Maybe it's cleaner to add a private helper method
> shouldHideScrollbars() to both FrameView and PaintLayerScrollableArea, which
> also checks for null, and then not having to worry about where it's invoked
> from...?

I filed crbug.com/649479 to clean this up.  For now I'd prefer to leave the null
checks in place, the performance impact should be negligible.  Up to you whether
you want a private helper.

[Eric Seckler, 2016-09-23 09:41:43]

On 2016/09/22 21:04:03, skobes wrote:
> On 2016/09/22 14:26:03, Eric Seckler wrote:
> > On 2016/09/22 14:12:45, skobes wrote:
> > > On 2016/09/22 12:41:56, Eric Seckler wrote:
> > > >
> > >
> >
>
https://codereview.chromium.org/2339873002/diff/80001/third_party/WebKit/Sour...
> > > > File third_party/WebKit/Source/core/paint/PaintLayerScrollableArea.cpp
> > > (right):
> > > > 
> > > >
> > >
> >
>
https://codereview.chromium.org/2339873002/diff/80001/third_party/WebKit/Sour...
> > > > third_party/WebKit/Source/core/paint/PaintLayerScrollableArea.cpp:1054:
if
> > > > (box().frame()->settings() &&
box().frame()->settings()->hideScrollbars())
> > > > On 2016/09/19 19:31:20, skobes wrote:
> > > > > On 2016/09/19 18:37:22, skobes wrote:
> > > > > > The question is whether Frame::m_host can ever be null at this
point. 
> I
> > > am
> > > > > not
> > > > > > quite sure, but we do these checks in a lot of places and
> > > Frame::settings()
> > > > > has
> > > > > > the comment "can be null".
> > > > > 
> > > > > Ok, let's change the null checks to DCHECKs.
> > > > 
> > > > Done. I also changed this in FrameView, assuming it's the same reasoning
> > > there.
> > > > Does that make sense?
> > > 
> > > dcheng does that seem right?  It makes me a little nervous since we have
> null
> > > checks on almost every other use of settings() in FrameView.
> > 
> > I'm not sure what the best practice for this is anyway. As Steve mentioned,
> > settings() doesn't really specify when it's safe to assume that it's not
null,
> > so skipping the check (based on impl detail knowledge) feels a bit like a
> breach
> > of modularity. Maybe it's cleaner to add a private helper method
> > shouldHideScrollbars() to both FrameView and PaintLayerScrollableArea, which
> > also checks for null, and then not having to worry about where it's invoked
> > from...?
> 
> I filed crbug.com/649479 to clean this up.  For now I'd prefer to leave the
null
> checks in place, the performance impact should be negligible.  Up to you
whether
> you want a private helper.

Thanks, added the checks back.

[skobes, 2016-09-23 15:21:04]

lgtm

[Eric Seckler, 2016-09-23 15:23:06]

The CQ bit was checked by eseckler@chromium.org

[Eric Seckler, 2016-09-23 15:23:07]

The patchset sent to the CQ was uploaded after l-g-t-m from sky@chromium.org,
dcheng@chromium.org, piman@chromium.org, halliwell@chromium.org,
pdr@chromium.org
Link to the patchset: https://codereview.chromium.org/2339873002/#ps160001
(title: "Reintroduce null-checks.")

[commit-bot: I haz the power, 2016-09-23 15:23:22]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[dcheng, 2016-09-23 15:26:24]

On 2016/09/23 15:23:22, commit-bot: I haz the power wrote:
> CQ is trying da patch. Follow status at
>  
>
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

Sorry, I missed the replies.

I'm not really OK with unnecessarily null checking this. I'd rather we didn't:
it propagates a bunch of pointless null checks throughout the codebase, and
makes it to understand when things actually *are* null.

[skobes, 2016-09-23 15:30:01]

On 2016/09/23 15:26:24, dcheng wrote:
> On 2016/09/23 15:23:22, commit-bot: I haz the power wrote:
> > CQ is trying da patch. Follow status at
> >  
> >
>
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
> 
> Sorry, I missed the replies.
> 
> I'm not really OK with unnecessarily null checking this. I'd rather we didn't:
> it propagates a bunch of pointless null checks throughout the codebase, and
> makes it to understand when things actually *are* null.

Can you prove that FrameView::setScrollbarModes is never called on a detached
frame?

[Eric Seckler, 2016-09-23 15:44:58]

The CQ bit was unchecked by eseckler@chromium.org

[dcheng, 2016-09-23 16:10:35]

On 2016/09/23 15:30:01, skobes wrote:
> On 2016/09/23 15:26:24, dcheng wrote:
> > On 2016/09/23 15:23:22, commit-bot: I haz the power wrote:
> > > CQ is trying da patch. Follow status at
> > >  
> > >
> >
>
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
> > 
> > Sorry, I missed the replies.
> > 
> > I'm not really OK with unnecessarily null checking this. I'd rather we
didn't:
> > it propagates a bunch of pointless null checks throughout the codebase, and
> > makes it to understand when things actually *are* null.
> 
> Can you prove that FrameView::setScrollbarModes is never called on a detached
> frame?

Starting set of principles:

- LocalFrame / FrameView should have roughly equivalent lifetime (in other
words, they should be created and detached together). A FrameView is disposed as
part of detaching frames (see Document::shutdown and LocalFrame::detach).
- Page / FrameHost / Settings / FrameClient have the same lifetime (Settings is
derived from Page, Page and FrameHost are really synonyms, FrameHost/FrameClient
are set to null in frame detach)
- The prior points are invariants, not ideals. This one is an ideal: layout
should never run for a detached Document (i.e. a Document with no LocalFrame).
But if it's not true, I think we have bigger problems.

From the codesearch listings, I see 8 primary callers.

FrameView::setCanHaveScollbars: This has 5 callers.
  - FrameView::init: called during construction. If we're constructing this on
an already detached LocalFrame, we have bigger problems.
  - FrameView::createView: Asserts page(). We're in big trouble otherwise.
  - SVGImage::dataChanged: The LocalFrame in question is created a few lines
before. LocalFrame::init() dispatches JS events, but this is in a newly created
LocalFrame that we control with no event listeners. If the frame is detached at
this point, we did something wrong.
  - InspectorOverlay::overlayPage: Similar to SVGImage::dataChanged.
  - WebLocalFrameImpl::setCanHaveScrollbars: When a frame is detached, frame()
will return null. It is theoretically possible someone could call this method in
WebFrameClient::frameDetached(), but that's a programming error, not something
we should support.

FrameView::layout (two places): We should not be doing layout if the Document is
detached / detaching.

FrameView::disableAutoSizeMode: This is used only by
WebViewImpl::configureAutoResizeMode(), which early returns after a long series
of (also probably unnecessary) null checks. By reaching the point where it
actually calls through, it's null checked all the state that would indicate a
Document / FrameView / LocalFrame is detached. In any case, even if we didn't
have these null checks, calling this on a detached WebView is a programming
error, not something we should support.

FrameView::setHorizontalScrollbarMode: Only called in layout.

FrameView::setVerticalScrollbarMode: Also only called in layout.

FrameViewAutoSizeInfo::autoSizeIfNeeded: Returns early if Document::isActive()
is not true.

LocalFrame::createView: asserts page(), so we must have a settings object.

Incidentally, that comment on Frame::settings() that it can be null has been
really problematic. It leads to code like this, where one person read the
comment and another didn't, so one line null checks even though it's
dereferenced with no check a line prior:
https://chromium.googlesource.com/chromium/src/+blame/master/third_party/WebK...

Obviously, it's going to be very hard to prove this rigorously, but I think we
can conclude that if settings() is null, we've violated some invariant in some
very interesting way that we could learn a lot from.

[Eric Seckler, 2016-09-23 16:25:26]

Thanks for the thorough investigation, sounds like we're safe. I'm happy to
commit patch set #5 (which uses DCHECKs only) if Steve's okay with this, too.

On 2016/09/23 16:10:35, dcheng wrote:
> Incidentally, that comment on Frame::settings() that it can be null has been
> really problematic. It leads to code like this, where one person read the
> comment and another didn't, so one line null checks even though it's
> dereferenced with no check a line prior:
>
https://chromium.googlesource.com/chromium/src/+blame/master/third_party/WebK...

This is probably a good argument for Steve's proposal in crbug.com/649479 to
make Settings garbage-collected and avoid the issue altogether? Shall we move
further discussion there?

[skobes, 2016-09-23 16:45:00]

On 2016/09/23 16:10:35, dcheng wrote:
> From the codesearch listings, I see 8 primary callers.
> [...]

That's a lot of callers and a lot of analysis based on assumptions that could
easily change in the future.

> Incidentally, that comment on Frame::settings() that it can be null has been
> really problematic. It leads to code like this, where one person read the
> comment and another didn't, so one line null checks even though it's
> dereferenced with no check a line prior:

The comment itself is quite clear, the path to madness is in selectively
ignoring it.  As long as the comment exists, the correct thing for callers to do
is to null check.  I can possibly see making an exception if the caller
obviously only makes sense when the frame is attached, e.g. FrameView::layout. 
But nothing about setting scrollbar modes conceptually implies an attached
frame.

> Obviously, it's going to be very hard to prove this rigorously, but I think we
> can conclude that if settings() is null, we've violated some invariant in some
> very interesting way that we could learn a lot from.

Invariants should be documented, not inferred from complex call graph analysis. 
Right now this "invariant" is not only not documented but explicitly
contradicted by the documentation.  If you want to change the comment from "//
can be null" to "// cannot be null" and put the DCHECK in Frame::settings I
would totally support that.  But we shouldn't block this patch on a settings
architecture cleanup, and until that cleanup is done I think we should respect
the documented behavior of the API.

[skobes, 2016-09-23 16:47:43]

lgtm

On 2016/09/23 16:25:26, Eric Seckler wrote:
> Thanks for the thorough investigation, sounds like we're safe. I'm happy to
> commit patch set #5 (which uses DCHECKs only) if Steve's okay with this, too.

Yeah I don't really agree with using DCHECKs only but I don't want to delay this
patch over it.  Go ahead and land and we'll sort it out on crbug.com/649479.

[Eric Seckler, 2016-09-23 17:39:01]

Patchset #6 (id:160001) has been deleted

[Eric Seckler, 2016-09-23 17:39:18]

The CQ bit was checked by eseckler@chromium.org

[commit-bot: I haz the power, 2016-09-23 17:40:01]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-23 20:05:35]

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

[commit-bot: I haz the power, 2016-09-23 20:05:36]

Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[Eric Seckler, 2016-09-23 20:51:57]

The CQ bit was checked by eseckler@chromium.org

[commit-bot: I haz the power, 2016-09-23 20:52:34]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-23 22:58:22]

Description was changed from

==========
Replace hide-scrollbars flag with a web setting.

Instead of the composited/non-composited distinction, we now enforce this purely
within in Blink in three places: PaintLayerScrollableArea, FrameView,
VisualViewport.

Also updates the use of the flag in chromecast/.

BUG=639806,internal b/29253042
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

to

==========
Replace hide-scrollbars flag with a web setting.

Instead of the composited/non-composited distinction, we now enforce this purely
within in Blink in three places: PaintLayerScrollableArea, FrameView,
VisualViewport.

Also updates the use of the flag in chromecast/.

BUG=639806,internal b/29253042
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

[commit-bot: I haz the power, 2016-09-23 22:58:27]

Committed patchset #5 (id:140001)

[commit-bot: I haz the power, 2016-09-23 23:00:16]

Description was changed from

==========
Replace hide-scrollbars flag with a web setting.

Instead of the composited/non-composited distinction, we now enforce this purely
within in Blink in three places: PaintLayerScrollableArea, FrameView,
VisualViewport.

Also updates the use of the flag in chromecast/.

BUG=639806,internal b/29253042
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

to

==========
Replace hide-scrollbars flag with a web setting.

Instead of the composited/non-composited distinction, we now enforce this purely
within in Blink in three places: PaintLayerScrollableArea, FrameView,
VisualViewport.

Also updates the use of the flag in chromecast/.

BUG=639806,internal b/29253042
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2

Committed: https://crrev.com/61ff9143240cc8bc02688b5d84cc939cb14e7685
Cr-Commit-Position: refs/heads/master@{#420766}
==========

[commit-bot: I haz the power, 2016-09-23 23:00:18]

Patchset 5 (id:??) landed as
https://crrev.com/61ff9143240cc8bc02688b5d84cc939cb14e7685
Cr-Commit-Position: refs/heads/master@{#420766}