[webkit-reviews] review denied: [Bug 110960] Web Inspector: [Protocol] Turn SecurityOrigin into an object containing "domainSetInDOM" : [Attachment 190474] Patch

[bugzilla-daemon at webkit.org]

Vsevolod Vlasov <vsevik at chromium.org> has denied Alexander Pavlov (apavlov)
<apavlov at chromium.org>'s request for review:
Bug 110960: Web Inspector: [Protocol] Turn SecurityOrigin into an object
containing "domainSetInDOM"
https://bugs.webkit.org/show_bug.cgi?id=110960

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

------- Additional Comments from Vsevolod Vlasov <vsevik at chromium.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=190474&action=review


> Source/WebCore/inspector/InspectorIndexedDBAgent.cpp:641
> +    if (!frame)

It is odd that frame nullity check is only present here, not in
requestDatabaseNames and requestData.
Looks like findFrameWithSecurityOrigin call actually belongs to assertDocument
method that should not set any value to errorString as discussed offline.

> Source/WebCore/inspector/InspectorPageAgent.cpp:981
> +    if (errorString)

I would remove this: it is pretty clear that the frame was not found from the
return value and we have decided that we don't need this kind of messages on
front-end anyway.
Let's keep "Invalid security origin" message only.

> Source/WebCore/inspector/InspectorPageAgent.h:164
> +    void securityOriginChanged(Frame*, SecurityOrigin*);

I don't see neither calls nor implementation of this method. Did you mean to
put it into another patch?

> Source/WebCore/inspector/front-end/ResourceTreeModel.js:669
> +    var object = JSON.parse(id);

I think ResourceTreeModel should assign ids to security origin and keep track
of them providing securityOriginForId and securityOriginForUrlAndDomainSetInDOM
getters instead of using JSON.parse/stringify methods.

> Source/WebCore/inspector/front-end/ResourceTreeModel.js:677
> +    get id()

please use functions instead of getters and setters.

> Source/WebCore/inspector/front-end/ResourcesPanel.js:1888
> +    if (origin.url === "file://")

You should extract this logic, put it into SecurityOrigin object and reuse for
file systems and IndexedDB