Bug 74603

Summary: Web Inspector: Implement CSS selector profiler backend
Product: WebKit Reporter: Alexander Pavlov (apavlov) <apavlov@chromium.org>
Component: Web Inspector (Deprecated)Assignee: Alexander Pavlov (apavlov) <apavlov@chromium.org>
Status: RESOLVED FIXED    
Severity: Normal CC: apavlov@chromium.org, bweinstein@apple.com, joepeck@webkit.org, keishi@webkit.org, loislo@chromium.org, pfeldman@chromium.org, pmuellr@yahoo.com, rik@webkit.org, timothy@apple.com, webkit.review.bot@gmail.com, yurys@chromium.org
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
Bug Depends on:    
Bug Blocks: 74004    
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch
none
Patch pfeldman: review+

Description From 2011-12-15 06:02:58 PST
Patch to follow.
------- Comment #1 From 2011-12-15 07:27:27 PST -------
Created an attachment (id=119425) [details]
Patch
------- Comment #2 From 2011-12-15 07:30:15 PST -------
Attachment 119425 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1

Source/WebCore/inspector/InspectorCSSAgent.cpp:556:  The parameter type should use PassRefPtr instead of RefPtr.  [readability/pass_ptr] [5]
Source/WebCore/inspector/InspectorCSSAgent.cpp:563:  The parameter type should use PassRefPtr instead of RefPtr.  [readability/pass_ptr] [5]
Source/WebCore/inspector/InspectorCSSAgent.h:93:  The parameter type should use PassRefPtr instead of RefPtr.  [readability/pass_ptr] [5]
Source/WebCore/inspector/InspectorCSSAgent.h:94:  The parameter type should use PassRefPtr instead of RefPtr.  [readability/pass_ptr] [5]
Total errors found: 4 in 5 files


If any of these errors are false positives, please file a bug against check-webkit-style.
------- Comment #3 From 2011-12-15 07:40:28 PST -------
(From update of attachment 119425 [details])
View in context: https://bugs.webkit.org/attachment.cgi?id=119425&action=review

> Source/WebCore/inspector/Inspector.json:1619
> +                "type": "object",

(You should expand the properties here). I don't think you need a header here. CPU / Heap profilers have it due to historical reasons. I think CSS profiles are lightweight enough to be pushed into the front-end atomically.

> Source/WebCore/inspector/Inspector.json:1628
> +                    { "name": "hits", "type": "integer", "description": "Number of times this selector was considered a candidate for matching against DOM elements." },

hitCount ?

> Source/WebCore/inspector/Inspector.json:1629
> +                    { "name": "matches", "type": "integer", "description": "Number of times this selector actually matched a DOM element." }

matchCount ?

> Source/WebCore/inspector/Inspector.json:1780
> +                "name": "stopSelectorProfiler"

This one should return the profile body.

> Source/WebCore/inspector/Inspector.json:1783
> +                "name": "getProfileHeaders",

... and you should not need this.

> Source/WebCore/inspector/Inspector.json:1789
> +                "name": "getProfile",

and this

> Source/WebCore/inspector/Inspector.json:1798
> +                "name": "removeProfile",

and this

> Source/WebCore/inspector/Inspector.json:1804
> +                "name": "clearProfiles"

and this :)

> Source/WebCore/inspector/Inspector.json:1813
> +                "name": "addProfileHeader",

and this

> Source/WebCore/inspector/Inspector.json:1819
> +                "name": "setRecordingProfile",

and this
------- Comment #4 From 2011-12-15 08:51:37 PST -------
An updated patch to be attached shortly.

(In reply to comment #3)
> (From update of attachment 119425 [details] [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=119425&action=review
> 
> > Source/WebCore/inspector/Inspector.json:1619
> > +                "type": "object",
> 
> (You should expand the properties here). I don't think you need a header here. CPU / Heap profilers have it due to historical reasons. I think CSS profiles are lightweight enough to be pushed into the front-end atomically.

Fine with that.

> > Source/WebCore/inspector/Inspector.json:1628
> > +                    { "name": "hits", "type": "integer", "description": "Number of times this selector was considered a candidate for matching against DOM elements." },
> 
> hitCount ?

Done.

> > Source/WebCore/inspector/Inspector.json:1629
> > +                    { "name": "matches", "type": "integer", "description": "Number of times this selector actually matched a DOM element." }
> 
> matchCount ?

Done.

> > Source/WebCore/inspector/Inspector.json:1780
> > +                "name": "stopSelectorProfiler"
> 
> This one should return the profile body.

Done.

> > Source/WebCore/inspector/Inspector.json:1783
> > +                "name": "getProfileHeaders",
> 
> ... and you should not need this.

Done.

> > Source/WebCore/inspector/Inspector.json:1789
> > +                "name": "getProfile",
> 
> and this

I will need getProfiles() to load profiles after a frontend has been re-opened.

> > Source/WebCore/inspector/Inspector.json:1798
> > +                "name": "removeProfile",
> 
> and this

Done.

> > Source/WebCore/inspector/Inspector.json:1804
> > +                "name": "clearProfiles"
> 
> and this :)

This one is necessary to react on the "Clear all profiles" button click (so that no profiles will be returned next time the frontend is re-opened.)

> > Source/WebCore/inspector/Inspector.json:1813
> > +                "name": "addProfileHeader",
> 
> and this

Done.

> > Source/WebCore/inspector/Inspector.json:1819
> > +                "name": "setRecordingProfile",
> 
> and this

Done.
------- Comment #5 From 2011-12-15 09:25:17 PST -------
> I will need getProfiles() to load profiles after a frontend has been re-opened.

I'm fine with this for now. However, as a debugger, backend should not collect the snapshots unless absolutely necessary. We should establish appropriate persistence for the front-end (like an IDE would have) instead. It might be time to assess IndexedDB for that.
------- Comment #6 From 2011-12-16 02:09:11 PST -------
(In reply to comment #5)
> > I will need getProfiles() to load profiles after a frontend has been re-opened.
> 
> I'm fine with this for now. However, as a debugger, backend should not collect the snapshots unless absolutely necessary. We should establish appropriate persistence for the front-end (like an IDE would have) instead. It might be time to assess IndexedDB for that.

Is it already in a good shape now?
On a side note, removeProfile() is also necessary, since a user can hit "Delete" on a profile, and it should get removed both in the UI and in the BE.
------- Comment #7 From 2011-12-16 02:24:14 PST -------
Created an attachment (id=119589) [details]
Patch
------- Comment #8 From 2011-12-16 02:28:30 PST -------
Attachment 119589 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1

Source/WebCore/inspector/InspectorCSSAgent.cpp:550:  The parameter type should use PassRefPtr instead of RefPtr.  [readability/pass_ptr] [5]
Source/WebCore/inspector/InspectorCSSAgent.cpp:563:  The parameter type should use PassRefPtr instead of RefPtr.  [readability/pass_ptr] [5]
Source/WebCore/inspector/InspectorCSSAgent.h:92:  The parameter type should use PassRefPtr instead of RefPtr.  [readability/pass_ptr] [5]
Source/WebCore/inspector/InspectorCSSAgent.h:93:  The parameter type should use PassRefPtr instead of RefPtr.  [readability/pass_ptr] [5]
Total errors found: 4 in 5 files


If any of these errors are false positives, please file a bug against check-webkit-style.
------- Comment #9 From 2011-12-18 06:13:35 PST -------
(From update of attachment 119589 [details])
View in context: https://bugs.webkit.org/attachment.cgi?id=119589&action=review

> Source/WebCore/inspector/Inspector.json:1618
> +                "id": "ProfileEntry",

SelectorProfileEntry

> Source/WebCore/inspector/Inspector.json:1629
> +                "id": "Profile",

SelectorProfile

> Source/WebCore/inspector/Inspector.json:1633
> +                    { "name": "uid", "type": "integer", "description": "Profile identifier (unique among CSS selector profiles.)" },

Should be of SelectorProfileId type inherited from string.

> Source/WebCore/inspector/Inspector.json:1806
> +                "name": "setRecordingProfile",

Why would you need this?

> Source/WebCore/inspector/InspectorCSSAgent.cpp:220
> +        .setData(data);

When will we have types collections?

> Source/WebCore/inspector/InspectorCSSAgent.cpp:245
> +    , m_recordingUserInitiatedSelectorProfile(false)

Nit: managing profile ids is way too complex. Why don't we persist the profiles in localStorage for now? It would make your change significantly smaller.

> Source/WebCore/inspector/InspectorCSSAgent.cpp:546
> +    toggleRecordButton(true);

You should never control front-end state from the backend. Let is update itself from the callback to startSelectorProfiler command.

> Source/WebCore/inspector/InspectorCSSAgent.h:95
> +    void clearProfiles(ErrorString*) { resetProfilerState(); }

I think it is more appropriate not to introduce getProfiles, removeProfile, clearProfiles in this change and do front-end side persistence. I know you were copying the CPU / Heap profile schema, but it itself is broken in some cases / needs to persist heap snapshot on the page side in other cases.
------- Comment #10 From 2011-12-19 04:39:37 PST -------
Created an attachment (id=119847) [details]
Patch
------- Comment #11 From 2011-12-19 04:43:06 PST -------
Attachment 119847 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1

Source/WebCore/inspector/InspectorCSSAgent.cpp:549:  The parameter type should use PassRefPtr instead of RefPtr.  [readability/pass_ptr] [5]
Source/WebCore/inspector/InspectorCSSAgent.h:92:  The parameter type should use PassRefPtr instead of RefPtr.  [readability/pass_ptr] [5]
Total errors found: 2 in 5 files


If any of these errors are false positives, please file a bug against check-webkit-style.
------- Comment #12 From 2011-12-19 04:43:47 PST -------
(From update of attachment 119847 [details])
View in context: https://bugs.webkit.org/attachment.cgi?id=119847&action=review

> Source/WebCore/inspector/Inspector.json:1637
> +                    { "name": "title", "type": "string", "description": "Profile title." },

You don't need a title for the profile

> Source/WebCore/inspector/Inspector.json:1638
> +                    { "name": "uid", "$ref": "SelectorProfileId", "description": "Profile identifier (unique among CSS selector profiles.)" },

Neither you need the uid or typeId.

> Source/WebCore/inspector/InspectorCSSAgent.cpp:136
> +static const char userInitiatedSelectorProfiling[] = "userInitiatedSelectorProfiling";

Rename to "selectorProfilerStarted"

> Source/WebCore/inspector/InspectorCSSAgent.cpp:140
> +static const char* const UserInitiatedProfileName = "org.webkit.profiles.user-initiated";

ditto

> Source/WebCore/inspector/InspectorCSSAgent.h:193
> +    unsigned m_nextUserInitiatedSelectorProfileNumber;

You don't need this.

> Source/WebCore/inspector/InspectorCSSAgent.h:203
> +    SelectorProfilesMap m_selectorProfiles;

You don't need this.
------- Comment #13 From 2011-12-19 05:41:06 PST -------
Created an attachment (id=119852) [details]
Patch
------- Comment #14 From 2011-12-19 05:43:30 PST -------
Attachment 119852 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1

Source/WebCore/inspector/InspectorCSSAgent.cpp:584:  The parameter type should use PassRefPtr instead of RefPtr.  [readability/pass_ptr] [5]
Source/WebCore/inspector/InspectorCSSAgent.h:93:  The parameter type should use PassRefPtr instead of RefPtr.  [readability/pass_ptr] [5]
Total errors found: 2 in 4 files


If any of these errors are false positives, please file a bug against check-webkit-style.
------- Comment #15 From 2011-12-19 05:53:08 PST -------
(From update of attachment 119852 [details])
View in context: https://bugs.webkit.org/attachment.cgi?id=119852&action=review

> Source/WebCore/inspector/InspectorCSSAgent.cpp:151
> +        : totalTime(0.0), hits(0), matches(0)

m_totalTime, m_hits, m_matches

> Source/WebCore/inspector/InspectorCSSAgent.cpp:163
> +class SelectorProfile : public RefCounted<SelectorProfile> {

Why is this ref-counted?

> Source/WebCore/inspector/InspectorCSSAgent.cpp:282
> +    , m_recordingSelectorProfile(false)

nuke?

> Source/WebCore/inspector/InspectorCSSAgent.cpp:287
> +    , m_id(1)

nuke?

> Source/WebCore/inspector/InspectorCSSAgent.h:99
> +    int id() const { return m_id; }

You don't need this id.

> Source/WebCore/inspector/InspectorCSSAgent.h:102
> +    struct RuleMatchData {

Can this still be a part of profile?

> Source/WebCore/inspector/InspectorCSSAgent.h:145
> +    bool m_recordingSelectorProfile;

nuke.

> Source/WebCore/inspector/InspectorCSSAgent.h:153
> +    int m_id;

nuke

> Source/WebCore/inspector/InspectorCSSAgent.h:156
> +    RuleMatchData m_currentMatchData;

move to profile
------- Comment #16 From 2011-12-19 06:48:28 PST -------
Created an attachment (id=119859) [details]
Patch
------- Comment #17 From 2011-12-19 06:55:06 PST -------
Attachment 119859 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1

Source/WebCore/inspector/InspectorCSSAgent.cpp:590:  The parameter type should use PassRefPtr instead of RefPtr.  [readability/pass_ptr] [5]
Source/WebCore/inspector/InspectorCSSAgent.h:93:  The parameter type should use PassRefPtr instead of RefPtr.  [readability/pass_ptr] [5]
Total errors found: 2 in 4 files


If any of these errors are false positives, please file a bug against check-webkit-style.
------- Comment #18 From 2011-12-19 07:02:53 PST -------
(From update of attachment 119859 [details])
View in context: https://bugs.webkit.org/attachment.cgi?id=119859&action=review

> Source/WebCore/inspector/InspectorCSSAgent.cpp:332
> +    if (m_state->getBoolean(CSSAgentState::isSelectorProfiling))

This will not work.

> Source/WebCore/inspector/InspectorCSSAgent.cpp:583
> +    if (m_state->getBoolean(CSSAgentState::isSelectorProfiling))

so you should remove this check.

> Source/WebCore/inspector/InspectorCSSAgent.h:92
> +    void startSelectorProfiler(ErrorString* = 0);

0 error string would be unexpected to the method implementor.
------- Comment #19 From 2011-12-19 07:14:32 PST -------
Committed r103236: <http://trac.webkit.org/changeset/103236>