[webkit-reviews] review denied: [Bug 108611] Web Inspector: Refactor InspectorDOMStorageAgent::getDOMStorageEntries to report the error messages : [Attachment 186004] Patch

[bugzilla-daemon at webkit.org]

Yury Semikhatsky <yurys at chromium.org> has denied Vivek Galatage
<vivek.vg at samsung.com>'s request for review:
Bug 108611: Web Inspector: Refactor
InspectorDOMStorageAgent::getDOMStorageEntries to report the error messages
https://bugs.webkit.org/show_bug.cgi?id=108611

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

------- Additional Comments from Yury Semikhatsky <yurys at chromium.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=186004&action=review


> Source/WebCore/inspector/InspectorDOMStorageAgent.cpp:126
> +    entries = TypeBuilder::Array<TypeBuilder::Array<String> >::create();

You shouldn't write to the output parameters partial results in cases there is
an error below. You can use local variable to collect the entries and on
success assign it to |entries| after the loop below.

> Source/WebCore/inspector/InspectorDOMStorageAgent.cpp:136
> +		   break;

Is it the only type of error? If not you should either process all cases or add
default: section here providing some generic error message.

> Source/WebCore/inspector/InspectorDOMStorageAgent.cpp:145
> +		   *errorString = "Security error";

ditto, also consider extracting the error handling into a separate function:

if (hadException(ec, errorString))
    return;