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

[bugzilla-daemon at webkit.org]

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





--- Comment #17 from Shinichiro Hamaji <hamaji at chromium.org>  2009-10-26 16:12:30 PDT ---
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!

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