Bug 191773 - Web Inspector: "Reload Web Inspector" button no longer partially works
Summary: Web Inspector: "Reload Web Inspector" button no longer partially works
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (show other bugs)
Version: WebKit Nightly Build
Hardware: All All
: P2 Normal
Assignee: Joseph Pecoraro
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2018-11-16 14:30 PST by Joseph Pecoraro
Modified: 2018-11-26 10:29 PST (History)
5 users (show)

See Also:


Attachments
[PATCH] Proposed Fix (21.33 KB, patch)
2018-11-16 19:48 PST, Joseph Pecoraro
drousso: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Joseph Pecoraro 2018-11-16 14:30:51 PST
"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?
Comment 1 Radar WebKit Bug Importer 2018-11-16 14:31:27 PST
<rdar://problem/46139932>
Comment 2 Joseph Pecoraro 2018-11-16 19:48:37 PST
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.
Comment 3 Joseph Pecoraro 2018-11-16 20:02:32 PST
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.
Comment 4 Devin Rousso 2018-11-19 00:18:14 PST
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
Comment 5 Joseph Pecoraro 2018-11-19 12:44:01 PST
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.
Comment 6 Joseph Pecoraro 2018-11-19 12:50:31 PST
(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.
Comment 7 Joseph Pecoraro 2018-11-19 12:52:04 PST
https://trac.webkit.org/r238378