529921 - RFC-2597/2598/3168 Quality-of-Service (DSCP) marking should be supported

Status: RESOLVED FIXED

(Core :: Networking, defect)

Description

[Philip Prindeville]

As more real-time/streaming applications make their way into the Internet, we need to start marking our traffic so that bandwidth is managed more optimally with respect to latency/jitter.

To do this, applications need to mark their traffic with the appropriate QoS requirements as per the above RFC's.

Since HTTP and Email are significant users of bandwidth, but don't require low-latency, FF and TB are major benefactors to the proper use of bandwidth.

Comment 1

[Philip Prindeville]

See also:

http://sourceware.org/bugzilla/show_bug.cgi?id=10789

this is where it made it's way into the glibc header files.

Comment 2

[Philip Prindeville]

Note that this bug will affect both nsprpub/ and netwerk/protocol/ as the low-level socket primitives will need to support the setsockopt() code, and the ftp, gopher, http, etc. code will need to add a call to this primitive.

Comment 3

[Philip Prindeville]

Actually, digging around, the code to support IP_TOS (PR_SockOpt_IpTypeOfService) is already present.

So just the protocol modules (NetLib) need to be modified.

Comment 4

[Christian :Biesinger (don't email me, ping me on IRC)]

HTTP would certainly like low-latency. Speed is important for web apps.

Comment 5

[Philip Prindeville]

Attachment: Patch for infrastructure and browser

Fairly straightforward patch.  The interface lets you specify what QoS bit you want on the connection, then when InitiateSocket() happens, the PRSetSocketOptions() happens.

Comment 6

[Philip Prindeville]

Attachment: Mail fixes

Needs a little cleanup for the error handling in nsMsgProtocol::GetProtocolInfo(), nsMsgProtocol::OpenNetworkSocketWithInfo(), and nsImapProtocol::SetupWithUrl().

Otherwise, it's been tested with and without entries in my prefs.js file:

user_pref("mailnews.imap.qos", 56);

which yields:

23:18:26.564448 IP (tos 0x38, ttl 64, id 13156, offset 0, flags [DF], proto TCP (6), length 52)
    192.168.1.10.34635 > 192.168.1.3.imaps: Flags [.], cksum 0xf45e (correct), seq 328, ack 1647, win 105, options [nop,nop,TS val 2619202032 ecr 259678746], length 0

with it deleted via the config-editor, we get:

23:21:10.525044 IP (tos 0x0, ttl 64, id 62490, offset 0, flags [DF], proto TCP (6), length 52)
    192.168.1.10.57883 > 192.168.1.3.imaps: Flags [.], cksum 0x95f4 (correct), seq 1192, ack 88321, win 501, options [nop,nop,TS val 2619365992 ecr 259842707], length 0

Comment 7

[Philip Prindeville]

Attachment: Mail fixes

Comment 8

[Philip Prindeville]

Attachment: Mail fixes

Comment 9

[Philip Prindeville]

Attachment: Mail fixes

Left out fix for nsNntpService.cpp

Comment 10

[Christian :Biesinger (don't email me, ping me on IRC)]

Comment on attachment 444236 [details] [diff] [review]
Patch for infrastructure and browser

So with the comments below, this patch looks good technically.

But since this patch adds a feature that will only be used by a tiny minority of Firefox users, I'd like the opinion of bz/jduell on whether we should take this.

+++ b/netwerk/base/public/nsISocketTransport.idl	Thu May 06 11:40:38 2010 -0600
+    attribute octet QoSBits;

You added this attribute in between setTimeout and the constants that setTimeout uses. Please move it after the constants.

+++ b/netwerk/base/src/nsSocketTransport2.cpp	Thu May 06 11:40:38 2010 -0600
+    // don't do any checking here of bits...

That comment should explain a bit more about why the checking isn't done. Maybe "Don't do any checking here, PR_SetSocketOption will deal with that later"

Comment 11

[Philip Prindeville]

Attachment: Patch for infrastructure and browser

(In reply to comment #10)

Addressing these two recommendations.

Comment 12

[Christian :Biesinger (don't email me, ping me on IRC)]

Comment on attachment 445794 [details] [diff] [review]
Patch for infrastructure and browser

re-setting feedback request, see comment 10

Comment 13

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 445794 [details] [diff] [review]
Patch for infrastructure and browser

This is fine, though I think the premise that high latency is ok for http across the board is wrong: web pages in practice require serial loads and people do things like streaming video over http...

Longer term it might make more sense to expose the QoS on a per-http-channel basis or something, but that can build on this infrastructure.  If we had _that_, it would be pretty nice.

Comment 14

[Philip Prindeville]

(In reply to comment #13)
> (From update of attachment 445794 [details] [diff] [review])
> This is fine, though I think the premise that high latency is ok for http
> across the board is wrong: web pages in practice require serial loads and
> people do things like streaming video over http...
> 
> Longer term it might make more sense to expose the QoS on a per-http-channel
> basis or something, but that can build on this infrastructure.  If we had
> _that_, it would be pretty nice.

Indeed, Christian and I discussed this in email... and the conclusion was that video is typically decoded in a plugin that manages its own sockets, so that plugin has the ability to manipulate QoS as it sees fit.

Since PR_SocketSetSocketOption() is part of NSPR, this capability is already exposed to plugins.

Comment 15

[Jason Duell]

Comment on attachment 445794 [details] [diff] [review]
Patch for infrastructure and browser

Should we check this into m-c, then?

Comment 16

[Philip Prindeville]

(In reply to comment #15)
> (From update of attachment 445794 [details] [diff] [review])
> Should we check this into m-c, then?

Are you asking me?  If so, then I say, "go for it".

As submitted, the code won't have any effect until people explicitly enable it by modifying their preferences.

Comment 17

[Nomis101]

I've tried both patches for Thunderbird (with default values and recommended DSCP values). Receiving mails (pop) works just fine. But sending mails results in a crash of TB. So I've backed out the mailnews patch and sending mails worked again. 
Therefore I've made an TB debug build with both patches and debbuged it with gdb. This gives me:

Program received signal EXC_BAD_ACCESS, Could not access memory.
Reason: KERN_PROTECTION_FAILURE at address: 0x00000000
0x01725970 in nsMsgAsyncWriteProtocol::SendData (this=0x2f986450, aURL=0x30537e14, dataBuffer=0xbfffde8c "EHLO iMac.local\r\n", aSuppressLogging=0) at /Volumes/Developer/temp/src/mailnews/base/util/nsMsgProtocol.cpp:1524
1524	  nsresult rv = m_outputStream->Write(dataBuffer, len, &cnt);
(gdb)

Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.6; en-US; rv:1.9.3a5pre) Gecko/20100519 Shredder/3.2a1pre

Comment 18

[Philip Prindeville]

(In reply to comment #17)
> I've tried both patches for Thunderbird (with default values and recommended
> DSCP values). Receiving mails (pop) works just fine. But sending mails results
> in a crash of TB. So I've backed out the mailnews patch and sending mails
> worked again. 
> Therefore I've made an TB debug build with both patches and debbuged it with
> gdb. This gives me:
> 
> Program received signal EXC_BAD_ACCESS, Could not access memory.
> Reason: KERN_PROTECTION_FAILURE at address: 0x00000000
> 0x01725970 in nsMsgAsyncWriteProtocol::SendData (this=0x2f986450,
> aURL=0x30537e14, dataBuffer=0xbfffde8c "EHLO iMac.local\r\n",
> aSuppressLogging=0) at
> /Volumes/Developer/temp/src/mailnews/base/util/nsMsgProtocol.cpp:1524
> 1524      nsresult rv = m_outputStream->Write(dataBuffer, len, &cnt);
> (gdb)
> 
> Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.6; en-US; rv:1.9.3a5pre)
> Gecko/20100519 Shredder/3.2a1pre

I'm thinking that the patch if it were bad would have caused an exception during the setsockopt() system call.  Otherwise, there's very little about this patch that would impact the kernel, since it's largely internal housekeeping in the objects themselves.

Can you breakpoint both setsockopt() and trace through it in the case of the option-name being IP_TOS, as well as tracing through nsMsgAsyncWriteProtocol::SendData() all the way down to the write()/send() system call and the returned value.

I suspect that MacOS might not be handling IP_TOS correctly in this case.

Comment 19

[Philip Prindeville]

(In reply to comment #18)

> I suspect that MacOS might not be handling IP_TOS correctly in this case.

There's an easy way to verify this.  Get into:

netwerk/base/src/nsSocketTransport2.cpp:1147

and comment our the line:

// PR_SetSocketOption(fd, &opt);

and then rebuild and rerun, and tell us what happens.

If the image then works, it means that MacOS is choking on setsockopt(..., IP_TOS, ...).

If you're running the image as patched, however, and you've not modified your QoS setting for SMTP, then:

if (mQoSBits) {
    opt.option = PR_SockOpt_IpTypeOfService;
    opt.value.tos = mQoSBits;
    PR_SetSocketOption(fd, &opt);
}

will be false and the PR_SetSocketOption (i.e. setsockopt()) should never be reached.

Comment 20

[Nomis101]

(In reply to comment #19)
> (In reply to comment #18)
> 
> > I suspect that MacOS might not be handling IP_TOS correctly in this case.
> 
> There's an easy way to verify this.  Get into:
> 
> netwerk/base/src/nsSocketTransport2.cpp:1147
> 
> and comment our the line:
> 
> // PR_SetSocketOption(fd, &opt);
> 
> and then rebuild and rerun, and tell us what happens.

OK, I've tried that. But I still get this crash.


    if (mQoSBits) {
        opt.option = PR_SockOpt_IpTypeOfService;
        opt.value.tos = mQoSBits;
//        PR_SetSocketOption(fd, &opt);
    }

Comment 21

[Philip Prindeville]

(In reply to comment #17)
> I've tried both patches for Thunderbird (with default values and recommended
> DSCP values).

Ok, let's back up a bit.  What happens when you build and run Firefox both with and without QoS set?

Comment 22

[Nomis101]

No problems with firefox in my quick and basic test.
Maybe someone else should try this patch with Thunderbird on OS X to assure that my SMTP issue with this patch is not limited to me and really relies to this patch...

Comment 23

[Philip Prindeville]

If you breakpoint in nsMsgAsyncWriteProtocol::SendData() just before the call to ::Write() and print *this what do you get?  Does it look sane?

Comment 24

[Nomis101]

(In reply to comment #23)
> If you breakpoint in nsMsgAsyncWriteProtocol::SendData() just before the call
> to ::Write() and print *this what do you get?  Does it look sane?

I can try this. But I don't know how to do this, I've never breakpointed something before...

Comment 25

[Philip Prindeville]

Please hold off on commit.

I'm looking at whether nsIProtocolHandler might be a better place to store QoS preferences.

Comment 26

[David :Bienvenu]

Comment on attachment 444785 [details] [diff] [review]
Mail fixes

I would suggest that you use the server type as an arg to GetProtocolInfo, not the server object. Or, you could use the url scheme to figure out what the server type is. For smtp, mailnewsUrl->GetServer isn't going to get you an smtp server, since  smtp servers aren't incoming servers.

Comment 27

[Philip Prindeville]

(In reply to comment #26)
> (From update of attachment 444785 [details] [diff] [review])
> I would suggest that you use the server type as an arg to GetProtocolInfo, not
> the server object. Or, you could use the url scheme to figure out what the
> server type is. For smtp, mailnewsUrl->GetServer isn't going to get you an smtp
> server, since  smtp servers aren't incoming servers.

Can you paste a snippet example?

Thanks.

Comment 28

[Philip Prindeville]

Attachment: Mail fixes

Reworked.

Made GetQoSBits() a method in nsMsgProtocol, and had the base class return NS_ERROR_NOT_IMPLEMENTED.

Modified IMAP, SMTP, POP3, and NNTP to grovel a setting out of preferences.

Wondering if the macro CLAMP() should be put in nsIPrefBranch.h.

Comment 29

[Philip Prindeville]

Attachment: Patch for infrastructure and browser

Added support for FTP and Gopher.

Comment 30

[Philip Prindeville]

Attachment: Alternative patch for browser

Same fix, but putting a common definition of the CLAMP() macro into nsIPrefBranch.idl ...

Comment 31

[Philip Prindeville]

Attachment: Alternate mail fixes

Using common CLAMP() definition for previous patch mozilla patch.

Comment 32

[Christian :Biesinger (don't email me, ping me on IRC)]

(In reply to comment #14)
> Indeed, Christian and I discussed this in email... and the conclusion was that
> video is typically decoded in a plugin that manages its own sockets, so that
> plugin has the ability to manipulate QoS as it sees fit.

I don't think that's what I said, because for HTTP plugins typically use NPN_GetURL which does not let them set QoS. That said a per-channel setting could easily be added based on this work.

Comment 33

[Christian :Biesinger (don't email me, ping me on IRC)]

Comment on attachment 447931 [details] [diff] [review]
Patch for infrastructure and browser

+++ b/netwerk/protocol/gopher/src/nsGopherHandler.cpp	Thu May 27 23:33:05 2010 -0600

You could save significant complexitly by just reading the pref in nsGopherChannel instead of caching it here. Please do that.

Comment 34

[Philip Prindeville]

(In reply to comment #33)
> (From update of attachment 447931 [details] [diff] [review])
> +++ b/netwerk/protocol/gopher/src/nsGopherHandler.cpp    Thu May 27 23:33:05
> 2010 -0600
> 
> You could save significant complexitly by just reading the pref in
> nsGopherChannel instead of caching it here. Please do that.

Well, the flip side to the "complexity" is then it becomes inconsistent with the way that other protocols handle this.  How about a compromise where I don't add the Observer, but simply query the preference from the protocolHandler whenever the QoS bits are queried?

I think "complexity" can also be measured by how often one deviates from an established model of a service or object.

Comment 35

[Philip Prindeville]

Attachment: Alternative patch for browser

As per comment #33.

Comment 36

[Christian :Biesinger (don't email me, ping me on IRC)]

(In reply to comment #34)
> Well, the flip side to the "complexity" is then it becomes inconsistent with
> the way that other protocols handle this.  How about a compromise where I don't
> add the Observer, but simply query the preference from the protocolHandler
> whenever the QoS bits are queried?
> 
> I think "complexity" can also be measured by how often one deviates from an
> established model of a service or object.

I'm fine with this specific change, but note that different protocols handle socket I/O rather differently anyway. Some open the socket transport in the channel, HTTP has a Connection object, FTP has nsFtpState, etc. So I don't feel that QoS/prefs have to work the same way across protocols.


As mentioned on IRC please move CLAMP to nscore.h and rename it to NS_CLAMP.

Comment 37

[Philip Prindeville]

Attachment: Alternative patch for browser

Moved CLAMP to nscore.h and renamed NS_CLAMP.

Comment 38

[Christian :Biesinger (don't email me, ping me on IRC)]

Comment on attachment 448536 [details] [diff] [review]
Alternative patch for browser

+ * Use NS_CLAMP to force a preference into a range.

preference -> value, or maybe "value, such as a preference"

Comment 39

[Philip Prindeville]

Attachment: Alternative patch for browser

Per comment #38.

Comment 40

[Philip Prindeville]

Attachment: Alternate mail fixes

Changing "CLAMP" to "NS_CLAMP" as per Christian's comment.

Comment 41

[Justin Wood (:Callek)]

Philip, a skim here doesn't accurately tell me which patch[es] are ready for checkin, I also see at least one pending review.

So, to be clear; which patches and which repo's?

(please re-add checkin-needed with the answer)

Comment 42

[Philip Prindeville]

(In reply to comment #41)
> Philip, a skim here doesn't accurately tell me which patch[es] are ready for
> checkin, I also see at least one pending review.
> 
> So, to be clear; which patches and which repo's?
> 
> (please re-add checkin-needed with the answer)

For now just the mozilla/browser attachment 448568 [details] [diff] [review], in comment #39.

The mailnews attachment (448591) can't go in until after the above patch, since that one patches the SocketTransport interface and contains a needed macro in nscore.h.

Comment 43

[Boris Zbarsky [:bzbarsky]]

Pushed browser patch as http://hg.mozilla.org/mozilla-central/rev/024e485e9c49

Leaving open so we don't lose track of the mailnews patch... arguably that should have been in a different bug.

Comment 44

[David :Bienvenu]

Comment on attachment 448591 [details] [diff] [review]
Alternate mail fixes

Instead of duplicating all that code, you should just define a virtual method that is the protocol type, and use that string in a generic method to read the right pref. I'll attach a patch that shows what I mean sometime soon.

Comment 45

[Philip Prindeville]

Attachment: Alternate mailnews patch as per David's comments.

We have a virtual method that returns the name of the preference, and then a base-class method just retrieves the preference value, akin to what David suggests in comment 44.

Comment 46

[David :Bienvenu]

Comment on attachment 449315 [details] [diff] [review]
Alternate mailnews patch as per David's comments.

sorry, I'm traveling today so I didn't get to describe exactly what I meant. It wouldn't return the name of the pref; it would return the protocol type, e.g., "smtp", "imap", etc. and the pref name would be constructed from that...

Comment 47

[Philip Prindeville]

(In reply to comment #46)
> (From update of attachment 449315 [details] [diff] [review])
> sorry, I'm traveling today so I didn't get to describe exactly what I meant. It
> wouldn't return the name of the pref; it would return the protocol type, e.g.,
> "smtp", "imap", etc. and the pref name would be constructed from that...

I understood that part, but that's not enough.

I would then need to change GetQoSBits do something like:

    char const *protoName = GetProtoName();
    char const *prefName;

    if (!strcmp(protoName, "imap"))
        prefName = "mail.imap.qos";
    else if (!strcmp(protoName, "news"))
        prefName = "news.qos";
    else if (!strcmp(protoName, "smtp"))
        prefName = "mail.smtp.qos";
    else if (!strcmp(protoName, "pop3"))
        prefName = "mail.pop3.qos";
    else
        prefName = nsnull;


now I can do that...  I just think it's a bit ugly.  But if that's what you want...

The whole point of having individual classes is to have "data-hiding"... If we put knowledge of how/where classes store their preferences into the base class, then it defeats the whole purpose of class hierarchy and private data.

Comment 48

[David :Bienvenu]

(In reply to comment #47)
> (In reply to comment #46)
> > (From update of attachment 449315 [details] [diff] [review] [details])
> > sorry, I'm traveling today so I didn't get to describe exactly what I meant. It
> > wouldn't return the name of the pref; it would return the protocol type, e.g.,
> > "smtp", "imap", etc. and the pref name would be constructed from that...
> 
> I understood that part, but that's not enough.
> 
> I would then need to change GetQoSBits do something like:
> 
>     char const *protoName = GetProtoName();
>     char const *prefName;
> 
>     if (!strcmp(protoName, "imap"))
>         prefName = "mail.imap.qos";
>     else if (!strcmp(protoName, "news"))
>         prefName = "news.qos";
>     else if (!strcmp(protoName, "smtp"))
>         prefName = "mail.smtp.qos";
>     else if (!strcmp(protoName, "pop3"))
>         prefName = "mail.pop3.qos";
>     else
>         prefName = nsnull;
> 
> 
> now I can do that...  I just think it's a bit ugly.  But if that's what you
> want...
> 
> The whole point of having individual classes is to have "data-hiding"... If we
> put knowledge of how/where classes store their preferences into the base class,
> then it defeats the whole purpose of class hierarchy and private data.

no, you would construct the pref name as "mail" + protoName + "qos".

Comment 49

[Philip Prindeville]

(In reply to comment #48) 
> no, you would construct the pref name as "mail" + protoName + "qos".

What about nntp?

Comment 50

[David :Bienvenu]

make the pref name mail.news.qos.

Comment 51

[David :Bienvenu]

er, mail.nntp.qos would be better.

Comment 52

[Philip Prindeville]

(In reply to comment #51)
> er, mail.nntp.qos would be better.

ew ew ew...

Comment 53

[Ben Bucksch (:BenB)]

I think all Mozilla QoS prefs should be in the same prefbranch, under e.g. network.qos.<protocol>, e.g. network.qos.imap and network.qos.http. Having them all with different names will make it harder to use.

comment 0:
> Since HTTP and Email are significant users of bandwidth, but don't require
> low-latency, FF and TB are major benefactors to the proper use of bandwidth.

Didn't you say the QoS marking of the client only influences the QoS of the request, but not the response? The requests from clients are typically small (and most of them interactive). It's the responses which may be big (e.g. HTTP downloads or IMAP syncs), but you said we won't influence their QoS anyways. So, FF and TB setting QoS of the requests won't make much difference for the network.

(I think that's a problem in the spec, if what you told me is true.)

Comment 54

[David :Bienvenu]

(In reply to comment #53)
> I think all Mozilla QoS prefs should be in the same prefbranch, under e.g.
> network.qos.<protocol>, e.g. network.qos.imap and network.qos.http. Having them
> all with different names will make it harder to use.
> 
I'm OK with that.

Comment 55

[Philip Prindeville]

(In reply to comment #53)
> I think all Mozilla QoS prefs should be in the same prefbranch, under e.g.
> network.qos.<protocol>, e.g. network.qos.imap and network.qos.http. Having them
> all with different names will make it harder to use.
> 
> comment 0:
> > Since HTTP and Email are significant users of bandwidth, but don't require
> > low-latency, FF and TB are major benefactors to the proper use of bandwidth.
> 
> Didn't you say the QoS marking of the client only influences the QoS of the
> request, but not the response? The requests from clients are typically small
> (and most of them interactive). It's the responses which may be big (e.g. HTTP
> downloads or IMAP syncs), but you said we won't influence their QoS anyways.
> So, FF and TB setting QoS of the requests won't make much difference for the
> network.
> 
> (I think that's a problem in the spec, if what you told me is true.)

Using QoS in one direction only is always a dangerous proposition, especially in a richly connected network such as an MPLS mesh, and more so if ECN is being used simultaneously... because QoS has the ability to cause asymmetrical routing to occur, in which case ECN information is useless at best and possibly even detrimental.

Using QoS in both directions gives a greater possibility of symmetrical paths.

You could ask: why not leave QoS out?  Because you can't assure that it will also be left out on the server side.  A lot of applications are now adding QoS support.  And even ones that don't, might have QoS tagging injected as a packet-munging exercise at the host-level (via iptables) or else at the egress/border router.

Comment 56

[Philip Prindeville]

(In reply to comment #53)
> I think all Mozilla QoS prefs should be in the same prefbranch, under e.g.
> network.qos.<protocol>, e.g. network.qos.imap and network.qos.http. Having them
> all with different names will make it harder to use.

You could also make the argument that having everything that affects the IMAP service under the mail.imap.* tree makes it easier to gather up and identify.

Comment 57

[Philip Prindeville]

Attachment: Alternate mailnews patch as per David's comments.

(In reply to comment #51)
> er, mail.nntp.qos would be better.

Done.

Comment 58

[David :Bienvenu]

Attachment: tweaks to previous patch

thx for the patch. I think it'll be quicker if I just make the last round of changes, which I've attached  - requesting sr from neil.

Comment 59

[Philip Prindeville]

I'm fine with your changes... though I'm not sure I understand why the conversion via nsDependentCString() is required for "protocol".

Comment 60

[David :Bienvenu]

(In reply to comment #59)
> I'm fine with your changes... though I'm not sure I understand why the
> conversion via nsDependentCString() is required for "protocol".

Neil will know for sure...

Comment 61

[Philip Prindeville]

(In reply to comment #60)
> (In reply to comment #59)
> > I'm fine with your changes... though I'm not sure I understand why the
> > conversion via nsDependentCString() is required for "protocol".
> 
> Neil will know for sure...

I'm not doubting it's necessary.

I was just hoping you'd share why for those not "in the know".

Comment 62

[Ben Bucksch (:BenB)]

Philip, check out <https://developer.mozilla.org/En/Mozilla_internal_string_guide>

Comment 63

[[e-mail]]

Comment on attachment 450887 [details] [diff] [review]
tweaks to previous patch

>+  nsCAutoString prefName("mail.");
>+  prefName += nsDependentCString(protocol) + NS_LITERAL_CSTRING(".qos");
As per bug 377319 we're trying to make the code compile against both the internal and external (libxul) APIs. Unfortunately the external API doesn't support the + operator so you'll have to append these strings in two steps. (Then again you won't need the nsDependentCString!)

>+  *aQoSBits = (PRUint8) NS_CLAMP(val, 0, 0xff);
What does NS_CLAMP do for us that a cast to PRUint8 on its own doesn't?

>@@ -879,6 +879,12 @@ nsresult nsImapProtocol::SetupWithUrl(ns
This is still on the main thread at this point right?

Comment 64

[Philip Prindeville]

(In reply to comment #63)
> (From update of attachment 450887 [details] [diff] [review])
> >+  nsCAutoString prefName("mail.");
> >+  prefName += nsDependentCString(protocol) + NS_LITERAL_CSTRING(".qos");
> As per bug 377319 we're trying to make the code compile against both the
> internal and external (libxul) APIs. Unfortunately the external API doesn't
> support the + operator so you'll have to append these strings in two steps.
> (Then again you won't need the nsDependentCString!)

Got it.

> >+  *aQoSBits = (PRUint8) NS_CLAMP(val, 0, 0xff);
> What does NS_CLAMP do for us that a cast to PRUint8 on its own doesn't?

Nothing, except it's more consistent.  There are other places where preferences are clamped to be in a smaller range than the domain (i.e. bit precision) of their containers...  PRInt32's that are constrained to positive values between 0 and N (where N is less than 2**31, etc).

> >@@ -879,6 +879,12 @@ nsresult nsImapProtocol::SetupWithUrl(ns
> This is still on the main thread at this point right?

That I can't answer.  I'll let David field it.

Not sure I understand what difference it ultimately makes, though.

Comment 65

[David :Bienvenu]

(In reply to comment #63)

> As per bug 377319 we're trying to make the code compile against both the
> internal and external (libxul) APIs. Unfortunately the external API doesn't
> support the + operator so you'll have to append these strings in two steps.
> (Then again you won't need the nsDependentCString!)
Ah, ok, thx.
> >@@ -879,6 +879,12 @@ nsresult nsImapProtocol::SetupWithUrl(ns
> This is still on the main thread at this point right?
Yes, this is on the UI thread.

Comment 66

[David :Bienvenu]

Attachment: fix addressing Neil's comment

Comment 67

[Philip Prindeville]

(In reply to comment #66)
> Created an attachment (id=450978) [details]
> fix addressing Neil's comment

That looks strangely familiar.  :-)

Comment 68

[Philip Prindeville]

(In reply to comment #67)
> (In reply to comment #66)
> > Created an attachment (id=450978) [details] [details]
> > fix addressing Neil's comment
> 
> That looks strangely familiar.  :-)

Oh, scratch that.  I was using the flawed += operator.  Not the .append() method.

Comment 69

[[e-mail]]

(In reply to comment #64)
> Not sure I understand what difference it ultimately makes, though.
I can't remember whether preferences are threadsafe, but it doesn't matter.

(In reply to comment #68)
> Oh, scratch that.  I was using the flawed += operator.
Actually it's the + operator that's flawed; += is fine.

Comment 70

[[e-mail]]

(In reply to comment #64)
>(In reply to comment #63)
>>>+  *aQoSBits = (PRUint8) NS_CLAMP(val, 0, 0xff);
>>What does NS_CLAMP do for us that a cast to PRUint8 on its own doesn't?
>Nothing, except it's more consistent.  There are other places where preferences
>are clamped to be in a smaller range than the domain (i.e. bit precision) of
>their containers...  PRInt32's that are constrained to positive values between
>0 and N (where N is less than 2**31, etc).
Yes, but given the name aQoSBits, I was expecting a bitmask constraint.

Comment 71

[Philip Prindeville]

(In reply to comment #70)
> Yes, but given the name aQoSBits, I was expecting a bitmask constraint.

Yeah, I thought about this.  Currently there's a handful of QoS values defined in RFC's 2597, 2598, and 2474 (well short of the 256 possible values).

It's possible although unlikely that during the useful lifetime of this release of Firefox and Thunderbird that additional QoS values may be introduced.  If that's the case, I didn't want the software to not be able to parse new values.

The other temptation was that if I was going to go down that route, then most people would probably prefer symbolic names for the constants, like "EF" and "AF21" rather than their numeric values... but that would be a whole new bit of complexity for what should have been a relatively simple feature.

Comment 72

[Justin Wood (:Callek)]

(In reply to comment #66)
> Created an attachment (id=450978) [details]
> fix addressing Neil's comment

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