Bug 175152

Summary: Improve WebKitLegacy video fullscreen animation begin and end rects.
Product: WebKit Reporter: Jeremy Jones <jeremyj-wk>
Component: MediaAssignee: Jeremy Jones <jeremyj-wk>
Status: RESOLVED FIXED    
Severity: Normal CC: commit-queue, eric.carlson, jer.noble, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Mac   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch for landing. none

Description Jeremy Jones 2017-08-03 13:24:37 PDT 
Improve WebKitLegacy video fullscreen animation begin and end rects.
Comment 1 Jeremy Jones 2017-08-03 13:29:12 PDT 
rdar://problem/32840576
Comment 2 Jeremy Jones 2017-08-03 13:34:40 PDT 
Created attachment 317146 [details]
Patch
Comment 3 Eric Carlson 2017-08-03 13:45:06 PDT 
Comment on attachment 317146 [details]
Patch

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

> Source/WebCore/platform/mac/WebVideoFullscreenController.mm:236
> +static NSRect frameExpandedToRatioOfFrame(NSRect frameToExpand, NSRect frame)

Nit: NSRect&

> Source/WebCore/platform/mac/WebVideoFullscreenController.mm:284
>      NSRect frame = [self videoElementRect];
>      NSRect endFrame = [screen frame];
> -    constrainFrameToRatioOfFrame(&endFrame, &frame);
> +    frame = frameExpandedToRatioOfFrame(frame, endFrame);

Nit: this would be slightly cleaner as something like

NSRect frame = frameExpandedToRatioOfFrame([self videoElementRect], endFrame);
Comment 4 Jeremy Jones 2017-08-03 14:25:36 PDT 
(In reply to Eric Carlson from comment #3)
> Comment on attachment 317146 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=317146&action=review
> 
> > Source/WebCore/platform/mac/WebVideoFullscreenController.mm:236
> > +static NSRect frameExpandedToRatioOfFrame(NSRect frameToExpand, NSRect frame)
> 
> Nit: NSRect&
> 

For small structs the cost of indirection might be higher than the cost of copying.

> > Source/WebCore/platform/mac/WebVideoFullscreenController.mm:284
> >      NSRect frame = [self videoElementRect];
> >      NSRect endFrame = [screen frame];
> > -    constrainFrameToRatioOfFrame(&endFrame, &frame);
> > +    frame = frameExpandedToRatioOfFrame(frame, endFrame);
> 
> Nit: this would be slightly cleaner as something like
> 
> NSRect frame = frameExpandedToRatioOfFrame([self videoElementRect],
> endFrame);

Done.
Comment 5 Jeremy Jones 2017-08-03 15:56:08 PDT 
Created attachment 317174 [details]
Patch for landing.
Comment 6 WebKit Commit Bot 2017-08-03 18:58:36 PDT 
Comment on attachment 317174 [details]
Patch for landing.

Clearing flags on attachment: 317174

Committed r220248: <http://trac.webkit.org/changeset/220248>