Bug 233773 - [Cocoa] Web Inspector: Unify Grid overlay drawing code
Summary: [Cocoa] Web Inspector: Unify Grid overlay drawing code
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: Patrick Angle
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2021-12-02 10:54 PST by Patrick Angle
Modified: 2021-12-06 22:21 PST (History)
6 users (show)

See Also:


Attachments
Patch v1.0 (23.72 KB, patch)
2021-12-02 11:28 PST, Patrick Angle
no flags Details | Formatted Diff | Diff
Patch v1.1 - Fix Debug builds, Update changelog (24.24 KB, patch)
2021-12-06 17:14 PST, Patrick Angle
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Patrick Angle 2021-12-02 10:54:06 PST
.
Comment 1 Radar WebKit Bug Importer 2021-12-02 10:54:59 PST
<rdar://problem/85979433>
Comment 2 Patrick Angle 2021-12-02 11:28:55 PST
Created attachment 445756 [details]
Patch v1.0
Comment 3 Devin Rousso 2021-12-02 12:07:46 PST
Comment on attachment 445756 [details]
Patch v1.0

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

> Source/WebKit/UIProcess/Inspector/ios/WKInspectorHighlightView.mm:43
> +    self.opaque = NO;

Is this intentional?  If so, why?

> Source/WebKit/UIProcess/Inspector/ios/WKInspectorHighlightView.mm:274
> +    auto context = WebCore::GraphicsContextCG(UIGraphicsGetCurrentContext());

This seems possibly expensive (e.g. retain count churn).  How often is this likely to be called?  Is there any way we can avoid doing this on every `-drawRect:`?
Comment 4 Patrick Angle 2021-12-02 12:20:25 PST
Comment on attachment 445756 [details]
Patch v1.0

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

>> Source/WebKit/UIProcess/Inspector/ios/WKInspectorHighlightView.mm:43
>> +    self.opaque = NO;
> 
> Is this intentional?  If so, why?

Because we now set an actual frame for this view and views default to being opaque - Without this the background of the view is rendered solid black which prevents seeing the actual page underneath.

>> Source/WebKit/UIProcess/Inspector/ios/WKInspectorHighlightView.mm:274
>> +    auto context = WebCore::GraphicsContextCG(UIGraphicsGetCurrentContext());
> 
> This seems possibly expensive (e.g. retain count churn).  How often is this likely to be called?  Is there any way we can avoid doing this on every `-drawRect:`?

This gets called on every frame we draw. It doesn't seem to be possible to avoid this, given this comment in the `-drawRect:` documentation:
```
You can get a reference to the graphics context using the UIGraphicsGetCurrentContext function, but do not establish a strong reference to the graphics context because it can change between calls to the drawRect: method.
```
(https://developer.apple.com/documentation/uikit/uiview/1622529-drawrect)

One thing I can think to try is seeing if we can hold onto the same GraphicsContextCG so long as the underlying CGContextRef remains the same between calls to `-drawRect:`, but that feels a bit sketchy given the above.
Comment 5 Patrick Angle 2021-12-02 14:48:35 PST
Comment on attachment 445756 [details]
Patch v1.0

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

>>> Source/WebKit/UIProcess/Inspector/ios/WKInspectorHighlightView.mm:274
>>> +    auto context = WebCore::GraphicsContextCG(UIGraphicsGetCurrentContext());
>> 
>> This seems possibly expensive (e.g. retain count churn).  How often is this likely to be called?  Is there any way we can avoid doing this on every `-drawRect:`?
> 
> This gets called on every frame we draw. It doesn't seem to be possible to avoid this, given this comment in the `-drawRect:` documentation:
> ```
> You can get a reference to the graphics context using the UIGraphicsGetCurrentContext function, but do not establish a strong reference to the graphics context because it can change between calls to the drawRect: method.
> ```
> (https://developer.apple.com/documentation/uikit/uiview/1622529-drawrect)
> 
> One thing I can think to try is seeing if we can hold onto the same GraphicsContextCG so long as the underlying CGContextRef remains the same between calls to `-drawRect:`, but that feels a bit sketchy given the above.

I just checked and unfortunately the `CGContextRef` is different on every invocation, so caching the `GraphicsContextCG` doesn't really win us anything.
Comment 6 Patrick Angle 2021-12-06 09:39:22 PST
Comment on attachment 445756 [details]
Patch v1.0

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

>>>> Source/WebKit/UIProcess/Inspector/ios/WKInspectorHighlightView.mm:274
>>>> +    auto context = WebCore::GraphicsContextCG(UIGraphicsGetCurrentContext());
>>> 
>>> This seems possibly expensive (e.g. retain count churn).  How often is this likely to be called?  Is there any way we can avoid doing this on every `-drawRect:`?
>> 
>> This gets called on every frame we draw. It doesn't seem to be possible to avoid this, given this comment in the `-drawRect:` documentation:
>> ```
>> You can get a reference to the graphics context using the UIGraphicsGetCurrentContext function, but do not establish a strong reference to the graphics context because it can change between calls to the drawRect: method.
>> ```
>> (https://developer.apple.com/documentation/uikit/uiview/1622529-drawrect)
>> 
>> One thing I can think to try is seeing if we can hold onto the same GraphicsContextCG so long as the underlying CGContextRef remains the same between calls to `-drawRect:`, but that feels a bit sketchy given the above.
> 
> I just checked and unfortunately the `CGContextRef` is different on every invocation, so caching the `GraphicsContextCG` doesn't really win us anything.

I should clarify that it gets called on every frame we need to update our drawing - Save for the possible redraw when switching away and back to the app, this should only really happen as a result of the `[self setNeedsDisplay]` call in `update:scale:frame:`. Additionally, the retain count churn/object churn in general here should be much less for any case with at least one grid, since previously every call to `update:scale:` would remove and forget about all current layers and rebuild all the layers from scratch. We could (and probably should) hit parity with the previous no-grid case and check for some number of grid highlights before creating a context to avoid this work in the no-grid case.
Comment 7 Devin Rousso 2021-12-06 11:58:35 PST
Comment on attachment 445756 [details]
Patch v1.0

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

r=mews, love it =D

>>> Source/WebKit/UIProcess/Inspector/ios/WKInspectorHighlightView.mm:43
>>> +    self.opaque = NO;
>> 
>> Is this intentional?  If so, why?
> 
> Because we now set an actual frame for this view and views default to being opaque - Without this the background of the view is rendered solid black which prevents seeing the actual page underneath.

Can you mention this somewhere in the ChangeLog so that if someone comes across this in the future it's clear? :)

>>>>> Source/WebKit/UIProcess/Inspector/ios/WKInspectorHighlightView.mm:274
>>>>> +    auto context = WebCore::GraphicsContextCG(UIGraphicsGetCurrentContext());
>>>> 
>>>> This seems possibly expensive (e.g. retain count churn).  How often is this likely to be called?  Is there any way we can avoid doing this on every `-drawRect:`?
>>> 
>>> This gets called on every frame we draw. It doesn't seem to be possible to avoid this, given this comment in the `-drawRect:` documentation:
>>> ```
>>> You can get a reference to the graphics context using the UIGraphicsGetCurrentContext function, but do not establish a strong reference to the graphics context because it can change between calls to the drawRect: method.
>>> ```
>>> (https://developer.apple.com/documentation/uikit/uiview/1622529-drawrect)
>>> 
>>> One thing I can think to try is seeing if we can hold onto the same GraphicsContextCG so long as the underlying CGContextRef remains the same between calls to `-drawRect:`, but that feels a bit sketchy given the above.
>> 
>> I just checked and unfortunately the `CGContextRef` is different on every invocation, so caching the `GraphicsContextCG` doesn't really win us anything.
> 
> I should clarify that it gets called on every frame we need to update our drawing - Save for the possible redraw when switching away and back to the app, this should only really happen as a result of the `[self setNeedsDisplay]` call in `update:scale:frame:`. Additionally, the retain count churn/object churn in general here should be much less for any case with at least one grid, since previously every call to `update:scale:` would remove and forget about all current layers and rebuild all the layers from scratch. We could (and probably should) hit parity with the previous no-grid case and check for some number of grid highlights before creating a context to avoid this work in the no-grid case.

Ah!  If it's only when we `setNeedsDisplay` (and other rare circumstances) then that's fine :)
Comment 8 Patrick Angle 2021-12-06 17:14:15 PST
Created attachment 446104 [details]
Patch v1.1 - Fix Debug builds, Update changelog
Comment 9 EWS 2021-12-06 22:20:59 PST
Committed r286588 (?): <https://commits.webkit.org/r286588>

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