500123 - Mac: useless-ui print preview shouldn't be in the mac context menu & the window title includes the app name

Status: RESOLVED FIXED

(Thunderbird :: General, defect)

Description

[Phil Ringnalda (:philor)]

As part of The Great War Against Use Of The Preprocessor, bug 474701 replaced various bits of code that want to know if we're running on a Mac with gPlatformOSX (which jminta will eventually want to remove in his Battle Against Global Vars That Just Obfuscate How You Should Really Find Things Out For Yourself). That'd be fine, I'm all for duelling Philosophies Of The One True Way To Write Code, except gPlatformOSX is busted: it expects the first three characters of a mac window.navigator.oscpu to be "mac", but in fact the two Mac values for oscpu are (as assigned by the preprocessor :)) "PPC Mac OS X" and "Intel Mac OS X". What we really want is (the preprocessor assigned) navigator.platform, which will be either MacIntel or MacPPC (until something that doesn't think any in-tree code that could just be using the preprocessor will be grovelling through those strings, and changes them).

Besides the "ooh, icky, it's not Mac-native" app name in the window title, that also gives us a totally broken Print Preview item in the Mac context menu.

Comment 1

[Mark Banner (:standard8)]

We should fix this before b3

Comment 2

[Phil Ringnalda (:philor)]

I'd guess "print previous" is broken everywhere :)

Comment 3

[Phil Ringnalda (:philor)]

And actually, we need to get rid of the "first three characters" part, too: nobody makes us any guarantees about "all .platform values which start with the characters m-a-c will be XP_MACOSX," so we can either do the right thing, and go back to using the preprocessor to set gPlatformOSX, which is the only thing that makes any promises (XP_MACOSX will always be OS X, and any change to it will change any use of "XP_MACOSX"), or do the wrong and fragile thing, test navigator.platform against MacIntel and MacPPC, and hope that we'll remember about this if there's ever another Mac CPU (which is at least slightly more likely than that we would remember to change from testing the first three characters if netwerk adds support for some non-Macintosh platform that happens to start with m-a-c).

Comment 4

[Dan Mosedale (:dmosedale, :dmose)]

I'm a little confused by the new summary: it sort of seems to imply that "print preview" shouldn't be in the context menu at all.  Is that what you intended?

As far as comment 3, I'd suggest that the right solution (perhaps not for b3, but soon) would be to cause .platform to make some non-fragile guarantee that we can use.

Comment 5

[Phil Ringnalda (:philor)]

Right: on OS X we don't have a print preview context menuitem, just like we don't have a print preview menuitem and just like every other Mac program doesn't have a print preview menuitem: Preview is a program that you send things to from the print dialog if you want to look at them before you print them.

Comment 6

[Dan Mosedale (:dmosedale, :dmose)]

Ah, yes; I had completely forgotten that.  In that case, I don't think this bug blocks b4.  Neither of those regression are really that big a deal.  Moving out.

Comment 7

[Dan Mosedale (:dmosedale, :dmose)]

I meant to type "b3", of course, but I don't think they'd block b4 either, so setting the target milestone at rc1 was a fine choice.  :-)

Comment 8

[Andrew Sutherland [:asuth] (he/him)]

It is well within our power to use XP_MACOSX without having to make a javascript file pre-processed and to reflect that into something accessible from all javascript code.

One possibility that would ensure ordering is to create a platform info JS module that has a OS X-specific copy installed for Macs and uses a generic one for everybody else.  I know we already have some OS X specific stuff with the hidden window mechanism.  That might also be workable but we would want to avoid a situation where it seemed like we were not on OS X for a bit and then we are on OS X for a bit.

I would also like to make it clear that as much as I think it is dubious to use preprocessing constructs that are illegal syntax in the language they are used in, my primary concern is over making the Thunderbird development process easier.  Preprocessed chrome files cannot be symlinked which means an extra step that must be performed when people are modifying chrome and also means they shoot themselves in the foot when they forget.

Comment 9

[Mark Banner (:standard8)]

So far I've come up with several different ways of fixing this:

1) Do what we do in some xpcshell-tests:

var isOSX = ("nsILocalFileMac" in Components.interfaces);

2) JS module selection from Makefile (as per Andrew's comment).

3) preprocessing in a standalone js module.

Given that 2 and 3 are basically a variant of 1, we could potentially do 1 anyway, even though it may be seen as a bit hackish.

However, rather than just exposing a global variable, how about we do an attribute on steelIApplication? We can then "hide" what we're doing, so it is easy to change should something happen, and I'd be willing to hack the build system so that we can have two versions of a unit test - one for mac, one for others.

Thoughts?

Comment 10

[Andrew Sutherland [:asuth] (he/him)]

Option 1 exposed via STEEL sounds like a great approach to me.

Comment 11

[Mark Banner (:standard8)]

Attachment: Proposed fix

This fixes it using steelIApplication as the new centre. The unit test selection uses the same check as nsILocalFileMac, not sure if that is good thing or not, but at least if core changes something we're likely to pick it up.

Comment 12

[Andrew Sutherland [:asuth] (he/him)]

Comment on attachment 386372 [details] [diff] [review]
Proposed fix

rich review: http://reviews.visophyte.org/r/386372/

poor review:

on file: mail/base/content/mailContextMenus.js line 199
>   if (Components.classes["@mozilla.org/steel/application;1"]
>                 .getService(Components.interfaces.steelIApplication)
>                 .platformIsMac)

STEEL goes out of its way to introduce the global "Application" into every
window's global namespace.  You should be able to just use that.  Please
change this in all usages.


on file: mail/base/content/tabmail.xml line 1025
>       <method name="setDocumentTitle">

good call on factoring this out.


on file: mail/steel/mac/test_platform.js line 11
> var XULAppInfo = {

Maybe log a bug on refactoring the fake xulrunner info into a reusable thing?


on file: mail/steel/mac/test_platform.js line 42
>   do_check_true(Cc["@mozilla.org/steel/application;1"]
>                   .getService(Ci.steelIApplication).platformIsMac);

This probably needs to stay like this though (doubt Application will exist
here.)

Comment 13

[Andrew Sutherland [:asuth] (he/him)]

philor, does this look good to you?  (I'm afraid to add you as r? in case you demand preprocessing be added ;)

Comment 14

[Phil Ringnalda (:philor)]

Yeah, toolkit==cocoa is an odd proxy for Machood, but we use it all over, and at least for the in-tree uses we aren't going to incur the wrath of the Darwin/X11 macports maintainer. Whether or not some user of the STEEL interface will misuse it as a proxy for either MOZ_FS_LAYOUT==bundle or OS_ARCH==Darwin is hard to guess, but I don't want to borrow that trouble yet.

However, once your back is turned I am going to preprocess an image or a text file or something, just to keep things in balance.

Comment 15

[Mark Banner (:standard8)]

Checked in: http://hg.mozilla.org/comm-central/rev/f9f606a5f2bd

Comment 16

[Mark Banner (:standard8)]

I'd love to update the STEEL page with the new attribute, but I can't work out how to sensibly refactor the devmo fuel/exthelper/steel pages to make it them work. Therefore adding dev-doc-needed.

Comment 17

[jenzed]

MDC refactoring  for FUEL / STEEL is done (https://developer.mozilla.org/en/Toolkit_API/STEEL). "platformIsMac" attribute noted on steelIApplication doc page.