Start removing functions that implicitly use composed tree

• Component: DOM
• Status: RESOLVED FIXED

[Darin Adler]

Start removing functions that implicitly use composed tree: documentOrder(Node, Node)

[Darin Adler]

Created attachment 412866 [details]
Patch

[Darin Adler]

Ryosuke made the comments/suggestions that led me to create this patch:

1) Ordering based on the composed tree is likely to often *not* be what we want, so we are making that explicit now so we can carefully look over each case.

2) Testing with internals is a better approach than TestWebKitAPI for these functions, or even consider testing them indirectly rather than directly unit testing them at all.

[Darin Adler]

Next steps:

- Make this explicit for *all* functions that implicitly order based on the composed tree. There are a few more. (Will be more difficult for the < and > family of operators for the Position class.)

- Change the function templates to default to the normal DOM tree, removing <Tree> at call sites that currently specify it explicitly.

- Visit each of the call sites using <ComposedTree> and change them to not use it any more unless there is a good reason.

[Darin Adler]

Also:

- Expand the test added here to cover other trees, currently it tests only treeOrder<ComposedTree>.

- Implicitly: Convert more of the tests to internals-based tests in the LayoutTests directory as needed since we are removing the functions they test.

[Darin Adler]

Created attachment 412870 [details]
Patch

[Darin Adler]

Created attachment 412872 [details]
Patch

[Ryosuke Niwa]

Comment on attachment 412872 [details]
Patch

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

> Source/WebCore/testing/Internals.cpp:6011
> +    if (is_eq(ordering))
> +        return "equivalent"_s;

Isn't this usually called "equal"?

[Sam Weinig]

Comment on attachment 412872 [details]
Patch

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

> Source/WebCore/dom/SimpleRange.h:72
> +WEBCORE_EXPORT bool containsForTesting(TreeType, const SimpleRange& outerRange, const SimpleRange& innerRange);
> +WEBCORE_EXPORT bool containsForTesting(TreeType, const SimpleRange&, const Node&);

I assume this is what you were referring to in Slack the other day. 

I believe you can avoid this in cases like these, where you only ever expect a finite number of specializations, but the syntax often eludes me. Something along the lines of:

template<TreeType> bool contains(const SimpleRange& outerRange, const SimpleRange& innerRange);
WEBCORE_EXPORT template <> void contains<Tree>(const SimpleRange&, const SimpleRange&); 
WEBCORE_EXPORT template <> void contains<ShadowIncludingTree>(const SimpleRange&, const SimpleRange&); 
WEBCORE_EXPORT template <> void contains<ComposedTree>(const SimpleRange&, const SimpleRange&);

[Darin Adler]

(In reply to Sam Weinig from comment #8)
> Comment on attachment 412872 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=412872&action=review
> 
> > Source/WebCore/dom/SimpleRange.h:72
> > +WEBCORE_EXPORT bool containsForTesting(TreeType, const SimpleRange& outerRange, const SimpleRange& innerRange);
> > +WEBCORE_EXPORT bool containsForTesting(TreeType, const SimpleRange&, const Node&);
> 
> I assume this is what you were referring to in Slack the other day. 
> 
> I believe you can avoid this in cases like these, where you only ever expect
> a finite number of specializations, but the syntax often eludes me.
> Something along the lines of:
> 
> template<TreeType> bool contains(const SimpleRange& outerRange, const
> SimpleRange& innerRange);
> WEBCORE_EXPORT template <> void contains<Tree>(const SimpleRange&, const
> SimpleRange&); 
> WEBCORE_EXPORT template <> void contains<ShadowIncludingTree>(const
> SimpleRange&, const SimpleRange&); 
> WEBCORE_EXPORT template <> void contains<ComposedTree>(const SimpleRange&,
> const SimpleRange&);

Yes, I have tried many variations of this and so far can’t get it to work.

[Darin Adler]

(In reply to Ryosuke Niwa from comment #7)
> Comment on attachment 412872 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=412872&action=review
> 
> > Source/WebCore/testing/Internals.cpp:6011
> > +    if (is_eq(ordering))
> > +        return "equivalent"_s;
> 
> Isn't this usually called "equal"?

There is a distinction between equivalent and equal in the C++ standard. However, in this case we could use either term. The term equivalent is used to express that there may be multiple values that are distinct but are equivalent. In this case that is not true, so either equivalent or equal would be fine.

[Darin Adler]

(In reply to Sam Weinig from comment #8)
> I believe you can avoid this in cases like these, where you only ever expect
> a finite number of specializations, but the syntax often eludes me.

I think you are talking about "explicit instantiation". The existing code does that inside the .cpp file to cause the appropriate version to be instantiated, so that callers outside the .cpp file have something to link to.

It’s documented pretty well here <https://en.cppreference.com/w/cpp/language/function_template>.

What have been unable to figure out is how to successfully combine this with WEBCORE_EXPORT.

[Darin Adler]

Committed r269253: <https://trac.webkit.org/changeset/269253>

[Radar WebKit Bug Importer]

<rdar://problem/70957462>

[Darin Adler]

(In reply to Darin Adler from comment #11)
> What have been unable to figure out is how to successfully combine this with
> WEBCORE_EXPORT.

When I try putting WEBCORE_EXPORT only on explicit instantiation lines in the .cpp file, I get a weak external symbol failure in our "Check For Weak VTables and Externals" shell script.

When I try putting the WEBCORE_EXPORT explicit instantiation lines into the .h file, I get "error: explicit instantiation of undefined function template".

[Darin Adler]

(In reply to Darin Adler from comment #14)
> When I try putting the WEBCORE_EXPORT explicit instantiation lines into the
> .h file, I get "error: explicit instantiation of undefined function
> template".

Oh, maybe I need to try this again and use the keyword "extern" before "template".