Closed Bug 794884 Opened 8 years ago Closed 8 years ago

Expose imgLoader to the rest of gecko

Categories

(Core :: ImageLib, defect)

x86
macOS
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla18

People

(Reporter: jrmuizel, Assigned: jrmuizel)

References

Details

Attachments

(4 files)

No description provided.
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.
Attachment #665468 - Flags: review?(joe)
Attachment #665468 - Flags: review?(bzbarsky)
This cleans things up quite a bit
Attachment #665470 - Flags: review?(joe)
Blocks: 794957
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.
Attachment #665468 - Flags: review?(bzbarsky) → review+
Attachment #665465 - Flags: review?(joe) → review+
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
Attachment #665468 - Flags: review?(joe) → review+
Attachment #665470 - Flags: review?(joe) → review+
---
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!
Oops sorry about that.
https://hg.mozilla.org/mozilla-central/rev/1758b93e29c7
https://hg.mozilla.org/mozilla-central/rev/bc14486d0742
https://hg.mozilla.org/mozilla-central/rev/9a71c92af5a4
Assignee: nobody → jmuizelaar
Status: NEW → RESOLVED
Closed: 8 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla18
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'
Depends on: 797823
(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.
(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.)
Also, imgLoader.h includes nsString.h which isn't useful to non-libxul consumers, although that's an easy fix (change to nsStringGlue.h).
Sorry, I meant imgRequest.h there.
Attachment #668802 - Flags: review?(joe)
Comment on attachment 668802 [details] [diff] [review]
External API fixes

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

Sort of ugly, but okay.
Attachment #668802 - Flags: review?(joe) → review+
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?
(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.
(In reply to neil@parkwaycc.co.uk 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?
(In reply to Jeff Muizelaar [:jrmuizel] from comment #21)
> mean't

really?!?
(In reply to Jeff Muizelaar [:jrmuizel] from comment #21)
> (In reply to neil@parkwaycc.co.uk 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).
And compile times are also waaay faster using external linkage (during development).
You need to log in before you can comment on or make changes to this bug.