Last Comment Bug 741549 - Mochitests for navigator.mozApps
: Mochitests for navigator.mozApps
Status: VERIFIED FIXED
:
Product: Core
Classification: Components
Component: DOM: Apps (show other bugs)
: unspecified
: All All
: -- normal (vote)
: mozilla15
Assigned To: dclarke@mozilla.com [:onecyrenus]
:
: [:fabrice] Fabrice Desré
Mentors:
Depends on: 754578 746848 754223
Blocks: 746465
  Show dependency treegraph
 
Reported: 2012-04-02 13:39 PDT by dclarke@mozilla.com [:onecyrenus]
Modified: 2012-07-28 09:24 PDT (History)
8 users (show)
ryanvm: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Patch for mozilla-central (55.72 KB, text/plain)
2012-04-11 17:17 PDT, dclarke@mozilla.com [:onecyrenus]
no flags Details
Revision (55.86 KB, patch)
2012-04-11 19:48 PDT, dclarke@mozilla.com [:onecyrenus]
no flags Details | Diff | Splinter Review
All passing on try server (55.54 KB, patch)
2012-04-12 12:34 PDT, dclarke@mozilla.com [:onecyrenus]
felipc: feedback+
Details | Diff | Splinter Review
navigator.mozApps mochitests (102.95 KB, patch)
2012-04-16 15:13 PDT, dclarke@mozilla.com [:onecyrenus]
no flags Details | Diff | Splinter Review
navigator.mozApps mochitests (102.95 KB, patch)
2012-04-16 15:25 PDT, dclarke@mozilla.com [:onecyrenus]
no flags Details | Diff | Splinter Review
navigator.mozApps mochitests (47.70 KB, patch)
2012-04-19 17:24 PDT, dclarke@mozilla.com [:onecyrenus]
no flags Details | Diff | Splinter Review
navigator.mozApps mochitests (46.85 KB, patch)
2012-04-23 15:14 PDT, dclarke@mozilla.com [:onecyrenus]
no flags Details | Diff | Splinter Review
navigator.mozApps mochitests (47.00 KB, patch)
2012-04-25 00:10 PDT, dclarke@mozilla.com [:onecyrenus]
felipc: review+
Details | Diff | Splinter Review

Description User image dclarke@mozilla.com [:onecyrenus] 2012-04-02 13:39:48 PDT
Developed a set of mochitests for navigator.mozApps

Currently there are two tests that are not working. 

#1) UTF-8 test.  There is an outstanding bug for this 
https://bugzilla.mozilla.org/show_bug.cgi?id=738298

#2) No content-type checking 
https://bugzilla.mozilla.org/show_bug.cgi?id=741526
https://bugzilla.mozilla.org/show_bug.cgi?id=712045
Comment 1 User image Jason Smith [:jsmith] 2012-04-02 15:37:31 PDT
This bug isn't applicable to the web apps integration into desktop feature. This bug I believe belongs in Core/DOM --> Mozilla Extensions. Myk could confirm this.
Comment 2 User image dclarke@mozilla.com [:onecyrenus] 2012-04-11 17:17:09 PDT
Created attachment 614232 [details]
Patch for mozilla-central
Comment 3 User image dclarke@mozilla.com [:onecyrenus] 2012-04-11 19:48:24 PDT
Created attachment 614261 [details] [diff] [review]
Revision
Comment 4 User image dclarke@mozilla.com [:onecyrenus] 2012-04-12 12:31:47 PDT
https://tbpl.mozilla.org/?tree=Try&pusher=dclarke@mozilla.com
Comment 5 User image dclarke@mozilla.com [:onecyrenus] 2012-04-12 12:34:10 PDT
Created attachment 614495 [details] [diff] [review]
All passing on try server
Comment 6 User image :Felipe Gomes (needinfo me!) 2012-04-16 05:17:35 PDT
Comment on attachment 614495 [details] [diff] [review]
All passing on try server

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

This is looking good, thanks for all these tests. No major changes necessary, just some hopefully straightforward ones.

Some notes:
- files with a license header should use MPL2 http://www.mozilla.org/MPL/headers/ (or public domain which is also accepted for test files, if you prefer)

- needs to clear out all debugging code (info("..") calls for example)

- biggest change necessary is to drop urlmatch.js and just use nsIURI. It's simple: where you have the `url` as a string, you just call:
  var uri = Services.io.newURI(url, null, null);

  Then you can use all of these properties: http://mxr.mozilla.org/mozilla-central/source/netwerk/base/public/nsIURI.idl#105

- all the functions in dom/tests/mochitest/webapps/apps/jshelper.js seems to be unused? If that was only for debugging can you remove that file and the places where it was included?
  And most of the ones in the other jshelper.js too. If there's only a few you could just move them all to apphelper.js and have a single file to include. If that complicates things though don't bother, just remove whatever is unused

- you can begin strings with quotes with ' to avoid having to escape the double quotes

- where does jschannel.js comes from? and is it used? i can't find any references

::: dom/tests/mochitest/webapps/Makefile.in
@@ +50,5 @@
> +include $(topsrcdir)/config/rules.mk
> +
> +_TEST_FILES	= \
> +		test_install_app.xul \
> +    test_list_api.xul \

inconsistent indentation in this block, please make it all match

::: dom/tests/mochitest/webapps/README
@@ +1,1 @@
> +This repo represents a WIP graft onto the Firefox mochitest tree of mozilla-central.

This file can be removed

::: dom/tests/mochitest/webapps/jshelper.js
@@ +105,5 @@
> +  }
> +  info(output);
> +}
> +
> +function js_traverse(template, check, object) {

explain what js_traverse does

@@ +150,5 @@
> +    check(eval(evaluate), "#" + object + "# is expected to be true per template #" + template + "#");
> +  }
> +}
> +
> +function mozAppscb(pending, comparatorObj, check, next) {

> function mozAppscb(pending, comparatorObj, check, next) {

can you rename this function to something more clear and write a brief comment on what's the purpose of this function?
Comment 7 User image dclarke@mozilla.com [:onecyrenus] 2012-04-16 15:13:31 PDT
Created attachment 615490 [details] [diff] [review]
navigator.mozApps mochitests
Comment 8 User image dclarke@mozilla.com [:onecyrenus] 2012-04-16 15:19:15 PDT
With this patch, covered everything except renaming mozAppscb, which I documented instead. 


dom/tests/mochitest/webapps/jshelper.js  < -- this file is the required file
dom/tests/mochitest/webapps/apps/jshelper.js <-- this file is not required
Removed apps/test.html
Removed urlmatch.js
Removed README

info messages are now only shown if DEBUG = 1
removed references to jschannel it was used awhile ago.. 
I didn't change all the quotes, but will keep that in mind for next tests 

apphelper / jshelper, cannot be merged, because apphelper only works in the chrome context, whereas jshelper can be loaded by the iframe, loading chrome context inside an iframe won't work :(
Comment 9 User image dclarke@mozilla.com [:onecyrenus] 2012-04-16 15:25:03 PDT
Created attachment 615498 [details] [diff] [review]
navigator.mozApps mochitests
Comment 10 User image Jason Smith [:jsmith] 2012-04-17 21:30:08 PDT
We should try to land this before the FF 14 merge.
Comment 11 User image Jennifer Arguello :ticachica 2012-04-18 16:57:08 PDT
what happens if this doesn't land? This effort is important, but do the basic flows break if it's not implemented? I say no so not a blocker. Someone can correct me though.
Comment 12 User image dclarke@mozilla.com [:onecyrenus] 2012-04-19 17:24:44 PDT
Created attachment 616819 [details] [diff] [review]
navigator.mozApps mochitests

Latest tests updated to work with the dry run variable.
Comment 13 User image dclarke@mozilla.com [:onecyrenus] 2012-04-19 17:25:58 PDT
Comment on attachment 616819 [details] [diff] [review]
navigator.mozApps mochitests

i'll remove the info('dry_run_set")
Comment 14 User image dclarke@mozilla.com [:onecyrenus] 2012-04-23 13:11:00 PDT
..?
Comment 15 User image Jason Smith [:jsmith] 2012-04-23 14:37:27 PDT
Is there someone else available to review David's patch? I know Felipe is busy with the firefox work week and a lot of the desktop bug fixes.
Comment 16 User image Myk Melez [:myk] [@mykmelez] 2012-04-23 14:51:58 PDT
Comment on attachment 616819 [details] [diff] [review]
navigator.mozApps mochitests

Almost all of the changes are in dom/tests/, which is actually part of the Document Object Model module <https://wiki.mozilla.org/Modules/All#Document_Object_Model>, so subject to review by the DOM module owner or a peer.

Johnny: can you review this or identify an appropriate reviewer?
Comment 17 User image :Felipe Gomes (needinfo me!) 2012-04-23 15:10:47 PDT
Sorry for not updating the status of this bug. I reviewed these tests and they look fine. I sent them to tryserver last night though but some of the tests failed:
https://tbpl.mozilla.org/?tree=Try&rev=a437d8b5036a

dclarke, any idea what happened?

I can still review it if someone from the DOM module thinks that is fine. It's all tests.
Comment 18 User image dclarke@mozilla.com [:onecyrenus] 2012-04-23 15:14:42 PDT
Created attachment 617664 [details] [diff] [review]
navigator.mozApps mochitests
Comment 19 User image Johnny Stenback (:jst, jst@mozilla.com) 2012-04-24 09:37:17 PDT
I have no problems with Felipe doing the review of these tests in the DOM code!
Comment 21 User image dclarke@mozilla.com [:onecyrenus] 2012-04-25 00:10:59 PDT
Created attachment 618175 [details] [diff] [review]
navigator.mozApps mochitests
Comment 22 User image :Felipe Gomes (needinfo me!) 2012-04-25 14:05:38 PDT
Comment on attachment 618175 [details] [diff] [review]
navigator.mozApps mochitests

Thanks Dave! I'll land this soon
Comment 23 User image dclarke@mozilla.com [:onecyrenus] 2012-04-26 13:50:53 PDT
awesome
Comment 24 User image :Felipe Gomes (needinfo me!) 2012-05-08 09:05:13 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/f0b9f5c391a8
Comment 25 User image Ryan VanderMeulen [:RyanVM] 2012-05-08 18:42:08 PDT
https://hg.mozilla.org/mozilla-central/rev/f0b9f5c391a8
Comment 26 User image :Felipe Gomes (needinfo me!) 2012-05-10 01:08:45 PDT
I removed the call to enablePrivilege("UniversalXPConnect") on a follow-up as it was unnecessary.

https://hg.mozilla.org/integration/mozilla-inbound/rev/e7bf8b1386e3
Comment 27 User image Ed Morley [:emorley] 2012-05-10 08:16:37 PDT
https://hg.mozilla.org/mozilla-central/rev/e7bf8b1386e3

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