Bug 222270

Created attachment 421197 [details] Patch

Created attachment 421199 [details] Patch Fix changelog nit

Comment on attachment 421197 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=421197&action=review Looking good, just a few changes here and there required. > Source/WebCore/ChangeLog:9 > + https://bugs.webkit.org/show_bug.cgi?id=222270 Duplicate entry > Source/WebCore/ChangeLog:13 > + * Modules/webxr/WebXRFrame.cpp: set WebXRView viewport modifiable value. Nit, the comment is added in the line having the method name not in the line with the source file (unless it's a general comment for the file) > Source/WebCore/Modules/webxr/WebXRSession.cpp:313 > + // Only immersive sessions viewport scaling. Maybe "support" is missing between "sessions" and "viewport" > Source/WebCore/Modules/webxr/WebXRView.cpp:38 > +static constexpr double kMinViewportScale = 0.2; Does it come from specs? If so please add a link for reference. > Source/WebCore/Modules/webxr/WebXRViewport.h:50 > + WebXRViewport(const IntRect&); explicit > Source/WebCore/Modules/webxr/WebXRWebGLLayer.cpp:217 > + // Compute the scaled viewport. From the specs it looks like this computation should be only done if 6. is true, so I guess you have to move it all to the if block above. > Source/WebCore/Modules/webxr/WebXRWebGLLayer.cpp:235 > + else /// XREye::None better write this as else { // XREye::None viewportRect. }

Comment on attachment 421197 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=421197&action=review >> Source/WebCore/ChangeLog:9 >> + https://bugs.webkit.org/show_bug.cgi?id=222270 > > Duplicate entry Fixed >> Source/WebCore/ChangeLog:13 >> + * Modules/webxr/WebXRFrame.cpp: set WebXRView viewport modifiable value. > > Nit, the comment is added in the line having the method name not in the line with the source file (unless it's a general comment for the file) Fixed >> Source/WebCore/Modules/webxr/WebXRSession.cpp:313 >> + // Only immersive sessions viewport scaling. > > Maybe "support" is missing between "sessions" and "viewport" Yes, I wanted to add "support" work in that comment >> Source/WebCore/Modules/webxr/WebXRView.cpp:38 >> +static constexpr double kMinViewportScale = 0.2; > > Does it come from specs? If so please add a link for reference. It's an arbitrary value. I added some comments. Chrome is using around 0.05 now and Gecko around 0.2 IIRC. >> Source/WebCore/Modules/webxr/WebXRViewport.h:50 >> + WebXRViewport(const IntRect&); > > explicit done >> Source/WebCore/Modules/webxr/WebXRWebGLLayer.cpp:217 >> + // Compute the scaled viewport. > > From the specs it looks like this computation should be only done if 6. is true, so I guess you have to move it all to the if block above. The spec only updates the viewports when dirty while the patch was computing them on each getViewport call. I updated the patch to user dirty checking. We also need to handle canvasResize to do it correctly >> Source/WebCore/Modules/webxr/WebXRWebGLLayer.cpp:235 >> + else /// XREye::None > > better write this as > > else { > // XREye::None > viewportRect. > } done

Created attachment 421306 [details] Patch Address review feedback. Only compute the viewports when dirty

Comment on attachment 421306 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=421306&action=review Awesome job, thanks! > Source/WebCore/Modules/webxr/WebXRSession.cpp:313 > + // Only immersive sessions support viewport scaling. We can probably get rid of this comment, the code bellow is self-descriptive (let's do it in a follow up though)

Committed r273381: <https://commits.webkit.org/r273381> All reviewed patches have been landed. Closing bug and clearing flags on attachment 421306 [details].

<rdar://problem/74691035>