Enable all of imagelib to be built in unified mode

RESOLVED FIXED in Firefox 55

Status

()

defect
RESOLVED FIXED
6 years ago
2 years ago

People

(Reporter: seth, Assigned: erahm)

Tracking

Trunk
mozilla55
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox55 fixed)

Details

Attachments

(5 attachments, 6 obsolete attachments)

2.74 KB, patch
tnikkel
: review+
Details | Diff | Splinter Review
5.83 KB, patch
tnikkel
: review+
Details | Diff | Splinter Review
1.98 KB, patch
tnikkel
: review+
Details | Diff | Splinter Review
1.20 KB, patch
tnikkel
: review+
Details | Diff | Splinter Review
1.28 KB, patch
tnikkel
: review+
Details | Diff | Splinter Review
Right now we are prevented from building many files in imagelib in unified mode due to the use of FORCE_PR_LOG in ImageLogging.h. I suspect that we'd get more benefit out of building all of imagelib in unified mode than we do from FORCE_PR_LOG. (And indeed, I'm skeptical that ImageLogging.h in general offers much value, though I need to look more deeply at how it's used before I'll be sure.)

We should take a look at what value ImageLogging.h actually offers and do what's necessary to enable fully unified builds in imagelib.
(See bug 941854 for more discussion of the FORCE_PR_LOG issue.)
Blocks: unified
Seth, what's happening here in this bug?
Flags: needinfo?(seth)
I am not working on this at the moment. I filed the bug to make sure that we kept track of the fact that issues remained with unified builds in imagelib.
Flags: needinfo?(seth)
(In reply to comment #3)
> I am not working on this at the moment. I filed the bug to make sure that we
> kept track of the fact that issues remained with unified builds in imagelib.

OK, thanks for the clarification!
Is this still an issue now that bug 806819 has landed?
Flags: needinfo?(seth)
David, I don't think so, though it's been so long since I filed this that I don't remember the details of the problem anymore. =)

Testing should be straightforward, so let's give it a try.
Flags: needinfo?(seth)
Assignee: nobody → seth
Status: NEW → ASSIGNED
Hmm... OK, it looks like some manual fixes are still needed.
Attachment #8600201 - Attachment is obsolete: true
Attachment #8849249 - Attachment is obsolete: true
Attachment #8849250 - Attachment is obsolete: true
Attachment #8849251 - Attachment is obsolete: true
Attachment #8849252 - Attachment is obsolete: true
Attachment #8849253 - Attachment is obsolete: true
MozReview-Commit-ID: G9sFOtbFYjP
Attachment #8849263 - Flags: review?(tnikkel)
Assignee: seth.bugzilla → erahm
MozReview-Commit-ID: B87v26HNtBn
Attachment #8849264 - Flags: review?(tnikkel)
MozReview-Commit-ID: ITT9T22WyYG
Attachment #8849265 - Flags: review?(tnikkel)
MozReview-Commit-ID: Ipo4Tb1bnHG
Attachment #8849266 - Flags: review?(tnikkel)
MozReview-Commit-ID: cRFkjJi4dP
Attachment #8849267 - Flags: review?(tnikkel)
Comment on attachment 8849263 [details] [diff] [review]
Add imageLoader.cpp to unified sources

It'd be nice if we could do "using mozilla::image" so we don't have to specify the image namespace while we are inside image/.
Attachment #8849263 - Flags: review?(tnikkel) → review+
Attachment #8849264 - Flags: review?(tnikkel) → review+
Attachment #8849265 - Flags: review?(tnikkel) → review+
Attachment #8849266 - Flags: review?(tnikkel) → review+
Attachment #8849267 - Flags: review?(tnikkel) → review+
(In reply to Timothy Nikkel (:tnikkel) from comment #20)
> Comment on attachment 8849263 [details] [diff] [review]
> Add imageLoader.cpp to unified sources
> 
> It'd be nice if we could do "using mozilla::image" so we don't have to
> specify the image namespace while we are inside image/.

Sorry I guess I should have included the various build failures. Basically the modification I made were to disambiguate |mozilla::layers::Image| and |mozilla::image::Image|. Adding a using statement didn't help.
Oh, that's disappointing.
Backed out for Windows bustage:

https://hg.mozilla.org/integration/mozilla-inbound/rev/083daa40d2d70bad15dd135276905f8ed9c08b51
https://hg.mozilla.org/integration/mozilla-inbound/rev/fc2be069fd576c9cf99c8797d541f35cdf09e4f5
https://hg.mozilla.org/integration/mozilla-inbound/rev/b1c38bf368292d2bb8fba967ac113ef376a95133
https://hg.mozilla.org/integration/mozilla-inbound/rev/e427c48b6a0a24cc3cae8890c2f6c44603c3f2de
https://hg.mozilla.org/integration/mozilla-inbound/rev/4b473fd58d07762772695d771096199da4acc00c

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=538a6a953f5dcd16f303597e05a6665a42d3e657&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=retry&filter-resultStatus=usercancel&filter-resultStatus=runnable
Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=85152753&repo=mozilla-inbound

21:51:54     INFO -  nsContentUtils.obj : error LNK2019: unresolved external symbol "public: enum nsresult __thiscall imgLoader::LoadImage(class nsIURI *,class nsIURI *,class nsIURI *,enum mozilla::net::ReferrerPolicy,class nsIPrincipal *,class nsILoadGroup *,class imgINotificationObserver *,class nsINode *,class nsIDocument *,unsigned int,class nsISupports *,unsigned int,class nsAString const &,class imgRequestProxy * *)" (?LoadImage@imgLoader@@QAE?AW4nsresult@@PAVnsIURI@@00W4ReferrerPolicy@net@mozilla@@PAVnsIPrincipal@@PAVnsILoadGroup@@PAVimgINotificationObserver@@PAVnsINode@@PAVnsIDocument@@IPAVnsISupports@@IABVnsAString@@PAPAVimgRequestProxy@@@Z) referenced in function "public: static enum nsresult __cdecl nsContentUtils::LoadImage(class nsIURI *,class nsINode *,class nsIDocument *,class nsIPrincipal *,class nsIURI *,enum mozilla::net::ReferrerPolicy,class imgINotificationObserver *,int,class nsAString const &,class imgRequestProxy * *,unsigned int)" (?LoadImage@nsContentUtils@@SA?AW4nsresult@@PAVnsIURI@@PAVnsINode@@PAVnsIDocument@@PAVnsIPrincipal@@0W4ReferrerPolicy@net@mozilla@@PAVimgINotificationObserver@@HABVnsAString@@PAPAVimgRequestProxy@@I@Z)
Flags: needinfo?(erahm)
Doing a try push to see if this is just a clobber issue.
Appears to have been a case of <windows.h> #defining |LoadImage| -> |LoadImageW|.
Flags: needinfo?(erahm)
You need to log in before you can comment on or make changes to this bug.