386163 - 'Set Desktop Background' refactoring: use canvas in all cases, support widescreen previews

Status: RESOLVED FIXED

(Firefox :: General, defect)

Description

[Dão Gottwald [:dao]]

Using canvas in all cases simplifies the code and makes the whole experience a bit smoother, as the image doesn't have to be reloaded (even if it's only from cache).

Before I can post the patch, I need the patches in bug 385844 and bug 385503 to be checked in.

This will also fix bug 385717.

Comment 1

[Dão Gottwald [:dao]]

Attachment: mozilla/browser/themes/pinstripe/browser/monitor.png (unnecessary padding removed)

Comment 2

[Dão Gottwald [:dao]]

Attachment: mozilla/browser/themes/pinstripe/browser/monitor_16-10.png

Comment 3

[Dão Gottwald [:dao]]

Attachment: mozilla/browser/themes/winstripe/browser/monitor_16-10.png

Comment 4

[Dão Gottwald [:dao]]

Attachment: patch

Comment 5

[Asaf Romano (gone)]

Dão: can you inline the <html:canvas>? Hopefully flex="1" on a html element inside a xul box works...

Comment 6

[Dão Gottwald [:dao]]

Attachment: patch v2

This contains a couple of more changes. Most importantly, I reorganized the ifdefs (e.g., OSX didn't use updateColor() and non-OSX didn't use observe()). The plain diff is quite chaotic, hopefully bugzilla's side-by-side view makes it easier to follow.

(In reply to comment #5)
> Dão: can you inline the <html:canvas>? Hopefully flex="1" on a html element
> inside a xul box works...

I needed the straight opposite; see the new comment in the xul file.

Comment 7

[Asaf Romano (gone)]

+    //XXX set the dimensions explicitly to prevent canvas from going crazy
+    this._canvas.width = this._canvas.clientWidth;
+    this._canvas.height = this._canvas.clientHeight;

is this issue filed/tracked somewhere?

Comment 8

[Dão Gottwald [:dao]]

(In reply to comment #7)
> +    //XXX set the dimensions explicitly to prevent canvas from going crazy
> +    this._canvas.width = this._canvas.clientWidth;
> +    this._canvas.height = this._canvas.clientHeight;
> 
> is this issue filed/tracked somewhere?

After thinking about it I wouldn't consider this a bug, since width and height control the size of the coordinate space. I should change the comment.

Comment 9

[Dão Gottwald [:dao]]

Comment on attachment 271775 [details] [diff] [review]
patch v2

>+    //XXX set the dimensions explicitly to prevent canvas from going crazy
changed to:
>+    // set the size of the coordinate space

>+        <!-- width,height=1 makes canvas fit the monitor image instead of stretching the stack -->
changed to:
>+        <!-- if width and height are not present, they default to 300x150 and stretch the stack -->

Comment 10

[Dão Gottwald [:dao]]

Attachment: patch v3

Comments updated and <groupbox id="preview" align="center"> reverted to <groupbox align="center">. I hope that makes sense.

Comment 11

[Asaf Romano (gone)]

Comment on attachment 274933 [details] [diff] [review]
patch v3

Sorry for the huge delay.

>Index: browser/components/shell/content/setDesktopBackground.js
>===================================================================

>-const kXUL_NS           = "http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul";
>-const kHTML_NS          = "http://www.w3.org/1999/xhtml";
>-const kIShellService    = Components.interfaces.nsIShellService;
>-#ifdef XP_MACOSX
>-const kIMacShellService = Components.interfaces.nsIMacShellService;
>-#endif
>+const Ci = Components.interfaces;
>

"var", otherwise this will throw on OS X.


>+    // set an image as their desktop background.
>+    var bundle = document.getElementById("backgroundBundle");
>     var setDesktopBackground = document.getElementById("setDesktopBackground");
>     setDesktopBackground.hidden = false;
>-    var bundle = document.getElementById("backgroundBundle");

hrm?


> #ifndef XP_MACOSX
>-    <hbox align="center" id="foo">
>+    <hbox align="center">

:)

r=mano otherwise.

Comment 12

[Dão Gottwald [:dao]]

Attachment: for checkin

Comment 13

[Dão Gottwald [:dao]]

Attachment: for checkin

as usual, I forgot to include the jar.mn changes ...

Comment 14

[Nickolay_Ponomarev]

mozilla/browser/components/shell/content/setDesktopBackground.js   1.9
mozilla/browser/components/shell/content/setDesktopBackground.xul  1.5
mozilla/browser/themes/pinstripe/browser/jar.mn                    1.49
mozilla/browser/themes/pinstripe/browser/monitor.png               1.3
mozilla/browser/themes/pinstripe/browser/monitor_16-10.png         1.1
mozilla/browser/themes/pinstripe/browser/setDesktopBackground.css  1.4
mozilla/browser/themes/winstripe/browser/jar.mn                    1.45
mozilla/browser/themes/winstripe/browser/monitor_16-10.png         1.1
mozilla/browser/themes/winstripe/browser/setDesktopBackground.css  1.4

Let's hope I didn't mess up the check-in.

Comment 15

[Dão Gottwald [:dao]]

(In reply to comment #14)
> Let's hope I didn't mess up the check-in.

:)

You've mixed the monitor_16-10.png images, see http://mxr.mozilla.org/seamonkey/source/browser/themes/pinstripe/browser/ and http://mxr.mozilla.org/seamonkey/source/browser/themes/winstripe/browser/.

Comment 16

[Nickolay_Ponomarev]

Should be better now.

Comment 17

[Marcia Knous [:marcia]]

Litmus Triage: marcia will cover test case.