Bug 239423

Created attachment 457755 [details]
For EWS

Comment on attachment 457755 [details]
For EWS

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

> Source/WebCore/dom/mac/ImageControlsMac.cpp:93
> +    controlLayer->setContentEditable("false"_s);

Mildly sad that we allocate and then deallocate the String containing "false" here. There is, however, a clean way to fix that. We can change HTMLElement::setContentEditable to take a StringView. Will still work with the String&& that the JavaScript bindings will pass to it. Probably will have to edit DOMHTMLElement.mm to keep it compiling since an NSString * knows how to turn itself into a String but not a StringView. Not really necessary to do *any* of this as part of this patch, just something I am noticing in passing.

Comment on attachment 457755 [details]
For EWS

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

>> Source/WebCore/dom/mac/ImageControlsMac.cpp:93
>> +    controlLayer->setContentEditable("false"_s);
> 
> Mildly sad that we allocate and then deallocate the String containing "false" here. There is, however, a clean way to fix that. We can change HTMLElement::setContentEditable to take a StringView. Will still work with the String&& that the JavaScript bindings will pass to it. Probably will have to edit DOMHTMLElement.mm to keep it compiling since an NSString * knows how to turn itself into a String but not a StringView. Not really necessary to do *any* of this as part of this patch, just something I am noticing in passing.

I totally agree that setContentEditable() should take a StringView. I also think that the most efficient code may be:
controlLayer->setAttributeWithoutSynchronization(contenteditableAttr, "false"_s);
or even
controlLayer->setAttributeWithoutSynchronization(contenteditableAttr, HTMLElement::falseName());
if we expose falseName() in the HTMLElement header (it is currently in the cpp only).

To avoid the string comparisons inside setContentEditable().

Thanks for the reviews, Chris and Darin!

(In reply to Chris Dumez from comment #3)
> Comment on attachment 457755 [details]
> For EWS
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=457755&action=review
> 
> >> Source/WebCore/dom/mac/ImageControlsMac.cpp:93
> >> +    controlLayer->setContentEditable("false"_s);
> > 
> > Mildly sad that we allocate and then deallocate the String containing "false" here. There is, however, a clean way to fix that. We can change HTMLElement::setContentEditable to take a StringView. Will still work with the String&& that the JavaScript bindings will pass to it. Probably will have to edit DOMHTMLElement.mm to keep it compiling since an NSString * knows how to turn itself into a String but not a StringView. Not really necessary to do *any* of this as part of this patch, just something I am noticing in passing.
> 
> I totally agree that setContentEditable() should take a StringView. I also
> think that the most efficient code may be:
> controlLayer->setAttributeWithoutSynchronization(contenteditableAttr,
> "false"_s);
> or even
> controlLayer->setAttributeWithoutSynchronization(contenteditableAttr,
> HTMLElement::falseName());
> if we expose falseName() in the HTMLElement header (it is currently in the
> cpp only).
> 
> To avoid the string comparisons inside setContentEditable().

That's a great point — I filed <https://webkit.org/b/239424> to track refactoring setContentEditable to take a StringView; I'll also tweak this patch so that it does:

```
controlLayer->setAttributeWithoutSynchronization(contenteditableAttr, "false"_s);
```

...instead of using `setContentEditable()`.

Created attachment 457758 [details]
For landing

Committed r292944 (249709@main): <https://commits.webkit.org/249709@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 457758 [details].