342638 - move nsIXFormsUtilityService into core

Status: RESOLVED FIXED

(Core Graveyard :: XForms, defect)

Description

[alexander :surkov (:asurkov)]

Comment 1

[alexander :surkov (:asurkov)]

Attachment: patch

Comment 2

[alexander :surkov (:asurkov)]

Now xpath component provides nsIXFormsUtilityService interface to work with xforms and it is hosted in xpath component directory. If we need xforms interface to be used by multiple components (now these are xpath and accessible), then we'll have to have common place to host it in the core.

cc'ing bz to be sure chosen place in the core is right.

Comment 3

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Why is this needed? Why can't other code access the XFormsUtilityService even if lives in the xforms library?

Additionally, the xforms xpath code is moving out of the core code in bug 278981.

Comment 4

[aaronr]

(In reply to comment #3)
> Why is this needed? Why can't other code access the XFormsUtilityService even
> if lives in the xforms library?
> 
> Additionally, the xforms xpath code is moving out of the core code in bug
> 278981.
> 


I thought that we couldn't require core code to build XForms.  So I suggested that the interface (either defined in .idl or .h) be checked into the core.  Since the XPath needed an interface and accessibility needs an interface, it is conceivable that in the future yet another core component will need the ability to interact with XForms if it is available.  That is why I suggested that it live in a more 'common' component that is often required by other components.

We are definitely open to other ideas, though.

Comment 5

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Ah, i see what you're doing. I think i would prefer not to have a specific xforms directory though, so maybe content/base/public would be better. Don't feel strongly about it though.

Comment 6

[Boris Zbarsky [:bzbarsky]]

Do we just need the _interface_ in core?  Or also the implementation?

Comment 7

[aaronr]

(In reply to comment #6)
> Do we just need the _interface_ in core?  Or also the implementation?
> 

For XPath, we currently only have the interface in the core and use do_GetService to get to the functions that live in the xforms dll.  We've been able to keep most  XForms-specific code on the XForms side.  I would assume that accessibility and any other core component that might need to interface with XForms would also have component-specific functionality that would interact with XForms but that this code wouldn't run if the service weren't found.

Alex, I'd suggest that the interface be non-scriptable and perhaps keep it in a header and not as an interface at all.  It is only meant to allow other Mozilla components a chance to interact with XForms.  I don't know that we want to support JS, Java, etc. interacting with XForms that way.  Also keep in mind that FF 2.0 beta freeze isn't far off.

Comment 8

[Peter Van der Beken [:peterv]]

(In reply to comment #4)
> Since the XPath needed an interface and accessibility needs an interface, it is
> conceivable that in the future yet another core component will need the ability
> to interact with XForms if it is available.

Note that XPath will not need it anymore after my patch for bug 278981 lands.

Comment 9

[alexander :surkov (:asurkov)]

(In reply to comment #7)
> 
> Alex, I'd suggest that the interface be non-scriptable and perhaps keep it in a
> header and not as an interface at all.  It is only meant to allow other Mozilla
> components a chance to interact with XForms.  I don't know that we want to
> support JS, Java, etc. interacting with XForms that way.  Also keep in mind
> that FF 2.0 beta freeze isn't far off.
> 

Currently patch contains non-scriptable interface, i'm not against if interface will be declared in header file, I just thought if something is interface then it should be in idl file. I'm not big interface guy though. If you think the header is more right way then I'll follow you.

Comment 10

[alexander :surkov (:asurkov)]

(In reply to comment #5)
> Ah, i see what you're doing. I think i would prefer not to have a specific
> xforms directory though, so maybe content/base/public would be better. Don't
> feel strongly about it though.
> 

There is only one reason why I put that interface into content/xforms. It is I think we will need more than one such iterface sometime.

Comment 11

[aaronr]

(In reply to comment #9)
> (In reply to comment #7)
> > 
> > Alex, I'd suggest that the interface be non-scriptable and perhaps keep it in a
> > header and not as an interface at all.  It is only meant to allow other Mozilla
> > components a chance to interact with XForms.  I don't know that we want to
> > support JS, Java, etc. interacting with XForms that way.  Also keep in mind
> > that FF 2.0 beta freeze isn't far off.
> > 
> 
> Currently patch contains non-scriptable interface, i'm not against if interface
> will be declared in header file, I just thought if something is interface then
> it should be in idl file. I'm not big interface guy though. If you think the
> header is more right way then I'll follow you.
> 

Well, I guess it doesn't really matter if it is .idl or .h.  .idl is fine with me.

(In reply to comment #10)
> (In reply to comment #5)
> > Ah, i see what you're doing. I think i would prefer not to have a specific
> > xforms directory though, so maybe content/base/public would be better. Don't
> > feel strongly about it though.
> > 
> 
> There is only one reason why I put that interface into content/xforms. It is I
> think we will need more than one such iterface sometime.
> 

As far as I can tell, having a directory under content seems to be for cases where there are public interfaces AND core source.  Since we don't anticipate being able to get our code into the core anytime in the near future and if we only foresee a couple more public interfaces, then I'd suggest that we put them in content/base/public like sicking suggested.  Any other opinions from core contributors cc'd on this bug?

I'm fine with the other parts of the patch, so deciding on the location should be the last hurdle (since no one cc'd on the bug at least seems too opposed to us having a public interface).

Comment 12

[alexander :surkov (:asurkov)]

Attachment: patch2

interface is moved to content/base/public

Comment 13

[aaronr]

Comment on attachment 228323 [details] [diff] [review]
patch2


>Index: content/base/public/nsIXFormsUtilityService.idl
>===================================================================
>RCS file: content/base/public/nsIXFormsUtilityService.idl
>diff -N content/base/public/nsIXFormsUtilityService.idl
>--- /dev/null	1 Jan 1970 00:00:00 -0000
>+++ content/base/public/nsIXFormsUtilityService.idl	6 Jul 2006 18:22:48 -0000

>+  /**
>+   * Function to retrieve the number of days represented by the
>+   * xsd:dateTime provided in aValue
>+   */
>+  long getDaysFromDateTime(in DOMString aValue);
>+}; 
>\ No newline at end of file

Looks like you need a newline at the end of the file.

with that, r=me

Comment 14

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 228323 [details] [diff] [review]
patch2

This is still an idl file though, is there a reason for that?

Comment 15

[alexander :surkov (:asurkov)]

(In reply to comment #14)
> (From update of attachment 228323 [details] [diff] [review] [edit])
> This is still an idl file though, is there a reason for that?
> 

I see the one reason. nsIXFormsUtilityService implementation in xforms depends on macros that are added by xpidl compiler. Previously nsIXFormsUtilityService was defined as header file and that macros was defined manually. I think it's not very nice. Can you explain when I should use header and when I should use idl?

Comment 16

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

In general we only use idl files for things that needs to be used by script since there is a minimal overhead in using idl files over .h files. Idl files are compiled into .xpt which are a binary representation of the idl and shipped with the product.

I do see your argument, but in this case I think i'd prefer to just live with the extra macros. Or simply put the function-declaraions in the class itself since there's only one implementation of this interface.

Comment 17

[alexander :surkov (:asurkov)]

(In reply to comment #16)
> In general we only use idl files for things that needs to be used by script
> since there is a minimal overhead in using idl files over .h files. Idl files
> are compiled into .xpt which are a binary representation of the idl and shipped
> with the product.
> 
> I do see your argument, but in this case I think i'd prefer to just live with
> the extra macros. Or simply put the function-declaraions in the class itself
> since there's only one implementation of this interface.
> 

Ok, I'll file new patch today later. Can I ask you for sr?

Comment 18

[alexander :surkov (:asurkov)]

Attachment: patch3

Comment 19

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 228436 [details] [diff] [review]
patch3

>+  /* nsIDOMNode getModelFromNode (in nsIDOMNode node); */
>+  NS_IMETHOD GetModelFromNode(nsIDOMNode *node, nsIDOMNode **aModel) = 0;

I don't think you really need the comment above the declaration here. Not a biggie if you really want to keep it though.

sr=sicking

Comment 20

[aaronr]

checked into trunk for surkov.  This change shouldn't go into the branches since it involves a core interface change.

Comment 21

[Peter Van der Beken [:peterv]]

I'm not convinced that this was a smart thing to do. What existing methods of nsIXFormsUtilityService will be used for accessibility? My patch for bug 278981 removes the whole implementation of that interface, so should I just remove all the methods from it too?

Comment 22

[aaronr]

(In reply to comment #21)
> I'm not convinced that this was a smart thing to do. What existing methods of
> nsIXFormsUtilityService will be used for accessibility? My patch for bug 278981
> removes the whole implementation of that interface, so should I just remove all
> the methods from it too?
> 

The first step was moving the interface and making sure we didn't break anything by doing that.  Accessibility will add changes to the interface via bug 337690.

I'd say that when xpath no longer needs the interface, then please make a note in bug 337690 or open another bug on us (XForms) to remove unnecessary interface methods once we know for sure what we need and don't need for accessibility.  For example, I seriously doubt that we'll need most of the second/month/time type of methods for accessiblity.

Comment 23

[Aaron Leventhal]

Depends what those time methods do. Ultimately we're going to be exposing types to alternative input aids like voice reco and onscreen keyboards. This will allow special accessible date pickers to set the value from outside mozilla.