Closed Bug 386163 Opened 13 years ago Closed 12 years ago

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

Categories

(Firefox :: General, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 3 alpha8

People

(Reporter: dao, Assigned: dao)

References

Details

Attachments

(4 files, 4 obsolete files)

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.
Attachment #270162 - Flags: ui-review?(beltzner)
Attachment #270163 - Flags: ui-review?(beltzner)
Attached patch patch (obsolete) — Splinter Review
Attachment #270700 - Flags: review?
Attachment #270700 - Flags: review? → review?(mano)
Dão: can you inline the <html:canvas>? Hopefully flex="1" on a html element inside a xul box works...
Attached patch patch v2 (obsolete) — Splinter Review
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.
Attachment #270700 - Attachment is obsolete: true
Attachment #271775 - Flags: review?(mano)
Attachment #270700 - Flags: review?(mano)
Attachment #270163 - Flags: ui-review?(beltzner) → ui-review+
Attachment #270162 - Flags: ui-review?(beltzner) → ui-review+
+    //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?
(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 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 -->
Attached patch patch v3 (obsolete) — Splinter Review
Comments updated and <groupbox id="preview" align="center"> reverted to <groupbox align="center">. I hope that makes sense.
Attachment #271775 - Attachment is obsolete: true
Attachment #274933 - Flags: review?(mano)
Attachment #271775 - Flags: review?(mano)
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.
Attachment #274933 - Flags: review?(mano) → review+
Attached patch for checkin (obsolete) — Splinter Review
Attachment #274933 - Attachment is obsolete: true
Keywords: checkin-needed
Whiteboard: don't forget the three monitor images
Attached patch for checkinSplinter Review
as usual, I forgot to include the jar.mn changes ...
Attachment #278883 - Attachment is obsolete: true
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.
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Flags: in-testsuite-
Keywords: checkin-needed
Resolution: --- → FIXED
Whiteboard: don't forget the three monitor images
Target Milestone: Firefox 3 M7 → Firefox 3 M8
(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/.
Should be better now.
Flags: in-litmus?
Litmus Triage: marcia will cover test case.
You need to log in before you can comment on or make changes to this bug.