Closed
Bug 1317724
Opened 4 years ago
Closed 4 years ago
Port |Bug 1310483 - Implement nsIURIWithQuery for having query part in simple URI| to Mailnews
Categories
(Thunderbird :: General, defect)
Thunderbird
General
Tracking
(thunderbird52 fixed, thunderbird53 fixed)
RESOLVED
FIXED
Thunderbird 53.0
People
(Reporter: jorgk-bmo, Assigned: jorgk-bmo)
Details
Attachments
(1 file, 2 obsolete files)
4.96 KB,
patch
|
rkent
:
review+
|
Details | Diff | Splinter Review |
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•4 years 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•4 years ago
|
||
Problem from comment #0 persists.
Assignee | ||
Comment 3•4 years ago
|
||
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•4 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=a0c3a43330721450d20ecb65b5430efeb73581aa
Comment 5•4 years 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•4 years ago
|
||
Kent, can you please check the JS Account part of this.
Flags: needinfo?(rkent)
Assignee | ||
Comment 7•4 years 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•4 years 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•4 years ago
|
||
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•4 years ago
|
||
This is green now.
Comment 11•4 years 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•4 years ago
|
||
https://hg.mozilla.org/comm-central/rev/93c90c76663078286cde1cc0edefbe895938b6c7 One bustage less, one to go ;-(
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 53.0
![]() |
||
Comment 13•4 years ago
|
||
Bug 1310483 has been uplifted to m-a today so this patch is needed there too.
![]() |
||
Updated•4 years ago
|
status-thunderbird52:
--- → affected
Assignee | ||
Comment 14•4 years ago
|
||
Aurora (TB 52): https://hg.mozilla.org/releases/comm-aurora/rev/0afece51de5b1726c7544144699b143826c26754 FRG, many thanks for letting me know!
You need to log in
before you can comment on or make changes to this bug.
Description
•