[Webkit-unassigned] [Bug 30555] Provide a way to get counter values with layoutTestContoller

Sun Oct 25 12:47:19 PDT 2009

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

Darin Adler <darin at apple.com> changed:

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

--- Comment #12 from Darin Adler <darin at apple.com>  2009-10-25 12:47:19 PDT ---
(From update of attachment 41648)
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.

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