Bug 150729 – Web Inspector: Convert remaining ContentViews to View base class

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Inspector

[Matt Baker]

* SUMMARY
Convert remaining ContentViews to View base class.

[Radar WebKit Bug Importer]

<rdar://problem/23337412>

[Matt Baker]

Created attachment 264825 [details]
[Patch] Proposed Fix

[Blaze Burg]

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.

[Matt Baker]

(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.

[Matt Baker]

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.

[Blaze Burg]

(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?

[Matt Baker]

Created attachment 264890 [details]
[Patch] Proposed Fix

[Matt Baker]

DatabaseContentView is being refactored under https://bugs.webkit.org/show_bug.cgi?id=150959.

[Blaze Burg]

Comment on attachment 264890 [details]
[Patch] Proposed Fix

r=me, nice work

[WebKit Commit Bot]

Comment on attachment 264890 [details]
[Patch] Proposed Fix

Clearing flags on attachment: 264890

Committed r192086: <http://trac.webkit.org/changeset/192086>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.