740668 - Add noise to background of image document

Status: RESOLVED FIXED

(Toolkit :: Themes, defect)

Description

[Paul O'Shannessy [:zpao] (not bugmail, email directly)]

Attachment: Sample: left = noise, right = status quo

I tried taking the noise from the newtab page and adding it to the view image page and it looked good. Frank agrees. So I think we should do it. And if from a Core:Layout POV we shouldn't, then we should do it at the browser level (and make this page themeable if it's not already).

Comment 1

[Frank Yan (:fryn)]

(In reply to Paul O'Shannessy [:zpao] from comment #0)
> Frank agrees.

And Jared. And Stephen. Yay, party! :D

> if from a Core:Layout POV we shouldn't, then we should do it at the browser
> level (and make this page themeable if it's not already).

We already theme the page using CSS in /layout/style/ :)
https://mxr.mozilla.org/mozilla-central/source/layout/style/TopLevelImageDocument.css

Comment 2

[Jared Wein [:jaws] (please needinfo? me)]

We really should move the CSS files to toolkit first.

Comment 3

[Jared Wein [:jaws] (please needinfo? me)]

Attachment: Patch for bug

So this patch should work, but it probably will fail tons of reftests due to anti-aliasing of the noise.

If we the noise causes problems, the only solution may be to update the reftests to not be standalone images but html documents with the image as the only child (unfortunate for maintenance and loss of test coverage).

Comment 4

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 613081 [details] [diff] [review]
Patch for bug

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

Well... the reftests pass locally on my machine, so maybe this patch will work :)

Justin, can you review the toolkit/themes changes?
Joe, can you review the image/test/ changes?

Comment 5

[Joe Drew (not getting mail)]

Comment on attachment 613081 [details] [diff] [review]
Patch for bug

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

Er, can we put this file on disk for reftest purposes?

Comment 6

[Jared Wein [:jaws] (please needinfo? me)]

Attachment: Patch for bug v2

Moved the data URI to a file for the reftests.

Paul, can you review the toolkit/theme changes?
Joe, can you review the image/test changes?

Comment 7

[Jared Wein [:jaws] (please needinfo? me)]

Attachment: patch for bug 713487 (apply this first)

Comment 8

[Mozilla RelEng Bot]

Autoland Patchset:
	Patches: 613699, 613698
	Branch: mozilla-central => try
	Destination: http://hg.mozilla.org/try/pushloghtml?changeset=22907ff6b3d5
Try run started, revision 22907ff6b3d5. To cancel or monitor the job, see: https://tbpl.mozilla.org/?tree=Try&rev=22907ff6b3d5

Comment 9

[Paul O'Shannessy [:zpao] (not bugmail, email directly)]

Comment on attachment 613698 [details] [diff] [review]
Patch for bug v2

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

(In reply to Jared Wein [:jaws] from comment #6)
> Moved the data URI to a file for the reftests.

I don't know anything about reftests, but you put noise.png on disk in 4 directories. Can you not just refer to the one at the lowest depth? url(../noise.png)

::: toolkit/themes/pinstripe/global/TopLevelImageDocument.css
@@ +3,5 @@
>   * You can obtain one at http://mozilla.org/MPL/2.0/. */
>  
>  @media not print {
>    body {
> +    background: #222 url(data:image/png;base64,iVBORw0KGgoAAAANSUhEUgAAAFgAAABYBAMAAACDuy0HAAAAG1BMVEX+/v4BAQH///8KCgoDAwN/f3/19fWAgID8/PzhDwT2AAAACXRSTlMFBQUFBQUFBQWHDtP9AAALwklEQVR4Xg3KOWOyWhAA0Bn2ci57eXEvQY1JCZp8sQTjVoJLTAkaE0swbj/7ve4UB37FLW4q86Lwwlh86J/ASAkpWaj+Krbb31HzH0Kjc2tIl7SADaWbpZBPE5dds6jJNyNdjAyKWqdroIixWRQIY6E/kOY7hIciL/ZfrAO3XP/06AuUJ3mSd/z95OB9vIal0DPlaZWHP7RE6DIXjmKqKkuGr+xNZylOnj1GSlUKvnxZDBOIzTfMe0fJgJ7c/GIIOdUuKxYyBFUOzvY6AC5AXx8R+o5O4S0j0wqBND3ErIYm/XHFbQjtH1MXD5dUbp19OFdjkDlys+HSwrBgHRvL9wVN/pi8ViOIwcv/D1GRW6UuDvJLLQA5lCI17iUdsKYpOuYfMATGnpn/Zs3W6gov51G+/Vs9Ay//we5kh8uwvEPum6o5HkDMDb3ZWunwtq+UzENU8NphDdbvNtKM3knx5gi6UMSQl+eGs+27mraDtxeWdH+T62Us/GylEtr7Ct8jlbeXKvAf5onx8D2uVt1J/GblV+XQyKUInOUG44fqjcszK266yHWAAYG9ekhvy4l4Maa44jYVyV2RFEuS54e2HcswtmNdqR/+V4P0O9e4XnpWgxVSQkNXpYMCxJ4Vel0lmi56jnYIIJAQMndF+zTEiyuj92r3ijJT1O0alPQnLWJvJLR7Xx7Xg9fm9QOqFu8o29m3QQqFwZN4bki/RoprNtMKKtEET9iMsJyKpkiguAorn2yzkv0wG3M1EEVDJP5VN7muLjYCglzdGQ7boYGgRmorzhRDq83gglgylC+hBLEyy6ZQWNwCmmqt6PvExAqGEA9V2XIT4/fS+I2cx1n5td85kOCjHfPWTg72FJ/+vKOyggt+rytFbEDJWL+mPwpgw6HtFLIHmq4o2m1nZ9saKwiKEOTVZtWlnqHODPu949VfKD+zzpfynd/ZZU5IWZ0dgnqRHC4uOBpBsT8N7YbFJzADiW2eo/T979OKFxY8zk/+HR/NNEkzgSBsmA35Sayz1m/ubxgmYQOmffyRh9gdx42mUVX512oqWkfxAzyuSCxx1cywx3jIXuXJEEbssymo0xMy7SskJW9C5IPYroPwQunt7f5FEPPXJLWRbGHcL4Q3sx3TLAN6W672r/I5CKkL6zSwwk0AI8+iBCSv1Y7QQP5RSoLE227uy8vn22Y6dhLBgEsRh18cTGjIv3y+60Kmt3YAZQX8qf3bJDUc/5pdjti+KwAZ9GzzQzd23d1JBAnSvWkWB8YfsIGlspHitNiMPYPFfR+OecRuPyxgfoP9/HkR3cR27IohiaDXCk/3VNP6lIxP9TBnsMeAAUZloq6P8KURLBsNFuiA3LsN/d9qpCeKKIBgSzsN5k+rdh3uh0VbvMuOIomJD1fBOiCqIsvklS5bOQhMaahJC+Rc+6lz+Uvxmq05Py+LoGIQlLKvlcaHsFG9Ui66H/qdHz67sPRGho+ruC92QgN5JEMmLsZREEiJu78FJbyzT8FsdK90XoEcezn2R5iLUzZhczJmf1yNY3gJNJUQvbpTznTAbnV5J8iL4q2OWuhJEndWVTyEr8M5VGTWtvOmUo1DsnOsqXE5ZzKE8K4/8cl8+c1XArp1RUKz+iKP96j2FcUmA+v0HnEr0iUdSrRK5duAj1FQamvpiaXR2JddD6g8n4SyFx/fjT4LkC+ghJckj1e1wP+DrHrpIiMaPH5F1rcaRvwZWfEn6fx+/C7PdXABGLNKjr1USZ5XyHjsafXMEoXtguAfjykMioMMHISXVAc9yQY5o5Qg8MM0nhWCA2HoiEgBc1EH+warLjxH3Ln68M/ciFqI1bG0mBOxiNreOuShEf/9pIzhm1Bh2cbYVxn2IYQ7eljYpab/5EdPF2PSmcy+62j6e2HBPNbe+8JVMuRQBrWdL9uBh4bYbQaQJ07FyfcpCuvSuxUyYjP6avvw9gTcAj0uTVohSwOHDDaHTs8nyachMBcWoVDWp3/lWgqeCLMneAUhSuhD2RJpufLOSi7emxOVhYsOGomV2JCEKjWu7kuqwueyFEmDgVhR0l4oHn8W87UZuxb8id54SxHWiSnPKnMyAhzdhi2wN/AoH3OYwLajuybB8h/QeJJiX1gIt+dfij+gr0CJRXQ2Y04Q6q8xHzfWm9FIgchiW0+X86tIotIGzRG1gENaKokQkLn+FXZ2x3KUcp7d/NUsmOmFCG/i03YB8pi0eiNS4LUIfA06AKvfQmP/VAXS1AP2kzJ+9LAaTafvFyO7bz8U9OCpld2q1eHGts+ZFrt04AmIlubOPP7Xayfi/r0tiX2aaPT9Dz4+TVPBoXsjHDzWfrmawOsZfmBT/k2+c6sz/hvD5wjrjT7XgRlnEzPuZermi1jqfUrE3q7VdFfJu5oT9Ad+VUh1fIwIFhBy8TmMuhIeX2XpmogmvS1C3ZuwiyR87ZSrj0Jv1DpEAYkbcL3RpjZXmZpPV4mXH8z8Nh8CS+R+PpcTnkhyr5UJaSiz0wjK22Ewl+zS+pTug0PQ0CSnJQ5LfdR77vVZufgjkQ/ydf4V5zpEaNq+JZmrQK6WdZBacmMHL9RmLnPUs0/MYwYFzoyrXYQMTHGAUJOfumR5r79MZO28DIEXQVT5wGw99TY1T0GOCC/BzWv8READwICd0LjUNKnE6ORVa0lOnqhoO0v33lwWcwF0ynTgTpFxy+0OKdphNDWJlH8ubKoG6WJXtKxAwbsilpBJB+GBwimvTsCrv1R7LSX9ExkAw44ZEcxU3L50OHnKAyKZNe1fih+hVqItRGCDf7shuvme+lTWteX5oYuc58NrCaqjYIrIV0PFyQeh2ZzZEqNS60LuhnP5wweMkkaU93pDA/RWPNeGpPCBgiUeDvV0L1NfdRP/Hn5i7rUK7kftlIWeIUIYbtzzFl9nlIeaNfoX+x/qyWzIABLTZDbeq/hDZpxg2gkh+ICfSU8OUpJ8yWY17uQ5EGa+GGWFmnrBd9vX3KOteYkJaMpPwJ4TjzDjbhkOMKmWKClzVJ2g81YGFl/c0xPIKncgJGdUKvZoUUJu0gYaIAh6E0xNeQ15qpJXzNITgf4W+w/oUaKOM54EMUi1j5yvOCsEe8JYpwVGj53lNiPMY9Rltgd4icp82fvN69zkSBUI40nJSRTeHz7h1IX42Cr0klWjxjO05MSX1IaTeDmTRGEeKvAvtaaBaLQnjftGJz+4cjFyy6/iCjLGF2/gW+jQhEUxbEBPyQzXi+Bb4kc9wK4jIwNLWbwQAOtYKRLaipDH+X4TPPOG8DCNY4IC9yBk1qcibjhUgRnDcf35pl9d5otbvQjOIXlEu5dVtm5LRaK5KWcD/PX6LaGd25CuNHG/vgeIB1kcpCme+J8idlcjfBALAJSggznsGHGOAJgdGduMnZg+bAaeGASGV9bh/X2wPsVTmBLxmTTQsBGFkEOkZJTsGAm+HrtMDbWwvTXOutX1u7BxIq9Xib6DkFMbUitNdrYsULkahsAhBEh9FjdzL9BNARxTSr7T3u1rE+IWUmCIpwTZHZCu5l9THCuCcOhZqfekuQxjQ7EoyGUJAwCv/q1JOuJeCc/3lknb76zAquO/DAQhK/62cP8X2s3+IBLIhvL8RHopoHpIArJysYTTmMMeubPXh8W760AvMVH67jqgg06+/ne5MZ631z6yROhloh3dPQirZoEpr80wgt/cEbhbAQTmRLtGh8lxCwDBBb5OeJ4aEq25XBNMT2rzWedW2zIzj+CCDKlnlyJBzT81qBWp69h7vlb3TmEV+DNm2rqj1iT7BQuwVVsuPkwq1e5P8tgNjVbIlMzwXeM11kZqjx3KKFOJzc3CAyFVhi8fxVZ5FvhdAM5mM6kS6OgKu16MFglq3/b/QVIwdw7HUCyeW04JPjC5dO+GC9OfqfB4VX+wwuift+ths2Ss3i6nkOE+JFyD+wKFL+WMX6nwwDva0S1/O8Mlnida69Ph96fuFvCoRMvXnCfsLPPmC/hA5RnMNE4fDK0pVOQ4BHLaErzv/wD99ABmjNZk0AAAAABJRU5ErkJggg==);

Personally, I'd prefer background-color and background-image to be specified individually as opposed to using the combined background rule. But based on Dao's review in the other bug, this is fine.

I also wish we would put that beautiful data uri in a shared location (browser/themes has browserShared.inc). It's not a huge deal though and we already do this so whatever.

Comment 10

[Jared Wein [:jaws] (please needinfo? me)]

(In reply to Paul O'Shannessy [:zpao] from comment #9)
> Comment on attachment 613698 [details] [diff] [review]
> Patch for bug v2
> 
> Review of attachment 613698 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> (In reply to Jared Wein [:jaws] from comment #6)
> > Moved the data URI to a file for the reftests.
> 
> I don't know anything about reftests, but you put noise.png on disk in 4
> directories. Can you not just refer to the one at the lowest depth?
> url(../noise.png)

This is because some of the reftests run their own server which has their local directory as the root of the server, so it's impossible to navigate up the folder hierarchy. I ran in to this issue while working on bug 379667 and had to duplicate ImageDocument.css for the same reason.

Comment 11

[Paul O'Shannessy [:zpao] (not bugmail, email directly)]

(In reply to Jared Wein [:jaws] from comment #10)
> This is because some of the reftests run their own server which has their
> local directory as the root of the server, so it's impossible to navigate up
> the folder hierarchy.

Ah ok. TIL.

Comment 12

[Jared Wein [:jaws] (please needinfo? me)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/920825c9873f

Comment 13

[Mozilla RelEng Bot]

Try run for 22907ff6b3d5 is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=22907ff6b3d5
Results (out of 13 total builds):
    success: 13
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-22907ff6b3d5

Comment 14

[Matt Brubeck (:mbrubeck)]

https://hg.mozilla.org/mozilla-central/rev/920825c9873f

Comment 15

[Justin Dolske [:Dolske]]

(In reply to Paul O'Shannessy [:zpao] from comment #9)

> I also wish we would put that beautiful data uri in a shared location
> (browser/themes has browserShared.inc). It's not a huge deal though and we
> already do this so whatever.

Yeah, giant data UIRs in CSS is something we should avoid (even though this patch is just shifting it around). Followup bug?

Comment 16

[Alfred Kayser]

What is 'color:#EEE' doing on the IMG?

Comment 17

[Justin Dolske [:Dolske]]

Does the error text for broken images use that?

Comment 18

[Jared Wein [:jaws] (please needinfo? me)]

(In reply to Alfred Kayser from comment #16)
> What is 'color:#EEE' doing on the IMG?

(In reply to Justin Dolske [:Dolske] from comment #17)
> Does the error text for broken images use that?

Yeah, there is native anonymous content for text within the image that can be seen if there are decoding errors or the image is loading slowly.