Summary: | Web Inspector: Convert remaining ContentViews to View base class | ||||||||
---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Matt Baker <mattbaker> | ||||||
Component: | Web Inspector | Assignee: | Matt Baker <mattbaker> | ||||||
Status: | RESOLVED FIXED | ||||||||
Severity: | Normal | CC: | bburg, commit-queue, graouts, joepeck, mattbaker, nvasilyev, timothy, webkit-bug-importer | ||||||
Priority: | P2 | Keywords: | InRadar | ||||||
Version: | WebKit Nightly Build | ||||||||
Hardware: | All | ||||||||
OS: | All | ||||||||
Bug Depends on: | 149186 | ||||||||
Bug Blocks: | |||||||||
Attachments: |
|
Description
Matt Baker
2015-10-30 13:05:17 PDT
Created attachment 264825 [details]
[Patch] Proposed Fix
Comment on attachment 264825 [details] [Patch] Proposed Fix View in context: https://bugs.webkit.org/attachment.cgi?id=264825&action=review r- because you missed a few addSubview cases. It would have been nice to review the database / apache changes separately, since the rest was mechanical. > Source/WebInspectorUI/UserInterface/Views/DatabaseContentView.js:95 > + this.prompt.updateLayout(); ConsolePrompt should be a subview. > Source/WebInspectorUI/UserInterface/Views/DatabaseContentView.js:98 > + for (let resultElement of results) { What the heck is this old crap... :) we should be adding DataGrid as a subview and remove this whole function. (In reply to comment #3) > Comment on attachment 264825 [details] > [Patch] Proposed Fix > > View in context: > https://bugs.webkit.org/attachment.cgi?id=264825&action=review > > r- because you missed a few addSubview cases. > > It would have been nice to review the database / apache changes separately, > since the rest was mechanical. Noted. I suspected as much during refactoring, and will remove from the follow up. > > Source/WebInspectorUI/UserInterface/Views/DatabaseContentView.js:95 > > + this.prompt.updateLayout(); > > ConsolePrompt should be a subview. The only other view using ConsolePrompt is the QuickConsole, which adds it as a subview. In this case, the prompt element is wrapped in another DOM element, which is why it can't be added directly as a subview. A possible solution is to extend ConsolePrompt, creating DatabaseConsolePrompt. It would be slightly awkward swapping out View.prototype._element with the wrapper element from the derived class, but it would work: class DatabaseConsolePrompt extends ConsolePrompt { constructor(delegate, mimeType) { super(delegate, mimeType); let wrapperElement = document.createElement("div"); wrapperElement.__view = this; wrapperElement.classList.add("database-query-prompt"); wrapperElement.appendChild(this.element); delete this.element.__view; this._element = wrapperElement; } }; We could support wrapper elements directly in the View base class with the following protected method: class View { // Protected insertRootElement(element) { console.assert(!this._element.parentNode); let oldRootElement = this._element; oldRootElement.__view = undefined; element.appendChild(oldRootElement); element.__view = this. this._element = element; } }; Then: class DatabaseConsolePrompt extends ConsolePrompt { constructor(delegate, mimeType) { super(delegate, mimeType); let wrapperElement = document.createElement("div"); wrapperElement.classList.add("database-query-prompt"); this.insertRootElement(wrapperElement); } }; > > Source/WebInspectorUI/UserInterface/Views/DatabaseContentView.js:98 > > + for (let resultElement of results) { > > What the heck is this old crap... :) we should be adding DataGrid as a > subview and remove this whole function. I'll look into it. Not sure how this is used. Actually, wrapping the View's DOM element from a derived class will be problematic. Superclasses between View and the most derived class would be operating on a different DOM element than the one they expect. (In reply to comment #4) > (In reply to comment #3) > > Comment on attachment 264825 [details] > > [Patch] Proposed Fix > > > > View in context: > > https://bugs.webkit.org/attachment.cgi?id=264825&action=review > > > > r- because you missed a few addSubview cases. > > > > It would have been nice to review the database / apache changes separately, > > since the rest was mechanical. > > Noted. I suspected as much during refactoring, and will remove from the > follow up. > > > > Source/WebInspectorUI/UserInterface/Views/DatabaseContentView.js:95 > > > + this.prompt.updateLayout(); > > > > ConsolePrompt should be a subview. > > The only other view using ConsolePrompt is the QuickConsole, which adds it > as a subview. In this case, the prompt element is wrapped in another DOM > element, which is why it can't be added directly as a subview. > > A possible solution is to extend ConsolePrompt, creating > DatabaseConsolePrompt. It would be slightly awkward swapping out > View.prototype._element with the wrapper element from the derived class, but > it would work: > > class DatabaseConsolePrompt extends ConsolePrompt > { > constructor(delegate, mimeType) > { > super(delegate, mimeType); > > let wrapperElement = document.createElement("div"); > wrapperElement.__view = this; > wrapperElement.classList.add("database-query-prompt"); > wrapperElement.appendChild(this.element); > > delete this.element.__view; > this._element = wrapperElement; > } > }; > > We could support wrapper elements directly in the View base class with the > following protected method: > Uh, that sounds complicated. Can we just straighten out the Database DOM tree, possibly in a followup patch if it will delay this much longer? Created attachment 264890 [details]
[Patch] Proposed Fix
DatabaseContentView is being refactored under https://bugs.webkit.org/show_bug.cgi?id=150959. Comment on attachment 264890 [details]
[Patch] Proposed Fix
r=me, nice work
Comment on attachment 264890 [details] [Patch] Proposed Fix Clearing flags on attachment: 264890 Committed r192086: <http://trac.webkit.org/changeset/192086> All reviewed patches have been landed. Closing bug. |