[webkit-reviews] review denied: [Bug 153137] Implement document.elementsFromPoint : [Attachment 315705] Patch

[bugzilla-daemon at webkit.org]

Simon Fraser (smfr) <simon.fraser at apple.com> has denied Ali Juma
<ajuma at chromium.org>'s request for review:
Bug 153137: Implement document.elementsFromPoint
https://bugs.webkit.org/show_bug.cgi?id=153137

Attachment 315705: Patch

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




--- Comment #25 from Simon Fraser (smfr) <simon.fraser at apple.com> ---
Comment on attachment 315705
  --> https://bugs.webkit.org/attachment.cgi?id=315705
Patch

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

Generally looks good but I'd like to see the suggested cleanup happen.

> Source/WebCore/dom/TreeScope.cpp:299
> +static bool absolutePointIfNotClipped(Document& document, const LayoutPoint&
clientPoint, LayoutPoint& absolutePoint)

Cool kids would return an optional<LayoutPoint>

> Source/WebCore/dom/TreeScope.cpp:367
> +Vector<Element*> TreeScope::elementsFromPoint(double x, double y)

Can we name these clientX, clientY (and rename args to elementFromPoint too)?

> Source/WebCore/dom/TreeScope.h:92
>      WEBCORE_EXPORT Element* elementFromPoint(double x, double y);
> +    WEBCORE_EXPORT Vector<Element*> elementsFromPoint(double x, double y);

I think elementFromPoint() should return a RefPtr<Element>, and
elementsFromPoint a Vector<RefPtr<Element>>.

> Source/WebCore/page/RuntimeEnabledFeatures.h:70
> +    void setElementsFromPointEnabled(bool isEnabled) {
m_elementsFromPointEnabled = isEnabled; }
> +    bool elementsFromPointEnabled() const { return
m_elementsFromPointEnabled; }

I don't think you need to add a runtime-enabled feature for this now.

> Source/WebCore/rendering/HitTestRequest.h:47
> +	   // Collect a list of nodes instead of just one. Used for
elementsFromPoint and rect-based tests.
> +	   CollectMultipleElements = 1 << 13,
> +	   // When using list-based testing, continue hit testing even after a
hit has been found.
> +	   InclusiveList = 1 << 14

I don't like InclusiveList. How about "IncludeAllElementsUnderPoint".

> Source/WebCore/rendering/HitTestRequest.h:70
> +    bool collectsMultipleElements() const { return m_requestType &
CollectMultipleElements; }

Maybe "resultIsElementList"? You call this a "list-based test" elsewhere.

> Source/WebCore/rendering/HitTestRequest.h:71
> +    bool inclusiveList() const { return m_requestType & InclusiveList; }

This also needs renaming.

> Source/WebCore/rendering/HitTestResult.cpp:678
> +	   return true;

Really hard to know what this boolean result means. Would be clearer with an
enum. Does it mean "continue testing"?

> Source/WebCore/rendering/HitTestResult.cpp:690
>	   return false;

Same with this one.

> Source/WebCore/rendering/HitTestResult.cpp:725
> +	   for (NodeSet::const_iterator it =
other.m_listBasedTestResult->begin(), last =
other.m_listBasedTestResult->end(); it != last; ++it)

auto. Can't we use a modern C++ loop with m_listBasedTestResult?

> Source/WebCore/rendering/HitTestResult.h:134
> +    // Returns true if the test is a list-based test and we should continue
testing.

Please make an enum for this.

> Source/WebCore/rendering/RenderView.cpp:186
> +    ASSERT(!location.isRectBasedTest() ||
request.collectsMultipleElements());

Is it really RenderView's job to make this assertion?

> Source/WebCore/rendering/svg/RenderSVGShape.cpp:356
> +	       LayoutPoint localLayoutPoint(localPoint);

Why is this local variable necessary?

> Source/WebKit/Shared/WebPreferencesDefinitions.h:294
> +    macro(ElementsFromPointEnabled, elementsFromPointEnabled, Bool, bool,
true, "ElementsFromPoint", "Enable document.elementsFromPoint support") \

No need for this.

> Source/WebKit/UIProcess/API/C/WKPreferences.cpp:1534
> +void WKPreferencesSetElementsFromPointEnabled(WKPreferencesRef
preferencesRef, bool enabled)
> +{
> +    toImpl(preferencesRef)->setElementsFromPointEnabled(enabled);
> +}
> +
> +bool WKPreferencesGetElementsFromPointEnabled(WKPreferencesRef
preferencesRef)
> +{
> +    return toImpl(preferencesRef)->elementsFromPointEnabled();
> +}

No need for this.

> Source/WebKit/UIProcess/API/C/WKPreferencesRefPrivate.h:510
> +// Defaults to true.
> +WK_EXPORT void WKPreferencesSetElementsFromPointEnabled(WKPreferencesRef,
bool enabled);
> +WK_EXPORT bool WKPreferencesGetElementsFromPointEnabled(WKPreferencesRef);

No need for this.

> Source/WebKit/WebProcess/WebPage/WebPage.cpp:3355
> +   
RuntimeEnabledFeatures::sharedFeatures().setElementsFromPointEnabled(store.getB
oolValueForKey(WebPreferencesKey::elementsFromPointEnabledKey()));

No need for this.

> Source/WebKitLegacy/mac/WebView/WebPreferenceKeysPrivate.h:192
> +#define WebKitElementsFromPointEnabledPreferenceKey
@"WebKitElementsFromPointEnabled"

No need for this.

> Source/WebKitLegacy/mac/WebView/WebPreferences.mm:680
> +	   @YES, WebKitElementsFromPointEnabledPreferenceKey,

No need for this.

> Source/WebKitLegacy/mac/WebView/WebPreferences.mm:3197
> +- (BOOL)elementsFromPointEnabled
> +{
> +    return [self
_boolValueForKey:WebKitElementsFromPointEnabledPreferenceKey];
> +}
> +
> +- (void)setElementsFromPointEnabled:(BOOL)flag
> +{
> +    [self _setBoolValue:flag
forKey:WebKitElementsFromPointEnabledPreferenceKey];
> +}

No need for this.

> Source/WebKitLegacy/mac/WebView/WebPreferencesPrivate.h:566
> +- (void)setElementsFromPointEnabled:(BOOL)flag;
> +- (BOOL)elementsFromPointEnabled;

No need for this.

> Source/WebKitLegacy/mac/WebView/WebView.mm:2998
> +   
RuntimeEnabledFeatures::sharedFeatures().setElementsFromPointEnabled([preferenc
es elementsFromPointEnabled]);

No need for this.

> Tools/DumpRenderTree/mac/DumpRenderTree.mm:968
> +    [preferences setElementsFromPointEnabled:YES];

No need for this.

> Tools/WebKitTestRunner/TestController.cpp:669
> +    WKPreferencesSetElementsFromPointEnabled(preferences, true);

No need for this.