[webkit-reviews] review denied: [Bug 64581] Web Inspector: ApplicationCache UI is confusing and incorrect. : [Attachment 114343] Patch

[bugzilla-daemon at webkit.org]

Pavel Feldman <pfeldman at chromium.org> has denied Vsevolod Vlasov
<vsevik at chromium.org>'s request for review:
Bug 64581: Web Inspector: ApplicationCache UI is confusing and incorrect.
https://bugs.webkit.org/show_bug.cgi?id=64581

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

------- Additional Comments from Pavel Feldman <pfeldman at chromium.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=114343&action=review


A bunch of nits.

> Source/WebCore/inspector/InspectorApplicationCacheAgent.cpp:76
> +    m_frontend->updateApplicationCacheStatus(m_pageAgent->frameId(frame),
manifestURL, status);

"applicationCacheStatusUpdated"

> Source/WebCore/inspector/InspectorApplicationCacheAgent.cpp:130
> +    Frame* frame = m_pageAgent->frameForId(frameId);

As Michael suggests, you should introduce:

DocumentLoader* documentLoader = assertFrameWithDocumentLoader(frameId);
if (!documentLoader)
    return;

As in DOM agent.

> Source/WebCore/inspector/front-end/ApplicationCacheModel.js:58
> +	   var frameId = event.data.frame.id;

Please use following notation for compilation:

var frame = /** @type {<frame type here>} */ event.data["frame"];

i.e. you should do event.data[] instead of event.data.

> Source/WebCore/inspector/front-end/ApplicationCacheModel.js:70
> +	   this._reset();

It took me few seconds to figure out that this._statuses is initialized from
constructor. Avoid this indirection and declare variables in the constructor
explicitly.

> Source/WebCore/inspector/front-end/ApplicationCacheModel.js:88
> +	       console.error(error);

return; ?

> Source/WebCore/inspector/front-end/ApplicationCacheModel.js:96
> +	   if (error)

ditto

> Source/WebCore/inspector/front-end/ApplicationCacheModel.js:100
> +	       this._frameManifestUpdated(framesWithManifests[i].frameId,
framesWithManifests[i].manifestURL, framesWithManifests[i].status);

I would pass framesWithManifests[i] here...

> Source/WebCore/inspector/front-end/ApplicationCacheModel.js:104
> +	* @param {string} frameId

... and declare parameter type as {ApplicationCacheAgent.FramesWithManifest}.
It has been generated for you.