Closed
Bug 406035
Opened 17 years ago
Closed 17 years ago
Convert mailnews/import so it can compile with frozen api on linux
Categories
(MailNews Core :: Import, defect)
MailNews Core
Import
Tracking
(Not tracked)
RESOLVED
FIXED
People
(Reporter: standard8, Assigned: standard8)
References
Details
Attachments
(1 file, 1 obsolete file)
19.08 KB,
patch
|
neil
:
review+
neil
:
superreview+
|
Details | Diff | Splinter Review |
The patch I'm attaching moves the linux parts of mailnews/import so that they can be compiled with the frozen api ready for libxul (this leaves the outlook & eudora import items).
We can't link it with the frozen api yet because the core mailnews code on which it depends still needs converting. There may be one other link problem, but I'm just looking at getting the majority of it converted.
However, this is another step in the right direction.
Attachment #290706 -
Flags: superreview?(neil)
Attachment #290706 -
Flags: review?(neil)
Comment 1•17 years ago
|
||
Comment on attachment 290706 [details] [diff] [review]
The fix
>+#ifdef MOZILLA_INTERNAL_API
> offset = buffer.Find(prefName,PR_FALSE, 0, -1);
>+#else
>+ offset = buffer.Find(prefName, PR_FALSE);
>+#endif
Under external API, the second parameter to Find is not a PRBool, but as it happens you want the default values for all optional parameters anyway, so:
offset = buffer.Find(prefName);
should suffice, shouldn't it? Similarly for the other uses of Find.
>- field.ReplaceSubstring( "\"\"", "\"");
>+ PRInt32 offset = field.Find(NS_LITERAL_CSTRING("\"\""));
>+ while (offset != -1) {
>+ field.Replace(offset, 2, "\"");
>+ }
You don't Find again after your Replace (also below). Also you can in theory write your Replace differently to just delete one of the two quotes.
> if ((idx != -1) && (idx > 0) && ((name.Length() - idx - 1) < 5)) {
>- nsString t;
>- name.Left( t, idx);
>- name = t;
>+ name = StringHead(name, idx);
> }
Would name.SetLength(idx); work?
Attachment #290706 -
Flags: superreview?(neil)
Attachment #290706 -
Flags: superreview-
Attachment #290706 -
Flags: review?(neil)
Assignee | ||
Comment 2•17 years ago
|
||
Addressed Neil's comments.
Attachment #290706 -
Attachment is obsolete: true
Attachment #290866 -
Flags: superreview?(neil)
Attachment #290866 -
Flags: review?(neil)
Comment 3•17 years ago
|
||
Comment on attachment 290866 [details] [diff] [review]
The fix v2
>+ if (endOffset != -1) {
>+ nsAutoString prefValue(Substring(buffer, offset + PREF_LENGTH, endOffset - (offset + PREF_LENGTH)));
>+ found = PR_TRUE;
>+ *retval = ToNewUnicode(prefValue);
>+ break;
> }
I notice you can call ToNewUnicode directly on a Substring, no need to copy it to an nsAutoString first. It also looks as if you forgot to indent this block.
>+ PRInt32 offset = field.Find(NS_LITERAL_CSTRING("\"\""));
>+ while (offset != -1) {
>+ field.Replace(offset, 1, EmptyCString());
>+ offset = field.Find(NS_LITERAL_CSTRING("\"\""));
>+ }
This won't work correctly if the field contains multiple consecutive quotes - because you don't start your match after the previous quote you can keep matching the same quote over and again. r+sr=me with this fixed. You can also use Cut instead of Replace. (The loops later on don't have this bug because they completely remove the search string, but the change would still optimise them).
Attachment #290866 -
Flags: superreview?(neil)
Attachment #290866 -
Flags: superreview+
Attachment #290866 -
Flags: review?(neil)
Attachment #290866 -
Flags: review+
Assignee | ||
Comment 4•17 years ago
|
||
I checked this in earlier with Neil's comments addressed.
Status: NEW → RESOLVED
Closed: 17 years ago
Resolution: --- → FIXED
Updated•16 years ago
|
Product: Core → MailNews Core
You need to log in
before you can comment on or make changes to this bug.
Description
•