929865 - Change remaining org.mozilla.fennec_$USER sources to use the org.mozilla.gecko package

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Brian Nicholson (:bnicholson)]

Currently, the *.java.in sources belong to either the org.mozilla.gecko package or the org.mozilla.fennec_X package, and there's no way to differentiate them without parsing the files. To tell them apart, we should move these files to a separate directory designated for the generated namespace.

Comment 1

[Nick Alexander :nalexander [he/him]]

I'd be pretty happy if more and more of mobile/android/base ended up in a subdirectory, like mobile/android/base/foo.  We're going to start tearing the directory apart to support GeckoView more fully anyway.

Comment 2

[Brian Nicholson (:bnicholson)]

Attachment: Isolate generated fennec package sources into fennecpkg directory

Moves these files into a new 'fennecpkg' directory. Open to suggestions for a better name!

Comment 3

[Nick Alexander :nalexander [he/him]]

Comment on attachment 820794 [details] [diff] [review]
Isolate generated fennec package sources into fennecpkg directory

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

Based on your comment in Bug 853045, I understand why you're proposing this, but this seems wrong to me: this is even further from coherent package naming than the existing soup.

It is possible to declare, in the AndroidManifest, an "activity-alias" (see http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/AndroidManifest.xml.in#183).  Could we use this to alias org.mozilla.fennec_$USER/org.mozilla.fennec_$USER.App to org.mozilla.fennec_$USER/org.mozilla.gecko.App?  Then we could stop preprocessing even more.

We would, of course, need to be on the lookout for regressions.  Anything that used ANDROID_PACKAGE_NAME internally could be hard-coding a path.  What say you?

Comment 4

[Nick Alexander :nalexander [he/him]]

(In reply to Nick Alexander :nalexander from comment #3)
> Comment on attachment 820794 [details] [diff] [review]
> Isolate generated fennec package sources into fennecpkg directory
> 
> Review of attachment 820794 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Based on your comment in Bug 853045, I understand why you're proposing this,
> but this seems wrong to me: this is even further from coherent package
> naming than the existing soup.
> 
> It is possible to declare, in the AndroidManifest, an "activity-alias" (see
> http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/
> AndroidManifest.xml.in#183).  Could we use this to alias
> org.mozilla.fennec_$USER/org.mozilla.fennec_$USER.App to
> org.mozilla.fennec_$USER/org.mozilla.gecko.App?  Then we could stop
> preprocessing even more.
> 
> We would, of course, need to be on the lookout for regressions.  Anything
> that used ANDROID_PACKAGE_NAME internally could be hard-coding a path.  What
> say you?

mfinkle, wesj: what is your perspective on the suggestion above, to try activity-alias?  I know there are at least a few Intent creations for WebApps that would need testing.

Comment 5

[Brian Nicholson (:bnicholson)]

Cool, I wish I had known about <activity-alias> back in bug 856163 or I would have done this long ago. This definitely looks promising, and assuming it works, I agree this is the way to go.

Comment 6

[Brian Nicholson (:bnicholson)]

Attachment: Use <activity-alias> to wrap activities in generated namespace

Uses the suggestion from comment 3 to change these activities to use the org.mozilla.gecko namespace. There are no longer any files using the generated fennec_$USER namespace \o/

Testing locally, I can verify that webapps still work as expected.

Comment 7

[Nick Alexander :nalexander [he/him]]

Comment on attachment 821201 [details] [diff] [review]
Use <activity-alias> to wrap activities in generated namespace

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

Throughout: I think the intent filters should be registered on the actual activities, not the aliases -- but I haven't tested and might not understand exactly 
how the activity-alias filters work.

Also, I expect there are changes needed to launch the real activities, not the aliased activities, from within Fennec itself.  We want to deprecate (as much as possible) org.mozilla.fennec.{App,BrowserApp}.

::: mobile/android/base/AndroidManifest.xml.in
@@ +81,5 @@
>  #else
>  		 android:debuggable="true">
>  #endif
>  
> +        <activity android:name="org.mozilla.gecko.BrowserApp"

This should keep its label.

@@ +89,5 @@
>                    android:theme="@style/Gecko.App">
> +        </activity>
> +
> +        <!-- Alias BrowserApp so we can launch it from the package namespace. -->
> +        <activity-alias android:name=".App"

You've tested that this works with

am start -a android.intent.action.VIEW -n org.mozilla.fennec/.App -d http://mygreatsite.info

and

am start -a android.intent.action.VIEW -n org.mozilla.fennec/org.mozilla.fennec.App -d http://mygreatsite.info

and with

am start -a android.intent.action.VIEW -n org.mozilla.fennec/org.mozilla.gecko.App -d http://mygreatsite.info

'cuz I feel like we've lost the intent filters on the actual activity.

@@ +96,3 @@
>              <intent-filter>
>                  <action android:name="android.intent.action.MAIN" />
>                  <category android:name="android.intent.category.LAUNCHER" />

I think these should be on the actual activity.  We only want the alias to catch hard coded (deprecated in the future) invocations, no?

@@ +178,2 @@
>              <intent-filter>
>                  <action android:name="org.mozilla.gecko.WEBAPP" />

Again, this should be on the real activity.

@@ +193,5 @@
>  
>          <!-- Masquerade as the Resolver so that we can be opened from the Marketplace. -->
>          <activity-alias
>              android:name="com.android.internal.app.ResolverActivity"
> +            android:targetActivity="org.mozilla.gecko.BrowserApp"

qa-wanted, I think.

::: mobile/android/base/WebAppManifestFragment.xml.frag
@@ +1,1 @@
> +        <activity android:name="org.mozilla.gecko.WebApps$WebApp@APPNUM@"

This shouldn't lose its label.  We'd like to deprecate the alias below, right?

@@ +15,2 @@
>              <intent-filter>
>                  <action android:name="org.mozilla.gecko.WEBAPP@APPNUM@" />

These intent-filters should also be on the real activity, I think.

Comment 8

[Brian Nicholson (:bnicholson)]

Attachment: IRC conversation

Had a short discussion about this with Nick on IRC (conversation attached).

Comment 9

[Brian Nicholson (:bnicholson)]

Attachment: Use <activity-alias> to wrap activities in generated namespace, v2

Updated just to keep the labels; no other changes were made from the previous version.

(In reply to Nick Alexander :nalexander from comment #7)
> Comment on attachment 821201 [details] [diff] [review]
> Use <activity-alias> to wrap activities in generated namespace
> 
> Review of attachment 821201 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Throughout: I think the intent filters should be registered on the actual
> activities, not the aliases -- but I haven't tested and might not understand
> exactly 
> how the activity-alias filters work.
> 
> Also, I expect there are changes needed to launch the real activities, not
> the aliased activities, from within Fennec itself.  We want to deprecate (as
> much as possible) org.mozilla.fennec.{App,BrowserApp}.

A good change to make, but this is beyond what is minimally necessary. I filed bug 931177 to take care of this.

> ::: mobile/android/base/AndroidManifest.xml.in
> @@ +81,5 @@
> >  #else
> >  		 android:debuggable="true">
> >  #endif
> >  
> > +        <activity android:name="org.mozilla.gecko.BrowserApp"
> 
> This should keep its label.

Done, along with other label comments since they carry no risk (note that this particular activity and alias don't need labels at all since the fallback is the <application> label, which is identical).

> @@ +89,5 @@
> >                    android:theme="@style/Gecko.App">
> > +        </activity>
> > +
> > +        <!-- Alias BrowserApp so we can launch it from the package namespace. -->
> > +        <activity-alias android:name=".App"
> 
> You've tested that this works with
> 
> am start -a android.intent.action.VIEW -n org.mozilla.fennec/.App -d
> http://mygreatsite.info
>
> and
>
> am start -a android.intent.action.VIEW -n
> org.mozilla.fennec/org.mozilla.fennec.App -d http://mygreatsite.info

Yes.

> am start -a android.intent.action.VIEW -n
> org.mozilla.fennec/org.mozilla.gecko.App -d http://mygreatsite.info

No, this won't (and will never) work since App is in the fennec_$USER package. I think you meant org.mozilla.gecko.BrowserApp? If so, that's now bug 931177.

> @@ +96,3 @@
> >              <intent-filter>
> >                  <action android:name="android.intent.action.MAIN" />
> >                  <category android:name="android.intent.category.LAUNCHER" />
> 
> I think these should be on the actual activity.  We only want the alias to
> catch hard coded (deprecated in the future) invocations, no?

To keep things simple, we can keep all intent-filters on the aliases now and push these to the activities in bug 931177.
 
> @@ +193,5 @@
> >  
> >          <!-- Masquerade as the Resolver so that we can be opened from the Marketplace. -->
> >          <activity-alias
> >              android:name="com.android.internal.app.ResolverActivity"
> > +            android:targetActivity="org.mozilla.gecko.BrowserApp"
> 
> qa-wanted, I think.

I'm curious how QA can test this -- install some dummy Nightly marketplace app? The chances of this being broken are nearly zero since the .App alias works as expected and points to the same targetActivity. But I'll mark qa-wanted anyway -- better safe than sorry!

Comment 10

[Brian Nicholson (:bnicholson)]

Tagging qawanted as suggested by Nick; hopefully he can elaborate on how QA can help test this.

Comment 11

[Nick Alexander :nalexander [he/him]]

(In reply to Brian Nicholson (:bnicholson) from comment #9)
> Created attachment 822522 [details] [diff] [review]
> Use <activity-alias> to wrap activities in generated namespace, v2
> 
> Updated just to keep the labels; no other changes were made from the
> previous version.
> 
> (In reply to Nick Alexander :nalexander from comment #7)
> > Comment on attachment 821201 [details] [diff] [review]
> > Use <activity-alias> to wrap activities in generated namespace
> > 
> > Review of attachment 821201 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > Throughout: I think the intent filters should be registered on the actual
> > activities, not the aliases -- but I haven't tested and might not understand
> > exactly 
> > how the activity-alias filters work.
> > 
> > Also, I expect there are changes needed to launch the real activities, not
> > the aliased activities, from within Fennec itself.  We want to deprecate (as
> > much as possible) org.mozilla.fennec.{App,BrowserApp}.
> 
> A good change to make, but this is beyond what is minimally necessary. I
> filed bug 931177 to take care of this.

Very good.

> > @@ +89,5 @@
> > >                    android:theme="@style/Gecko.App">
> > > +        </activity>
> > > +
> > > +        <!-- Alias BrowserApp so we can launch it from the package namespace. -->
> > > +        <activity-alias android:name=".App"
> > 
> > You've tested that this works with
> > 
> > am start -a android.intent.action.VIEW -n org.mozilla.fennec/.App -d
> > http://mygreatsite.info
> >
> > and
> >
> > am start -a android.intent.action.VIEW -n
> > org.mozilla.fennec/org.mozilla.fennec.App -d http://mygreatsite.info
> 
> Yes.
> 
> > am start -a android.intent.action.VIEW -n
> > org.mozilla.fennec/org.mozilla.gecko.App -d http://mygreatsite.info
> 
> No, this won't (and will never) work since App is in the fennec_$USER
> package. I think you meant org.mozilla.gecko.BrowserApp? If so, that's now
> bug 931177.

I did mean BrowserApp.

> > @@ +193,5 @@
> > >  
> > >          <!-- Masquerade as the Resolver so that we can be opened from the Marketplace. -->
> > >          <activity-alias
> > >              android:name="com.android.internal.app.ResolverActivity"
> > > +            android:targetActivity="org.mozilla.gecko.BrowserApp"
> > 
> > qa-wanted, I think.
> 
> I'm curious how QA can test this -- install some dummy Nightly marketplace
> app? The chances of this being broken are nearly zero since the .App alias
> works as expected and points to the same targetActivity. But I'll mark
> qa-wanted anyway -- better safe than sorry!

We must have added this to support some Marketplace functionality (that I don't understand).  I want to make sure we don't break that, and getting QA to verify we haven't is done with qawanted.  From IRC:

13:48 bnicholson: rnewman: i saw you added this activity-alias -- do you think it's still necessary? http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/AndroidManifest.xml.in#183
13:49 rnewman looks
13:49 rnewman: I don't believe that was me -- sure that wasn't a whitespace-only or merge change?
13:50 rnewman: er, log says it was
13:51 rnewman: bnicholson: I speculate that it's still necessary. read the bug comments: https://bugzilla.mozilla.org/show_bug.cgi?id=724292
13:51 firebot: Bug 724292 nor, P3, Firefox 13, mbrubeck, RESO FIXED, Android Marketplace activity opening issue
13:51 rnewman: that is: so long as we have a single launcher icon, we'll be fine, but I doubt that will continue to be true, and I don't want the fix to be lost.
13:51 bnicholson: rnewman: yeah i'm reading those now -- is sync still a launchable activity?
13:52 rnewman: no, it's not, but perhaps we'll have Marketplace, or god knows what else
13:52 rnewman: or maybe Reading List, or even resurrecting a Set Up Sync icon

So QA can dig into 724292 to understand what this is about.

Comment 12

[Nick Alexander :nalexander [he/him]]

Comment on attachment 822522 [details] [diff] [review]
Use <activity-alias> to wrap activities in generated namespace, v2

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

This looks good but lets land it post-merge.

::: mobile/android/base/AndroidManifest.xml.in
@@ +81,5 @@
>  #else
>  		 android:debuggable="true">
>  #endif
>  
> +        <activity android:name="org.mozilla.gecko.BrowserApp"

Can we get a big comment explaining this?

/**
 * Fennec is shipped as the Android package named org.mozilla.{fennec,firefox,firefox_beta}.  The internal Java package hierarchy inside the Android package has both an org.mozilla.{fennec,firefox,firefox_beta} subtree *and* an org.mozilla.gecko subtree.  The non-org.mozilla.gecko is deprecated and we would like to get rid of it entirely.  Until that happens, we have external consumers (such as intents and bookmarks) of non-org.mozilla.gecko Activity classes, so we define activity aliases for backwards compatibility.
*/

::: mobile/android/base/Makefile.in
@@ -305,5 @@
>  DEFINES += -DMOZ_ANDROID_ANR_REPORTER=1
>  endif
>  
>  FENNEC_PP_JAVA_FILES := \
> -  App.java \

You're a good man, bnicholson.

::: mobile/android/base/WebAppManifestFragment.xml.frag
@@ +8,5 @@
>                    android:excludeFromRecents="true"
>                    android:theme="@style/Gecko.App">
> +        </activity>
> +
> +        <!-- Alias WebApp so we can launch it from the package namespace. -->

Reference comment in AndroidManifest.xml.in.

Comment 13

[Nick Alexander :nalexander [he/him]]

AaronMT: this changes a few things internally with how web apps are launched.  Can you do (or delegate) a smoketest to make sure we haven't busted launching webapps from the homescreen and however else they might be launched?  Doing this over an upgrade cycle would be best: install webapp with old version; upgrade to new version that has this code; verify that webapp isn't busted.

Thanks!

Comment 14

[Nick Alexander :nalexander [he/him]]

(In reply to Nick Alexander :nalexander from comment #13)
> AaronMT: this changes a few things internally with how web apps are
> launched.  Can you do (or delegate) a smoketest to make sure we haven't
> busted launching webapps from the homescreen and however else they might be
> launched?  Doing this over an upgrade cycle would be best: install webapp
> with old version; upgrade to new version that has this code; verify that
> webapp isn't busted.

This testing is all post-landing.  Sorry for not being clear.

Comment 15

[Brian Nicholson (:bnicholson)]

https://hg.mozilla.org/integration/fx-team/rev/8ebfd8ca2b65

Comment 16

[Brian Nicholson (:bnicholson)]

Backed out for robocop failures: https://tbpl.mozilla.org/?tree=Fx-Team&rev=8ebfd8ca2b65

https://hg.mozilla.org/integration/fx-team/rev/962365beac97

Comment 17

[Brian Nicholson (:bnicholson)]

Try run with update to BaseTest.java.in: https://tbpl.mozilla.org/?tree=Try&rev=c207ae0d9054

Comment 18

[Ryan VanderMeulen [:RyanVM]]

FWIW, this also had some needs-clobber bustage, so you may want to touch CLOBBER as well when you re-land :)

Comment 19

[Aaron Train [:aaronmt]]

New attempt to land?

Comment 20

[Brian Nicholson (:bnicholson)]

Still waiting on SUTAgent to be deployed to TBPL (bug 933501).

Comment 21

[Kevin Brosnan [Ex-Mozilla]]

Clearing the qawanted till this lands again.

Comment 22

[Nick Alexander :nalexander [he/him]]

IDEs are worth fighting for:

http://tbpl.mozilla.org/?tree=Try&rev=f1dae210e0a6

Comment 23

[Nick Alexander :nalexander [he/him]]

Take the second:

https://tbpl.mozilla.org/?tree=Try&rev=ed3d705558be

Comment 24

[Nick Alexander :nalexander [he/him]]

Attachment: Use <activity-alias> to wrap activities in generated namespace. r=bnicholson,myk

Motivated by how badly the front-end team wants proper IDE support, I
decided to have another push on this piece.

bnicholson: this is a re-tread of the previous patch we were ready to
land.  Try looks green.  Shit is still scary, but only one way to know
what's missing...

myk: this is scary shit.  We're changing how Activities get launched,
and hopefully nothing busts.  You've got the best context on where and
how webapps actually get launched to review this.

As an aside, getting rid of the gnarliest WebApps preprocessing is not
necessary, but we should do it eventually.  I'm happy to not make it
part of this ticket if either of you view it as a blocker to landing.

Comment 25

[Brian Nicholson (:bnicholson)]

Comment on attachment 8440497 [details] [diff] [review]
Use <activity-alias> to wrap activities in generated namespace. r=bnicholson,myk

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

Glad we can finally land this.

::: mobile/android/base/WebApps.java
@@ +14,5 @@
> +    public static class WebApp0 extends Webapp {
> +        @Override
> +        protected int getIndex() { return 0; }
> +    }
> +    

Nit: attack of the whitespace!

Comment 26

[Brian Nicholson (:bnicholson)]

Comment on attachment 8440497 [details] [diff] [review]
Use <activity-alias> to wrap activities in generated namespace. r=bnicholson,myk

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

One difference between this and the previous patch is that you're keeping App.java and WebApp.java around. These classes are only there to act as namespace wrappers for the actual implementations (GeckoApp and WebappImpl), but with <activity-alias>, we should be able to drop them entirely. I assume you're keeping them around to minimize the potential damage, but I think we should still aim to remove this dead code if we can. Please file a follow-up if you don't want to remove them here.

Comment 27

[Nick Alexander :nalexander [he/him]]

(In reply to Brian Nicholson (:bnicholson) from comment #26)
> Comment on attachment 8440497 [details] [diff] [review]
> Use <activity-alias> to wrap activities in generated namespace.
> r=bnicholson,myk
> 
> Review of attachment 8440497 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> One difference between this and the previous patch is that you're keeping
> App.java and WebApp.java around. These classes are only there to act as
> namespace wrappers for the actual implementations (GeckoApp and WebappImpl),
> but with <activity-alias>, we should be able to drop them entirely. I assume
> you're keeping them around to minimize the potential damage, but I think we
> should still aim to remove this dead code if we can. Please file a follow-up
> if you don't want to remove them here.

Good catch; this was due to tryserver laziness.  As you say, I'm trying to minimize changes, so will land with them and follow-up.

Comment 28

[Myk Melez [:myk] [@mykmelez]]

Comment on attachment 8440497 [details] [diff] [review]
Use <activity-alias> to wrap activities in generated namespace. r=bnicholson,myk

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

This looks good, and I think it'll work (and not break anything!).  But see below for suggestions for further improvements, which we may choose to do in followups.

::: mobile/android/base/AndroidManifest.xml.in
@@ +78,5 @@
>                   android:name="org.mozilla.gecko.GeckoApplication"
>                   android:hardwareAccelerated="true"
> +# The preprocessor does not yet support arbitrary parentheses, so this cannot
> +# be parenthesized thus to clarify that the logical AND operator has precedence:
> +#   !defined(MOZILLA_OFFICIAL) || (defined(NIGHTLY_BUILD) && defined(MOZ_DEBUG))

A creature from another planet!

@@ +238,5 @@
>                    android:excludeFromRecents="true"
>                    android:theme="@style/Gecko.App">
> +        </activity>
> +
> +        <!-- Alias WebApp so we can launch it from the package namespace. -->

Nit: WebApp -> Webapp

::: mobile/android/base/WebappManifestFragment.xml.frag.in
@@ +7,5 @@
>                    android:launchMode="singleTop"
>                    android:exported="true"
>          />
> +
> +        <!-- Alias WebApp so we can launch it from the package namespace. -->

Nit: WebApp -> WebApps

But I don't think we need to do this.  The webapp APK library doesn't refer to Fennec by package name; it simply fires an intent to "VIEW" an "application/webapp":

https://github.com/mozilla/apk-factory-library/blob/897396dc05f0a63a36d0363c69506aa76f9ffebc/src/org/mozilla/android/synthapk/LauncherActivity.java#L53-L59

Fennec's webapp.Dispatcher then creates an explicit intent with a class name that contains the package name:

http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/webapp/Dispatcher.java?rev=e3b9b91bf893#48

But that class name could be "org.mozilla.gecko.WebApps$WebApp" + index, since Dispatcher uses the "Intent setClassName (Context packageContext, String className)" form of setClassName, which *should* invoke the application's own activity by that name, even if another application (f.e. another installation of Fennec) defines the same activity.

(But I'm not an expert here! Please confirm that assumption!)

Thus we should be able to simply change the name of this activity to org.mozilla.gecko.WebApps$WebApp@APPNUM@, here and in Dispatcher.java.

And, if we're willing to copy that activity declaration into AndroidManifest.xml.in one hundred times, we should be able to remove one more bit of preprocessing.

Finally, it seems like we could move WebApps.java into mobile/android/base/webapp/, get rid of Webapp.java, and make each WebApps$WebApp@APPNUM@ class extend WebappImpl directly (or rename WebappImpl to Webapp).

(I would also rename the WebApps class to Webapps and each WebApp@APPNUM@ class to Webapp@APPNUM@ in the process, as we've been standardizing around that capitalization.)

Comment 29

[Nick Alexander :nalexander [he/him]]

With additional patches implementing feedback from bnicholson and myk:

https://tbpl.mozilla.org/?tree=Try&rev=1b8ddd25de46

Comment 30

[Nick Alexander :nalexander [he/him]]

Attachment: Part 1: Reference org.mozilla.gecko directly when handling application/webapp intent. r=myk

This was a small but important piece of your review comments.

Comment 31

[Nick Alexander :nalexander [he/him]]

Attachment: Part 2: Use <activity-alias> to wrap activities in generated namespace. r=bnicholson,myk

This has r+ from bnicholson and myk already.

Comment 32

[Nick Alexander :nalexander [he/him]]

Attachment: Part 3: Replace org.mozilla.gecko.App with BrowserApp. r=bnicholson

This was one of your review comments, bnicholson.

Comment 33

[Nick Alexander :nalexander [he/him]]

Attachment: Part 4: Move WebApps to webapp.Webapps; rename WebApp<n> to Webapp<n>. r=myk

This was another of your review comments, myk.

Comment 34

[Nick Alexander :nalexander [he/him]]

Attachment: Part 5: Remove WebApp entirely. r=rnewman,wesj

This is speculative.  I can't find where this is used; myk didn't know
either.  Is this left over from the "old webapps implementation" that
rnewman recently excised?

Comment 35

[Nick Alexander :nalexander [he/him]]

Forgot to update Robocop.  bnicholson, your review will be updated.

https://tbpl.mozilla.org/?tree=Try&rev=2113c446669a

Comment 36

[Nick Alexander :nalexander [he/him]]

Comment on attachment 8444738 [details] [diff] [review]
Part 3: Replace org.mozilla.gecko.App with BrowserApp. r=bnicholson

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

This will have test changes folded into it.

Comment 37

[Nick Alexander :nalexander [he/him]]

Comment on attachment 8444737 [details] [diff] [review]
Part 2: Use <activity-alias> to wrap activities in generated namespace. r=bnicholson,myk

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

Carrying forward flags.

Comment 38

[Nick Alexander :nalexander [he/him]]

http://tbpl.mozilla.org/?tree=Try&rev=9d19f0ecd116

Comment 39

[Richard Newman [:rnewman]]

Comment on attachment 8444742 [details] [diff] [review]
Part 5: Remove WebApp entirely. r=rnewman,wesj

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

Webapp.java should have been removed in Bug 1021342, I think.

As to removing the manifest parts: if we don't need them for anything else (apps added to your home screen like bookmarks?), and Myk is fine with it, then I'm fine with it.

Comment 40

[Nick Alexander :nalexander [he/him]]

Attachment: Part 3: Replace org.mozilla.gecko.App with BrowserApp. r=bnicholson

I busted the tests a few different ways on this before catching
everything (I hope), but this even lets us remove pre-processing from
Robocop. \o/

And if you look, that's a very green try run. \o/

Comment 41

[Brian Nicholson (:bnicholson)]

Comment on attachment 8445238 [details] [diff] [review]
Part 3: Replace org.mozilla.gecko.App with BrowserApp. r=bnicholson

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

\o/

Comment 42

[Myk Melez [:myk] [@mykmelez]]

Comment on attachment 8444737 [details] [diff] [review]
Part 2: Use <activity-alias> to wrap activities in generated namespace. r=bnicholson,myk

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

You didn't request review from me on this, but its commit comment suggests I reviewed it, so I'm doing so anyway!

Comment 43

[Myk Melez [:myk] [@mykmelez]]

Comment on attachment 8444742 [details] [diff] [review]
Part 5: Remove WebApp entirely. r=rnewman,wesj

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

I *think* this code is to support launching webapps from the Firefox Launcher (i.e. e.me), so we can't remove it; but mfinkle and/or blassey should know for sure.

Comment 44

[Nick Alexander :nalexander [he/him]]

(In reply to Myk Melez [:myk] [@mykmelez] from comment #42)
> Comment on attachment 8444737 [details] [diff] [review]
> Part 2: Use <activity-alias> to wrap activities in generated namespace.
> r=bnicholson,myk
> 
> Review of attachment 8444737 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> You didn't request review from me on this, but its commit comment suggests I
> reviewed it, so I'm doing so anyway!

Yeah, this was the meat of the earlier patch that you and bnicholson reviewed.

Comment 45

[Nick Alexander :nalexander [he/him]]

Just to make sure nothing gets lost:

(In reply to Myk Melez [:myk] [@mykmelez] from comment #28)
> Comment on attachment 8440497 [details] [diff] [review]
> Use <activity-alias> to wrap activities in generated namespace.
> r=bnicholson,myk
> 
> Review of attachment 8440497 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This looks good, and I think it'll work (and not break anything!).  But see
> below for suggestions for further improvements, which we may choose to do in
> followups.
> 
> ::: mobile/android/base/AndroidManifest.xml.in
> @@ +78,5 @@
> >                   android:name="org.mozilla.gecko.GeckoApplication"
> >                   android:hardwareAccelerated="true"
> > +# The preprocessor does not yet support arbitrary parentheses, so this cannot
> > +# be parenthesized thus to clarify that the logical AND operator has precedence:
> > +#   !defined(MOZILLA_OFFICIAL) || (defined(NIGHTLY_BUILD) && defined(MOZ_DEBUG))
> 
> A creature from another planet!
> 
> @@ +238,5 @@
> >                    android:excludeFromRecents="true"
> >                    android:theme="@style/Gecko.App">
> > +        </activity>
> > +
> > +        <!-- Alias WebApp so we can launch it from the package namespace. -->
> 
> Nit: WebApp -> Webapp
> 
> ::: mobile/android/base/WebappManifestFragment.xml.frag.in
> @@ +7,5 @@
> >                    android:launchMode="singleTop"
> >                    android:exported="true"
> >          />
> > +
> > +        <!-- Alias WebApp so we can launch it from the package namespace. -->
> 
> Nit: WebApp -> WebApps
> 
> But I don't think we need to do this.  The webapp APK library doesn't refer
> to Fennec by package name; it simply fires an intent to "VIEW" an
> "application/webapp":
> 
> https://github.com/mozilla/apk-factory-library/blob/
> 897396dc05f0a63a36d0363c69506aa76f9ffebc/src/org/mozilla/android/synthapk/
> LauncherActivity.java#L53-L59
> 
> Fennec's webapp.Dispatcher then creates an explicit intent with a class name
> that contains the package name:
> 
> http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/webapp/
> Dispatcher.java?rev=e3b9b91bf893#48
> 
> But that class name could be "org.mozilla.gecko.WebApps$WebApp" + index,
> since Dispatcher uses the "Intent setClassName (Context packageContext,
> String className)" form of setClassName, which *should* invoke the
> application's own activity by that name, even if another application (f.e.
> another installation of Fennec) defines the same activity.
> 
> (But I'm not an expert here! Please confirm that assumption!)
> 
> Thus we should be able to simply change the name of this activity to
> org.mozilla.gecko.WebApps$WebApp@APPNUM@, here and in Dispatcher.java.

Confirmed assumption, and I did what you suggested.

> And, if we're willing to copy that activity declaration into
> AndroidManifest.xml.in one hundred times, we should be able to remove one
> more bit of preprocessing.

I'm all for removing preprocessing, but we're never going to stop preprocessing AndroidManifest.xml, so I don't care to make this change.

> Finally, it seems like we could move WebApps.java into
> mobile/android/base/webapp/, get rid of Webapp.java, and make each
> WebApps$WebApp@APPNUM@ class extend WebappImpl directly (or rename
> WebappImpl to Webapp).

I made this change, but made removing Webapp a separate patch, since it was confusing.

> (I would also rename the WebApps class to Webapps and each WebApp@APPNUM@
> class to Webapp@APPNUM@ in the process, as we've been standardizing around
> that capitalization.)

Done.

Comment 46

[Nick Alexander :nalexander [he/him]]

Comment on attachment 8444742 [details] [diff] [review]
Part 5: Remove WebApp entirely. r=rnewman,wesj

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

blassey seems to think this is still necessary, so we'll drop this part entirely.

Comment 47

[Nick Alexander :nalexander [he/him]]

https://hg.mozilla.org/integration/fx-team/rev/0a15f881d837
https://hg.mozilla.org/integration/fx-team/rev/457213f9d867
https://hg.mozilla.org/integration/fx-team/rev/a4f39c9db1d9
https://hg.mozilla.org/integration/fx-team/rev/408f1e04c1c4

Comment 48

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/0a15f881d837
https://hg.mozilla.org/mozilla-central/rev/457213f9d867
https://hg.mozilla.org/mozilla-central/rev/a4f39c9db1d9
https://hg.mozilla.org/mozilla-central/rev/408f1e04c1c4