Bug 218678 - Web Inspector: drop `shown`/`hidden` in favor of `attached`/`detached`
Summary: Web Inspector: drop `shown`/`hidden` in favor of `attached`/`detached`
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (show other bugs)
Version: WebKit Nightly Build
Hardware: All All
: P2 Normal
Assignee: Devin Rousso
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2020-11-06 20:53 PST by Devin Rousso
Modified: 2020-11-20 14:37 PST (History)
12 users (show)

See Also:


Attachments
[Patch] WIP (97.24 KB, patch)
2020-11-06 21:05 PST, Devin Rousso
hi: commit-queue-
Details | Formatted Diff | Diff
[Patch] WIP (98.58 KB, patch)
2020-11-11 17:43 PST, Devin Rousso
no flags Details | Formatted Diff | Diff
Patch (124.72 KB, patch)
2020-11-11 18:48 PST, Devin Rousso
no flags Details | Formatted Diff | Diff
Patch (124.90 KB, patch)
2020-11-11 18:52 PST, Devin Rousso
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Devin Rousso 2020-11-06 20:53:51 PST
- `attached`/`detached` is tied into `WI.View`, meaning that `attached` is called right after the node is added to the main DOM tree and `detached` is called right before the node is removed from the main DOM tree (we should be removing nodes that aren't visible from the DOM anyways)
 - `shown`/`hidden` are usually called right after `addSubview`/`removeSubview`, which should have already called `attached`/`detached`
 - `attached`/`detached` is already propagated to the entire view subtree, whereas `shown`/`hidden` must be propagated manually
Comment 1 Devin Rousso 2020-11-06 21:05:42 PST
Created attachment 413514 [details]
[Patch] WIP

I think this all works, but I'm going to live on it for a bit before writing a ChangeLog and such
Comment 2 Devin Rousso 2020-11-11 17:43:33 PST
Created attachment 413891 [details]
[Patch] WIP

Fixed an issue I just discovered where switching to the Console Tab when the split console is showing caused the split console to always appear blank.  This happens because the `WI.ConsoleDrawer` is never removed from the DOM even when hidden, meaning that `detached` is never called on it's `WI.ContentViewContainer` which is what's responsible for hiding all of the entries in the back/forward list and therefore when the `WI.consoleContentView` is re-shown (via `showContentView`) it matches the current item in the back/forward list and is not re-added.  Changed `WI.ConsoleDrawer.prototype.set collapsed` to manually call `attached`/`detached`.

Alongside this, I also added a fix/drive-by for re-showing the split console when switching away from the Console Tab if the split console was previously shown.
Comment 3 Devin Rousso 2020-11-11 18:48:44 PST
Created attachment 413898 [details]
Patch
Comment 4 Devin Rousso 2020-11-11 18:52:22 PST
Created attachment 413899 [details]
Patch
Comment 5 BJ Burg 2020-11-13 11:34:34 PST
Comment on attachment 413899 [details]
Patch

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

> Source/WebInspectorUI/UserInterface/Views/View.js:129
> +        view._didMoveToParent(null);

I feel it's more accurate that this would be _willMoveToParent() since it happens prior to element.remove()... but it probably doesn't matter.
Comment 6 Devin Rousso 2020-11-13 12:40:59 PST
Comment on attachment 413899 [details]
Patch

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

>> Source/WebInspectorUI/UserInterface/Views/View.js:129
>> +        view._didMoveToParent(null);
> 
> I feel it's more accurate that this would be _willMoveToParent() since it happens prior to element.remove()... but it probably doesn't matter.

IMO it either should have a `will*`/`did*` pair or not have any tense at all (e.g. `moveToParent`).  But yeah it's just naming :P
Comment 7 Radar WebKit Bug Importer 2020-11-13 20:54:14 PST
<rdar://problem/71395601>
Comment 8 BJ Burg 2020-11-20 14:19:42 PST
Comment on attachment 413899 [details]
Patch

r=me
Comment 9 EWS 2020-11-20 14:37:07 PST
Committed r270134: <https://trac.webkit.org/changeset/270134>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 413899 [details].