Bug 54227 - Web Inspector: make InspectorAgent own sub-agents, align agent creation/deletion routines.
Summary: Web Inspector: make InspectorAgent own sub-agents, align agent creation/delet...
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:
Blocks:
 
Reported: 2011-02-10 11:14 PST by Pavel Feldman
Modified: 2011-02-11 07:23 PST (History)
10 users (show)

See Also:


Attachments
[PATCH] wip patch. (28.78 KB, patch)
2011-02-10 11:14 PST, Pavel Feldman
no flags Details | Formatted Diff | Diff
Patch (48.96 KB, patch)
2011-02-10 23:19 PST, Pavel Feldman
no flags Details | Formatted Diff | Diff
Patch (60.14 KB, patch)
2011-02-11 06:34 PST, Pavel Feldman
no flags Details | Formatted Diff | Diff
Patch (56.10 KB, patch)
2011-02-11 06:40 PST, Pavel Feldman
yurys: review+
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-02-10 11:14:10 PST
This patch will align some of our agents' lifetime, creation and deletion code, state interaction and such.
Comment 1 Pavel Feldman 2011-02-10 11:14:49 PST
Created attachment 82010 [details]
[PATCH] wip patch.
Comment 2 Pavel Feldman 2011-02-10 23:19:14 PST
Created attachment 82106 [details]
Patch
Comment 3 Yury Semikhatsky 2011-02-11 02:29:15 PST
Comment on attachment 82106 [details]
Patch

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

> Source/WebCore/inspector/InspectorAgent.cpp:-337
> -    releaseFrontendLifetimeAgents();

Please put an ASSERT(!m_frontend) here.

> Source/WebCore/inspector/InspectorDOMAgent.cpp:943
> +void InspectorDOMAgent::mainDOMContentLoaded()

mainDOMContentLoaded -> mainFrameDOMContentLoaded

> Source/WebCore/inspector/InspectorDatabaseResource.cpp:57
>  void InspectorDatabaseResource::bind(InspectorFrontend* frontend)

Let's rename bind into pushToFrontend
Comment 4 Ilya Tikhonovsky 2011-02-11 02:38:20 PST
Comment on attachment 82106 [details]
Patch

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

> Source/WebCore/inspector/InspectorDOMAgent.cpp:279
>  void InspectorDOMAgent::startListening(Document* doc)
>  {
> -    if (m_documents.contains(doc))
> -        return;
> -
> -    doc->addEventListener(eventNames().DOMContentLoadedEvent, this, false);
> -    doc->addEventListener(eventNames().loadEvent, this, true);
>      m_documents.add(doc);
>  }
>  
>  void InspectorDOMAgent::stopListening(Document* doc)
>  {
> -    if (!m_documents.contains(doc))
> -        return;
> -
> -    doc->removeEventListener(eventNames().DOMContentLoadedEvent, this, false);
> -    doc->removeEventListener(eventNames().loadEvent, this, true);
>      m_documents.remove(doc);
>  }

There are no accessors to the m_documents map but mainFrameDocument.
Looks like we can drop map and handle only MainFrame document pointer.
Comment 5 Pavel Feldman 2011-02-11 06:34:29 PST
Created attachment 82128 [details]
Patch
Comment 6 Yury Semikhatsky 2011-02-11 06:36:33 PST
Comment on attachment 82128 [details]
Patch

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

> LayoutTests/inspector/command-line-api.html:8
> +	var expressions = [

This fix should go in a separate change, r- for this.

> LayoutTests/inspector/command-line-api.html:19
> +            InspectorTest.completeTest();

wrong alignment
Comment 7 Pavel Feldman 2011-02-11 06:40:11 PST
Created attachment 82129 [details]
Patch
Comment 8 Yury Semikhatsky 2011-02-11 06:47:46 PST
Comment on attachment 82129 [details]
Patch

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

> Source/WebCore/inspector/InspectorDatabaseAgent.h:45
> +    class FrontendProvider : public RefCounted<FrontendProvider> {

No need to define this class in the public part of the agent. You can just put forward declaration here and define the class in .cpp.
Comment 9 Pavel Feldman 2011-02-11 07:23:54 PST
Committed r78336: <http://trac.webkit.org/changeset/78336>