[Webkit-unassigned] [Bug 124326] Web Inspector: implement Page.captureScreenshot in the backend

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=124326


Joseph Pecoraro <joepeck at webkit.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #217242|1                           |0
        is obsolete|                            |




--- Comment #9 from Joseph Pecoraro <joepeck at webkit.org>  2013-11-18 16:41:59 PST ---
(From update of attachment 217242)
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.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.