Bug 119774

Summary: Web Inspector: Download Web Archive of Inspected Page
Product: WebKit Reporter: Joseph Pecoraro <joepeck>
Component: Web InspectorAssignee: Joseph Pecoraro <joepeck>
Status: RESOLVED FIXED    
Severity: Normal CC: abecsi, cdumez, cmarcelo, commit-queue, graouts, gyuyoung.kim, joepeck, menard, rakuco, timothy, webkit-bug-importer, webkit-ews
Priority: P2 Keywords: InRadar
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
Bug Depends on: 120682    
Bug Blocks:    
Attachments:
Description Flags
[PATCH] Proposed Fix
webkit-ews: commit-queue-
[PATCH] Proposed Fix
none
[PATCH] Proposed Fix - addressed review comments
graouts: review+
[PATCH] Proposed Fix - +Test +NewUI +Rebaselined
timothy: review+
[IMAGE] New Download Button is in Resources Sidebar for Main Frame, by Reload none

Description Joseph Pecoraro 2013-08-13 15:56:39 PDT
Make ⌘S for a DOMContentView save a Web Archive.
Comment 1 Joseph Pecoraro 2013-08-13 16:04:17 PDT
Created attachment 208686 [details]
[PATCH] Proposed Fix
Comment 2 Radar WebKit Bug Importer 2013-08-13 16:04:59 PDT
<rdar://problem/14729687>
Comment 3 Early Warning System Bot 2013-08-13 16:10:23 PDT
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
Comment 4 Early Warning System Bot 2013-08-13 16:10:34 PDT
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
Comment 5 Joseph Pecoraro 2013-08-13 16:20:54 PDT
Created attachment 208689 [details]
[PATCH] Proposed Fix

Moved the #if ENABLE(WEB_ARCHIVE) line earlier.
Comment 6 Antoine Quint 2013-08-14 02:25:12 PDT
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.
Comment 7 Joseph Pecoraro 2013-08-14 10:44:01 PDT
(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().
Comment 8 Joseph Pecoraro 2013-08-14 15:11:12 PDT
Created attachment 208763 [details]
[PATCH] Proposed Fix - addressed review comments

Make enabled state part of supportsSave.
Comment 9 Antoine Quint 2013-08-14 15:48:41 PDT
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.
Comment 10 Joseph Pecoraro 2013-08-14 18:02:16 PDT
(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.
Comment 11 Joseph Pecoraro 2013-08-16 12:54:58 PDT
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.
Comment 12 Joseph Pecoraro 2013-08-28 18:20:43 PDT
Created attachment 209938 [details]
[PATCH] Proposed Fix - +Test +NewUI +Rebaselined
Comment 13 Joseph Pecoraro 2013-08-28 18:22:12 PDT
Created attachment 209939 [details]
[IMAGE] New Download Button is in Resources Sidebar for Main Frame, by Reload
Comment 14 Timothy Hatcher 2013-08-29 09:17:56 PDT
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.
Comment 15 Joseph Pecoraro 2013-08-29 11:58:32 PDT
Committed <http://trac.webkit.org/changeset/154828>.