[webkit-reviews] review granted: [Bug 180086] Alternative Presentation Button: Improve substitution heuristic : [Attachment 328350] Patch and layout test

[bugzilla-daemon at webkit.org]

Ryosuke Niwa <rniwa at webkit.org> has granted Daniel Bates <dbates at webkit.org>'s
request for review:
Bug 180086: Alternative Presentation Button: Improve substitution heuristic
https://bugs.webkit.org/show_bug.cgi?id=180086

Attachment 328350: Patch and layout test

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




--- Comment #33 from Ryosuke Niwa <rniwa at webkit.org> ---
Comment on attachment 328350
  --> https://bugs.webkit.org/attachment.cgi?id=328350
Patch and layout test

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

> Source/WebCore/editing/Editor.cpp:3866
> +	   for (auto* tagName : tagNames) {
> +	       if (element.hasTagName(*tagName))
> +		   return true;
> +	   }
> +	   return false;

Instead of for looping, we should create a NeverDestroyed HashSet.

> Source/WebCore/editing/Editor.cpp:3868
> +    auto findElementForAlternativePresentation = [&] (const
Vector<Ref<Element>>& ancestorElements) {

What's the point of having this inner lambda? It would a lot cleaner to have
this function as a separate static local function.

> Source/WebCore/editing/Editor.cpp:3904
> +    // Fix up the the list of elements to hide so that we do not hide the
element for alternative presentation.

Instead of having a comment like this, it's a lot better to extract a static
local function
of a descriptive name like elementsToHideExcludingAlternativePresentation.

> LayoutTests/fast/forms/alternative-presentation-button/replacement.html:43
> +	   <a>a</a>

You should add a test case with href: <a href="">

By the way, can't we use dump-as-markup.js in these tests? That would make the
output a lot more readable & platform independent.