Bug 72587 – Web Inspector: get rid of Panel::reset in the front-end.

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

[Pavel Feldman]

Patch to follow.

[Pavel Feldman]

Created attachment 115544 [details]
Patch

[WebKit Review Bot]

Comment on attachment 115544 [details]
Patch

Attachment 115544 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/10461108

New failing tests:
inspector/extensions/extensions-events.html

[Timothy Hatcher]

Comment on attachment 115544 [details]
Patch

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.FrameDetached, frameId);
> +        this.dispatchEventToListeners(WebInspector.ResourceTreeModel.EventTypes.FrameDetached, frame);

Seems unrelated to this change.

> Source/WebCore/inspector/front-end/ResourceTreeModel.js:312
> -            this.dispatchEventToListeners(WebInspector.ResourceTreeModel.EventTypes.FrameDetached, subframes[i].id);
> +            this.dispatchEventToListeners(WebInspector.ResourceTreeModel.EventTypes.FrameDetached, 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?

[Pavel Feldman]

Comment on attachment 115544 [details]
Patch

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

I'll extract compilation fixes from this patch.

>> Source/WebCore/inspector/InspectorInstrumentation.cpp:-647
>> -#endif
> 
> Is this now assumed?

We are now resetting debugger state upon windowObjectCleared event.

>> Source/WebCore/inspector/front-end/ApplicationCacheModel.js:70
>> +        this._frameManifestRemoved(frame.id);
> 
> Seems unrelated to this change.

This is related to this change, event's data is now frame, not frame's id.

>> Source/WebCore/inspector/front-end/ElementsPanel.js:345
>> +        this._highlightCurrentSearchResult();
> 
> Seems unrelated to this change.

This method does not accept parameters. JS compilation failed and I applied the drive-by fix.

>> Source/WebCore/inspector/front-end/JavaScriptContextManager.js:65
>> +        var context = this._frameIdToContext[frame.id];
> 
> Seems unrelated to this change.

Related.

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

Related.

>> Source/WebCore/inspector/front-end/ResourceTreeModel.js:189
>> +        this.dispatchEventToListeners(WebInspector.ResourceTreeModel.EventTypes.FrameDetached, frame);
> 
> Seems unrelated to this change.

This is related.

>> Source/WebCore/inspector/front-end/ResourceTreeModel.js:312

> 
> Seems unrelated to this change.

This is related.

>> Source/WebCore/inspector/front-end/ResourcesPanel.js:230
>> +            this._reset();
> 
> It seems odd that the main frame can detach.

We generate this event for navigation.

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

ditto

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

Also a drive-by for compilation fix.

>> Source/WebCore/inspector/front-end/inspector.js:675
>>  {
> 
> So what is reset called for now? Why do we need this still?

I'll remote it as soon as the two calls below are also eliminated.

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

Drive-by compilation fix.

>> Source/WebCore/inspector/generate-protocol-externs:166
>> +            output_file.write("%sAgent.capability%s;\n" % (domain_name, dash_to_camelcase(capability["name"])));
> 
> Wrong patch?

Drive-by compilation fix.

[Pavel Feldman]

Created attachment 115794 [details]
Patch

[WebKit Review Bot]

Comment on attachment 115794 [details]
Patch

Attachment 115794 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/10393143

New failing tests:
inspector/extensions/extensions-events.html

[Pavel Feldman]

Created attachment 115807 [details]
[Patch] Same with reset removed from protocol.

[Yury Semikhatsky]

Comment on attachment 115807 [details]
[Patch] Same with reset removed from protocol.

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

> Source/WebCore/inspector/Inspector.json:1959
> +                "name": "globalObjectCleared",

This event should contain identifier of the global object that was cleared. But since now we don't have such entity in the protocol we may add it later.

[WebKit Review Bot]

Comment on attachment 115807 [details]
[Patch] Same with reset removed from protocol.

Attachment 115807 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/10495133

New failing tests:
inspector/extensions/extensions-events.html

[Pavel Feldman]

Committed r100906: <http://trac.webkit.org/changeset/100906>