1729794 - audit Get(Cumulative)Resolution callers for fission

Status: RESOLVED FIXED

(Core :: Panning and Zooming, task)

Description

[Timothy Nikkel (:tnikkel)]

There was an earlier audit related to this in bug 1518908 but there are a few deeper issues I noticed so I want to go over these in more detail.

Comment 1

[Timothy Nikkel (:tnikkel)]

dom/events/ContentEventHandler.cpp
-used for things like macOS "lookup" a word to highlight it, probably would be in wrong place if done on a word in an oop iframe while pinched zoomed, which is a rare combination. Not sure what else this code is used for. Low priority to fix for now.

layout/base/AccessibleCaret.cpp
accessible/generic/DocAccessible.cpp
accessible/generic/HyperTextAccessible.cpp
accessible/generic/LocalAccessible.cpp
-I don't know much about this, in contact accessibility team about these

layout/base/DisplayPortUtils.cpp
-either correct as is or fixed by bug 1732600 (displayport/apz related code)

layout/base/PresShell.cpp
-all of these are the basic code to set/get resolution except PresShell::CreateRangePaintInfo which was specifically made to work with fission in bug 1712400

layout/base/nsLayoutUtils.cpp
-either correct as fixed by bug 1732600 (displayport/apz related code)

layout/generic/nsGfxScrollFrame.cpp
-ScrollFrameHelper::GetAvailableScrollingDirectionsForUserInputEvents resolution used to determine if scroll frame has at least one half pixel to scroll-> low priority to fix for now.
-nsHTMLScrollFrame::TryLayout, code inside an if checking presShell->GetMobileViewportManager() which is only true for root content documents cross process
-GetPaintedLayerScaleForFrame, fix in bug 1732660
-ScrollFrameHelper::RestrictToRootDisplayPort, use is inside if checking IsRootContentDocumentCrossProcess, okay.
-ScrollFrameHelper::DecideScrollableLayer, code is correct, if local resolution is 1 then nothing to do, okay.
-ScrollFrameHelper::SaveState code to just save/restore resolution, okay.

layout/generic/nsImageFrame.cpp
-correct (bug 1724808 fixed it)

dom/base/Document.cpp
-just used to get and restore resolution, seems fine.

dom/base/VisualViewport.cpp
-VisualViewport::Scale other browsers match our behaviour of returning 1 for subdocuments, okay.

dom/base/nsDOMWindowUtils.cpp
-used in privileged test code only, okay.

dom/html/ImageDocument.cpp
-I think the use here only matters for top level image documents -> okay.

gfx/layers/apz/util/APZCCallbackHelper.cpp
-correct

gfx/layers/wr/StackingContextHelper.cpp
-correct

layout/base/GeckoMVMContext.cpp
-these only get created for root content documents cross process, okay.

layout/base/PositionedEventTargeting.cpp
-used only for android -> look at for fission android, okay for now.

layout/generic/nsSubDocumentFrame.cpp
-correct

layout/base/ViewportUtils.cpp
-ConvertToScreenRelativeVisual, this function has fission specific code (TryInferEnclosingResolution) so likely already audited for fission, okay.
-ViewportUtils::GetVisualToLayoutTransform, this function converts from visual to layout coords, so if the enclosing presshell is not the root content document then we don't need to access to the resolution. botond can you confirm?

layout/painting/nsDisplayList.cpp
-nsDisplayStickyPosition::UpdateScrollData only used for root content document cross process -> correct.
-nsDisplayTransform::ShouldPrerenderTransformedContent, hiro could you comment?

Comment 2

[Hiroyuki Ikezoe (:hiro)]

(In reply to Timothy Nikkel (:tnikkel) from comment #1)

-nsDisplayTransform::ShouldPrerenderTransformedContent, hiro could you comment?

Filed bug 1732800. I didn't make the bug to block this since the partial pre-render feature hasn't been enabled on beta/release channels.

Comment 3

[Timothy Nikkel (:tnikkel)]

(In reply to Timothy Nikkel (:tnikkel) from comment #1)

dom/events/ContentEventHandler.cpp
-used for things like macOS "lookup" a word to highlight it, probably would be in wrong place if done on a word in an oop iframe while pinched zoomed, which is a rare combination. Not sure what else this code is used for. Low priority to fix for now.

IME seems to be fine, works in all combinations of oop iframe, css transforms, pinch zoom.

macOS "lookup" is broken with fission: the position is off (that part is not related to Get(Cumulative)Resolution), and the size of the text is off (that part is related to Get(Cumulative)Resolution). I have patches for this.

Comment 4

[Botond Ballo [:botond]]

(In reply to Timothy Nikkel (:tnikkel) from comment #1)

-ViewportUtils::GetVisualToLayoutTransform, this function converts from visual to layout coords, so if the enclosing presshell is not the root content document then we don't need to access to the resolution. botond can you confirm?

Yep, in nested content processes the incoming visual coords should have already been through the parent-to-child conversion matrix, which will include any resolution in the root content processes.

Comment 5

[Timothy Nikkel (:tnikkel)]

Now that the audit is complete and we know the extent of the issues, I don't think any of the issues we've found are severe enough to block fission mvp (all of the issues that I think need to be fixed have patches that are almost ready land anyways), so I don't think this bug needs to block mvp.

Comment 6

[Chris Peterson [:cpeterson]]

(In reply to Timothy Nikkel (:tnikkel) from comment #5)

Now that the audit is complete and we know the extent of the issues, I don't think any of the issues we've found are severe enough to block fission mvp (all of the issues that I think need to be fixed have patches that are almost ready land anyways), so I don't think this bug needs to block mvp.

I think we can close this bug because the audit task itself is complete. Of the bugs filed, the two open bugs (bug 1732812 and bug 1733509) are already tracked for the Fission Future backlog (though it looks like those bugs have not be triaged by the Layout or APZ team because the bugs have no priority or severity set).