Bug 171013 – Expose obscured insets to web content (as "safe area insets")

• Component: New Bugs
• Status: RESOLVED FIXED

[Tim Horton]

Expose obscured insets to web content (as "safe area insets")

[Tim Horton]

Created attachment 307513 [details]
Patch

[Tim Horton]

Created attachment 307521 [details]
Patch

[Tim Horton]

Tests were missing from the first patch.

[Wenson Hsieh]

Comment on attachment 307521 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=307521&action=review

r=me

> Source/WebCore/dom/ConstantPropertyMap.cpp:96
> +void ConstantPropertyMap::didChangeObscuredInsets()

imo, updateObscuredInsetsFromPage would be a more informative name here, or maybe just updateObscuredInsets, and then pass a const ref to the insets around.

> Source/WebCore/page/Page.cpp:2292
> +void Page::setObscuredInsets(FloatBoxExtent insets)

Nit - I think this can be a const &

> Source/WebCore/page/Page.h:333
> +    WEBCORE_EXPORT void setObscuredInsets(FloatBoxExtent);

Nit - const FloatBoxExtent&?

> Source/WebCore/page/Page.h:692
>      // This is only used for history scroll position restoration.

Is this comment still valid? (now that it's referring to m_enclosedInScrollableAncestorView)

[Build Bot]

Comment on attachment 307521 [details]
Patch

Attachment 307521 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/3566887

Number of test failures exceeded the failure limit.

[Build Bot]

Created attachment 307541 [details]
Archive of layout-test-results from ews123 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews123  Port: ios-simulator-wk2  Platform: Mac OS X 10.11.6

[Tim Horton]

(In reply to Wenson Hsieh from comment #4)
> Comment on attachment 307521 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=307521&action=review
> 
> r=me

I would like Hyatt's eyes as well.

Simon has also suggested exposing something simpler (like scrollbar width) as an initial patch, and I have a partial implementation of that but it's actually much more complicated to get right for a number of reasons, so I don't think that should be out first shot.

> > Source/WebCore/dom/ConstantPropertyMap.cpp:96
> > +void ConstantPropertyMap::didChangeObscuredInsets()
> 
> imo, updateObscuredInsetsFromPage would be a more informative name here, or
> maybe just updateObscuredInsets, and then pass a const ref to the insets
> around.

I will change this around a bit.

> > Source/WebCore/page/Page.cpp:2292
> > +void Page::setObscuredInsets(FloatBoxExtent insets)
> 
> Nit - I think this can be a const &
> 
> > Source/WebCore/page/Page.h:333
> > +    WEBCORE_EXPORT void setObscuredInsets(FloatBoxExtent);
> 
> Nit - const FloatBoxExtent&?

Indeed.

> > Source/WebCore/page/Page.h:692
> >      // This is only used for history scroll position restoration.
> 
> Is this comment still valid? (now that it's referring to
> m_enclosedInScrollableAncestorView)

It is not! I assumed it was about both, but it is not. I will remove it.

[Tim Horton]

(In reply to Tim Horton from comment #7)
> (In reply to Wenson Hsieh from comment #4)
> > Comment on attachment 307521 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=307521&action=review
> > 
> > r=me
> 
> I would like Hyatt's eyes as well.
> 
> Simon has also suggested exposing something simpler (like scrollbar width)
> as an initial patch, and I have a partial implementation of that but it's
> actually much more complicated to get right for a number of reasons, so I
> don't think that should be out first shot.
> 
> > > Source/WebCore/dom/ConstantPropertyMap.cpp:96
> > > +void ConstantPropertyMap::didChangeObscuredInsets()
> > 
> > imo, updateObscuredInsetsFromPage would be a more informative name here, or
> > maybe just updateObscuredInsets, and then pass a const ref to the insets
> > around.

Ehh, no point in passing it in, because I have to reach up and grab them during initialization anyway, and there's no reason to have two paths.

> I will change this around a bit.
> 
> > > Source/WebCore/page/Page.cpp:2292
> > > +void Page::setObscuredInsets(FloatBoxExtent insets)
> > 
> > Nit - I think this can be a const &
> > 
> > > Source/WebCore/page/Page.h:333
> > > +    WEBCORE_EXPORT void setObscuredInsets(FloatBoxExtent);
> > 
> > Nit - const FloatBoxExtent&?
> 
> Indeed.
> 
> > > Source/WebCore/page/Page.h:692
> > >      // This is only used for history scroll position restoration.
> > 
> > Is this comment still valid? (now that it's referring to
> > m_enclosedInScrollableAncestorView)
> 
> It is not! I assumed it was about both, but it is not. I will remove it.

[Tim Horton]

Created attachment 307649 [details]
Patch

[Tim Horton]

Created attachment 307659 [details]
Patch

[Tim Horton]

Created attachment 307662 [details]
Patch

[Tim Horton]

https://trac.webkit.org/changeset/215597/webkit

[Alexey Proskuryakov]

This made every test fail under GuardMalloc. Rolling out.

Thread 0 Crashed:: Dispatch queue: com.apple.main-thread
0   com.apple.WebCore             	0x0000000115328160 WebCore::CSSVariableData::consumeAndUpdateTokens(WebCore::CSSParserTokenRange const&) + 80
1   com.apple.WebCore             	0x00000001158e5bb2 WebCore::ConstantPropertyMap::updateConstantsForObscuredInsets() + 290
2   com.apple.WebCore             	0x00000001158e5867 WebCore::ConstantPropertyMap::buildValues() + 215
3   com.apple.WebCore             	0x00000001158e5776 WebCore::ConstantPropertyMap::values() const + 22
4   com.apple.WebCore             	0x0000000115fa8cb9 WebCore::Style::resolveForDocument(WebCore::Document const&) + 2073

[WebKit Commit Bot]

Re-opened since this is blocked by bug 171102

[Tim Horton]

Insanely stupid. Relanding.

[Tim Horton]

https://trac.webkit.org/changeset/215607/webkit

[Alexey Proskuryakov]

Can we make it so that compiler catches such issues?

-    CSSParserTokenRange tokenRange(Vector<CSSParserToken> { token });
+    Vector<CSSParserToken> tokens { token };
+    CSSParserTokenRange tokenRange(tokens);

[Tim Horton]

(In reply to Alexey Proskuryakov from comment #17)
> Can we make it so that compiler catches such issues?
> 
> -    CSSParserTokenRange tokenRange(Vector<CSSParserToken> { token });
> +    Vector<CSSParserToken> tokens { token };
> +    CSSParserTokenRange tokenRange(tokens);

I don't think so? It's not really categorically wrong, it just happens that CSSParserTokenRange captures iterators from the vector and tries to use them later.