1591156 - Fix inLayoutUtils::GetSubDocumentFor API to work with Fission

Status: RESOLVED WONTFIX

(Core :: Layout, task)

Description

[Daniel Holbert [:dholbert]]

(Spinning this off of a code-review observation in https://phabricator.services.mozilla.com/D50168#inline-304775 )

The inLayoutUtils class has the following API, which will probably need some changes in order to keep working with Fission:

.h:

  static mozilla::dom::Document* GetSubDocumentFor(nsINode* aNode);

.cpp:

Document* inLayoutUtils::GetSubDocumentFor(nsINode* aNode) {
  nsCOMPtr<nsIContent> content = do_QueryInterface(aNode);
  if (content) {
    nsCOMPtr<Document> doc = content->GetComposedDoc();
    if (doc) {
      return doc->GetSubDocumentFor(content);
    }
  }

  return nullptr;
}

(Searchfox is having issues right now, which is why I'm not including a searchfox link.)

Comment 1

[Daniel Holbert [:dholbert]]

Searchfox link to this function impl:
https://searchfox.org/mozilla-central/rev/2a355e56c490768be676689de18629485c9d699b/layout/inspector/inLayoutUtils.cpp#31-41

Comment 2

[Chris Peterson [:cpeterson]]

Tentatively moving all bugs whose summaries mention "Fission" (or other Fission-related keywords) but are not assigned to a Fission Milestone to the "?" triage milestone.

This will generate a lot of bugmail, so you can filter your bugmail for the following UUID and delete them en masse:

0ee3c76a-bc79-4eb2-8d12-05dc0b68e732

Comment 3

[Chris Peterson [:cpeterson]]

@ Layout triage: we need a Layout engineer to audit GetSubDocumentFor callers for Fission.

What is actually broken by this bug in Fission? Should this bug block enabling Fission in Nightly (M6 milestone)?

Comment 4

[Chris Peterson [:cpeterson]]

Tentatively tracking for Fission Nightly (M6)

Comment 5

[Daniel Holbert [:dholbert]]

(In reply to Chris Peterson [:cpeterson] from comment #3)

What is actually broken by this bug in Fission?

(emilio, do you know? I filed this bug at your request from https://phabricator.services.mozilla.com/D50168#inline-304775 :) )

Comment 6

[Emilio Cobos Álvarez (:emilio)]

So this is only used by devtools in two places:

• https://searchfox.org/mozilla-central/rev/7fba7adfcd695343236de0c12e8d384c9b7cd237/devtools/server/actors/inspector/document-walker.js#72-73
• https://searchfox.org/mozilla-central/rev/7fba7adfcd695343236de0c12e8d384c9b7cd237/devtools/shared/layout/utils.js#365-366

Julian, do you know whether they're needed and how do we plan to deal with fission iframes in those cases?

Comment 7

[Julian Descottes [:jdescottes]]

I don't know in details how these properties impact the behavior of the walker, but based on the names, I guess it means that the "document" of a frame is returned by the walker as a child "node" of the frame. I think the short answer is that we don't expect the platform walker to work with remote frames, but we still expect it to work for same process frames.

The main inspector use case for document-walker is to use the walker to discover the children of a given node, if we detect that the node is a remote frame, the DevTools client will ask to create a new Walker in the process of the remote frame:
https://searchfox.org/mozilla-central/rev/7fba7adfcd695343236de0c12e8d384c9b7cd237/devtools/client/fronts/walker.js#427-446

So in theory we should never even try to walk the children of remote frames. I don't know if it throws with an explicit exception today, but that would be fine for this use case.

Now for the other call site: https://searchfox.org/mozilla-central/rev/7fba7adfcd695343236de0c12e8d384c9b7cd237/devtools/shared/layout/utils.js#365-366, I think it mostly relates to highlighters and screenshot utilities, which should handle Fission remote frames in their own way as well. This is used in safelyGetContentWindow, which is only used in getFrameContentOffset, which is used in getFrameOffsets and getRect.

I will forward the question to Razvan, who has been working on Fission support for highlighters. I believe we are planning to use new APIs to get offsets for remote frames.

Comment 8

[Julian Descottes [:jdescottes]]

Razvan: Hi! Read the comment above for some more context, my question is about getFrameContentOffset https://searchfox.org/mozilla-central/rev/7fba7adfcd695343236de0c12e8d384c9b7cd237/devtools/shared/layout/utils.js#392 . I know you are working with Brad on some fission friendly APIs for highlighters. Is this one of the methods you are planning to replace/remove?

Comment 9

[Razvan Caliman [:rcaliman]]

I don't think we need any changes to this API specifically for Fission so long as it continues to work as-is for same-process frames.

With the revised highlighters for Fission, we intend to reuse the existing code and spawn multiple highlighters as needed for each out-of-process frame. We reconcile on the DevTools client which highlighter should be visible. If the functionality of GetSubDocumentFor API for same-process frames remains the same, this should be fine for us as highlighters for same-process frames would continue to work.

An alternative approach we considered for highlighters (rendering a single highlighter in the parent process and communicate coordinates to it from all processes) is served by getBoxQuadsFromWindowOrigin() which Brad introduced a while ago in Bug 1593756. This obviates the need for getFrameContentOffset() if/when we ever decide to use this alternative approach. That also means there isn't a requirement on our end for GetSubDocumentFor to work with Fission.

Comment 10

[Sean Voisen (:svoisen)]

Sounds like we don't really need to take any action on this, but I'll leave it open for now and remove any Fission milestones.

Comment 11

[Chris Peterson [:cpeterson]]

Nika says we can WONTFIX this bug because inLayoutUtils::GetSubDocumentFor will never be fixed to work cross-process.

If someone wants a new Fission-compatible alternative to inLayoutUtils::GetSubDocumentFor API, please file a new bug.