Bug 198956 – Web Inspector: Debug: "Reset Web Inspector" should also clear the saved window size and attachment side

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Inspector

[Devin Rousso]

# STEPS TO REPRODUCE:
1. inspect any page
2. change the Web Inspector window size/docking to be different than default
3. enable the debug settings panel (⌥⇧⌘D)
4. click "Reset Web Inspector"
 => Web Inspector window has the same size/docking as before the reset

[Devin Rousso]

Created attachment 372400 [details]
Patch

Lots of plumbing -.-

[EWS Watchlist]

Attachment 372400 [details] did not pass style-queue:


ERROR: Source/WebKitLegacy/cf/WebCoreSupport/WebInspectorClientCF.cpp:94:  Use nullptr instead of NULL.  [readability/null] [5]
Total errors found: 1 in 46 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Devin Rousso]

Created attachment 372403 [details]
Patch

Looks like I missed a few platforms 😅

[EWS Watchlist]

Attachment 372403 [details] did not pass style-queue:


ERROR: Source/WebKitLegacy/cf/WebCoreSupport/WebInspectorClientCF.cpp:94:  Use nullptr instead of NULL.  [readability/null] [5]
Total errors found: 1 in 46 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Joseph Pecoraro]

Comment on attachment 372403 [details]
Patch

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

> Source/WebCore/inspector/InspectorFrontendHost.cpp:203
> +    if (m_client)
> +        m_client->resetWindowState();

Perhaps this should be `m_client->reset()` or `resetState()`? What if some non-window state creeps in, we would still want to reset it here.

> Source/WebKit/UIProcess/mac/RemoteWebInspectorProxyMac.mm:128
> +    [m_window removeFrameUsingName:@"WKRemoteWebInspectorWindowFrame"];

The magic string should be shared in this file (perhaps a file static const) to ensure no chance of changing one without the other.

> Source/WebKitLegacy/mac/WebCoreSupport/WebInspectorClient.h:86
> +    void deleteInspectorStartsAttached();

Why two calls? I seem unlikely that one will be called without the other.

> Source/WebKitLegacy/mac/WebCoreSupport/WebInspectorClient.mm:267
> +    [m_frontendWindowController.get() setWindowFrameAutosaveName:@"Web Inspector 2"];

Ditto: magic string.

[Devin Rousso]

Comment on attachment 372403 [details]
Patch

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

>> Source/WebCore/inspector/InspectorFrontendHost.cpp:203
>> +        m_client->resetWindowState();
> 
> Perhaps this should be `m_client->reset()` or `resetState()`? What if some non-window state creeps in, we would still want to reset it here.

I'd hope that all non-window state is held in the frontend, but good point in case not :)

>> Source/WebKitLegacy/mac/WebCoreSupport/WebInspectorClient.h:86
>> +    void deleteInspectorStartsAttached();
> 
> Why two calls? I seem unlikely that one will be called without the other.

I wanted to match the "flexibility" of having each be separate.  Not to mention, there was already a getter/setter specifically for each, so I continued that "pattern".

[Devin Rousso]

Created attachment 372412 [details]
Patch

[EWS Watchlist]

Attachment 372412 [details] did not pass style-queue:


ERROR: Source/WebKitLegacy/cf/WebCoreSupport/WebInspectorClientCF.cpp:94:  Use nullptr instead of NULL.  [readability/null] [5]
Total errors found: 1 in 46 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Devin Rousso]

Comment on attachment 372412 [details]
Patch

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

> Source/WebInspectorUI/UserInterface/Base/Main.js:3229
> +WI.reset = async function()

This was moved into Main.js primarily to make an easier way of resetting non-engineering builds (assuming you have "access" to inspector2).

[Matt Baker]

Comment on attachment 372412 [details]
Patch

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

r=me, looks like you addressed Joe's comments.

> Source/WebKitLegacy/cf/WebCoreSupport/WebInspectorClientCF.cpp:94
> +    CFPreferencesSetAppValue(createKeyForPreferences(key).get(), NULL, kCFPreferencesCurrentApplication);

You can pass nullptr for the CFPropertyListRef argument. Should fix the [style] error.

[Devin Rousso]

Created attachment 373309 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 373309 [details]
Patch

Clearing flags on attachment: 373309

Committed r247043: <https://trac.webkit.org/changeset/247043>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/52508508>