Bug 100162 – [WebKit2 API] Add properties to get textRects from a WKDOMRange or WKDOMNode

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

[Andy Estes]

[WebKit2 API] Add a method to get textRects from a WKDOMRange or WKDOMNode

[Andy Estes]

Created attachment 170246 [details]
Patch

[Sam Weinig]

Comment on attachment 170246 [details]
Patch

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

> Source/WebCore/dom/Range.cpp:1626
> +void Range::textRectsAfterUpdatingLayout(Vector<IntRect>& rects, bool useSelectionHeight, RangeInFixedPosition* inFixed) const
> +{
> +    ownerDocument()->updateLayoutIgnorePendingStylesheets();
> +    textRects(rects, useSelectionHeight, inFixed);
> +}

I would remove this and just call updateLayoutIgnorePendingStylesheets() where needed.

> Source/WebKit2/WebProcess/InjectedBundle/API/mac/WKDOMRange.h:43
> +- (NSArray *)textRects;

How about a property?

[Andy Estes]

Committed r132282: <http://trac.webkit.org/changeset/132282>

[Darin Adler]

Comment on attachment 170246 [details]
Patch

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

>> Source/WebCore/dom/Range.cpp:1626
>> +}
> 
> I would remove this and just call updateLayoutIgnorePendingStylesheets() where needed.

I don’t understand why Range has a member function, textRects, that requires updating layout. That seems like a design mistake. Functions that rely on layout should be in render objects, even if they do take a Range* as an argument. Generally speaking, DOM nodes should not have functions that answer questions about layout, unless they are needed because they are part of the public DOM API that can be called from JavaScript. And those that are part of the public DOM should contain the updateLayout function calls inside the member functions.

How did this design mistake get in? Who should I talk to, to help them understand this design principle? Maybe I should write a document explaining the concept?

[Andy Estes]

(In reply to comment #4)
> (From update of attachment 170246 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=170246&action=review
> 
> >> Source/WebCore/dom/Range.cpp:1626
> >> +}
> > 
> > I would remove this and just call updateLayoutIgnorePendingStylesheets() where needed.
> 
> I don’t understand why Range has a member function, textRects, that requires updating layout. That seems like a design mistake. Functions that rely on layout should be in render objects, even if they do take a Range* as an argument. Generally speaking, DOM nodes should not have functions that answer questions about layout, unless they are needed because they are part of the public DOM API that can be called from JavaScript. And those that are part of the public DOM should contain the updateLayout function calls inside the member functions.
> 
> How did this design mistake get in?

Trac tells me we started doing this in <http://trac.webkit.org/changeset/21635>, which fixed <rdar://problem/5084400> -[DOMRange lineBoxRects] returns an empty array when the DOM nodes have just been created.

> Who should I talk to, to help them understand this design principle? Maybe I should write a document explaining the concept?

You should talk to me.

Generally speaking I understand the design principle you describe, but I also see why we need to update layout in this case. For other DOM API functions where we depend on updated layout, like getClientRects(), we encapsulate this call in a WebCore member function. This is what I initially did here, too, although there was some disagreement about the name I chose (Range::textRectsAfterUpdatingLayout()).