4.10 KB, patch
Vidar Haarr (not reading bugmail): review+
Vidar Haarr (not reading bugmail): superreview+
Robert Kaiser: approval-seamonkey1.0+
|Details | Diff | Splinter Review|
894 bytes, patch
Robert Kaiser: review+
jag (Peter Annema): superreview+
|Details | Diff | Splinter Review|
According to my desktop property, all file names but "Mozilla Wallpaper" are written in Japanese. IIRC, default wallpapers in Windows95 were also in Japanese. I don't know much about MS GUI guidelines, but it's nice to remove this hard coding.. https://bugzilla.mozilla.org/show_bug.cgi?id=181273#c25
Created attachment 169797 [details] [diff] [review] patch v1 [suite, windows] (maybe already obsolete) Is this OS=ALL bug? Hmm, it seems more investigation needed...
OK, accoring to lxr, hardcoded "Wallpaper" is in http://lxr.mozilla.org/mozilla/source/xpfe/components/winhooks/nsWindowsHooks.cpp http://lxr.mozilla.org/mozilla/source/browser/components/shell/src/nsWindowsShellService.cpp http://lxr.mozilla.org/mozilla/source/browser/components/shell/src/nsGNOMEShellService.cpp That is, seamonkey Windows firefox Windows firefox Linux
Torisugari: Does this patch need to be updated to trunk, or is it ready for review ?
(In reply to comment #3) Sorry for the long absence. > Does this patch need to be updated to trunk, > or is it ready for review ? Well, imho, this patch is 99% ready. Only one thing: The locale data enty of the patch is located in mozilla/xpfe/components/winhooks/locale/en-US/nsWindowsHooks.properties However, if we are going to implement this wall paper image functionality on linux : bug 215532 (and mac and so on as well), nsWindowsHooks.properties may not be the proper file. And in that case, I'm not sure which file I should move the entry to, at all. Anybody any idea/info?
I asked because I wanted you to mark it "review?" from for example Dean or Neil :)
Comment on attachment 169797 [details] [diff] [review] patch v1 [suite, windows] (maybe already obsolete) >+ nsXPIDLString brandName, fileLeaf; You're allowed long variable names, you know... fileLeafName is much more readable. > nsCOMPtr<nsIStringBundleService> bundleService(do_GetService(bundleCID)); >+ NS_ENSURE_TRUE(bundleService, NS_ERROR_FAILURE); You should probably use the 2-arg form of do_GetService so you can return the correct rv on failure.
Created attachment 195277 [details] [diff] [review] patch v2 Patch updated to trunk and fixes issues mentioned in comment #6. Please note that I do not have a Windows environment and can't really test this patch, but it is only attachment 169797 [details] [diff] [review] updated to trunk (chrome://global/locale/brand.properties is now chrome://branding/locale/brand.properties) and two trivial changes, so it should be OK, provided email@example.com tested his original patch. :-)
Mano: Any thoughts on what to do about the various implementations that exist in toolkit for setting desktop background? The difference in the nsShellService::SetDesktopBackground implementations for the different platforms are crazy (toolkit). For example, for the filename, Windows uses a .properties string, Mac uses the image filename and GNOME uses $brandname_wallpaper.png. the mac impl also supports setting images that are not fully loaded yet as the background because it actually fires off a new SaveURI and implements nsIWebProgressListener, and it fires off a global notification "shell:desktop-background-changed" when it actually sets the background in OnStateChange.
Comment on attachment 195277 [details] [diff] [review] patch v2 >+ const PRUnichar* stringArray = brandName.get(); My superreview hat tells me that a PRUnichar* is not a string array. You could rename it, or you could tweak the code so that it is actually an array (in which case use NS_ARRAY_LENGTH instead of 1).
Created attachment 198911 [details] [diff] [review] version 2.1 Thank you, Neil. Please note, again, that I have no means of testing this patch, so reviewers should take special care and even check for compile errors/warnings.
Comment on attachment 198911 [details] [diff] [review] version 2.1 Code looks good, just a minor nit to pick. >Index: xpfe/components/winhooks/nsWindowsHooks.cpp >=================================================================== > nsCID bundleCID = NS_STRINGBUNDLESERVICE_CID; >+ nsCOMPtr<nsIStringBundleService> bundleService(do_GetService(bundleCID, &rv)); Just get rid of the nsCID stuff and use the CONTRACTID instead: nsCOMPtr<nsIStringBundleService> bundleService(do_GetService(NS_STRINGBUNDLE_CONTRACTID, &rv)); or if you care about the 80 columns style rule: nsCOMPtr<nsIStringBundleService> bundleService = do_GetService(NS_STRINGBUNDLE_CONTRACTID, &rv); I don't have a Windows tree I can build right now, let alone test this patch, so if you make that change please have whoever's gonna check this in do a test build first :-) sr=jag
Created attachment 198982 [details] [diff] [review] patch for checkin, I hope. The nsCID has a brother, so I killed him too. This file doesn't care very much about wrapping on 80, but it may look like it cares sometimes. Personally, I don't care.
Fix checked in.
Should the ".bmp" part really be localizable? Also, shouldn't there be a localization note about what the %S means?
I agree about comment 14, so feel free to reopen. -> verified
Good call, biesi. The .bmp part doesn't need to be localizable and probably shouldn't be. Vidar, would you mind addressing that, and adding a quick note to nsWindowsHooks.properties to explain what the %S there stands for?
Created attachment 199110 [details] [diff] [review] adds localization notes Adds a localization note to wallpaperFile and promptText. Neil had already done the ".bmp" appending before checking in attachment 198982 [details] [diff] [review].
Comment on attachment 199110 [details] [diff] [review] adds localization notes Thos L10n note look good to me ;-)
Comment on attachment 199110 [details] [diff] [review] adds localization notes Thanks for the reviews! If someone could help me check this in, that would be great.
L10n notes checked in as well, thanks!
Comment on attachment 198982 [details] [diff] [review] patch for checkin, I hope. (including the L10N notes of course)
Comment on attachment 198982 [details] [diff] [review] patch for checkin, I hope. a=me given you check it in with the same modifications as done on trunk
Please advise if you want me to provide a patch for branch checkin.
Err.. Comment 23 was mine. Yay for public access computers.
(In reply to comment #23) >Please advise if you want me to provide a patch for branch checkin. This case was particularly easy as there were no other changes on trunk.