[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 #217362|review?                     |review+
               Flag|                            |




--- Comment #13 from Joseph Pecoraro <joepeck at webkit.org>  2013-11-19 17:19:05 PST ---
(From update of attachment 217362)
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

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