Bug 191773 – Web Inspector: "Reload Web Inspector" button no longer partially works

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

[Joseph Pecoraro]

"Reload Web Inspector" button no longer partially works

The reload button only partially worked before (breakpoints would end up in an unresolved state). But now it totally doesn't work in a WI.MultiplexingBackendTarget world, since the TargetAgent doesn't re-send the targets like it does on first initialization. One approach would be to add and call disable on all agents and re-enable them, but it is still likely to be imperfect. It would be better to have a Close / Reopen approach.

This is used by Experimental Settings and there is a "Reload" debug navigation item.

Can we replace the button with a Reopen button or just ask developers to close and reopen?

[Radar WebKit Bug Importer]

<rdar://problem/46139932>

[Joseph Pecoraro]

Created attachment 355173 [details]
[PATCH] Proposed Fix

Introduce InspectorFrontendHost.reopen which closes and reopens the Web Inspector.

I tests:

  - WebKit
    - local
    - remote
  - WebKitLegacy Mac

I didn't test WebKitLegacy Windows but its nearly identical code.

[Joseph Pecoraro]

Comment on attachment 355173 [details]
[PATCH] Proposed Fix

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

> Source/WebKitLegacy/mac/WebCoreSupport/WebInspectorClient.mm:-667
> -    if (_attachedToInspectedWebView)

This was leaving behind empty white windows if web inspector was detached because we weren't calling window close. So just unconditionally close.

[Devin Rousso]

Comment on attachment 355173 [details]
[PATCH] Proposed Fix

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

r=me

> Source/WebInspectorUI/ChangeLog:10
> +        * UserInterface/Debug/UncaughtExceptionReporter.js:

I think there's also a `location.reload()`  inside Bootstrap.js:39 (reload Web Inspector keyboard shortcut).  Frankly, I've never used it (and didn't know it existed till now), so I'd also be fine if you removed it.

> Source/WebInspectorUI/ChangeLog:12
> +        (sheetElement.innerHTML.div):
> +        (let.dismissOptionHTML.loadCompleted.string_appeared_here.dt.A.frivolous.exception.will.not.stop.me.dt.dd.a):

lolwut

> Source/WebKit/UIProcess/RemoteWebInspectorProxy.cpp:99
> +    load(m_debuggableType, m_backendCommandsURL);

Should we add assertions that `m_debuggableType` and `m_backendCommandsURL` are both valid?  `load` doesn't so maybe I'm just being paranoid...

> Source/WebKitLegacy/win/WebCoreSupport/WebInspectorClient.cpp:276
> +    if (Page* inspectedPage = m_inspectedWebView->page())

Not sure about our style on this, but IIRC this would be a great `auto*` case

[Joseph Pecoraro]

Comment on attachment 355173 [details]
[PATCH] Proposed Fix

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

>> Source/WebKit/UIProcess/RemoteWebInspectorProxy.cpp:99
>> +    load(m_debuggableType, m_backendCommandsURL);
> 
> Should we add assertions that `m_debuggableType` and `m_backendCommandsURL` are both valid?  `load` doesn't so maybe I'm just being paranoid...

I'll assert that they are not empty, that sounds like a good idea.

>> Source/WebKitLegacy/win/WebCoreSupport/WebInspectorClient.cpp:276
>> +    if (Page* inspectedPage = m_inspectedWebView->page())
> 
> Not sure about our style on this, but IIRC this would be a great `auto*` case

This matches style later in the file. I'll keep the file consistent for now.

[Joseph Pecoraro]

(In reply to Joseph Pecoraro from comment #5)
> Comment on attachment 355173 [details]
> [PATCH] Proposed Fix
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=355173&action=review
> 
> >> Source/WebKit/UIProcess/RemoteWebInspectorProxy.cpp:99
> >> +    load(m_debuggableType, m_backendCommandsURL);
> > 
> > Should we add assertions that `m_debuggableType` and `m_backendCommandsURL` are both valid?  `load` doesn't so maybe I'm just being paranoid...
> 
> I'll assert that they are not empty, that sounds like a good idea.

Err, at least m_debuggableType. backendCommandsURL can be empty.

[Joseph Pecoraro]

https://trac.webkit.org/r238378