Closed Bug 831236 Opened 11 years ago Closed 11 years ago

Remove mobile/xul/

Categories

(Firefox for Android Graveyard :: General, defect)

ARM
Android
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
Future

People

(Reporter: emorley, Assigned: Gavin)

References

Details

(Whiteboard: [RIP])

Attachments

(1 file, 1 obsolete file)

It is my understanding that mobile/xul/ was primarily being kept around for the metro work. However I believe the necessary files have since been copied to browser/metro/ so mobile/xul/ is no longer required as far as metro is concerned.

Jimm: can you confirm if this is the case?

Others CCed: Other than metro, is there any other reason why we need/want to keep mobile/xul in the tree?

(If we are ok to remove mobile/xul/, we'll at the least want to wait until bug 784841 lands, so we don't bitrot it any more).
Flags: needinfo?(jmathies)
I know some people were interested in that code for internet kiosk kind of stuff, but mobile/xul being unmaintained, I doubt it works at all, by now, with all the platform changes. It doesn't even build (at least, for android, maybe it does for desktop). Surely, it's easy to dig in mercurial history to find mobile/xul if someone does need it.

That being said, if metro does inherit code from mobile/xul, it should be marked as such in mercurial, which would allow for proper blame/history for the inherited code, which means mobile/xul can't be removed before metro lands.
(In reply to Mike Hommey [:glandium] from comment #1)
> That being said, if metro does inherit code from mobile/xul, it should be
> marked as such in mercurial, which would allow for proper blame/history for
> the inherited code, which means mobile/xul can't be removed before metro
> lands.

It does and there are hg copy relationships for most of the metro front end code on elm. However, those relationships have been a pain to deal with during merges from mc, so we were planning on breaking them in the elm to mc merge by simply adding the metro front end to mc. This brings up an interesting conflict with this bug - we weren't worried about the lost blame in metro code because we knew we had mobile/xul blame to fall back on for reference. This has been valuable at times in tracking down original bug discussions on various areas of the code currently in use for metro.
Flags: needinfo?(jmathies)
example blame on elm:

http://hg.mozilla.org/projects/elm/annotate/e283743cbdc1/browser/metro/base/content/browser.js

We did not plan on merging elm into mc, so we knew we would loose this blame but have it in mobile/xul. If we could migrate this history somehow to mc I have no doubt it would come in handy down the road.
We'll still have mobile/xul in the Mercurial history, so no worry about deleting it on tip.  You can view blame on historical changesets in hgweb or with "hg annotate".

The last person who came into #mobile asking about how to build XUL Fennec just wanted to do a performance comparison versus native Fennec.  They couldn't build tip so they ended up building from a slightly older changeset.

romaxa, are you still keeping XUL Fennec for Meego up to date?
Flags: needinfo?(romaxa)
Example blame for a deleted file on m-c:
http://hg.mozilla.org/mozilla-central/annotate/f84023d2e919/mobile/android/chrome/content/browser-ui.js

I was also planning on pushing elm to a user repo so we still have a copy of the post-fork pre-merge history around for future archaeologists.
(In reply to Matt Brubeck (:mbrubeck) from comment #4)
> We'll still have mobile/xul in the Mercurial history, so no worry about
> deleting it on tip.  You can view blame on historical changesets in hgweb or
> with "hg annotate".

It's still way better when you don't need to do that manually.
> romaxa, are you still keeping XUL Fennec for Meego up to date?

No, I don't, currently I'm using mobile/android and b2g code in order to create embedding base for building meaningful Mozilla browser with native UI for other platforms.
Flags: needinfo?(romaxa)
Sounds like we are getting close to consensus. I'll throw in my "yes" vote as mobile module owner.
Whiteboard: [RIP]
Not just yet, please :)
Depends on: 784841
(In reply to :Ms2ger from comment #9)
> Not just yet, please :)

I hadn't forgotten you (comment 0) :-))
I assume any bug filed to refactor code (like replacing/removing APIs) touching mobile/xul can be wontfixed since we don't build nor run tests on it?
I have some bugs on file to remove/replace deprecate APIs and some of these are used in mobile/xul, but if it's going away there's no point in spending time on it.
(In reply to Marco Bonardo [:mak] from comment #11)
> I assume any bug filed to refactor code (like replacing/removing APIs)
> touching mobile/xul can be wontfixed since we don't build nor run tests on
> it?

That's correct.
Bug 784841 landed, metro is on m-c, seems like we can pull the trigger now.
Attached patch patch (obsolete) — Splinter Review
r?mfinkle on the removal in general (see also configure decision below).

Non-removal changes:
- r?Axel

l10n stuff. I mostly just removed the references to mobile/xul, seems like this could be further simplified in a followup though.

- r?gps

Made configure fail for MOZ_BUILD_APP=mobile. Once this is in for a while I imagine we could re-add it as an alias for mobile/android, but I didn't want to make that change in one step since that might confuse existing configs.

Random other build changes:
toolkit/mozapps/installer/packager.mk
embedding/moz.build
build/binary-location.mk

I did not remove some references in package manifests because those should be cleaned up separately.
Assignee: nobody → gavin.sharp
Status: NEW → ASSIGNED
Attachment #720923 - Flags: review?(mark.finkle)
Attachment #720923 - Flags: review?(l10n)
Attachment #720923 - Flags: review?(gps)
Can't embedding/android/* go away now too? :-)
yes
(In reply to Ed Morley [:edmorley UTC+0] from comment #15)
> Can't embedding/android/* go away now too? :-)

We actually have plans to re-use embedding/android. First we'll clean it out, then put the current set of files back in. We have too many "embedding" files in mobile/android now.
Comment on attachment 720923 [details] [diff] [review]
patch

I didn't dig in deep, I just verified that the localizers get notified that they're xul strings are obsolete once the patch lands.
Attachment #720923 - Flags: review?(l10n) → review+
Comment on attachment 720923 [details] [diff] [review]
patch

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

I'm privileged to put my stamp of approval on removing so much code from the tree.

::: mobile/locales/filter.py
@@ +3,5 @@
>  # file, You can obtain one at http://mozilla.org/MPL/2.0/.
>  
>  """This routine controls which localizable files and entries are
>  reported and l10n-merged.
> +This needs to stay in sync with the copy in mobile/android.

Please file a follow-up to consolidate the copies. I hate DRY violations.

::: toolkit/mozapps/installer/packager.mk
@@ +322,5 @@
>  ABI_DIR = armeabi
>  endif
>  endif
>  
> +ifneq (,$(filter b2g,$(MOZ_BUILD_APP)))

ifeq (b2g,$(MOZ_BUILD_APP))

Also, if this parallels the logic in /embedding/moz.build, didn't we decide this didn't matter for B2G?
Attachment #720923 - Flags: review?(gps) → review+
Depends on: 848582
(In reply to Gregory Szorc [:gps] from comment #19)
> Also, if this parallels the logic in /embedding/moz.build, didn't we decide
> this didn't matter for B2G?

Given that it's apparently used for APK stuff, that seems reasonable. I'll just remove this special case.
Attached patch patch v2Splinter Review
Glandium: can you sign off on the packager.mk change, per the previous comments?
Attachment #720923 - Attachment is obsolete: true
Attachment #720923 - Flags: review?(mark.finkle)
Attachment #721926 - Flags: review?(mh+mozilla)
Attachment #721926 - Flags: review?(mark.finkle)
Comment on attachment 721926 [details] [diff] [review]
patch v2

Looks like you have all the relevant bits covered
Attachment #721926 - Flags: review?(mark.finkle) → review+
Since we don't actually build mobile/xul anymore, we shouldn't see any breakage from this if you did miss a piece or two. If someone stumbles across more bits we can remove, they should file followups.
Comment on attachment 721926 [details] [diff] [review]
patch v2

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

::: mobile/android/locales/filter.py
@@ +3,5 @@
>  # file, You can obtain one at http://mozilla.org/MPL/2.0/.
>  
>  """This routine controls which localizable files and entries are
>  reported and l10n-merged.
> +This needs to stay in sync with the copy in mobile/android.

This is ambiguous, when read from mobile/android/locales/filter.py
Attachment #721926 - Flags: review?(mh+mozilla) → review+
(In reply to Mike Hommey [:glandium] from comment #24)
> This is ambiguous, when read from mobile/android/locales/filter.py

Pushed a followup: https://hg.mozilla.org/integration/mozilla-inbound/rev/763731b09013
https://hg.mozilla.org/mozilla-central/rev/47be4c04cb6e
https://hg.mozilla.org/mozilla-central/rev/763731b09013
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → Future
You need to log in before you can comment on or make changes to this bug.