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)
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)
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.
... this also fixes a bug in initial change where condition for discardInjectedScripts() wasn't properly inverted.
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?
> 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.
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.
(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?
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().
Comment on attachment 63443 [details] patch Clearing flags on attachment: 63443 Committed r64745: <http://trac.webkit.org/changeset/64745>
All reviewed patches have been landed. Closing bug.