Use suite/branding as default --with-branding

RESOLVED FIXED

Status

SeaMonkey
Build Config
RESOLVED FIXED
8 years ago
8 years ago

People

(Reporter: glandium, Assigned: Robert Kaiser)

Tracking

(Blocks: 1 bug)

Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 4 obsolete attachments)

(Reporter)

Description

8 years ago
Created attachment 409683 [details] [diff] [review]
Patch against seamonkey 2.0

Currently, when using --with-branding=somedir, the content from suite/branding still gets used and may clash with whatever is done in the externally given branding directory.

Not going into suite/branding directly, but using suite/branding as a branding directory makes that work better (though it still leaves some complains during mozilla/configure)
Attachment #409683 - Flags: review?
Attachment #409683 - Flags: review? → review?(neil)

Updated

8 years ago
Attachment #409683 - Flags: review?(neil) → review?(kairo)
(Reporter)

Comment 1

8 years ago
Created attachment 409922 [details] [diff] [review]
Patch against seamonkey 2.0, v2

Same patch with some additional changes needed to have the Makefile in the given branding directory generated properly.
Attachment #409683 - Attachment is obsolete: true
Attachment #409922 - Flags: review?(kairo)
Attachment #409683 - Flags: review?(kairo)
Comment on attachment 409922 [details] [diff] [review]
Patch against seamonkey 2.0, v2

>diff --git a/allmakefiles.sh b/allmakefiles.sh

>+if test -n "$MOZ_BRANDING_DIRECTORY"; then
>+  add_makefiles "
>+    $MOZ_BRANDING_DIRECTORY/Makefile
>+  "
>+fi

This is incorrect. It assumes that Thunderbird and Sunbird need this precise set up, and they don't - they need something different in both cases. Please put this in suite/makefiles.sh.

>diff --git a/suite/confvars.sh b/suite/confvars.sh

>+MOZ_BRANDING_DIRECTORY=suite/branding

Would it be better if this was suite/branding/nightly (which would mean the default defined in configure.in could be used), or maybe suite/branding/official? This would then allow other setups in exactly the same way as Firefox and Thunderbird do (though I'm not sure SeaMonkey needs them)

I realise that means moving the directories, but hg makes that easy.
(Assignee)

Updated

8 years ago
Attachment #409922 - Flags: review?(kairo) → review-
(Assignee)

Comment 3

8 years ago
Comment on attachment 409922 [details] [diff] [review]
Patch against seamonkey 2.0, v2

r- for the comments Mark made (who actually knows all the branding stuff better than me as he figured it all out for Thunderbird).
We want to match Firefox and Thunderbird where possible and useful, as that makes life easier for everyone. What we want to avoid right now is creating any different branding for nightlies or unofficial builds in our tree, though.
(Assignee)

Comment 4

8 years ago
Created attachment 425464 [details] [diff] [review]
new patch to model things after what Firefox/Thunderbird have

This new patch models the SeaMonkey branding code after what Firefox and Thunderbird have. The one area I've left out (and which probably deserves a followup, but not sure of the exact solution) is locales.
And it puts the only branding we have in nightly/ as suggested in this bug earlier.
Assignee: nobody → kairo
Attachment #409922 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #425464 - Flags: review?(bugzilla)
(Reporter)

Comment 5

8 years ago
Actually, the venkman icons should simply not been part of the seamonkey branding, but part of mozilla/extensions/venkman, a bit like mozilla/extensions/irc which contains the chatzilla icons (although the branding also has them).
(Reporter)

Comment 6

8 years ago
Sorry this was intended for bug 546456
Comment on attachment 425464 [details] [diff] [review]
new patch to model things after what Firefox/Thunderbird have

I've not tested this, but the general idea seems fine.

> ifdef MOZ_BRANDING_DIRECTORY
> tier_app_dirs += $(MOZ_BRANDING_DIRECTORY)
>+else
>+tier_app_dirs += suite/branding/nightly
> endif

I think there's another bug around somewhere that is changing this. If I see it I'll try and review it today.

>+SUBMAKEFILES += \
>+	$(DEPTH)/$(MOZ_BRANDING_DIRECTORY)/Makefile \
>+#	$(DEPTH)/$(MOZ_BRANDING_DIRECTORY)/locales/Makefile \
>+	$(NULL)

That's not going to work quite how you expect, make will see this line as:

SUBMAKEFILES += $(DEPTH)/$(MOZ_BRANDING_DIRECTORY)/Makefile # $(DEPTH)/$(MOZ_BRANDING_DIRECTORY)/locales/Makefile $(NULL)

Which obviously isn't what you want.

>+	@$(MAKE) -C $(DEPTH)/$(MOZ_BRANDING_DIRECTORY)/locales AB_CD=$* XPI_NAME=locale-$* BOTH_MANIFESTS=1

Won't this fail because you've not got a locales directory?

>+  add_makefiles "
>+    $MOZ_BRANDING_DIRECTORY/Makefile
>+#    $MOZ_BRANDING_DIRECTORY/locales/Makefile
>+  "

Again, this doesn't look quite right unless add_makefiles supports it.
Attachment #425464 - Flags: review?(bugzilla) → review-
(Assignee)

Updated

8 years ago
Blocks: 551485
(Assignee)

Comment 8

8 years ago
Created attachment 431648 [details] [diff] [review]
v1.1: leave out locales for now

I hoped that the comments would be parsed before the file being interpreted, but it may just have worked magically (after commenting out that other call as well) because those entries were always the last ones in their respective area.

I now moved the whole locales topic off to bug 551485 and this new patch just leaves them out, which should fix all your comments.
Attachment #425464 - Attachment is obsolete: true
Attachment #431648 - Flags: review?(bugzilla)
Depends on: 514519
Version: SeaMonkey 2.0 Branch → Trunk
(In reply to comment #2)
> This is incorrect. It assumes that Thunderbird and Sunbird need this precise
> set up, and they don't - they need something different in both cases.

(Maybe now they do?)

(In reply to comment #7)
> I think there's another bug around somewhere that is changing this.

Bug 514519.
(Assignee)

Comment 10

8 years ago
(In reply to comment #9)
> (In reply to comment #7)
> > I think there's another bug around somewhere that is changing this.
> 
> Bug 514519.

Grr, so I need to rework this patch once again just because it took so long to review it? :(
(Assignee)

Comment 11

8 years ago
Created attachment 435602 [details] [diff] [review]
Also update for bug 514519

OK, so here's yet another patch that is updated for bug 514519 changes as well.

I added the var for official branding even if it points to the same dir, but that makes sure that people can use --enable-official-branding just like for Thunderbird and Firefox, even if the builds don't differ from nightlies (for now).
Attachment #431648 - Attachment is obsolete: true
Attachment #435602 - Flags: review?(bugzilla)
Attachment #431648 - Flags: review?(bugzilla)
Comment on attachment 435602 [details] [diff] [review]
Also update for bug 514519

I've done a quick build on Mac with this, and all seems fine.

>+add_makefiles "
>+$MOZ_BRANDING_DIRECTORY/Makefile
>+"

nit: I don't really see any point in making this a separate section to the main add_makefiles.
Attachment #435602 - Flags: review?(bugzilla) → review+
(Assignee)

Comment 13

8 years ago
Thanks, pushed as http://hg.mozilla.org/comm-central/rev/1331ca255dd4
Status: ASSIGNED → RESOLVED
Last Resolved: 8 years ago
Resolution: --- → FIXED
(Assignee)

Comment 14

8 years ago
Needed to fix up Windows bustage due to forgotten file moves, see http://hg.mozilla.org/comm-central/rev/29fcb3e1e2c6
You need to log in before you can comment on or make changes to this bug.