[webkit-reviews] review denied: [Bug 187578] Make it easier to hit the significant rendered text layout milestone on pages with main article elements : [Attachment 344827] Patch

[bugzilla-daemon at webkit.org]

Ryosuke Niwa <rniwa at webkit.org> has denied Wenson Hsieh
<wenson_hsieh at apple.com>'s request for review:
Bug 187578: Make it easier to hit the significant rendered text layout
milestone on pages with main article elements
https://bugs.webkit.org/show_bug.cgi?id=187578

Attachment 344827: Patch

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




--- Comment #6 from Ryosuke Niwa <rniwa at webkit.org> ---
Comment on attachment 344827
  --> https://bugs.webkit.org/attachment.cgi?id=344827
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.