Last Comment Bug 831236 - Remove mobile/xul/
: Remove mobile/xul/
Status: RESOLVED FIXED
[RIP]
:
Product: Fennec Graveyard
Classification: Graveyard
Component: General (show other bugs)
: Trunk
: ARM Android
: -- normal with 1 vote (vote)
: Future
Assigned To: :Gavin Sharp [email: gavin@gavinsharp.com]
:
Mentors:
Depends on: 848582 784841
Blocks: 847839
  Show dependency treegraph
 
Reported: 2013-01-16 05:17 PST by Ed Morley [:emorley]
Modified: 2013-11-13 02:20 PST (History)
22 users (show)
See Also:
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
patch (2.17 MB, patch)
2013-03-04 14:54 PST, :Gavin Sharp [email: gavin@gavinsharp.com]
l10n: review+
gps: review+
Details | Diff | Splinter Review
patch v2 (2.17 MB, patch)
2013-03-06 15:24 PST, :Gavin Sharp [email: gavin@gavinsharp.com]
mark.finkle: review+
mh+mozilla: review+
Details | Diff | Splinter Review

Description Ed Morley [:emorley] 2013-01-16 05:17:10 PST
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).
Comment 1 Mike Hommey [:glandium] 2013-01-16 06:50:06 PST
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.
Comment 2 Jim Mathies [:jimm] 2013-01-16 07:31:21 PST
(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.
Comment 3 Jim Mathies [:jimm] 2013-01-16 07:42:20 PST
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.
Comment 4 Matt Brubeck (:mbrubeck) 2013-01-16 07:43:20 PST
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?
Comment 5 Matt Brubeck (:mbrubeck) 2013-01-16 07:50:12 PST
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.
Comment 6 Mike Hommey [:glandium] 2013-01-16 07:50:45 PST
(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.
Comment 7 Oleg Romashin (:romaxa) 2013-01-16 08:19:34 PST
> 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.
Comment 8 Mark Finkle (:mfinkle) (use needinfo?) 2013-01-16 08:41:18 PST
Sounds like we are getting close to consensus. I'll throw in my "yes" vote as mobile module owner.
Comment 9 :Ms2ger (⌚ UTC+1/+2) 2013-01-16 08:58:45 PST
Not just yet, please :)
Comment 10 Ed Morley [:emorley] 2013-01-16 09:04:58 PST
(In reply to :Ms2ger from comment #9)
> Not just yet, please :)

I hadn't forgotten you (comment 0) :-))
Comment 11 Marco Bonardo [::mak] 2013-01-17 06:44:59 PST
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.
Comment 12 Matt Brubeck (:mbrubeck) 2013-01-17 07:38:18 PST
(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.
Comment 13 :Gavin Sharp [email: gavin@gavinsharp.com] 2013-03-04 13:33:09 PST
Bug 784841 landed, metro is on m-c, seems like we can pull the trigger now.
Comment 14 :Gavin Sharp [email: gavin@gavinsharp.com] 2013-03-04 14:54:10 PST
Created attachment 720923 [details] [diff] [review]
patch

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.
Comment 15 Ed Morley [:emorley] 2013-03-04 15:57:00 PST
Can't embedding/android/* go away now too? :-)
Comment 16 Doug Turner (:dougt) 2013-03-04 23:15:15 PST
yes
Comment 17 Mark Finkle (:mfinkle) (use needinfo?) 2013-03-05 01:50:56 PST
(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 18 Axel Hecht [pto-Aug-30][:Pike] 2013-03-05 02:59:19 PST
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.
Comment 19 Gregory Szorc [:gps] 2013-03-05 16:40:54 PST
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?
Comment 20 :Gavin Sharp [email: gavin@gavinsharp.com] 2013-03-06 15:23:12 PST
(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.
Comment 21 :Gavin Sharp [email: gavin@gavinsharp.com] 2013-03-06 15:24:21 PST
Created attachment 721926 [details] [diff] [review]
patch v2

Glandium: can you sign off on the packager.mk change, per the previous comments?
Comment 22 Mark Finkle (:mfinkle) (use needinfo?) 2013-03-06 17:02:03 PST
Comment on attachment 721926 [details] [diff] [review]
patch v2

Looks like you have all the relevant bits covered
Comment 23 Mark Finkle (:mfinkle) (use needinfo?) 2013-03-06 17:03:21 PST
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 24 Mike Hommey [:glandium] 2013-03-06 17:30:10 PST
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
Comment 25 :Gavin Sharp [email: gavin@gavinsharp.com] 2013-03-07 09:37:00 PST
https://hg.mozilla.org/integration/mozilla-inbound/rev/47be4c04cb6e
Comment 26 :Gavin Sharp [email: gavin@gavinsharp.com] 2013-03-07 09:39:16 PST
(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

Note You need to log in before you can comment on or make changes to this bug.