Title: [283728] trunk
Revision
283728
Author
bb...@apple.com
Date
2021-10-07 11:38:12 -0700 (Thu, 07 Oct 2021)

Log Message

Web Inspector: WebInspectorExtensionTabContentView should not reload its iframe when detached
https://bugs.webkit.org/show_bug.cgi?id=230758
<rdar://74714861>

Reviewed by Timothy Hatcher.

Source/WebInspectorUI:

When an <iframe> element detaches from the DOM, the script context is destroyed, which we don't
want to happen for _WKInspectorExtension tabs. Fix this by teaching ContentViewContainer to
'hide' such content views by setting `display:none` rather than detaching from the DOM.

* UserInterface/Views/ContentViewContainer.js:
(WI.ContentViewContainer.prototype._showEntry):
(WI.ContentViewContainer.prototype._hideEntry):
Set and unset 'display:none' instead of calling addSubview() / removeSubview().

* UserInterface/Views/ContentViewContainer.css:
(.content-view-container > .content-view.hidden-for-detach): Added.

(WI.ContentViewContainer.prototype._disassociateFromContentView):
Clean up any remaining content views that were not detached due to overriding shouldNotRemoveFromDOMWhenHidden.

* UserInterface/Views/ContentView.js:
(WI.ContentView.prototype.get shouldNotRemoveFromDOMWhenHidden): Added.

* UserInterface/Views/WebInspectorExtensionTabContentView.js:
(WI.WebInspectorExtensionTabContentView):
(WI.WebInspectorExtensionTabContentView.prototype.get shouldNotRemoveFromDOMWhenHidden):
Override this to opt into the alternate behavior that does not detach from the DOM. It is still
necessary to call attached() and detached() so that WebInpectorExtensionTabContentView can generate
didShowExtensionTab/didHideExtensionTab event callbacks.

* UserInterface/Controllers/WebInspectorExtensionController.js:
(WI.WebInspectorExtensionController.prototype.get registeredExtensionIDs):
* UserInterface/Debug/Bootstrap.js:
(updateMockWebExtensionTab):
(WI.runBootstrapOperations):
This is a drive-by fix to address a console assertion seen while developing the API test.
Don't unregister the mock extension if it is not registered in the first place.

Tools:

Add a new test to verify that re-selecting an extension tab does not cause it to reload.

* TestWebKitAPI/Tests/WebKitCocoa/InspectorExtension-basic-tab.html:
* TestWebKitAPI/Tests/WebKitCocoa/WKInspectorExtension.mm:
(TEST):

Modified Paths

Diff

Modified: trunk/Source/WebInspectorUI/ChangeLog (283727 => 283728)


--- trunk/Source/WebInspectorUI/ChangeLog	2021-10-07 18:35:01 UTC (rev 283727)
+++ trunk/Source/WebInspectorUI/ChangeLog	2021-10-07 18:38:12 UTC (rev 283728)
@@ -1,3 +1,44 @@
+2021-10-07  BJ Burg  <bb...@apple.com>
+
+        Web Inspector: WebInspectorExtensionTabContentView should not reload its iframe when detached
+        https://bugs.webkit.org/show_bug.cgi?id=230758
+        <rdar://74714861>
+
+        Reviewed by Timothy Hatcher.
+
+        When an <iframe> element detaches from the DOM, the script context is destroyed, which we don't
+        want to happen for _WKInspectorExtension tabs. Fix this by teaching ContentViewContainer to
+        'hide' such content views by setting `display:none` rather than detaching from the DOM.
+
+        * UserInterface/Views/ContentViewContainer.js:
+        (WI.ContentViewContainer.prototype._showEntry):
+        (WI.ContentViewContainer.prototype._hideEntry):
+        Set and unset 'display:none' instead of calling addSubview() / removeSubview().
+
+        * UserInterface/Views/ContentViewContainer.css:
+        (.content-view-container > .content-view.hidden-for-detach): Added.
+
+        (WI.ContentViewContainer.prototype._disassociateFromContentView):
+        Clean up any remaining content views that were not detached due to overriding shouldNotRemoveFromDOMWhenHidden.
+
+        * UserInterface/Views/ContentView.js:
+        (WI.ContentView.prototype.get shouldNotRemoveFromDOMWhenHidden): Added.
+
+        * UserInterface/Views/WebInspectorExtensionTabContentView.js:
+        (WI.WebInspectorExtensionTabContentView):
+        (WI.WebInspectorExtensionTabContentView.prototype.get shouldNotRemoveFromDOMWhenHidden):
+        Override this to opt into the alternate behavior that does not detach from the DOM. It is still
+        necessary to call attached() and detached() so that WebInpectorExtensionTabContentView can generate
+        didShowExtensionTab/didHideExtensionTab event callbacks.
+
+        * UserInterface/Controllers/WebInspectorExtensionController.js:
+        (WI.WebInspectorExtensionController.prototype.get registeredExtensionIDs):
+        * UserInterface/Debug/Bootstrap.js:
+        (updateMockWebExtensionTab):
+        (WI.runBootstrapOperations):
+        This is a drive-by fix to address a console assertion seen while developing the API test.
+        Don't unregister the mock extension if it is not registered in the first place.
+
 2021-10-07  Nikita Vasilyev  <nvasil...@apple.com>
 
         Web Inspector: Styles: format style declarations after editing

Modified: trunk/Source/WebInspectorUI/UserInterface/Controllers/WebInspectorExtensionController.js (283727 => 283728)


--- trunk/Source/WebInspectorUI/UserInterface/Controllers/WebInspectorExtensionController.js	2021-10-07 18:35:01 UTC (rev 283727)
+++ trunk/Source/WebInspectorUI/UserInterface/Controllers/WebInspectorExtensionController.js	2021-10-07 18:38:12 UTC (rev 283728)
@@ -37,6 +37,11 @@
 
     // Public
 
+    get registeredExtensionIDs()
+    {
+        return new Set(this._extensionForExtensionIDMap.keys());
+    }
+
     registerExtension(extensionID, displayName)
     {
         if (this._extensionForExtensionIDMap.has(extensionID)) {

Modified: trunk/Source/WebInspectorUI/UserInterface/Debug/Bootstrap.js (283727 => 283728)


--- trunk/Source/WebInspectorUI/UserInterface/Debug/Bootstrap.js	2021-10-07 18:35:01 UTC (rev 283727)
+++ trunk/Source/WebInspectorUI/UserInterface/Debug/Bootstrap.js	2021-10-07 18:38:12 UTC (rev 283728)
@@ -1,5 +1,6 @@
 /*
  * Copyright (C) 2015 University of Washington.
+ * Copyright (C) 2021 Apple Inc. All rights reserved.
  *
  * Redistribution and use in source and binary forms, with or without
  * modification, are permitted provided that the following conditions
@@ -150,7 +151,9 @@
 
         // Simulates the steps taken by WebInspectorUIExtensionController to create an extension tab in WebInspectorUI.
         if (!WI.settings.engineeringShowMockWebExtensionTab.value) {
-            InspectorFrontendAPI.unregisterExtension(mockData.extensionID);
+            if (WI.sharedApp.extensionController.registeredExtensionIDs.has(mockData.extensionID))
+                InspectorFrontendAPI.unregisterExtension(mockData.extensionID);
+
             return;
         }
 

Modified: trunk/Source/WebInspectorUI/UserInterface/Views/ContentView.js (283727 => 283728)


--- trunk/Source/WebInspectorUI/UserInterface/Views/ContentView.js	2021-10-07 18:35:01 UTC (rev 283727)
+++ trunk/Source/WebInspectorUI/UserInterface/Views/ContentView.js	2021-10-07 18:38:12 UTC (rev 283728)
@@ -1,5 +1,5 @@
 /*
- * Copyright (C) 2013, 2015 Apple Inc. All rights reserved.
+ * Copyright (C) 2013-2021 Apple Inc. All rights reserved.
  *
  * Redistribution and use in source and binary forms, with or without
  * modification, are permitted provided that the following conditions
@@ -42,6 +42,13 @@
 
     // Static
 
+    static shouldNotRemoveFromDOMWhenHidden()
+    {
+        // Implemented by subclasses.
+        // Returns true if the content view should *not* be detached from the DOM when hidden.
+        return false;
+    }
+
     static createFromRepresentedObject(representedObject, extraArguments)
     {
         console.assert(representedObject);

Modified: trunk/Source/WebInspectorUI/UserInterface/Views/ContentViewContainer.css (283727 => 283728)


--- trunk/Source/WebInspectorUI/UserInterface/Views/ContentViewContainer.css	2021-10-07 18:35:01 UTC (rev 283727)
+++ trunk/Source/WebInspectorUI/UserInterface/Views/ContentViewContainer.css	2021-10-07 18:38:12 UTC (rev 283728)
@@ -1,5 +1,5 @@
 /*
- * Copyright (C) 2013 Apple Inc. All rights reserved.
+ * Copyright (C) 2013-2021 Apple Inc. All rights reserved.
  *
  * Redistribution and use in source and binary forms, with or without
  * modification, are permitted provided that the following conditions
@@ -37,3 +37,7 @@
 
     overflow: hidden;
 }
+
+.content-view-container > .content-view.hidden-simulating-dom-detached {
+    display: none;
+}

Modified: trunk/Source/WebInspectorUI/UserInterface/Views/ContentViewContainer.js (283727 => 283728)


--- trunk/Source/WebInspectorUI/UserInterface/Views/ContentViewContainer.js	2021-10-07 18:35:01 UTC (rev 283727)
+++ trunk/Source/WebInspectorUI/UserInterface/Views/ContentViewContainer.js	2021-10-07 18:38:12 UTC (rev 283728)
@@ -421,6 +421,11 @@
             return;
         }
 
+        // Deselected extension tabs are still attached to the DOM via `this.element`,
+        // so this is the last chance to actually remove the subview and detach from the DOM.
+        if (contentView.constructor.shouldNotRemoveFromDOMWhenHidden() && contentView.isAttached)
+            this.removeSubview(contentView);
+
         console.assert(!contentView.isAttached);
 
         if (!contentView._parentContainer)
@@ -457,6 +462,10 @@
 
         if (!this.subviews.includes(entry.contentView))
             this.addSubview(entry.contentView);
+        else if (entry.contentView.constructor.shouldNotRemoveFromDOMWhenHidden()) {
+            entry.contentView.element.classList.remove("hidden-simulating-dom-detached");
+            entry.contentView._didMoveToParent(this);
+        }
 
         entry.prepareToShow();
     }
@@ -471,8 +480,13 @@
             return;
 
         entry.prepareToHide();
-        if (this.subviews.includes(entry.contentView))
-            this.removeSubview(entry.contentView);
+        if (this.subviews.includes(entry.contentView)) {
+            if (entry.contentView.constructor.shouldNotRemoveFromDOMWhenHidden()) {
+                entry.contentView.element.classList.add("hidden-simulating-dom-detached");
+                entry.contentView._didMoveToParent(null);
+            } else
+                this.removeSubview(entry.contentView);
+        }
     }
 
     _tombstoneContentViewContainersForContentView(contentView)

Modified: trunk/Source/WebInspectorUI/UserInterface/Views/TabBrowser.js (283727 => 283728)


--- trunk/Source/WebInspectorUI/UserInterface/Views/TabBrowser.js	2021-10-07 18:35:01 UTC (rev 283727)
+++ trunk/Source/WebInspectorUI/UserInterface/Views/TabBrowser.js	2021-10-07 18:38:12 UTC (rev 283728)
@@ -171,8 +171,10 @@
             this._tabBar.addTabBarItem(tabBarItem, options);
 
         console.assert(this._recentTabContentViews.length === this._tabBar.tabCount);
-        console.assert(!this.selectedTabContentView || this.selectedTabContentView === this._recentTabContentViews[0]);
 
+        let shouldSaveTab = this.selectedTabContentView?.constructor.shouldSaveTab() || this.selectedTabContentView?.constructor.shouldPinTab();
+        console.assert(!this.selectedTabContentView || this.selectedTabContentView === this._recentTabContentViews[0] || !shouldSaveTab);
+
         return true;
     }
 

Modified: trunk/Source/WebInspectorUI/UserInterface/Views/WebInspectorExtensionTabContentView.js (283727 => 283728)


--- trunk/Source/WebInspectorUI/UserInterface/Views/WebInspectorExtensionTabContentView.js	2021-10-07 18:35:01 UTC (rev 283727)
+++ trunk/Source/WebInspectorUI/UserInterface/Views/WebInspectorExtensionTabContentView.js	2021-10-07 18:38:12 UTC (rev 283728)
@@ -40,9 +40,6 @@
         this._tabInfo = tabInfo;
         this._sourceURL = sourceURL;
 
-        // FIXME: the <iframe>'s document is implicitly reloaded when this
-        // content view's element is detached and later re-attached to the DOM.
-        // This is a bug and will be addressed in <https://webkit.org/b/230758>.
         this._iframeElement = this.element.appendChild(document.createElement("iframe"));
         this._iframeElement.addEventListener("load", this._extensionFrameDidLoad.bind(this));
         this._iframeElement.src = ""
@@ -87,6 +84,11 @@
 
     static shouldSaveTab() { return false; }
 
+    static shouldNotRemoveFromDOMWhenHidden() {
+        // This is necessary to avoid the <iframe> content from being reloaded when the extension tab is hidden.
+        return true;
+    }
+
     // Private
 
     _extensionFrameDidLoad()

Modified: trunk/Tools/ChangeLog (283727 => 283728)


--- trunk/Tools/ChangeLog	2021-10-07 18:35:01 UTC (rev 283727)
+++ trunk/Tools/ChangeLog	2021-10-07 18:38:12 UTC (rev 283728)
@@ -1,3 +1,17 @@
+2021-10-07  BJ Burg  <bb...@apple.com>
+
+        Web Inspector: WebInspectorExtensionTabContentView should not reload its iframe when detached
+        https://bugs.webkit.org/show_bug.cgi?id=230758
+        <rdar://74714861>
+
+        Reviewed by Timothy Hatcher.
+
+        Add a new test to verify that re-selecting an extension tab does not cause it to reload.
+
+        * TestWebKitAPI/Tests/WebKitCocoa/InspectorExtension-basic-tab.html:
+        * TestWebKitAPI/Tests/WebKitCocoa/WKInspectorExtension.mm:
+        (TEST):
+
 2021-10-07  Youenn Fablet  <you...@apple.com>
 
         REGRESSION (r283613): [ macOS ] TestWebKitAPI.ResourceLoadDelegate.LoadInfo is failing

Modified: trunk/Tools/TestWebKitAPI/Tests/WebKitCocoa/InspectorExtension-basic-tab.html (283727 => 283728)


--- trunk/Tools/TestWebKitAPI/Tests/WebKitCocoa/InspectorExtension-basic-tab.html	2021-10-07 18:35:01 UTC (rev 283727)
+++ trunk/Tools/TestWebKitAPI/Tests/WebKitCocoa/InspectorExtension-basic-tab.html	2021-10-07 18:38:12 UTC (rev 283728)
@@ -1,10 +1,24 @@
 <html>
 <head>
 <script>
+    // Used by WKInspectorExtension.CanEvaluateScriptInExtensionTab
     window._secretValue = {answer:42};
+
+    // 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;
+    }
 </script>
 <body>
 <h1>This is a test extension.</h1>
 <p>In a normal extension, this area would show the extension's user interface.</p>
+<p>The unique value for this iframe's execution context is:
+    <span id="uniqueValueField">TBD</span>
+</p>
 </body>
 </html>

Modified: trunk/Tools/TestWebKitAPI/Tests/WebKitCocoa/WKInspectorExtension.mm (283727 => 283728)


--- trunk/Tools/TestWebKitAPI/Tests/WebKitCocoa/WKInspectorExtension.mm	2021-10-07 18:35:01 UTC (rev 283727)
+++ trunk/Tools/TestWebKitAPI/Tests/WebKitCocoa/WKInspectorExtension.mm	2021-10-07 18:38:12 UTC (rev 283728)
@@ -47,6 +47,7 @@
 static RetainPtr<TestInspectorURLSchemeHandler> sharedURLSchemeHandler;
 static RetainPtr<_WKInspectorExtension> sharedInspectorExtension;
 static RetainPtr<NSString> sharedExtensionTabIdentifier;
+static RetainPtr<NSObject> evaluationResult;
 
 static void resetGlobalState()
 {
@@ -188,4 +189,121 @@
     TestWebKitAPI::Util::run(&pendingCallbackWasCalled);
 }
 
+TEST(WKInspectorExtension, ExtensionTabIsPersistent)
+{
+    resetGlobalState();
+
+    auto webViewConfiguration = adoptNS([WKWebViewConfiguration new]);
+    webViewConfiguration.get().preferences._developerExtrasEnabled = YES;
+    auto webView = adoptNS([[WKWebView alloc] initWithFrame:CGRectMake(0, 0, 800, 600) configuration:webViewConfiguration.get()]);
+    auto uiDelegate = adoptNS([UIDelegateForTestingInspectorExtension new]);
+
+    [webView setUIDelegate:uiDelegate.get()];
+    [webView loadHTMLString:@"<head><title>Test page to be inspected</title></head><body><p>Filler content</p></body>" baseURL:[NSURL URLWithString:@"http://example.com/"]];
+
+    [[webView _inspector] show];
+    TestWebKitAPI::Util::run(&didAttachLocalInspectorCalled);
+
+    auto extensionID = [NSUUID UUID].UUIDString;
+    auto extensionDisplayName = @"SecondExtension";
+
+    // Register the test extension.
+    pendingCallbackWasCalled = false;
+    [[webView _inspector] registerExtensionWithID:extensionID displayName:extensionDisplayName completionHandler:^(NSError * _Nullable error, _WKInspectorExtension * _Nullable extension) {
+        EXPECT_NULL(error);
+        EXPECT_NOT_NULL(extension);
+        sharedInspectorExtension = extension;
+
+        pendingCallbackWasCalled = true;
+    }];
+    TestWebKitAPI::Util::run(&pendingCallbackWasCalled);
+
+    auto extensionDelegate = adoptNS([InspectorExtensionDelegateForTestingInspectorExtension new]);
+    [sharedInspectorExtension setDelegate:extensionDelegate.get()];
+
+    // Create and show an extension tab.
+    auto iconURL = [NSURL URLWithString:@"test-resource://SecondExtension/InspectorExtension-TabIcon-30x30.png"];
+    auto sourceURL = [NSURL URLWithString:@"test-resource://SecondExtension/InspectorExtension-basic-tab.html"];
+
+    pendingCallbackWasCalled = false;
+    [sharedInspectorExtension createTabWithName:@"SecondExtension-Tab" tabIconURL:iconURL sourceURL:sourceURL completionHandler:^(NSError * _Nullable error, NSString * _Nullable extensionTabIdentifier) {
+        EXPECT_NULL(error);
+        EXPECT_NOT_NULL(extensionTabIdentifier);
+        sharedExtensionTabIdentifier = extensionTabIdentifier;
+
+        pendingCallbackWasCalled = true;
+    }];
+    TestWebKitAPI::Util::run(&pendingCallbackWasCalled);
+
+    pendingCallbackWasCalled = false;
+    didShowExtensionTabWasCalled = false;
+    [[webView _inspector] showExtensionTabWithIdentifier:sharedExtensionTabIdentifier.get() completionHandler:^(NSError * _Nullable error) {
+        EXPECT_NULL(error);
+
+        pendingCallbackWasCalled = true;
+    }];
+    TestWebKitAPI::Util::run(&pendingCallbackWasCalled);
+    TestWebKitAPI::Util::run(&didShowExtensionTabWasCalled);
+
+    auto scriptSource = @"!!window.getUniqueValue && window.getUniqueValue()";
+
+    // Read back a value that is unique to the <iframe>'s script context.
+    pendingCallbackWasCalled = false;
+    [sharedInspectorExtension _evaluateScript:scriptSource inExtensionTabWithIdentifier:sharedExtensionTabIdentifier.get() completionHandler:^(NSError * _Nullable error, NSDictionary * _Nullable result) {
+        EXPECT_NULL(error);
+        EXPECT_NOT_NULL(result);
+        evaluationResult = result;
+
+        pendingCallbackWasCalled = true;
+    }];
+    TestWebKitAPI::Util::run(&pendingCallbackWasCalled);
+
+    // Cause the extension tab to be hidden. Its <iframe> should not detach from the DOM.
+    [[webView _inspector] showConsole];
+    TestWebKitAPI::Util::run(&didHideExtensionTabWasCalled);
+
+    // Check the unique value again, while the <iframe> is being hidden. If the <iframe> is
+    // detached from the DOM, then this evaluation will fail or hang.
+    pendingCallbackWasCalled = false;
+    [sharedInspectorExtension _evaluateScript:scriptSource inExtensionTabWithIdentifier:sharedExtensionTabIdentifier.get() completionHandler:^(NSError * _Nullable error, NSDictionary * _Nullable result) {
+        EXPECT_NULL(error);
+        EXPECT_NOT_NULL(result);
+        EXPECT_NS_EQUAL(result, evaluationResult.get());
+
+        pendingCallbackWasCalled = true;
+    }];
+    TestWebKitAPI::Util::run(&pendingCallbackWasCalled);
+
+    // Reselect the extension tab.
+    pendingCallbackWasCalled = false;
+    didShowExtensionTabWasCalled = false;
+    [[webView _inspector] showExtensionTabWithIdentifier:sharedExtensionTabIdentifier.get() completionHandler:^(NSError * _Nullable error) {
+        EXPECT_NULL(error);
+
+        pendingCallbackWasCalled = true;
+    }];
+    TestWebKitAPI::Util::run(&pendingCallbackWasCalled);
+    TestWebKitAPI::Util::run(&didShowExtensionTabWasCalled);
+
+    // Check the unique value again after reselecting the extension tab.
+    pendingCallbackWasCalled = false;
+    [sharedInspectorExtension _evaluateScript:scriptSource inExtensionTabWithIdentifier:sharedExtensionTabIdentifier.get() completionHandler:^(NSError * _Nullable error, NSDictionary * _Nullable result) {
+        EXPECT_NULL(error);
+        EXPECT_NOT_NULL(result);
+        EXPECT_NS_EQUAL(result, evaluationResult.get());
+
+        pendingCallbackWasCalled = true;
+    }];
+    TestWebKitAPI::Util::run(&pendingCallbackWasCalled);
+
+    // Unregister the test extension.
+    pendingCallbackWasCalled = false;
+    [[webView _inspector] unregisterExtension:sharedInspectorExtension.get() completionHandler:^(NSError * _Nullable error) {
+        EXPECT_NULL(error);
+
+        pendingCallbackWasCalled = true;
+    }];
+    TestWebKitAPI::Util::run(&pendingCallbackWasCalled);
+}
+
 #endif // ENABLE(INSPECTOR_EXTENSIONS)
_______________________________________________
webkit-changes mailing list
webkit-changes@lists.webkit.org
https://lists.webkit.org/mailman/listinfo/webkit-changes

Reply via email to