This change streamlines the way we operate views.
Created attachment 111806 [details] Patch
Comment on attachment 111806 [details] Patch Removing r? as pfeldman is still working on it.
Created attachment 111928 [details] [Patch] same with asserions instead of DOM listeners
Created attachment 111956 [details] [Patch] Same with the test.
Comment on attachment 111956 [details] [Patch] Same with the test. View in context: https://bugs.webkit.org/attachment.cgi?id=111956&action=review LGTM with a handful of nits. Some other considerations that may be addressed in the future patches: - we now lack a method to dispose of a view that was marked with setHideOnDetach(). Such views would linger in DOM forever, as detach would not really detach them; -- Managing isRoot is a bit obscure (we set it explicitly but may implicitly clear) -- We never call base implementations of willHide/wasShown from Views, yet always call from Panels. This is inconsistent. We also have some logic in dispatchFoo methods, which is a bit confusing considering these names. > Source/WebCore/inspector/front-end/View.js:50 > + return this._isShowing || this._isRoot; So does this mean that a view that was once marked as root will remain isShowing() after being detached? > Source/WebCore/inspector/front-end/View.js:69 > this.restoreScrollPositions(); Having logic like this in a method called dispatchSomething is a bit surprising. > Source/WebCore/inspector/front-end/View.js:82 > this.storeScrollPositions(); ditto. > Source/WebCore/inspector/front-end/View.js:141 > + while (parentElement && !parentElement.__view) > + parentElement = parentElement.parentElement; Reusing input parameter is confusing. Consider var parentViewElement? > Source/WebCore/inspector/front-end/View.js:258 > + while (parentElement) { nit: for seems more appropriate > Source/WebCore/inspector/front-end/View.js:264 > +WebInspector.View._decrementViewCounter = function(parentElement, childElement) nit: Generalize with _incrementViewCounter, e.g. adjustViewCounter?
> So does this mean that a view that was once marked as root will remain isShowing() after being detached? I don't see a better solution for now. Root is the main panels + drawer view. I'll figure something prettier out. > Having logic like this in a method called dispatchSomething is a bit surprising. Done. > > Source/WebCore/inspector/front-end/View.js:82 > > this.storeScrollPositions(); > Done. > > Source/WebCore/inspector/front-end/View.js:141 > > + while (parentElement && !parentElement.__view) > > + parentElement = parentElement.parentElement; > > Reusing input parameter is confusing. Consider var parentViewElement? Done. > > Source/WebCore/inspector/front-end/View.js:258 > > + while (parentElement) { > > nit: for seems more appropriate Changed this code a bit. > > Source/WebCore/inspector/front-end/View.js:264 > > +WebInspector.View._decrementViewCounter = function(parentElement, childElement) > > nit: Generalize with _incrementViewCounter, e.g. adjustViewCounter? I am not a fan of this :).
Created attachment 111966 [details] [Patch] Review comments addressed.
Comment on attachment 111966 [details] [Patch] Review comments addressed. View in context: https://bugs.webkit.org/attachment.cgi?id=111966&action=review > Source/WebCore/inspector/front-end/View.js:277 > +WebInspector.View._assert = function(condition, message) This looks generic enough to be in utilities.js
Comment on attachment 111966 [details] [Patch] Review comments addressed. View in context: https://bugs.webkit.org/attachment.cgi?id=111966&action=review > Source/WebCore/inspector/front-end/View.js:175 > + if (childIndex < 0) { Could be _assert
Committed r98194: <http://trac.webkit.org/changeset/98194>