831720 - GCLI needs an appcache command

Status: RESOLVED FIXED

(DevTools Graveyard :: Graphic Commandline and Toolbar, defect)

Description

[Joe Walker [:jwalker]]

Create 3 new commands
* appcache help
  * re-display error/warning messages
  * links to MDN
  * simple 'what next' message?
  * buttons for show/clear
* appcache show
  * re-display of error/warning messages
  * table with obvious URL|Type|Size columns
  * button to clear
* appcache clear [--all]

Add 'appcache help' to the button bar or similar, to help discoverability.


This assumes that the appcache system is instrumented to help us answer these questions and to tell us about what when wrong, and display this in the web console.

Comment 1

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Boiling Kevin's Docs down:

    Fixing errors
        Manifest missing
        Manifest parse error
        Bad mime type
        Missing resource
        cache-control: no-store wrongly set
        changing manifest file (kevin's #6)
        forgot to update manifest
        forgot to refresh a second time
    Additional errors
        When SSL, same-origin policy
    Confidence that it's working
        what came from the appcache?


Solution:

    Fixing errors
        Should be handled by web-console/log messages and "appcache show" command
    Displaying what we've got (appcache show)
    Manipulating the data
        appcache clear
        appcache edit (not needed for kevin's problems)
    Helping with getting it right
        appcache advise


Commands:

    appcache show
        If invalid:
            Explanation as to why (last error)
            Correction advice
            Links to MDN
            With 'retry' button
        If it's valid:
            Output a table containing the obvious URL|Type|Size columns
            With 'clear' button
    appcache help
        Links to MDN
        Advice if status is invalid
    appcache clear
        --all (?)

Do we need to surface a button in the toolbar?

Comment 2

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Attachment: Patch v1

We had to trim the tool back because some things are not possible using the current appcache service.

The tests don't run through as a batch so we will need to wait on bug 864834 to fix that but there is no reason not to review this patch.

There are changes to helper.js that we will need to move out to GCLI as discussed.

Comment 3

[Joe Walker [:jwalker]]

Comment on attachment 740872 [details] [diff] [review]
Patch v1

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

::: browser/devtools/commandline/test/helpers.js
@@ +770,5 @@
> +        if (typeof expected.output === "string") {
> +          doTestString(actualOutput, expected.output);
> +        } else {
> +          doTestRegEx(actualOutput, expected.output);
> +        }

These 5 lines are fairly much duplicates from above - we could have just a doTest function that contains this 'if' statement?

::: browser/devtools/shared/AppCacheUtils.jsm
@@ +29,5 @@
> +
> +Cu.import("resource://gre/modules/XPCOMUtils.jsm");
> +Cu.import("resource://gre/modules/Services.jsm");
> +Cu.import("resource://gre/modules/commonjs/sdk/core/promise.js");
> +Cu.import("resource://gre/modules/NetUtil.jsm");

I think we should get used to the pattern:
  let { X } = Cu.import("...", {});

No need to change it, but I'm not keen on the magic of Cu.import, and IIRC the magic is nasty because sometimes it imports more than you want.

@@ +66,5 @@
> +
> +      this._getURIInfo(this.manifestURI).then(uriInfo => {
> +        this._parseManifest(uriInfo).then(() => {
> +          // Sort errors by line number.
> +          this.errors.sort(function(a, b) {

Did you mean to suddenly jump back to non-fat-arrow?

@@ +360,5 @@
> +    });
> +  },
> +};
> +
> +function ManifestParser(manifestText, manifestURI) {

I think it's worth a comment about why we couldn't use the system manifest parser here. There is a risk of the 2 getting out of date.

::: browser/locales/en-US/chrome/browser/devtools/appcacheutils.properties
@@ +10,5 @@
> +# You want to make that choice consistent across the developer tools.
> +# A good criteria is the language in which you'd find the best
> +# documentation on web development on the web.
> +
> +noManifest=The specified page has no manifest.

We're going to need l10n comments here?

Comment 4

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

I suspect that bug 864834 might not get any love any time soon. We can try having the valid test run first and the valid one running second. Failing that we should disable the valid test until bug 864834 is fixed.

Comment 5

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Attachment: Patch v2

(In reply to Joe Walker [:jwalker] from comment #3)
> Comment on attachment 740872 [details] [diff] [review]
> Patch v1
> 
> Review of attachment 740872 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: browser/devtools/commandline/test/helpers.js
> @@ +770,5 @@
> > +        if (typeof expected.output === "string") {
> > +          doTestString(actualOutput, expected.output);
> > +        } else {
> > +          doTestRegEx(actualOutput, expected.output);
> > +        }
> 
> These 5 lines are fairly much duplicates from above - we could have just a
> doTest function that contains this 'if' statement?
> 

You are right. I have now fixed this so that there is only doTest()

> ::: browser/devtools/shared/AppCacheUtils.jsm
> @@ +29,5 @@
> > +
> > +Cu.import("resource://gre/modules/XPCOMUtils.jsm");
> > +Cu.import("resource://gre/modules/Services.jsm");
> > +Cu.import("resource://gre/modules/commonjs/sdk/core/promise.js");
> > +Cu.import("resource://gre/modules/NetUtil.jsm");
> 
> I think we should get used to the pattern:
>   let { X } = Cu.import("...", {});
> 
> No need to change it, but I'm not keen on the magic of Cu.import, and IIRC
> the magic is nasty because sometimes it imports more than you want.
> 

I have changed it just to help me get into the habit. In fact, I discovered that we didn't need NetUtil.jsm

> @@ +66,5 @@
> > +
> > +      this._getURIInfo(this.manifestURI).then(uriInfo => {
> > +        this._parseManifest(uriInfo).then(() => {
> > +          // Sort errors by line number.
> > +          this.errors.sort(function(a, b) {
> 
> Did you mean to suddenly jump back to non-fat-arrow?
> 

Yes, there is no need for that function to be bound.

> @@ +360,5 @@
> > +    });
> > +  },
> > +};
> > +
> > +function ManifestParser(manifestText, manifestURI) {
> 
> I think it's worth a comment about why we couldn't use the system manifest
> parser here. There is a risk of the 2 getting out of date.
> 

Comment added.

> ::: browser/locales/en-US/chrome/browser/devtools/appcacheutils.properties
> @@ +10,5 @@
> > +# You want to make that choice consistent across the developer tools.
> > +# A good criteria is the language in which you'd find the best
> > +# documentation on web development on the web.
> > +
> > +noManifest=The specified page has no manifest.
> 
> We're going to need l10n comments here?

Done.

I also have the tests working using:
http://sub1.test1.example.com
http://sub1.test2.example.com

Try:
https://tbpl.mozilla.org/?tree=Try&rev=56239e446a28

Comment 6

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

We need to hold off landing to fix a possible test issue.

Comment 7

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Attachment: Fixed security check and removed backwards compatibility issue

In the past few days a change landed that moves the offline data notification from the notification bar to a doorhanger which broke these tests. jwalker also discovered a small backwards compatibility issue in helpers.js.

These issues are now fixed.

Try:
https://tbpl.mozilla.org/?tree=Try&rev=230228188fcf

Comment 8

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Attachment: Removed inverse property and used post

We have now removed the inverse property and used post as this is far more useful for complex tests.

I have also improved feedback when comparing strings to output (this was skipped on passes and the actual text was missing on test failures).

Comment 9

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

https://hg.mozilla.org/integration/fx-team/rev/c8fa5b17e69b

Comment 10

[Tim Taubert [:ttaubert] (inactive)]

https://hg.mozilla.org/mozilla-central/rev/c8fa5b17e69b