Closed Bug 1034730 Opened 10 years ago Closed 10 years ago

Test installing an app that uses AppCache

Categories

(Firefox Graveyard :: Web Apps, defect, P2)

Product:
Firefox Graveyard
Component:
Web Apps
Type:
defect

Tracking

(Not tracked)

Status:
RESOLVED FIXED
Milestone:
Firefox 33

People

(Reporter: marco, Assigned: marco)

References

Details

Attachments

(1 file, 2 obsolete files)

Patch v2
18.21 KB, patch
myk
: review+
Details | Diff | Splinter Review
Attached patch Patch (obsolete) — — Splinter Review 
This should be the last important test needed to cover the installation code.

Asking for feedback because I'd like to add an assertion to verify that some files are created in the app profile.
I don't yet know a simple way to get the cache directory (that is not the same as the profile), if I don't find one I'll just build it manually.
Getting the cache directory is also needed to properly cleanup the cached data after the test.
Attachment #8451197 - Flags: feedback?(myk)
Attached patch Patch (obsolete) — — Splinter Review 
Forgot to qref.

Myk, take this feedback request as a review request with the awareness that there's a missing bit ;)
Assignee: nobody → mar.castelluccio
Attachment #8451197 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #8451197 - Flags: feedback?(myk)
Attachment #8451198 - Flags: feedback?(myk)
Comment on attachment 8451198 [details] [diff] [review]
Patch

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

::: toolkit/webapps/tests/data/appcached_app/appcached_app.html
@@ +5,5 @@
> +</head>
> +<body>
> +<script type="application/javascript">
> +window.applicationCache.addEventListener('noupdate', function(aEvent) {
> +  if (aEvent.type == "noupdate") {

Can aEvent.type be anything else if you only add this event listener for the "noupdate" type?

::: toolkit/webapps/tests/data/appcached_app/appcached_app.webapp
@@ +1,1 @@
> +{

Nit: I'd call this file manifest.webapp, since it's in the "appcached_app" directory, so putting "appcached_app" in its name is redundant, whereas "manifest" provides additional information about the file.

::: toolkit/webapps/tests/test_install_appcache.xul
@@ +20,5 @@
>  
>  <script type="application/javascript">
>  <![CDATA[
>  
> +/** Test for Bug XXX **/

I guess these XXXes can all be set to 1034730.
Attachment #8451198 - Flags: feedback?(myk) → feedback+
Priority: -- → P2
Attached patch Patch v2 — — Splinter Review 
Most changes are in head.js now. There is some code to get the cache directory and a function to calculate the size of a directory.
Attachment #8451198 - Attachment is obsolete: true
Attachment #8453760 - Flags: review?(myk)
During the installation of an app with AppCache, the data is stored in the profile directory (under OfflineCache). When the app is launched, the data is in the proper place (on Linux, /home/<user>/.cache/...). Is this the expected behavior?
Flags: needinfo?(honzab.moz)
I really don't remember exactly.  IIRC, the installed first creates a "roaming" profile on user's disk.  Populates the appcache there.  When the app first launches, we move it to "local" profile.  I have no idea where the two profile locations are on linux.  I can only refer to windows.
Flags: needinfo?(honzab.moz)
(In reply to Honza Bambas (:mayhemer) from comment #5)
> I really don't remember exactly.  IIRC, the installed first creates a
> "roaming" profile on user's disk.  Populates the appcache there.  When the
> app first launches, we move it to "local" profile.  I have no idea where the
> two profile locations are on linux.  I can only refer to windows.

OK, this is what's happening. I was afraid the data wasn't being copied but re-downloaded.
Comment on attachment 8453760 [details] [diff] [review]
Patch v2

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

::: toolkit/webapps/tests/data/appcached_app/appcached_app.html
@@ +1,1 @@
> +<!DOCTYPE html>

Nit: I'd call this file index.html for the same reason I suggested changing the name of the manifest file.

::: toolkit/webapps/tests/test_install_appcache.xul
@@ +84,5 @@
> +    } catch (e) {
> +      yield wait(1000);
> +    }
> +  } while (size == 0);
> +  ok(size > 100000, "There are some files in the OfflineCache directory");

This seems imprecise.  What's the risk of failure here?

@@ +98,5 @@
>      yield wait(1000);
>    }
>    ok(!appClosed, "App was launched and is still running");
>  
> +  size = 0;

Nit: instead of reusing the variable above, which is disconnected from this usage (which makes this look like an accidental failure to declare a local variable), use a let definition <https://developer.mozilla.org/en-US/docs/Web/JavaScript/New_in_JavaScript/1.7#let_definitions> here and above to declare and initialize the variable for only the block of code that uses it:

  let (size = 0) {
    …
  }
Attachment #8453760 - Flags: review?(myk) → review+
(In reply to Myk Melez [:myk] [@mykmelez] from comment #7)
> ::: toolkit/webapps/tests/test_install_appcache.xul
> @@ +84,5 @@
> > +    } catch (e) {
> > +      yield wait(1000);
> > +    }
> > +  } while (size == 0);
> > +  ok(size > 100000, "There are some files in the OfflineCache directory");
> 
> This seems imprecise.  What's the risk of failure here?

Yes, this is really imprecise, but I didn't have a precise (and simple) way to know what the size should've been. I've manually checked the size and it's ~200 KB, so this shouldn't fail easily (and in case it does, we can easily lower the bar).
https://hg.mozilla.org/mozilla-central/rev/3db5124bd6c0
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → Firefox 33
Blocks: 1046004
Product: Firefox → Firefox Graveyard
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: