Bug 233841 – [iOS] Web content process sometimes crashes under WebPage::positionInformation()

• Status: RESOLVED
• Resolution: FIXED
• Component: Platform

[Wenson Hsieh]

rdar://85917212

[Wenson Hsieh]

Created attachment 445911 [details]
Patch

[Geoffrey Garen]

Comment on attachment 445911 [details]
Patch

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

r=me

> Tools/TestWebKitAPI/Tests/WebKitCocoa/ImageAnalysisTests.mm:87
> +static CGPoint gSwizzledLocationInView = CGPoint { 100, 100 };
>  static CGPoint swizzledLocationInView(id, SEL, UIView *)
>  {
> -    return CGPointMake(100, 100);
> +    return gSwizzledLocationInView;
>  }

Nit:

The most efficient thing to do here is "return CGPoint { 100, 100 }". On all modern hardware, that will just fill two registers with constants and return. Maybe that's the best thing to do.

If we do have a specific motivation to use a constant, I'd suggest putting the static inside the function and marking it const, to reduce its scope / potential for incorrect usage.

[Wenson Hsieh]

(In reply to Geoffrey Garen from comment #2)
> Comment on attachment 445911 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=445911&action=review
> 
> r=me
> 
> > Tools/TestWebKitAPI/Tests/WebKitCocoa/ImageAnalysisTests.mm:87
> > +static CGPoint gSwizzledLocationInView = CGPoint { 100, 100 };
> >  static CGPoint swizzledLocationInView(id, SEL, UIView *)
> >  {
> > -    return CGPointMake(100, 100);
> > +    return gSwizzledLocationInView;
> >  }
> 
> Nit:
> 
> The most efficient thing to do here is "return CGPoint { 100, 100 }". On all
> modern hardware, that will just fill two registers with constants and
> return. Maybe that's the best thing to do.
> 
> If we do have a specific motivation to use a constant, I'd suggest putting
> the static inside the function and marking it const, to reduce its scope /
> potential for incorrect usage.

Ah, so I pulled this out into a variable because I needed to be able to change this to another value (500, 500) for the new test below.

[Geoffrey Garen]

Comment on attachment 445911 [details]
Patch

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

>>> Tools/TestWebKitAPI/Tests/WebKitCocoa/ImageAnalysisTests.mm:87
>>>  }
>> 
>> Nit:
>> 
>> The most efficient thing to do here is "return CGPoint { 100, 100 }". On all modern hardware, that will just fill two registers with constants and return. Maybe that's the best thing to do.
>> 
>> If we do have a specific motivation to use a constant, I'd suggest putting the static inside the function and marking it const, to reduce its scope / potential for incorrect usage.
> 
> Ah, so I pulled this out into a variable because I needed to be able to change this to another value (500, 500) for the new test below.

Oh, lol, I totally missed that!

Seems fine as-is.

[Wenson Hsieh]

Comment on attachment 445911 [details]
Patch

Thanks for the review!

[EWS]

Committed r286522 (244856@main): <https://commits.webkit.org/244856@main>

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