Bug 187578 – Make it easier to hit the significant rendered text layout milestone on pages with main article elements

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit Misc.

[Wenson Hsieh]

Our current heuristics are very conservative, with the intention of avoiding false positives.

We can probably relax some of these constraints when we've detected the presence of a main article element on the page. (e.g. NYTimes)

[Wenson Hsieh]

Created attachment 344822 [details]
Patch

[Daniel Bates]

Comment on attachment 344822 [details]
Patch

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

I have not checked this patch for correctness or read through it as I am tired. Just noticed some very minor issues.

> Source/WebCore/ChangeLog:8
> +        Our current heuristics for triggering the signficant rendered text layout milestone are very conservative, with

signficant => significant

> Source/WebCore/dom/Document.cpp:7715
> +    static unsigned maxNumberOfArticlesBeforeIgnoringMainContentArticle = 10;

static  => const

> Source/WebCore/dom/Document.cpp:7720
> +    static float mainContentArticleHeightFactor = 4;

Ditto.

> Source/WebCore/dom/Document.cpp:7721
> +    static float mainContentArticleMinimumHeight = 1000;

Ditto.

[Wenson Hsieh]

Created attachment 344827 [details]
Patch

[Wenson Hsieh]

Comment on attachment 344822 [details]
Patch

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

>> Source/WebCore/ChangeLog:8
>> +        Our current heuristics for triggering the signficant rendered text layout milestone are very conservative, with
> 
> signficant => significant

Good catch!

>> Source/WebCore/dom/Document.cpp:7715
>> +    static unsigned maxNumberOfArticlesBeforeIgnoringMainContentArticle = 10;
> 
> static  => const

Changed to const.

[Radar WebKit Bug Importer]

<rdar://problem/42104637>

[Ryosuke Niwa]

Comment on attachment 344827 [details]
Patch

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

I think we need another iteration since this keeping track of elements in a document is really trickily.
I can't r+ this until I see the final patch.

> Source/WebCore/dom/Document.cpp:7721
> +    const float mainContentArticleMinimumHeight = 1000;

This should probably a factor of the viewport size or width.
A hard coded height in px doesn't make much sense.

> Source/WebCore/dom/Element.cpp:1756
> +    if (UNLIKELY(hasTagName(articleTag) && newDocument))

Don't do this in Element::insertedIntoAncestor. Add HTMLArticleElement::insertedIntoAncestor instead.

> Source/WebCore/dom/Element.cpp:1806
> +            if (UNLIKELY(hasTagName(articleTag)))

Ditto.

[Wenson Hsieh]

Comment on attachment 344827 [details]
Patch

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

>> Source/WebCore/dom/Document.cpp:7721
>> +    const float mainContentArticleMinimumHeight = 1000;
> 
> This should probably a factor of the viewport size or width.
> A hard coded height in px doesn't make much sense.

Will fix!

>> Source/WebCore/dom/Element.cpp:1756
>> +    if (UNLIKELY(hasTagName(articleTag) && newDocument))
> 
> Don't do this in Element::insertedIntoAncestor. Add HTMLArticleElement::insertedIntoAncestor instead.

HTMLArticleElement doesn't exist. Should I introduce it?

[Ryosuke Niwa]

Comment on attachment 344827 [details]
Patch

Hm... okay. I guess we'd have to keep it in HTMLElement in that case.

[Wenson Hsieh]

Created attachment 344889 [details]
Address feedback.

[Wenson Hsieh]

Created attachment 344909 [details]
Move registration logic to HTMLElement.

[EWS Watchlist]

Comment on attachment 344909 [details]
Move registration logic to HTMLElement.

Attachment 344909 [details] did not pass mac-wk2-ews (mac-wk2):
Output: https://webkit-queues.webkit.org/results/8522178

New failing tests:
accessibility/roles-exposed.html

[EWS Watchlist]

Created attachment 344914 [details]
Archive of layout-test-results from ews107 for mac-sierra-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews107  Port: mac-sierra-wk2  Platform: Mac OS X 10.12.6

[EWS Watchlist]

Comment on attachment 344909 [details]
Move registration logic to HTMLElement.

Attachment 344909 [details] did not pass mac-ews (mac):
Output: https://webkit-queues.webkit.org/results/8522219

New failing tests:
accessibility/roles-exposed.html

[EWS Watchlist]

Created attachment 344915 [details]
Archive of layout-test-results from ews102 for mac-sierra

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews102  Port: mac-sierra  Platform: Mac OS X 10.12.6

[Wenson Hsieh]

Created attachment 344920 [details]
Move registration logic to HTMLElement (v2)

[Wenson Hsieh]

(In reply to Ryosuke Niwa from comment #8)
> Comment on attachment 344827 [details]
> Patch
> 
> Hm... okay. I guess we'd have to keep it in HTMLElement in that case.

@Ryosuke — does https://bugs.webkit.org/attachment.cgi?id=344920&action=review look reasonable? If so, I'll send it off to commit queue.

[Ryosuke Niwa]

Comment on attachment 344920 [details]
Move registration logic to HTMLElement (v2)

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

> Source/WebCore/html/HTMLElement.cpp:404
> +Node::InsertedIntoAncestorResult HTMLElement::insertedIntoAncestor(InsertionType insertionType, ContainerNode& parentOfInsertedTree)

I think we should just go with the check in Element. This patch introduces an extra function call.

[Wenson Hsieh]

(In reply to Ryosuke Niwa from comment #17)
> Comment on attachment 344920 [details]
> Move registration logic to HTMLElement (v2)
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=344920&action=review
> 
> > Source/WebCore/html/HTMLElement.cpp:404
> > +Node::InsertedIntoAncestorResult HTMLElement::insertedIntoAncestor(InsertionType insertionType, ContainerNode& parentOfInsertedTree)
> 
> I think we should just go with the check in Element. This patch introduces
> an extra function call.

👍 Will do.

[Wenson Hsieh]

Created attachment 344927 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 344927 [details]
Patch for landing

Clearing flags on attachment: 344927

Committed r233794: <https://trac.webkit.org/changeset/233794>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.