Summary: | Web Inspector: implement Page.captureScreenshot in the backend | ||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Brian Burg <burg> | ||||||||||||
Component: | Web Inspector | Assignee: | Nobody <webkit-unassigned> | ||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||
Severity: | Normal | CC: | buildbot, commit-queue, eflews.bot, graouts, gtk-ews, gyuyoung.kim, joepeck, philn, rniwa, simon.fraser, thorton, timothy, webkit-bug-importer, xan.lopez | ||||||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||||
Hardware: | All | ||||||||||||||
OS: | All | ||||||||||||||
Bug Depends on: | 124325 | ||||||||||||||
Bug Blocks: | 125322 | ||||||||||||||
Attachments: |
|
Description
Brian Burg
2013-11-13 17:44:21 PST
Created attachment 216946 [details]
v1.0
Depends on patch in 124325. Going to attempt writing a test... Comment on attachment 216946 [details] v1.0 View in context: https://bugs.webkit.org/attachment.cgi?id=216946&action=review > Source/WebCore/inspector/InspectorClient.h:91 > + virtual bool captureScreenshot(int /*x*/, int /*y*/, int /*width*/, int /*height*/, bool /*usePageCoordinates*/, String* /*outData*/) We usually just drop the names if they are not used. > Source/WebCore/inspector/protocol/Page.json:348 > "name": "captureScreenshot", > "description": "Capture page screenshot.", I wonder if we should rename this to something better? We don't aren't compatible with Chrome's now that we have parameters. > Source/WebKit/mac/WebCoreSupport/WebInspectorClient.mm:202 > +bool WebInspectorClient::captureScreenshot(int x, int y, int width, int height, bool usePageCoordinates, String* outData) We will need to add this to Windows as well. Can you look at that or file a bug? > Source/WebKit/mac/WebCoreSupport/WebInspectorClient.mm:208 > + DEFINE_STATIC_LOCAL(String, pngMimeType, (ASCIILiteral("image/png"))); I don't think using a static local for this helps much. Just pass the ASCIILiteral to toDataURL. > Source/WebKit2/WebProcess/WebCoreSupport/WebInspectorClient.cpp:97 > + DEFINE_STATIC_LOCAL(String, pngMimeType, (ASCIILiteral("image/png"))); Ditto. Comment on attachment 216946 [details] v1.0 View in context: https://bugs.webkit.org/attachment.cgi?id=216946&action=review >> Source/WebCore/inspector/protocol/Page.json:348 >> "description": "Capture page screenshot.", > > I wonder if we should rename this to something better? We don't aren't compatible with Chrome's now that we have parameters. I agree. I imagine there could be a number of screenshot APIs: 1. Screenshot of the visible page (viewport on OS X, fake viewport on iOS / mobile) 2. Screenshot of the entire page (1 big image) 3. Screenshot of a section of the page (arbitrary, element bounds, maybe a layer's bounds) Created attachment 217242 [details]
rebased patch
Comment on attachment 217242 [details] rebased patch View in context: https://bugs.webkit.org/attachment.cgi?id=217242&action=review > Source/WebCore/ChangeLog:8 > + Reviewed by NOBODY (OOPS!). > + > + * WebCore.xcodeproj/project.pbxproj: Say what you did. > Source/WebInspectorUI/ChangeLog:6 > + https://bugs.webkit.org/show_bug.cgi?id=124326 > + > + Reviewed by NOBODY (OOPS!). Ditto. > Source/WebKit/mac/ChangeLog:8 > + Reviewed by NOBODY (OOPS!). > + > + * WebCoreSupport/WebInspectorClient.h: Ditto. > Source/WebKit2/ChangeLog:8 > + Reviewed by NOBODY (OOPS!). > + > + * WebProcess/WebCoreSupport/WebInspectorClient.cpp: Ditto. > Source/WebCore/inspector/InspectorClient.h:91 > + virtual bool capturePageSnapshot(int, int, int, int, bool, String*) { return false; } You should name those ints (put the parameter names in comments). Can it be a rect? > Source/WebCore/inspector/InspectorPageAgent.h:132 > + virtual void capturePageSnapshot(ErrorString*, const int* x, const int* y, const int* width, const int* height, const bool* usePageCoordinates, String* outData); What are x and y relative to? Document? Frame? Viewport? Created attachment 217251 [details]
v1.1
Comment on attachment 217242 [details] rebased patch View in context: https://bugs.webkit.org/attachment.cgi?id=217242&action=review > Source/WebCore/WebCore.xcodeproj/project.pbxproj:885 > + 22BD9F7F1353625C009BD102 /* ImageBufferData.h in Headers */ = {isa = PBXBuildFile; fileRef = 22BD9F7D1353625C009BD102 /* ImageBufferData.h */; settings = {ATTRIBUTES = (Private, ); }; }; > + 22BD9F81135364FE009BD102 /* ImageBufferDataCG.h in Headers */ = {isa = PBXBuildFile; fileRef = 22BD9F80135364FE009BD102 /* ImageBufferDataCG.h */; settings = {ATTRIBUTES = (Private, ); }; }; Nit: With the other comments, if this stays in WebCore you won't need to export these files or symbols in WebCore.exp.in. > Source/WebCore/inspector/InspectorPageAgent.cpp:1269 > + result = m_client->capturePageSnapshot(viewport.x(), viewport.y(), viewport.width(), viewport.height(), false, outData); Nit: Why the client? (see later comment) > Source/WebCore/inspector/protocol/Page.json:348 > + "name": "capturePageSnapshot", > + "description": "Capture a snapshot of the page using the supplied rectangle. If any arguments are missing, the entire viewport is captured.", I don't want to dwell on naming and prevent landing, but I think having separate function names for different operations would be cleaner. Then an x/y/width/height specific function could have non-optional arguments. > Source/WebCore/inspector/protocol/Page.json:354 > + { "name": "usePageCoordinates", "type": "boolean", "optional": true, "description": "Indicates whether the provided parameters are in page coordinates or in viewport coordinates (the default)." } Rather then a boolean, an enum type could be used. That could later extend this list of "page" / "viewport" to maybe include something else in the future (document + frameId?) > Source/WebKit/mac/WebCoreSupport/WebInspectorClient.h:76 > + virtual bool capturePageSnapshot(int x, int y, int width, int height, bool usePageCoordinates, String* outData); Why implement this in the client at all? r- for this unless I'm overlooking something. It doesn't look like any port specific work is being done in the different port implementations of capturePageSnapshot. it seem like this could just be in InspectorPageAgent, then all ports get to take advantage of it. > Source/WebKit2/WebProcess/WebCoreSupport/WebInspectorClient.cpp:104 > + *outData = snapshot->toDataURL("image/png"); Nit: ASCIILiteral("image/png") because this is a C string literal that is turning into a WTF::String. Created attachment 217362 [details]
v3
Addressed JoePeck's comments in patch v3. Comment on attachment 217362 [details] v3 Attachment 217362 [details] did not pass efl-ews (efl): Output: http://webkit-queues.appspot.com/results/29658011 Comment on attachment 217362 [details] v3 View in context: https://bugs.webkit.org/attachment.cgi?id=217362&action=review I like this much better then the previous patch! r=me but some nits worth addressing. > Source/WebCore/inspector/InspectorPageAgent.cpp:1268 > + Frame* frame = mainFrame(); > + if (!frame) { > + *errorString = "No main frame"; > + return; > + } Nit: This if check and error is unnecessary. The mainFrame will never be null. This is a side effect of old code. mainFrame() should actually return a reference: Frame* InspectorPageAgent::mainFrame() { // FIXME: This should return a Frame& return &m_page->mainFrame(); } > Source/WebCore/inspector/InspectorPageAgent.cpp:1278 > + *errorString = "Could not capture snapshot"; Nit: Heh, because ErrorStyle is a typedef of String, we can ASCIILiteral(...) the strings assigned to it. > Source/WebCore/inspector/InspectorPageAgent.cpp:1291 > + Frame* frame = mainFrame(); > + if (!frame) { > + *errorString = "No main frame"; > + return; > + } Ditto. > Source/WebCore/inspector/InspectorPageAgent.cpp:1301 > + *errorString = "Could not capture snapshot"; Ditto. > Source/WebCore/inspector/protocol/Page.json:354 > + "description": "Capture a snapshot of a the specified node that does not include unrelated layers.", Typo: "of a the" => "of the" > Source/WebInspectorUI/UserInterface/InspectorBackendCommands.js:-337 > -InspectorBackend.registerCommand("Page.captureScreenshot", [], ["data"]); I would suggest retroactively removing the iOS 7 legacy Page.captureScreenshot APIs. We don't want to expose it via its InspectorBackendCommands because it wouldn't really work anyways. This would entail: 1. Removing Page.captureScreenshot from Source/WebInspectorUI/Inspector-iOS-7.0.json 2. Regenerating Legacy BackendCommands.js files by running: Source/WebInspectorUI/Scripts/update-InspectorBackendCommands.rb Comment on attachment 217362 [details] v3 Attachment 217362 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.appspot.com/results/29658015 Comment on attachment 217362 [details] v3 Attachment 217362 [details] did not pass win-ews (win): Output: http://webkit-queues.appspot.com/results/29968002 Comment on attachment 217362 [details] v3 Attachment 217362 [details] did not pass mac-ews (mac): Output: http://webkit-queues.appspot.com/results/30008001 Comment on attachment 217362 [details] v3 Attachment 217362 [details] did not pass efl-wk2-ews (efl-wk2): Output: http://webkit-queues.appspot.com/results/29828046 Comment on attachment 217362 [details] v3 Attachment 217362 [details] did not pass gtk-ews (gtk): Output: http://webkit-queues.appspot.com/results/30128028 Created attachment 218550 [details]
v3.1 - fix style nits of v3, re-run EWS after 124325 landed
Comment on attachment 218550 [details]
v3.1 - fix style nits of v3, re-run EWS after 124325 landed
Gosh, it would be really nice to add a protocol test for this. Could you do that in a follow-up?
Comment on attachment 218550 [details] v3.1 - fix style nits of v3, re-run EWS after 124325 landed Clearing flags on attachment: 218550 Committed r160202: <http://trac.webkit.org/changeset/160202> All reviewed patches have been landed. Closing bug. |