On 8/4/2011 4:22 AM, Petr Vobornik wrote:
new version attached

Almost there, just a few more minor issues.

Also these changes should be reverted back to maintain the Ajax context:

- that.on_error.call(this, xhr, text_status, error_thrown);
+ that.on_error(xhr, text_status, error_thrown);

- that.on_success.call(this, data, text_status, xhr);
+ that.on_success(data, text_status, xhr);

Reverted back. Just for my information: ajax context is preserved for
some future use, or it is already used somewhere?

The Ajax context right now is only used to get the URL causing HTTP error (ipa.js:301). Things might have changed, I'm not sure how to generate HTTP error anymore. The URL can actually be obtained from the url variable in the execute() method, but there are other things that you can get from Ajax context that might be useful in the future. Try setting a breakpoint inside the success_handler() or error_handler() and inspect the 'this' variable. I think we should make sure the callback functions behave like real Ajax callback function to avoid future problems, so 'this' should always point to Ajax context.

There are actually a few places where the Ajax context doesn't get passed to the callback function:
 - ipa.js:409,418,428,431,436,620
 - host.js:155
A bunch of these are existing issues. We can fix them separately.

Another thing, in the init() you can access the spec object directly, so
don't really have to pass it as a parameter.

Yeah, I know. The purpose for this was to be able to call init method
again later (which was made public as xxx_init(spec)). But probably it
isn't in compliance with removes of public init methods.

The init() method that we removed recently was a method that was called to initialize the object after the metadata becomes available. In the past some objects were created before the metadata was available, but now it's no longer the case so the object can be created and initialized at the same time. There's nothing wrong with creating an init() method to encapsulate the initialization sequence, but it doesn't need to be made public like before because the subclasses no longer need to call it explicitly. No need to change anything here.

The default values in ipa.js:576-579 are redundant because they will be overridden by the spec in init(). I think the assignments in init() can be replaced by something like this:
    that.xhr = spec.xhr || {};
Note that the default value for xhr and error_thrown should be an empty object.

There are some unit test failures in ipa_tests.js because IPA.error_dialog used to point to the dialog instance. You might want to change it to get the instance using something else, e.g. element ID.

There are some other other unit test failures, but they seem to be caused by the earlier failure. They actually pass if run separately.

Endi S. Dewata

Freeipa-devel mailing list

Reply via email to