Bug 30555 – Provide a way to get counter values with layoutTestContoller

• Component: Tools / Tests
• Status: RESOLVED FIXED

[Shinichiro Hamaji]

dumpAsText() (which uses innerText internally) doesn't output the values of CSS counters. So, we need to do pixel tests for CSS counter tests. Let's add layoutTestContoller.dumpCounterNode and eliminate pixel tests.

[Shinichiro Hamaji]

Created attachment 41488 [details]
Patch v1

[Darin Adler]

I like that this makes it possible to have text-only tests of counter behavior. But I don't like the fact that this dumps the internal counter data structure, because I think that data structure isn't so great and may need to be changed. Can we instead come up with something that will dump the actual counter value generated text that will be displayed rather than exposing the tree?

[Shinichiro Hamaji]

Created attachment 41553 [details]
Patch v2

[Shinichiro Hamaji]

Thanks for the comment! Yes, I see. Dumping counter tree was a bad idea. I modified my patch so that it outputs texts in counters. Now we can check more complicated counter texts like "α.β".

Comment on attachment 41553 [details]
Patch v2

> +void dumpCounterNode(RenderObject* o)
> +{
> +    for (; o; o = o->nextInPreOrder()) {
> +        if (o->isCounter()) {
> +            String str(toRenderText(o)->originalText().releaseRef());
> +            printf("%s\n", str.utf8().data());
> +        }
> +    }
> +}

Using printf() here is inelegant and limits the usefulness of the code. I think a function that returns a string would be better and more flexible. You could decide to dump the value in DumpRenderTree, but you could also just expose it to the test through the LayoutTestController.

It’s somewhat confusing that a function called dumpCounterNode() doesn’t take a counter as its argument. Is the argument even necessary?

[Shinichiro Hamaji]

Created attachment 41559 [details]
Patch v3

[Shinichiro Hamaji]

Thanks for the review!

> Using printf() here is inelegant and limits the usefulness of the code. I think
> a function that returns a string would be better and more flexible. You could
> decide to dump the value in DumpRenderTree, but you could also just expose it
> to the test through the LayoutTestController.

I see. I update my patch and it returns String. 

> It’s somewhat confusing that a function called dumpCounterNode() doesn’t take a
> counter as its argument. Is the argument even necessary?

The argument is the root node of render tree and dumpCounterNode (this name was gone in my new patch) outputted all counters in the render tree. I think the name of this function was wrong. Now I changed the name to counterValuesFromRenderTree. I hope this name sounds better.

Also, as now we don't output counter tree, I renamed the dump function from dumpCounterNode to dumpCounterValues.

[Darin Adler]

Comment on attachment 41559 [details]
Patch v3

This looks OK.

For our more modern tests, we do more than just avoid dumping the render tree. We also like do to targeted tests and use the script-tests system so we can test lots of things in a clear way in a single test. For those purposes, the most useful kind of feature would be one where we could get the counter text for a particular element in the document. We would want a JavaScript function to return that text given the element, not one that dumps all the counters in an entire document. That kind of feature would enable us to make thorough, easy to understand regression tests using the script-tests machinery and would be a lot more valuable than a dump feature.

It looks to me like you modeled the interface of your counterValuesFromRenderTree function on the existing externalRepresentation function in RenderTreeAsText.h. The design of that function is not good, and not something to be emulated.

First, it does a layout() call in an unsafe way. After calling layout(), existing render object pointers may be invalid, so if the function needs to do layout it should take a different argument. I think it probably should take a Document* or perhaps a Node* or Element*. At some point someone should fix that.

Second, it seems to promise more generality than it delivers. It takes an arbitrary RenderObject, but if you ignore the SVG quirks, it won't dump anything for a node that does not have a RenderLayer and thus for any renderer that is not a RenderBox. So I don't think it's helpful to copy its design.

Your new function has an unusual argument. It takes a RenderObject* and then dumps counter values starting there, moving on forward in document order to the end of the document. I would suggest instead that you do one of these things:

    1) Take my comment above to heart and instead add a function that gives the counter value for just one node. The function would take either a Node* or an Element* and get the corresponding render object, then the counter return the text that would be rendered.

    2) Change the function to take a Document* instead of a RenderObject* if its purpose remains to dump the entire document. Also add a call to layout(). It's not right to expose something to JavaScript that won't work if layout hasn't already happened. We can talk about this more if you decide to stick with this design against my recommendation.

For the tests you are converting, you can easily write something in JavaScript that will iterate the document and dump all the counter values. Those tests should be the exception rather than the rule. Future tests can use a more modern design.

My other concern about this patch is that it only seems to take care of the Mac platform. When adding something like this, please do as many platforms as you can. The Windows platform used by my team here at Apple is almost always kept up to date when making a change like this, and I'd like to see at least that.

> +            String str(toRenderText(o)->originalText().releaseRef());

There are two major things wrong here:

    A) It's not good to test by calling originalText here. We want to test the entire counter machinery, so you should be using the actual text. You should get that with the text() function rather than the originalText() function. If you had problems with that before, we should figure out why. One possibility is that this was caused by the lack of a layout() call as mentioned above. I'm not even sure why the originalText() function is public. In the future we might fix this and make it private as it should be.

    B) This has an unnecessary storage leak in it. You should almost never call releaseRef() and it is definitely incorrect here. Instead you would call get() here.

Please consider making a more useful general purpose tool here. Since there's quite a bit of work to hook this up to DumpRenderTree it would be best to get the most out of the effort. But at a minimum, we should not add something like this in a way that's unnecessarily fragile because it depends on layout state (see comment [2] above).

review- because of the comments above

[Shinichiro Hamaji]

Created attachment 41646 [details]
Patch v4

[Shinichiro Hamaji]

Created attachment 41648 [details]
Patch v5

[Shinichiro Hamaji]

Thanks for the detailed comments. The advises were really helpful!

> For our more modern tests, we do more than just avoid dumping the render tree.
> We also like do to targeted tests and use the script-tests system so we can
> test lots of things in a clear way in a single test. For those purposes, the
> most useful kind of feature would be one where we could get the counter text
> for a particular element in the document. We would want a JavaScript function
> to return that text given the element, not one that dumps all the counters in
> an entire document. That kind of feature would enable us to make thorough, easy
> to understand regression tests using the script-tests machinery and would be a
> lot more valuable than a dump feature.

I see. In my new patch, I've added layoutTestContoller.getCounterValueById, instead of dumpCounterNode.

>     1) Take my comment above to heart and instead add a function that gives the
> counter value for just one node. The function would take either a Node* or an
> Element* and get the corresponding render object, then the counter return the
> text that would be rendered.

I like this approach. I have a few questions:

1. As inserted/removed DOM elements aren't layouted
   (element->renderer() is NULL) when the element is added, I needed
   to wait the layout using setTimeout before I obtain counter values
   by getCounterValueById. Is this unavoidable? I think calling
   layout() here is not good as same reason as externalRepresentation
   you mentioned. If there are no way to avoid this limitation,
   WebCore::getCounterValue should return a warning message like "this
   element is not layouted yet!" to relax potential confusion?

2. I'm not sure if the file I added functions are correct. I've added:

   - WebFrame(WebKitDebug).getCounterValue in WebKit/mac/Misc/WebCoreStatistics
   - WebCore::getCounterValue in WebCore/rendering/RenderTreeAsText

   If there are better places for them, please let me know and I'll
   move them.

> My other concern about this patch is that it only seems to take care of the Mac
> platform. When adding something like this, please do as many platforms as you
> can. The Windows platform used by my team here at Apple is almost always kept
> up to date when making a change like this, and I'd like to see at least that.

OK, I'll try working on other platforms as well. But, I don't want to update my patch for many platforms several times. So, I'd like to work with the following scheme:

1. Fix the design of the API using the patch for Mac.
2. Once Mac patch becomes good, I'll create patches for other platforms.
3. All of them get review+, then I check them in at once.

Is this OK?

[Darin Adler]

Comment on attachment 41648 [details]
Patch v5

Thanks very much for keeping at this. I've been out sick for a few days which is why this round of review was delayed.

> +  <script src="../../js/resources/js-test-pre.js"></script>

The js-test-pre.js file is part of the script-tests machinery. Normally we use either the entire script-tests machinery or none of it. It's a little strange to have a hybrid test like this, but I think it's OK for now.

> +  function checkCounterValues() {

WebKit code style puts the brace on the following line, not the same line as the function name.

> +      debug('<br /><span class="pass">TEST COMPLETE</span>');

We normally do not use the debug function with strings that include markup. And we definitely don't do "<br />" -- there's no need for that slash in HTML and this is not XHTML even if we pretend it is. This is probably OK as-is, but it would be cleaner to just do the usual:

    debug('');
    debug('TEST COMPLETE');

> +      // Eliminate confusing messages (counter values won't be dumped by dumpAsText).
> +      document.getElementById("view").innerHTML = '';

This can be done more cleanly by removing the view element from its parent with core DOM methods instead of using innerHTML.

    var viewElement = document.getElementById("view");
    viewElement.parentNode.removeChild(viewElement);

But that's only slightly better.

> +          // Wait for the layout using setTimeout.
> +          setTimeout('checkCounterValues()', 0);

This is the wrong way to do it. The new function you added to RenderTreeAsText.cpp should request the layout since it is getting values that depend on layout, and it should not be left up to the JavaScript code in the test to do it. And using a timeout is not a reliable way to cause layout to happen.

The code to cause the layout is simple. Just add this to the function:

    element->document()->updateLayout();

As a side note, not your responsibility but worth doing eventually, the externalRepresentation function needs to be changed to trigger layout this way instead of the code it currently has using frameView(); and it should also take a Frame* instead of a RenderObject* since it already assumes it's dumping an entire frame, and causing layout can invalidate a RenderObject* so it's not a safe interface.

> +static void getCounterValueInAnonymous(TextStream& ts, RenderObject* o)

We normally use argument names that are words, not just letters. This file uses "ts" so much that it seems OK to follow that, although I would have used "stream", but "o" instead should be "renderer" or maybe "parent" if you change this function to have "children" in its name (see below).

Normally we would not use "get" in the name of a function that adds something to a stream. The verb "get" is used normally for a function that has an out argument with a return value. The verb for a function that adds something to a text stream should be "write" (see examples throughout this source file).

The name here says "in anonymous", but "anonymous" is an adjective without a noun and so a bit confusing and oblique. I would call this function "writeCounterValuesFromChildren", since it appends the values of any children that are counters. There's nothing in it specific to an anonymous renderer so I don't think the function needs "anonymous" in its name.

> +String getCounterValue(Element* element)

A function like this should not have "get" in its name inside WebKit. We name getter functions with return values with noun phrases, and not the verb "get".

> +    // The counter renderer should be children of anonymous children
> +    // (i.e., :before or :after pseudo-elements).

I think you mean "renderers" here not "renderer". Or you could leave it as "renderer" and say "one of the children of". I think that if we actually used this to test a case with more than one counter per node we might want to change the syntax to use some kind of separator, maybe " " or "," or ", ", but for now it's OK since the test never uses it for any case like that.

> +    if (RenderObject* o = element->renderer()) {

I would use "renderer" here rather than "o" for the local variable name.

> +- (NSString *)getCounterValue:(DOMElement*)element;

To follow Objective-C method naming, this should be named counterValueForElement: rather than getCounterValue:.

> +static JSValueRef getCounterValueByIdCallback(JSContextRef context, JSObjectRef function, JSObjectRef thisObject, size_t argumentCount, const JSValueRef arguments[], JSValueRef* exception)

It seems OK to do it this way. I suppose you were copying what the existing function elementDoesAutoCompleteForElementWithId does.

It would be more powerful to have a command that took an arbitrary element pointer but I suppose it's slightly harder to use the DOM in layout test controller code since there's no cross-platform way to handle DOM operations. You could, though, just pass a JSValueRef or JSObjectRef through to the LayoutTestController and let it handle converting that into an DOM object. But that's a lot of pioneering work to ask for and you've already done much -- I think this is fine for now.

> +    JSRetainPtr<JSStringRef> elementId(Adopt, JSValueToStringCopy(context, arguments[0], exception));
> +    ASSERT(!*exception);

Why do we have an assert here? There's no guarantee that converting the value to a string won't raise an exception, and there need not be a guarantee, so there should not be an assert. Instead we should just be sure to return from the function if an exception occurs.

Did you copy this from existing layout test controller functions? If so, they are doing things wrong. It's not right to assert something you can't guarantee.

> +    JSRetainPtr<JSStringRef> counterValue(Adopt, controller->getCounterValueById(elementId.get()));

A function with this name should return a JSRetainPtr<JSStringRef>, not a raw JSStringRef. If you want to return a raw JSStringRef, then the function needs "copy" or "create" in its name. Other functions in LayoutTestController are not all doing this right. The best thing to do is to:

    1) Name the function counterValueForElementById.
    2) Have it return a JSRetainPtr<JSStringRef>.

The one thing here mandatory to fix is the layout thing. The rest of the comments are good ideas, but optional. review- so you can fix at least the mandatory part.

[Darin Adler]

(In reply to comment #11)
> 1. As inserted/removed DOM elements aren't layouted
>    (element->renderer() is NULL) when the element is added, I needed
>    to wait the layout using setTimeout before I obtain counter values
>    by getCounterValueById. Is this unavoidable? I think calling
>    layout() here is not good as same reason as externalRepresentation
>    you mentioned. If there are no way to avoid this limitation,
>    WebCore::getCounterValue should return a warning message like "this
>    element is not layouted yet!" to relax potential confusion?

A call to updateLayout() will solve this problem as I mentioned in my previous.

The problem with externalRepresentation is that it does this while holding a render object pointer in a local variable, which is illegal. The problem does not affect your new function since it takes an Element*, although to be 100% safe the Element* should go into a RefPtr<Element> local variable since otherwise updateLayout() could have a side effect of deleting the element. So you should put the element argument into a local variable of type RefPtr<Element>.

By the way, the issue isn't only the fact that the renderer() returns 0. Even if it returned a non-0 value, it might be a deallocated object.

>    - WebFrame(WebKitDebug).getCounterValue in WebKit/mac/Misc/WebCoreStatistics

Seems not such a great place for this, but fine to be consistent with the existing method already there.

>    - WebCore::getCounterValue in WebCore/rendering/RenderTreeAsText

Seems a fine place for this.

> Is this OK?

Sure, fine to do this one platform at a time, but it's important that you land expected results for each of the other platforms that match the incorrect results they will get until they implement this DumpRenderTree feature. That way the tree stays green for all platforms.

[Shinichiro Hamaji]

Created attachment 41910 [details]
Patch v6

[Shinichiro Hamaji]

Comment on attachment 41910 [details]
Patch v6

Oops, I forgot to update the ChangeLogs... Will upload again soon later.

[Shinichiro Hamaji]

Created attachment 41912 [details]
Patch v7

[Shinichiro Hamaji]

I think I fixed the names of variables and functions as you suggested. I'll reply for other topics.

> WebKit code style puts the brace on the following line, not the same line as
> the function name.

Ah, I forgot about this rule here. Fixed.

> We normally do not use the debug function with strings that include markup. And
> we definitely don't do "<br />" -- there's no need for that slash in HTML and
> this is not XHTML even if we pretend it is. This is probably OK as-is, but it
> would be cleaner to just do the usual:
> 
>     debug('');
>     debug('TEST COMPLETE');

I see. I fixed this with two debug() as you suggested.

> This can be done more cleanly by removing the view element from its parent with
> core DOM methods instead of using innerHTML.
> 
>     var viewElement = document.getElementById("view");
>     viewElement.parentNode.removeChild(viewElement);
> 
> But that's only slightly better.

Fixed.

> This is the wrong way to do it. The new function you added to
> RenderTreeAsText.cpp should request the layout since it is getting values that
> depend on layout, and it should not be left up to the JavaScript code in the
> test to do it. And using a timeout is not a reliable way to cause layout to
> happen.
> 
> The code to cause the layout is simple. Just add this to the function:
> 
>     element->document()->updateLayout();
> 
> As a side note, not your responsibility but worth doing eventually, the
> externalRepresentation function needs to be changed to trigger layout this way
> instead of the code it currently has using frameView(); and it should also take
> a Frame* instead of a RenderObject* since it already assumes it's dumping an
> entire frame, and causing layout can invalidate a RenderObject* so it's not a
> safe interface.

Ah, now I understand what you meant on externalRepresentation. Sorry I didn't understand in the previous round... I'll try fixing externalRepresentation, too. Now this function calls updateLayout and have a RefPtr<Element*> to make sure that the element is not freed.

> I think you mean "renderers" here not "renderer". Or you could leave it as
> "renderer" and say "one of the children of". I think that if we actually used
> this to test a case with more than one counter per node we might want to change
> the syntax to use some kind of separator, maybe " " or "," or ", ", but for now
> it's OK since the test never uses it for any case like that.

I'd like to separate the patch for this change. I'll do this with some tests soon after this bug is fixed.

> > +static JSValueRef getCounterValueByIdCallback(JSContextRef context, JSObjectRef function, JSObjectRef thisObject, size_t argumentCount, const JSValueRef arguments[], JSValueRef* exception)
> 
> It seems OK to do it this way. I suppose you were copying what the existing
> function elementDoesAutoCompleteForElementWithId does.
> 
> It would be more powerful to have a command that took an arbitrary element
> pointer but I suppose it's slightly harder to use the DOM in layout test
> controller code since there's no cross-platform way to handle DOM operations.
> You could, though, just pass a JSValueRef or JSObjectRef through to the
> LayoutTestController and let it handle converting that into an DOM object. But
> that's a lot of pioneering work to ask for and you've already done much -- I
> think this is fine for now.

Yeah, first I tried to make this function take a DOM element as the argument, but I couldn't figure out how. Also, I thought specifying an ID as a string is acceptable. Please let me go with the current way.

> > +    JSRetainPtr<JSStringRef> elementId(Adopt, JSValueToStringCopy(context, arguments[0], exception));
> > +    ASSERT(!*exception);
> 
> Why do we have an assert here? There's no guarantee that converting the value
> to a string won't raise an exception, and there need not be a guarantee, so
> there should not be an assert. Instead we should just be sure to return from
> the function if an exception occurs.
> 
> Did you copy this from existing layout test controller functions? If so, they
> are doing things wrong. It's not right to assert something you can't guarantee.

Yes, I copied this from other code around here. I fixed this so this function raises an exception if toString throws an exception.

I also made this function return undefined when the element specified by the ID is not found.

Now it seems we are getting close to fix the API. I'll start working for other platforms. Thanks again for your review!

[Darin Adler]

Comment on attachment 41912 [details]
Patch v7

Looks good.

>   the purpose of this test. That's why we use timeout for this test. -->
>   <body onload="setTimeout('run()', 0)">

Now that we have a countervalueForElementById function you could make the test work without a setTimeout.

The first set of calls to counterValueForElementById will force a layout and compute the counter values. So then you will have a valid test of a subsequent dynamic change. The test will write out the values both before and after the dynamic change.

I don't see any need to have setTimeout any more.

> +    if (!element)
> +        return NULL;

We use "0", not "NULL", for this in WebKit code.

I am not sure that you included changes to make bots stay green on non-Mac platforms, either by skipping the tests or landing expected results that expect the test to fail. If you did, I could say review+ and we could land this first version.

For now, review- because of the two comments above and my uncertainty about what you are doing for the other platforms at this time.

[Shinichiro Hamaji]

Created attachment 41928 [details]
Patch v8

[Shinichiro Hamaji]

Created attachment 41929 [details]
For gtk port

[Shinichiro Hamaji]

Created attachment 41930 [details]
For qt port

[Shinichiro Hamaji]

> Now that we have a countervalueForElementById function you could make the test
> work without a setTimeout.
> 
> The first set of calls to counterValueForElementById will force a layout and
> compute the counter values. So then you will have a valid test of a subsequent
> dynamic change. The test will write out the values both before and after the
> dynamic change.

I see. Fixed.

> We use "0", not "NULL", for this in WebKit code.

Fixed.

> I am not sure that you included changes to make bots stay green on non-Mac
> platforms, either by skipping the tests or landing expected results that expect
> the test to fail. If you did, I could say review+ and we could land this first
> version.
> 
> For now, review- because of the two comments above and my uncertainty about
> what you are doing for the other platforms at this time.

I was planning to commit my change after my patches for other platforms become ready. So, I think we don't need to push expected failures, right? Now I've just uploaded the patches for gtk and qt port. I'll work on windows tomorrow (I'm downloading service pack for Visual Studio now :). Also, I want to add the same method for chromium's test shell before I commit this change.

[Darin Adler]

(In reply to comment #22)
> I was planning to commit my change after my patches for other platforms become
> ready.

If so, then you should post a single combined patch for review. We don't want to keep reviewing pieces. It's not helpful to have patches up for review that you won't be landing -- just makes extra work for reviewers.

Please clear the review flags until you have a patch you think is suitable to land.

[Shinichiro Hamaji]

(In reply to comment #23)
> If so, then you should post a single combined patch for review. We don't want
> to keep reviewing pieces. It's not helpful to have patches up for review that
> you won't be landing -- just makes extra work for reviewers.
> 
> Please clear the review flags until you have a patch you think is suitable to
> land.

I see. I'll combine my patches into one patch and upload again.

[Shinichiro Hamaji]

Created attachment 41995 [details]
Patch v9

[Shinichiro Hamaji]

OK, I think the new patch supports windows, gtk, and qt in addition to mac.

[Darin Adler]

Comment on attachment 41995 [details]
Patch v9

r=me

[Shinichiro Hamaji]

Committed r50233: <http://trac.webkit.org/changeset/50233>

[Tor Arne Vestbø]

(In reply to comment #26)
> OK, I think the new patch supports windows, gtk, and qt in addition to mac.

Thanks for keeping the Qt port in mind when doing these type of changes, we
really appreciate it! :)

One note though: the Qt API should not be extended without a Qt person
reviewing the API. This means all classes in WebKit/qt/Api

In this case the API was added to support the DRT, in which case we use
helper-functions instead of API int he class. You can see how we import these
functions at the top of LayoutTestControllerQt.cpp

Revision 50166 is an example of moving functions from class API to helper
functions, after a change that had the same problem.

Again, we really appreciate the consideration of adding DRT callbacks for the
Qt port too when making these changes, but in the future please use the q_drt
format for these callbacks.

Thanks!! :)

[Shinichiro Hamaji]

> In this case the API was added to support the DRT, in which case we use
> helper-functions instead of API int he class. You can see how we import these
> functions at the top of LayoutTestControllerQt.cpp

I fixed this in Bug 30882. Thanks for letting me know the proper way to add an API for DTR.

[Shinichiro Hamaji]

Also, thanks Darin for your quick and insightful reviews on this bug!

[Mark Rowe (bdash)]

This change broke using ToT WebKit with the released version of Safari on Windows.  See bug 30938 for details.  When adding new methods to interfaces they need to be added to the *end* of the interface to avoid changing the order of methods in the vtable.

[Shinichiro Hamaji]

(In reply to comment #32)
> This change broke using ToT WebKit with the released version of Safari on
> Windows.  See bug 30938 for details.  When adding new methods to interfaces
> they need to be added to the *end* of the interface to avoid changing the order
> of methods in the vtable.

Ah, I see... Sorry, and thanks for fixing this.