Bug 124326

Summary: Web Inspector: implement Page.captureScreenshot in the backend
Product: WebKit Reporter: Brian Burg <burg>
Component: Web InspectorAssignee: 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 Flags
v1.0
none
rebased patch
none
v1.1
none
v3
none
v3.1 - fix style nits of v3, re-run EWS after 124325 landed none

Description Brian Burg 2013-11-13 17:44:21 PST
patch forthcoming
Comment 1 Radar WebKit Bug Importer 2013-11-13 17:44:39 PST
<rdar://problem/15465594>
Comment 2 Brian Burg 2013-11-14 09:14:34 PST
Created attachment 216946 [details]
v1.0
Comment 3 Brian Burg 2013-11-14 09:15:35 PST
Depends on patch in 124325. Going to attempt writing a test...
Comment 4 Timothy Hatcher 2013-11-14 10:39:44 PST
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 5 Joseph Pecoraro 2013-11-14 11:26:03 PST
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)
Comment 6 Brian Burg 2013-11-18 15:19:34 PST
Created attachment 217242 [details]
rebased patch
Comment 7 Simon Fraser (smfr) 2013-11-18 15:28:22 PST
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?
Comment 8 Brian Burg 2013-11-18 16:37:18 PST
Created attachment 217251 [details]
v1.1
Comment 9 Joseph Pecoraro 2013-11-18 16:41:59 PST
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.
Comment 10 Brian Burg 2013-11-19 17:02:14 PST
Created attachment 217362 [details]
v3
Comment 11 Brian Burg 2013-11-19 17:03:17 PST
Addressed JoePeck's comments in patch v3.
Comment 12 EFL EWS Bot 2013-11-19 17:10:49 PST
Comment on attachment 217362 [details]
v3

Attachment 217362 [details] did not pass efl-ews (efl):
Output: http://webkit-queues.appspot.com/results/29658011
Comment 13 Joseph Pecoraro 2013-11-19 17:19:05 PST
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 14 Build Bot 2013-11-19 17:33:16 PST
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 15 Build Bot 2013-11-19 17:39:32 PST
Comment on attachment 217362 [details]
v3

Attachment 217362 [details] did not pass win-ews (win):
Output: http://webkit-queues.appspot.com/results/29968002
Comment 16 Build Bot 2013-11-19 17:46:56 PST
Comment on attachment 217362 [details]
v3

Attachment 217362 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/30008001
Comment 17 EFL EWS Bot 2013-11-19 20:20:59 PST
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 18 kov's GTK+ EWS bot 2013-11-20 02:02:18 PST
Comment on attachment 217362 [details]
v3

Attachment 217362 [details] did not pass gtk-ews (gtk):
Output: http://webkit-queues.appspot.com/results/30128028
Comment 19 Brian Burg 2013-12-05 15:51:28 PST
Created attachment 218550 [details]
v3.1 - fix style nits of v3, re-run EWS after 124325 landed
Comment 20 Joseph Pecoraro 2013-12-05 16:26:59 PST
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 21 WebKit Commit Bot 2013-12-05 16:54:50 PST
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>
Comment 22 WebKit Commit Bot 2013-12-05 16:54:54 PST
All reviewed patches have been landed.  Closing bug.