Bug 124326 – Web Inspector: implement Page.captureScreenshot in the backend

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Inspector

[Brian Burg]

patch forthcoming

[Radar WebKit Bug Importer]

<rdar://problem/15465594>

[Brian Burg]

Created attachment 216946 [details]
v1.0

[Brian Burg]

Depends on patch in 124325. Going to attempt writing a test...

[Timothy Hatcher]

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.

[Joseph Pecoraro]

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)

[Brian Burg]

Created attachment 217242 [details]
rebased patch

[Simon Fraser (smfr)]

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?

[Brian Burg]

Created attachment 217251 [details]
v1.1

[Joseph Pecoraro]

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.

[Brian Burg]

Created attachment 217362 [details]
v3

[Brian Burg]

Addressed JoePeck's comments in patch v3.

[EFL EWS Bot]

Comment on attachment 217362 [details]
v3

Attachment 217362 [details] did not pass efl-ews (efl):
Output: http://webkit-queues.appspot.com/results/29658011

[Joseph Pecoraro]

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

[Build Bot]

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

[Build Bot]

Comment on attachment 217362 [details]
v3

Attachment 217362 [details] did not pass win-ews (win):
Output: http://webkit-queues.appspot.com/results/29968002

[Build Bot]

Comment on attachment 217362 [details]
v3

Attachment 217362 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/30008001

[EFL EWS Bot]

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

[kov's GTK+ EWS bot]

Comment on attachment 217362 [details]
v3

Attachment 217362 [details] did not pass gtk-ews (gtk):
Output: http://webkit-queues.appspot.com/results/30128028

[Brian Burg]

Created attachment 218550 [details]
v3.1 - fix style nits of v3, re-run EWS after 124325 landed

[Joseph Pecoraro]

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?

[WebKit Commit Bot]

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>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.