Bug 105023 - Web Inspector: Change the InspectorOverlay to use native rather than canvas
Summary: Web Inspector: Change the InspectorOverlay to use native rather than canvas
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Devin Rousso
URL:
Keywords: InRadar
Depends on:
Blocks: 195400
  Show dependency treegraph
 
Reported: 2012-12-14 07:18 PST by Konrad Piascik
Modified: 2019-03-06 23:24 PST (History)
21 users (show)

See Also:


Attachments
[Patch] WIP (4.96 KB, patch)
2018-12-20 15:47 PST, Devin Rousso
no flags Details | Formatted Diff | Diff
Patch (794.81 KB, patch)
2018-12-30 14:32 PST, Devin Rousso
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews102 for mac-sierra (3.54 MB, application/zip)
2018-12-30 15:41 PST, EWS Watchlist
no flags Details
Archive of layout-test-results from ews113 for mac-sierra (3.09 MB, application/zip)
2018-12-30 16:32 PST, EWS Watchlist
no flags Details
Patch (636.58 KB, patch)
2018-12-31 17:56 PST, Devin Rousso
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews104 for mac-sierra-wk2 (4.23 MB, application/zip)
2018-12-31 19:16 PST, EWS Watchlist
no flags Details
Archive of layout-test-results from ews101 for mac-sierra (3.47 MB, application/zip)
2018-12-31 19:23 PST, EWS Watchlist
no flags Details
Archive of layout-test-results from ews112 for mac-sierra (3.01 MB, application/zip)
2018-12-31 20:08 PST, EWS Watchlist
no flags Details
Patch (635.07 KB, patch)
2018-12-31 20:47 PST, Devin Rousso
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews106 for mac-sierra-wk2 (4.31 MB, application/zip)
2018-12-31 22:10 PST, EWS Watchlist
no flags Details
Patch (631.79 KB, patch)
2019-01-02 15:19 PST, Devin Rousso
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews102 for mac-sierra (2.76 MB, application/zip)
2019-01-02 16:15 PST, EWS Watchlist
no flags Details
Archive of layout-test-results from ews107 for mac-sierra-wk2 (4.68 MB, application/zip)
2019-01-02 16:39 PST, EWS Watchlist
no flags Details
Archive of layout-test-results from ews114 for mac-sierra (2.30 MB, application/zip)
2019-01-02 17:13 PST, EWS Watchlist
no flags Details
Patch (192.76 KB, patch)
2019-01-31 17:18 PST, Devin Rousso
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews126 for ios-simulator-wk2 (2.56 MB, application/zip)
2019-01-31 19:22 PST, EWS Watchlist
no flags Details
Patch (194.08 KB, patch)
2019-02-04 09:32 PST, Devin Rousso
no flags Details | Formatted Diff | Diff
[HTML] Test page for node highlighting (3.23 KB, text/html)
2019-02-04 09:33 PST, Devin Rousso
no flags Details
[HTML] Test page for rulers and node bounds (40.20 KB, text/html)
2019-02-04 09:33 PST, Devin Rousso
no flags Details
Patch (238.80 KB, patch)
2019-02-24 19:15 PST, Devin Rousso
no flags Details | Formatted Diff | Diff
Patch (236.37 KB, patch)
2019-02-24 19:18 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 Konrad Piascik 2012-12-14 07:18:40 PST
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.
Comment 1 BJ Burg 2014-02-08 19:02:27 PST
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?
Comment 2 Timothy Hatcher 2014-02-11 16:34:20 PST
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.
Comment 3 Radar WebKit Bug Importer 2014-12-17 11:19:58 PST
<rdar://problem/19281432>
Comment 4 Brian Burg 2015-01-20 09:58:33 PST
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).
Comment 5 BJ Burg 2018-08-23 13:49:11 PDT
<rdar://problem/13443692>
Comment 6 Devin Rousso 2018-12-20 15:47:17 PST
Created attachment 357882 [details]
[Patch] WIP
Comment 7 Devin Rousso 2018-12-20 15:48:35 PST
<rdar://problem/19281432>
Comment 8 Devin Rousso 2018-12-20 15:50:41 PST
<rdar://problem/13443692>
Comment 9 Devin Rousso 2018-12-30 14:32:11 PST
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 :)
Comment 10 EWS Watchlist 2018-12-30 15:41:15 PST Comment hidden (obsolete)
Comment 11 EWS Watchlist 2018-12-30 15:41:17 PST Comment hidden (obsolete)
Comment 12 EWS Watchlist 2018-12-30 16:32:45 PST Comment hidden (obsolete)
Comment 13 EWS Watchlist 2018-12-30 16:32:47 PST Comment hidden (obsolete)
Comment 14 Devin Rousso 2018-12-31 17:56:43 PST
Created attachment 358158 [details]
Patch
Comment 15 EWS Watchlist 2018-12-31 19:16:45 PST Comment hidden (obsolete)
Comment 16 EWS Watchlist 2018-12-31 19:16:47 PST Comment hidden (obsolete)
Comment 17 EWS Watchlist 2018-12-31 19:23:33 PST Comment hidden (obsolete)
Comment 18 EWS Watchlist 2018-12-31 19:23:35 PST Comment hidden (obsolete)
Comment 19 EWS Watchlist 2018-12-31 20:08:43 PST Comment hidden (obsolete)
Comment 20 EWS Watchlist 2018-12-31 20:08:45 PST Comment hidden (obsolete)
Comment 21 Devin Rousso 2018-12-31 20:47:50 PST
Created attachment 358163 [details]
Patch
Comment 22 EWS Watchlist 2018-12-31 22:09:58 PST Comment hidden (obsolete)
Comment 23 EWS Watchlist 2018-12-31 22:10:00 PST Comment hidden (obsolete)
Comment 24 Simon Fraser (smfr) 2019-01-02 12:10:20 PST
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.
Comment 25 Devin Rousso 2019-01-02 15:10:43 PST
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).
Comment 26 Devin Rousso 2019-01-02 15:19:03 PST
Created attachment 358215 [details]
Patch
Comment 27 Simon Fraser (smfr) 2019-01-02 15:19:08 PST
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?
Comment 28 Simon Fraser (smfr) 2019-01-02 15:25:26 PST
(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.
Comment 29 Devin Rousso 2019-01-02 15:29:07 PST
(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).
Comment 30 Simon Fraser (smfr) 2019-01-02 15:35:45 PST
How will we make the inspector overlay accessible?
Comment 31 Nikita Vasilyev 2019-01-02 15:41:34 PST
(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?
Comment 32 Devin Rousso 2019-01-02 15:48:05 PST
(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.
Comment 33 Simon Fraser (smfr) 2019-01-02 15:51:22 PST
(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.
Comment 34 EWS Watchlist 2019-01-02 16:15:21 PST Comment hidden (obsolete)
Comment 35 EWS Watchlist 2019-01-02 16:15:23 PST Comment hidden (obsolete)
Comment 36 EWS Watchlist 2019-01-02 16:39:18 PST Comment hidden (obsolete)
Comment 37 EWS Watchlist 2019-01-02 16:39:21 PST Comment hidden (obsolete)
Comment 38 EWS Watchlist 2019-01-02 17:13:07 PST Comment hidden (obsolete)
Comment 39 EWS Watchlist 2019-01-02 17:13:09 PST Comment hidden (obsolete)
Comment 40 Devin Rousso 2019-01-02 18:39:53 PST
(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).
Comment 41 Tim Horton 2019-01-02 23:27:42 PST
(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.
Comment 42 Devin Rousso 2019-01-31 17:18:42 PST
Created attachment 360812 [details]
Patch
Comment 43 Devin Rousso 2019-01-31 17:47:38 PST
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
Comment 44 Devin Rousso 2019-01-31 17:48:39 PST
(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...
Comment 45 EWS Watchlist 2019-01-31 19:22:18 PST Comment hidden (obsolete)
Comment 46 EWS Watchlist 2019-01-31 19:22:20 PST Comment hidden (obsolete)
Comment 47 BJ Burg 2019-02-01 14:18:04 PST
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.
Comment 48 Devin Rousso 2019-02-01 15:56:58 PST
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).
Comment 49 Devin Rousso 2019-02-04 09:32:22 PST
Created attachment 361060 [details]
Patch
Comment 50 Devin Rousso 2019-02-04 09:33:13 PST
Created attachment 361061 [details]
[HTML] Test page for node highlighting
Comment 51 Devin Rousso 2019-02-04 09:33:32 PST
Created attachment 361062 [details]
[HTML] Test page for rulers and node bounds
Comment 52 Devin Rousso 2019-02-24 19:15:18 PST
Created attachment 362873 [details]
Patch
Comment 53 Devin Rousso 2019-02-24 19:18:11 PST
Created attachment 362874 [details]
Patch
Comment 54 WebKit Commit Bot 2019-02-24 20:42:00 PST
Comment on attachment 362874 [details]
Patch

Clearing flags on attachment: 362874

Committed r242019: <https://trac.webkit.org/changeset/242019>
Comment 55 WebKit Commit Bot 2019-02-24 20:42:02 PST
All reviewed patches have been landed.  Closing bug.