467021 - about / chrome start pages should be allowed to load in offline mode

Status: RESOLVED FIXED

(Thunderbird :: Preferences, defect)

Comment 1

[Przemyslaw Bialik]

Attachment: Fix v1

Allows to load uri.scheme about and chrome in online/offline mode.

For completeness data and file schemes could be also allowed ?
While I'm at it I could enhance the IF
> if (uri && [...]
and fix partially Bug 388509, wanted ?

Comment 2

[Magnus Melin [:mkmelin]]

Comment on attachment 350376 [details] [diff] [review]
Fix v1

>+    // Load about:blank as the start page if we are offline and uri.scheme
>+    // is different from about / chrome or we don't have a start page url...
>+    var scheme = "";
>+    var uri = null;

No need to initialize either of these, and uri should be declared where first used.

>+    try {
>+      uri = makeURI(startpage);
>+      scheme = uri.scheme;
>+    } catch (ex) {}

No need to try catch here, (especially) as you're inside a try-block already.
For future reference, in cases you need to catch, usually dumping out what
the exception was is helpful.

>+    if (scheme == "about" || scheme == "chrome" || MailOfflineMgr.isOnline()
>+                                                && startpage != "")

What about file:// and res://? Blacklist offline http:// and https:// instead?

"" is falsy, so prefer to use !startpage

>+      GetMessagePaneFrame().location.href = startpage;
>+    else
>+      GetMessagePaneFrame().location.href = startpage = "about:blank";

I think I'd much prefer if you do an if MailOfflineMgr.isOnline() and put all 
the additional scheme checking inside that.

Comment 3

[Magnus Melin [:mkmelin]]

>+      GetMessagePaneFrame().location.href = startpage = "about:blank";

An extra = startpage snuck in.

Comment 4

[Przemyslaw Bialik]

> >+    try {
> >+      uri = makeURI(startpage);
> >+      scheme = uri.scheme;
> >+    } catch (ex) {}
> No need to try catch here, (especially) as you're inside a try-block already.
> For future reference, in cases you need to catch, usually dumping out what
> the exception was is helpful.
without try catch header pane is being hidden when trying to load badly formatted uri (instead of error or about:blank) - makeURI(startpage) failing ?
 
> What about file:// and res://? Blacklist offline http:// and https:// instead?
OK, will blacklist for offline

> "" is falsy, so prefer to use !startpage
startpage != "" is needed to not try to load undefined startpage -> you end up with thunderbird window loaded :)
 
should I check for properly formatted uri and prevent "File Not Found" or leave it for Bug 388509

Comment 5

[Przemyslaw Bialik]

(In reply to comment #4)
> should I check for properly formatted uri and prevent "File Not Found" or leave
> it for Bug 388509
even if it would be fixed it would rather not apply to existing prefs hmm

BTW I've used this part as my "book example"
http://hg.mozilla.org/mozilla-central/file/a8f64c6df6b8/toolkit/mozapps/extensions/content/about.js#l77

Comment 6

[Magnus Melin [:mkmelin]]

(In reply to comment #4)
> without try catch header pane is being hidden when trying to load badly
> formatted uri (instead of error or about:blank) - makeURI(startpage) failing ?

Ok, keep the try-catch. I suppose that's a bit better behaviour.

> startpage != "" is needed to not try to load undefined startpage -> you end up
> with thunderbird window loaded :)

What i meant was: if (startpage != "") behaves the same way as if (startpage)

> should I check for properly formatted uri and prevent "File Not Found" or leave
> it for Bug 388509

Your choice;)

Comment 7

[Przemyslaw Bialik]

> > should I check for properly formatted uri and prevent "File Not Found" or leave
> > it for Bug 388509
> Your choice;)
thanks ;)
I'll leave it as it is and I will look into mentioned bug later on

Comment 8

[Przemyslaw Bialik]

Attachment: Fix v2

v2
I didn't want to introduce too many IFs but if this one is too long I can split it into 1 or 2 inside

Comment 9

[Magnus Melin [:mkmelin]]

Comment on attachment 350671 [details] [diff] [review]
Fix v2

Nit: |var scheme| isn't properly indented.

With that, r=mkmelin. Thanks for the patch!

Comment 10

[Przemyslaw Bialik]

Attachment: Fix v3

nit fixed, carrying forward r+

Comment 11

[Mark Banner (:standard8)]

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

Comment 12

[Przemyslaw Bialik]

Attachment: comment fix

description fix

Comment 13

[Przemyslaw Bialik]

I've just noticed "small" bug in description, sorry!
Can you Magnus or Mark please checkin it quickly ?

Comment 14

[Mark Banner (:standard8)]

Comment on attachment 351048 [details] [diff] [review]
comment fix

Pushed as http://hg.mozilla.org/comm-central/rev/5385b69dd22c