Why ? Because this way it is way more useful and it could be used in a similar way to the Firefox homepage / bookmark.

Comment on attachment 345559 [details] [diff] [review] Patch v1 >+ Spurious whitespace ^^^ >+ var startpageenabled = pref.getBoolPref("mailnews.start_page.enabled"); >+ if (gDisplayStartupPage && startpageenabled) > { > loadStartPage(); > gDisplayStartupPage = false; >- UpdateMailToolbar("gDisplayStartupPage"); >- } >+ } Spurious whitespace ^^^ ... but more importantly, there is no "pref" defined in the above code context. The exception is happily swallowed somewhere though... :/ And there is no need to call UpdateMailToolbar in case the start page isn't getting loaded.

(In reply to comment #2) > ... but more importantly, there is no "pref" defined in the above code context. >+ var startpageenabled = pref.getBoolPref("mailnews.start_page.enabled"); ? > The exception is happily swallowed somewhere though... :/ you want exception for the above pref ? , loadstartpage() function is catching exception for loading the page > And there is no need to call UpdateMailToolbar in case the start page isn't > getting loaded. I had too move it out because with option to check for mail on startup and with disabled startpage, getmail button was disabled (until I clicked somewhere) now I've noticed that it happens only with Lightning extension enabled, it's not a big problem in this case but my patch did it and I would like to fix it but unfortunately I don't know why it happens :|

> > And there is no need to call UpdateMailToolbar in case the start page isn't > > getting loaded. > I had too move it out because with option to check for mail on startup and with > disabled startpage, getmail button was disabled (until I clicked somewhere) > now I've noticed that it happens only with Lightning extension enabled, it's > not a big problem in this case but my patch did it and I would like to fix it > but unfortunately I don't know why it happens :| previously loadStartPage() was executed at every startup which calls at the end ClearMessageSelection(), with my patch loadStartPage() called only when there is a need to so should I leave UpdateMailToolbar where it is, use ClearMessageSelection() instead or you have other idea to solve disabled getmail for Lightning ?

(In reply to comment #3) What I meant was, all pref.getBoolPref will ever do there is cause an exception, since pref isn't defined. And since you'll always get the exception, the i can't see how the UpdateMailToolbar call would ever get reached in the first place...

(In reply to comment #5) > What I meant was, all pref.getBoolPref will ever do there is cause an > exception, since pref isn't defined. hmm I think we have some misunderstanding here, I'm not using new pref because there is no need to, I'm using existing pref to check if user wants to show start page on startup or not as you see from the patch pref.getboolpref was moved from mailWindow.js/loadStartPage() to commandglue.js/FolderPaneSelectionChange() sorry, maybe I should have commented more but I hope all is clear now ? I've tested the patch in a various ways and all works well in the practice > And since you'll always get the exception, the i can't see how the > UpdateMailToolbar call would ever get reached in the first place...

Yes, I know you used the existing pref name, what i'm saying is the "pref" in pref.getBoolPref isn't defined inside the function you are now using it in.

OK, sorry... so adding var pref = Components.classes["@mozilla.org/preferences-service;1"] .getService(Components.interfaces.nsIPrefBranch2); before var startpageenabled should be enough ? I would notice it if this would not work but it did :| Regarding UpdateMailToolbar - should I leave it where it was originally and file bug for Lightning or fix it now as proposed in the current patch ?

Looks like gPrefBranch is defined, so use that. Re UpdateMailToolbar I doubt it's a problem when the code is actually working ;) But if it is, no we shouldn't say startpage was loaded if it wasn't. If it's needed just UpdateMailToolbar("FolderPaneSelectionChange"); or something

Updated to comments, I hope ;)

Comment on attachment 345957 [details] [diff] [review] Patch v2 Since we're always going to do UpdateMailToolbar("FolderPaneSelectionChange"); at the end, you can remove the two previous UpdateMailToolbar calls... Updating it once should be enough. Also, please wrap the GetMessagePaneFrame().location.href line at ~80 chars. BTW, I see now pref was indeed defined (through mailWindow.js i suppose), though using gPrefBranch is still the way to go in this file.

> Since we're always going to do UpdateMailToolbar("FolderPaneSelectionChange"); > at the end, you can remove the two previous UpdateMailToolbar calls... Updating > it once should be enough. yeah, should be enough > Also, please wrap the GetMessagePaneFrame().location.href line at ~80 chars. OK, if it would be not the best wrap feel free to fix it during checkin > BTW, I see now pref was indeed defined (through mailWindow.js i suppose), > though using gPrefBranch is still the way to go in this file. so if I understand you properly there is no need to redefine if it's working (as in the last patch) patch coming

Updated to comments

Comment on attachment 346089 [details] [diff] [review] Patch v3 Looks good, though the wrapping of the line is a bit odd. I can fix that before checking in.

Thanks, it's odd because my editor fooled me -> it got ditched :)

As a side effect of the UpdateMailToolbar call, this also fixes bug 449679. changeset: 1006:90b2b7edbdcd http://hg.mozilla.org/comm-central/rev/90b2b7edbdcd ->FIXED

-> VERIFIED Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.9.1b2pre) Gecko/20081105 Lightning/1.0pre Shredder/3.0b1pre