943686 - Enable all of imagelib to be built in unified mode

Status: RESOLVED FIXED

(Core :: Graphics: ImageLib, defect)

Description

[Seth Fowler [:seth] [:s2h]]

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.

Comment 1

[Seth Fowler [:seth] [:s2h]]

(See bug 941854 for more discussion of the FORCE_PR_LOG issue.)

Comment 2

[(no longer active)]

Seth, what's happening here in this bug?

Comment 3

[Seth Fowler [:seth] [:s2h]]

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.

Comment 4

[(no longer active)]

(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!

Comment 5

[(Away)]

Is this still an issue now that bug 806819 has landed?

Comment 6

[Seth Fowler [:seth] [:s2h]]

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.

Comment 7

[Seth Fowler [:seth] [:s2h]]

Attachment: Build remaining nonunified ImageLib sources in unified mode

Here's the patch.

Comment 8

[Seth Fowler [:seth] [:s2h]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=dd9f26edeabb

Comment 9

[Seth Fowler [:seth] [:s2h]]

Hmm... OK, it looks like some manual fixes are still needed.

Comment 10

[Eric Rahm [:erahm]]

Attachment: Bug 943686 - Add imageLoader.cpp to unified sources. ?

Review commit: https://reviewboard.mozilla.org/r/122066/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/122066/

Comment 11

[Eric Rahm [:erahm]]

Attachment: Bug 943686 - Add imgRequest.cpp to unified sources. ?

Review commit: https://reviewboard.mozilla.org/r/122068/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/122068/

Comment 12

[Eric Rahm [:erahm]]

Attachment: Bug 943686 - Add imgRequestProxy.cpp to unified sources. ?

Review commit: https://reviewboard.mozilla.org/r/122070/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/122070/

Comment 13

[Eric Rahm [:erahm]]

Attachment: Bug 943686 - Add ProgressTracker.cpp to unified sources. ?

Review commit: https://reviewboard.mozilla.org/r/122072/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/122072/

Comment 14

[Eric Rahm [:erahm]]

Attachment: Bug 943686 - Add RasterImage.cpp to unified sources. ?

Review commit: https://reviewboard.mozilla.org/r/122074/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/122074/

Comment 15

[Eric Rahm [:erahm]]

Attachment: Add imageLoader.cpp to unified sources

MozReview-Commit-ID: G9sFOtbFYjP

Comment 16

[Eric Rahm [:erahm]]

Attachment: Add imgRequest.cpp to unified sources

MozReview-Commit-ID: B87v26HNtBn

Comment 17

[Eric Rahm [:erahm]]

Attachment: Add imgRequestProxy.cpp to unified sources

MozReview-Commit-ID: ITT9T22WyYG

Comment 18

[Eric Rahm [:erahm]]

Attachment: Add ProgressTracker.cpp to unified sources

MozReview-Commit-ID: Ipo4Tb1bnHG

Comment 19

[Eric Rahm [:erahm]]

Attachment: Add RasterImage.cpp to unified sources

MozReview-Commit-ID: cRFkjJi4dP

Comment 20

[Timothy Nikkel (: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/.

Comment 21

[Eric Rahm [:erahm]]

(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.

Comment 22

[Timothy Nikkel (:tnikkel)]

Oh, that's disappointing.

Comment 23

[Eric Rahm [:erahm]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/8c34803d1392c125b736614b652f1935ea6f5019
Bug 943686 - Add imageLoader.cpp to unified sources. r=tn

https://hg.mozilla.org/integration/mozilla-inbound/rev/726f50ce3a40fc35279cf9d240128d54e4053fd2
Bug 943686 - Add imgRequest.cpp to unified sources. r=tn

https://hg.mozilla.org/integration/mozilla-inbound/rev/46d1aeb34ad2f66a04938e718f8cd80980ab71d9
Bug 943686 - Add imgRequestProxy.cpp to unified sources. r=tn

https://hg.mozilla.org/integration/mozilla-inbound/rev/2828bb6eabefd57f2c876a218d0479c0c39d360b
Bug 943686 - Add ProgressTracker.cpp to unified sources. r=tn

https://hg.mozilla.org/integration/mozilla-inbound/rev/538a6a953f5dcd16f303597e05a6665a42d3e657
Bug 943686 - Add RasterImage.cpp to unified sources. r=tn

Comment 24

[Eric Rahm [:erahm]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=b9dda3032289

Comment 25

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

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)

Comment 26

[Eric Rahm [:erahm]]

Doing a try push to see if this is just a clobber issue.

Comment 27

[Eric Rahm [:erahm]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/a7f4eebaf86b386a5e940c081c9c85cd2ce4452a
Bug 943686 - Add imageLoader.cpp to unified sources. r=tn

https://hg.mozilla.org/integration/mozilla-inbound/rev/050ac6eabd673c2151d7f00dcbd3bc7eef576dbf
Bug 943686 - Add imgRequest.cpp to unified sources. r=tn

https://hg.mozilla.org/integration/mozilla-inbound/rev/8f2a4bf4983f703bacd79d03782588f4cdd21f44
Bug 943686 - Add imgRequestProxy.cpp to unified sources. r=tn

https://hg.mozilla.org/integration/mozilla-inbound/rev/1943b9b0b58524d145b8b5d82ace9000de9bf6f2
Bug 943686 - Add ProgressTracker.cpp to unified sources. r=tn

https://hg.mozilla.org/integration/mozilla-inbound/rev/17d0a9fdad9eefbac4c9025d587e945bbb7a58ab
Bug 943686 - Add RasterImage.cpp to unified sources. r=tn

Comment 28

[Eric Rahm [:erahm]]

Appears to have been a case of <windows.h> #defining |LoadImage| -> |LoadImageW|.

Comment 29

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/a7f4eebaf86b
https://hg.mozilla.org/mozilla-central/rev/050ac6eabd67
https://hg.mozilla.org/mozilla-central/rev/8f2a4bf4983f
https://hg.mozilla.org/mozilla-central/rev/1943b9b0b585
https://hg.mozilla.org/mozilla-central/rev/17d0a9fdad9e