Summary: | Expose obscured insets to web content (as "safe area insets") | ||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Tim Horton <thorton> | ||||||||||||||
Component: | New Bugs | Assignee: | Tim Horton <thorton> | ||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||
Severity: | Normal | CC: | andersca, ap, bdakin, buildbot, commit-queue, dino, eoconnor, hyatt, ryanhaddad, simon.fraser, wenson_hsieh | ||||||||||||||
Priority: | P2 | ||||||||||||||||
Version: | WebKit Nightly Build | ||||||||||||||||
Hardware: | Unspecified | ||||||||||||||||
OS: | Unspecified | ||||||||||||||||
Bug Depends on: | 171102 | ||||||||||||||||
Bug Blocks: | |||||||||||||||||
Attachments: |
|
Description
Tim Horton
2017-04-19 15:09:48 PDT
Created attachment 307513 [details]
Patch
Created attachment 307521 [details]
Patch
Tests were missing from the first patch. 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 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. 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
(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. (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. Created attachment 307649 [details]
Patch
Created attachment 307659 [details]
Patch
Created attachment 307662 [details]
Patch
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 Re-opened since this is blocked by bug 171102 Insanely stupid. Relanding. Can we make it so that compiler catches such issues? - CSSParserTokenRange tokenRange(Vector<CSSParserToken> { token }); + Vector<CSSParserToken> tokens { token }; + CSSParserTokenRange tokenRange(tokens); (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. |