Bug 93444 – Web Inspector: SourceFrame shouldn't be a View

• Component: Web Inspector (Deprecated)
• Status: RESOLVED INVALID

[Jan Keromnes]

Currently, SourceFrame is a View, and 100% of its screen space is filled by its TextEditor, which is also a View. Aside from being unnecessary, this will be problematic if we want future ExtensionTextEditors to create only one iframe ExtensionView for all the SourceFrames.

[Jan Keromnes]

Created attachment 157138 [details]
Patch

[Jan Keromnes]

Uploaded patch makes the following test fail:

inspector/styles/edit-inspector-stylesheet.html

It looks like it is flaky and I don't understand why it worked before.

[WebKit Review Bot]

Comment on attachment 157138 [details]
Patch

Attachment 157138 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/13460079

New failing tests:
inspector/styles/edit-inspector-stylesheet.html

[WebKit Review Bot]

Created attachment 157144 [details]
Archive of layout-test-results from gce-cr-linux-02

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: gce-cr-linux-02  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.39-gcg-201203291735-x86_64-with-Ubuntu-10.04-lucid

[Pavel Feldman]

Comment on attachment 157138 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=157138&action=review

> Source/WebCore/inspector/front-end/SourceFrame.js:88
> +    show: function(parent)

The change in the behavior here is that wasShown was called every time the view was shown (every time it got on screen be it switching on the container panel or switching to the tab that contains it). Now it happens only upon explicit show call.

> Source/WebCore/inspector/front-end/SourceFrame.js:97
> +        //WebInspector.View.prototype.willHide.call(this);

Same is true for detach. So I think that might be the source of the flakiness you were seeing. The right way of addressing it would be to add a listener for WasShown / WasHidden on the view object. Then SourceFrame would receive it exactly when it used to and no changes to the behavior would happen. And your show and detach would delegate to the corresponsing show and detach of the textEditor inside.

[Jan Keromnes]

Created attachment 157288 [details]
Patch

[Jan Keromnes]

(In reply to comment #5)
> The change in the behavior here is that wasShown was called every time the view was shown (every time it got on screen be it switching on the container panel or switching to the tab that contains it). Now it happens only upon explicit show call.
>
> Same is true for detach. So I think that might be the source of the flakiness you were seeing. The right way of addressing it would be to add a listener for WasShown / WasHidden on the view object. Then SourceFrame would receive it exactly when it used to and no changes to the behavior would happen. And your show and detach would delegate to the corresponsing show and detach of the textEditor inside.

Thanks for the advice, I made SourceFrame listen for WasShown and WillHide on its TextEditor and it works like a charm! On to #92769...

[Pavel Feldman]

Comment on attachment 157288 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=157288&action=review

Looks like it is almost ready.

> Source/WebCore/inspector/front-end/JavaScriptSourceFrame.js:66
> +    show: function(parent)

Just nuke the entire method.

> Source/WebCore/inspector/front-end/JavaScriptSourceFrame.js:74
>          this._popoverHelper.hidePopover();

This should go into the willHide listener (and be done every time we hide)

> Source/WebCore/inspector/front-end/SourceFrame.js:112
> +        //WebInspector.View.prototype.willHide.call(this);

nuke this line

[Jan Keromnes]

Created attachment 158151 [details]
Patch

[Jan Keromnes]

Should be ready now. Thanks for the feedback!

[Pavel Feldman]

Comment on attachment 158151 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=158151&action=review

> Source/WebCore/inspector/front-end/JavaScriptSourceFrame.js:66
> +    _onTextEditorWillHide: function()

You can't override private methods in a different file. _bar are only used within single file.

> Source/WebCore/inspector/front-end/SourceFrame.js:-98
> -        this._textEditor.show(this.element);

This still changes the behavior. We used to append the text editor upon showing it in the screen, i.e. lazily. Now it is appended right away. Why did you change this?

[Jan Keromnes]

Created attachment 158447 [details]
Patch

[Jan Keromnes]

(In reply to comment #11)
> > Source/WebCore/inspector/front-end/JavaScriptSourceFrame.js:66
> > +    _onTextEditorWillHide: function()
> 
> You can't override private methods in a different file. _bar are only used within single file.

Fixed by attaching a second listener to TextEditor.Events.WillHide instead of overriding a private method.

> > Source/WebCore/inspector/front-end/SourceFrame.js:-98
> > -        this._textEditor.show(this.element);
> 
> This still changes the behavior. We used to append the text editor upon showing it in the screen, i.e. lazily. Now it is appended right away. Why did you change this?

The only problem was TabbedEditorContainer calling show() on its tabs (i.e. sourceFrames, i.e. textEditors) even when itself wasn't showing. I fixed this by making TabbedPane lazy with its Views: Now, nobody calls sourceFrame.show() or textEditor.show() in an "unlazy fashion". Does that solve the problem?

[Jan Keromnes]

Comment on attachment 158447 [details]
Patch

The TabbedPane change doesn't work.

[Jan Keromnes]

Created attachment 158469 [details]
Patch

[Pavel Feldman]

Comment on attachment 158469 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=158469&action=review

> Source/WebCore/inspector/front-end/SourceFrame.js:-39
> -    this.element.addStyleClass("script-view");

You should remove the corresponding css definition.

> Source/WebCore/inspector/front-end/SourceFrame.js:-98
> -        this._textEditor.show(this.element);

I guess you should either refactor View.js (see below) or preserve the original behavior (store parent from show and use it in here).

> Source/WebCore/inspector/front-end/TabbedPane.js:461
> +        if (this.isShowing())

This kind of change should be done in the View.js once for all view containers, no need to specifically fix it for TabbedPane.

[Pavel Feldman]

Comment on attachment 158469 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=158469&action=review

>> Source/WebCore/inspector/front-end/TabbedPane.js:461
>> +        if (this.isShowing())
> 
> This kind of change should be done in the View.js once for all view containers, no need to specifically fix it for TabbedPane.

i did not realize that the laziness that was there was already mitigated by the view subsystem
earlier in the day, everything was in the dom at all times
so that laziness made sense. Then i added this view subsystem and it saved us a lot of time. I did not realize it was covering this case as well, sorry.

I think this part can be simply reverted.

[Jan Keromnes]

Created attachment 159016 [details]
Patch

[WebKit Review Bot]

Comment on attachment 159016 [details]
Patch

Clearing flags on attachment: 159016

Committed r125880: <http://trac.webkit.org/changeset/125880>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[WebKit Review Bot]

Re-opened since this is blocked by 94347

[johnjbarton]

Jan has returned to his studies.

This change was reverted because the compile fails:
-------------
Source/WebCore/inspector/front-end/TabbedEditorContainer.js:279: WARNING - actual parameter 3 of WebInspector.TabbedPane.prototype.appendTab does not match formal parameter
found   : (WebInspector.SourceFrame|null)
required: (WebInspector.View|null)
        this._tabbedPane.appendTab(tabId, title, view, tooltip, userGesture);
                                                 ^

Source/WebCore/inspector/front-end/TabbedEditorContainer.js:338: WARNING - actual parameter 2 of WebInspector.TabbedPane.prototype.changeTabView does not match formal parameter
found   : (WebInspector.SourceFrame|null)
required: (WebInspector.View|null)
        this._tabbedPane.changeTabView(tabId, this._delegate.viewForFile(uiSourceCode));
-------------

However all of the tests pass. This means that SourceFrame is adequate for the Tab 'view' API.

In particular TabbedPaneTab, a private class of TabbedPane, has a 'view' property that needs to support methods:
  .detach(), .show(), SourceFrame delegates to TextEditor
  .highlightLine(), .canHighlightLine() SourceFrame implements

The tab.view is also is exported by two events, TabClosed and TabSelected. However if that field is removed from the event.data all tests pass.

Finally TabbedPane has a .visibleView which exports the TabbedPaneTab.view. In TabbedEditorContainer it is called:
   .defaultFocusedElement(), SourceFrame delegates to TextEditor

But .visibleView is called by many other places. I guess that clients of .visibleView either rely on only the methods listed above, or they rely on knowing that they alone supply the .view value and thus can operate on it correctly. (ie, the 'view' objects are not mixed across containers so code that inserts knows the contained object type).

The correct solution to the compile failures depends upon the plan for SourceFrame. Is it still in flux? Suggestions?

[Pavel Feldman]

> 
> However all of the tests pass. This means that SourceFrame is adequate for the Tab 'view' API.
> 

When compilation fails and the tests pass that rather means that we have poor test coverage.

> In particular TabbedPaneTab, a private class of TabbedPane, has a 'view' property that needs to support methods:
>   .detach(), .show(), SourceFrame delegates to TextEditor
>   .highlightLine(), .canHighlightLine() SourceFrame implements
>

It is a 'view' property that is annotate with the type WebInspector.View.

> The correct solution to the compile failures depends upon the plan for SourceFrame. Is it still in flux? Suggestions?

Well, no matter what the plans for SourceFrame are, this patch is wrong. Instead of passing SourceFrame into all of these TabbedPane methods, Jan should have passed SourceFrame's view explicitly. I.e. instead of wrapping View's methods and faking a view, we should introduce view: function() {} in SourceFrame and use it where appropriate.

[johnjbarton]

(In reply to comment #23)
> > 
> > However all of the tests pass. This means that SourceFrame is adequate for the Tab 'view' API.
> > 
> 
> When compilation fails and the tests pass that rather means that we have poor test coverage.

Isn't it also possible that the annotations are incomplete?

> 
> > In particular TabbedPaneTab, a private class of TabbedPane, has a 'view' property that needs to support methods:
> >   .detach(), .show(), SourceFrame delegates to TextEditor
> >   .highlightLine(), .canHighlightLine() SourceFrame implements
> >
> 
> It is a 'view' property that is annotate with the type WebInspector.View.

Yes, but the annotation is not completely correct. TabbedPaneTab will 
work with a View but it will also work with an object with a subset of the
View API. In particular the client of TabbedPaneTab need not have an element even though View does have an element.

> 
> > The correct solution to the compile failures depends upon the plan for SourceFrame. Is it still in flux? Suggestions?
> 
> Well, no matter what the plans for SourceFrame are, this patch is wrong. Instead of passing SourceFrame into all of these TabbedPane methods, Jan should have passed SourceFrame's view explicitly. I.e. instead of wrapping View's methods and faking a view, we should introduce view: function() {} in SourceFrame and use it where appropriate.

But what object will this function return? It can't be the underlying TextEditor (which is a View) because that object does not currently implement all of the View methods as required by SourceFrame.

If further changes to SourceFrame will cause TextEditor to absorb the SourceFrame's View implementations, then we can make those changes and solve this bug. 

If further changes to SourceFrame aren't planned, then we could create a new API object, eg TabbedPaneTabDelegate, to allow an object like SourceFrame to be placed in the TabbedPane.

These are quite different directions and perhaps not the only two.