Bug 74603 – Web Inspector: Implement CSS selector profiler backend

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

[Alexander Pavlov (apavlov)]

Patch to follow.

[Alexander Pavlov (apavlov)]

Created attachment 119425 [details]
Patch

[WebKit Review Bot]

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.

[Pavel Feldman]

Comment on attachment 119425 [details]
Patch

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

[Alexander Pavlov (apavlov)]

An updated patch to be attached shortly.

(In reply to comment #3)
> (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.

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.

[Pavel Feldman]

> 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.

[Alexander Pavlov (apavlov)]

(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.

[Alexander Pavlov (apavlov)]

Created attachment 119589 [details]
Patch

[WebKit Review Bot]

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.

[Pavel Feldman]

Comment on attachment 119589 [details]
Patch

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.

[Alexander Pavlov (apavlov)]

Created attachment 119847 [details]
Patch

[WebKit Review Bot]

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.

[Pavel Feldman]

Comment on attachment 119847 [details]
Patch

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.

[Alexander Pavlov (apavlov)]

Created attachment 119852 [details]
Patch

[WebKit Review Bot]

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.

[Pavel Feldman]

Comment on attachment 119852 [details]
Patch

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

[Alexander Pavlov (apavlov)]

Created attachment 119859 [details]
Patch

[WebKit Review Bot]

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.

[Pavel Feldman]

Comment on attachment 119859 [details]
Patch

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.

[Alexander Pavlov (apavlov)]

Committed r103236: <http://trac.webkit.org/changeset/103236>