Simplify providing an nsIController implementation for tab modes

RESOLVED FIXED

Status

defect
RESOLVED FIXED
10 years ago
10 years ago

People

(Reporter: mnyromyr, Assigned: mnyromyr)

Tracking

Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 3 obsolete attachments)

Any tab mode can provide an nsIController implementation to (de)activate commands. But the order of parameters is different from that expected by nsIController, hence we can't just inherit from other nsIController implementations.
Attachment #385212 - Flags: superreview?(bugzilla)
Attachment #385212 - Flags: review?(bugzilla)
Posted patch un-bitrotted version (obsolete) — Splinter Review
I think I unbitrotted this right, but I'm seeing this js error on startup:

JavaScript error: chrome://messenger/content/mailWindowOverlay.js, line 2166: DefaultController is not defined

though the code seems to actually be working fine. So I'm not quite sure what is going on here.

(note, no sr needed for mail/).
Attachment #385212 - Attachment is obsolete: true
Attachment #386704 - Flags: review-
Attachment #385212 - Flags: superreview?(bugzilla)
Attachment #385212 - Flags: review?(bugzilla)
> JavaScript error: chrome://messenger/content/mailWindowOverlay.js, line 2166:
> DefaultController is not defined
> 
> though the code seems to actually be working fine. So I'm not quite sure what
> is going on here.

That's because TB puts the tabmail object into mailWindowOverlay.js, which gets drawn into SearchDialog.xul.
I also forgot the standalone message window in the first version.
This patch reflects the current state of the SMTabMail patch.
Attachment #386704 - Attachment is obsolete: true
Attachment #396495 - Flags: review?(bugzilla)
(In reply to comment #2)
> Created an attachment (id=396495) [details]
> only use existing default controllers
> 
> > JavaScript error: chrome://messenger/content/mailWindowOverlay.js, line 2166:
> > DefaultController is not defined
> > 
> > though the code seems to actually be working fine. So I'm not quite sure what
> > is going on here.
> 
> That's because TB puts the tabmail object into mailWindowOverlay.js, which gets
> drawn into SearchDialog.xul.

I think you more mean hiddenWindow.xul. SearchDialog.xul doesn't get loaded on startup.

So it is because mailWindowOverlay.js is being drawn in and that wants a DefaultController because of the newly created inheritance? That feels like we should move it elsewhere, but I won't laden that on you (out of interest, where does SM put it?) - but please change the comment to explicitly say why you're alternately using MessageWindowController even though it isn't ever going to be called like that.

> I also forgot the standalone message window in the first version.

I don't understand - is this just because we got the error opening a standalone message window as well?


You also didn't update specialTabs.js (contentTabType).
Attachment #396495 - Flags: review?(bugzilla) → review-
Comment on attachment 396495 [details] [diff] [review]
only use existing default controllers

r- based on previous comment.
(In reply to comment #3)
> > drawn into SearchDialog.xul.
> 
> I think you more mean hiddenWindow.xul.

Actually, it's msgAccountCentral.xul, sorry for the confusion.
msgAccountCentral.xul gets loaded on startup and draws mailWindowOverlay.js, which contains mailTabType.

> So it is because mailWindowOverlay.js is being drawn in and that wants a
> DefaultController because of the newly created inheritance?

Yes.

> That feels like we should move it elsewhere, but I won't laden that on you
> (out of interest, where does SM put it?)

Yes, you should put mailTabType into its own file and only include it messenger.xul (and possibly in messageWindow, if you need certain stuff there).
SM defines its mailTabType in a separate tabmail.js file, which gets included by  messenger.xul and messageWindow.xul.

> - but please change the comment to explicitly say why you're alternately
> using MessageWindowController even though it isn't ever going to be called
> like that.

Fixed.

> > I also forgot the standalone message window in the first version.
> 
> I don't understand - is this just because we got the error opening a 
> standalone message window as well?

The standalone window indirectly draws mailTabType as well, but its default controller is named "MessageWindowController", not "DefaultController".
TB doesn't actually seem to use mailTabType in messageWindow.xul, afaict, though, so taking care of it may be bit "overcorrect".

> You also didn't update specialTabs.js (contentTabType).

Fixed.
Attachment #396495 - Attachment is obsolete: true
Attachment #396700 - Flags: review?(bugzilla)
Comment on attachment 396700 [details] [diff] [review]
fix up additional implementations

Thanks for tidying this up. r=Standard8
Attachment #396700 - Flags: review?(bugzilla) → review+
Pushed as <http://hg.mozilla.org/comm-central/rev/67a4f048f573>, finally.
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.