Port bug 1323683 [Fold nsIURIWithQuery into nsIURI to mailnews]

RESOLVED FIXED in Thunderbird 53.0

Status

MailNews Core
Composition
RESOLVED FIXED
a year ago
a year ago

People

(Reporter: Paenglab, Assigned: frg)

Tracking

Thunderbird 53.0
Dependency tree / graph

Thunderbird Tracking Flags

(thunderbird53 fixed, seamonkey2.50 fixed)

Details

Attachments

(2 attachments, 2 obsolete attachments)

(Reporter)

Description

a year ago
Build errors like this:

12:07.02 z:/Mozilla/TB/dist/include\nsMsgMailNewsUrl.h(48): error C2144: syntax error: 'nsresult' should be preceded by ';'
12:07.02 z:/Mozilla/TB/dist/include\nsMsgMailNewsUrl.h(48): error C4430: missing type specifier - int assumed. Note: C++ does not support default-int
12:07.03 z:\mozilla\comm-central\mailnews\jsaccount\src\JaUrl.h(70): error C2059: syntax error: '('
12:07.03 z:\mozilla\comm-central\mailnews\jsaccount\src\JaUrl.h(70): error C2059: syntax error: ')'
12:07.03 z:\mozilla\comm-central\mailnews\jsaccount\src\JaUrl.h(70): error C2143: syntax error: missing ')' before '->'
12:07.03 z:\mozilla\comm-central\mailnews\jsaccount\src\JaUrl.h(70): error C3613: missing return type after '->' ('int' assumed)
12:07.03 z:\mozilla\comm-central\mailnews\jsaccount\src\JaUrl.h(70): error C2143: syntax error: missing ';' before ')'
12:07.03 z:\mozilla\comm-central\mailnews\jsaccount\src\JaUrl.h(70): error C3551: if a trailing return type is used then the leading return type shall be the single type-specifier 'auto' (not 'int')

Back out of bug 1323683 let's build properly.
(Reporter)

Comment 1

a year ago
Created attachment 8822717 [details] [diff] [review]
WIP patch

This patch removes the nsIURIWithQuery and mJsIURIWithQuery but I still get this errors:

0:14.37 JaUrl.cpp
 0:14.37 z:/Mozilla/TB/dist/include\nsMsgMailNewsUrl.h(48): error C2144: syntax error: 'nsresult' should be preceded by ';'
 0:14.38 z:/Mozilla/TB/dist/include\nsMsgMailNewsUrl.h(48): error C4430: missing type specifier - int assumed. Note: C++ does not support default-int

My C++ knowledge is like zero and I don't know how this errors could be resolved.
(Assignee)

Comment 2

a year ago
Remove NS_DECL_NSIURIWITHQUERY from /mailnews/base/util/nsMsgMailNewsUrl.h

Then you get a link error because the methods must be implemented.

>> My C++ knowledge is like zero and I don't know how this errors could be resolved.
Me too :)

Comment 3

a year ago
Sorry, out for dinner, I'll fix it tonight. Compiling but not linking is already a step forward ;-)
(Assignee)

Comment 4

a year ago
Created attachment 8822733 [details] [diff] [review]
1326433-wip2.patch

This one also links and didn't crash SeaMonkey.

Comment 5

a year ago
Comment on attachment 8822733 [details] [diff] [review]
1326433-wip2.patch

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

Please supply a commit message:
Bug 1326433 - Port bug 1323683 to mailnews: Remove nsIURIWithQuery and implement {Get|Set}FilePath and {Get|Set}Query. r=jorgk

I assume the implemented methods were the ones that prevented linking.
Attachment #8822733 - Flags: review+

Comment 6

a year ago
Comment on attachment 8822717 [details] [diff] [review]
WIP patch

Thanks, nice start!
Attachment #8822717 - Attachment is obsolete: true
(Assignee)

Comment 7

a year ago
>> I assume the implemented methods were the ones that prevented linking.

Yes. Are the added attributes from netwerk/base/nsIURI.idl.

>> Please supply a commit message

You sure this is ok so?

FRG
(Assignee)

Comment 8

a year ago
Created attachment 8822758 [details] [diff] [review]
1326433-nsIURIWithQuery.patch

Basically the WIP2 fix but I move some methods a little higher. Please check if none of them really needs an own implementation in mailnews.
Attachment #8822733 - Attachment is obsolete: true
Attachment #8822758 - Flags: review?(jorgk)
(Assignee)

Comment 9

a year ago
Created attachment 8822759 [details] [diff] [review]
1326433-whitespace-fix.patch

Additional whitespace fixes on top of the patch. Removed trailing blanks and tabs.
Assignee: nobody → frgrahl
Status: NEW → ASSIGNED
Attachment #8822759 - Flags: review?(jorgk)

Comment 10

a year ago
Comment on attachment 8822758 [details] [diff] [review]
1326433-nsIURIWithQuery.patch

These need to be defined in the base classes as you have done, see for example:
https://dxr.mozilla.org/comm-central/search?q=etHostAndPort&redirect=false

void setHostAndPort(in AUTF8String hostport); is defined in nsIURI.idl and implemented in all the base classes.

The methods you're adding are already in nsMsgMailNewsUrl:
nsMsgMailNewsUrl.cpp#607 and further down.
Attachment #8822758 - Flags: review?(jorgk) → review+

Comment 11

a year ago
Comment on attachment 8822759 [details] [diff] [review]
1326433-whitespace-fix.patch

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

I'm not keep on those white-space changes since they will make uplifts harder. TB 55+ would be a better time for a clean-up since less uplift-prone. Anyway, if you want to go ahead, you have my blessing ;-)
r=jorgk.
Attachment #8822759 - Flags: review?(jorgk) → review+

Comment 12

a year ago
s/keep/keen/
(Assignee)

Comment 13

a year ago
> These need to be defined in the base classes as you have done, 

Yes I saw but I was uneasy. But if there were needed in the first place they whould have probably already been implemented for nsIURIWithQuery in mailnews.

I assumed CLOSED tree to actually fix the closed tree:)

https://hg.mozilla.org/comm-central/rev/f7bc43a2fcccb6c5c4e8cd74221ee79c82c71664
https://hg.mozilla.org/comm-central/rev/e84176264e2b82b8eb362ff63e879537be53e4b7

> I'm not keep on those white-space changes since they will make uplifts harder.

I know but trailing blanks are such a nuisance...
(Assignee)

Updated

a year ago
Status: ASSIGNED → RESOLVED
Last Resolved: a year ago
Resolution: --- → FIXED
(Assignee)

Updated

a year ago
status-seamonkey2.50: --- → fixed
status-thunderbird53: --- → fixed
Target Milestone: --- → Thunderbird 53.0
Version: unspecified → 53
Blocks: 1326520
You need to log in before you can comment on or make changes to this bug.