[webkit-reviews] review denied: [Bug 72587] Web Inspector: get rid of Panel::reset in the front-end. : [Attachment 115544] Patch

[bugzilla-daemon at webkit.org]

Timothy Hatcher <timothy at apple.com> has denied Pavel Feldman
<pfeldman at chromium.org>'s request for review:
Bug 72587: Web Inspector: get rid of Panel::reset in the front-end.
https://bugs.webkit.org/show_bug.cgi?id=72587

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

------- Additional Comments from Timothy Hatcher <timothy at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=115544&action=review


> Source/WebCore/ChangeLog:4
> +	   Web Inspector: get rid of Panel::reset in the front-end.
> +	   https://bugs.webkit.org/show_bug.cgi?id=72587

Please describe the reason/motivation for these changes. I've been trying to
get back into the inspector code and not having good descriptions for changes
are unhelpful. I'm going to r- any change that does not have a useful
description/motivation. The bugilla bug doesn't help either.

The same goes for describing what changes are in the methods in the ChangeLog.

> Source/WebCore/inspector/InspectorInstrumentation.cpp:-647
> -#if ENABLE(JAVASCRIPT_DEBUGGER)
> -	   if (InspectorDebuggerAgent* debuggerAgent =
instrumentingAgents->inspectorDebuggerAgent()) {
> -	       KURL url = inspectorAgent->inspectedURLWithoutFragment();
> -	       debuggerAgent->inspectedURLChanged(url);
> -	   }
> -#endif

Is this now assumed?

> Source/WebCore/inspector/front-end/ApplicationCacheModel.js:70
> -	   var frameId = event.data;
> -	   this._frameManifestRemoved(frameId);
> +	   var frame = event.data;
> +	   this._frameManifestRemoved(frame.id);

Seems unrelated to this change.

> Source/WebCore/inspector/front-end/ElementsPanel.js:345
> -	   this._highlightCurrentSearchResult(true);
> +	   this._highlightCurrentSearchResult();

Seems unrelated to this change.

> Source/WebCore/inspector/front-end/ElementsPanel.js:357
> -	   this._highlightCurrentSearchResult(false);
> +	   this._highlightCurrentSearchResult();

Seems unrelated to this change.

> Source/WebCore/inspector/front-end/JavaScriptContextManager.js:65
> -	   var frameId = event.data;
> -	   var context = this._frameIdToContext[frameId];
> +	   var frame = event.data;
> +	   var context = this._frameIdToContext[frame.id];

Seems unrelated to this change.

> Source/WebCore/inspector/front-end/JavaScriptContextManager.js:69
> -	   delete this._frameIdToContext[frameId];
> +	   delete this._frameIdToContext[frame.id];

Seems unrelated to this change.

> Source/WebCore/inspector/front-end/JavaScriptContextManager.js:98
> -	   return this._frame.id
> +	   return this._frame.id;

Seems unrelated to this change.

> Source/WebCore/inspector/front-end/ResourceTreeModel.js:189
> -	  
this.dispatchEventToListeners(WebInspector.ResourceTreeModel.EventTypes.FrameDe
tached, frameId);
> +	  
this.dispatchEventToListeners(WebInspector.ResourceTreeModel.EventTypes.FrameDe
tached, frame);

Seems unrelated to this change.

> Source/WebCore/inspector/front-end/ResourceTreeModel.js:312
> -	      
this.dispatchEventToListeners(WebInspector.ResourceTreeModel.EventTypes.FrameDe
tached, subframes[i].id);
> +	      
this.dispatchEventToListeners(WebInspector.ResourceTreeModel.EventTypes.FrameDe
tached, subframes[i]);

Seems unrelated to this change.

> Source/WebCore/inspector/front-end/ResourcesPanel.js:230
> +	       // Reset on main frame detach
> +	       this._reset();

It seems odd that the main frame can detach.

> Source/WebCore/inspector/front-end/ResourcesPanel.js:237
> -	   delete this._treeElementForFrameId[frameId];
> +	   delete this._treeElementForFrameId[frame.id];

Seems unrelated to this change.

> Source/WebCore/inspector/front-end/externs.js:183
> +}

Wrong patch?

> Source/WebCore/inspector/front-end/inspector.js:675
>  WebInspector.reset = function()
>  {

So what is reset called for now? Why do we need this still?

> Source/WebCore/inspector/generate-protocol-externs:73
> +def dash_to_camelcase(word):
> +    return "".join(x.capitalize() or "-" for x in word.split("-"))

Wrong patch?

> Source/WebCore/inspector/generate-protocol-externs:166
> +    if "capabilities" in domain:
> +	   for capability in domain["capabilities"]:
> +	       output_file.write("/** @type {string} */\n")
> +	       output_file.write("%sAgent.capability%s;\n" % (domain_name,
dash_to_camelcase(capability["name"])));

Wrong patch?