794884 - Expose imgLoader to the rest of gecko

Status: RESOLVED FIXED

(Core :: Graphics: ImageLib, defect)

Comment 1

[Jeff Muizelaar [:jrmuizel]]

Attachment: Avoid unecessarily including imgStatusTracker.h

Comment 2

[Jeff Muizelaar [:jrmuizel]]

Attachment: Make SupportImageWithMimeType a static function

This removes supportImageWithMimeType from the idl and makes all the callers call the static function directly. To do this we also need to export imgLoader.h and imgRequest.h.

Boris, this patch moves a comment around. Does it still make sense in the new location.

A patch to make the function return a bool is next.

Comment 3

[Jeff Muizelaar [:jrmuizel]]

Attachment: make SupportImageWithMimeType return a bool

This cleans things up quite a bit

Comment 4

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 665468 [details] [diff] [review]
Make SupportImageWithMimeType a static function

Yeah, this seems fine, as long as there are no JS (including extensions) consumers.

Comment 5

[Joe Drew (not getting mail)]

Comment on attachment 665468 [details] [diff] [review]
Make SupportImageWithMimeType a static function

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

::: image/public/imgILoader.idl
@@ +84,1 @@
>     * libpr0n does NOT keep a strong ref to the observer; this prevents

change the uuid

Comment 6

[(no longer active)]

---
content/base/src/nsObjectLoadingContent.cpp | 9 ++-------
docshell/base/nsWebNavigationInfo.cpp | 8 +++++---
docshell/base/nsWebNavigationInfo.h | 5 -----
image/public/imgILoader.idl | 9 +--------
image/src/Makefile.in | 4 ++++
image/src/imgLoader.cpp | 2 +-
image/src/imgLoader.h | 1 +
layout/build/nsContentDLF.cpp | 5 ++---
8 files changed, 16 insertions(+), 27 deletions(-) 

Yikes!

Comment 7

[Jeff Muizelaar [:jrmuizel]]

Oops sorry about that.

Comment 8

[Jeff Muizelaar [:jrmuizel]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/9a71c92af5a4
https://hg.mozilla.org/integration/mozilla-inbound/rev/bc14486d0742
https://hg.mozilla.org/integration/mozilla-inbound/rev/1758b93e29c7

Comment 9

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/1758b93e29c7
https://hg.mozilla.org/mozilla-central/rev/bc14486d0742
https://hg.mozilla.org/mozilla-central/rev/9a71c92af5a4

Comment 10

[Stefan Sitter [:ssitter]]

It seems you forgot about comm-central broke building Thunderbird:
> .../src/mailnews/mime/src/mimei.cpp(726) : error C2039: 'SupportImageWithMimeType' : is not a member of 'imgILoader'
> ...\mozilla\dist\include\imgILoader.h(47) : see declaration of 'imgILoader'

Comment 11

[Mark Banner (:standard8)]

(In reply to Stefan Sitter from comment #10)
> It seems you forgot about comm-central broke building Thunderbird:
> > .../src/mailnews/mime/src/mimei.cpp(726) : error C2039: 'SupportImageWithMimeType' : is not a member of 'imgILoader'
> > ...\mozilla\dist\include\imgILoader.h(47) : see declaration of 'imgILoader'

Please remember, there are no requirements for Gecko changes to work in Thunderbird as well - that is Thunderbird's responsibility. That doesn't prevent gecko folks fixing Thunderbird as well (which is always nice and appreciated), but it is not a responsibility for gecko folks.

Comment 12

[[e-mail]]

(In reply to Boris Zbarsky from comment #4)
> Comment on attachment 665468 [details] [diff] [review]
> Make SupportImageWithMimeType a static function
> 
> Yeah, this seems fine, as long as there are no JS (including extensions)
> consumers.

What about other non-libxul consumers? I want to build mail as a separate library, and it can't link to this function because it's not exported. (This wasn't a problem when it was virtual of course.)

Comment 13

[[e-mail]]

Also, imgLoader.h includes nsString.h which isn't useful to non-libxul consumers, although that's an easy fix (change to nsStringGlue.h).

Comment 14

[[e-mail]]

Sorry, I meant imgRequest.h there.

Comment 15

[[e-mail]]

Attachment: External API fixes

Comment 16

[Joe Drew (not getting mail)]

Comment on attachment 668802 [details] [diff] [review]
External API fixes

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

Sort of ugly, but okay.

Comment 17

[Jeff Muizelaar [:jrmuizel]]

Comment on attachment 668802 [details] [diff] [review]
External API fixes

This should have a comment about why it needs to be supported. Also, is there a reason that thunderbird is not building with libxul?

Comment 18

[[e-mail]]

(In reply to Jeff Muizelaar from comment #17)
> This should have a comment about why it needs to be supported. Also, is
> there a reason that thunderbird is not building with libxul?

Depends on what you mean by "with libxul". There are three possible configurations:
1. "Fatlibxul" whereby libxul contains Thunderbird
2. "With libxul" whereby libxul is built and then Thunderbird is built
3. "With libxul sdk" whereby Thunderbird is built using a previously compiled SDK
Attachment 668802 [details] [diff] supports cases 2 and 3.

Comment 19

[[e-mail]]

Comment on attachment 668802 [details] [diff] [review]
External API fixes

https://hg.mozilla.org/integration/mozilla-inbound/rev/b57278dc0172

Comment 20

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/b57278dc0172

Comment 21

[Jeff Muizelaar [:jrmuizel]]

(In reply to [e-mail] from comment #18)
> (In reply to Jeff Muizelaar from comment #17)
> > This should have a comment about why it needs to be supported. Also, is
> > there a reason that thunderbird is not building with libxul?
> 
> Depends on what you mean by "with libxul". There are three possible
> configurations:
> 1. "Fatlibxul" whereby libxul contains Thunderbird
> 2. "With libxul" whereby libxul is built and then Thunderbird is built
> 3. "With libxul sdk" whereby Thunderbird is built using a previously
> compiled SDK
> Attachment 668802 [details] [diff] supports cases 2 and 3.

I mean't fatlibxul. Are there plans to continue supporting options 2 and 3? If so why?

Comment 22

[Joe Drew (not getting mail)]

(In reply to Jeff Muizelaar [:jrmuizel] from comment #21)
> mean't

really?!?

Comment 23

[Jan Horak [:jhorak]]

(In reply to Jeff Muizelaar [:jrmuizel] from comment #21)
> (In reply to [e-mail] from comment #18)
> > (In reply to Jeff Muizelaar from comment #17)
> > > This should have a comment about why it needs to be supported. Also, is
> > > there a reason that thunderbird is not building with libxul?
> > 
> > Depends on what you mean by "with libxul". There are three possible
> > configurations:
> > 1. "Fatlibxul" whereby libxul contains Thunderbird
> > 2. "With libxul" whereby libxul is built and then Thunderbird is built
> > 3. "With libxul sdk" whereby Thunderbird is built using a previously
> > compiled SDK
> > Attachment 668802 [details] [diff] supports cases 2 and 3.
> 
> I mean't fatlibxul. Are there plans to continue supporting options 2 and 3?
> If so why?
We (Fedora) would like to share libxul for Firefox and Thunderbird. This should bring users less memory consumption when using both applications concurrently and should make security updates smaller. AFAIK rest of Linux distributions would like to do the same (ie. option 3).

Comment 24

[Magnus Melin [:mkmelin]]

And compile times are also waaay faster using external linkage (during development).