Bug 108611 – Web Inspector: Refactor InspectorDOMStorageAgent::getDOMStorageEntries to report the error messages

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

[Vivek Galatage]

Refactor InspectorDOMStorageAgent::getDOMStorageEntries to report the error messages.

Patch follows.

[Vivek Galatage]

Created attachment 186004 [details]
Patch

[Alexander Pavlov (apavlov)]

Comment on attachment 186004 [details]
Patch

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

> Source/WebCore/inspector/InspectorDOMStorageAgent.cpp:116
> +        *errorString = "Storage resource not found for the provided storage Id";

id -> identifier

> Source/WebCore/inspector/InspectorDOMStorageAgent.cpp:132
> +        if (ec) {

Does this (and below) check structure mean you bail out of the method without providing any errorString for the other ec values? You have retained the pre-existing behavior for the other cases, but the code was unable to provide ANY errorString previously :)

> Source/WebCore/inspector/InspectorDOMStorageAgent.cpp:133
> +            switch (ec) {

Not quite sure, but I remember some compilers complain about not all the enum cases being handled if not all of them are enumerated and the switch does not contain the "default:" case.

[Yury Semikhatsky]

Comment on attachment 186004 [details]
Patch

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;

[Vivek Galatage]

Created attachment 186182 [details]
Patch

[Vivek Galatage]

Comment on attachment 186182 [details]
Patch

Thank you!

[WebKit Review Bot]

Comment on attachment 186182 [details]
Patch

Clearing flags on attachment: 186182

Committed r141690: <http://trac.webkit.org/changeset/141690>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.