1317724 - Port |Bug 1310483 - Implement nsIURIWithQuery for having query part in simple URI| to Mailnews

Status: RESOLVED FIXED

Description

[Jorg K (GMT+2)]

We see:

24:22.61 c:/mozilla-source/comm-central/mailnews/jsaccount/src/JaUrl.cpp(139): error C2259: 'mozilla::mailnews::JaCppUrlDelegator::Super': cannot instantiate abstract class
24:22.61 c:/mozilla-source/comm-central/mailnews/jsaccount/src/JaUrl.cpp(139): note: due to following members:
24:22.61 c:/mozilla-source/comm-central/mailnews/jsaccount/src/JaUrl.cpp(139): note: 'nsresult nsIURIWithQuery::GetFilePath(nsACString_internal &)': is abstract
24:22.61 c:\mozilla-source\comm-central\obj-x86_64-pc-mingw32\dist\include\nsIURIWithQuery.h(31): note: see declaration of 'nsIURIWithQuery::GetFilePath'
24:22.62 c:/mozilla-source/comm-central/mailnews/jsaccount/src/JaUrl.cpp(139): note: 'nsresult nsIURIWithQuery::SetFilePath(const nsACString_internal &)': is abstract
24:22.62 c:\mozilla-source\comm-central\obj-x86_64-pc-mingw32\dist\include\nsIURIWithQuery.h(32): note: see declaration of 'nsIURIWithQuery::SetFilePath'
24:22.62 c:/mozilla-source/comm-central/mailnews/jsaccount/src/JaUrl.cpp(139): note: 'nsresult nsIURIWithQuery::GetQuery(nsACString_internal &)': is abstract
24:22.62 c:\mozilla-source\comm-central\obj-x86_64-pc-mingw32\dist\include\nsIURIWithQuery.h(35): note: see declaration of 'nsIURIWithQuery::GetQuery'
24:22.63 c:/mozilla-source/comm-central/mailnews/jsaccount/src/JaUrl.cpp(139): note: 'nsresult nsIURIWithQuery::SetQuery(const nsACString_internal &)': is abstract
24:22.63 c:\mozilla-source\comm-central\obj-x86_64-pc-mingw32\dist\include\nsIURIWithQuery.h(36): note: see declaration of 'nsIURIWithQuery::SetQuery'

Comment 1

[Jorg K (GMT+2)]

Look like I didn't report all errors, here are more:

 0:56.34 nsMsgMailNewsUrl.cpp
 0:56.34 c:/mozilla-source/comm-central/mailnews/base/util/nsMsgMailNewsUrl.cpp(651): error C2509: 'GetQuery': member function not declared in 'nsMsgMailNewsUrl'
 0:56.34 c:\mozilla-source\comm-central\mailnews\base\util\nsMsgMailNewsUrl.h(39): note: see declaration of 'nsMsgMailNewsUrl'
 0:56.35 c:/mozilla-source/comm-central/mailnews/base/util/nsMsgMailNewsUrl.cpp(656): error C2509: 'SetQuery': member function not declared in 'nsMsgMailNewsUrl'
 0:56.35 c:\mozilla-source\comm-central\mailnews\base\util\nsMsgMailNewsUrl.h(39): note: see declaration of 'nsMsgMailNewsUrl'
 0:56.36 c:/mozilla-source/comm-central/mailnews/base/util/nsMsgMailNewsUrl.cpp(671): error C2509: 'GetFilePath': member function not declared in 'nsMsgMailNewsUrl'
 0:56.36 c:\mozilla-source\comm-central\mailnews\base\util\nsMsgMailNewsUrl.h(39): note: see declaration of 'nsMsgMailNewsUrl'
 0:56.37 c:/mozilla-source/comm-central/mailnews/base/util/nsMsgMailNewsUrl.cpp(676): error C2509: 'SetFilePath': member function not declared in 'nsMsgMailNewsUrl'
 0:56.37 c:\mozilla-source\comm-central\mailnews\base\util\nsMsgMailNewsUrl.h(39): note: see declaration of 'nsMsgMailNewsUrl'

That's somewhat puzzling. nsIURL got refactored a bit
https://hg.mozilla.org/mozilla-central/rev/aef4a0fdca31#l8.27
but nsIMsgMailNewsUrl is a nsIURL so I don't see why that's affected.

nsMsgMailNewsUrl::GetQuery() still exists and it should.

Valentin can you please give me a hint. I've been looking at this for a while but I don't see it.

Comment 2

[Jorg K (GMT+2)]

Created attachment 8810955 [details] [diff] [review]
1317724.patch - WIP: This fixes at least the errors in comment #1

Problem from comment #0 persists.

Comment 3

[Jorg K (GMT+2)]

Created attachment 8810962 [details] [diff] [review]
1317724.patch

This compiles now. Sadly Kent is on his "grandchild day" today, so I need another reviewer. Magnus, do you understand this C++ magic?

Comment 4

[Jorg K (GMT+2)]

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

Comment 5

[Magnus Melin]

Comment on attachment 8810962 [details] [diff] [review]
1317724.patch

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

Looks reasonable to me, r=mkmelin

Comment 6

[Jorg K (GMT+2)]

Kent, can you please check the JS Account part of this.

Comment 7

[Jorg K (GMT+2)]

Comment on attachment 8810962 [details] [diff] [review]
1317724.patch

Thanks for the review. I'm putting Kent back to look at the Js Account stuff. Maybe he can spend five minutes educating us ;-)

Comment 8

[Jorg K (GMT+2)]

While this compiles and mostly works, it crashes in JS Account tests:
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=a0c3a43330721450d20ecb65b5430efeb73581aa

TEST-UNEXPECTED-FAIL | mailnews/jsaccount/test/unit/test_fooUrl.js | xpcshell return code: 1
PROCESS | 6828 | Assertion failure: mRawPtr != nullptr (You can't dereference a NULL nsCOMPtr with operator->().), at c:\\builds\\moz2_slave\\tb-try-c-cen-w32-d-00000000000\\build\\objdir-tb\\dist\\include\\nsCOMPtr.h:769
PROCESS-CRASH | mailnews/jsaccount/test/unit/test_fooUrl.js | application crashed [@ nsCOMPtr<nsIURIWithQuery>::operator->()]

or

TEST-UNEXPECTED-FAIL | mailnews/jsaccount/test/unit/test_fooUrl.js | xpcshell return code: 1
PROCESS-CRASH | mailnews/jsaccount/test/unit/test_fooUrl.js | application crashed [@ mozilla::mailnews::JaCppUrlDelegator::GetFilePath(nsACString_internal &)]

Comment 9

[Jorg K (GMT+2)]

Created attachment 8811040 [details] [diff] [review]
1317724.patch (v2)

OK, here I've done some more copy/paste ;-)

See whether this works better:
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=dac65fb48ad4fafcc5852cf9b919688522f29b17

Comment 10

[Jorg K (GMT+2)]

This is green now.

Comment 11

[Kent James (:rkent)]

Comment on attachment 8811040 [details] [diff] [review]
1317724.patch (v2)

OK why not, LGTM!

Comment 12

[Jorg K (GMT+2)]

https://hg.mozilla.org/comm-central/rev/93c90c76663078286cde1cc0edefbe895938b6c7
One bustage less, one to go ;-(

Comment 13

[Frank-Rainer Grahl (:frg)]

Bug 1310483 has been uplifted to m-a today so this patch is needed there too.

Comment 14

[Jorg K (GMT+2)]

Aurora (TB 52):
https://hg.mozilla.org/releases/comm-aurora/rev/0afece51de5b1726c7544144699b143826c26754

FRG, many thanks for letting me know!