Closed Bug 831720 Opened 13 years ago Closed 12 years ago

GCLI needs an appcache command

Categories

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

defect
Not set
normal

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.
Blocks: 831715, 784790, 794413
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]
Status: ASSIGNED → RESOLVED
Closed: 12 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.

Attachment

General

Created:
Updated:
Size: