Bug 184753 – Detect system preview links

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Dean Jackson]

Detect system preview links

[Dean Jackson]

<rdar://problem/39500514>

[Dean Jackson]

Created attachment 338256 [details]
Patch

[Dean Jackson]

Created attachment 338260 [details]
Patch

[Antoine Quint]

Comment on attachment 338260 [details]
Patch

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

> Source/WebCore/html/HTMLAnchorElement.cpp:388
> +            return (numChildren == 1 || numChildren == 2);

Don't need the parentheses.

> Source/WebCore/testing/Internals.cpp:4494
> +    return downcast<HTMLAnchorElement>(element).isSystemPreviewLink();

return is<HTMLAnchorElement>(element) && downcast<HTMLAnchorElement>(element).isSystemPreviewLink();

> LayoutTests/system-preview/detection.html:12
> +        children = link.children;

Single-clause if statement should not use curly braces.

> LayoutTests/system-preview/detection.html:25
> +window.addEventListener("load", function () {

() =>

> LayoutTests/system-preview/detection.html:44
> +            addResult(result, "No link.");

Single-clause else statement should not use curly braces.

[Dean Jackson]

Created attachment 338267 [details]
Patch

[Said Abou-Hallawa]

Comment on attachment 338260 [details]
Patch

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

> Source/WebCore/html/HTMLAnchorElement.cpp:380
> +    // The relList is created on demand, which means that calling relList()
> +    // is only available in a non-const method.

Does this comment describe the new code or the existing code? I think in your patch relList() is const which means it can be called from const and non-const methods.

>> Source/WebCore/html/HTMLAnchorElement.cpp:388
>> +            return (numChildren == 1 || numChildren == 2);
> 
> Don't need the parentheses.

It can also be written like this:

    auto* child = firstElementChild();
    if (!child || childElementCount() > 2)
        return false;
    return is<HTMLImageElement>(child) || is<HTMLPictureElement>(child);

Or even shorter

    auto* child = firstElementChild();
    return child && childElementCount() <= 2 && (is<HTMLImageElement>(child) || is<HTMLPictureElement>(child));

> Source/WebCore/html/HTMLPictureElement.cpp:79
> +bool HTMLPictureElement::isSystemPreviewImage() const

Why this function is named isSystemPreviewImage()? Should not this function be named isSystemPreviewPicture()? Or maybe it is better to remove the element name from all these functions:

isSystemPreviewLink()
isSystemPreviewImage()
isSystemPreviewPicture()

and have a single name for all of them isSystemPreviewable()

> Source/WebCore/testing/Internals.cpp:4504
> +bool Internals::isSystemPreviewLink(Element& element) const
> +{
> +    if (!is<HTMLAnchorElement>(element))
> +        return false;
> +    return downcast<HTMLAnchorElement>(element).isSystemPreviewLink();
> +}
> +
> +bool Internals::isSystemPreviewImage(Element& element) const
> +{
> +    if (is<HTMLImageElement>(element))
> +        return downcast<HTMLImageElement>(element).isSystemPreviewImage();
> +    if (is<HTMLPictureElement>(element))
> +        return downcast<HTMLPictureElement>(element).isSystemPreviewImage();
> +    return false;
> +}

Repeating the casting in the Internals and in HTMLPictureElement and HTMLImageElement may suggest having a virtual function Element::isSystemPreviewable() which returns false. HTMLPictureElement and HTMLImageElement override isSystemPreviewable() by just calling parent->isSystemPreviewable(). And of course HTMLAnchorElement::isSystemPreviewable() will have the real logic.

[Dean Jackson]

Committed r230788: <https://trac.webkit.org/changeset/230788>

[Dean Jackson]

Thanks Said. I'll make follow-up fixes.