GNOME Bugzilla – Bug 719803
Various fixes for the background code
Last modified: 2013-12-04 00:23:21 UTC
Happened out of a memory leak investigation that ended up finding stuff like this... still working on the background cache
Created attachment 263443 [details] [review] background: Fix the check for spanning backgrounds this._monitorIndex does not exist, and neither does MetaBackground.monitor_index...
Created attachment 263444 [details] [review] background: Remove more bogus checks The effects on the image don't matter, as we'll re-set the effects when we copy the candidate content, and the content can never be null...
Created attachment 263445 [details] [review] background: Don't wait for gdk-pixbuf to fail before loading animations We don't have any better way of determining whether something is a slideshow animation, so discriminate on the .xml filename instead of waiting for gdk-pixbuf to determine whether it can load a file or not.
Review of attachment 263443 [details] [review]: looks right
Review of attachment 263444 [details] [review]: The point is to avoid having to reset the effects if we don't need to. Note, that check happens after the candidate is assigned so it still gets used if there's nothing better.
Review of attachment 263445 [details] [review]: ::: js/ui/background.js @@ +543,2 @@ cancellable: this._cancellable, onFinished: Lang.bind(this, function(content) { don't you still need to return if content is null? (image fails to load for other reason?)
Created attachment 263458 [details] [review] background: Fix the check for spanning backgrounds this._monitorIndex does not exist, and neither does MetaBackground.monitor_index...
Created attachment 263459 [details] [review] background: Remove more bogus checks The effects on the image don't matter, as we'll re-set the effects when we copy the candidate content, and the content can never be null...
Created attachment 263460 [details] [review] background: Don't wait for gdk-pixbuf to fail before loading animations We don't have any better way of determining whether something is a slideshow animation, so discriminate on the .xml filename instead of waiting for gdk-pixbuf to determine whether it can load a file or not.
Created attachment 263461 [details] [review] background: Remove the system noise content when not in use
Created attachment 263462 [details] [review] background: Remove unused variable
Created attachment 263463 [details] [review] background: Simplify animation code
Created attachment 263464 [details] [review] background: Clarify the intent of the code Stomping on local variables and trying to keep loop state isn't too fun. Just use a new variable here so we aren't too confused with what we're doing.
Created attachment 263465 [details] [review] background: Add copied content from pending image loads to the cache
Created attachment 263466 [details] [review] background: Don't silently fizzle out when removing bad content from the cache If the background is already removed, or we're trying to remove bad content, this is probably a bug in content accounting, so let us crash so we can fix the bugs.
Created attachment 263467 [details] [review] background: Remove more bogus checks The content in these arrays can never be null... -- Sorry, bugmail was down because of the maintenance, so I didn't see these reviews. The code here isn't too clear, and I even had to double-check that it is doing what you said it would, so I got confused.
Comment on attachment 263458 [details] [review] background: Fix the check for spanning backgrounds Attachment 263458 [details] pushed as 04ea950 - background: Fix the check for spanning backgrounds Nothing changed here, so it's still ACN. I just didn't see the review.
Review of attachment 263461 [details] [review]: sure
Review of attachment 263462 [details] [review]: ++
Review of attachment 263463 [details] [review]: this doesn't really seem to make the code simpler to me... but i'm pretty indifferent, if you feel strongly.
Review of attachment 263464 [details] [review]: ok
Review of attachment 263465 [details] [review]: Should have a justification like: "This way if the original is removed from the cache, any subsequent copies will be available in the cache for future loads"
Review of attachment 263466 [details] [review]: Okay. Do you have reason to believe this is happening?
Review of attachment 263466 [details] [review]: It was happening before, which is when we attempted to removeImageContent for the pendingFileLoad contents that were never added to the cache.
Review of attachment 263467 [details] [review]: i think that's right.
Review of attachment 263460 [details] [review]: ::: js/ui/background.js @@ +536,2 @@ cancellable: this._cancellable, onFinished: Lang.bind(this, function(content) { i think you still need if (!content) { return; } here. "Is a slide show" isn't the only reason content can be null, right?
Created attachment 263471 [details] [review] background: Don't wait for gdk-pixbuf to fail before loading animations We don't have any better way of determining whether something is a slideshow animation, so discriminate on the .xml filename instead of waiting for gdk-pixbuf to determine whether it can load a file or not.
Review of attachment 263471 [details] [review]: i think this is fine. Bonus points for falling back to gdk-pixbuf in case it's an svg file or something
Attachment 263461 [details] pushed as 7f3aadc - background: Remove the system noise content when not in use Attachment 263462 [details] pushed as adb49bd - background: Remove unused variable Attachment 263463 [details] pushed as 8875907 - background: Simplify animation code Attachment 263464 [details] pushed as 5262a41 - background: Clarify the intent of the code Attachment 263465 [details] pushed as 4cfb000 - background: Add copied content from pending image loads to the cache Attachment 263466 [details] pushed as 7249b11 - background: Don't silently fizzle out when removing bad content from the cache Attachment 263467 [details] pushed as dbdc884 - background: Remove more bogus checks Attachment 263471 [details] pushed as eb1c85f - background: Don't wait for gdk-pixbuf to fail before loading animations