389085 - Move xpfe/components/urlwidget to suite/browser...

Status: RESOLVED FIXED

(SeaMonkey :: Build Config, defect)

Description

[Mark Banner (:standard8)]

Attachment: cvs copies

urlwidget is currently only built for SeaMonkey on windows. We should move it from xpfe/components to under suite/browser (its specific to the browser part of SeaMonkey I believe.

I'm proposing we put it under suite/browser/public and suite/browser/src. When the xpfe/components/bookmarks source code is moved to suite/ that can join urlwidget under the same folders.

Comment 1

[Mark Banner (:standard8)]

Attachment: The fix

This patch will do the work. Note that I've included the add/remove of the cvs copied files in this patch as its easier to apply for test purposes. The only one of the cvs copies that really changes is the Makefile.in that goes into suite/browser/src.

Comment 2

[[e-mail]]

Comment on attachment 273266 [details] [diff] [review]
The fix

>Index: suite/confvars.sh
>===================================================================
>RCS file: /cvsroot/mozilla/suite/confvars.sh,v
>retrieving revision 1.3
>diff -u -p -r1.3 confvars.sh
>--- suite/confvars.sh	4 Jun 2007 15:09:50 -0000	1.3
>+++ suite/confvars.sh	21 Jul 2007 21:07:54 -0000
>@@ -39,6 +39,7 @@
> MOZ_APP_NAME=seamonkey
> MOZ_APP_DISPLAYNAME=SeaMonkey
> MOZ_MAIL_NEWS=1
>+MOZ_STATIC_MAIL_BUILD=1
> MOZ_LDAP_XPCOM=1
> MOZ_STATIC_MAIL_BUILD=1
Oops ;-) (It turned out I actually have this change in my tree, so I guess someone moved the line between their review request and check in...)

Comment 3

[Robert Kaiser]

Comment on attachment 273243 [details] [diff] [review]
cvs copies

r=me on the locations of the move (also backed up by bsmedberg over IRC)

Comment 4

[Robert Kaiser]

Comment on attachment 273266 [details] [diff] [review]
The fix

This patch is hard to review, it usually is better to give a patch that shows the actual _changes_ that you're doing, without the noise of the move.

>Index: suite/confvars.sh
>===================================================================
>RCS file: /cvsroot/mozilla/suite/confvars.sh,v
>retrieving revision 1.3
>diff -u -p -r1.3 confvars.sh
>--- suite/confvars.sh	4 Jun 2007 15:09:50 -0000	1.3
>+++ suite/confvars.sh	21 Jul 2007 21:07:54 -0000
>@@ -39,6 +39,7 @@
> MOZ_APP_NAME=seamonkey
> MOZ_APP_DISPLAYNAME=SeaMonkey
> MOZ_MAIL_NEWS=1
>+MOZ_STATIC_MAIL_BUILD=1
> MOZ_LDAP_XPCOM=1
> MOZ_STATIC_MAIL_BUILD=1
> MOZ_COMPOSER=1

Please remove this part, we don't need to double the var.

>Index: suite/browser/Makefile.in
>===================================================================
>RCS file: /cvsroot/mozilla/suite/browser/Makefile.in,v
>retrieving revision 1.6
>diff -u -p -r1.6 Makefile.in
>--- suite/browser/Makefile.in	3 Jun 2007 11:44:30 -0000	1.6
>+++ suite/browser/Makefile.in	21 Jul 2007 21:07:54 -0000
>@@ -42,6 +42,11 @@ VPATH		= @srcdir@
> 
> include $(DEPTH)/config/autoconf.mk
> 
>+# For now, only build public and src on windows
>+ifeq ($(OS_ARCH),WINNT)
>+DIRS = public src
>+endif
>+
> EXTRA_COMPONENTS = nsBrowserContentHandler.js \
> 	nsAboutAbout.js \
> 	$(NULL)

As we expect stuff to land there that is built on all platforms, I'd prefer to at least additionally add the logic in the Makefiles of those dirs that makes the urlwidget parts be built on Windows only.
If it doesn't make a change to the other platforms, it might even be good to make then descend into those dirs, even if it results in building nothing there.

But I think it's better to do a separate patch with that, with a separate sr from Neil - either in a followup bug or an additional patch in this bug, as you like ;-)

r=me with leaving confvars.sh unchanged and given you do the followup. And I hope Neil has tested that it actually works - I can only confirm that it doesn't break Linux, I can't build on other platforms.

Comment 5

[Mark Banner (:standard8)]

Attachment: Make all platforms enter suite/browser/{public,src}

I checked the previous patch in earlier today.

This patch fixes the build so that all platforms enter suite/browser/{public,src} as Robert requested.

Comment 6

[Robert Kaiser]

Comment on attachment 275025 [details] [diff] [review]
Make all platforms enter suite/browser/{public,src}

yes, this looks good, r=me

Comment 7

[Mark Banner (:standard8)]

Additional patch checked in -> fixed.

Comment 8

[Serge Gautherie (:sgautherie)]

A regression between
[Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.9a7pre) Gecko/200708020202 SeaMonkey/2.0a1pre] (nightly) (W2Ksp4)
and
[Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.9a7pre) Gecko/200708030202 SeaMonkey/2.0a1pre] (nightly) (W2Ksp4)

Every time, when starting up the application:
{{
Error: uncaught exception: [Exception... "Component returned failure code: 0x80570018 (NS_ERROR_XPC_BAD_IID) [nsIJSCID.getService]"  nsresult: "0x80570018 (NS_ERROR_XPC_BAD_IID)"  location: "JS frame :: chrome://navigator/content/navigator.js :: <TOP_LEVEL> :: line 1874"  data: no]
}}

Code is
{{
1870 jaggernaut        1.355 const NS_URLWIDGET_CONTRACTID = "@mozilla.org/urlwidget;1";
1871 blakeross         1.314 var urlWidgetService = null;
1872 jaggernaut        1.355 if (NS_URLWIDGET_CONTRACTID in Components.classes) {
1873                           urlWidgetService = Components.classes[NS_URLWIDGET_CONTRACTID]
1874 blakeross         1.314                                .getService(Components.interfaces.nsIUrlWidget);
1875                         }
}}

There are 2 related checkins:
*2007-08-02 10:18	bugzilla%standard8.demon.co.uk 	
*2007-08-02 12:28	bugzilla%standard8.demon.co.uk

Comment 9

[Mark Banner (:standard8)]

Attachment: Fixes the regression

Fixes the regression - add suitebrowser.xpt to the packaging lists.

Comment 10

[Frank Wein [:mcsmurf]]

Comment on attachment 275132 [details] [diff] [review]
Fixes the regression

r+ if you add components/urlwidgt.xpt to suite/installer/removed-files.in

Comment 11

[Mark Banner (:standard8)]

Attachment: Fixes the regression v2

This should be better - requesting review again just to check

Comment 12

[Mark Banner (:standard8)]

I've just checked the regression fix in. Thanks for letting us know about it Serge.

Comment 13

[Serge Gautherie (:sgautherie)]

[Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.9a8pre) Gecko/200708040202 SeaMonkey/2.0a1pre] (nightly) (W2Ksp4)

Comment 8 regression is V.Fixed :-)