Closed Bug 831720 Opened 7 years ago Closed 7 years ago

GCLI needs an appcache command

Categories

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

defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED
Firefox 23

People

(Reporter: jwalker, Assigned: miker)

References

Details

Attachments

(1 file, 3 obsolete files)

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.
No longer blocks: 784790, 794413
No longer blocks: 831715
Assignee: nobody → mratcliffe
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?
Status: NEW → ASSIGNED
No longer depends on: 861866
Attached patch Patch v1 (obsolete) — Splinter Review
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.
Attachment #740872 - Flags: review?(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?
Attachment #740872 - Flags: review?(jwalker) → review+
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.
Attached patch Patch v2 (obsolete) — Splinter Review
(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
Attachment #740872 - Attachment is obsolete: true
Whiteboard: [land-in-fx-team]
We need to hold off landing to fix a possible test issue.
Whiteboard: [land-in-fx-team]
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
Attachment #742383 - Attachment is obsolete: true
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).
Attachment #744076 - Attachment is obsolete: true
Whiteboard: [land-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/c8fa5b17e69b
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 23
Depends on: 869233
Blocks: 881057
Product: Firefox → DevTools
Product: DevTools → DevTools Graveyard
You need to log in before you can comment on or make changes to this bug.