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.
Created attachment 157138 [details] Patch
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.
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
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
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.
Created attachment 157288 [details] Patch
(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...
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
Created attachment 158151 [details] Patch
Should be ready now. Thanks for the feedback!
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?
Created attachment 158447 [details] Patch
(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?
Comment on attachment 158447 [details] Patch The TabbedPane change doesn't work.
Created attachment 158469 [details] Patch
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.
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.
Created attachment 159016 [details] Patch
Comment on attachment 159016 [details] Patch Clearing flags on attachment: 159016 Committed r125880: <http://trac.webkit.org/changeset/125880>
All reviewed patches have been landed. Closing bug.
Re-opened since this is blocked by 94347
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?
> > 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.
(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.