Implement IMAP cache logging

RESOLVED FIXED in Thunderbird 66.0

Status

enhancement
RESOLVED FIXED
4 months ago
4 months ago

People

(Reporter: jorgk, Assigned: gds)

Tracking

(Blocks 1 bug)

Trunk
Thunderbird 66.0
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

Reporter

Description

4 months ago

+++ This bug was initially created as a clone of Bug #1454542 +++

Let's land the patch from bug 1454542 here.

Reporter

Comment 1

4 months ago

OK, I fixed heaps of little issues, this should be good to go now.

Attachment #9035982 - Flags: review+
Attachment #9035982 - Flags: feedback?(gds)
Reporter

Comment 2

4 months ago

I played through the different scenarios of part caching and the output looks good and useful. A few tweaks where required. I'm going to land this now.

Attachment #9035982 - Attachment is obsolete: true
Attachment #9035982 - Flags: feedback?(gds)
Attachment #9036005 - Flags: review+
Reporter

Comment 3

4 months ago

Killed few useless concatenations of "" :-(

Reporter

Updated

4 months ago
Assignee: nobody → gds
Status: NEW → ASSIGNED
Assignee

Comment 4

4 months ago
Comment on attachment 9036007 [details] [diff] [review]
1519387-IMAPCache-logging.patch (v2b)

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

Looks good to me!
Attachment #9036007 - Flags: feedback+

Comment 5

4 months ago

Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/f751be7a1779
Implement IMAP cache logging. r=jorgk

Status: ASSIGNED → RESOLVED
Last Resolved: 4 months ago
Resolution: --- → FIXED
Reporter

Updated

4 months ago
Target Milestone: --- → Thunderbird 66.0
Reporter

Updated

4 months ago
Attachment #9036005 - Attachment is obsolete: true
Reporter

Updated

4 months ago
Attachment #9036007 - Flags: review+
Assignee

Comment 6

4 months ago

(In reply to Jorg K (GMT+1) from comment #3)

Created attachment 9036007 [details] [diff] [review]
1519387-IMAPCache-logging.patch (v2b)

Killed few useless concatenations of "" :-(

Diffing the patches, it looks like v2b removes the closing quote on the log strings. So closing quote in PRId32 not needed when at the end of a string?

-            ("2. Do a sync?: ShowDeletedMsgs=%s, exists=%" PRId32 ", mFolderTotalMsgCount=%" PRId32 "",
+            ("2. Do a sync?: ShowDeletedMsgs=%s, exists=%" PRId32 ", mFolderTotalMsgCount=%" PRId32,
             GetShowDeletedMessages() ? "true" : "false", GetServerStateParser().NumberOfMessages(),
             mFolderTotalMsgCount));

Also, looks like this was removed from nsImapProtocol::PseudoInterruptMsgLoad():

+  MOZ_LOG(IMAPCache, LogLevel::Debug, ("PseudoInterruptMsgLoad(): Entering"));

Sorry for the late comment. (Can you disable this markdown stuff?)

Reporter

Comment 7

4 months ago

Hmm, the markdown stuff is a little annoying.

PRId32 is a string, normally "d", so why add another empty string to the end? That was wrong all along, so I now fixed all the occurrences. MOZ_LOG(IMAPCache, LogLevel::Debug, ("PseudoInterruptMsgLoad(): Entering"));didn't appear to be useful.

Comment 8

4 months ago

The patch does not seem to compile on trunk, at least on Linux and OS X"
/builds/worker/workspace/build/src/comm/mailnews/imap/src/nsImapProtocol.cpp:9758:81: error: data argument not used by format string [-Werror,-Wformat-extra-args]

Possibly this line:

  • MOZ_LOG(IMAPCache, LogLevel::Debug, ("ReadFromMemCache(): Returning " PRIx32, static_cast<uint32_t>(rv)));

There is no % before PRIx32 to consume the 'rv' argument.

Comment 9

4 months ago
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/95215e0170b6
Follow-up: Add forgotten % in format specifier. rs=bustage-fix DONTBUILD

Comment 10

4 months ago
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/0fb460d27117
Follow-up: remove unneeded \n from logging and assertions in IMAP. r=me DONTBUILD
You need to log in before you can comment on or make changes to this bug.