Bug 171013 - Expose obscured insets to web content (as "safe area insets")
Summary: Expose obscured insets to web content (as "safe area insets")
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Tim Horton
URL:
Keywords:
Depends on: 171102
Blocks:
  Show dependency treegraph
 
Reported: 2017-04-19 15:09 PDT by Tim Horton
Modified: 2017-04-21 10:31 PDT (History)
11 users (show)

See Also:


Attachments
Patch (24.21 KB, patch)
2017-04-19 15:30 PDT, Tim Horton
no flags Details | Formatted Diff | Diff
Patch (33.24 KB, patch)
2017-04-19 16:25 PDT, Tim Horton
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews123 for ios-simulator-wk2 (5.31 MB, application/zip)
2017-04-19 19:22 PDT, Build Bot
no flags Details
Patch (25.68 KB, patch)
2017-04-20 15:40 PDT, Tim Horton
no flags Details | Formatted Diff | Diff
Patch (25.74 KB, patch)
2017-04-20 16:38 PDT, Tim Horton
no flags Details | Formatted Diff | Diff
Patch (34.75 KB, patch)
2017-04-20 16:54 PDT, Tim Horton
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Tim Horton 2017-04-19 15:09:48 PDT
Expose obscured insets to web content (as "safe area insets")
Comment 1 Tim Horton 2017-04-19 15:30:37 PDT
Created attachment 307513 [details]
Patch
Comment 2 Tim Horton 2017-04-19 16:25:12 PDT
Created attachment 307521 [details]
Patch
Comment 3 Tim Horton 2017-04-19 16:25:27 PDT
Tests were missing from the first patch.
Comment 4 Wenson Hsieh 2017-04-19 17:32:35 PDT
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)
Comment 5 Build Bot 2017-04-19 19:22:28 PDT
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.
Comment 6 Build Bot 2017-04-19 19:22:29 PDT
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
Comment 7 Tim Horton 2017-04-20 00:45:27 PDT
(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.
Comment 8 Tim Horton 2017-04-20 01:03:05 PDT
(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.
Comment 9 Tim Horton 2017-04-20 15:40:19 PDT
Created attachment 307649 [details]
Patch
Comment 10 Tim Horton 2017-04-20 16:38:04 PDT
Created attachment 307659 [details]
Patch
Comment 11 Tim Horton 2017-04-20 16:54:25 PDT
Created attachment 307662 [details]
Patch
Comment 12 Tim Horton 2017-04-20 18:01:18 PDT
https://trac.webkit.org/changeset/215597/webkit
Comment 13 Alexey Proskuryakov 2017-04-20 23:01:21 PDT
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
Comment 14 WebKit Commit Bot 2017-04-20 23:02:16 PDT
Re-opened since this is blocked by bug 171102
Comment 15 Tim Horton 2017-04-21 00:32:31 PDT
Insanely stupid. Relanding.
Comment 16 Tim Horton 2017-04-21 00:34:43 PDT
https://trac.webkit.org/changeset/215607/webkit
Comment 17 Alexey Proskuryakov 2017-04-21 09:28:54 PDT
Can we make it so that compiler catches such issues?

-    CSSParserTokenRange tokenRange(Vector<CSSParserToken> { token });
+    Vector<CSSParserToken> tokens { token };
+    CSSParserTokenRange tokenRange(tokens);
Comment 18 Tim Horton 2017-04-21 10:31:48 PDT
(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.