Summary: | Web Inspector: Turn HelpScreen to be View. | ||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | eustas.bug | ||||||||||||||||
Component: | Web Inspector (Deprecated) | Assignee: | eustas.bug | ||||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||||
Severity: | Normal | CC: | apavlov, bweinstein, caseq, eustas.bug, joepeck, keishi, loislo, pfeldman, pmuellr, rik, timothy, yurys | ||||||||||||||||
Priority: | P2 | ||||||||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||||||||
Hardware: | All | ||||||||||||||||||
OS: | All | ||||||||||||||||||
Attachments: |
|
Description
eustas.bug
2012-05-05 07:51:21 PDT
Created attachment 140400 [details]
Patch
Comment on attachment 140400 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=140400&action=review Overall looks good. Note that since you are going to make it tabbed pane, you will not need the custom show / hide logic - your help screen will become singleton. So most of the code here will vanish. > Source/WebCore/inspector/front-end/HelpScreen.js:68 > show: function(onHide) Do we actually use onHide? > Source/WebCore/inspector/front-end/HelpScreen.js:-62 > - if (this._isShown) You are overriding View's show with different parameters. I would pick another name. > Source/WebCore/inspector/front-end/HelpScreen.js:70 > + var visibleHelpScreen = WebInspector.HelpScreen.visibleScreen_; _ is before the name, not after > Source/WebCore/inspector/front-end/HelpScreen.js:79 > + WebInspector.View.prototype.show.call(this, document.body); document.body -> WebInspector.inspectorView.element Comment on attachment 140400 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=140400&action=review >> Source/WebCore/inspector/front-end/HelpScreen.js:68 >> show: function(onHide) > > Do we actually use onHide? For sure. It is used, at least, to remove highlight from "Settings" icon. >> Source/WebCore/inspector/front-end/HelpScreen.js:-62 >> - if (this._isShown) > > You are overriding View's show with different parameters. I would pick another name. Done. >> Source/WebCore/inspector/front-end/HelpScreen.js:70 >> + var visibleHelpScreen = WebInspector.HelpScreen.visibleScreen_; > > _ is before the name, not after Wasn't me =) Fixed. >> Source/WebCore/inspector/front-end/HelpScreen.js:79 >> + WebInspector.View.prototype.show.call(this, document.body); > > document.body -> WebInspector.inspectorView.element Done. Created attachment 141151 [details]
Patch
Comment on attachment 141151 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=141151&action=review > Source/WebCore/inspector/front-end/HelpScreen.js:39 > + this.markAsRoot(); You are showing it as a child of inspector view, no need to mark as root. > Source/WebCore/inspector/front-end/HelpScreen.js:70 > + present: function(onHide) Naming of the function is not clear. Created attachment 141357 [details]
Patch
Comment on attachment 141357 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=141357&action=review > Source/WebCore/inspector/front-end/SettingsController.js:34 > +WebInspector.SettingsController = function() This can be a part of the settings screen for simplicity. > Source/WebCore/inspector/front-end/ShortcutsScreen.js:54 > + willShow: function() Why new callback? Doesn't wasShown work? > Source/WebCore/inspector/front-end/ShortcutsScreen.js:57 > + WebInspector.HelpScreen.prototype.willShow.call(this); Is there anything in this prototype? You should not override View's methods here, HelpScreen should provide appropriate apis for extensibility. Comment on attachment 141357 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=141357&action=review >> Source/WebCore/inspector/front-end/SettingsController.js:34 >> +WebInspector.SettingsController = function() > > This can be a part of the settings screen for simplicity. Moved. >> Source/WebCore/inspector/front-end/ShortcutsScreen.js:54 >> + willShow: function() > > Why new callback? Doesn't wasShown work? IMHO it is much better to manipulate DOM until it became visible, not after. >> Source/WebCore/inspector/front-end/ShortcutsScreen.js:57 >> + WebInspector.HelpScreen.prototype.willShow.call(this); > > Is there anything in this prototype? You should not override View's methods here, HelpScreen should provide appropriate apis for extensibility. willShow/wasShown/willHide are clearly hook methods created specially for purposes like that. It seems to be inappropriate to create a new layer of hooks. Created attachment 141395 [details]
Patch
Comment on attachment 141395 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=141395&action=review > Source/WebCore/ChangeLog:6 > + Motivation: for further UI changes, HelpSceen need to be View. s/need/needs. Also, you may want to mention the intended future changes (e.g. make it a tabbed view). > Source/WebCore/inspector/front-end/SettingsScreen.js:607 > + var button = new WebInspector.StatusBarButton(WebInspector.UIString("Settings"), "settings-status-bar-item"); > + button.addEventListener("click", this._buttonClicked.bind(this), false); > + > + /** @type {!WebInspector.StatusBarButton} */ > + this._statusBarButton = button; Can we make this simpler by not using the intermediate local variable? > Source/WebCore/inspector/front-end/SettingsScreen.js:610 > + /** @type {?WebInspector.SettingsScreen} */ > + this._settingsScreen; What's that for? > Source/WebCore/inspector/front-end/SettingsScreen.js:634 > + showSettingsScreen: function() does this have to be public? > Source/WebCore/inspector/front-end/SettingsScreen.js:643 > + hideSettingsScreen: function() ditto > Source/WebCore/inspector/front-end/ShortcutsScreen.js:56 > this._buildTable(this.contentElement); Does it really make a difference whether we render this before, not after this is shown? I.e., are there strong reasons we can't live with wasShown()? Comment on attachment 141395 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=141395&action=review >> Source/WebCore/ChangeLog:6 >> + Motivation: for further UI changes, HelpSceen need to be View. > > s/need/needs. Also, you may want to mention the intended future changes (e.g. make it a tabbed view). Fixed. >> Source/WebCore/inspector/front-end/SettingsScreen.js:607 >> + this._statusBarButton = button; > > Can we make this simpler by not using the intermediate local variable? We can. But doesn't local variable here make code more readable? Fixed. >> Source/WebCore/inspector/front-end/SettingsScreen.js:610 >> + this._settingsScreen; > > What's that for? That is member type declaration. >> Source/WebCore/inspector/front-end/SettingsScreen.js:634 >> + showSettingsScreen: function() > > does this have to be public? This method is planned to be used outside of controller (with optional parameter "tabToShowId"). Fixed for now. >> Source/WebCore/inspector/front-end/SettingsScreen.js:643 >> + hideSettingsScreen: function() > > ditto Fixed. >> Source/WebCore/inspector/front-end/ShortcutsScreen.js:56 >> this._buildTable(this.contentElement); > > Does it really make a difference whether we render this before, not after this is shown? I.e., are there strong reasons we can't live with wasShown()? There are no strong reasons. Reverted. Created attachment 141417 [details]
Patch
Comment on attachment 141417 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=141417&action=review LGTM with one nit. Pavel, could you please have a look? > Source/WebCore/inspector/front-end/SettingsScreen.js:603 > + /** @type {!WebInspector.StatusBarButton} */ I think annotation is redundant there, type should be inferred by the JS compiler from the initialization. Comment on attachment 141417 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=141417&action=review > Source/WebCore/ChangeLog:12 > + Reviewed by NOBODY (OOPS!). This line should go before the description. Created attachment 141567 [details]
Patch
Rebased, removed redundant type annotation, fixed ChangeLog. Created attachment 141572 [details]
Patch
Darkened background color, fixed scrollbar spacing. Committed r116854: <http://trac.webkit.org/changeset/116854> |