RESOLVED FIXED 70523
Web Inspector: get rid of View::hide, View::set visible, View::attach.
https://bugs.webkit.org/show_bug.cgi?id=70523
Summary Web Inspector: get rid of View::hide, View::set visible, View::attach.
Pavel Feldman
Reported 2011-10-20 10:39:01 PDT
This change streamlines the way we operate views.
Attachments
Patch (44.42 KB, patch)
2011-10-20 10:40 PDT, Pavel Feldman
no flags
[Patch] same with asserions instead of DOM listeners (59.69 KB, patch)
2011-10-21 02:44 PDT, Pavel Feldman
no flags
[Patch] Same with the test. (71.70 KB, patch)
2011-10-21 05:33 PDT, Pavel Feldman
no flags
[Patch] Review comments addressed. (72.01 KB, patch)
2011-10-21 08:00 PDT, Pavel Feldman
yurys: review+
Pavel Feldman
Comment 1 2011-10-20 10:40:34 PDT
Yury Semikhatsky
Comment 2 2011-10-21 01:07:43 PDT
Comment on attachment 111806 [details] Patch Removing r? as pfeldman is still working on it.
Pavel Feldman
Comment 3 2011-10-21 02:44:15 PDT
Created attachment 111928 [details] [Patch] same with asserions instead of DOM listeners
Pavel Feldman
Comment 4 2011-10-21 05:33:49 PDT
Created attachment 111956 [details] [Patch] Same with the test.
Andrey Kosyakov
Comment 5 2011-10-21 07:14:38 PDT
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?
Pavel Feldman
Comment 6 2011-10-21 07:57:43 PDT
> 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 :).
Pavel Feldman
Comment 7 2011-10-21 08:00:01 PDT
Created attachment 111966 [details] [Patch] Review comments addressed.
Andrey Kosyakov
Comment 8 2011-10-21 08:35:57 PDT
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
Yury Semikhatsky
Comment 9 2011-10-21 08:50:53 PDT
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
Pavel Feldman
Comment 10 2011-10-22 12:31:16 PDT
Note You need to log in before you can comment on or make changes to this bug.