Bug 40886 – Web Inspector: Backend Should Provide Full Supported CSS Properties List

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

[Joseph Pecoraro]

Would be useful to have a full list calculated in the backend, so
that the frontend doesn't calculate it every page load. Places
that would be affected (for the better) are:

  - console autocompletion (injected side)
  InjectedScript._hiddenStyleProperties
  bug 38448

  - styles autocompletion (sidebar)
  WebInspector.CSSCompletions (bug 
  bug 17374

  - css tokenizer (highlighting)
  WebInspector.SourceCSSTokenizer._propertyKeywords
  http://trac.webkit.org/browser/trunk/WebCore/inspector/front-end/SourceCSSTokenizer.re2js

[Joseph Pecoraro]

This is what I see so far:

  window.getComputedStyle (no shorthands)
    - provides a list without shorthands, but with non-typical CSS properties
       such as SVG and dashboard styles. This is in effect using properties from
       any .in file with "CSSProperty" in the name, but it then strips shorthand
       properties.

  CSSTokenizer's list (shorthands, but missing some)
    - this is using everything from CSSProperty.in but not SVG, etc.

So, a total list of all styles, including shorthands, can come from:

    shell> find . -name '*CSSProperty*.in'
    ./css/CSSPropertyNames.in
    ./css/DashboardSupportCSSPropertyNames.in
    ./css/SVGCSSPropertyNames.in
    ./css/WCSSPropertyNames.in

There is already a global list generated with all of the above, it
is generated via css/makeprop.pl. The derived files are:

  .../DerivedSources/WebCore/CSSPropertyNames.cpp
  .../DerivedSources/WebCore/CSSPropertyNames.gperf
  .../DerivedSources/WebCore/CSSPropertyNames.h
  .../DerivedSources/WebCore/CSSPropertyNames.in

Inside .cpp we have this nice, global, list of all the CSS properties
that getComputedStyle provides, including shorthands!

Was it that easy?

[Joseph Pecoraro]

I got this working, but making asynchronous calls to the backend to get the
supported list of CSS properties probably isn't what we want (although I
could get it to work). I'll continue in this direction for a bit.

I think it would be nice to just run a script and generate a JavaScript file
for us. So I think later I will try this approach. The problem here is it
won't help with the Injected side, but the need for it on the injected
side (for now) can be fixed in a different way.

[Pavel Feldman]

> I think it would be nice to just run a script and generate a JavaScript file
> for us. So I think later I will try this approach. The problem here is it
> won't help with the Injected side, but the need for it on the injected
> side (for now) can be fixed in a different way.

Just expose it in the CSSPropertyNames.h and pass data to injected script via the InjectedScriptHost interface (should probably be a lazy getter). Then you can get the data on the inspected page side and everybody is happy!

[Joseph Pecoraro]

Created attachment 60276 [details]
[PATCH] InspectorFrontendHost Provides the List

This is no longer needed for the injected script side. This is only currently
needed for the frontend side.

@Pavel. I used ScriptArray and InspectorArray because this was a local call.
Do you agree with that, or should I have gone async?

Tests looked good, but I'm rebasing and testing now.

[WebKit Review Bot]

Attachment 60276 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style', '--no-squash']" exit_code: 1
WebCore/inspector/InspectorFrontendHost.cpp:39:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 1 in 8 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Joseph Pecoraro]

Created attachment 60304 [details]
[PATCH] Addressed Style Issue and Rebased

Tests passed just fine.

[Pavel Feldman]

Comment on attachment 60304 [details]
[PATCH] Addressed Style Issue and Rebased

WebCore/inspector/InspectorFrontendHost.cpp:35
 +  #include "CSSPropertyNames.h"
This should not be defined in a front-end host. Imagine the remote debugging scenario. Frontend host is a simple stub there. What you really need is to fetch the actual list of properties from the inspected side.

WebCore/inspector/InspectorFrontendHost.cpp:206
 +  ScriptArray InspectorFrontendHost::supportedCSSProperties()
ScriptArray will soon go away. But you can still use it in InspectorController - we will migrate everything alltogether. However, using debuggerWorld her is a hack - if you implement it properly on the inspected page side, you will not have such a problem - there is a factory for ScriptArrays.

WebCore/inspector/front-end/CSSCompletions.js:3
 +  WebInspector.CSSCompletions.startsWith = function(prefix)
Nit: When did we get this file... I think we should merge it with CSSStyleModel!

WebCore/inspector/front-end/SourceCSSTokenizer.js: 
 +      this._propertyKeywords = [
Nice!

[Joseph Pecoraro]

Created attachment 64933 [details]
[PATCH] List Generated on Backend

> This should not be defined in a front-end host. [...] What you really
> need is to fetch the actual list of properties from the inspected side.
> 
> ScriptArray will soon go away. [...]

Done by rebasing.

> WebCore/inspector/front-end/CSSCompletions.js:3
>  +  WebInspector.CSSCompletions.startsWith = function(prefix)
> Nit: When did we get this file... I think we should merge it with CSSStyleModel!

Hmm, I didn't address this yet. I think CSSCompletions, being
a special array is a bad idea (it required me to individually
copy over elements). I think this "special array" concept should
be removed, and it can be merged into CSSStyleModel as a
separate patch.

-----

Maybe, instead of "fetching it" from the Frontend, it should just be an automatic
[notify] from the backend once the frontend is connected? Let me know what
you think about that idea.

[Pavel Feldman]

Comment on attachment 64933 [details]
[PATCH] List Generated on Backend

Nice patch! Saves footprint too!

WebCore/inspector/front-end/inspector.js:561
 +      // As a DOMAgent method, this needs to happen after the frontend has loaded and the agent is available.
Nit: You did not make it a domAgent method, but are stating so in the comment.

[Joseph Pecoraro]

> WebCore/inspector/front-end/inspector.js:561
>  +      // As a DOMAgent method, this needs to happen after the frontend has loaded and the agent is available.
> Nit: You did not make it a domAgent method, but are stating so in the comment.

Hmm, I"ll take a look at putting this on the front end's DOM Agent, or maybe some other more appropriate place. It is just the backend's Dom agent that needs to be created, which isn't done until that frontend host loaded call.

[Joseph Pecoraro]

Committed r65942
	M	WebCore/ChangeLog
	M	WebCore/inspector/InspectorDOMAgent.h
	M	WebCore/inspector/Inspector.idl
	M	WebCore/inspector/front-end/SourceCSSTokenizer.js
	M	WebCore/inspector/front-end/CSSCompletions.js
	M	WebCore/inspector/front-end/SourceCSSTokenizer.re2js
	M	WebCore/inspector/front-end/inspector.js
	M	WebCore/inspector/InspectorDOMAgent.cpp
	M	WebCore/css/makeprop.pl
r65942 = 6e1dd170c5b9f7416d6f63d11db82e09fd707f6d
http://trac.webkit.org/changeset/65942