Summary: | Web Inspector: Create a container class for SidebarPane instances | ||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Vladislav Kaznacheev <kaznacheev> | ||||||||||||||
Component: | Web Inspector (Deprecated) | Assignee: | Vladislav Kaznacheev <kaznacheev> | ||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||
Severity: | Normal | CC: | apavlov, buildbot, keishi, loislo, pfeldman, pmuellr, rniwa, vsevik, web-inspector-bugs, webkit.review.bot, yurys | ||||||||||||||
Priority: | P2 | ||||||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||||||
Hardware: | All | ||||||||||||||||
OS: | All | ||||||||||||||||
Attachments: |
|
Description
Vladislav Kaznacheev
2013-01-29 05:27:59 PST
Created attachment 185770 [details]
Patch
Created attachment 185772 [details]
Patch
Comment on attachment 185772 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=185772&action=review Overall looks good. We'll need to go through the sidebars to check whether it regressed anything UI-wise. > Source/WebCore/ChangeLog:9 > + SidebarPane's are inserted into DOM lazily and can belong to more than one container. SidebarPanes are... > Source/WebCore/inspector/front-end/ElementsPanel.js:89 > + this.sidebarPanes.styles.setShowCallback(this.updateStyles.bind(this)); Might be a good time to introduce WebInspector.View.Events.WasShown event. Could be a part of separate change though. > Source/WebCore/inspector/front-end/SidebarPane.js:38 > +// this.titleElement = document.createElement("div"); Remove this line. > Source/WebCore/inspector/front-end/SidebarPane.js:39 > + this.titleElement = document.createDocumentFragment(); this._auxTitleFragment. Btw, what is the point of using fragment here? > Source/WebCore/inspector/front-end/SidebarPane.js:43 > + this._expanded = false; It is weird that we still have this._expanded. > Source/WebCore/inspector/front-end/SidebarPane.js:91 > + setExpandCallback: function(callback) Please annotate these > Source/WebCore/inspector/front-end/SidebarPane.js:132 > + addPane: function(pane) Here and below: please annotate. > Source/WebCore/inspector/front-end/inspector.css:1788 > +.pane-title + .pane-title, .pane:not(.visible) + .pane-title, .pane-title:first-of-type { We'll need to test this thoroughly since there is a lot of minor features associated with the titles. Comment on attachment 185772 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=185772&action=review >> Source/WebCore/ChangeLog:9 >> + SidebarPane's are inserted into DOM lazily and can belong to more than one container. > > SidebarPanes are... Done >> Source/WebCore/inspector/front-end/ElementsPanel.js:89 >> + this.sidebarPanes.styles.setShowCallback(this.updateStyles.bind(this)); > > Might be a good time to introduce WebInspector.View.Events.WasShown event. Could be a part of separate change though. Will file a bug for that. >> Source/WebCore/inspector/front-end/SidebarPane.js:39 >> + this.titleElement = document.createDocumentFragment(); > > this._auxTitleFragment. Btw, what is the point of using fragment here? Removed the comment. The fragment is used here because the 5 of the SidebarPane inheritors actively use this.titleElement.appendChild and I did not want to change their code. >> Source/WebCore/inspector/front-end/SidebarPane.js:43 >> + this._expanded = false; > > It is weird that we still have this._expanded. Got rid of _expanded. The expand state is now stored only in SidebarPaneStack >> Source/WebCore/inspector/front-end/SidebarPane.js:91 >> + setExpandCallback: function(callback) > > Please annotate these Done >> Source/WebCore/inspector/front-end/SidebarPane.js:132 >> + addPane: function(pane) > > Here and below: please annotate. Done >> Source/WebCore/inspector/front-end/inspector.css:1788 >> +.pane-title + .pane-title, .pane:not(.visible) + .pane-title, .pane-title:first-of-type { > > We'll need to test this thoroughly since there is a lot of minor features associated with the titles. Oh, I have and I will again. Created attachment 186021 [details]
Patch
Created attachment 186041 [details]
Patch
Created attachment 186050 [details]
Patch
Comment on attachment 186050 [details] Patch Attachment 186050 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/16297939 New failing tests: inspector/extensions/extensions-events.html Comment on attachment 186050 [details] Patch Attachment 186050 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/16293938 New failing tests: inspector/extensions/extensions-events.html Comment on attachment 186050 [details]
Patch
Please resolve tests issues prior to landing.
Created attachment 186388 [details]
Patch
Comment on attachment 186388 [details] Patch Clearing flags on attachment: 186388 Committed r141777: <http://trac.webkit.org/changeset/141777> All reviewed patches have been landed. Closing bug. |