1381969 - Optimize TB's rendering of attached inline images: Conditional compiling of Seamonkey-specific code for mail.enable_automatic_image_resizing pref

Status: RESOLVED FIXED

(MailNews Core :: MIME, enhancement)

Description

[Thomas D. (:thomas8)]

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

https://dxr.mozilla.org/comm-central/source/mailnews/mailnews.js#832-834

> #ifdef MOZ_SUITE
> // automatically scale attached images that are displayed inline
> pref("mail.enable_automatic_image_resizing", true);

TB doesn't have this pref, but when parsing mime image attachments, we run a chunk of pref-related code for every attached image in every message. To optimize runtime rendering, pref-related code should only be compiled for SeaMonkey, using #ifdef MOZ_SUITE.

Otherwise, the best solution would be for SeaMonkey to port bug 534083, then we can get rid of the pref and related code completely, as suggested in bug 534083 comment #27.

https://dxr.mozilla.org/comm-central/source/mailnews/mime/src/mimemoz2.cpp#1107-1115

>   nsCOMPtr<nsIPrefBranch> prefBranch;
>   nsCOMPtr<nsIPrefService> prefSvc(do_GetService(NS_PREFSERVICE_CONTRACTID));
>   bool resize = true;
>
>   if (prefSvc)
>     prefSvc->GetBranch("", getter_AddRefs(prefBranch));
>   if (prefBranch)
>     prefBranch->GetBoolPref("mail.enable_automatic_image_resizing", &resize); // ignore return value
>   prefix = resize ? scaledPrefix : unscaledPrefix;

Comment 1

[Jorg K (CEST = GMT+2)]

Attachment: 1381969-non-existant-pref.patch (v1).

Comment 2

[Thomas D. (:thomas8)]

Comment on attachment 8887647 [details] [diff] [review]
1381969-non-existant-pref.patch (v1).

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

Sweet and simple!
Wearing my perfectionist hat, I'd like to optimize this even more... ;)

::: mailnews/mime/src/mimemoz2.cpp
@@ +1103,5 @@
>    /* Internal-external-reconnect only works when going to the screen. */
>    if (!mid->istream)
>      return strdup("<DIV CLASS=\"moz-attached-image-container\"><IMG SRC=\"resource://gre-resources/loading-image.png\" ALT=\"[Image]\"></DIV>");
>  
> +  bool resize = true;

Nit: Not needed for TB, if we optimize below. Let's move this under #ifdef

@@ +1117,1 @@
>    prefix = resize ? scaledPrefix : unscaledPrefix;

For optimum performance:
TB doesn't need this conditional assignment. Let's give to Suite what belongs to Suite, and to TB what belongs to TB. I suggest:

#ifdef MOZ_SUITE
  bool resize = true;
  ...
  prefix = resize ? scaledPrefix : unscaledPrefix;
#else
  prefix = scaledPrefix;
#endif

We could even move creation of unscaledPrefix into the MOZ_SUITE block, but maybe that's taking it a bit far for now...

Comment 3

[Jorg K (CEST = GMT+2)]

Attachment: 1381969-non-existant-pref.patch - no good.

Your suggestion shown here won't compile since 'unscaledPrefix' is unused.

Comment 4

[Thomas D. (:thomas8)]

Comment on attachment 8887677 [details] [diff] [review]
1381969-non-existant-pref.patch - no good.

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

(In reply to Jorg K (GMT+2) from comment #3)
> Created attachment 8887677 [details] [diff] [review]
> 1381969-non-existant-pref.patch - no good.
> 
> Your suggestion shown here won't compile since 'unscaledPrefix' is unused.

Jörg always loves to educate me, especially when I make mistakes... ;)
Duh, unused variables are no good indeed... 
I'm sure he knows the solution already! ...
So the compiler is stricter than me and enforces what I already suggested above:

As unscaledPrefix is only used in Suite, it must be in the Suite block. Fair enough. No problem. So how's this?

Unfortunately, I don't know much about compiling, because I don't compile, and with my current hardware, I couldn't anyway...

::: mailnews/mime/src/mimemoz2.cpp
@@ +1104,5 @@
>    if (!mid->istream)
>      return strdup("<DIV CLASS=\"moz-attached-image-container\"><IMG SRC=\"resource://gre-resources/loading-image.png\" ALT=\"[Image]\"></DIV>");
>  
> +  // Thunderbird doesn't have this pref.
> +#ifdef MOZ_SUITE

const char *unscaledPrefix = "<P><CENTER><IMG CLASS=\"moz-attached-image\" SRC=\"";

Comment 5

[:aceman]

(In reply to Thomas D. (currently busy elsewhere; needinfo?me) from comment #4)
> As unscaledPrefix is only used in Suite, it must be in the Suite block. Fair
> enough. No problem. So how's this?

Yes, that could work but maybe it would be good to keep the strings together to see the difference between them.

Comment 6

[:aceman]

Attachment: 1381969-unscaledPrefix-inside-ifdef

What about this?

Comment 7

[Thomas D. (:thomas8)]

Comment on attachment 8889602 [details] [diff] [review]
1381969-unscaledPrefix-inside-ifdef

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

Nice! :)

Comment 8

[Jorg K (CEST = GMT+2)]

Comment on attachment 8889602 [details] [diff] [review]
1381969-unscaledPrefix-inside-ifdef

OK, let's go with this then.

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/942c522bd77e
Don't read non-existent mail.enable_automatic_image_resizing in TB. r=jorgk