Closed
Bug 831720
Opened 13 years ago
Closed 12 years ago
GCLI needs an appcache command
Categories
(DevTools Graveyard :: Graphic Commandline and Toolbar, defect)
DevTools Graveyard
Graphic Commandline and Toolbar
Tracking
(Not tracked)
RESOLVED
FIXED
Firefox 23
People
(Reporter: jwalker, Assigned: miker)
References
Details
Attachments
(1 file, 3 obsolete files)
81.99 KB,
patch
|
Details | Diff | Splinter Review |
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.
Reporter | ||
Updated•13 years ago
|
Reporter | ||
Updated•13 years ago
|
Assignee | ||
Updated•12 years ago
|
Assignee: nobody → mratcliffe
Assignee | ||
Comment 1•12 years ago
|
||
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?
Assignee | ||
Updated•12 years ago
|
Status: NEW → ASSIGNED
Assignee | ||
Comment 2•12 years ago
|
||
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)
Reporter | ||
Comment 3•12 years ago
|
||
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+
Assignee | ||
Comment 4•12 years ago
|
||
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.
Assignee | ||
Comment 5•12 years ago
|
||
(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
Assignee | ||
Updated•12 years ago
|
Whiteboard: [land-in-fx-team]
Assignee | ||
Comment 6•12 years ago
|
||
We need to hold off landing to fix a possible test issue.
Whiteboard: [land-in-fx-team]
Assignee | ||
Comment 7•12 years ago
|
||
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
Assignee | ||
Comment 8•12 years ago
|
||
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
Assignee | ||
Updated•12 years ago
|
Whiteboard: [land-in-fx-team]
Assignee | ||
Comment 9•12 years ago
|
||
Whiteboard: [land-in-fx-team] → [fixed-in-fx-team]
Comment 10•12 years ago
|
||
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 23
Updated•7 years ago
|
Product: Firefox → DevTools
Updated•7 years ago
|
Product: DevTools → DevTools Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•