.
Created attachment 316187 [details] Patch
Created attachment 316188 [details] [Image] After Patch is applied
Created attachment 316190 [details] Patch
Screenshot of the result?
(In reply to Matt Baker from comment #4) > Screenshot of the result? The file is downloaded to wherever the user selects with the same name format as the macOS screenshot. Screen Shot ${year}-${month}-${day} at ${hours}.${minutes}.${seconds}.png
Comment on attachment 316190 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=316190&action=review r- for lack of test for the utility function. But also cause I wanted to see a version 2. Were we going to add a full page screenshot button in the navigation bar? Or is that going to be done separately? > Source/WebInspectorUI/UserInterface/Base/Utilities.js:1131 > +Object.defineProperty(Number, "pad", > +{ > + value(num, length) > + { > + let string = num.toLocaleString(); > + while (string.length < length) > + string = "0" + string; > + return string; > + }, > +}); String.prototype.padStart already exists and is standard. It can be used here instead of the while loop. How about making this Number.zeroPad / Number.prototype.zeroPad? let string = num.toLocaleString(); return string.padStart(length, "0"); This should have a test in: LayoutTests/inspector/unit-tests/number-utilities.html > Source/WebInspectorUI/UserInterface/Models/DOMNode.js:468 > + function scrollIntoView() Since we are moving this, lets fix the name of this inner function to be: function inspectedPage_node_scrollIntoView That is because this function is evaluated in the inspected page and the `this` value is the node. We name these specially so (1) they are easy to find and (2) we are aware that we have to be backwards compatible with older iOS releases for the contents of these functions. We can't use `let` for instance (we support back to iOS 7). > Source/WebInspectorUI/UserInterface/Views/ContextMenuUtilities.js:203 > + contextMenu.appendItem(WebInspector.UIString("Screenshot Node"), () => { We should wrap this in: if (window.PageAgent) { ... } I don't think there exists a case where it is possible to have DOMNodes without a PageAgent, but lets do it anyways to be safe. > Source/WebInspectorUI/UserInterface/Views/ContextMenuUtilities.js:218 > + // url: "web-inspector:///Node.png", Stale comment it seems.
Comment on attachment 316190 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=316190&action=review After taking my comments, please test saving a screenshot with Arabic system language. It should match the same filename format as a normal screen capture. > Source/WebInspectorUI/ChangeLog:19 > + Add Number.pad to add leading zeros up to a certain width. It might be better to add support for pad to our String.format implementation. >> Source/WebInspectorUI/UserInterface/Views/ContextMenuUtilities.js:203 >> + contextMenu.appendItem(WebInspector.UIString("Screenshot Node"), () => { > > We should wrap this in: > > if (window.PageAgent) { > ... > } > > I don't think there exists a case where it is possible to have DOMNodes without a PageAgent, but lets do it anyways to be safe. Two nits: 1) "Screenshot" is not a verb, at least not enough in common usage that it is suitable for a menu item 2) Most people don't know node vs element, so just calling it Element is fine. Together, I would prefer "Capture Element Screenshot" or "Take Element Screenshot". Either of these are workable with a similar "Take Viewport/Page Screenshot" context menu items. > Source/WebInspectorUI/UserInterface/Views/ContextMenuUtilities.js:205 > + if (error) We should log an error to console so the user knows why nothing showed up. > Source/WebInspectorUI/UserInterface/Views/ContextMenuUtilities.js:208 > + let date = new Date; Unfortunately we can't replicate what Cocoa does directly in JS because the Intl API doesn't do what NSDateFormatter does with arbitrary format strings. We can get around this, however, by using WebInspector.UIString("Screen Shot %d-%d-%d at %d.%d.%d").format(...). This will get us proper localization because UIString("...").format(...) will use the correct number characters for Arabic locales. AFAIK, in all other locales the date format is the same even if it's not what would be produced by default in the locale. For example, `new Date.toLocaleDateString()` will get you mm-dd-yyyy, instead of yyyy-mm-dd as the system does for whatever reason. > Source/WebInspectorUI/UserInterface/Views/DOMTreeElement.js:-1524 > - _scrollIntoView() Nice cleanup.
Created attachment 316357 [details] Patch
Comment on attachment 316357 [details] Patch Attachment 316357 [details] did not pass ios-sim-ews (ios-simulator-wk2): Output: http://webkit-queues.webkit.org/results/4182945 New failing tests: imported/w3c/IndexedDB-private-browsing/idbfactory_open12.html
Created attachment 316359 [details] Archive of layout-test-results from ews124 for ios-simulator-wk2 The attached test failures were seen while running run-webkit-tests on the ios-sim-ews. Bot: ews124 Port: ios-simulator-wk2 Platform: Mac OS X 10.12.5
Comment on attachment 316357 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=316357&action=review r=me > Source/WebInspectorUI/UserInterface/Views/ContextMenuUtilities.js:213 > + WebInspector.consoleLogViewController.appendConsoleMessage(consoleMessage); Very nice, thanks. > Source/WebInspectorUI/UserInterface/Views/ContextMenuUtilities.js:226 > + let filename = WebInspector.UIString("Screen Shot %s-%s-%s at %s.%s.%s").format(...values); Does .format do the right thing for %s? Hmm..
Comment on attachment 316357 [details] Patch Clearing flags on attachment: 316357 Committed r219870: <http://trac.webkit.org/changeset/219870>
All reviewed patches have been landed. Closing bug.