Bug 119774 – Web Inspector: Download Web Archive of Inspected Page

• Component: Web Inspector
• Status: RESOLVED FIXED

[Joseph Pecoraro]

Make ⌘S for a DOMContentView save a Web Archive.

[Joseph Pecoraro]

Created attachment 208686 [details]
[PATCH] Proposed Fix

[Radar WebKit Bug Importer]

<rdar://problem/14729687>

[Early Warning System Bot]

Comment on attachment 208686 [details]
[PATCH] Proposed Fix

Attachment 208686 [details] did not pass qt-ews (qt):
Output: http://webkit-queues.appspot.com/results/1456603

[Early Warning System Bot]

Comment on attachment 208686 [details]
[PATCH] Proposed Fix

Attachment 208686 [details] did not pass qt-wk2-ews (qt-wk2):
Output: http://webkit-queues.appspot.com/results/1448712

[Joseph Pecoraro]

Created attachment 208689 [details]
[PATCH] Proposed Fix

Moved the #if ENABLE(WEB_ARCHIVE) line earlier.

[Antoine Quint]

Comment on attachment 208689 [details]
[PATCH] Proposed Fix

View in context: https://bugs.webkit.org/attachment.cgi?id=208689&action=review

> Source/WebInspectorUI/UserInterface/DOMTreeContentView.js:156
> +        return this._downloadArchiveNavigationItem.enabled;

This seems backward to me. Shouldn't the enabled property of this._downloadArchiveNavigationItem be tied to the supportsSave property rather than the other way around since this is a UI element afforded when save is supported?

> Source/WebInspectorUI/UserInterface/DOMTreeContentView.js:446
> +            this._downloadArchiveNavigationItem.enabled = true;

Do we want this set even if there's an error in the callback?

> Source/WebInspectorUI/UserInterface/DOMTreeContentView.js:465
> +        this._downloadArchiveNavigationItem.enabled = archiveable;

Per my comment above, I feel that we should keep archiveable as an ivar and bind the enabled state of this._downloadArchiveNavigationItem to it.

[Joseph Pecoraro]

(In reply to comment #6)
> (From update of attachment 208689 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=208689&action=review
> 
> > Source/WebInspectorUI/UserInterface/DOMTreeContentView.js:156
> > +        return this._downloadArchiveNavigationItem.enabled;
> 
> This seems backward to me. Shouldn't the enabled property of this._downloadArchiveNavigationItem be tied to the supportsSave property rather than the other way around since this is a UI element afforded when save is supported?

That sounds good! I added supportsSave stuff at the end, so naturally you can see how I got here.


> > Source/WebInspectorUI/UserInterface/DOMTreeContentView.js:446
> > +            this._downloadArchiveNavigationItem.enabled = true;
> 
> Do we want this set even if there's an error in the callback?

Pulling an archive from iOS can take ~3-4 seconds. Maybe we need a real failure state. However, at this point I don't expect any failures on Mac. For other ports, we may need a way to check "IFH.canSave()".


> > Source/WebInspectorUI/UserInterface/DOMTreeContentView.js:465
> > +        this._downloadArchiveNavigationItem.enabled = archiveable;
> 
> Per my comment above, I feel that we should keep archiveable as an ivar and bind the enabled state of this._downloadArchiveNavigationItem to it.

No need for an extra ivar if this can be part of supportsSave().

[Joseph Pecoraro]

Created attachment 208763 [details]
[PATCH] Proposed Fix - addressed review comments

Make enabled state part of supportsSave.

[Antoine Quint]

Comment on attachment 208763 [details]
[PATCH] Proposed Fix - addressed review comments

View in context: https://bugs.webkit.org/attachment.cgi?id=208763&action=review

> Source/WebInspectorUI/UserInterface/DOMTreeContentView.js:158
> +    get supportsSave()
> +    {
> +        var archiveable = WebInspector.Resource.Type.fromMIMEType(WebInspector.frameResourceManager.mainFrame.mainResource.mimeType) === WebInspector.Resource.Type.Document;
> +        return archiveable;
> +    },

I would just return directly and not use the variable. Also, does it need to be Public?

> Source/WebInspectorUI/UserInterface/DOMTreeContentView.js:444
> +    _downloadArchive: function(forceSaveAs)
> +    {
> +        this._downloadArchiveNavigationItem.enabled = false;

Seems to me that while we wait for the callback we should turn the _downloadArchiveNavigationItem into a spinner or something that shows the action is in progress, since it could take a while on iOS. This would be fine as a followup.

> Source/WebInspectorUI/UserInterface/InspectorBackendCommands.js:406
> +

Seems like there's one extra newline at the end of this file.

[Joseph Pecoraro]

(In reply to comment #9)
> (From update of attachment 208763 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=208763&action=review
> 
> > Source/WebInspectorUI/UserInterface/DOMTreeContentView.js:158
> > +    get supportsSave()
> > +    {
> > +        var archiveable = WebInspector.Resource.Type.fromMIMEType(WebInspector.frameResourceManager.mainFrame.mainResource.mimeType) === WebInspector.Resource.Type.Document;
> > +        return archiveable;
> > +    },
> 
> I would just return directly and not use the variable. Also, does it need to be Public?

It is public because supportsSave is how ⌘S support works. ContentBrowser calls it.


> > Source/WebInspectorUI/UserInterface/InspectorBackendCommands.js:406
> > +
> 
> Seems like there's one extra newline at the end of this file.

This is the generator. I can hunt it down.

[Joseph Pecoraro]

Comment on attachment 208763 [details]
[PATCH] Proposed Fix - addressed review comments

View in context: https://bugs.webkit.org/attachment.cgi?id=208763&action=review

> Source/WebInspectorUI/UserInterface/DOMTreeContentView.js:156
> +        var archiveable = WebInspector.Resource.Type.fromMIMEType(WebInspector.frameResourceManager.mainFrame.mainResource.mimeType) === WebInspector.Resource.Type.Document;

Now that I think about it. I should also check if PageAgent.archive exists. For backwards compatibility.

[Joseph Pecoraro]

Created attachment 209938 [details]
[PATCH] Proposed Fix - +Test +NewUI +Rebaselined

[Joseph Pecoraro]

Created attachment 209939 [details]
[IMAGE] New Download Button is in Resources Sidebar for Main Frame, by Reload

[Timothy Hatcher]

Comment on attachment 209938 [details]
[PATCH] Proposed Fix - +Test +NewUI +Rebaselined

View in context: https://bugs.webkit.org/attachment.cgi?id=209938&action=review

> Source/WebInspectorUI/UserInterface/ResourceTreeElement.css:33
> +<<<<<<< HEAD

Oops.

> Source/WebInspectorUI/UserInterface/ResourceTreeElement.css:52
> +=======
> +>>>>>>> d928684... IN PROGRESS: Download Arrow

Oops.

[Joseph Pecoraro]

Committed <http://trac.webkit.org/changeset/154828>.