Closed Bug 324440 Opened 18 years ago Closed 17 years ago

make calendar code link with xpcom_glue instead of xpcom directly, convert to frozen linkage

Categories

(Calendar :: Internal Components, defect)

defect
Not set
normal

Tracking

(Not tracked)

VERIFIED FIXED

People

(Reporter: dmosedale, Assigned: dbo)

References

()

Details

Attachments

(2 obsolete files)

This will help us things like bug 321244 in the future.
"avoid things" that should have read.
I assume that in order for this to be effective, we would also need to make anything that lightning ships (webdav, storage, and xmlextras) link against the glue as well.  Is that likely to be practical?
xmlextras is part of gecko, unless something is vastly wrong (does tbird disable it!?)

storage will be also, though this doesn't help you for tbird 1.5... I suspect that making storage build with glue-only symbols might be possible on that branch (are there other consumers we need to worry about?)

I have no clue about webdav's needs and such.
Tbird might disable xmlextras, though I'm not sure exactly why they would.  storage uses nsIScriptable and some things related to that, but I'm not sure if/where that comes out of the glue's domain.  webdav requires crap like nsRect because of the event -> gfx entrainment issue, but that might not be a problem either.

I bet that wasn't helpful at all!
Tbird has xmlextras: http://lxr.mozilla.org/mozilla1.8.0/source/configure.in#4100

I bet (without being sure) that nsRect can be worked-around, but if webdavneeds nsIDocument/nsIContent I'm less optimistic (I had to hack those up significantly on trunk with nsStringGlue.h to get various pieces to compile properly).
Depends on: 325397
Depends on: 325398
Blocks: 343053
Blocks: 321244
Information about what to do can be found on devmo:
http://developer.mozilla.org/en/docs/Migrating_from_Internal_Linkage_to_Frozen_Linkage

Here are some useful queries from mxr that might help to get started:

- Use of truncate():
http://mxr.mozilla.org/seamonkey/search?string=truncate%28&find=%2Fcalendar%2F&findi=&filter=&tree=seamonkey

- Use of nsString.h:
http://mxr.mozilla.org/seamonkey/search?string=nsString.h&find=%2Fcalendar%2F&findi=&filter=&tree=seamonkey

- Use of NS_New* utility functions:
http://mxr.mozilla.org/seamonkey/search?string=NS_New&find=%2Fcalendar%2F&findi=&filter=&tree=seamonkey

- Use of nsCRT.h:
http://mxr.mozilla.org/seamonkey/search?string=nsCRT.h&find=%2Fcalendar%2F&findi=&filter=&tree=seamonkey
Component: Lightning Only → Internal Components
QA Contact: lightning → base
Summary: make lightning link with xpcom_glue instead of xpcom directly → make calendar code link with xpcom_glue instead of xpcom directly
Summary: make calendar code link with xpcom_glue instead of xpcom directly → make calendar code link with xpcom_glue instead of xpcom directly, convert to frozen linkage
Blocks: 293563
Trying to compile against frozen API, I ran into the problem that the nsAString::IsVoid et al mimic seems to have been dropped which is quite extensively used internally (i.e. in js, API returning a string returns null rather than throwing e.g. NS_ERROR_NOT_AVAILABLE). Of course, the calendar API seems to be underspecified/underdocumented with respect to this.
I propose to fix this in the 0.7 timeframe, because it affects API, which in general should be fixed earlier than later.
Flags: blocking-calendar0.7?
Referring to bug 380783 here.
I don't see resources nor volunteers; moreover even if we clean up the linkage we potentially break on every non-frozen API change that might occur on trunk. This makes (the rather theoretical goal) of shipping one lightning runnable on thunderbird 2 and 3 hardly feasible. => 0.7-
Flags: blocking-calendar0.7? → blocking-calendar0.7-
FWIW, Tbird3 is moving to frozen linkage so they can ship against XR, which would mean you'd have no choice but to use frozen linkage.
(In reply to comment #11)
When is that expected to come? Hopefully bug 380783 will be fixed then.
Bug 380783 has r/sr and is currently just awaiting approval1.9+ for checkin.
Depends on: 380783
Sum'ed up: When thunderbird switches, we have two options:
- keep our API as is, but let our code base diverge (branch vs trunk, no easy cross-commits anymore)
- change the API to get rid of IsVoid et al (comment #8)

So I think we should decide on this one when we know thunderbird's timeline: 0.7?
Flags: blocking-calendar0.7- → blocking-calendar0.7?
Given comments 2-6 (and since WebDAV needs nsIDocument at least), won't the WebDAV (and therefore CalDAV) code just fail to work in Thunderbird 3 as soon as it switches to frozen linkage?
nsIDocument is now usable from frozen-linkage code, though I recommend finding better ways to do what you need with it since it may go away later, and may not even stay stable even within security updates of the 1.9 branch (policy TBD there).
Since trunk supports nsA[C]String IsVoid mimic, there is no need to revise the API => 0.7-
Flags: blocking-calendar0.7? → blocking-calendar0.7-
- lightning trunk uses frozen linkage
- lightning branch still uses internal linkage (because of missing string IsVoid mimic)
- sunbird trunk still uses internal linkage (for now only, will require reworking installer packaging list)
- tested on windows (msvc 7.1) only with recent thunderbird trunk nightly

I'd appreciate some comments on the patch, e.g. from smedberg or dmose; I may have missed something or you have some useful hints/helpers. Further more I'd appreciate some broader testing on our supported platform baselines.

Thanks in advance!
Assignee: nobody → daniel.boelzle
Status: NEW → ASSIGNED
BTW: Is there a better alternative to get a flat C string than using nsStringAPI.h's PromiseFlatCString (i.e. nsCString) which constructs a deep copy of nsACString?
Looks reasonable to me.  lilmatt might have thoughts too; he's spent more time close to the build system stuff in calendar than I have.  I don't know of any other way to get a flat string than PromiseFlatCString.
Attachment #277250 - Flags: review?(mvl)
Comment on attachment 277250 [details] [diff] [review]
[checked in] first shot, lightning only -- use NS_ENSURE_SUCCESS before checking in

Just some comments from a quick look. Not yet a complete review.

>Index: mozilla/calendar/base/build/Makefile.in
>+ifdef MOZILLA_INTERNAL_API
> EXTRA_DSO_LDOPTS += \
> 	$(LIBS_DIR) \
> 	$(EXTRA_DSO_LIBS) \
> 	$(MOZ_COMPONENT_LIBS) \
> 	$(MOZ_JS_LIBS) \
> 	$(NULL)
>+else
>+EXTRA_DSO_LDOPTS += \
>+	$(LIBS_DIR) \
>+	$(EXTRA_DSO_LIBS) \
>+	$(NSPR_LIBS) \
>+	$(XPCOM_GLUE_LDOPTS) \
>+	$(MOZ_JS_LIBS) \
>+	$(NULL)
>+endif

Most of those are the same. You could split it into one common part (outside ifdef) and then one part where branch and trunk differ (inside ifdef)

>Index: mozilla/calendar/base/src/calICSService.cpp
>+    nsCOMPtr<nsIStringInputStream> const aStringStream(
>+        do_CreateInstance(NS_STRINGINPUTSTREAM_CONTRACTID, &rv));
>+    if (NS_SUCCEEDED(rv)) {

I personally like the NS_ENSURE_SUCCESS macro for cases like this. At least in debug builds they give more information when something fails.

>Index: mozilla/calendar/base/src/calUtils.cpp
>+namespace cal {
The portability guide http://www.mozilla.org/hacking/portable-cpp.html#dont_use_namespace says that you should not use namespaces. I'm not sure how outdated that document is, but namespaces are still not common style.

>+    explicit UTF8StringEnumerator(nsAutoPtr<nsCStringArray> & takeOverArray)
>+        : mArray(takeOverArray), mPos(0) {}

The array is an autoptr. Do you really rake over such an array? It's using refcounting, right?

>+nsresult createUTF8StringEnumerator(nsAutoPtr<nsCStringArray> & takeOverArray,
>+                                    nsIUTF8StringEnumerator ** ppRet)

'ppRet' doesn't mean anything to me. Can you use some more descriptive name?

>Index: mozilla/calendar/base/src/calUtils.h
>+ * The Original Code is Sun Microsystems code.

Is it really sun code, proted to mozilla? I'd say that it is mozilla calendar code.

>+ * The Initial Developer of the Original Code is
>+ * Sun Microsystems, Inc.

'cause you mention sun anyway here :)
(In reply to comment #21)
> >Index: mozilla/calendar/base/src/calUtils.cpp
> >+namespace cal {
> The portability guide
> http://www.mozilla.org/hacking/portable-cpp.html#dont_use_namespace says that
> you should not use namespaces. I'm not sure how outdated that document is, but
> namespaces are still not common style.
Right, I've read that once, too, but I doubt that there's any relevance left. I don't know of any current compiler that doesn't support or use namespaces (e.g. in standard headers). I can imagine that some compilers still don't support anonymous namespaces properly, but that's just a rough guess.

> >+    explicit UTF8StringEnumerator(nsAutoPtr<nsCStringArray> & takeOverArray)
> >+        : mArray(takeOverArray), mPos(0) {}
> 
> The array is an autoptr. Do you really rake over such an array? It's using
> refcounting, right?
No, copy-constructing or assigning auto_ptrs passes over ownership, meaning the UTF8StringEnumerator will be in charge of deleting it. That's why it's passed via non-const &.

> >+nsresult createUTF8StringEnumerator(nsAutoPtr<nsCStringArray> & takeOverArray,
> >+                                    nsIUTF8StringEnumerator ** ppRet)
> 
> 'ppRet' doesn't mean anything to me. Can you use some more descriptive name?
I could of course. I just like prefixes, reading the above like "pointer to pointer to returned interface).

> >Index: mozilla/calendar/base/src/calUtils.h
> >+ * The Original Code is Sun Microsystems code.
> 
> Is it really sun code, proted to mozilla? I'd say that it is mozilla calendar
> code.
> 
> >+ * The Initial Developer of the Original Code is
> >+ * Sun Microsystems, Inc.
> 
> 'cause you mention sun anyway here :)
I sticked to the same header scheme like in e.g. calIcsService.cpp.

thanks for reviewing!
>> The portability guide
>> http://www.mozilla.org/hacking/portable-cpp.html#dont_use_namespace says 
>> that you should not use namespaces. I'm not sure how outdated that document 
>> is, but namespaces are still not common style.
>
> Right, I've read that once, too, but I doubt that there's any relevance left. 
> I don't know of any current compiler that doesn't support or use namespaces 
> (e.g. in standard headers). I can imagine that some compilers still don't 
> support anonymous namespaces properly, but that's just a rough guess.

There are still some systems out there that use rather antiquated compilers, but I don't know whether working ports of our code exist for those systems at all (AIX, HP-UX, etc.)

It's probably best to ask in the mozilla.dev.platform newsgroup whether that document is still considered relevant. I've done that, see http://groups.google.com/group/mozilla.dev.platform/browse_frm/thread/25391c7df4b1f8bb
(In reply to comment #23)
> There are still some systems out there that use rather antiquated compilers,
> but I don't know whether working ports of our code exist for those systems at
> all (AIX, HP-UX, etc.)
> 
> It's probably best to ask in the mozilla.dev.platform newsgroup whether that
> document is still considered relevant. I've done that, see
> http://groups.google.com/group/mozilla.dev.platform/browse_frm/thread/25391c7df4b1f8bb

Thanks for asking on the list, Simon, but I don't expect a definitive answer. My experience from OpenOffice shows that almost nobody states a definite compiler requirement (baseline or features), because there ~may~ still be someone who compiles on older versions you don't want to shoot... .)
So I assume it's up to us and I see no obvious reason for not requiring a reasonable compiler with namespace support. We're not talking about template specialization or similar top-notch C++ features. Almost every system (including your stated ones) supports a reasonable recent gcc.

BTW: It would be great to have C++ compiler baselines; it could also pave the way for using fantastic libraries like boost or with respect to TR1.
From Benjamin Smedberg in mozilla.dev.platform:

|The breakpad code already uses C++ namespaces, though it is only enabled by 
|default on our major platforms. I believe we should remove this restriction 
|and allow C++ namespaces to be used.

So there's your definitive answer, Daniel.
(In reply to comment #25)
> |The breakpad code already uses C++ namespaces, though it is only enabled by 
> |default on our major platforms. I believe we should remove this restriction 
> |and allow C++ namespaces to be used.
> 
> So there's your definitive answer, Daniel.
Yes, thanks again. Although it's just a ~should~, bsmedberg's opinion counts a lot. That said, we drop that restriction for calendar code and use namespaces.
(In reply to comment #22)
> I sticked to the same header scheme like in e.g. calIcsService.cpp.
the fact that the schema was used before doesn't mean it is correct :)


what about my comment on NS_ENSURE_SUCCESS?


(In reply to comment #27)
> what about my comment on NS_ENSURE_SUCCESS?
I generally dislike macros, expecially those containing a return statement. But however this one is quite common, I can use it; I'm unemotional about it.
mvl, anything else on the patch? I'd like to prevent bitrotting.
Comment on attachment 277250 [details] [diff] [review]
[checked in] first shot, lightning only -- use NS_ENSURE_SUCCESS before checking in

ok, looks good to me (for after 0.7)
Attachment #277250 - Flags: review?(mvl) → review+
Attachment #277250 - Attachment description: first shot, lightning only → first shot, lightning only -- use NS_ENSURE_SUCCESS before checking in
- We still package storage, although I don't see the need for that. Any objections removing it from calendar/lightning/Makefile.in?

- The webdav component seems to be the last missing piece towards full frozen linkage of lightning. Since bug 325397 seems to be stalled (integrating webdav into gecko/xulrunner), I'd like to change the webdav code to link frozen (and hope not so much non-frozen API is used). Any objections?
Keywords: checkin-needed
Whiteboard: [checkin-needed after 0.7]
There is not a chance that the code in extensions/webdav is going to become part our platform, so you should definitely go forward with making it use frozen linkage.
Thunderbird 1.5 comes without storage, so keep branch builds still package storage.
Attachment #285463 - Flags: review?(mvl)
If we decide to drop TB 1.5 support after the 0.7 release (see http://weblogs.mozillazine.org/calendar/2007/10/future_of_lightning_support_fo.html) the additional #ifdef could go as well.
- filed follow-up bug 400382 and patch for webdav
- removed dependency on bug 325397 per comment #32
- added dependency on bug 400382
Depends on: 400382
No longer depends on: 325397
(In reply to comment #34)
Simon, I share the same opinion you posted on the blog:
"In addition I do not expect us to blatantly break TB 1.5 support even if we decide to officially drop our support for it. The core code for TB 1.5 and TB 2 does not differ significantly and even if we do not support it any longer, one *might* still get Lightning to work on TB 1.5..."

Thus we should still ship webdav. My Windows TB 1.5.0.10 still seems to run lightning well, although I haven't tested it deeper.
Attachment #285463 - Flags: review?(mvl) → review+
Flags: wanted-calendar0.8+
Keywords: checkin-needed
Whiteboard: [checkin-needed after 0.7]
Target Milestone: --- → 0.8
Attachment #277250 - Attachment description: first shot, lightning only -- use NS_ENSURE_SUCCESS before checking in → [checked in] first shot, lightning only -- use NS_ENSURE_SUCCESS before checking in
Attachment #277250 - Attachment is obsolete: true
Attachment #285463 - Attachment description: package lightning without storage on trunk builds → [checked in] package lightning without storage on trunk builds
Attachment #285463 - Attachment is obsolete: true
Flags: wanted-calendar0.8+
Flags: blocking-calendar0.7-
Blocks: 394879
What still needs to be done to resolve this bug?

trunk only:
Moving the question from bug 400382 comment #13 and following to this bug:
Is it feasible at all to link a static (frozen only) *sunbird*?
If the static mozilla core libs require internal linkage, I only see one further effort which could be spent: forcing calbscmp to be a frozen shared lib (like webdav). This would be favourable w.r.t. bug 400949, too.

Any opinions?
- calendar code (+webdav) links frozen (trunk only)
- all components come as shared lib (bug 400949)
=> FIXED
Status: ASSIGNED → RESOLVED
Closed: 17 years ago
Resolution: --- → FIXED
verified trunk lightning builds
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.