Bug 43344 – Web Inspector: Extensions API tests fail on Qt & GTK

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

[Andrey Kosyakov]

inspector/extensions-api.html -> failed
.
inspector/extensions.html -> failed
......
inspector/styles-source-lines-inline.html -> failed

(The last failure is induced by lack of cleanup from extension* tests)

[Ilya Tikhonovsky]

The tests were temporary disabled

Committed r64459
	M	LayoutTests/platform/chromium/test_expectations.txt
	M	LayoutTests/platform/qt/Skipped
	M	LayoutTests/platform/gtk/Skipped
	M	LayoutTests/ChangeLog
r64459 = 70396b2cf3f66fef8028a1a0caacf7139b6014e4 (refs/remotes/trunk)

[Andrey Kosyakov]

Created attachment 63367 [details]
patch

The tests broke because platform-specific DumpRenderTree code for Qt & GTK happened to have developerExtrasEnabled set to false. This was enabled per-page by the test running code, but not for the inspector page itself.
Performing API injection conditionally on WebInspector::enabled() happened to be bad idea when enabled() may vary per-page. The only chance to have something in m_scriptsToEvaluateOnLoad is via WebInspector, so injecting these scripts when inspector is not enabled() for inspector's page should be ok.

[Andrey Kosyakov]

... this also fixes a bug in initial change where condition for discardInjectedScripts() wasn't properly inverted.

[Joseph Pecoraro]

This patch confuses me. I don't know the loading and teardown well enough yet.
I assume it would be okay to discard injected scripts if the inspector is disabled,
because there probably wouldn't be any at all. But I don't see why we should inject
scripts, even if it is disabled. You mention it should be harmless, but is there an
advantage to doing it?

[Pavel Feldman]

> Performing API injection conditionally on WebInspector::enabled() happened to be bad idea when enabled() may vary per-page. The only chance to have something in m_scriptsToEvaluateOnLoad is via WebInspector, so injecting these scripts when inspector is not enabled() for inspector's page should be ok.

It is just that we should use a different way of injecting extension scripts on load as we discussed earlier. And we probably want to remote existing schema as a whole once workers get real debugging capabilities.

[Pavel Feldman]

Comment on attachment 63367 [details]
patch

WebCore/inspector/InspectorController.cpp:464
 +      if (m_scriptsToEvaluateOnLoad.size()) {
So we are now hitting this code for every page. I never liked this thing, I don't think we should extend its capabilities.

[Andrey Kosyakov]

(In reply to comment #6)
> (From update of attachment 63367 [details])
> WebCore/inspector/InspectorController.cpp:464
>  +      if (m_scriptsToEvaluateOnLoad.size()) {
> So we are now hitting this code for every page. 

Yes we do, but what's the problem with this? This is the cheapest check that we're going to have ever. The only way something under this conditional is going to be executed is when inspector previously added scripts to evaluate on load, which currently only happens either (a) for a page being inspected if user has enabled workers debugging or (b) for inspector page if addExtensions has been called.

This also used to work unconditionally on enabled(), just upon a slightly later event for half a year: http://trac.webkit.org/changeset/64458/trunk/WebCore/inspector/InspectorController.cpp

> I never liked this thing, I don't think we should extend its capabilities.

What we essentially need is an ability of inspector to inject some JS to certain pages at the earlist possible moment. We now have a generic means for that, which covers inspector extensions, workers and any possible other case where we could need to emulate some API for page being inspected (we intentially made the scheme generic for that, as you suggested back when it was introdcued). Now, we could have it more specialized for extensions API, but it would be doing roughly the same: setting the API from inspector and injecting it in about the same moment into inspector's page. So, what exactly would be the difference that would justify the additional code?

[Andrey Kosyakov]

Created attachment 63443 [details]
patch

As discussed offline -- I'm splitting script injection for extensions API and for inspected page. The formers goes without enabled() check, the latter is only performed if enabled().

[WebKit Commit Bot]

Comment on attachment 63443 [details]
patch

Clearing flags on attachment: 63443

Committed r64745: <http://trac.webkit.org/changeset/64745>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.