[webkit-reviews] review denied: [Bug 213659] [iOS] Editable regions causes ~1% slowdown in PLT5 : [Attachment 402971] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has denied Daniel Bates <dbates at webkit.org>'s
request for review:
Bug 213659: [iOS] Editable regions causes ~1% slowdown in PLT5
https://bugs.webkit.org/show_bug.cgi?id=213659

Attachment 402971: Patch

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




--- Comment #9 from Darin Adler <darin at apple.com> ---
Comment on attachment 402971
  --> https://bugs.webkit.org/attachment.cgi?id=402971
Patch

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

I am confused about the lifetime of this setting. If WebCore::Page makes the
setting persist forever, then why does the UI process keep setting state back
to NO? What sets it to true on a new page if the web process crashes?

Generally a bit puzzled.

General idea seems fine, but the details seem to not be 100% solid. What is the
invariant here about how this gets set? When it does get set, what guarantees
everything is regenerated? How long does the flag last once it’s true? What can
set it back to false?

> Source/WebCore/page/Page.cpp:930
> +    auto frameView = makeRefPtr(mainFrame().view());

Seems strange that we are putting FrameView into a local variable if we are not
null checking it.

Also not sure why doing a ref/deref on it is particularly valuable here.

> Source/WebCore/page/SettingsBase.cpp:322
> +	  
m_page->setEditableRegionEnabled(m_page->settings().visibleDebugOverlayRegions(
) & EditableElementRegion);

This seems wrong. We don’t want to set this back to false if it was set to true
by _requestTextInputContextsInRect just because the debug overlay regions
switch was flipped.

> Source/WebCore/rendering/EventRegion.h:90
> +    void ensureEditableRegion() { m_editableRegion = Region { }; }

This implementation does not match the name. The name implies this function
will leave the editable region alone unless it doesn’t exist at all, in which
case it will create one. But the implementation will overwrite an existing
editable region with an empty one.

Is that what the caller wants? Can we name this more appropriately?

> Source/WebCore/rendering/RenderBlock.cpp:1267
> -	   if (!isTextControl())
> +	   if (page().isEditableRegionEnabled() && !isTextControl())
>	       needsTraverseDescendants |=
document().mayHaveEditableElements();

Must admit I don’t understand this one.

> Source/WebCore/rendering/RenderElement.cpp:749
> +	       if (page().isEditableRegionEnabled()) {

Maybe we can do the page() checking after the userModify checking? Should be
decided based on which is faster. Do the faster check first.

> Source/WebCore/rendering/RenderLayerBacking.cpp:1711
> -    if (renderer().document().mayHaveEditableElements())
> +    if (renderer().page().isEditableRegionEnabled() &&
renderer().document().mayHaveEditableElements())

Are these correctly ordered with the faster check first?

> Source/WebCore/rendering/RenderLayerBacking.cpp:1743
> +	   // The existence of the editable region, even if the document has 0
editable elements, is the
> +	   // performance optimization. It allows the UI process to avoid
messaging the web process to
> +	   // ask for editable elements when there are none. If there is no
editable region then the UI
> +	   // process must always message the web process because it does not
know whether or not the
> +	   // page has editable elements.

This long comment still doesn’t explain to me why the code below is correct or
needed. Maybe a shorter comment that focuses on why this code is beneficial
rather than trying to thoroughly describe the background.

> Source/WebCore/rendering/RenderLayerCompositor.cpp:2361
> +inline void
RenderLayerCompositor::applyToCompositedLayerIncludingDescendants(RenderLayer&
layer, const ApplyFunctionType& function)

The "inline" here is not needed. Won’t even make it more likely that it gets
inlined. Please omit.

> Source/WebCore/testing/InternalSettings.cpp:290
> +    m_page->setEditableRegionEnabled(true);

Seems an unfortunate default if it makes all tests slightly slower and also
tests the less common mode.

> Source/WebKit/UIProcess/RemoteLayerTree/ios/RemoteLayerTreeViews.mm:123
> +    bool seenAnEditableRegion = false;

Grammatically questionable to name this "seen". Maybe "saw"?

> Source/WebKit/UIProcess/ios/WKContentViewInteraction.mm:938
> +    _editableRegionEnabled = NO;

I don’t understand why this is helpful. Why do we want to forget whether we
have told the page to enable editable regions?

> Source/WebKit/UIProcess/ios/WKContentViewInteraction.mm:4303
> +    _editableRegionEnabled = NO;

Ditto.

> Source/WebKit/UIProcess/ios/WKContentViewInteraction.mm:5232
> +    if (!_editableRegionEnabled)

Is this if statement an important optimization? I suggest omitting the "if".