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

RESOLVED FIXED

Status

SeaMonkey
Build Config
RESOLVED FIXED
11 years ago
11 years ago

People

(Reporter: standard8, Assigned: standard8)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(4 attachments, 1 obsolete attachment)

(Assignee)

Description

11 years ago
Created attachment 273243 [details] [diff] [review]
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.
(Assignee)

Comment 1

11 years ago
Created attachment 273266 [details] [diff] [review]
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.
Attachment #273266 - Flags: superreview?(neil)
Attachment #273266 - Flags: review?(kairo)
(Assignee)

Updated

11 years ago
Attachment #273243 - Flags: superreview?(neil)
Attachment #273243 - Flags: review?(kairo)

Comment 2

11 years ago
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...)
Attachment #273266 - Flags: superreview?(neil) → superreview+

Updated

11 years ago
Attachment #273243 - Flags: superreview?(neil) → superreview+

Comment 3

11 years ago
Comment on attachment 273243 [details] [diff] [review]
cvs copies

r=me on the locations of the move (also backed up by bsmedberg over IRC)
Attachment #273243 - Flags: review?(kairo) → review+
(Assignee)

Updated

11 years ago
Depends on: 389432

Comment 4

11 years ago
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.
Attachment #273266 - Flags: review?(kairo) → review+
(Assignee)

Comment 5

11 years ago
Created attachment 275025 [details] [diff] [review]
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.
Attachment #275025 - Flags: review?(kairo)

Comment 6

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

yes, this looks good, r=me
Attachment #275025 - Flags: review?(kairo) → review+
(Assignee)

Comment 7

11 years ago
Additional patch checked in -> fixed.
Status: NEW → RESOLVED
Last Resolved: 11 years ago
Resolution: --- → FIXED
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
(Assignee)

Comment 9

11 years ago
Created attachment 275132 [details] [diff] [review]
Fixes the regression

Fixes the regression - add suitebrowser.xpt to the packaging lists.
Attachment #275132 - Flags: review?(bugzilla)
Comment on attachment 275132 [details] [diff] [review]
Fixes the regression

r+ if you add components/urlwidgt.xpt to suite/installer/removed-files.in
Attachment #275132 - Flags: review?(bugzilla) → review+
(Assignee)

Comment 11

11 years ago
Created attachment 275144 [details] [diff] [review]
Fixes the regression v2

This should be better - requesting review again just to check
Attachment #275132 - Attachment is obsolete: true
Attachment #275144 - Flags: review?(bugzilla)

Updated

11 years ago
Attachment #275144 - Flags: review?(bugzilla) → review+
(Assignee)

Comment 12

11 years ago
I've just checked the regression fix in. Thanks for letting us know about it Serge.
[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 :-)
You need to log in before you can comment on or make changes to this bug.