Closed
Bug 653333
Opened 13 years ago
Closed 13 years ago
Remove legacy splash screen code (nsSplashScreen, MOZ_SPLASHSCREEN, splash.bmp) now that no platforms use it
Categories
(Core :: General, defect)
Core
General
Tracking
()
RESOLVED
FIXED
mozilla8
People
(Reporter: emorley, Assigned: emorley)
References
Details
Attachments
(1 file, 2 obsolete files)
22.38 KB,
patch
|
emorley
:
review+
|
Details | Diff | Splinter Review |
My patch in bug 652445 (once it lands) removes nsSplashScreenWin.cpp, since it was only used under WinCE. This leaves all other platforms using nsSplashScreenDummy.cpp: http://mxr.mozilla.org/mozilla-central/source/toolkit/xre/Makefile.in#77 ...which does nothing but return null: http://mxr.mozilla.org/mozilla-central/source/toolkit/xre/nsSplashScreenDummy.cpp These files can thus be removed: - /toolkit/xre/nsSplashScreenDummy.cpp - /toolkit/xre/nsSplashScreen.h Which leaves MOZ_SPLASHSCREEN unused, so can be removed: http://mxr.mozilla.org/mozilla-central/search?string=MOZ_SPLASHSCREEN And also several splash.bmp files unused, so can be removed: http://mxr.mozilla.org/mozilla-central/find?string=splash.bmp&tree=mozilla-central
Assignee | ||
Comment 1•13 years ago
|
||
Note: The Android splash screen doesn't use nsSplashScreen or MOZ_SPLASHSCREEN; so isn't affected: http://mxr.mozilla.org/mozilla-central/source/embedding/android/GeckoApp.java#140 http://mxr.mozilla.org/mozilla-central/source/embedding/android/GeckoSurfaceView.java#103
Assignee | ||
Updated•13 years ago
|
Assignee: nobody → bmo
Status: NEW → ASSIGNED
Assignee | ||
Comment 2•13 years ago
|
||
Remove all #ifdef MOZ_SPLASHSCREEN code & hg rm the following: - browser/branding/*/splash.bmp - mobile/app/splash.bmp - toolkit/xre/nsSplashScreen.h - toolkit/xre/nsSplashScreenDummy.cpp Passed try: http://dev.philringnalda.com/tbpl/?tree=Try&rev=890f09eb3976 Dave: Not sure who to ask for review, since this touches a few areas + looking at the hg logs wasn't conclusive. Picked you due to: https://wiki.mozilla.org/Modules/Toolkit#Toolkit ; but please let me know if I should be asking someone else. Thanks! (Note: This patch applies on top of the ones in bug 652445, which haven't yet landed).
Attachment #528853 -
Flags: review?(dtownsend)
Comment 3•13 years ago
|
||
Comment on attachment 528853 [details] [diff] [review] Patch v1 I'd like Benjamin to assert he's ok with this then I think it's good to go.
Attachment #528853 -
Flags: review?(dtownsend)
Attachment #528853 -
Flags: review?(benjamin)
Attachment #528853 -
Flags: review+
Comment 4•13 years ago
|
||
Comment on attachment 528853 [details] [diff] [review] Patch v1 I am 99% sure that the Android splash screen does not use this code, but please confirm that with mfinkle before landing this.
Attachment #528853 -
Flags: review?(benjamin) → review+
Assignee | ||
Comment 5•13 years ago
|
||
Mark, bsmedberg has asked that I check with you regarding comment 4. The MXR links in comment 1 make me believe that Android does not use nsSplashScreen - but if you could confirm here, that would be great. Thanks!
Comment 6•13 years ago
|
||
Android does not use this code for a splashscreen. We use the Java loader code to display a splashscreen, before the Mozilla platform is really even initialized. However, here is a patch to add splash screen support to maemo/meego and it uses this code. See bug 643122. Lets hold off on removing the splashscreen support until we see if Meego, and any other new mobile platforms, might need it.
Assignee | ||
Comment 7•13 years ago
|
||
Thanks for the heads up. Suspect even if bug 643122 lands in it's nsSplashScreen using form, there are going to be unused splash.bmp files from the WinCE era (eg the nightly/aurora splash.bmp are out of date); but will just be easier to wait until the dust settles on that bug before moving this one further...
Assignee | ||
Updated•13 years ago
|
Flags: in-testsuite-
Assignee | ||
Comment 8•13 years ago
|
||
Only change from previously r+'d patch are the two lines in mobile/app/Makefile.in - requesting review just for those. Thanks! :-) http://dev.philringnalda.com/tbpl/?tree=Try&rev=bea4b20fdc0b
Attachment #528853 -
Attachment is obsolete: true
Attachment #546887 -
Flags: review?(dtownsend)
Updated•13 years ago
|
Attachment #546887 -
Flags: review?(dtownsend) → review+
Assignee | ||
Comment 9•13 years ago
|
||
Updated to tip. No other changes, so carrying forwards r+. Sent to try again for build only, for sanity check: http://dev.philringnalda.com/tbpl/?tree=Try&rev=d57322b447c4 See comment 2 for older full try run. Thanks for the review Dave :-)
Attachment #546887 -
Attachment is obsolete: true
Attachment #549876 -
Flags: review+
Assignee | ||
Updated•13 years ago
|
Keywords: checkin-needed
Updated•13 years ago
|
Keywords: checkin-needed
Whiteboard: [inbound]
Assignee | ||
Comment 10•13 years ago
|
||
http://hg.mozilla.org/mozilla-central/rev/3a3e648530b2
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Whiteboard: [inbound]
Target Milestone: --- → mozilla8
Assignee | ||
Comment 11•13 years ago
|
||
For reference, this effectively reverts (the still present parts of) the following: http://hg.mozilla.org/mozilla-central/rev/31fd342897c7 http://hg.mozilla.org/mozilla-central/rev/6004e947157b http://hg.mozilla.org/mozilla-central/rev/c4678bf7b3fb http://hg.mozilla.org/mozilla-central/rev/167d89514f81 http://hg.mozilla.org/mobile-browser/rev/09a352cfd5fe Adding deps to help future history tracking.
You need to log in
before you can comment on or make changes to this bug.
Description
•