Bug 105023 – Web Inspector: Change the InspectorOverlay to use native rather than canvas

• Component: Web Inspector
• Status: RESOLVED FIXED

[Konrad Piascik]

The painting of the inspector highlight using 2D canvas is expensive and slow.  To speed up performance we should use the native GraphicsContext directly as was done previously.

[Blaze Burg]

Is this actually an issue? The highlights lag by a few frames when moving the mouse quickly in node-selecting mode, but is this a problem for most use cases?

[Timothy Hatcher]

FWIW, it use to be native until the Chromium folks made it use canvas. I never knew why it was changed in the first place…

We lost line box highlighting in the process.

[Radar WebKit Bug Importer]

<rdar://problem/19281432>

[Brian Burg]

As a halfway option, I think we should make paint flashing always use a native backend directly (iOS uses CALayers, I think).

For inspecting nodes and interacting with the DOM tree, I think we still want canvas and JS running in the overlay page. The engineering effort required to implement and maintain the highlight rendering code in C++ is not worth it, IMO.

If a user simply wants to follow a rapidly-changing element on the screen, we can use a native code path to draw only a quad for it rather than the full highlight information (which is a substantial amount of data).

[Blaze Burg]

<rdar://problem/13443692>

[Devin Rousso]

Created attachment 357882 [details]
[Patch] WIP

[Devin Rousso]

<rdar://problem/19281432>

[Devin Rousso]

<rdar://problem/13443692>

[Devin Rousso]

Created attachment 358151 [details]
Patch

Since we no longer need any of the OverlayTypes protocol objects, it doesn't make sense to keep them around just for tests, especially since the actual code path wouldn't use the types either.  I\'m not set on the idea of using a base64 encoded image of the contents of the overlay as a means of testing the correctness of the overlay, but I figured that it was a step in the right direction.  This would also allow us to test interactions a bit more (e.g. making sure that things don\'t overlap unless we want them to), as well as more general overlay things (e.g. rulers).  Feedback welcome :)

[EWS Watchlist]

Comment on attachment 358151 [details]
Patch

Attachment 358151 [details] did not pass mac-ews (mac):
Output: https://webkit-queues.webkit.org/results/10586813

New failing tests:
inspector/dom/highlightNodeList.html
inspector/dom/hideHighlight.html
inspector/dom/highlightSelector.html
inspector/dom/highlight-shape-outside-margin.html
inspector/dom/highlightFrame.html
inspector/dom/highlightNode.html

[EWS Watchlist]

Created attachment 358152 [details]
Archive of layout-test-results from ews102 for mac-sierra

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

[EWS Watchlist]

Comment on attachment 358151 [details]
Patch

Attachment 358151 [details] did not pass mac-debug-ews (mac):
Output: https://webkit-queues.webkit.org/results/10586887

New failing tests:
inspector/dom/highlightNodeList.html
inspector/dom/hideHighlight.html
inspector/dom/highlightSelector.html
inspector/dom/highlight-shape-outside-margin.html
inspector/dom/highlightFrame.html
inspector/dom/highlightNode.html

[EWS Watchlist]

Created attachment 358153 [details]
Archive of layout-test-results from ews113 for mac-sierra

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews113  Port: mac-sierra  Platform: Mac OS X 10.12.6

[Devin Rousso]

Created attachment 358158 [details]
Patch

[EWS Watchlist]

Comment on attachment 358158 [details]
Patch

Attachment 358158 [details] did not pass mac-wk2-ews (mac-wk2):
Output: https://webkit-queues.webkit.org/results/10595343

New failing tests:
inspector/dom/highlightNodeList.html
inspector/dom/hideHighlight.html
inspector/dom/highlightSelector.html
inspector/dom/highlight-shape-outside-margin.html
inspector/dom/highlightFrame.html
inspector/dom/highlightNode.html

[EWS Watchlist]

Created attachment 358160 [details]
Archive of layout-test-results from ews104 for mac-sierra-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews104  Port: mac-sierra-wk2  Platform: Mac OS X 10.12.6

[EWS Watchlist]

Comment on attachment 358158 [details]
Patch

Attachment 358158 [details] did not pass mac-ews (mac):
Output: https://webkit-queues.webkit.org/results/10595369

New failing tests:
inspector/dom/highlightNodeList.html
inspector/dom/highlightSelector.html
inspector/dom/highlightFrame.html
inspector/dom/hideHighlight.html
inspector/dom/highlightNode.html

[EWS Watchlist]

Created attachment 358161 [details]
Archive of layout-test-results from ews101 for mac-sierra

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

[EWS Watchlist]

Comment on attachment 358158 [details]
Patch

Attachment 358158 [details] did not pass mac-debug-ews (mac):
Output: https://webkit-queues.webkit.org/results/10595432

New failing tests:
inspector/dom/highlightNodeList.html
inspector/dom/highlightSelector.html
inspector/dom/highlightFrame.html
inspector/dom/highlightNode.html
inspector/dom/hideHighlight.html

[EWS Watchlist]

Created attachment 358162 [details]
Archive of layout-test-results from ews112 for mac-sierra

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews112  Port: mac-sierra  Platform: Mac OS X 10.12.6

[Devin Rousso]

Created attachment 358163 [details]
Patch

[EWS Watchlist]

Comment on attachment 358163 [details]
Patch

Attachment 358163 [details] did not pass mac-wk2-ews (mac-wk2):
Output: https://webkit-queues.webkit.org/results/10596266

New failing tests:
inspector/dom/highlightNodeList.html
inspector/dom/hideHighlight.html
inspector/dom/highlightSelector.html
inspector/dom/highlight-shape-outside-margin.html
inspector/dom/highlightFrame.html
inspector/dom/highlightNode.html

[EWS Watchlist]

Created attachment 358165 [details]
Archive of layout-test-results from ews106 for mac-sierra-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews106  Port: mac-sierra-wk2  Platform: Mac OS X 10.12.6

[Simon Fraser (smfr)]

Comment on attachment 358163 [details]
Patch

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

> Source/WebCore/inspector/InspectorOverlay.cpp:86
> +    return Color(r / 255.0f, g / 255.0f, b / 255.0f, a);

Use the Color constructor that takes ints.

> Source/WebCore/inspector/InspectorOverlay.cpp:215
> +static Path quadToPath(const FloatQuad& quad, FloatRect& bounds)
> +{
> +    Path path;
> +    path.moveTo(quad.p1());
> +    path.addLineTo(quad.p2());
> +    path.addLineTo(quad.p3());
> +    path.addLineTo(quad.p4());
> +    path.closeSubpath();
> +
> +    bounds.unite(path.boundingRect());
> +
> +    return path;
> +}

Maybe this should be a function on Path.

> Source/WebCore/inspector/InspectorOverlay.cpp:533
> +                                path.apply([&] (const PathElement& pathElement) {
> +                                    const auto localToRoot = [&] (size_t index) {
> +                                        const FloatPoint& point = pathElement.points[index];
> +                                        return localPointToRootPoint(containingView, renderer->localToAbsolute(shapeOutsideInfo->shapeToRendererPoint(point)));
> +                                    };
> +
> +                                    switch (pathElement.type) {
> +                                    case PathElementMoveToPoint:
> +                                        newPath.moveTo(localToRoot(0));
> +                                        break;
> +
> +                                    case PathElementAddLineToPoint:
> +                                        newPath.addLineTo(localToRoot(0));
> +                                        break;
> +
> +                                    case PathElementAddCurveToPoint:
> +                                        newPath.addBezierCurveTo(localToRoot(0), localToRoot(1), localToRoot(2));
> +                                        break;
> +
> +                                    case PathElementAddQuadCurveToPoint:
> +                                        newPath.addQuadCurveTo(localToRoot(0), localToRoot(1));
> +                                        break;
> +
> +                                    case PathElementCloseSubpath:
> +                                        newPath.closeSubpath();
> +                                        break;
> +                                    }
> +                                });
> +                                return newPath;
> +                            };

When is this not just a path translate?

> Source/WebCore/inspector/InspectorOverlay.cpp:589
> +                        HashSet<AtomicString> usedClassNames;
> +                        const SpaceSplitString& classNamesString = element->classNames();
> +                        for (size_t i = 0; i < classNamesString.size(); ++i) {
> +                            const AtomicString& className = classNamesString[i];
> +                            if (usedClassNames.contains(className))
> +                                continue;
> +
> +                            usedClassNames.add(className);
> +
> +                            builder.append('.');
> +                            builder.append(DOMCSSNamespace::escape(className));
> +                        }
> +
> +                        elementClassValue = builder.toString();
> +                        truncateWithEllipsis(elementClassValue, 50);

Can you use DOMTokenList to do this?

> Source/WebCore/inspector/InspectorOverlay.cpp:621
> +#if OS(MAC_OS_X)

Really? Don't we have a "system monospace" font that works everywhere?

> Source/WebCore/inspector/InspectorOverlay.cpp:697
> +

This function is getting really long. Suggest you break it up.

> Source/WebCore/inspector/InspectorOverlay.cpp:794
> +    FloatSize contentInset(0, pageView->topContentInset(ScrollView::TopContentInsetType::WebCoreOrPlatformContentInset));

You shouldn't adjust by topContentInset manually. You should use something like ScrollView's contentsToRootView().

> Source/WebCore/inspector/InspectorOverlay.cpp:834
> +    FloatSize contentInset(0, pageView->topContentInset(ScrollView::TopContentInsetType::WebCoreOrPlatformContentInset));

Ditto.

[Devin Rousso]

Comment on attachment 358163 [details]
Patch

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

Thanks for the comments.  I was mainly focused on getting it working and seeing if it performed well, so I didn't think really at all about optimizing/refactoring.

>> Source/WebCore/inspector/InspectorOverlay.cpp:86
>> +    return Color(r / 255.0f, g / 255.0f, b / 255.0f, a);
> 
> Use the Color constructor that takes ints.

I tend to prefer using floats for alpha values, but I don't see any reason not to do this.  I'll change it.

>> Source/WebCore/inspector/InspectorOverlay.cpp:533
>> +                            };
> 
> When is this not just a path translate?

This is what the old code did (in addition to serializing each point), so I kept it as is.  From what I can see, it looks like `ShapeOutsideInfo::shapeToRendererPoint` can transpose the given point, so that may be a potential difference.

>> Source/WebCore/inspector/InspectorOverlay.cpp:621
>> +#if OS(MAC_OS_X)
> 
> Really? Don't we have a "system monospace" font that works everywhere?

I did a bit more digging and it seems like it's referred to as  `fixedFontFamily`, not anything using "monospace".  Within WebInspector, we prefer using "Menlo" as our monospace font (not sure why, it's been that way for a while), so this is to keep that consistent.  I'll make this a bit more platform agnostic.

>> Source/WebCore/inspector/InspectorOverlay.cpp:794
>> +    FloatSize contentInset(0, pageView->topContentInset(ScrollView::TopContentInsetType::WebCoreOrPlatformContentInset));
> 
> You shouldn't adjust by topContentInset manually. You should use something like ScrollView's contentsToRootView().

Why do you say I shouldn't directly use `topContentInset`?  Unless I'm misreading the code, `contentsToRootView` takes into account the `scrollPosition`, which is something I'm trying to avoid.  I'm only really using this to make sure I don't draw things outside the visible area (e.g. behind the tab bar).

[Devin Rousso]

Created attachment 358215 [details]
Patch

[Simon Fraser (smfr)]

If we ever wanted to do more complex inspector overlays, like Firefox's Grid and Flex inspectors, is this the right direction to go in?

[Simon Fraser (smfr)]

(In reply to Devin Rousso from comment #25)

> 
> >> Source/WebCore/inspector/InspectorOverlay.cpp:794
> >> +    FloatSize contentInset(0, pageView->topContentInset(ScrollView::TopContentInsetType::WebCoreOrPlatformContentInset));
> > 
> > You shouldn't adjust by topContentInset manually. You should use something like ScrollView's contentsToRootView().
> 
> Why do you say I shouldn't directly use `topContentInset`?  Unless I'm
> misreading the code, `contentsToRootView` takes into account the
> `scrollPosition`, which is something I'm trying to avoid.  I'm only really
> using this to make sure I don't draw things outside the visible area (e.g.
> behind the tab bar).

topContentInset is an implementation detail. Code should use higher-level coordinate mapping functions provided by Widget/ScrollView etc.

[Devin Rousso]

(In reply to Simon Fraser (smfr) from comment #27)
> If we ever wanted to do more complex inspector overlays, like Firefox's Grid and Flex inspectors, is this the right direction to go in?
If we ever wanted to do more complex inspector overlays, like Firefox's Grid and Flex inspectors, is this the right direction to go in?
Speaking as someone who hadn't worked with `WebCore::GraphicsContext` until this patch, I found it super easy to use and almost everything I would use/expect from a <canvas> was available (in some form).  I don't think it would be that much more difficult to build something like you're suggesting with `WebCore::GraphicsContext` instead of a <canvas> (the main "difficulty" is just working around how strict C++ is compared to JavaScript).

[Simon Fraser (smfr)]

How will we make the inspector overlay accessible?

[Nikita Vasilyev]

(In reply to Devin Rousso from comment #29)
> (In reply to Simon Fraser (smfr) from comment #27)
> > If we ever wanted to do more complex inspector overlays, like Firefox's Grid and Flex inspectors, is this the right direction to go in?
> If we ever wanted to do more complex inspector overlays, like Firefox's Grid
> and Flex inspectors, is this the right direction to go in?
> Speaking as someone who hadn't worked with `WebCore::GraphicsContext` until
> this patch, I found it super easy to use and almost everything I would
> use/expect from a <canvas> was available (in some form).  I don't think it
> would be that much more difficult to build something like you're suggesting
> with `WebCore::GraphicsContext` instead of a <canvas> (the main "difficulty"
> is just working around how strict C++ is compared to JavaScript).

I have the same concern as Simon.

Is the current canvas overlay performance still an issue? I've never experienced it personally, so I honestly don't know. If it is, is the C++ version measurably faster?

[Devin Rousso]

(In reply to Simon Fraser (smfr) from comment #30)
> How will we make the inspector overlay accessible?
That's a good question.  Considering the old overlay was majority <canvas> UI, I don't think using C++ is any worse per-se, but this definitely needs work (WebInspector itself as well).  Right now, it isn't possible to interact with the inspector overlay directly (other than seeing the visuals), so everything that appears is triggered by some action inside WebInspector (the one exception to this is "Develop > Start Element Selection ⇧⌘C").

(In reply to Nikita Vasilyev from comment #31)
> I have the same concern as Simon.
> 
> Is the current canvas overlay performance still an issue? I've never experienced it personally, so I honestly don't know. If it is, is the C++ version measurably faster?
Yes, this is still very much so an issue.  It noticeably lags (+1s) when my laptop is connected to an external display and I have the window maximized.  I'm OOO right now without an external display, but I asked @JoePeck to test it on my behalf, and he confirmed that it didn't have the same "lagging" issues as the JavaScript version.

[Simon Fraser (smfr)]

(In reply to Devin Rousso from comment #32)

> > Is the current canvas overlay performance still an issue? I've never experienced it personally, so I honestly don't know. If it is, is the C++ version measurably faster?
> Yes, this is still very much so an issue.  It noticeably lags (+1s) when my
> laptop is connected to an external display and I have the window maximized. 
> I'm OOO right now without an external display, but I asked @JoePeck to test
> it on my behalf, and he confirmed that it didn't have the same "lagging"
> issues as the JavaScript version.

But fixing this lag just requires making accelerated drawing work. It seems counter-productive to just throw away the overlay stuff to fix this.

If anything, we should make the overlay more like web page, not less.

[EWS Watchlist]

Comment on attachment 358215 [details]
Patch

Attachment 358215 [details] did not pass mac-ews (mac):
Output: https://webkit-queues.webkit.org/results/10609526

New failing tests:
inspector/dom/highlight-shape-outside-margin.html
inspector/dom/highlightFrame.html

[EWS Watchlist]

Created attachment 358222 [details]
Archive of layout-test-results from ews102 for mac-sierra

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

[EWS Watchlist]

Comment on attachment 358215 [details]
Patch

Attachment 358215 [details] did not pass mac-wk2-ews (mac-wk2):
Output: https://webkit-queues.webkit.org/results/10609657

New failing tests:
inspector/dom/highlightNodeList.html
inspector/dom/hideHighlight.html
inspector/dom/highlightSelector.html
inspector/dom/highlight-shape-outside-margin.html
inspector/dom/highlightFrame.html
inspector/dom/highlightNode.html

[EWS Watchlist]

Created attachment 358223 [details]
Archive of layout-test-results from ews107 for mac-sierra-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews107  Port: mac-sierra-wk2  Platform: Mac OS X 10.12.6

[EWS Watchlist]

Comment on attachment 358215 [details]
Patch

Attachment 358215 [details] did not pass mac-debug-ews (mac):
Output: https://webkit-queues.webkit.org/results/10609676

New failing tests:
inspector/dom/highlight-shape-outside-margin.html
inspector/dom/highlightFrame.html

[EWS Watchlist]

Created attachment 358226 [details]
Archive of layout-test-results from ews114 for mac-sierra

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews114  Port: mac-sierra  Platform: Mac OS X 10.12.6

[Devin Rousso]

(In reply to Simon Fraser (smfr) from comment #33)
> But fixing this lag just requires making accelerated drawing work. It seems counter-productive to just throw away the overlay stuff to fix this.
> 
> If anything, we should make the overlay more like web page, not less.
Personally, I don't like the idea of having the inspector overlay be a web page.  I've been wanting to do this work for a while now.  It feels like a "hack" to me, especially considering the fact that the vast majority of the work being done in the inspector overlay is drawing to a <canvas>.  Yes, it makes drawing the DOM element information easier, but even that isn't so simple because it needs to be exactly positioned to match the <canvas> (the arrow and background is from the <canvas>; only the text is in the DOM).  Additionally, the extra work of serializing all of the data used to draw really seems unnecessary given that we're just drawing to a canvas.  I'm in favor of transitioning the inspector canvas back to a native approach, since we could also use this later on to draw more things on other platforms (e.g. iOS only draws highlights right now, not even DOM element info).

[Tim Horton]

(In reply to Simon Fraser (smfr) from comment #30)
> How will we make the inspector overlay accessible?

Technically PageOverlay does have limited accessibility support as long as you're willing to do the hit-testing within your content yourself.

[Devin Rousso]

Created attachment 360812 [details]
Patch

[Devin Rousso]

After discussing this offline, we've decided to move forward with a native approach.


# Reasons for using a page/<canvas>
 1. it's more friendly for web developers, as <canvas> (as well as HTML/CSS) has a well known API surface
 2. it already "works" (albeit with some issues, like this bug)
 3. HTML/CSS has better existing accessibility support (assuming we move away from <canvas>, and that we can expose the page's accessibility tree)

## Counterpoints
 1. the API surface of native solutions (including WebCore::GraphicsContext) is <canvas> extremely similar to that of canvas (with the exception of drawing text, although that is not difficult to do)
 2. the work has been done to make a native approach work
 3. this makes a lot of assumptions, namely that a screen reader (or other assistive tools) is even able to interact/read the overlay's contents, which seems highly unlikely right now given that the overlay isn't actually a page per-se, more-so it's a rendering tool we leverage to "populate" a WebCore::GraphicsContext


# Reasons for using a native approach
 1. it (should) allows for a more unified path to drawing the overlay contents on iOS (e.g. element data text, rulers, etc)
 2. in testing (e.g. using it on my machine) it has shown to be vastly more performant (albeit I haven't compared how well it works if we had fixed the acceleration issues as mentioned by @Simon)s
 3. it potentially gives us greater control of what we do as we can interact directly with the drawing surface, instead of having to interface with a <canvas> (or the DOM/CSS tree)
 4. we don't need to do any stringification/objectification of data in order to evaluate it in the context of a page
 5. it's potentially more memory efficient, as we don't need to create an entire page to draw

## Counterpoints
 1. if we decided to just use the full page on iOS, there wouldn't even be a different code path (there would be increased memory usage on iOS, however)
 2. as mentioned, this isn't comparing the performance against an accelerated page, so this may not be true
 3. we can always add special functionality that is only available in the page, although that is harder to achieve
 4. this removes a way of control that we theoretically could've exposed to the frontend (e.g. add a `Page.drawOverlay` command that accepts an `OverlayTypes` protocol object and passes it directly to the page, giving full control over what's drawn)
 5. although it may not keep as much in active memory, it may require more computations to be done each time we wish to draw something (e.g. recalculating all of the inset areas)


Given the above, I believe that the memory/performance improvements that we have seen (so far) with a native approach (as well as the potential of unifying with the iOS code path) is a good justification to transition to it instead of using a page/<canvas>.

Worst case, we can always rollout and reevaluate :P

[Devin Rousso]

(In reply to Devin Rousso from comment #42)
Still not 100% sure about how to test this, but one idea I had was to try to add console logs when `m_page.inspectorController().isUnderTest()` for each of the actual draw commands, so we can better match that we're drawing things in the right place.  Frankly, a visual test is the best way of knowing if we're doing something wrong, but that's a bit more difficult to automate...

[EWS Watchlist]

Comment on attachment 360812 [details]
Patch

Attachment 360812 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: https://webkit-queues.webkit.org/results/10984097

New failing tests:
imported/w3c/web-platform-tests/webrtc/simplecall.https.html

[EWS Watchlist]

Created attachment 360821 [details]
Archive of layout-test-results from ews126 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews126  Port: ios-simulator-wk2  Platform: Mac OS X 10.13.6

[Blaze Burg]

Comment on attachment 360812 [details]
Patch

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

r=me Looks good overall. I am not sure it's worth the effort to make a regression test using the draw commands since they are not as easy to hook now. Maybe you can take screenshots of the various highlight modes and save them in a wiki so that myself / others can easily see if there is a visual regression.

> Source/WebCore/inspector/InspectorOverlay.cpp:266
> +                    contentQuad = highlight.quads[3];

This looks weird to me. Make it all aligned and not nested?

> Source/WebCore/inspector/InspectorOverlay.cpp:307
> +    if (paths.shape.length()) {

Please invert this so (!path.shape.length()) is handled first as an early return. It's a much shorter case.

> Source/WebCore/inspector/InspectorOverlay.cpp:308
> +        const auto mapPoints = [&] (const Path& path) {

This lambda would be better named 'convertPathLocalToRoot' or something. (I wasn't familiar with WebCore graphics structs, but this does look pretty neat.)

> Source/WebCore/inspector/InspectorOverlay.cpp:347
> +            context.setFillColor(Color(96, 82, 127, 153));

Please extract this to a named variable so I know what the color is for.

> Source/WebCore/inspector/InspectorOverlay.cpp:358
> +    } else {

(Right, that was so much drawing I forgot what the else is paired up with. ;-))

> Source/WebCore/inspector/InspectorOverlay.cpp:-192
> -#endif

Where did this code go? What does it do..

> Source/WebCore/inspector/InspectorOverlay.cpp:569
> +

Nit: extra newline.

> Source/WebCore/inspector/InspectorOverlay.cpp:574
> +

Nit: extra newline

> Source/WebCore/inspector/InspectorOverlay.cpp:694
> +    // Draw backgrounds

Nit (throughout): WebKit style requires trailing period in general.

[Devin Rousso]

Comment on attachment 360812 [details]
Patch

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

>> Source/WebCore/inspector/InspectorOverlay.cpp:266
>> +                    contentQuad = highlight.quads[3];
> 
> This looks weird to me. Make it all aligned and not nested?

This was mainly to avoid unnecessary work (e.g. if the size is only 1, we don't also need to check if it's at least 3 since we already confirmed it's not at least 2).

>> Source/WebCore/inspector/InspectorOverlay.cpp:-192
>> -#endif
> 
> Where did this code go? What does it do..

As far as I could tell, this code had no effect on the overlay page, as it specified its own colors for everything (e.g. it never used the system appearance).

[Devin Rousso]

Created attachment 361060 [details]
Patch

[Devin Rousso]

Created attachment 361061 [details]
[HTML] Test page for node highlighting

[Devin Rousso]

Created attachment 361062 [details]
[HTML] Test page for rulers and node bounds

[Devin Rousso]

Created attachment 362873 [details]
Patch

[Devin Rousso]

Created attachment 362874 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 362874 [details]
Patch

Clearing flags on attachment: 362874

Committed r242019: <https://trac.webkit.org/changeset/242019>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.