Closed Bug 1550945 Opened 5 years ago Closed 5 years ago

Port bug 1536744 to C-C [Make NS_NewURI work off main thread and remove nsIProtocolHandler.newURI]

Categories

(MailNews Core :: General, task)

Product:
MailNews Core
Component:
General
Type:
task
Priority:
Not set
Severity:
normal

Tracking

(Not tracked)

Status:
RESOLVED FIXED
Milestone:
Thunderbird 69.0

People

(Reporter: jorgk-bmo, Assigned: jorgk-bmo)

References

(Blocks 1 open bug)

Details

Attachments

(10 files, 10 obsolete files)

1550945-part1.patch
13.97 KB, patch
benc
: review+
Details | Diff | Splinter Review
1550945-part2.patch
1.57 KB, patch
valentin
: review+
Details | Diff | Splinter Review
1550945-part3.patch
2.69 KB, patch
benc
: review+
Details | Diff | Splinter Review
1550945-part4.patch (v3)
19.78 KB, patch
darktrojan
: review+
Details | Diff | Splinter Review
1550945-part5.patch (v2)
18.75 KB, patch
benc
: review+
Details | Diff | Splinter Review
1550945-part6.patch
1.66 KB, patch
benc
: review+
Details | Diff | Splinter Review
1550945-part7.patch
1.57 KB, patch
jorgk-bmo
: review+
Details | Diff | Splinter Review
1550945-part8.patch
1.25 KB, patch
jorgk-bmo
: review+
Details | Diff | Splinter Review
1550945-part-9.patch
2.52 KB, patch
valentin
: feedback+
Details | Diff | Splinter Review
1550945-part-2-follow-up.patch
1.82 KB, patch
valentin
: review+
Details | Diff | Splinter Review

There are URI changes coming in the 69 cycle, see a summary in bug 1536744 comment #26.

Setting ni? to provide overview of necessary changes.

Flags: needinfo?(valentin.gosu)

As I said in bug 1536744, the best course here would be to add


#if defined(MOZ_THUNDERBIRD) || defined(MOZ_SUITE)
   return NS_NewThunderbirdURI(aURI, aSpec, aCharset, aBaseURI, aIOService);
#endif

to NS_NewURI. This method would deal with instantiating any c-c specific URIs, and any threadsafeness issues that may be relevant.

The implementation would look something like:

  if (scheme.EqualsLiteral("addbook")) {
    return NS_MutateURI(new nsAddbookUrl::Mutator()).SetSpec(aSpec).Finalize(aURI);
  }

Basically we move the implementation of nsIProtocolHandler.newURI to NS_NewThunderbirdURI
This is only possible when we can create the URI thread-safely, without depending on the protocol handler.

When creating a URI is tightly tied to the protocol handler (uses member variables, etc), we instantiate the protocol handler, and call a newURI method on the protocol handler (has thhe same signature like the current newURI, but is defined in each class, not in nsIProtocolHandler). If the protocol handler can't be made threadsafe (ResProtocolHander was made threadsafe by adding locks to SubstitutingProtocolHandler, ExtensionProtocolHandler is still main-thread only), we return NS_ERROR_UNKNOWN_PROTOCOL when called off-main-thread.

Also, I think we can remove some of the implementations.
nsCidProtocolHandler.cpp doesn't really seem to be used for anything.
Instead of nsAddbookUrl, we can probably just use an nsSimpleURI and instead of calling the GetAddbookOperation method, you just check the query for ?action=print or ?action=add

https://searchfox.org/comm-central/rev/68306e54cddea95b96f572cae989d3a21afa1daf/mailnews/compose/src/nsSMTPProtocolHandler.js#23

Also the impl of these protocols' newURI needs to be moved to NS_NewThunderbirdURI

https://searchfox.org/comm-central/source/calendar/base/src/calProtocolHandler.js#22
https://searchfox.org/comm-central/source/calendar/lightning/components/calItipProtocolHandler.js#59
https://searchfox.org/comm-central/source/chat/components/src/smileProtocolHandler.js#26
https://searchfox.org/comm-central/source/ldap/xpcom/src/nsLDAPProtocolHandler.js#21
https://searchfox.org/comm-central/source/mailnews/compose/src/nsSMTPProtocolHandler.js#23
https://searchfox.org/comm-central/source/suite/components/nsGopherProtocolStubHandler.js#37

Flags: needinfo?(valentin.gosu)

Thanks for the comment. The CID protocol handler is used where a message part referenced another, typically a HTML part references an image like so:

  <body text="#000000" bgcolor="#FFFFFF">
    <p><img src="cid:part1.A3CEFC6E.2595EFAB@jorgk.com" alt=""></p>
  </body>

Before we dig into it further, I'd like to understand the following: Why do the protocol handlers create the URI? Why is that concept harmful and being discontinued? And if you discontinue it, how will the various M-C URIs compensate. I guess the answer is in the various patches of bug 1536744.

Looking at the C++ NewURI() implementations we have, the address book, CID and Mailto: functions don't do anything interesting. The story is very different for mailbox:, imap:, nntp: and pop3:. I guess that answers the first question.

In fact, as you pointed out at the end of comment #2, there are also calendar, chat as well as LDAP and SMTP protocol handler written in JS. Let's disregard SeaMonkey's Gopher.

Finally it would be good to have the 20+ patches in bug 1536744 in one for easy application.

(In reply to Jorg K (GMT+2) from comment #3)

Before we dig into it further, I'd like to understand the following: Why do the protocol handlers create the URI? Why is that concept harmful and being discontinued? And if you discontinue it, how will the various M-C URIs compensate. I guess the answer is in the various patches of bug 1536744.

So, historically, nsIURI has been so flexible, that you could have JS implementations of it, and those implementations did more than just represent a nsIURI; for example mailbox, pop3 (or blob for that matter) add a lot of extra logic to the creation of a URI, sometimes keeping track of the created URIs, other times attaching extra info to the object which can be QI'd to other interfaces and used at a later time.

But now we have a URL spec. And we'd like to reduce the number of URL parsers in Gecko as much as possible (to 1, ideally. And using rust-url as the only implementation is a long time goal of ours). This means centralizing the URL parsing, and making it thread safe.
So nsIProtocolHandler shouldn't be able to add any other nsIURI implementations. Also, all of the information held by a URI should be in a spec - and the rest of the info currently tied with the URIs should be put in a hashtable<nsIURI, extraInfo>

Attached patch all.diff (obsolete) — — Splinter Review

This is a diff containing all of the changesets as they currently are.
I expect them to change a bit during review, but the functionality should be more or less the same.

Assignee: nobody → valentin.gosu
Assignee: valentin.gosu → nobody
Attached patch backout-22-changesets (obsolete) — — Splinter Review

Here's a backout of the 22 patches of bug 1536744 if someone wants to build locally.

Attachment #9064333 - Attachment is obsolete: true

This fixes the compile bustage, which is about 10% of the work. More coming.

Assignee: nobody → jorgk
Status: NEW → ASSIGNED
Keywords: leave-open
Target Milestone: --- → Thunderbird 69.0
Version: 60 → Trunk

With part 1, TB starts and displays messages, even IMAP. I tried opening an attachment and it crashes straight away. Let's see what the tests say.

Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/0326ae4cf85d
Part 1: nsIProtocolHandler::NewURI() no longer exists. rs=bustage-fix

I guess we will need this in some way, shape or form. I put the hunk towards the end of the function, so it can handle all the M-C URLs first. I wasn't sure whether to place it before or after the if (aBaseURI) { block. Comments?

Attachment #9068292 - Flags: review?(valentin.gosu)
Comment on attachment 9068292 [details] [diff] [review]
1550945-part2.patch

Review of attachment 9068292 [details] [diff] [review]:
-----------------------------------------------------------------

I think this is the right place for it. Thanks!
Attachment #9068292 - Flags: review?(valentin.gosu) → review+

OK, here's the C-C counterpart. Needs to be fleshed out, but compiles so far.

Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/mozilla-central/rev/537b6d1915e8
Part 2, M-C part: Hook into nsNetUtil::NS_NewURI(). r=valentin a=aryx,npotb DONTBUILD
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/09d4067eb5ff
Introduce NS_NewMailnewsURI. rs=bustage-fix CLOSED TREE DONTBUILD
Attached patch 1550945-part4.patch (obsolete) — — Splinter Review

This is part 4, it's very ugly, it hard-codes all the schemes we have. Let's see:

https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=85eaa0199f2b038392fe59f8c337c702e3ab5792

Attached patch 1550945-part4.patch (v2) (obsolete) — — Splinter Review

The previous run wasn't a total success, so let's try again. This time based on M-C rev 537b6d1915e8d78f1da3112f0579d6101b6abda8 to rule out further bustage from the last merge.

https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=a91fe5ab7ea4cb2253f5220ae92a1e95f3339fd5

Attachment #9068353 - Attachment is obsolete: true

This should mostly work, some Xpcshell failures. I'll disable a few URL related MozMill tests.

Attachment #9068389 - Attachment is obsolete: true
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/21ef273cf681
Part 4: Flesh out NS_NewMailnewsURI(). rs=bustage-fix
https://hg.mozilla.org/comm-central/rev/b2e2cb73dfce
Disable 7 Mozmill tests. rs=bustage-fix
Comment on attachment 9068464 [details] [diff] [review]
1550945-part4.patch (v3)

Geoff, can you check the calendar hunks, well, only removals, and the calendar schemes in nsNewMailnewsURI.cpp. BTW, comm/calendar/test/unit/test_webcal.js seems to pass.
Attachment #9068464 - Flags: review?(geoff)
Attachment #9068286 - Flags: review?(benc)
Attachment #9068301 - Flags: review?(benc)
Attached patch 1550945-part5.patch (obsolete) — — Splinter Review

This fixes the failing news, imap, smtp and mailbox Xpcshell test failures. The SMTP "new URI" was incorrectly ported from the JS protocol handler and needed to be aligned with the other "URI creations".

LDAP will most likely need the same treatment since its "new URI" was originally also in JS.

Let's see:
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=4c63488cce01a1ca3cda8e74e2a9e531efdb2c4b

Attachment #9068517 - Flags: review?(benc)
Attachment #9068186 - Attachment is obsolete: true

This needed another tweak to get news tests to pass. Now we're left with three failing tests:
TEST-UNEXPECTED-FAIL | comm/ldap/xpcom/tests/unit/test_nsLDAPURL.js | xpcshell return code: 0
TEST-UNEXPECTED-FAIL | comm/mailnews/addrbook/test/unit/test_ldap1.js | xpcshell return code: 0
TEST-UNEXPECTED-FAIL | comm/mailnews/addrbook/test/unit/test_ldap2.js | xpcshell return code: 0
The LDAP stuff will need a tweak, see comment #21.

Attachment #9068517 - Attachment is obsolete: true
Attachment #9068517 - Flags: review?(benc)
Attachment #9068539 - Flags: review?(benc)
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/5c0f43def1f3
Part 5: Fix tests so they don't create URL via protocol handler, rework SMTP URL creation. rs=bustage-fix
Attachment #9068286 - Flags: review?(benc) → review+
Attachment #9068301 - Flags: review?(benc) → review+
Comment on attachment 9068539 [details] [diff] [review]
1550945-part5.patch (v2)

Review of attachment 9068539 [details] [diff] [review]:
-----------------------------------------------------------------

All looks sane to me.

::: mailnews/compose/src/nsSmtpService.cpp
@@ +279,5 @@
> +nsresult nsSmtpService::NewSmtpURI(const nsACString &aSpec,
> +                                   const char *aOriginCharset, nsIURI *aBaseURI,
> +                                   nsIURI **_retval) {
> +  NS_ENSURE_ARG_POINTER(_retval);
> +  *_retval = 0;

nit: should be nullptr?
Attachment #9068539 - Flags: review?(benc) → review+

Somehow the mutator approach doesn't appear to work for LDAP, so fall back to the "classic" approach of instantiating the class. That makes the remaining Xpcshell tests pass leaving the 7 disabled Mozmill tests to be investigated.

I intend to file a bug to clean this up a bit.

Attachment #9068552 - Flags: review?(benc)
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/25cf997ee812
Part 6: Fix LDAP tests. rs=bustage-fix
Attachment #9068552 - Flags: review?(benc) → review+
Comment on attachment 9068286 [details] [diff] [review]
1550945-part1.patch

Review of attachment 9068286 [details] [diff] [review]:
-----------------------------------------------------------------

::: mailnews/local/src/nsMailboxService.cpp
@@ +542,5 @@
>          do_GetService(NS_POP3SERVICE_CONTRACTID1, &rv);
>      if (NS_SUCCEEDED(rv)) {
>        nsCOMPtr<nsIURI> pop3Uri;
>  
> +      rv = NewURI(spec, "" /* ignored */, aURI, getter_AddRefs(pop3Uri));

Shouldn't it be `nsPop3Service::NewURI`? (although It was not possible at the time of part 1, part 4 made the method `static`.)

Hmm, compiled without it, but I can add it. EDIT: Oops, now I see the issue.

Attachment #9068625 - Flags: review+
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/aff89fb3045e
Part 7: Correction of nsMailboxService::NewChannel(), hint by :emk. r=me

So now we have to worry about the three failing Mozmill tests:

mozmake SOLO_TEST=composition/test-image-display.js mozmill-one
EXCEPTION: Loaded image cidImage is of zero size
So and embedded image via CID URL isn't displayed. One can see that running the test manually. Strange.

mozmake SOLO_TEST=message-window/test-vcard-actions.js mozmill-one
Displays a message with embedded vCard. Clicking onto the vCard link, an address book URL does nothing, so the expected window doesn't show up, hence: EXCEPTION: Timeout waiting for modal dialog to open.

mozmake SOLO_TEST=content-policy/test-compose-mailto.js mozmill-one
Displays some mailto: links. Clicking on them does nothing, so the expected window doesn't show up, hence: EXCEPTION: Timeout waiting for modal dialog to open.

So why can't those links be clicked now? Valentin?

Flags: needinfo?(valentin.gosu)

Probably unrelated with the failing Mozmill tests, but I found that the current NS_NewMailnewsURI does not handle the "pop" (not "pop3") scheme:
https://searchfox.org/comm-central/rev/2ddfe0d5bd910e7e7bdbdebb5b826b4863dfd880/mailnews/local/public/nsMsgLocalCID.h#131

FYI All scheme names in the tree:
https://searchfox.org/comm-central/search?q=%2Fprotocol%3B1
https://searchfox.org/comm-central/search?q=NS_NETWORK_PROTOCOL_CONTRACTID_PREFIX

Thanks, I'll add "pop" although I can't see that this is used.

Attachment #9068638 - Flags: review+
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/bfac42da27a2
Part 8: Correction of pop handling, hint by :emk. r=me
Comment on attachment 9068464 [details] [diff] [review]
1550945-part4.patch (v3)

According to the links that Masatoshi-san posted in comment #33, `moz-cal-handle-itip` and `webcal[s]` are correct, so we might as well move the review to Ben.
Attachment #9068464 - Flags: review?(geoff) → review?(benc)
Comment on attachment 9068464 [details] [diff] [review]
1550945-part4.patch (v3)

Review of attachment 9068464 [details] [diff] [review]:
-----------------------------------------------------------------

Seems okay to me.
Attachment #9068464 - Flags: review?(benc) → review+

Oh, so that's what Bugzilla was complaining about. Ben can review it if he likes, I don't know why you didn't just ask him in the first place.

Because I wasn't sure about the Calendar schemes. Anyway, since Calendar is involved, it's better that a Calendar peer(?) looked at this. Thanks!

Blocks: 1555633

Further to comment #32: You don't need to run a test. Simply write an e-mail, paste some bitmap image into it and insert a mailto: link. Save the draft. Result: No picture shown and nothing happens when mailto: link is clicked. So somehow our URLs are, well, "poisoned" so that the Mozilla platform doesn't seem to follow them any more. Valentin? Do we need another hook somewhere?

Maybe you will have to replicate this logic?
https://searchfox.org/mozilla-central/rev/7556a400affa9eb99e522d2d17c40689fa23a729/netwerk/base/nsNetUtil.cpp#1720-1736
Currently NS_NewMailnewsURI just fails if net_ExtractURLScheme fails.

Thanks Masatoshi-san, sadly it doesn't make a difference. Valentin, do we need this code?

Attachment #9068905 - Flags: feedback?(valentin.gosu)
Comment on attachment 9068905 [details] [diff] [review]
1550945-part-9.patch - NECESSARY?

Review of attachment 9068905 [details] [diff] [review]:
-----------------------------------------------------------------

::: mailnews/base/util/nsNewMailnewsURI.cpp
@@ +31,5 @@
> +
> +    if (!aSpec.IsEmpty() && aSpec[0] == '#') {
> +      // Looks like a reference instead of a fully-specified URI.
> +      // --> initialize |uri| as a clone of |aBaseURI|, with ref appended.
> +      return NS_GetURIWithNewRef(aBaseURI, aSpec, aURI);

This part is not really needed. We handle this in NS_NewURI already, we wel'll probably never enter this if condition.

@@ +35,5 @@
> +      return NS_GetURIWithNewRef(aBaseURI, aSpec, aURI);
> +    }
> +
> +    rv = aBaseURI->GetScheme(scheme);
> +    if (NS_FAILED(rv)) return rv;

I think this part *is* needed. If aSpec doesn't have a scheme, we're looking for the handler of its base URI.
Attachment #9068905 - Flags: feedback?(valentin.gosu)

(In reply to Jorg K (GMT+2) from comment #32)

So now we have to worry about the three failing Mozmill tests:

mozmake SOLO_TEST=composition/test-image-display.js mozmill-one
EXCEPTION: Loaded image cidImage is of zero size
So and embedded image via CID URL isn't displayed.
mozmake SOLO_TEST=message-window/test-vcard-actions.js mozmill-one
Displays a message with embedded vCard. Clicking onto the vCard link, an address book URL does nothing,
mozmake SOLO_TEST=content-policy/test-compose-mailto.js mozmill-one
Displays some mailto: links. Clicking on them does nothing,

So why can't those links be clicked now? Valentin?

Normally if parsing the URL fails, you can't click it (and you don't see a mouse-over popup for it).
So I assume it just fails to parse? You can add some printfs to NS_NewMailnewsURI in order to figure out how they fail.

printf("spec:%s base:%s\n", aSpec.BeginReading(), aBase ? aBase->GetSpecOrDefault().get() : "");

Then you can probably use that input in an xpcshell-test to make it easier to fix.

Flags: needinfo?(valentin.gosu)
Attached patch 1550945-part-9.patch (obsolete) — — Splinter Review

Like this then? The question remains, why can't our URL's be clicked?

Attachment #9068905 - Attachment is obsolete: true
Attachment #9068911 - Flags: feedback?(valentin.gosu)
Attachment #9068911 - Flags: feedback?(valentin.gosu) → feedback+

Thanks, Valentin, hovering those links works fine. The debug gives: spec:mailto:jorgk@jorgk.com base:

However, when clicking the mailto: link I get:

spec:mailbox:///C:/Users/jorgk/AppData/Roaming/Thunderbird/Profiles/testing.testing/Mail/Local%20Folders/Drafts?number=137 base:
spec:mailto:jorgk@jorgk.com base:
[2944, Main Thread] WARNING: NS_ENSURE_SUCCESS(rv, rv) failed with result 0x804B000A: file c:/mozilla-source/comm-central/toolkit/components/url-classifier/nsUrlClassifierDBService.cpp, line 1801
spec:mailbox:///C:/Users/jorgk/AppData/Roaming/Thunderbird/Profiles/testing.testing/Mail/Local%20Folders/Drafts?number=137 base:
[2944, Main Thread] WARNING: NS_ENSURE_SUCCESS(rv, rv) failed with result 0x80004005: file c:/mozilla-source/comm-central/uriloader/base/nsURILoader.cpp, line 308

Looks like it fails here:
https://searchfox.org/mozilla-central/rev/7556a400affa9eb99e522d2d17c40689fa23a729/toolkit/components/url-classifier/nsUrlClassifierDBService.cpp#1800
and here
https://searchfox.org/mozilla-central/rev/7556a400affa9eb99e522d2d17c40689fa23a729/toolkit/components/url-classifier/nsUrlClassifierUtils.cpp#260

0x804B000A is NS_ERROR_MALFORMED_URI.

So the issue isn't that there is something wrong with the link, somehow the process of loading fails in the URI loader. As we know, mailbox URLs don't have a host. But even moving the message to an IMAP folder when IMAP URLs have a host gives:

spec:mailto:jorgk@jorgk.com base:
spec:imap://jorgk%40jorgk%2Ecom@mail.your-server.de:143/fetch%3EUID%3E.INBOX.Drafts%3E346 base:
[2944, Main Thread] WARNING: NS_ENSURE_SUCCESS(rv, rv) failed with result 0x804B000A: file c:/mozilla-source/comm-central/toolkit/components/url-classifier/nsUrlClassifierDBService.cpp, line 1801
spec:imap://jorgk%40jorgk%2Ecom@mail.your-server.de:143/fetch%3EUID%3E.INBOX.Drafts%3E346 base:
[2944, Main Thread] WARNING: NS_ENSURE_SUCCESS(rv, rv) failed with result 0x8000
4005: file c:/mozilla-source/comm-central/uriloader/base/nsURILoader.cpp, line 308

I'll be afk until after lunch now.

Ah, OK, I think I have it. The problem is in attachment 9068292 [details] [diff] [review]
The call should come before the if (aBaseURI) block, otherwise we never get to create the thunderbird URI.
Sorry I didn't catch this in the review.

Attached patch 1550945-part-2-follow-up.patch (obsolete) — — Splinter Review

Thanks, Valentin, that makes the tests pass. Now I'll really be afk.

Attachment #9068928 - Flags: review?(valentin.gosu)
Attachment #9068928 - Flags: review?(valentin.gosu) → review+
Comment on attachment 9068928 [details] [diff] [review]
1550945-part-2-follow-up.patch

Review of attachment 9068928 [details] [diff] [review]:
-----------------------------------------------------------------

::: netwerk/base/nsNetUtil.cpp
@@ +1866,5 @@
>          .Finalize(aURI);
>    }
>  
> +#if defined(MOZ_THUNDERBIRD) || defined(MOZ_SUITE)
> +  return NS_NewMailnewsURI(aURI, aSpec, aCharset, aBaseURI, aIOService);

Note that now unknown schemes might fail to parse in t-b. Not sure if you care about that.
You either need to have the code below in NS_NewMailnewsURI, or alternatively (preferred), if NS_NewMailnewsURI is called for any scheme it is not aware of, it should return NS_ERROR_UNKNOWN_PROTOCOL, and NS_NewURI should check for that error code and continue with the code below.
Attached patch 1550945-part-2-follow-up.patch (obsolete) — — Splinter Review

Yes, I thought the same while my afk activity fell through.

Attachment #9068911 - Attachment is obsolete: true
Attachment #9068928 - Attachment is obsolete: true
Attachment #9068932 - Flags: review?(valentin.gosu)

OK, I'm now returning an error if we don't handle it.

Attachment #9068933 - Flags: feedback?(valentin.gosu)
Comment on attachment 9068932 [details] [diff] [review]
1550945-part-2-follow-up.patch

Review of attachment 9068932 [details] [diff] [review]:
-----------------------------------------------------------------

::: netwerk/base/nsNetUtil.cpp
@@ +1867,5 @@
>    }
>  
> +#if defined(MOZ_THUNDERBIRD) || defined(MOZ_SUITE)
> +  rv = NS_NewMailnewsURI(aURI, aSpec, aCharset, aBaseURI, aIOService);
> +  if (NS_SUCCEEDED(rv)) return rv;

This should be 
```C++
if (rv != NS_UNKNOWN_PROTOCOL) {
  return rv;
}
```

And make sure to return NS_UNKNOWN_PROTOCOL at the end of NS_NewMailnewsURI
Attachment #9068932 - Flags: review?(valentin.gosu)
Attachment #9068933 - Flags: feedback?(valentin.gosu) → feedback+

OK :-)

Attachment #9068932 - Attachment is obsolete: true
Attachment #9068936 - Flags: review?(valentin.gosu)
Comment on attachment 9068936 [details] [diff] [review]
1550945-part-2-follow-up.patch

Review of attachment 9068936 [details] [diff] [review]:
-----------------------------------------------------------------

::: netwerk/base/nsNetUtil.cpp
@@ +1867,5 @@
>    }
>  
> +#if defined(MOZ_THUNDERBIRD) || defined(MOZ_SUITE)
> +  rv = NS_NewMailnewsURI(aURI, aSpec, aCharset, aBaseURI, aIOService);
> +  if (rv != NS_ERROR_UNKNOWN_PROTOCOL) return rv;

nit: I prefer it with the { } around return and newline (see prev comment)
Attachment #9068936 - Flags: review?(valentin.gosu) → review+

Thanks, Valentin. Will land with the added braces. I think we're done here now. Follow-up in bug 1555633.

Keywords: leave-open
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/mozilla-central/rev/909f78f4ebae
Follow-up to part 2, M-C: Move hook in nsNetUtil::NS_NewURI(). r=valentin a=Aryx DONTBUILD NPOTB

Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/538ca411c73d
Part 9: Handle scheme extraction failure, hint by :emk. Return error on unknown scheme. f=valentin, r=me
https://hg.mozilla.org/comm-central/rev/f70bf49b71f2
Backed out changeset b2e2cb73dfce to re-enable tests. a=backout

Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Blocks: 1560639
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: