958358 - make "webapp" capitalization consistent

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: Web Apps (PWAs), defect, P1)

Description

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

Some usage of the term "webapp" in variable, property, event, and notification names has inconsistent capitalization (primarily Webapp vs. WebApp), which leads to programming errors and extra effort.  We should review the usage and fix unintentional inconsistencies to minimize those errors and additional work.

Comment 1

[Martyn Haigh (:mhaigh)]

Attachment: 0001-958358-rename-WebApp-to-Webapps.patch

This is a tricky one - where do we stop?  There seems to be a very fine line between doing it correctly and going too far.

I haven't changed anything outside of mobile/, and I've not renamed any files outside of webapps/.  I've initially looked at messaging identifiers, variable names and method names but didn't want to do too much so currently there still exists instances of 'WebApps' and 'Webapps' in the same file.

Comment 2

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

Comment on attachment 8362934 [details] [diff] [review]
0001-958358-rename-WebApp-to-Webapps.patch

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

(In reply to Martyn Haigh (:mhaigh) from comment #1)
> Created attachment 8362934 [details] [diff] [review]
> 0001-958358-rename-WebApp-to-Webapps.patch
> 
> This is a tricky one - where do we stop?  There seems to be a very fine line
> between doing it correctly and going too far.

I would err on the side of doing too little, since it's easier to do more later than to unspool over-extensive changes.


> I haven't changed anything outside of mobile/, and I've not renamed any
> files outside of webapps/.  I've initially looked at messaging identifiers,
> variable names and method names but didn't want to do too much so currently
> there still exists instances of 'WebApps' and 'Webapps' in the same file.

This looks pretty good to me, but let's get early review from WesJ to make sure we're not violating some important convention for the Fennec Java code.

WesJ: the context here is that the platform code uses the "Webapp" capitalization exclusively in filenames (f.e. Webapps.jsm) and message names (for the numerous Webapps:* messages).  And we've spent hours banging our head against typos on several occasions because the Fennec code uses a different convention.  So we'd like to make the two consistent (in favor of the simpler "Webapp").

Comment 3

[Wesley Johnston (:wesj)]

Comment on attachment 8362934 [details] [diff] [review]
0001-958358-rename-WebApp-to-Webapps.patch

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

I've got no problem with the rename (even doing it in pieces if you want, i.e. you don't have to fix everything in one patch), but would like to use 'hg move' if we can. If I'm wrong about that, r+!

::: mobile/android/base/GeckoAppShell.java
@@ +714,1 @@
>          int index = WebAppAllocator.getInstance(getContext()).findAndAllocateIndex(aOrigin, aTitle, (String) null);

I'm surprised that you rename this class but don't get complaints here. I guess we have two WebappAllocator classes... I'd rather you renamed both :)

::: mobile/android/base/webapp/WebAppDispatcher.java
@@ -8,5 @@
> -import android.app.Activity;
> -import android.content.Intent;
> -import android.os.Bundle;
> -
> -public class WebAppDispatcher extends Activity {

It doesn't look like you used hg move for these. I'd rather you did to maintain some history/blame.

Comment 4

[Martyn Haigh (:mhaigh)]

Attachment: Make 'webapp' capitalisation consistent

A more inclusive patch this time - most instances of "WebApp" have been changes within the mobile code paths

Comment 5

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

Comment on attachment 8372246 [details] [diff] [review]
Make 'webapp' capitalisation consistent

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

The bad news is that I already busted this patch with my update flow commit from bug 934760!

  02-09 22:53 > hg import ~/0001-make-webapp-capitalisation-consistent.patch 
  applying /Users/myk/0001-make-webapp-capitalisation-consistent.patch
  patching file mobile/android/base/GeckoApp.java
  Hunk #1 FAILED at 635
  Hunk #2 FAILED at 1557
  Hunk #3 FAILED at 2087
  3 out of 4 hunks FAILED -- saving rejects to file mobile/android/base/GeckoApp.java.rej
  patching file mobile/android/base/GeckoAppShell.java
  Hunk #3 FAILED at 789
  1 out of 4 hunks FAILED -- saving rejects to file mobile/android/base/GeckoAppShell.java.rej
  patching file mobile/android/base/webapp/EventListener.java
  Hunk #1 succeeded at 17 with fuzz 2 (offset 2 lines).
  Hunk #2 succeeded at 42 with fuzz 2 (offset 7 lines).
  Hunk #3 FAILED at 54
  1 out of 6 hunks FAILED -- saving rejects to file mobile/android/base/webapp/EventListener.java.rej
  patching file mobile/android/modules/WebappManager.jsm
  Hunk #1 FAILED at 88
  Hunk #3 succeeded at 172 with fuzz 1 (offset 32 lines).
  1 out of 3 hunks FAILED -- saving rejects to file mobile/android/modules/WebappManager.jsm.rej
  abort: patch failed to apply

But that's the last big patch in the queue for the moment, so this should be easier to keep up-to-date from now on (although we should still strive to land it expeditiously so we don't have to keep rebasing it).

Incidentally, `git apply` also fails to apply case-only filename changes on my Mac:

  error: mobile/android/base/Webapp.java.in: already exists in working directory
  error: mobile/android/base/WebappAllocator.java: already exists in working directory
  error: mobile/android/base/WebappFragmentRepeater.inc: already exists in working directory
  error: mobile/android/base/WebappImpl.java: already exists in working directory
  error: mobile/android/base/WebappManifestFragment.xml.frag.in: already exists in working directory
  error: mobile/android/base/Webapps.java.in: already exists in working directory
  error: mobile/android/base/WebappsFragment.java.frag: already exists in working directory
  error: mobile/android/base/webapp/WebappImpl.java: already exists in working directory
  error: mobile/android/chrome/content/WebappRT.js: already exists in working directory

Fortunately, `hg apply` has no such problem, and since I'll be landing this into a Mercurial repository, that's probably ok (although I'd still like to know how to apply it with Git!).

::: mobile/android/base/GeckoApp.java
@@ +636,4 @@
>                  final String title = message.getString("title");
>                  final String type = message.getString("shortcutType");
>                  GeckoAppShell.removeShortcut(title, url, origin, type);
> +            } else if (!AppConstants.MOZ_ANDROID_SYNTHAPKS && event.equals("Webapps:PreInstall")) {

Nit: while we're touching these lines, we might as well change "preInstall" to "preinstall", which is actually a single word <http://en.wiktionary.org/wiki/preinstall>.

::: mobile/android/base/GeckoAppShell.java
@@ +755,5 @@
>          Intent intent = new Intent();
>          intent.setAction(GeckoApp.ACTION_WEBAPP_PREFIX + aIndex);
>          intent.setData(Uri.parse(aURI));
>          intent.setClassName(AppConstants.ANDROID_PACKAGE_NAME,
> +                            AppConstants.ANDROID_PACKAGE_NAME + ".Webapps$Webapp" + aIndex);

Won't this break existing shortcuts created by the old implementation?  It seems like this might be a case where we need to leave the string the same in order to support existing shortcuts that reference the class with a name that includes "WebApp".

::: mobile/android/base/WebappAllocator.java
@@ +17,5 @@
>  import java.util.ArrayList;
>  
> +public class WebappAllocator {
> +    private final String LOGTAG = "GeckoWebappAllocator";
> +    // The number of Webapp# and WEBAPP# activites/apps/intents

Nit: while you're changing this line, activites -> activities.

::: mobile/android/base/webapp/EventListener.java
@@ +64,5 @@
>  
>      public static void unregisterEvents() {
> +        unregisterEventListener("Webapps:PreInstall");
> +        unregisterEventListener("Webapps:InstallApk");
> +        unregisterEventListener("Webapps:PostInstall");

Nit: postinstall doesn't have a convenient Wiktionary entry, but it's plausibly a single word, given the way the "post-" prefix is used to form words <http://en.wiktionary.org/wiki/post-> and usage like in NPM <https://npmjs.org/doc/misc/npm-scripts.html>.

So I think we should treat it like a single word posthaste!

::: mobile/android/chrome/content/browser.js
@@ +468,4 @@
>  
>    _initRuntime: function(status, url, callback) {
>      let sandbox = {};
> +    Services.scriptloader.loadSubScript("chrome://browser/content/WebappRT.js", sandbox);

*sigh* If only we could alter loadSubScript… and scriptloader!

Comment 6

[Martyn Haigh (:mhaigh)]

Attachment: make webapp capitalisation consistent

Nits Fixed!

>The bad news is that I already busted this patch with my update flow commit 
>from bug 934760!

No problemo - have rebased

>Incidentally, `git apply` also fails to apply case-only filename changes on 
>my Mac... I'd still like to know how to apply it with Git!).

Indeed!  I'm seeing this on my end also and can't find a fix.  Have asked the collective over at StackOverflow to see if they know.


>Nit: while we're touching these lines, we might as well change "preInstall" to 
>"preinstall", which is actually a single word <http://en.wiktionary.org
>/wiki/preinstall>.

Done!

> Won't this break existing shortcuts created by the old implementation?  It 
> seems like this might be a case where we need to leave the string the same in 
> order to support existing shortcuts that reference the class with a name that 
> includes "WebApp".
Good spot - have reverted this change.

> Nit: while you're changing this line, activites -> activities.

You have the eyes of a hawk.

> Nit: postinstall doesn't have a convenient Wiktionary entry, but it's 
> plausibly a single word, given the way the "post-" prefix is used to form 
> words <http://en.wiktionary.org/wiki/post-> and usage like in NPM 
> <https://npmjs.org/doc/misc/npm-scripts.html>.
> 
> So I think we should treat it like a single word posthaste!

I made it so!

Comment 7

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

Comment on attachment 8373432 [details] [diff] [review]
make webapp capitalisation consistent

It looks like you requested feedback "from the wind", whereas I expect you meant to ask wesj for feedback.  But this seems ready for review, so I'm requesting that instead!

Comment 8

[Wesley Johnston (:wesj)]

Comment on attachment 8373432 [details] [diff] [review]
make webapp capitalisation consistent

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

Whoops. Meant to r+ this yesterday.

Comment 9

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/8983c0bf1297

Comment 10

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/8983c0bf1297

Comment 11

[Sylvestre Ledru [:Sylvestre]]

Myk, you want these changes to be uplifted in aurora/29?

Comment 12

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

(In reply to Sylvestre Ledru [:sylvestre] from comment #11)
> Myk, you want these changes to be uplifted in aurora/29?

Yes, I do!  These changes are refactoring-only, so they have no user-facing impact.  But they change the capitalization in a bunch of file and symbol names, and I'm concerned about the potential for future uplifts to be more complicated and risky if this patch isn't uplifted and we have to rewrite each future patch we uplift to undo the capitalization changes.

Comment 13

[Sylvestre Ledru [:Sylvestre]]

OK. I am waiting for your 'official' uplift request then :)

Comment 14

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

Comment on attachment 8373432 [details] [diff] [review]
make webapp capitalisation consistent

[Approval Request Comment]

Bug caused by (feature/regressing bug #): 
  This patch refactors code implemented by multiple bugs.

User impact if declined: 
  There is no direct user-facing impact to these changes.

Testing completed (on m-c, etc.): 
  This has been on Central for two days.

Risk to taking this patch (and alternatives if risky): 
  The patch is larger than a simple bug fix, which adds some inherent risk.
  And it refactors symbol and file names, which creates the risk of bustage
  from a typo or other programmer error.

  However, there's also a risk to not taking the patch: if we need to uplift
  anything else that touches the same code, we'll need to recreate its patch
  to change those names, which exposes the same risk of bustage from a typo
  or the like.

  Overall, I think we're better off with the patch than without it, riskwise,
  as I expect we'll continue to land fixes that we want to uplift.

String or IDL/UUID changes made by this patch:
  None

Comment 15

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/8371388d4d5f

Comment 16

[Benjamin Kerensa [:bkerensa]]

We will not be tracking this as its not a regression and is low impact to our users and fixed :)