WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
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
Details
Formatted Diff
Diff
[Patch] same with asserions instead of DOM listeners
(59.69 KB, patch)
2011-10-21 02:44 PDT
,
Pavel Feldman
no flags
Details
Formatted Diff
Diff
[Patch] Same with the test.
(71.70 KB, patch)
2011-10-21 05:33 PDT
,
Pavel Feldman
no flags
Details
Formatted Diff
Diff
[Patch] Review comments addressed.
(72.01 KB, patch)
2011-10-21 08:00 PDT
,
Pavel Feldman
yurys
: review+
Details
Formatted Diff
Diff
Show Obsolete
(3)
View All
Add attachment
proposed patch, testcase, etc.
Pavel Feldman
Comment 1
2011-10-20 10:40:34 PDT
Created
attachment 111806
[details]
Patch
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
Committed
r98194
: <
http://trac.webkit.org/changeset/98194
>
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug