Summary: | Web Inspector: show something useful when the inspector frontend fails to load | ||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Blaze Burg <bburg> | ||||||||||
Component: | Web Inspector | Assignee: | Blaze Burg <bburg> | ||||||||||
Status: | RESOLVED FIXED | ||||||||||||
Severity: | Normal | CC: | bburg, commit-queue, graouts, joepeck, mattbaker, nvasilyev, timothy, webkit-bug-importer | ||||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||||
Version: | WebKit Nightly Build | ||||||||||||
Hardware: | All | ||||||||||||
OS: | All | ||||||||||||
Attachments: |
|
Description
Blaze Burg
2015-11-28 18:40:55 PST
Created attachment 266217 [details]
[Screenshot] Undocked Inspector
Created attachment 266218 [details]
[Screenshot] Docked inspector
Created attachment 266220 [details]
Proposed Fix
Attachment 266220 [details] did not pass style-queue:
ERROR: Source/WebInspectorUI/UserInterface/Debug/CatchEarlyErrors.js:63: Line contains single-quote character. [js/syntax] [5]
ERROR: Source/WebInspectorUI/UserInterface/Debug/CatchEarlyErrors.js:64: Line contains single-quote character. [js/syntax] [5]
Total errors found: 2 in 4 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 266221 [details]
Proposed Fix
New patch fixes some issues that come up if a ReferenceError happens after some of the inspector has been loaded, such as in WebInspector.contentLoaded. Unconditionally disarm all event handlers to prevent other weirdness.
Attachment 266221 [details] did not pass style-queue:
ERROR: Source/WebInspectorUI/UserInterface/Debug/CatchEarlyErrors.js:75: Line contains single-quote character. [js/syntax] [5]
ERROR: Source/WebInspectorUI/UserInterface/Debug/CatchEarlyErrors.js:76: Line contains single-quote character. [js/syntax] [5]
Total errors found: 2 in 4 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 266221 [details] Proposed Fix View in context: https://bugs.webkit.org/attachment.cgi?id=266221&action=review Nice! Text could be a tad smaller, especially in the docked state. I'd change margins and padding too. I think it would be easier if I send a follow-up patch. > Source/WebInspectorUI/UserInterface/Debug/CatchEarlyErrors.css:61 > +} Nit: no empty CSS rules in the patch ready for landing. > Source/WebInspectorUI/UserInterface/Debug/CatchEarlyErrors.js:32 > + document.write("<body></body></html>"); Do we have to use document.write? Can we do: if (!document.body) document.body = document.createElement("body"); ? > Source/WebInspectorUI/UserInterface/Debug/CatchEarlyErrors.js:44 > + document.addEventListener(name, stopEventPropagation, true); When would we need to do this? > Source/WebInspectorUI/UserInterface/Debug/CatchEarlyErrors.js:69 > + Web Inspector encountered an error while loading. No need for dot at the end of title. I'm not a reviewer, so I'll keep it r?. Comment on attachment 266221 [details] Proposed Fix View in context: https://bugs.webkit.org/attachment.cgi?id=266221&action=review >> Source/WebInspectorUI/UserInterface/Debug/CatchEarlyErrors.js:32 >> + document.write("<body></body></html>"); > > Do we have to use document.write? > Can we do: > > if (!document.body) > document.body = document.createElement("body"); > > ? I see no difference. >> Source/WebInspectorUI/UserInterface/Debug/CatchEarlyErrors.js:44 >> + document.addEventListener(name, stopEventPropagation, true); > > When would we need to do this? See the comment that was posted with the second patch. I added a shorter comment above. >> Source/WebInspectorUI/UserInterface/Debug/CatchEarlyErrors.js:69 >> + Web Inspector encountered an error while loading. > > No need for dot at the end of title. > > I'm not a reviewer, so I'll keep it r?. Everything else on the page is a full sentence. (In reply to comment #8) > Comment on attachment 266221 [details] > Proposed Fix > > View in context: > https://bugs.webkit.org/attachment.cgi?id=266221&action=review > > Nice! > > Text could be a tad smaller, especially in the docked state. I'd change > margins and padding too. I think it would be easier if I send a follow-up > patch. I prefer bigger text, since the error page doesn't zoom. That said, it's a step bigger than what Reader uses by default. To match what Reader uses, I can reduce the font size a bit (but keep the Sans Serif). Comment on attachment 266221 [details] Proposed Fix View in context: https://bugs.webkit.org/attachment.cgi?id=266221&action=review >>> Source/WebInspectorUI/UserInterface/Debug/CatchEarlyErrors.js:32 >>> + document.write("<body></body></html>"); >> >> Do we have to use document.write? >> Can we do: >> >> if (!document.body) >> document.body = document.createElement("body"); >> >> ? > > I see no difference. We try not to use things like innerHTML, document.write unless it is needed. Committed r192837: <http://trac.webkit.org/changeset/192837> I think this caused a broken inspector on minified builds. The Debug/Foo.js being early on may be causing the minified/combined Main.html to output Main.js before its dependencies: <script src="Main.js"></script> <script src="CodeMirror.js"></script> <script src="Esprima.js"></script> <script src="ESLint.js"></script> Comment on attachment 266221 [details] Proposed Fix View in context: https://bugs.webkit.org/attachment.cgi?id=266221&action=review > Source/WebInspectorUI/UserInterface/Debug/CatchEarlyErrors.js:68 > + <img src="Images/Errors.svg" /> No need for "/>" on the known self-closing tags like <img>. |