Summary: | Web Inspector: "Reload Web Inspector" button no longer partially works | ||||||
---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Joseph Pecoraro <joepeck> | ||||
Component: | Web Inspector | Assignee: | Joseph Pecoraro <joepeck> | ||||
Status: | RESOLVED FIXED | ||||||
Severity: | Normal | CC: | hi, inspector-bugzilla-changes, joepeck, timothy, webkit-bug-importer | ||||
Priority: | P2 | Keywords: | InRadar | ||||
Version: | WebKit Nightly Build | ||||||
Hardware: | All | ||||||
OS: | All | ||||||
See Also: | https://bugs.webkit.org/show_bug.cgi?id=191971 | ||||||
Attachments: |
|
Description
Joseph Pecoraro
2018-11-16 14:30:51 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 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 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 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. (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. |