[webkit-reviews] review granted: [Bug 233773] [Cocoa] Web Inspector: Unify Grid overlay drawing code : [Attachment 445756] Patch v1.0

[bugzilla-daemon at webkit.org]

Devin Rousso <drousso at apple.com> has granted Patrick Angle <pangle at apple.com>'s
request for review:
Bug 233773: [Cocoa] Web Inspector: Unify Grid overlay drawing code
https://bugs.webkit.org/show_bug.cgi?id=233773

Attachment 445756: Patch v1.0

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




--- Comment #7 from Devin Rousso <drousso at apple.com> ---
Comment on attachment 445756
  --> https://bugs.webkit.org/attachment.cgi?id=445756
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 :)