<rdar://problem/41744338>
Created attachment 344462 [details] Patch
Comment on attachment 344462 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=344462&action=review > Source/WebCore/ChangeLog:9 > + Implements a new layout milestone: `DidReachSignificantRenderedTextThreshold`. This is similar to the existing Why not DidReachSignificantAmoutOfTextRenderedThreshold or DidRenderedSignificantAmoutOfText? I don't think "Threshold" adds much value here since everything is heuristics based. > Source/WebCore/page/FrameView.cpp:4364 > + if (!m_numberOfTextRenderers || m_visuallyNonEmptyCharacterCount / (float)m_numberOfTextRenderers < significantRenderedTextMeanLength) Use static_cast<float> > Source/WebCore/page/FrameView.h:934 > + ++m_numberOfTextRenderers; I think more canonical way of naming this variable would be m_renderTextCount. But this is the total number of render text ever created since FrameView::reset() is last called so that's not quite right either. How about m_renderTextCountForVisuallyNonEmptyCharacters?
Comment on attachment 344462 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=344462&action=review >> Source/WebCore/ChangeLog:9 >> + Implements a new layout milestone: `DidReachSignificantRenderedTextThreshold`. This is similar to the existing > > Why not DidReachSignificantAmoutOfTextRenderedThreshold or DidRenderedSignificantAmoutOfText? > I don't think "Threshold" adds much value here since everything is heuristics based. Sounds good — I'll go with DidRenderSignificantAmountOfText >> Source/WebCore/page/FrameView.cpp:4364 >> + if (!m_numberOfTextRenderers || m_visuallyNonEmptyCharacterCount / (float)m_numberOfTextRenderers < significantRenderedTextMeanLength) > > Use static_cast<float> 👍 >> Source/WebCore/page/FrameView.h:934 >> + ++m_numberOfTextRenderers; > > I think more canonical way of naming this variable would be m_renderTextCount. > But this is the total number of render text ever created since FrameView::reset() is last called > so that's not quite right either. > How about m_renderTextCountForVisuallyNonEmptyCharacters? Sounds good — I'll go with m_renderTextCountForVisuallyNonEmptyCharacters.
Comment on attachment 344462 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=344462&action=review > Source/WebCore/ChangeLog:16 > + being capable of Reader mode. In many article-like pages, the average length of an uninterrupted text node is page is capable of being capable of > Source/WebCore/ChangeLog:17 > + significantly longer than other types of pages; thus, on pages where the average length of a text node is very What's an uninterrupted text node? > Source/WebCore/ChangeLog:19 > + Safari reader mode on watchOS. Is this a watchOS only feature? > Source/WebCore/ChangeLog:38 > + layout, this milestone is never guaranteed to fire. What if an iframe fires this milestone and not the main frame? The page is still considered Reader like? > Source/WebCore/page/FrameView.cpp:4315 > + return snappedIntRect(element.renderBox()->layoutOverflowRect()).height() >= documentHeightThreshold; on trunk it is guaranteed that element.renderBox != nullptr since qualifiesAsVisuallyNonEmpty early returns, but now I can just call this function when element->renderer() is nullptr.
Comment on attachment 344462 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=344462&action=review >> Source/WebCore/ChangeLog:16 >> + being capable of Reader mode. In many article-like pages, the average length of an uninterrupted text node is > > page is capable of being capable of Oops! Fixed. >> Source/WebCore/ChangeLog:17 >> + significantly longer than other types of pages; thus, on pages where the average length of a text node is very > > What's an uninterrupted text node? I really just meant "text node" here — the "uninterrupted" part should've just been omitted. >> Source/WebCore/ChangeLog:19 >> + Safari reader mode on watchOS. > > Is this a watchOS only feature? It'll fire on iOS and macOS as well if the SPI client opts into the milestone (the API test included in this patch even exercises this using the iOS simulator). That being said, Safari on watchOS is the only client that might make use of this layout milestone at the moment. >> Source/WebCore/ChangeLog:38 >> + layout, this milestone is never guaranteed to fire. > > What if an iframe fires this milestone and not the main frame? The page is still considered Reader like? That's...an interesting point. At the moment, an iframe isn't capable of firing this milestone (since FrameView::fireLayoutRelatedMilestonesIfNeeded limits firing layout milestones to the main frame). I'm not certain what our eventual policy should be regarding layout milestones for subframes. FWIW, we disable cross-origin subframes altogether on watchOS, so I don't think this will matter much for the time being. >> Source/WebCore/page/FrameView.cpp:4315 >> + return snappedIntRect(element.renderBox()->layoutOverflowRect()).height() >= documentHeightThreshold; > > on trunk it is guaranteed that element.renderBox != nullptr since qualifiesAsVisuallyNonEmpty early returns, but now I can just call this function when element->renderer() is nullptr. Fair point. I'll make this robust when `!element.renderBox()`.
Created attachment 344507 [details] v2
Comment on attachment 344462 [details] Patch Attachment 344462 [details] did not pass win-ews (win): Output: https://webkit-queues.webkit.org/results/8463300 New failing tests: http/tests/security/canvas-remote-read-remote-video-redirect.html
Created attachment 344509 [details] Archive of layout-test-results from ews201 for win-future The attached test failures were seen while running run-webkit-tests on the win-ews. Bot: ews201 Port: win-future Platform: CYGWIN_NT-6.1-2.9.0-0.318-5-3-x86_64-64bit
Comment on attachment 344507 [details] v2 Attachment 344507 [details] did not pass win-ews (win): Output: https://webkit-queues.webkit.org/results/8466035 New failing tests: http/tests/preload/onload_event.html
Created attachment 344521 [details] Archive of layout-test-results from ews201 for win-future The attached test failures were seen while running run-webkit-tests on the win-ews. Bot: ews201 Port: win-future Platform: CYGWIN_NT-6.1-2.9.0-0.318-5-3-x86_64-64bit
Comment on attachment 344507 [details] v2 Clearing flags on attachment: 344507 Committed r233623: <https://trac.webkit.org/changeset/233623>