[Webkit-unassigned] [Bug 30555] Add layoutTestContoller.dumpCounterValues

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Wed Oct 21 10:34:54 PDT 2009


https://bugs.webkit.org/show_bug.cgi?id=30555


Darin Adler <darin at apple.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #41559|review?                     |review-
               Flag|                            |




--- Comment #8 from Darin Adler <darin at apple.com>  2009-10-21 10:34:54 PDT ---
(From update of attachment 41559)
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

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.



More information about the webkit-unassigned mailing list