[webkit-reviews] review granted: [Bug 192016] Web Inspector: REGRESSION(?): all "Show *" develop menu items cause the page to crash : [Attachment 355936] [PATCH] Proposed Fix

[bugzilla-daemon at webkit.org]

Devin Rousso <drousso at apple.com> has granted Joseph Pecoraro
<joepeck at webkit.org>'s request for review:
Bug 192016: Web Inspector: REGRESSION(?): all "Show *" develop menu items cause
the page to crash
https://bugs.webkit.org/show_bug.cgi?id=192016

Attachment 355936: [PATCH] Proposed Fix

https://bugs.webkit.org/attachment.cgi?id=355936&action=review




--- Comment #5 from Devin Rousso <drousso at apple.com> ---
Comment on attachment 355936
  --> https://bugs.webkit.org/attachment.cgi?id=355936
[PATCH] Proposed Fix

View in context: https://bugs.webkit.org/attachment.cgi?id=355936&action=review

r=me

> Source/WebInspectorUI/UserInterface/Base/Main.js:160
> +    this._targetsAvailablePromise = new WI.WrappedPromise;

This does seem like the "perfect" use case for a Promise, but it scares me a
bit cause it means delaying the code it runs just _that_ much more. 
Considering this is at startup, I'd like to avoid any delays (no matter how
small) whenever/wherever we can.  Perhaps we can do something like this:

--- WI.loaded ---
	this._loadOperations = new Set;
--- WI.loaded ---

--- WI.initializeBackendTarget ---
	this._completeLoadOperation("initializeBackendTarget");
--- WI.initializeBackendTarget ---

--- WI.contentLoaded ---
	this._completeLoadOperation("contentLoaded");
--- WI.contentLoaded ---

    WI._completeLoadOperation = function(name) {
	this._loadOperations.add(name);
	if (this._loadOperations.has("initializeBackendTarget") &&
this._loadOperations.has("contentLoaded"))
	    InspectorFrontendAPI.loadCompleted();
    };

> Source/WebInspectorUI/UserInterface/Base/Main.js:559
> +    })

Style: missing `;`

> Source/WebInspectorUI/UserInterface/Base/Main.js:588
> +WI.whenTargetsAvailable = function()

Since `WI._targetsAvailablePromise` is only used within `Main.js`, can we
remove this function and just inline:

    WI._targetsAvailablePromise.promise.then(() => {
	InspectorFrontendAPI.loadCompleted();
    });

As an aside, maybe we should add a `then()` to `WI.WrappedPromise` so we can
avoid this 🤔 (e.g. forwarding)

> Source/WebInspectorUI/UserInterface/Base/Main.js:591
> +}

Style: missing `;`

> Source/WebKit/ChangeLog:12
> +	   once the UIProcess creates it. So queue actions that to take place

Typo: "actions that take place"

> Source/WebKit/WebProcess/WebPage/WebInspector.cpp:195
> +    whenFrontendConnectionEstablished([=] {

Capture by value?  Not by reference?

> Source/WebKit/WebProcess/WebPage/WebInspector.cpp:207
> +    whenFrontendConnectionEstablished([=] {

Ditto (195)

> Source/WebKit/WebProcess/WebPage/WebInspector.cpp:219
> +    whenFrontendConnectionEstablished([=] {

Ditto (195)

> Source/WebKit/WebProcess/WebPage/WebInspector.cpp:238
> +	  
m_frontendConnection->send(Messages::WebInspectorUI::ShowMainResourceForFrame(i
nspectorFrameIdentifier), 0);

Ditto (195)

> Source/WebKit/WebProcess/WebPage/WebInspector.cpp:247
> +    whenFrontendConnectionEstablished([=] {

Ditto (195)

> Source/WebKit/WebProcess/WebPage/WebInspector.cpp:257
> +    whenFrontendConnectionEstablished([=] {

Ditto (195)

> Source/WebKit/WebProcess/WebPage/WebInspector.cpp:267
> +    whenFrontendConnectionEstablished([=] {

Ditto (195)

> Source/WebKit/WebProcess/WebPage/WebInspector.cpp:277
> +    whenFrontendConnectionEstablished([=] {

Ditto (195)