Summary: | Web Inspector: Add a way to test the Manager and model classes | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Alexandru Chiculita <achicu> | ||||||||
Component: | Web Inspector | Assignee: | Alexandru Chiculita <achicu> | ||||||||
Status: | RESOLVED FIXED | ||||||||||
Severity: | Normal | CC: | commit-queue, graouts, joepeck, timothy, webkit-bug-importer | ||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||
Version: | 528+ (Nightly build) | ||||||||||
Hardware: | All | ||||||||||
OS: | All | ||||||||||
Attachments: |
|
Description
Alexandru Chiculita
2013-10-23 14:52:23 PDT
Created attachment 215002 [details]
Patch V1
Comment on attachment 215002 [details] Patch V1 View in context: https://bugs.webkit.org/attachment.cgi?id=215002&action=review Clever, I like it. This looks good to me, but this will just need to fix Main.html missing new includes. > LayoutTests/http/tests/inspector-protocol/resources/InspectorTest.js:110 > + if (xhr.status !== 0) > + throw new Error("Invalid script URL: " + scriptName); A status of 200 may need to be allowed if there is a LayoutTest/http/inspector-protocol test. Though I'm not sure that will ever happen. > LayoutTests/http/tests/inspector-protocol/resources/InspectorTest.js:122 > + "InspectorFrontendAPI", When this is included it will overwrite the "InspectorFrontendAPI" implemented by LayoutTests/http/tests/inspector-protocol/resources/InspectorTest.js. We will need to be careful, if a test wants to grab events we'll need to re-incorporate a way to get back the protocol-test InspectorTest.eventHandler["Debugger.scriptParsed"] syntax, or something like it. But if we are indeed just testing the model, then we probably will never care about this. > Source/WebInspectorUI/UserInterface/MessageDispatcher.js:1 > +/* These new files will need to be included by Main.html, in the right order, so that the frontend itself still works! r- for this. > Source/WebInspectorUI/UserInterface/URLUtilities.js:1 > +/* I like that you've put all of this code in one file. I've had that itch for a while! Created attachment 215009 [details]
Patch V2
Nice catch with the main.html file. I've run the test runner so many times, I forget to actually run the inspector itself.
Comment on attachment 215009 [details] Patch V2 View in context: https://bugs.webkit.org/attachment.cgi?id=215009&action=review > LayoutTests/http/tests/inspector-protocol/resources/InspectorTest.js:157 > + InspectorBackend.registerPageDispatcher(new WebInspector.PageObserver); > + InspectorBackend.registerDOMDispatcher(new WebInspector.DOMObserver); > + > + WebInspector.frameResourceManager = new WebInspector.FrameResourceManager; > + WebInspector.domTreeManager = new WebInspector.DOMTreeManager; > + > + InspectorFrontendHost.loaded(); This will need to mimic the code in Main.js. Either we should share it or at the very least add a comment to keep it in sync. > Source/WebInspectorUI/UserInterface/URLUtilities.js:177 > +WebInspector.displayNameForURL = function(url, urlComponents) It is tempting to remove the namespace here too to match the other functions now. > Source/WebInspectorUI/UserInterface/URLUtilities.js:192 > +WebInspector.displayNameForHost = function(host) Ditto. Created attachment 215074 [details]
Patch V3
I've added a comment, but I didn't remove the namespace. I think we should namespace the other globals, otherwise it will just pollute the global window. I'm not changing that part of this patch as I might introduce other unrelated issues if I don't find all the usages.
Comment on attachment 215074 [details] Patch V3 Clearing flags on attachment: 215074 Committed r157938: <http://trac.webkit.org/changeset/157938> All reviewed patches have been landed. Closing bug. |