Bug 105315 – Web Inspector: introduce Page.captureScreenshot

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Inspector (Deprecated)

[Pavel Feldman]

It will be primarily used by the automation clients, but maybe we find a good place for it in the front-end as well.

[Pavel Feldman]

Created attachment 179958 [details]
Patch

[WebKit Review Bot]

Please wait for approval from [e-mail], [e-mail], [e-mail], [e-mail] or [e-mail] before submitting, as this patch contains changes to the Chromium public API. See also https://trac.webkit.org/wiki/ChromiumWebKitAPI.

[Brian Burg]

Is there a platform-agnostic way to expose the screenshot image data to the inspector frontend? I'm not aware of any other Inspector functionality that creates image data with a backend Inspector*Agent and exposes it to the Inspector's DOM. Do you have a sense of the difficulty?

It would be extremely useful for debugging animation frames, showing youtube-like scrubbing previews, etc.

[Pavel Feldman]

(In reply to comment #3)
> Is there a platform-agnostic way to expose the screenshot image data to the inspector frontend? I'm not aware of any other Inspector functionality that creates image data with a backend Inspector*Agent and exposes it to the Inspector's DOM. Do you have a sense of the difficulty?
> 
> It would be extremely useful for debugging animation frames, showing youtube-like scrubbing previews, etc.

We do ship images from the backend into front-end using base64 encoded strings and display them using data: urls. If you have some good feature in mind that would leverage that for screenshots, please let us know or send a mock!

[Pavel Feldman]

Created attachment 180147 [details]
Patch

[Alexander Pavlov (apavlov)]

Comment on attachment 180147 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=180147&action=review

> Source/WebKit/chromium/src/WebDevToolsAgentImpl.cpp:84
> +static const char screenshot[] = "screenshot";

Guess we should migrate to "static const char* const" whenever possible (to make use of the constant memory)

> Source/WebKit/chromium/src/WebDevToolsAgentImpl.cpp:814
> +    case BrowserDataHintScreenshot:

The WebKit style for this is:

case Foo: {
    BarOne;
    BarTwo;
}

(see e.g. CSSParser.cpp:1822)

[Andrey Kosyakov]

Comment on attachment 180147 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=180147&action=review

>> Source/WebKit/chromium/src/WebDevToolsAgentImpl.cpp:814
>> +    case BrowserDataHintScreenshot:
> 
> The WebKit style for this is:
> 
> case Foo: {
>     BarOne;
>     BarTwo;
> }
> 
> (see e.g. CSSParser.cpp:1822)

Why does this need braces at all?

[Andrey Kosyakov]

Comment on attachment 180147 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=180147&action=review

> Source/WebCore/inspector/InspectorPageAgent.cpp:1204
> +        *errorString = "Could not capture screenshot.";

please drop the punctuation from error messages.

[Pavel Feldman]

Comment on attachment 180147 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=180147&action=review

>> Source/WebCore/inspector/InspectorPageAgent.cpp:1204
>> +        *errorString = "Could not capture screenshot.";
> 
> please drop the punctuation from error messages.

I'd need to do it in 10 more places throughout this file. I'd better do that separately.

>> Source/WebKit/chromium/src/WebDevToolsAgentImpl.cpp:84
>> +static const char screenshot[] = "screenshot";
> 
> Guess we should migrate to "static const char* const" whenever possible (to make use of the constant memory)

Will do.

>>> Source/WebKit/chromium/src/WebDevToolsAgentImpl.cpp:814
>>> +    case BrowserDataHintScreenshot:
>> 
>> The WebKit style for this is:
>> 
>> case Foo: {
>>     BarOne;
>>     BarTwo;
>> }
>> 
>> (see e.g. CSSParser.cpp:1822)
> 
> Why does this need braces at all?

Historical reasons: I used to have variable declaration there.

[Yury Semikhatsky]

Comment on attachment 180147 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=180147&action=review

> Source/WebKit/chromium/public/WebDevToolsAgent.h:101
> +    WEBKIT_EXPORT static WebString patchWithBrowserData(const WebString& message, BrowserDataHint, const WebString& hintData);

This means that the client of this API will always be responsible for formatting the hintData. Is it always going to be a string? What about raw cookies, wouldn't we need to serialize them in some protocol-defined format if so then we should expose a way to set the patch data and and let WebCore serialize it into a message.

>>> Source/WebKit/chromium/src/WebDevToolsAgentImpl.cpp:84
>>> +static const char screenshot[] = "screenshot";
>> 
>> Guess we should migrate to "static const char* const" whenever possible (to make use of the constant memory)
> 
> Will do.

This sounds weird, the array content is already immutable, why making the pointer itself immutable changes where its content is stored?

[Yury Semikhatsky]

Comment on attachment 180147 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=180147&action=review

> Source/WebKit/chromium/src/WebDevToolsAgentImpl.cpp:782
> +    String messageString(message, messageLength);

This will always copy the message while we want shouldPatchWithBrowserData to be as fast as possible since we are going to call it for each message. You should either use C-functions to find the substring or create the String as String(ASCIILiteral(message)) if the message is always is a null-terminated string.

[Pavel Feldman]

Comment on attachment 180147 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=180147&action=review

>> Source/WebKit/chromium/public/WebDevToolsAgent.h:101
>> +    WEBKIT_EXPORT static WebString patchWithBrowserData(const WebString& message, BrowserDataHint, const WebString& hintData);
> 
> This means that the client of this API will always be responsible for formatting the hintData. Is it always going to be a string? What about raw cookies, wouldn't we need to serialize them in some protocol-defined format if so then we should expose a way to set the patch data and and let WebCore serialize it into a message.

I was planning on adding as much parameters as possible here. Having said that, I think they need to be of primitive types so that we don't grow WebKit API too much. We will be able to serialize data such as cookies into the protocol string on the browser-side as soon as they become "public" in the protocol, i.e. standardized.

>>>> Source/WebKit/chromium/src/WebDevToolsAgentImpl.cpp:84
>>>> +static const char screenshot[] = "screenshot";
>>> 
>>> Guess we should migrate to "static const char* const" whenever possible (to make use of the constant memory)
>> 
>> Will do.
> 
> This sounds weird, the array content is already immutable, why making the pointer itself immutable changes where its content is stored?

Leaving as []

>> Source/WebKit/chromium/src/WebDevToolsAgentImpl.cpp:782
>> +    String messageString(message, messageLength);
> 
> This will always copy the message while we want shouldPatchWithBrowserData to be as fast as possible since we are going to call it for each message. You should either use C-functions to find the substring or create the String as String(ASCIILiteral(message)) if the message is always is a null-terminated string.

There is a guard above that makes it work only for the overridden messages that are always small ("message" is not always null-terminated).

>>>> Source/WebKit/chromium/src/WebDevToolsAgentImpl.cpp:814
>>>> +    case BrowserDataHintScreenshot:
>>> 
>>> The WebKit style for this is:
>>> 
>>> case Foo: {
>>>     BarOne;
>>>     BarTwo;
>>> }
>>> 
>>> (see e.g. CSSParser.cpp:1822)
>> 
>> Why does this need braces at all?
> 
> Historical reasons: I used to have variable declaration there.

Fixed.

[Pavel Feldman]

Committed r138236: <http://trac.webkit.org/changeset/138236>