[webkit-reviews] review granted: [Bug 124326] Web Inspector: implement Page.captureScreenshot in the backend : [Attachment 217362] v3

[bugzilla-daemon at webkit.org]

Joseph Pecoraro <joepeck at webkit.org> has granted Brian Burg
<burg at cs.washington.edu>'s request for review:
Bug 124326: Web Inspector: implement Page.captureScreenshot in the backend
https://bugs.webkit.org/show_bug.cgi?id=124326

Attachment 217362: v3
https://bugs.webkit.org/attachment.cgi?id=217362&action=review

------- Additional Comments from Joseph Pecoraro <joepeck at webkit.org>
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