Bug 233773 – [Cocoa] Web Inspector: Unify Grid overlay drawing code

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Inspector

[Patrick Angle]

.

[Radar WebKit Bug Importer]

<rdar://problem/85979433>

[Patrick Angle]

Created attachment 445756 [details]
Patch v1.0

[Devin Rousso]

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:`?

[Patrick Angle]

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.

[Patrick Angle]

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.

[Patrick Angle]

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.

[Devin Rousso]

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 :)

[Patrick Angle]

Created attachment 446104 [details]
Patch v1.1 - Fix Debug builds, Update changelog

[EWS]

Committed r286588 (?): <https://commits.webkit.org/r286588>

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