Make ⌘S for a DOMContentView save a Web Archive.
Created attachment 208686 [details] [PATCH] Proposed Fix
<rdar://problem/14729687>
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 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
Created attachment 208689 [details] [PATCH] Proposed Fix Moved the #if ENABLE(WEB_ARCHIVE) line earlier.
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.
(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().
Created attachment 208763 [details] [PATCH] Proposed Fix - addressed review comments Make enabled state part of supportsSave.
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.
(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 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.
Created attachment 209938 [details] [PATCH] Proposed Fix - +Test +NewUI +Rebaselined
Created attachment 209939 [details] [IMAGE] New Download Button is in Resources Sidebar for Main Frame, by Reload
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.
Committed <http://trac.webkit.org/changeset/154828>.