954801 - Suppress errors when running without libpurple

Status: RESOLVED FIXED

(Chat Core :: General, enhancement)

Description

[Benedikt Pfeifer]

*** Original post on bio 1367 at 2012-04-10 13:01:00 UTC ***

Some parts of Instantbird rely on having libpurple being available and generate error messages otherwise (removing purplexpcom.dll, purple.dll and the registration code from the manifest allow to run without libpurple and only JS-based protocol plugins).

Among them are code showing the libpurple version number and proxy handling stuff.

Goal: make these parts failsafe.

Look for: http://lxr.instantbird.org/instantbird/search?string=instantbird.org%2Flibpurple

Comment 1

[Benedikt Pfeifer]

Attachment: WIP, fixing the "About"-dialog

*** Original post on bio 1367 as attmnt 1355 at 2012-04-14 15:20:00 UTC ***

I've no idea what I could or shouldn't do with the pieces touching proxy-stuff but here's a WIP for the "About"-dialog already.

Comment 2

[Florian Quèze [:florian]]

*** Original post on bio 1367 at 2012-04-16 10:20:55 UTC ***

I would prefer something like a test for ("@instantbird.org/libpurple/core;1" in Components.classes) instead of the try/catch, although I'm not sure that would be false if only the .dll file has been removed but the entries in the .manifest file are still there.


The libpurple proxy code of the account manager should never be in your way as long as this.proto.usePurpleProxy is false, which it will always be for JS and unknown protocols.
So I think the only place where you need changes related to proxies when libpurple/purplexpcom aren't there is the Advanced->Network tab of the preferences dialog.

Comment 3

[Benedikt Pfeifer]

Attachment: Fixing aboutDialog and preferences tab

*** Original post on bio 1367 as attmnt 1363 at 2012-04-17 10:22:00 UTC ***

This patch only does front-end changes.

I'll look into "--disable-libpurple" and can replace the "if"s with "#ifdef"s if that's wanted then. That would lose the opportunity to re-show the parts if someone upgrades a libpurple-less Ib with libpurple from an extension though(if someone created such a thing).

Comment 4

[Benedikt Pfeifer]

Comment on attachment 8353108 [details] [diff] [review]
WIP, fixing the "About"-dialog

*** Original change on bio 1367 attmnt 1355 at 2012-04-17 10:22:17 UTC was without comment, so any subsequent comment numbers will be shifted ***

Comment 5

[Florian Quèze [:florian]]

*** Original post on bio 1367 at 2012-10-09 14:46:14 UTC ***

The check at http://lxr.instantbird.org/instantbird/source/instantbird/modules/ibCore.jsm#33:
 33     if (!Ci.purpleICoreService) {
 34       this._promptError("startupFailure.purplexpcomFileError");
 35       return false;
 36     }
also needs to be changed. We can maybe use imICoreService instead of purpleICoreService?

Comment 6

[Florian Quèze [:florian]]

*** Original post on bio 1367 at 2012-10-09 15:40:16 UTC ***

Anything blocking this? Any reason why there's no review request on this patch?

I looked quickly at the patch, the only thing that seems strange is that the "For updates, addons, not-libpurple-based accounts, ..." text is still shown if the button for the libpurple proxy dialog is hidden.

Comment 7

[Benedikt Pfeifer]

*** Original post on bio 1367 at 2012-10-10 14:29:28 UTC ***

(In reply to comment #4)
> The check at
> http://lxr.instantbird.org/instantbird/source/instantbird/modules/ibCore.jsm#33:
>  33     if (!Ci.purpleICoreService) {
>  34       this._promptError("startupFailure.purplexpcomFileError");
>  35       return false;
>  36     }
> also needs to be changed. We can maybe use imICoreService instead of
> purpleICoreService?

I'd need to have a look what is happening there. The message shown is about a "missing or corrupted file 'instantbird.xpt'". That's where interfaces are defined in, aren't they?

(In reply to comment #5)
> Anything blocking this? Any reason why there's no review request on this patch?

No, not really. I think the reason was that disabling libpurple was discussed to be part of the bug too and this were just frontend changes for that.

> I looked quickly at the patch, the only thing that seems strange is that the
> "For updates, addons, not-libpurple-based accounts, ..." text is still shown if
> the button for the libpurple proxy dialog is hidden.

What about differentiating when libpurple is present and showing a label similar to Firefox' when it's not (i.e. something without details, like "Configure how Instantbird connects to the internet.").

Comment 8

[Florian Quèze [:florian]]

*** Original post on bio 1367 at 2012-10-10 14:57:50 UTC ***

(In reply to comment #6)
> (In reply to comment #4)
> > The check at
> > http://lxr.instantbird.org/instantbird/source/instantbird/modules/ibCore.jsm#33:
> >  33     if (!Ci.purpleICoreService) {
> >  34       this._promptError("startupFailure.purplexpcomFileError");
> >  35       return false;
> >  36     }
> > also needs to be changed. We can maybe use imICoreService instead of
> > purpleICoreService?
> 
> I'd need to have a look what is happening there. The message shown is about a
> "missing or corrupted file 'instantbird.xpt'". That's where interfaces are
> defined in, aren't they?

So I can explain the situation, hopefully it will save you some time.

When building, lots of .xpt files are generated. Lots of them are from the Mozilla platform. We also generate an xpt file for each of these folders that contain .idl files:
instantbird/components/mintrayr
purple/purplexpcom/public
chat/components/public

At packaging time, all the .xpt files that are packaged are linked together into a single <appname>.xpt file.

The startup check that we need to modify was to ensure that the xpt file for Instantbird specific interfaces is correctly loaded (ie that we have more than the interfaces defined in the Mozilla platform).

We wrote this before the purple/ vs chat/ split and at the time purpleICoreService was required to start the core of the application.

The current equivalent of that is imICoreService.

The reason why Instantbird worked without this change during your testing is that you removed the purplexpcom.dll file, but not the purplexpcom.xpt file, as you couldn't see it because it was linked into instantbird.xpt.

> > I looked quickly at the patch, the only thing that seems strange is that the
> > "For updates, addons, not-libpurple-based accounts, ..." text is still shown if
> > the button for the libpurple proxy dialog is hidden.
> 
> What about differentiating when libpurple is present and showing a label
> similar to Firefox' when it's not

Making it as similar as possible to Firefox when libpurple isn't there sounds great.

> (i.e. something without details, like
> "Configure how Instantbird connects to the internet.").

We already have the "Configure how Instantbird connects to the Internet" label (but I suspect nobody reads it because it's displayed as the second line after the information message saying that we intend to merge the 2 parts later).

I think a good longer term solution would be to add the purple-specific parts with an overlay from the purple/ folder, so that they aren't there when libpurple support isn't there, and so that the overlay can be part of the Thunderbird libpurple add-on.
(However, I'm not requesting this change here, it can be a follow up someday when someone feels like working on it.)

Of course an even better long term solution is to hack libpurple's proxy handling to use Mozilla stuff :).

Comment 9

[Benedikt Pfeifer]

Attachment: Patch v1

*** Original post on bio 1367 as attmnt 1941 at 2012-10-10 20:16:00 UTC ***

That's the earlier patch with following modifications:

* Replaced the check for imICoreService in ibCore.jsm now (as I understood that's what I had to do)
* Change the connections groupbox on the advanced pane of the preferences to look similar to Firefox' when libpurple is not there + try to look acceptable otherwise ;)

Comment 10

[Benedikt Pfeifer]

Comment on attachment 8353116 [details] [diff] [review]
Fixing aboutDialog and preferences tab

*** Original change on bio 1367 attmnt 1363 at 2012-10-10 20:16:45 UTC was without comment, so any subsequent comment numbers will be shifted ***

Comment 11

[Benedikt Pfeifer]

Attachment: Patch v2

*** Original post on bio 1367 as attmnt 1946 at 2012-10-10 22:30:00 UTC ***

Patch keeping the description at top of the network pane as it is.

Comment 12

[Benedikt Pfeifer]

Comment on attachment 8353697 [details] [diff] [review]
Patch v1

*** Original change on bio 1367 attmnt 1941 at 2012-10-10 22:30:56 UTC was without comment, so any subsequent comment numbers will be shifted ***

Comment 13

[Florian Quèze [:florian]]

Comment on attachment 8353703 [details] [diff] [review]
Patch v2

*** Original change on bio 1367 attmnt 1946 at 2012-10-10 23:17:41 UTC ***

Looks good. I tested this and the only thing that looks like it should be improved is the separator between the connectionGroupHeader box and the purpleConnectionBox box that should be hidden when both these boxes are hidden.
I'm going to do this before the check-in.

Thanks for the updated patch! :)

Comment 14

[Patrick Cloke [:clokep]]

*** Original post on bio 1367 at 2012-10-11 01:08:43 UTC ***

http://hg.instantbird.org/instantbird/rev/dde2670cf8c4

Thanks for looking at this not very exciting bug. :-)