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

RESOLVED FIXED in Thunderbird 53.0

Status

Thunderbird
General
RESOLVED FIXED
5 months ago
4 months ago

People

(Reporter: Jorg K (GMT+2), Assigned: Jorg K (GMT+2))

Tracking

Trunk
Thunderbird 53.0

Thunderbird Tracking Flags

(thunderbird52 fixed, thunderbird53 fixed)

Details

Attachments

(1 attachment, 2 obsolete attachments)

(Assignee)

Description

5 months ago
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'
(Assignee)

Comment 1

5 months ago
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.
Flags: needinfo?(valentin.gosu)
(Assignee)

Comment 2

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

Problem from comment #0 persists.
(Assignee)

Comment 3

5 months ago
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?
Assignee: nobody → jorgk
Attachment #8810955 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Flags: needinfo?(valentin.gosu)
Attachment #8810962 - Flags: review?(rkent)
Attachment #8810962 - Flags: review?(mkmelin+mozilla)
(Assignee)

Comment 4

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

Comment 5

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

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

Looks reasonable to me, r=mkmelin
Attachment #8810962 - Flags: review?(rkent)
Attachment #8810962 - Flags: review?(mkmelin+mozilla)
Attachment #8810962 - Flags: review+
(Assignee)

Comment 6

5 months ago
Kent, can you please check the JS Account part of this.
Flags: needinfo?(rkent)
(Assignee)

Comment 7

5 months ago
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 ;-)
Attachment #8810962 - Flags: review?(rkent)
(Assignee)

Comment 8

5 months ago
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 &)]
(Assignee)

Comment 9

5 months ago
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
Attachment #8810962 - Attachment is obsolete: true
Attachment #8810962 - Flags: review?(rkent)
Attachment #8811040 - Flags: review?(rkent)
(Assignee)

Comment 10

5 months ago
This is green now.

Comment 11

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

OK why not, LGTM!
Flags: needinfo?(rkent)
Attachment #8811040 - Flags: review?(rkent) → review+
(Assignee)

Comment 12

5 months ago
https://hg.mozilla.org/comm-central/rev/93c90c76663078286cde1cc0edefbe895938b6c7
One bustage less, one to go ;-(
Status: ASSIGNED → RESOLVED
Last Resolved: 5 months ago
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 53.0

Comment 13

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

Updated

4 months ago
status-thunderbird52: --- → affected
(Assignee)

Comment 14

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

FRG, many thanks for letting me know!
status-thunderbird52: affected → fixed
status-thunderbird53: --- → fixed
You need to log in before you can comment on or make changes to this bug.