Main.js and Test.js are doing a lot of the same things. We should have a singleton AppController rather than hanging a ton of singleton state off of the WI object. This is a large project that will be completed incrementally.
Created attachment 320962 [details] Patch
Comment on attachment 320962 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=320962&action=review Nice refactor, Main.js has become increasingly unwieldy. r=me with comments. > Source/WebInspectorUI/UserInterface/Controllers/AppController.js:48 > + for (var domain of domains) { Use `let` throughout. > Source/WebInspectorUI/UserInterface/Controllers/AppControllerBase.js:38 > + get hasExtraDomains() { throw new Error("This method must be overridden by a subclass."); } Since we use this pattern pretty heavily I'd go so far as to add a WI.NotImplementedError that has this text baked in. > Source/WebInspectorUI/UserInterface/Main.html:829 > + WI.sharedApp = new WI.AppController; I'm not sure about this name. Why not simply `WI.appController`?
(In reply to Matt Baker from comment #2) > Comment on attachment 320962 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=320962&action=review > > Nice refactor, Main.js has become increasingly unwieldy. r=me with comments. > > > Source/WebInspectorUI/UserInterface/Controllers/AppController.js:48 > > + for (var domain of domains) { > > Use `let` throughout. Yup, I caught this later in the day. > > Source/WebInspectorUI/UserInterface/Controllers/AppControllerBase.js:38 > > + get hasExtraDomains() { throw new Error("This method must be overridden by a subclass."); } > > Since we use this pattern pretty heavily I'd go so far as to add a > WI.NotImplementedError that has this text baked in. OK > > > Source/WebInspectorUI/UserInterface/Main.html:829 > > + WI.sharedApp = new WI.AppController; > > I'm not sure about this name. Why not simply `WI.appController`? It matches what is done in Cocoa, and it's shorter. It also makes it pretty obvious that this is a singleton.
Created attachment 321123 [details] For landing
Comment on attachment 321123 [details] For landing Clearing flags on attachment: 321123 Committed r222181: <http://trac.webkit.org/changeset/222181>
All reviewed patches have been landed. Closing bug.
<rdar://problem/34693386>