[webkit-reviews] review granted: [Bug 177024] Web Inspector: introduce an AppController class and shared instance of it : [Attachment 320962] Patch
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Fri Sep 15 15:13:58 PDT 2017
Matt Baker <mattbaker at apple.com> has granted Brian Burg <bburg at apple.com>'s
request for review:
Bug 177024: Web Inspector: introduce an AppController class and shared instance
of it
https://bugs.webkit.org/show_bug.cgi?id=177024
Attachment 320962: Patch
https://bugs.webkit.org/attachment.cgi?id=320962&action=review
--- Comment #2 from Matt Baker <mattbaker at apple.com> ---
Comment on attachment 320962
--> https://bugs.webkit.org/attachment.cgi?id=320962
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`?
More information about the webkit-reviews
mailing list