957056 - figure out when BrowserApp._loadWebapp should pass different status parameter to _initRuntime

Status: RESOLVED FIXED

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

Description

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

BrowserApp._loadWebapp calls _initRuntime, passing its status parameter an empty string.  We should figure out when, if ever, it should pass a different status parameter (like "new").

Comment 1

[Martyn Haigh (:mhaigh)]

Attachment: 957056.patch

Comment 2

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

Comment on attachment 8357788 [details] [diff] [review]
957056.patch

Nice catch!  This looks right to me.

My only concern is that there's another call site for startupStatus, and that function can only be called once per startup (since it changes the pref it inspects if the mstone values are different).  I don't think both will be called, but it'd be useful to ensure that by refactoring these calls such that startupStatus is only ever called once.  For example, we could store its result in a property of BrowserApp and then refer to it from _loadWebapp.

Comment 3

[Martyn Haigh (:mhaigh)]

Attachment: 0001-957056-call-_initiRuntime-with-correct-status-parame.patch

Changes code to call _initRuntime with the correct status parameter, and ensures the method which returns the param only ever gets called once per startup

Comment 4

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

Comment on attachment 8359317 [details] [diff] [review]
0001-957056-call-_initiRuntime-with-correct-status-parame.patch

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

::: mobile/android/chrome/content/browser.js
@@ +283,4 @@
>  
>    deck: null,
>  
> +  _startupStatus: null, 

Nit: trailing space.

Also, this would be a perfect place for a memoizing unary getter, i.e. something like:

  get _startupStatus() {
    delete this._startupStatus;

    let savedmstone = null;
    try {
      savedmstone = Services.prefs.getCharPref("browser.startup.homepage_override.mstone");
    } catch (e) {
    }
#expand    let ourmstone = "__MOZ_APP_VERSION__";
    if (ourmstone != savedmstone) {
      Services.prefs.setCharPref("browser.startup.homepage_override.mstone", ourmstone);
      this._startupStatus = savedmstone ? "upgrade" : "new";
    }
    else {
      this._startupStatus = "";
    }

    return this._startupStatus;
  },

Comment 5

[Martyn Haigh (:mhaigh)]

Attachment: 0001-957056.patch

Refactor path to include memoizing unary getter

Comment 6

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

Comment on attachment 8359757 [details] [diff] [review]
0001-957056.patch

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

Looks great!  And I appreciate the expansion of "mstone" to "milestone", as "mstone" confuses me every time I read it.

Let's get wesj to review this.

Comment 7

[Wesley Johnston (:wesj)]

Comment on attachment 8359757 [details] [diff] [review]
0001-957056.patch

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

::: mobile/android/chrome/content/browser.js
@@ +444,5 @@
> +    if (ourMilestone != savedMilestone) {
> +      Services.prefs.setCharPref("browser.startup.homepage_override.mstone", ourMilestone);
> +      this._startupStatus = savedMilestone ? "upgrade" : "new";
> +    } else {
> +      this._startupStatus = "";

I'd rather avoid the 'else' if we can. Can we do

this._startupStatus = "";
if (ourMilestone != saved) {
  this._startupStatus = "something";
}

return this._startupStatus;

Comment 8

[Martyn Haigh (:mhaigh)]

Attachment: updated patch

Updated patch with slight refactor to remove else condition

Comment 9

[Wesley Johnston (:wesj)]

Comment on attachment 8362516 [details] [diff] [review]
updated patch

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

::: mobile/android/chrome/content/browser.js
@@ +457,5 @@
>      } catch (e) {
>      }
> +#expand    let ourMilestone = "__MOZ_APP_VERSION__";
> +    this._startupStatus = ""
> +    if (ourMilestone != savedMilestone) {

If this isn't true, we'll just have _startupStatus = undefined. Is that ok? Its odd to pass something undefined to methods.

Comment 10

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

(In reply to Wesley Johnston (:wesj) from comment #9)
> > +    this._startupStatus = ""
> > +    if (ourMilestone != savedMilestone) {
> 
> If this isn't true, we'll just have _startupStatus = undefined. Is that ok?
> Its odd to pass something undefined to methods.

Erm, _startupStatus can only be the empty string (""), "upgrade", or "new" with this patch, since it gets set to the empty string unconditionally and then is changed to either "upgrade" or "new" depending on the values of ourMilestone and savedMilestone.

Perhaps you meant that it's odd to pass an empty string to methods?  If so, I agree!  It would be better for _startupStatus to be something like "existing" if the version of the app is identical to the version the last time the app was started.

But this patch doesn't introduce that empty string, it just refactors an implementation that already uses it.  And perhaps there's a reason why it was chosen (because it evaluates to false in falsy contexts?).  So I'm loathe to change it here, where we're just refactoring it in order to use it in two places safely.


Comment on attachment 8362516 [details] [diff] [review]:

> +    this._startupStatus = ""

Nit: append a semi-colon to this statement.


> Subject: [PATCH] 957056

Nit: if you make the changeset comment sufficiently descriptive, then it'll be easier to get a sheriff to commit the change by setting the checkin-needed keyword on the bug.  Sheriffs like comments that describe the fix (even if the title of the bug report describes the bug).  In this case, I would make it something like:

  bug 957056 - pass correct status parameter to _initRuntime; r=wesj

(That "r=wesj" part is hard to predict in advance, so it only makes sense for patches you attach after you've obtained review, f.e. to fix nits.  But sheriffs can probably add that themselves provided you've otherwise constructed the comment to their specifications.)

Comment 11

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/9e9ad0ded84b

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/9e9ad0ded84b