Closed Bug 1039866 Opened 10 years ago Closed 9 years ago

Remove the metro browser from mozilla-central

Categories

(Firefox for Metro Graveyard :: Build Config, defect)

defect
Not set
normal

Tracking

(firefox40 fixed)

RESOLVED FIXED
Firefox 40
Tracking Status
firefox40 --- fixed

People

(Reporter: khuey, Assigned: kats)

References

Details

Attachments

(5 files, 8 obsolete files)

329.93 KB, patch
jimm
: review+
Details | Diff | Splinter Review
4.87 MB, patch
Gavin
: review+
Details | Diff | Splinter Review
2.03 MB, patch
jimm
: review+
Gavin
: review+
robert.strong.bugs
: review+
Details | Diff | Splinter Review
497.05 KB, patch
Gavin
: review+
jimm
: review+
robert.strong.bugs
: review+
Details | Diff | Splinter Review
5.37 KB, text/plain
Details
It messes with my MXR search results to see what APIs/etc are used.  And it's always in version control if we want it back.
No objections here.  We have a few people working on unofficial projects based on metrofox, but we encourage them to use https://hg.mozilla.org/projects/metro as a base, rather than mozilla-central (since we have no protection against regressions on m-c).  We can keep the code alive there, and merge core changes from m-c to projects/metro periodically.
(In reply to Kyle Huey [:khuey] (khuey@mozilla.com) from comment #0)
> It messes with my MXR search results to see what APIs/etc are used.  And
> it's always in version control if we want it back.

Are you suggesting removing browser/metro or everything idfef'd MOZ_METRO? If we can keep platform for another year or so to see, that would make merges much simpler.
Blocks: 1042108
In my humble opinion, if metro will be only live in project/metro branch, in case of big changing m-c, supporting of work MetroFF will be harder and more increase.
Objections: There are several issues in m-c at current time. This issues blocking merging from m-c to project/metro branch. As result we have old version of MetroFF in project/metro branch and we can not get new updates from m-c. That's why I think that MetroFF should live in m-c and should be compiled possibly always (for example, with flag --enable-metro). Only in this case project/metro branch can have stable version of MetroFF.
Having Metro in mozilla-central is not the right cost/benefit tradeoff.

I'd like to at least remove browser/metro, removing all of the #ifdef MOZ_METRO code is probably less important.
I can see Gavin's point. We have to consider the larger project group too. Having Metro in m-c means there is more code to worry about during refactors, more code to consider when adding features, and more code that just continues to fall out of date.

I don't object to removing it.
The work to create a reference implementation of Pointer Events in Firefox for Metro was basically completed a few weeks ago.  If any followups are necessary in the future we could use the "metro" project branch.  I have no more objections to removing the code from m-c.
What is the scope of this bug? Should dependent bugs be created?

To remove?
/browser/metro (removes 58k LOC)
metro searchplugins
widget/windows/winrt, and cleanup more widget stuff
code ifdef-ed with MOZ_METRO
code which checks for Services.metro
code which checks for (XRE_GetWindowsEnvironment() == WindowsEnvironmentType_Metro)
toolkit code related to metro (e.g. updater)
metro tests
etc.
browser/metro is my primary concern. Less concerned about the other things you list, though eventually those should go too.
Successful Try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=5764d1fc551b
Assignee: nobody → archaeopteryx
Status: NEW → ASSIGNED
Attachment #8562000 - Flags: review?(jmathies)
Comment on attachment 8562000 [details] [diff] [review]
patch removing /browser/metro, v1

Seems rather sloppy ripping out the easy part and ignoring the rest? Sounds like Gavin wants this part gone though, so transferring the review request over to him.
Attachment #8562000 - Flags: review?(jmathies) → review?(gavin.sharp)
Comment on attachment 8562000 [details] [diff] [review]
patch removing /browser/metro, v1

I think we should be a bit more thorough than just a straight hg rm of browser/metro, assuming that's what this is. I think cleaning up all of browser/ at once is probably a good first step.

I think that means that beyond this, we should also audit all of the results at https://mxr.mozilla.org/mozilla-central/search?string=metro&find=browser%2F outside of browser/metro and see what else should be removed.
Attachment #8562000 - Flags: review?(gavin.sharp) → feedback+
Assignee: archaeopteryx → jmathies
There's some benefit in splitting this bug into the easy "delete the files" part and the harder "wipe up all the blood" part -- doing the former unblocks other untangling where the metro code has some dependency, such as Bug 1148979.

Unless someone wants to green-light landing changes in the mean time that outright break code in browser/metro, of course…
(In reply to Richard Newman [:rnewman] from comment #13)
> Unless someone wants to green-light landing changes in the mean time that
> outright break code in browser/metro, of course…

Code in browser/metro is pretty much broken already. I've reviewed and wrote patches that did that and I'm sure other people did the same.
(In reply to Richard Newman [:rnewman] from comment #13)
> Unless someone wants to green-light landing changes in the mean time that
> outright break code in browser/metro, of course…

Such changes have been green lighted for a while now.
\o/
No longer blocks: 1148979
Attached patch Part 2 - Remove browser/metro (obsolete) — Splinter Review
Attachment #8562000 - Attachment is obsolete: true
I think there might still be a bunch of JS and related assets floating around, but this gets rid of the bulk of it. Haven't tried compiling anything yet.
Updated to remove more metro stuff. There's still a bit left in browser/installer/windows/nsis/shared.nsh which I'm not sure what to do with, and it looks scary to touch.
Attachment #8592882 - Attachment is obsolete: true
Attached file References to metro still left (obsolete) —
Here's a case-insensitive grep of stuff still left in the tree after applying the three patches above. Apparently still a lot left :(
Attached file References to metro still left (obsolete) —
Whoops, wrong version
Attachment #8592895 - Attachment is obsolete: true
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #20)
> Created attachment 8592887 [details] [diff] [review]
> Part 3 - Remove MOZ_METRO and friends
> 
> Updated to remove more metro stuff. There's still a bit left in
> browser/installer/windows/nsis/shared.nsh which I'm not sure what to do
> with, and it looks scary to touch.

I think you can safely kick this out to a followup if you don't want to mess with it.

Thanks for working on this btw!
I updated part 3 a bit more to remove stuff I thought was safe, and did a try push. *fingers crossed*

https://treeherder.mozilla.org/#/jobs?repo=try&revision=794a002a5a87
Thanks for digging into this, kats!

Bug 1151598 fixed some of the toolkit/modules hits from attachment 8592896 [details].
I have become sad. MetroFireFox, I will miss You!
Better try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=626480d08d1b. Once that's done I'll upload the latest patches.
Assignee: jmathies → bugmail.mozilla
Attachment #8592880 - Attachment is obsolete: true
Attachment #8593401 - Flags: review?(jmathies)
Attachment #8592881 - Attachment is obsolete: true
Attachment #8593402 - Flags: review?(gavin.sharp)
I don't know who all needs to review this. Here's the list of files I touched. If you want me to split it up into multiple patches let me know.

CLOBBER
accessible/windows/msaa/AccessibleWrap.cpp
browser/app-rules.mk
browser/app/moz.build
browser/app/nsBrowserApp.cpp
browser/app/profile/firefox.js
browser/base/content/browser-menubar.inc
browser/base/content/browser.js
browser/base/content/sync/setup.js
browser/base/content/sync/setup.xul
browser/branding/aurora/Makefile.in
browser/branding/aurora/content/Makefile.in
browser/branding/aurora/content/jar.mn
browser/branding/aurora/content/metro-about-footer.png
browser/branding/aurora/content/metro-about-wordmark.png
browser/branding/aurora/content/metro-about.css
browser/branding/aurora/content/metro_firstrun_logo.png
browser/branding/aurora/content/metro_firstrun_logo@1.4x.png
browser/branding/aurora/content/metro_firstrun_logo@1.8x.png
browser/branding/nightly/Makefile.in
browser/branding/nightly/content/Makefile.in
browser/branding/nightly/content/jar.mn
browser/branding/nightly/content/metro-about-footer.png
browser/branding/nightly/content/metro-about-wordmark.png
browser/branding/nightly/content/metro-about.css
browser/branding/nightly/content/metro_firstrun_logo.png
browser/branding/nightly/content/metro_firstrun_logo@1.4x.png
browser/branding/nightly/content/metro_firstrun_logo@1.8x.png
browser/branding/official/Makefile.in
browser/branding/official/content/Makefile.in
browser/branding/official/content/jar.mn
browser/branding/official/content/metro-about-footer.png
browser/branding/official/content/metro-about-wordmark.png
browser/branding/official/content/metro-about.css
browser/branding/official/content/metro_firstrun_logo.png
browser/branding/official/content/metro_firstrun_logo@1.4x.png
browser/branding/official/content/metro_firstrun_logo@1.8x.png
browser/branding/unofficial/Makefile.in
browser/branding/unofficial/content/Makefile.in
browser/branding/unofficial/content/jar.mn
browser/branding/unofficial/content/metro-about-footer.png
browser/branding/unofficial/content/metro-about-wordmark.png
browser/branding/unofficial/content/metro-about.css
browser/branding/unofficial/content/metro_firstrun_logo.png
browser/branding/unofficial/content/metro_firstrun_logo@1.4x.png
browser/branding/unofficial/content/metro_firstrun_logo@1.8x.png
browser/build.mk
browser/components/customizableui/CustomizableUI.jsm
browser/components/customizableui/CustomizableWidgets.jsm
browser/components/customizableui/test/browser_876944_customize_mode_create_destroy.js
browser/components/customizableui/test/browser_880382_drag_wide_widgets_in_panel.js
browser/components/customizableui/test/browser_890140_orphaned_placeholders.js
browser/components/customizableui/test/head.js
browser/components/preferences/advanced.js
browser/components/preferences/advanced.xul
browser/components/preferences/in-content/advanced.js
browser/components/preferences/in-content/advanced.xul
browser/components/preferences/in-content/main.js
browser/components/preferences/main.js
browser/extensions/Makefile.in
browser/installer/package-manifest.in
browser/installer/windows/nsis/defines.nsi.in
browser/installer/windows/nsis/installer.nsi
browser/installer/windows/nsis/shared.nsh
browser/installer/windows/nsis/uninstaller.nsi
browser/locales/Makefile.in
browser/locales/en-US/chrome/browser/browser.dtd
browser/locales/en-US/chrome/browser/customizableui/customizableWidgets.properties
browser/locales/en-US/chrome/browser/preferences/advanced.dtd
browser/locales/en-US/chrome/browser/syncSetup.dtd
browser/locales/en-US/profile/bookmarks.inc
browser/locales/en-US/searchplugins/bingmetrofx.xml
browser/locales/en-US/searchplugins/googlemetrofx.xml
browser/locales/en-US/searchplugins/metrolist.txt
browser/locales/en-US/searchplugins/wikipediametrofx.xml
browser/locales/en-US/searchplugins/yahoometrofx.xml
browser/locales/filter.py
browser/modules/BrowserUITelemetry.jsm
browser/moz.build
browser/themes/shared/browser.inc
browser/themes/windows/Metro_Glyph-inverted.png
browser/themes/windows/Metro_Glyph-menuPanel.png
browser/themes/windows/Metro_Glyph.png
browser/themes/windows/browser.css
browser/themes/windows/jar.mn
build/automation.py.in
build/win32/mozconfig.vs2010-win64
build/win32/mozconfig.vs2013-win64
build/win64/mozconfig.vs2010
build/win64/mozconfig.vs2013
config/recurse.mk
configure.in
dom/base/nsContentUtils.h
dom/svg/SVGDocument.cpp
gfx/doc/AsyncPanZoom.md
gfx/layers/d3d11/CompositorD3D11.cpp
gfx/thebes/gfxWindowsPlatform.cpp
ipc/glue/WindowsMessageLoop.cpp
js/src/configure.in
layout/reftests/forms/input/color/reftest.list
modules/libpref/Preferences.cpp
profile/dirserviceprovider/nsProfileDirServiceProvider.cpp
startupcache/StartupCache.cpp
testing/mochitest/Makefile.in
testing/mozbase/mozrunner/mozrunner/runners.py
testing/profiles/prefs_general.js
toolkit/components/crashmonitor/CrashMonitor.jsm
toolkit/components/downloads/nsDownloadManager.cpp
toolkit/components/jsdownloads/src/DownloadIntegration.jsm
toolkit/components/startup/nsAppStartup.cpp
toolkit/components/startup/nsAppStartup.h
toolkit/components/startup/public/nsIAppStartup.idl
toolkit/content/widgets/browser.xml
toolkit/crashreporter/nsExceptionHandler.cpp
toolkit/library/moz.build
toolkit/locales/en-US/chrome/places/places.properties
toolkit/mozapps/installer/windows/nsis/common.nsh
toolkit/mozapps/update/common/updatehelper.cpp
toolkit/mozapps/update/common/updatehelper.h
toolkit/mozapps/update/nsIUpdateService.idl
toolkit/mozapps/update/tests/chrome/utils.js
toolkit/mozapps/update/updater/updater.cpp
toolkit/webapps/WebappOSUtils.jsm
toolkit/xre/nsAppRunner.cpp
toolkit/xre/nsEmbedFunctions.cpp
toolkit/xre/nsUpdateDriver.cpp
toolkit/xre/nsWindowsWMain.cpp
toolkit/xre/nsXREDirProvider.cpp
tools/profiler/platform.cpp
uriloader/exthandler/nsExternalHelperAppService.cpp
widget/MetroUIUtils.idl
widget/NativeKeyToDOMKeyName.h
widget/moz.build
widget/nsAppShellSingleton.h
widget/nsIWinMetroUtils.idl
widget/nsWidgetsCID.h
widget/windows/KeyboardLayout.cpp
widget/windows/WinMouseScrollHandler.cpp
widget/windows/WinTaskbar.cpp
widget/windows/WinUtils.cpp
widget/windows/moz.build
widget/windows/nsClipboard.cpp
widget/windows/nsLookAndFeel.cpp
widget/windows/nsTextStore.cpp
widget/windows/nsTextStore.h
widget/windows/nsWidgetFactory.cpp
widget/windows/nsWindow.cpp
xpcom/base/ErrorList.h
xpcom/base/nsDebugImpl.cpp
xpcom/base/nsSystemInfo.cpp
xpcom/base/nsWindowsHelpers.h
xpcom/build/IOInterposer.cpp
xpcom/build/nsXULAppAPI.h
xpcom/io/nsAppDirectoryServiceDefs.h
xpcom/io/nsLocalFileWin.cpp
Attachment #8592887 - Attachment is obsolete: true
Attachment #8593404 - Flags: review?(jmathies)
Attachment #8593404 - Flags: review?(gavin.sharp)
Attached patch References to metro still left (obsolete) — Splinter Review
And here's an updated list of references still left.
Attachment #8592896 - Attachment is obsolete: true
Attachment #8593401 - Flags: review?(jmathies) → review+
Comment on attachment 8593404 [details] [diff] [review]
Part 3 - Remove MOZ_METRO and friends

Review of attachment 8593404 [details] [diff] [review]:
-----------------------------------------------------------------

cc'ing rstrong on the updater and installer changes.

::: browser/branding/aurora/Makefile.in
@@ -48,5 @@
>  BRANDING_TARGET := export
>  INSTALL_TARGETS += BRANDING
> -
> -ifeq ($(MOZ_WIDGET_TOOLKIT) $(DIST_SUBDIR),windows metro)
> -VISUALMANIFEST := VisualElementsManifest.xml

did these files get deleted somewhere, I'm not seeing that.

::: browser/branding/aurora/content/Makefile.in
@@ -6,5 @@
>  #  - jars chrome artwork
> -
> -# resources needed for the metro tile interface
> -ifeq ($(MOZ_WIDGET_TOOLKIT) $(DIST_SUBDIR),windows metro)
> -TILE_FILES := $(wildcard $(srcdir)/VisualElements*)

ditto?
Attachment #8593404 - Flags: review?(robert.strong.bugs)
Attachment #8593404 - Flags: review?(jmathies)
Attachment #8593404 - Flags: review+
Comment on attachment 8593402 [details] [diff] [review]
Part 2 - Remove browser/metro

r=me assuming this is a straight "hg rm browser/metro".
Attachment #8593402 - Flags: review?(gavin.sharp) → review+
Comment on attachment 8593409 [details] [diff] [review]
References to metro still left

>./browser/app/profile/firefox.js:// incompatibilities are ignored by updates in Metro

Should just remove this line.

>./browser/base/content/browser.js:      // Pulls in Metro controlled prefs and pushes out Desktop controlled prefs

We should remove all references to WindowsPrefSync (which covers this comment).

>./services/datareporting/DataReporting.manifest:#   metro browser:  {99bceaaa-e3c6-48c1-b981-ef9b46b67d60}
>./services/sync/SyncComponents.manifest:#   metro browser:  {99bceaaa-e3c6-48c1-b981-ef9b46b67d60}
>./services/sync/tps/extensions/mozmill/resource/driver/controller.js:    case "MetroFirefox":
>./services/sync/tps/extensions/mozmill/resource/driver/controller.js:    case "MetroFirefox":
>./services/sync/tps/extensions/mozmill/resource/driver/controller.js:    case "MetroFirefox":
>./services/sync/tps/extensions/mozmill/resource/stdlib/utils.js:  '{99bceaaa-e3c6-48c1-b981-ef9b46b67d60}': 'MetroFirefox'
>./services/sync/tps/extensions/mozmill/resource/stdlib/utils.js:    case "MetroFirefox":
>./testing/mochitest/browser-harness.xul:                    gConfig.testRoot == "metro" ? "navigator:browser" :
>./testing/mochitest/browser-test.js:      gConfig.testRoot == "metro" ||
>./toolkit/components/crashmonitor/CrashMonitor.jsm:    if (Services.metro && Services.metro.immersive) {
>./toolkit/components/crashmonitor/CrashMonitor.jsm:      OS.File.makeDir(OS.Path.join(OS.Constants.Path.profileDir, "metro"));
>./toolkit/mozapps/update/tests/data/shared.js:const PREF_APP_UPDATE_METRO_ENABLED       = "app.update.metro.enabled";
>./toolkit/mozapps/update/tests/data/xpcshellUtilsAUS.js:  Services.prefs.setBoolPref(PREF_APP_UPDATE_METRO_ENABLED, true);
>./toolkit/mozapps/update/updater/updater.cpp:      bool updateFromMetro = false;
>./toolkit/mozapps/update/updater/updater.cpp:      DWORD waitTime = updateFromMetro ?
>./toolkit/mozapps/update/updater/updater.cpp:      if (result != WAIT_OBJECT_0 && !updateFromMetro)

At least all of these should just be ripped out too. Maybe best in another bug.
Comment on attachment 8593404 [details] [diff] [review]
Part 3 - Remove MOZ_METRO and friends

You should be able to remove these as well
./toolkit/mozapps/update/tests/data/shared.js:const PREF_APP_UPDATE_METRO_ENABLED       = "app.update.metro.enabled";
./toolkit/mozapps/update/tests/data/xpcshellUtilsAUS.js:  Services.prefs.setBoolPref(PREF_APP_UPDATE_METRO_ENABLED, true);
Please remove these if you don't mind though I am ok with doing them in a new bug.

The remaining changes needed to the installer and updater I'm fine with doing in new bugs.
Attachment #8593404 - Flags: review?(robert.strong.bugs) → review+
(In reply to Jim Mathies [:jimm] from comment #32)
> 
> did these files get deleted somewhere, I'm not seeing that.
> 

Whoops, I missed those. I'll add a new patch with the results of:
  find . -name "VisualElements*" | xargs git rm
which removed those xml files as well as the png files they referenced.

(In reply to :Gavin Sharp [email: gavin@gavinsharp.com] from comment #33)
> r=me assuming this is a straight "hg rm browser/metro".

It is, yes.

(In reply to :Gavin Sharp [email: gavin@gavinsharp.com] from comment #34)
> >./browser/app/profile/firefox.js:// incompatibilities are ignored by updates in Metro
> 
> Should just remove this line.

Done

> >./browser/base/content/browser.js:      // Pulls in Metro controlled prefs and pushes out Desktop controlled prefs
> 
> We should remove all references to WindowsPrefSync (which covers this
> comment).

Done

> At least all of these should just be ripped out too. Maybe best in another
> bug.

Done

All the above comments are addressed in this part 3b, which I'll squash into part 3 on landing. Kept it separate for easier review. I'll do another try push as well.
Attachment #8593718 - Flags: review?(jmathies)
Attachment #8593718 - Flags: review?(gavin.sharp)
Attachment #8593722 - Attachment is patch: false
Attachment #8593718 - Flags: review?(jmathies) → review+
Comment on attachment 8593718 [details] [diff] [review]
Part 3b - More removal

Looks good to me!
Attachment #8593718 - Flags: review+
Thanks! Am I good to land this now, or is there somebody else who should still review these changes?
(gavin said on IRC he'd find somebody, but not sure if that was :rstrong or somebody else...)
Comment on attachment 8593404 [details] [diff] [review]
Part 3 - Remove MOZ_METRO and friends

>diff --git a/browser/branding/aurora/Makefile.in b/browser/branding/aurora/Makefile.in

>-# On Windows only do this step for browser, skip for metro.
> ifeq ($(MOZ_WIDGET_TOOLKIT) $(DIST_SUBDIR),windows browser)

You can remove the DIST_SUBDIR check and make this just:

ifeq ($(MOZ_WIDGET_TOOLKIT),windows)

Same comment applies to the other branding package Makefile.ins (browser/branding/nightly/Makefile.in, browser/branding/official/Makefile.in, browser/branding/unofficial/Makefile.in, etc.)

I haven't reviewed this fully, I will try to get back to it soon.
Should someone like Mark Mayo have the opportunity to sign off on this as well?
Attachment #8593718 - Flags: review?(gavin.sharp) → review+
Looking at patch part 3, I'm a bit surprised that we seem to have some examples of code in (XRE_GetWindowsEnvironment() == WindowsEnvironmentType_Metro) conditionals that is further #ifdef'ed MOZ_METRO.

Was it really possible to somehow run a non-MOZ_METRO build such that (XRE_GetWindowsEnvironment() == WindowsEnvironmentType_Metro), or was that just guaranteed dead code?
Flags: needinfo?(jmathies)
Comment on attachment 8593404 [details] [diff] [review]
Part 3 - Remove MOZ_METRO and friends

>diff --git a/browser/locales/en-US/chrome/browser/preferences/advanced.dtd b/browser/locales/en-US/chrome/browser/preferences/advanced.dtd

>-<!-- Note either updateAuto1 is used or (updateAutoMetro and updateAutoDesktop),
>+<!-- Note either updateAuto1 is used or updateAutoDesktop,

The updateAutoDesktop strings are no longer used after this patch, and so they can be removed also. You can then just remove this L10N note entirely.

>diff --git a/config/recurse.mk b/config/recurse.mk

> # The rules here are doing directory traversal, so we don't want further
> # recursion to happen when running make -C subdir $tier. But some make files
> # further call make -C something else, and sometimes expect recursion to
>-# happen in that case (see browser/metro/locales/Makefile.in for example).
>+# happen in that case.
> # Conveniently, every invocation of make increases MAKELEVEL, so only stop
> # recursion from happening at current MAKELEVEL + 1.

Was that Metro Makefile the only Makefile that did this? Perhaps we can rip out more of this stuff, or find another example? Separate bug.

>diff --git a/dom/base/nsContentUtils.h b/dom/base/nsContentUtils.h

>    * Making the fullscreen API content only is useful on platforms where we
>    * still want chrome to be visible or accessible while content is
>-   * fullscreen, like on Windows 8 in Metro mode.
>+   * fullscreen.

Looking at this code, Metro seems to be the only thing that sets full-screen-api.content-only=true. So we can probably rip out a bunch more related code. This can be a new bug too.

>diff --git a/testing/profiles/prefs_general.js b/testing/profiles/prefs_general.js

>-// prefs for firefox metro.
> // Disable first-tun tab
> user_pref("browser.firstrun.count", 0);

At least this pref (and maybe some of the ones after it?) is Metro-only and can be removed.

>diff --git a/toolkit/mozapps/update/nsIUpdateService.idl b/toolkit/mozapps/update/nsIUpdateService.idl

>   readonly attribute boolean isOtherInstanceHandlingUpdates;

This stuff could probably all be ripped out too, though maybe we want to leave it. Separate bug.

>diff --git a/toolkit/xre/nsAppRunner.cpp b/toolkit/xre/nsAppRunner.cpp

>+ * @return true in all environments
> */
> static bool
> CanShowProfileManager()

This is probably not useful anymore?

>diff --git a/tools/profiler/platform.cpp b/tools/profiler/platform.cpp

> static const char * gGeckoThreadName = "GeckoMain";

This may no longer be useful either, maybe also worth a separate bug.

r=me with those addressed / bugs filed.
Attachment #8593404 - Flags: review?(gavin.sharp) → review+
(In reply to :Gavin Sharp [email: gavin@gavinsharp.com] from comment #45)
>...
> >diff --git a/toolkit/mozapps/update/nsIUpdateService.idl b/toolkit/mozapps/update/nsIUpdateService.idl
> 
> >   readonly attribute boolean isOtherInstanceHandlingUpdates;
> 
> This stuff could probably all be ripped out too, though maybe we want to
> leave it. Separate bug.
That is definitely not for metro so no need.
(In reply to :Gavin Sharp [email: gavin@gavinsharp.com] from comment #44)
> Looking at patch part 3, I'm a bit surprised that we seem to have some
> examples of code in (XRE_GetWindowsEnvironment() ==
> WindowsEnvironmentType_Metro) conditionals that is further #ifdef'ed
> MOZ_METRO.
> 
> Was it really possible to somehow run a non-MOZ_METRO build such that
> (XRE_GetWindowsEnvironment() == WindowsEnvironmentType_Metro), or was that
> just guaranteed dead code?

The reverse was true, a MOZ_METRO build where XRE_GetWindowsEnvironment() != WindowsEnvironmentType_Metro. Looking at one example here, the modules/libpref/Preferences.cpp code.
Flags: needinfo?(jmathies)
(In reply to :Gavin Sharp [email: gavin@gavinsharp.com] from comment #45)
> The updateAutoDesktop strings are no longer used after this patch, and so
> they can be removed also. You can then just remove this L10N note entirely.

I removed this in my local patches.

> > # Conveniently, every invocation of make increases MAKELEVEL, so only stop
> > # recursion from happening at current MAKELEVEL + 1.
> 
> Was that Metro Makefile the only Makefile that did this? Perhaps we can rip
> out more of this stuff, or find another example? Separate bug.

Filed bug 1157684 for this.

> Looking at this code, Metro seems to be the only thing that sets
> full-screen-api.content-only=true. So we can probably rip out a bunch more
> related code. This can be a new bug too.

Filed bug 1157685 for this.

> > user_pref("browser.firstrun.count", 0);
> 
> At least this pref (and maybe some of the ones after it?) is Metro-only and
> can be removed.

Removed this locally. The next few prefs seem to be not Metro-only so I left that as-is. If you have specific prefs in mind let me know.

> >diff --git a/toolkit/mozapps/update/nsIUpdateService.idl b/toolkit/mozapps/update/nsIUpdateService.idl
> 
> >   readonly attribute boolean isOtherInstanceHandlingUpdates;
> 
> This stuff could probably all be ripped out too, though maybe we want to
> leave it. Separate bug.

Didn't file a bug for this, as per comment 46.

> > CanShowProfileManager()
> 
> This is probably not useful anymore?

Filed bug 1157686 for this.

> > static const char * gGeckoThreadName = "GeckoMain";
> 
> This may no longer be useful either, maybe also worth a separate bug.

This looks like it is used in the profiler for other platforms, so I left it in. (I've definitely seen threads labelled GeckoMain in non-metro profiles, so I'm pretty sure this is used).

Did a new try-push with the above changes, parts 3+4 squashed, and rebased to latest m-c:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=8e17f6dc0ec8

I'll land it if the try run is green.
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #48)
> Removed this locally. The next few prefs seem to be not Metro-only so I left
> that as-is. If you have specific prefs in mind let me know.

Some grepping showed that loop.throttled and browser.uitour.pinnedTabUrl are two prefs in that file that aren't referenced anywhere else in m-c. Not sure if either of those are metro-specific though.
Part 3 also included a UUID bump to nsIAppStartup which the mercurial push hook caught for me :)
MetroFireFox, I will miss You!
Still trying to figure out what happened to filter.py, but now all localized searchplugins are reported as obsolete, plus keys in region.properties. To be honest I'm tempted to ask a back-out

Example: https://l10n.mozilla.org/dashboard/compare?run=489973
Where do the localized searchplugins come from? They're not in my mozilla-central tree and I don't know very much about how the l10n process works, sorry.
Doh this is a stupid mistake on my part. Missing ')' in filter.py. I'll land a follow-up.
Yep, confirming the issue (it took me way too long to find it)

if (re.match(r"searchplugins\/.+\.xml", path):

The first parenthesis should not be there at all

if re.match(r"searchplugins\/.+\.xml", path):
Flags: needinfo?(bugmail.mozilla)
Flags: needinfo?(bugmail.mozilla)
(In reply to Francesco Lodolo [:flod] (UTC+2) from comment #57)
> Yep, confirming the issue (it took me way too long to find it)
> 
> if (re.match(r"searchplugins\/.+\.xml", path):
> 
> The first parenthesis should not be there at all
> 
> if re.match(r"searchplugins\/.+\.xml", path):

Oh, I did it the C++ way and left the extra parentheses in. It should work either way, next time this file is touched somebody can take it out. Or if you want me to do it anyway let me know.
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #59)
> Oh, I did it the C++ way and left the extra parentheses in. It should work
> either way, next time this file is touched somebody can take it out. Or if
> you want me to do it anyway let me know.

No need for me.
Depends on: 1160597
Depends on: 1163902
Depends on: 1251704
You need to log in before you can comment on or make changes to this bug.