Bug 230758 – Web Inspector: WebInspectorExtensionTabContentView should not reload its iframe when detached

• Component: Web Inspector
• Status: RESOLVED FIXED

[Blaze Burg]

<rdar://74714861>

[Blaze Burg]

I've written an API test that demonstrates the issue :D

[Blaze Burg]

Created attachment 439785 [details]
Patch v1.0

[Patrick Angle]

Comment on attachment 439785 [details]
Patch v1.0

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

> Source/WebInspectorUI/ChangeLog:25
> +        (WI.ContentView.prototype.get shouldDetachWhenHidden): Add new static getter, default is `true`.

If this is supposed to be static, it should probably be in a separate section of the class from the overridable instance variables? I left some comments below before I looped back to this one, but I also left a comment on ContentViewContainer.js:426 making the argument that this shouldn't be treated as static, even if currently no implementation dynamically determines this.

> Source/WebInspectorUI/UserInterface/Views/ContentView.js:380
> +        // Returns false if the content view should *not* be detached from the DOM when hidden.

NIT: This comment could probably be replaced with a slightly more descriptive name instead, e.g. `shouldRemoveFromDOMWhenHidden`
NIT: Should have a comment `// Implemented by subclasses.`

> Source/WebInspectorUI/UserInterface/Views/ContentViewContainer.js:426
> +        if (!contentView.constructor.shouldDetachWhenHidden && contentView.isAttached)

Why `.constructor.`? It seems like we wouldn't want that if a future ContentView subclass dynamically determined `shouldDetachWhenHidden`.

> Source/WebInspectorUI/UserInterface/Views/ContentViewContainer.js:465
> +        else if (!entry.contentView.constructor.shouldDetachWhenHidden) {

Ditto :426

> Source/WebInspectorUI/UserInterface/Views/ContentViewContainer.js:466
> +            entry.contentView.element.classList.remove("hidden-for-detach");

`hidden-simulating-dom-detached`? The current class name doesn't quite sound right to me since we aren't actually detaching the view.

> Source/WebInspectorUI/UserInterface/Views/ContentViewContainer.js:484
> +            if (entry.contentView.constructor.shouldDetachWhenHidden)

Ditto :426

> Source/WebInspectorUI/UserInterface/Views/ContentViewContainer.js:487
> +                entry.contentView.element.classList.add("hidden-for-detach");

Ditto :466

> Source/WebInspectorUI/UserInterface/Views/ContentViewContainer.js:488
> +                entry.contentView.detached();

See ContentView.js:380 - having the check be `entry.contentView.shouldDetachWhenHidden` and only calling `detached` here when the former is false isn't intuitive. A better name would help this.

> Source/WebInspectorUI/UserInterface/Views/WebInspectorExtensionTabContentView.js:65
> +    // Protected

This is marked as `// Public` in the superclass, and will be read by other classes, so protected doesn't make sense here.

> Tools/TestWebKitAPI/Tests/WebKitCocoa/InspectorExtension-basic-tab.html:15
> +    // Used by WKInspectorExtension.ExtensionTabIsPersistent
> +    window.getUniqueValue = function() {
> +        if (!window._cachedUniqueValue) {
> +            window._cachedUniqueValue = Math.floor(Math.random() * 10e9);
> +            document.getElementById("uniqueValueField").innerText = window._cachedUniqueValue;
> +        }
> +
> +        return window._cachedUniqueValue;
> +    }

I think this and the test itself might be easier to follow if the test started by calling something like `setUniqueValue()` that performed the first half of this logic, but with a stable value (e.g. `window.setUniqueValue(42)`), and then future tests just call window.getUniqueValue() and make sure it is the magic value the test set instead of the default undefined/TBD.

> Tools/TestWebKitAPI/Tests/WebKitCocoa/WKInspectorExtension.mm:208
> +    auto extensionDisplayName = @"SecondExtension";

Are the extensions persisting between tests requiring this extension to have a different name, source URL, etc.? If so it seems like we may want tests to clean up after themselves - if not, why the change from `FirstExtension` to `SecondExtension` here?

> Tools/TestWebKitAPI/Tests/WebKitCocoa/WKInspectorExtension.mm:248
> +    auto scriptSource = @"!!window.getUniqueValue && window.getUniqueValue()";

See InspectorExtension-basic-tab.html:7-15

[Devin Rousso]

Comment on attachment 439785 [details]
Patch v1.0

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

>> Source/WebInspectorUI/ChangeLog:25
>> +        (WI.ContentView.prototype.get shouldDetachWhenHidden): Add new static getter, default is `true`.
> 
> If this is supposed to be static, it should probably be in a separate section of the class from the overridable instance variables? I left some comments below before I looped back to this one, but I also left a comment on ContentViewContainer.js:426 making the argument that this shouldn't be treated as static, even if currently no implementation dynamically determines this.

I think "static" was meant more as a "never changes" instead of "defined on the class object instead of per-instance".  This is how most of the other `WI.ContentView` things are done.

In this case tho I do kinda agree that maybe this should be a `static` method (or at least have some sort of comment explaining that this should not change at runtime) so that we never run into a scenario where it's `false` when the `WI.ContentView` is removed and then somehow `true` when it's added back (which could trigger some assertions about the `WI.View` already being parented).

> Source/WebInspectorUI/UserInterface/Debug/Bootstrap.js:153
> +            if (WI.sharedApp.extensionController.registeredExtensions.has(mockData.extensionID))

Instead of `get registeredExtensions` can we make a `isValidExtensionID()` so that we don't have to expose the internally used `Map` (this would also be a little more efficient as we wouldn't need to copy the `.keys()` to a `Set`)?

>> Source/WebInspectorUI/UserInterface/Views/ContentView.js:380
>> +        // Returns false if the content view should *not* be detached from the DOM when hidden.
> 
> NIT: This comment could probably be replaced with a slightly more descriptive name instead, e.g. `shouldRemoveFromDOMWhenHidden`
> NIT: Should have a comment `// Implemented by subclasses.`

+1 to both of these

also see my comment above

> Source/WebInspectorUI/UserInterface/Views/ContentView.js:381
> +        return true;

NIT: We normally prefer optional things default to a falsy value, so maybe we could rename this to keep that trend?  How about `shouldNotRemoveFromDOM`?

> Source/WebInspectorUI/UserInterface/Views/ContentViewContainer.css:41
> +.content-view-container > .content-view.hidden-for-detach {

Is this a specific enough selector?  Do we need to add an `!important` (yuck, but perhaps the least bad option)?

> Source/WebInspectorUI/UserInterface/Views/ContentViewContainer.css:42
> +    display:  none;

Style: extra space

>> Source/WebInspectorUI/UserInterface/Views/ContentViewContainer.js:426
>> +        if (!contentView.constructor.shouldDetachWhenHidden && contentView.isAttached)
> 
> Why `.constructor.`? It seems like we wouldn't want that if a future ContentView subclass dynamically determined `shouldDetachWhenHidden`.

This also doesn't make sense as `shouldNotRemoveFromDOM` is not `static`, meaning that it'll never be defined on `WI.ContentView` (the constructor object).

>> Source/WebInspectorUI/UserInterface/Views/ContentViewContainer.js:488
>> +                entry.contentView.detached();
> 
> See ContentView.js:380 - having the check be `entry.contentView.shouldDetachWhenHidden` and only calling `detached` here when the former is false isn't intuitive. A better name would help this.

Yeah I dislike using the term "detached" since it's been overloaded by the whole `WI.View` system to mean something other than "not part of the main DOM tree".

Also, this is kinda slightly misleading since `entry.contentView.isAttached` will still be `true` since `entry.contentView._didMoveToParent(null)` was never called.  Perhaps `shouldNotRemoveFromDOM` should really be defined on `WI.View` so that this logic can be done inside `WI.View.prototype.addSubview` and `WI.View.prototype.removeSubview` instead, especially since there's really nothing about `shouldNotRemoveFromDOM` that's specific to `WI.ContentView` (i.e. I could see this being useful for other non-`WI.ContentView` things too).

> Tools/TestWebKitAPI/Tests/WebKitCocoa/WKInspectorExtension.mm:262
> +    [[webView _inspector] showConsole];

NIT: Does this actually jump to the Console Tab or just show the Split Console?  Perhaps we should use `-showResource` instead in case that behavior changes in the future.

[Blaze Burg]

Comment on attachment 439785 [details]
Patch v1.0

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

>>> Source/WebInspectorUI/ChangeLog:25
>>> +        (WI.ContentView.prototype.get shouldDetachWhenHidden): Add new static getter, default is `true`.
>> 
>> If this is supposed to be static, it should probably be in a separate section of the class from the overridable instance variables? I left some comments below before I looped back to this one, but I also left a comment on ContentViewContainer.js:426 making the argument that this shouldn't be treated as static, even if currently no implementation dynamically determines this.
> 
> I think "static" was meant more as a "never changes" instead of "defined on the class object instead of per-instance".  This is how most of the other `WI.ContentView` things are done.
> 
> In this case tho I do kinda agree that maybe this should be a `static` method (or at least have some sort of comment explaining that this should not change at runtime) so that we never run into a scenario where it's `false` when the `WI.ContentView` is removed and then somehow `true` when it's added back (which could trigger some assertions about the `WI.View` already being parented).

I changed it to a static method. Sorry about the confusion.

>> Source/WebInspectorUI/UserInterface/Debug/Bootstrap.js:153
>> +            if (WI.sharedApp.extensionController.registeredExtensions.has(mockData.extensionID))
> 
> Instead of `get registeredExtensions` can we make a `isValidExtensionID()` so that we don't have to expose the internally used `Map` (this would also be a little more efficient as we wouldn't need to copy the `.keys()` to a `Set`)?

I don't understand the problem–the Map itself can't be mutated by the client, and the returned values are string keys copied from an iterator. I do think this would be problematic if the iterator came from the Map itself. This is Bootstrap.js code in engineering builds only so I'm not worried about efficiency for efficiency's sake. (Also I use this as a dumper for debugging and logging pretty often.)

>> Source/WebInspectorUI/UserInterface/Views/ContentView.js:381
>> +        return true;
> 
> NIT: We normally prefer optional things default to a falsy value, so maybe we could rename this to keep that trend?  How about `shouldNotRemoveFromDOM`?

OK, I knew this but couldn't come up with an appropriate name. Thanks!

>>> Source/WebInspectorUI/UserInterface/Views/ContentViewContainer.js:488
>>> +                entry.contentView.detached();
>> 
>> See ContentView.js:380 - having the check be `entry.contentView.shouldDetachWhenHidden` and only calling `detached` here when the former is false isn't intuitive. A better name would help this.
> 
> Yeah I dislike using the term "detached" since it's been overloaded by the whole `WI.View` system to mean something other than "not part of the main DOM tree".
> 
> Also, this is kinda slightly misleading since `entry.contentView.isAttached` will still be `true` since `entry.contentView._didMoveToParent(null)` was never called.  Perhaps `shouldNotRemoveFromDOM` should really be defined on `WI.View` so that this logic can be done inside `WI.View.prototype.addSubview` and `WI.View.prototype.removeSubview` instead, especially since there's really nothing about `shouldNotRemoveFromDOM` that's specific to `WI.ContentView` (i.e. I could see this being useful for other non-`WI.ContentView` things too).

I wanted to keep this conceptually as part of the ContentViewContainer system. If there is a concrete reason to make it more generic then let me know. I just don't have time to over-build and test this functionality outside of the tabs / ContentViewContainer use case.

>> Tools/TestWebKitAPI/Tests/WebKitCocoa/InspectorExtension-basic-tab.html:15
>> +    }
> 
> I think this and the test itself might be easier to follow if the test started by calling something like `setUniqueValue()` that performed the first half of this logic, but with a stable value (e.g. `window.setUniqueValue(42)`), and then future tests just call window.getUniqueValue() and make sure it is the magic value the test set instead of the default undefined/TBD.

I considered doing that. This is what the other test case does. However, if the <iframe> has loaded about:blank (the previous buggy behavior upon hiding) then setting and getting the value will work as expected, and yet a major bug will not be detected (wrong page has loaded into the iframe).

>> Tools/TestWebKitAPI/Tests/WebKitCocoa/WKInspectorExtension.mm:208
>> +    auto extensionDisplayName = @"SecondExtension";
> 
> Are the extensions persisting between tests requiring this extension to have a different name, source URL, etc.? If so it seems like we may want tests to clean up after themselves - if not, why the change from `FirstExtension` to `SecondExtension` here?

The call to -unregisterExtension will clean up. Besides, each of these tests creates a fresh WKWebView / _WKInspector, and the set of _WKInspectorExtension currently active is not persisted anywhere in WebInspectorUI.

The use of different names is to make it easier to keep track of what test is actually running when remote inspecting TestWebKitAPI.

>> Tools/TestWebKitAPI/Tests/WebKitCocoa/WKInspectorExtension.mm:262
>> +    [[webView _inspector] showConsole];
> 
> NIT: Does this actually jump to the Console Tab or just show the Split Console?  Perhaps we should use `-showResource` instead in case that behavior changes in the future.

It jumps to the Console Tab. I considered renaming to showConsoleTab, but it would require Safari changes. It's not worth the trouble IMO.

If the behavior of -showConsole changes in the future such that didHideExtensionTabWasCalled no longer fires, then this test will need to be amended in order to pass.

[Blaze Burg]

Created attachment 440080 [details]
Patch v1.1

[Patrick Angle]

Comment on attachment 440080 [details]
Patch v1.1

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

Provisional r+ from me with a few NITs.

> Source/WebInspectorUI/UserInterface/Views/ContentView.js:45
> +    // Implemented by subclasses.

NIT: This comment should be inside the method like below for the public overridables.

> Source/WebInspectorUI/UserInterface/Views/WebInspectorExtensionTabContentView.js:66
> +    // This is necessary to avoid the <iframe> content from being reloaded when the extension tab is hidden.
> +    static shouldNotRemoveFromDOMWhenHidden() { return true; }

NIT: Should probably put this below with `static shouldSaveTab()` to keep the static functions together.

> Source/WebInspectorUI/UserInterface/Views/WebInspectorExtensionTabContentView.js:68
> +    // Protected

Aren't `attached` and `detached` public?

[Devin Rousso]

Comment on attachment 440080 [details]
Patch v1.1

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

> Source/WebInspectorUI/UserInterface/Controllers/WebInspectorExtensionController.js:40
> +    get registeredExtensions()

NIT: `registeredExtensionIDs`

> Source/WebInspectorUI/UserInterface/Views/ContentViewContainer.js:426
> +        if (contentView.constructor.shouldNotRemoveFromDOMWhenHidden() && contentView.isAttached)

`contentView.isAttached` will _always_ be true for something that is `shouldNotRemoveFromDOMWhenHidden` since nothing ever calls `contentView._didMoveToParent(null)` which means that `_isAttachedToRoot` is still `true`

perhaps instead of manually calling `attached()` and `detached()` you could `contentView._didMoveToParent(this)` and `contentView._didMoveToParent(null)`? though this would be using a "private" function outside of the owner class :/

this also may run into issues since we're still leaving the `contentView` inside `this._subviews`

>> Source/WebInspectorUI/UserInterface/Views/WebInspectorExtensionTabContentView.js:66
>> +    static shouldNotRemoveFromDOMWhenHidden() { return true; }
> 
> NIT: Should probably put this below with `static shouldSaveTab()` to keep the static functions together.

also please put the comment inside the function not above it

[Blaze Burg]

Comment on attachment 440080 [details]
Patch v1.1

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

>> Source/WebInspectorUI/UserInterface/Views/ContentViewContainer.js:426
>> +        if (contentView.constructor.shouldNotRemoveFromDOMWhenHidden() && contentView.isAttached)
> 
> `contentView.isAttached` will _always_ be true for something that is `shouldNotRemoveFromDOMWhenHidden` since nothing ever calls `contentView._didMoveToParent(null)` which means that `_isAttachedToRoot` is still `true`
> 
> perhaps instead of manually calling `attached()` and `detached()` you could `contentView._didMoveToParent(this)` and `contentView._didMoveToParent(null)`? though this would be using a "private" function outside of the owner class :/
> 
> this also may run into issues since we're still leaving the `contentView` inside `this._subviews`

I think it's OK to drop the `contentView.isAttached` check as it is not necessary.

I avoided calling _didMoveToParent() because that is what eventually attaches/detaches elements from the DOM.

[Devin Rousso]

Comment on attachment 440080 [details]
Patch v1.1

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

> Source/WebInspectorUI/UserInterface/Views/ContentView.js:46
> +    static shouldNotRemoveFromDOMWhenHidden()

NIT: I just realized, I feel like we maybe should make this into a `static get` so that it's even more "hinted" at being something that shouldn't have any complicated work inside it

>>> Source/WebInspectorUI/UserInterface/Views/ContentViewContainer.js:426
>>> +        if (contentView.constructor.shouldNotRemoveFromDOMWhenHidden() && contentView.isAttached)
>> 
>> `contentView.isAttached` will _always_ be true for something that is `shouldNotRemoveFromDOMWhenHidden` since nothing ever calls `contentView._didMoveToParent(null)` which means that `_isAttachedToRoot` is still `true`
>> 
>> perhaps instead of manually calling `attached()` and `detached()` you could `contentView._didMoveToParent(this)` and `contentView._didMoveToParent(null)`? though this would be using a "private" function outside of the owner class :/
>> 
>> this also may run into issues since we're still leaving the `contentView` inside `this._subviews`
> 
> I think it's OK to drop the `contentView.isAttached` check as it is not necessary.
> 
> I avoided calling _didMoveToParent() because that is what eventually attaches/detaches elements from the DOM.

Unless I'm reading it wrong, I'm pretty sure the code that adds/removes from the DOM is actually inside `insertSubviewBefore`/`removeSubview`.  `_didMoveToParent` just handles the `WI.View` logic of keeping track of dirty counts and pending layouts and whatnot.

[Blaze Burg]

Comment on attachment 440080 [details]
Patch v1.1

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

>> Source/WebInspectorUI/UserInterface/Controllers/WebInspectorExtensionController.js:40
>> +    get registeredExtensions()
> 
> NIT: `registeredExtensionIDs`

Good catch

>> Source/WebInspectorUI/UserInterface/Views/ContentView.js:46
>> +    static shouldNotRemoveFromDOMWhenHidden()
> 
> NIT: I just realized, I feel like we maybe should make this into a `static get` so that it's even more "hinted" at being something that shouldn't have any complicated work inside it

I'd prefer it remain a static method like other ContentView / View opt-ins. If you'd like to update the prevailing style everywhere, I wouldn't stop you.

>>>> Source/WebInspectorUI/UserInterface/Views/ContentViewContainer.js:426
>>>> +        if (contentView.constructor.shouldNotRemoveFromDOMWhenHidden() && contentView.isAttached)
>>> 
>>> `contentView.isAttached` will _always_ be true for something that is `shouldNotRemoveFromDOMWhenHidden` since nothing ever calls `contentView._didMoveToParent(null)` which means that `_isAttachedToRoot` is still `true`
>>> 
>>> perhaps instead of manually calling `attached()` and `detached()` you could `contentView._didMoveToParent(this)` and `contentView._didMoveToParent(null)`? though this would be using a "private" function outside of the owner class :/
>>> 
>>> this also may run into issues since we're still leaving the `contentView` inside `this._subviews`
>> 
>> I think it's OK to drop the `contentView.isAttached` check as it is not necessary.
>> 
>> I avoided calling _didMoveToParent() because that is what eventually attaches/detaches elements from the DOM.
> 
> Unless I'm reading it wrong, I'm pretty sure the code that adds/removes from the DOM is actually inside `insertSubviewBefore`/`removeSubview`.  `_didMoveToParent` just handles the `WI.View` logic of keeping track of dirty counts and pending layouts and whatnot.

Oops, you are right. I must have misunderstood what was going on while hacking. I will replace attached() and detached().

>>> Source/WebInspectorUI/UserInterface/Views/WebInspectorExtensionTabContentView.js:66
>>> +    static shouldNotRemoveFromDOMWhenHidden() { return true; }
>> 
>> NIT: Should probably put this below with `static shouldSaveTab()` to keep the static functions together.
> 
> also please put the comment inside the function not above it

OK

>> Source/WebInspectorUI/UserInterface/Views/WebInspectorExtensionTabContentView.js:68
>> +    // Protected
> 
> Aren't `attached` and `detached` public?

OK

[Blaze Burg]

Created attachment 440255 [details]
Patch v1.2

Patch v1.2 addresses all outstanding comments.

[Patrick Angle]

Comment on attachment 440255 [details]
Patch v1.2

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

> Source/WebInspectorUI/UserInterface/Views/ContentView.js:45
> +    // Implemented by subclasses.

NIT: Put this inside the function.

> Source/WebInspectorUI/UserInterface/Views/WebInspectorExtensionTabContentView.js:92
> +

NIT: Extra newline

[Blaze Burg]

Comment on attachment 440255 [details]
Patch v1.2

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

>> Source/WebInspectorUI/UserInterface/Views/ContentView.js:45
>> +    // Implemented by subclasses.
> 
> NIT: Put this inside the function.

OK

[Blaze Burg]

Created attachment 440514 [details]
Patch v1.2.1 (for landing)

[EWS]

Committed r283728 (242652@main): <https://commits.webkit.org/242652@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 440514 [details].

[Devin Rousso]

Comment on attachment 440514 [details]
Patch v1.2.1 (for landing)

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

> Source/WebInspectorUI/UserInterface/Views/ContentViewContainer.js:467
> +            entry.contentView._didMoveToParent(this);

this is technically a private violation since this method is not public

FWIW i'd suggested moving the logic to `WI.View` to avoid this

can you file a bug for fixing this please?

> Source/WebInspectorUI/UserInterface/Views/ContentViewContainer.js:486
> +                entry.contentView._didMoveToParent(null);

ditto (:467)