Bug 72587 - Web Inspector: get rid of Panel::reset in the front-end.
Summary: Web Inspector: get rid of Panel::reset in the front-end.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (Deprecated) (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Pavel Feldman
URL:
Keywords:
Depends on: 72887
Blocks:
  Show dependency treegraph
 
Reported: 2011-11-17 01:29 PST by Pavel Feldman
Modified: 2011-11-21 08:04 PST (History)
12 users (show)

See Also:


Attachments
Patch (21.51 KB, patch)
2011-11-17 02:04 PST, Pavel Feldman
no flags Details | Formatted Diff | Diff
Patch (18.71 KB, patch)
2011-11-18 05:55 PST, Pavel Feldman
no flags Details | Formatted Diff | Diff
[Patch] Same with reset removed from protocol. (27.55 KB, patch)
2011-11-18 07:47 PST, Pavel Feldman
yurys: review+
webkit.review.bot: commit-queue-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Pavel Feldman 2011-11-17 01:29:33 PST
Patch to follow.
Comment 1 Pavel Feldman 2011-11-17 02:04:20 PST
Created attachment 115544 [details]
Patch
Comment 2 WebKit Review Bot 2011-11-17 02:32:43 PST
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
Comment 3 Timothy Hatcher 2011-11-17 07:48:10 PST
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?
Comment 4 Pavel Feldman 2011-11-18 05:21:23 PST
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.
Comment 5 Pavel Feldman 2011-11-18 05:55:08 PST
Created attachment 115794 [details]
Patch
Comment 6 WebKit Review Bot 2011-11-18 06:39:50 PST
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
Comment 7 Pavel Feldman 2011-11-18 07:47:01 PST
Created attachment 115807 [details]
[Patch] Same with reset removed from protocol.
Comment 8 Yury Semikhatsky 2011-11-18 08:01:00 PST
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.
Comment 9 WebKit Review Bot 2011-11-18 08:20:04 PST
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
Comment 10 Pavel Feldman 2011-11-21 05:37:22 PST
Committed r100906: <http://trac.webkit.org/changeset/100906>