894979 - Fix webapprt-test-chrome

Status: RESOLVED FIXED

(Firefox Graveyard :: Webapp Runtime, defect)

Description

[Marco Castelluccio [:marco]]

Attachment: Patch

Comment 1

[Marco Castelluccio [:marco]]

For details see bug 894162 comment 5 and following.

Comment 2

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

Comment on attachment 777206 [details] [diff] [review]
Patch

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

Looks great!  Just a couple minor issues.  But Fabrice should review the dom/apps/src/ changes, for which my comments here are just feedback.

::: dom/apps/src/AppDownloadManager.jsm
@@ +66,1 @@
>      return this.downloads[aManifestURL];

Is it important to return `null` in this case?  Without the change, this function returns `undefined`, which tends to have the same effect (f.e. on callers that check for truthiness, since both `null` and `undefined` evaluate to `false` in conditionals).

In situations like this, it's useful to add a code comment explaining why the code is doing something that doesn't appear to be necessary at first glance.

::: dom/apps/src/Webapps.jsm
@@ +2788,3 @@
>          Cu.reportError("DOMApplicationRegistry: Exception on app uninstall: " +
>                         ex + "\n" + ex.stack);
>        }

Normally I would say it's a good thing to check that a function exists before calling it!  But in this case we catch all exceptions and treat them the same.  So it isn't clear why it's important to check the function.  Can you elaborate?

Here again, if there's a subtle behavior variation, it would be useful to know what it is, ideally via a code comment that explained why the code is doing this extra check.

Also, nit: the emerging convention in Mozilla code is to use brackets around all conditional blocks, even one-liners.

::: webapprt/test/chrome/browser_window-title.js
@@ +13,5 @@
>      let tests = [
>        ["http://example.com/webapprtChrome/webapprt/test/chrome/window-title.html",
>         "http://example.com" + " - " + WebappRT.config.app.manifest.name,
>         "window title should show origin of page at different origin"],
> +      ["http://test:80/webapprtChrome/webapprt/test/chrome/window-title.html",

Nice use of test:80 to make these URLs shorter!  But is the port reference (":80") necessary?  It seems like it shouldn't be, since the "http" scheme implies that default.

::: webapprt/test/chrome/head.js
@@ +11,5 @@
>  
> +const MANIFEST_URL_BASE = Services.io.newURI(
> +  "http://test:80/webapprtChrome/webapprt/test/chrome/", null, null);
> +
> +let manifestName;

Instead of declaring *manifestName* globally so you can call *registerCleanupFunction* globally, call *registerCleanupFunction* within *loadWebapp* so you can also declare *manifestName* locally within that function, and there isn't a chance of multiple tests within the same test script stepping on each others' toes.

Comment 3

[Marco Castelluccio [:marco]]

(In reply to Myk Melez [:myk] [@mykmelez] from comment #2)
> Looks great!  Just a couple minor issues.  But Fabrice should review the
> dom/apps/src/ changes, for which my comments here are just feedback.
> 
> ::: dom/apps/src/AppDownloadManager.jsm
> @@ +66,1 @@
> >      return this.downloads[aManifestURL];
> 
> Is it important to return `null` in this case?  Without the change, this
> function returns `undefined`, which tends to have the same effect (f.e. on
> callers that check for truthiness, since both `null` and `undefined`
> evaluate to `false` in conditionals).
> 
> In situations like this, it's useful to add a code comment explaining why
> the code is doing something that doesn't appear to be necessary at first
> glance.
> 
> ::: dom/apps/src/Webapps.jsm
> @@ +2788,3 @@
> >          Cu.reportError("DOMApplicationRegistry: Exception on app uninstall: " +
> >                         ex + "\n" + ex.stack);
> >        }
> 
> Normally I would say it's a good thing to check that a function exists
> before calling it!  But in this case we catch all exceptions and treat them
> the same.  So it isn't clear why it's important to check the function.  Can
> you elaborate?
> 
> Here again, if there's a subtle behavior variation, it would be useful to
> know what it is, ideally via a code comment that explained why the code is
> doing this extra check.
> 
> Also, nit: the emerging convention in Mozilla code is to use brackets around
> all conditional blocks, even one-liners.

I made both changes just to avoid showing warnings during the tests:
JavaScript Warning: "ReferenceError: reference to undefined property this.downloads[aManifestURL]" {file: "resource://gre/modules/AppDownloadManager.jsm" line: 62}
JavaScript Error: "DOMApplicationRegistry: Exception on app uninstall: TypeError: aOnSuccess is not a function

Comment 4

[Marco Castelluccio [:marco]]

Attachment: Patch v2

This patch doesn't contain dom/apps changes (only a typo fix that shouldn't require fabrice's attention).

Comment 5

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

(In reply to Marco Castelluccio [:marco] from comment #3)
> I made both changes just to avoid showing warnings during the tests:
> JavaScript Warning: "ReferenceError: reference to undefined property
> this.downloads[aManifestURL]" {file:
> "resource://gre/modules/AppDownloadManager.jsm" line: 62}
> JavaScript Error: "DOMApplicationRegistry: Exception on app uninstall:
> TypeError: aOnSuccess is not a function

Ah, that's actually a good reason to make those changes (and doesn't require further commentary, as I suggested; I and other developers should see why the checks exist)!

Comment 6

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

Comment on attachment 777421 [details] [diff] [review]
Patch v2

Looks good, r=myk!  And feedback+ on the changes in dom/apps/, now that I understand them.  But you'll still need Fabrice's review for them, since I'm not a reviewer of that code.

Comment 7

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

Inbound "CLOSED. Debug browser-chrome logs are getting truncated." -> checkin-needed

Comment 8

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/40ecd8a299ad

Comment 9

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/40ecd8a299ad