201078 - Create a separate module for web services

Status: RESOLVED FIXED

(Core Graveyard :: Web Services, defect, P1)

Description

[harishd]

Move proxy/interfaceinfo/schema/soap/wsdl from xmlextras module into webservice
module.

Comment 1

[harishd]

Attachment: Patch v1.0_Part1

Patch contains the following:
1) Updated allmakefiles.sh, configure.in, configure, and packaging files to
include the new module - webservices.

Note: The patch also contains changes to MozillaBuild*.pm but am not sure if
it's necessary. Also, I have cvs removed ( not commited ) webservices related
information from xmlextras module ( files moved into webservices module ) but
was not able to post the patch because, apparently, bugzilla does not like
patches more that 500KB.

Comment 2

[harishd]

Attachment: Part2 ( webservices module - tar ball gzipped )

Comment 3

[harishd]

Sean: Could you please r= xpinstall changes? Thanks

Comment 4

[Sean Su]

Comment on attachment 120456 [details] [diff] [review]
Patch v1.0_Part1

in the following packages files:

  mozilla\xpinstall\packager\packages-mac
  mozilla\xpinstall\packager\packages-os2

you forgot to remove 'bin/components/xmlextras_interfaceinfo.xpt'

in the following packages files:

  mozilla\embedding\config\basebrowser-installer-win.pkg
mozilla\embedding\config\gre-installer-win.pkg

you forgot to remove the following files:

  components\xmlextras.xpt
  components\xmlextras_interfaceinfo.xpt

There are also package file lists in the ns tree in ns\xpinstall\packager. 
Please make sure that the same files are removed from there as well.

Why are you adding the following to xpinstall\packager\windows\browser.jst:

  +    deleteThisFile("Components", "websrvcs.dll");
  +    deleteThisFile("Components", "websrvcs.xpt");

You should be adding the files that you are removing (if they're not already
in), like:
  bin/components/xmlextras_interfaceinfo.xpt
  bin/components/xmlsoap.xpt
  bin/components/xmlschema.xpt
  bin/components/wsdl.xpt
  bin/components/wsproxy.xpt

Of the list above, make sure that the following are within the 'if(!gGreLocal)'
statement in browser.jst:
  xmlextras.xpt
  xmlextras_interfaceinfo.xpt

Don't forget about the browser.jst in the ns tree too.

Comment 5

[harishd]

>You should be adding the files that you are removing (if they're not already
>in), like:
>  bin/components/xmlextras_interfaceinfo.xpt
>  bin/components/xmlsoap.xpt
>  bin/components/xmlschema.xpt
>  bin/components/wsdl.xpt
>  bin/components/wsproxy.xpt

These xpts will be gone for good. Should I still add them to deleteThisFile list?

Comment 6

[Sean Su]

For the files that are 'gone for good', please add them to the (ns/mozilla)
browser.jst's deleteThisFile() list, but _outside_ of the 'if(!gGreLocal)'
statement.

For the files that have been 'moved to GRE package (from the browser package)',
please add them to the (ns/mozilla) browser.jst's deleteThisFile() list, but
_inside_ the 'if(!gGreLocal)' statement.

Comment 7

[Peter Van der Beken [:peterv]]

Doesn't xmlextras make more sense for the schema stuff?

Comment 8

[harishd]

> Doesn't xmlextras make more sense for the schema stuff?

Yes and no :-). The only consumer of schema ( and interfaceinfo as well ) is
webservices. And therefore it's ok for schema/interfaceinfo to live under
webservices...for now. In the future, if need be, we can consider a more apt module.

Comment 9

[harishd]

Attachment: Patch v1.0.1

Addresses Sean's concerns.

Comment 10

[Heikki Toivonen (remove -bugzilla when emailing directly)]

Comment on attachment 120456 [details] [diff] [review]
Patch v1.0_Part1

Let's see a new patch.

Comment 11

[Heikki Toivonen (remove -bugzilla when emailing directly)]

Comment on attachment 120457 [details]
Part2 ( webservices module - tar ball gzipped )

There were some empty directories here, like docs and wsdl/tests. If this is
all just moving stuff, why not write a shell script that copies all the files
to the new location, and then deletes from the old location if a flag is set.
That way we could easily test that all the files are included properly, and
leaf could use that script on the CVS server to copy the files (and NOT delete
the originals, which is why the separate deletion step with flag;)

Comment 12

[Heikki Toivonen (remove -bugzilla when emailing directly)]

Comment on attachment 120457 [details]
Part2 ( webservices module - tar ball gzipped )

And do cvs -uN for the new files (makefiles, anything else?)

Comment 13

[harishd]

>There were some empty directories here, like docs and wsdl/tests

I don't know why these dirs are empty...because they are not empty for me. And I
have already talked to leaf about the files to be copied to webservices dir.

Comment 14

[harishd]

Attachment: Patch v1.1_Part1

changes to allmakefiles.sh, configure.in, configure, mac build scripts

Comment 15

[harishd]

Attachment: Patch v1.1_Part2

Added and changed files to the new webservices module.

Comment 16

[harishd]

Attachment: Patch v1.1_Part3

Remove webservices related info. from xmlextras module. Unfortunately, I
coulnd't attach a patch that contained the files removed ( because bugzilla
does not allow patch sizes greater than 500KB )

Comment 17

[harishd]

Attachment: Patch v1.1_Part4

Packaging changes

Comment 18

[Heikki Toivonen (remove -bugzilla when emailing directly)]

Comment on attachment 121342 [details] [diff] [review]
Patch v1.1_Part1

>Index: allmakefiles.sh
>===================================================================
>Index: configure.in
>===================================================================
>Index: configure
>===================================================================

Maybe you don't need to checkin configure changes; there is supposedly a script
that does it. But check with leaf.

>Index: build/mac/build_scripts/MozillaBuildFlags.txt
>===================================================================
>+webservices	1

Yucky tab, use spaces.

>Index: build/mac/build_scripts/MozillaBuildList.pm
>===================================================================

Yucky tabs, use spaces.

Comment 19

[Sean Su]

Comment on attachment 121345 [details] [diff] [review]
Patch v1.1_Part4

>Index: xpinstall/packager/windows/browser.jst
>===================================================================
>RCS file: /cvsroot/mozilla/xpinstall/packager/windows/browser.jst,v
>retrieving revision 1.88
>diff -u -u -r1.88 browser.jst
>--- xpinstall/packager/windows/browser.jst	8 Apr 2003 05:11:54 -0000	1.88
>+++ xpinstall/packager/windows/browser.jst	22 Apr 2003 22:04:11 -0000
>@@ -437,6 +437,10 @@
>   deleteThisFile("Chrome",     "chrome.rdf");
>   deleteThisFile("Chrome",     "forms.jar");
>   deleteThisFile("Chrome",     "typeaheadfind.jar");
>+  deleteThisFile("Chrome",     "xmlsoap.xpt");
>+  deleteThisFile("Chrome",     "xmlschema.xpt");
>+  deleteThisFile("Chrome",     "wsdl.xpt");
>+  deleteThisFile("Chrome",     "wsproxy.xpt");
>   deleteThisFolder("Chrome",   "overlayinfo");
>   deleteThisFolder("Components",   "Netscape/Gecko1.0");
>   deleteThisFolder("Components",   "Netscape/Netscape6.00");

Aren't those files to be delted from the Components dir instead of the Chrome
dir?

>@@ -574,6 +578,7 @@
>     deleteThisFile("Components", "webbrwsr.dll");
>     deleteThisFile("Components", "xmlextras.dll");
>     deleteThisFile("Components", "xmlextras.xpt");
>+    deleteThisFile("Chrome",     "xmlextras_interfaceinfo.xpt");
>     deleteThisFile("Components", "xpc3250.dll");
>     deleteThisFile("Components", "xpinstal.dll");
>     deleteThisFile("Components", "xppref32.dll");

move the delete of 'xmlextras_interfaceinfo.xpt' next to the above deletes
because you had mentioned that this file is gone for good, so it shouldn't be
inside the 'if(!gGreLocal)' statement.	Also, shouldn't the folder should be
Components instead of Chrome?

Comment 20

[Heikki Toivonen (remove -bugzilla when emailing directly)]

Comment on attachment 121345 [details] [diff] [review]
Patch v1.1_Part4

>Index: xpinstall/packager/windows/browser.jst
>===================================================================

What about mac, unix, os2 and XPI_JSTs versions of browser.jst?

Comment 21

[Sean Su]

> What about mac, unix, os2 and XPI_JSTs versions of browser.jst?

Mac and Unix don't have the upgradeCleanup() function.

However OS2 does, but it doesn't have the 'if(!gGreLocal)' statement.  The
deleteThisFile() calls should just go in the main part of
os2\browser.jst::upgradeCleanup().

Harishd should talk to [e-mail] to see if we should do these OS2
updates for him or not.

We can ignore everything in the XPI_JSTs because we're not doing anything with
them right now.  They were a first attempt at trying to generalize the installer
build process.  It didn't get anywhere :(

Comment 22

[Heikki Toivonen (remove -bugzilla when emailing directly)]

Comment on attachment 121344 [details] [diff] [review]
Patch v1.1_Part3

I checked in changes in these files so just make sure you resolve any conflicts
properly :)

Comment 23

[Heikki Toivonen (remove -bugzilla when emailing directly)]

Comment on attachment 121343 [details] [diff] [review]
Patch v1.1_Part2

>Index: extensions/webservices/Makefile.in
>===================================================================
>Index: extensions/webservices/build/Makefile.in
>===================================================================
>Index: extensions/webservices/build/src/Makefile.in
>===================================================================
>+srcdir		= @srcdir@

tabs

>+MODULE		= websrvcs

tabs

>+LIBRARY_NAME	= websrvcs
>+EXPORT_LIBRARY = 1
>+ifneq ($(OS_ARCH),WINNT)
>+SHORT_LIBNAME	= websrvcs
>+endif

I don't think you need the short_libname stuff because the name is 8 chars
long.

>+MODULE_NAME	= nsWebServiceModule

Can't this be websrvces?

>+GRE_MODULE	= 1

I think this is not a GRE module.

>+ifdef MOZ_WSP
>+REQUIRES += iiextras
>+endif

Do we still use this?

>Index: extensions/webservices/build/src/nsWebServiceModule.cpp
>===================================================================
>+ * Version: NPL 1.1/GPL 2.0/LGPL 2.1

Should change license to MPL.

Will continue...

Comment 24

[Heikki Toivonen (remove -bugzilla when emailing directly)]

Comment on attachment 121343 [details] [diff] [review]
Patch v1.1_Part2


>Index: extensions/webservices/build/src/Makefile.in
>===================================================================
>+MODULE_NAME	= nsWebServiceModule

Can't this be websrvcs? (there was typo in my previous comment)

>Index: extensions/webservices/build/src/nsWebServiceModule.cpp
>===================================================================
>+#define NS_WEB_SRVC_CONTRACTID "@mozilla.org/extension/webservice;1"

Nit, SRVCS (notice last s;)

Comment 25

[harishd]

Heikki: I've made all those changes...will post a new patch after you've gone
thro' the attached patch ( Patch v1.1_Part2 ) completely.

Comment 26

[Mike Kaply [:mkaply]]

Our packaged build never delivers separate XPT files, so we would never have a
need to delete them.

Comment 27

[Johnny Stenback (:jst)]

Comment on attachment 121343 [details] [diff] [review]
Patch v1.1_Part2

- In nsWebServiceModule.cpp:

+#define NS_WEB_SRVC_CONTRACTID "@mozilla.org/extension/webservice;1"
+
+class nsWebService : public nsISupports 
+{
+public:
+  nsWebService();
+  virtual ~nsWebService();
+
+  NS_DEFINE_STATIC_CID_ACCESSOR(NS_WEB_SRVC_CID);
+
+  // nsISupports
+  NS_DECL_ISUPPORTS
+};
+
...
+NS_GENERIC_FACTORY_CONSTRUCTOR(nsWebService)
...
+static const nsModuleComponentInfo gComponents[] = {
+  { "WebService Component", NS_WEB_SRVC_CID, NS_WEB_SRVC_CONTRACTID,
+    nsWebServiceConstructor, RegisterWebService },

The class nsWebService isn't used for anything, the only reason it's there is
to have a hook for the RegisterWebService() function. I'd say remove the dummy
class and move the RegisterWebService function reference into the registration
of another class (any class where you pass null for the registration callback.

Other than that, sr=jst

Comment 28

[Johnny Stenback (:jst)]

Comment on attachment 121344 [details] [diff] [review]
Patch v1.1_Part3

sr=jst

Comment 29

[Heikki Toivonen (remove -bugzilla when emailing directly)]

Comment on attachment 121343 [details] [diff] [review]
Patch v1.1_Part2

>Index: extensions/webservices/build/src/nsWebServiceModule.cpp
>===================================================================
>Index: extensions/webservices/interfaceinfo/Makefile.in
>===================================================================
>Index: extensions/webservices/interfaceinfo/src/Makefile.in
>===================================================================
>Index: extensions/webservices/proxy/Makefile.in
>===================================================================
>Index: extensions/webservices/proxy/src/Makefile.in
>===================================================================
>Index: extensions/webservices/proxy/tests/Makefile.in
>===================================================================
>Index: extensions/webservices/public/Makefile.in
>===================================================================
>Index: extensions/webservices/schema/Makefile.in
>===================================================================
>Index: extensions/webservices/schema/src/Makefile.in
>===================================================================
>Index: extensions/webservices/security/Makefile.in
>===================================================================
>Index: extensions/webservices/security/src/Makefile.in
>===================================================================
>Index: extensions/webservices/soap/Makefile.in
>===================================================================
>Index: extensions/webservices/soap/src/Makefile.in
>===================================================================
>Index: extensions/webservices/wsdl/Makefile.in
>===================================================================
>Index: extensions/webservices/wsdl/src/Makefile.in
>===================================================================

Bad tabs in a lot of the makefiles, fix those and r=heikki.

Comment 30

[Sean Su]

Comment on attachment 121345 [details] [diff] [review]
Patch v1.1_Part4

review- until my questions are answered in comment #19.

Comment 31

[harishd]

>move the delete of 'xmlextras_interfaceinfo.xpt' next to the above deletes
>because you had mentioned that this file is gone for good, so it shouldn't be
>inside the 'if(!gGreLocal)' statement.	Also, shouldn't the folder should be
>Components instead of Chrome?

Sean, I was just following what you said in comment #4. However, I should have
followed comment #6. Will post another patch with your suggested changes. :( 

Comment 32

[harishd]

Attachment: Patch v1.1_Part4.1

Incorporated Sean's suggestions.

Comment 33

[harishd]

Comment on attachment 121496 [details] [diff] [review]
Patch v1.1_Part4.1

Carrying forward heikki's sr=

Comment 34

[Sean Su]

Comment on attachment 121496 [details] [diff] [review]
Patch v1.1_Part4.1

looks great!
r=ssu

Comment 35

[Daniel (Leaf) Nunes]

Comment on attachment 121342 [details] [diff] [review]
Patch v1.1_Part1

r=leaf, looks good.

Comment 36

[Asa Dotzler [:asa]]

Comment on attachment 121496 [details] [diff] [review]
Patch v1.1_Part4.1

a=asa (on behalf of drivers) for checkin to 1.4b.

Comment 37

[Asa Dotzler [:asa]]

patch approved to land. 1.4b- because we wouldn't block for this but don't let
that stop you from landing (the sooner the better).

Comment 38

[harishd]

Fix landed.