Bug 143694 – Web Inspector: Indent top-level arrays, set, and maps differently from arrays, set, and maps inside objects

• Status: ASSIGNED
• Component: Web Inspector

[Nikita Vasilyev]

https://cloudup.com/cLWiZVlYqpN
https://cloudup.com/cMMumnHlf8H

[Radar WebKit Bug Importer]

<rdar://problem/20530612>

[Nikita Vasilyev]

WIP: http://n12v.com.s3.amazonaws.com/webkit-inspector/1/index.html

What do you think? Should I go ahead and submit the patch?

Note that it works in ToT WebKit, but not in Safari 8.0.5. It may take a while to load.

I grew tired rebuilding and restarting WebKit after every change, so I pulled HTML and CSS from the ToT inspector into a static page. It made tweaking CSS much more pleasant and, as a side-effect, easier to share where I’m at.

[Timothy Hatcher]

Yes, please post a patch so we can see the changes involved. From previewing the HTML it looks good to me.

[Nikita Vasilyev]

Created attachment 250988 [details]
Patch

[Build Bot]

Comment on attachment 250988 [details]
Patch

Attachment 250988 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/5354139441692672

New failing tests:
compositing/scrolling/touch-scroll-to-clip.html

[Build Bot]

Created attachment 250997 [details]
Archive of layout-test-results from ews102 for mac-mavericks

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews102  Port: mac-mavericks  Platform: Mac OS X 10.9.5

[Nikita Vasilyev]

Created attachment 251000 [details]
[Image] Popovers with the patch applied

[Timothy Hatcher]

Comment on attachment 250988 [details]
Patch

Looks fine to me. Joe, okay with you?

[Joseph Pecoraro]

Comment on attachment 250988 [details]
Patch

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

Seeing as this special cases the Console and Popover. What do the default values look like in the Scopes sidebar or Node details sidebars?

> Source/WebInspectorUI/UserInterface/Views/ObjectTreeView.css:131
> +.console-message .object-tree.object-type-array > .object-tree-outline > .object-tree-array-index {
> +    left: -24px;
> +}

This file (ObjectTreeView.css) shouldn't include ".console-message" as ObjectTreeViews do not contain console messages. It should be in ConsoleMessageView.css, because it is about the console message view styling a object tree sub-component.

> Source/WebInspectorUI/UserInterface/Views/ObjectTreeView.css:143
> +.popover .object-tree.object-type-array > .object-tree-outline > .object-tree-array-index {
> +    left: -18px;
> +}

Likewise, these would need to be in Popover.css, or SourceCodeTextEditor.css.

[Joseph Pecoraro]

After applying this patch, it looks like this doesn't handle the case of on Array within a Set in either the Scopes sidebar or Popovers. The Console did handle this correctly. See the image attachment.

[Joseph Pecoraro]

Created attachment 251031 [details]
[IMAGE] Issues in Popover and Scopes Sidebar

[Joseph Pecoraro]

Comment on attachment 250988 [details]
Patch

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

>> Source/WebInspectorUI/UserInterface/Views/ObjectTreeView.css:131
>> +}
> 
> This file (ObjectTreeView.css) shouldn't include ".console-message" as ObjectTreeViews do not contain console messages. It should be in ConsoleMessageView.css, because it is about the console message view styling a object tree sub-component.

In fact it is weird to have to give custom styles to the console at all. What I would expect is:

  1. Object Tree styles handle proper indentation of indices at the top level and nested
  2. Console shouldn't need to make any adjustments
  3. Popovers can just shift the object-tree container over, without messing with the internals of ObjectTrees

> Source/WebInspectorUI/UserInterface/Views/ObjectTreeView.js:52
> +        this._element.classList.add("object-type-" + (object.subtype || object.type));

Which should make this unnecessary.

There is no doubt this looks better, but I think it goes about it in a very weird way, and as such it misses these cases.

[Nikita Vasilyev]

Created attachment 251114 [details]
Patch

(In reply to comment #12)
> Comment on attachment 250988 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=250988&action=review
> 
> >> Source/WebInspectorUI/UserInterface/Views/ObjectTreeView.css:131
> >> +}
> > 
> > This file (ObjectTreeView.css) shouldn't include ".console-message" as ObjectTreeViews do not contain console messages. It should be in ConsoleMessageView.css, because it is about the console message view styling a object tree sub-component.
> 
> In fact it is weird to have to give custom styles to the console at all.

Yes, I agree, but I couldn’t do it without them. Read below.

> What I would expect is:
> 
>   1. Object Tree styles handle proper indentation of indices at the top
> level and nested
>   2. Console shouldn't need to make any adjustments
>   3. Popovers can just shift the object-tree container over, without messing
> with the internals of ObjectTrees

I tried it, here is the patch.

Look at the objects outline – they need to be pulled on the left. I’d need to write at least one CSS rule specific for top-level objects inside of popups to fix it.

[Nikita Vasilyev]

Created attachment 251115 [details]
[Image] Popovers with the patch applied 2