Closed Bug 1591120 Opened 5 years ago Closed 3 years ago

nsIContentViewer::emulatePrefersColorScheme should be forward to children remote iframes

Categories

(Core :: Layout, enhancement)

Product:
Core
Component:
Layout
Type:
enhancement

Tracking

()

Status:
RESOLVED FIXED
Milestone:
87 Branch
Project Flags:
Fission Milestone M7
Tracking Flags:
Tracking Status
firefox87 --- fixed

People

(Reporter: ochameau, Assigned: emilio)

References

(Depends on 1 open bug)

Details

(Whiteboard: dt-fission-m3-mvp)

Attachments

(1 file)

Bug 1591120 - Move print and color-scheme simulation to browsingContext. r=ochameau!,nika!
48 bytes, text/x-phabricator-request
Details | Review

DevTools starts using this attribute in bug 1550804 and would expect it to be applied to all the iframes, including the remote ones.

According to Nika: "That looks like something which could be a BrowsingContext field and be automatically propagated".

I can only point to the existing implementation:
https://searchfox.org/mozilla-central/rev/088e2cf29c59d733d57af43903eb0267dbf72e2a/layout/base/nsDocumentViewer.cpp#2965-2999

As I have no clue on how to convert this nsIContentViewer attribute to become a BrowsingContext field.

Assignee: nobody → bhackett1024

So, after reviewing the code and talking with Nika, the BrowsingContext fields referred to here are a set of fields defined in BrowsingContextFieldList.h. These are mutable fields of the C++ BrowsingContext object which, when written, propagate those updates to the BrowsingContexts in other processes (every process has its own local copy of the BrowsingContext tree). I don't think using these fields would help us all that much, for reasons that I'll outline below.

What I think we need is an easy-to-use mechanism for applying changes to all docshells (and associated data, like documents, content viewers etc.) which are being covered (i.e. debugged) by a given toolbox. I've had to deal with this before, in setting the watchedByDevtools flag on all these docshells. I added this flag in bug 1546736, but it's broken, and I've had to write fixes in part 4 of bug 1573327 and in bug 1591743. The emulatePrefersColorScheme setting seems to be broken in different yet similar ways: even without the fission.autostart preference on, if I change the preferred color scheme and trigger creation of a new iframe on the page, the preferred color scheme is not applied to that new iframe. We need to get this right, and need to do it in a reusable way.

What I think we should do is have a docshell configuration (a JSON object) which is part of a toolbox and is applied to all docshells covered by that toolbox. The client can make changes to the configuration, and whenever a given docshell's configuration changes --- it is added or removed from the covered list for a toolbox, or the toolbox's docshell configuration changes --- then the server code for the target managing that docshell invokes a function which updates the docshell from its old to its new configuration. We might also want to invoke this update function when certain data inside the docshell changes, like the document or content viewer, but I don't have a good example of the need for that right now (it seems like the content viewer for a docshell can change and that the emulatePrefersColorScheme setting won't respond to that, but I don't know this stuff well enough to say for sure). This would also ease coordination between different toolboxes operating on the same content, as with both watchedByDevtools and emulatePrefersColorScheme it is easy for toolboxes to overwrite each others' settings.

Using an automatically-propagated field in the BrowsingContext would only take care of part of this: migrating changes from the parent's BrowsingContext to the relevant child process' BrowsingContext. This could easily be done with a client->server RDP message instead, i.e. target.updateDocshellConfiguration(json). The main issue why I think the BrowsingContext fields are inappropriate, though, is that they are only accessible from C++. Any field we wanted to expose to JS would need a fair amount of glue so that JS could access the field and respond to changes to the field in content processes. If we use RDP we can do everything in JS, which will help with maintenance and accessibility for devtools developers.

I'd like to work on this but I'm pretty wary. I've worked on some bugs lately that involve the toolbox targets (bug 1573327, bug 1586539), and getting these reviewed has been an awful experience. I'd like some assurance that I won't be wasting my time here.

Assignee: bhackett1024 → nobody

The emulatePrefersColorScheme setting seems to be broken in different yet similar ways: even without the fission.autostart preference on, if I change the preferred color scheme and trigger creation of a new iframe on the page, the preferred color scheme is not applied to that new iframe.

FWIW this was known when I wrote it, and print emulation has the same issue, afaict.

We need to get this right, and need to do it in a reusable way.

Yes, I agree. It felt a bit out of scope to fix it when I was adding the media emulation stuff, fwiw.

Using an automatically-propagated field in the BrowsingContext would only take care of part of this: migrating changes from the parent's BrowsingContext to the relevant child process' BrowsingContext. This could easily be done with a client->server RDP message instead, i.e. target.updateDocshellConfiguration(json). The main issue why I think the BrowsingContext fields are inappropriate, though, is that they are only accessible from C++. Any field we wanted to expose to JS would need a fair amount of glue so that JS could access the field and respond to changes to the field in content processes. If we use RDP we can do everything in JS, which will help with maintenance and accessibility for devtools developers.

I agree with this in principle. That being said, one question: I don't know what other kind of stuff devtools needs to propagate like this, but at least for media emulation it'd be nice if the pres context was created with the right value to begin with, to avoid a flash of "unemulated" content.

Probably not impossible to do just in chrome js, but maybe worth thinking about?

First thing first: there is no immediate urge to fix this in DevTools. Setting a Color Scheme from DevTools is a brand new feature, and not the most important one to address for fission frames.

So, I would rather like to take some time to maximize collaboration between all teams: DevTools, DOM/Fission and Layout so that we all benefit from each others.

i.e. I'm not looking forward a DevTools-only workaround to fix the DevTools right away but only the DevTools.

I'm eager to learn about what is planned around all attributes that DevTools is interacting with, outside of just DevTools immediate goals.

Here, we are talking about:

  • nsIContentViewer::emulatePrefersColorScheme()

DevTools is also setting:

  • nsIContentViewer::overrideDPPX
  • nsIContentViewer::emulateMedium()
  • nsIDocShell::touchEventsOverride
  • nsIDocShell::metaViewportOverride
  • nsIDocShell::allowJavascript
  • nsIWindowUtils::paintFlashing
  • nsIWindowUtils::serviceWorkersTestingEnabled (enable SW from http instead of https)
  • nsIDocShell::defaultLoadFlags
  • nsIDocShell::cssErrorReportingEnabled

I looked for docshell, contentViewer and windowUtils. Would there be any other typical class to look for?

(In reply to Emilio Cobos Álvarez (:emilio) from comment #2)

FWIW this was known when I wrote it, and print emulation has the same issue, afaict.

Is there a bug already opened about that? I imagine we might fix it if we happen to use synced BrowsingContext attributes.

I agree with this in principle. That being said, one question: I don't know what other kind of stuff devtools needs to propagate like this, but at least for media emulation it'd be nice if the pres context was created with the right value to begin with, to avoid a flash of "unemulated" content.

+1.
I imagine we will have to make a few attributes on nsIDocShell, nsIContentViewer and nsIWindowUtils Fission compatible.
I mean, outside of DevTools scope. When the DevTools are off, we would expect the color-scheme to be correctly applied to all iframes, including the remote one trigerred by Fission. And I imagine similar pattern would apply to a few other atributes.

For example, nsIDocShell::allowJavascript is used by GeckoView. I imagine we will end up enabling Fission in GeckoView at some point, and, we would expect this attribute to have an impact on all frames, including the remote iframes.
So we would have to do something anyway.

The question then, is what do we do?
It may depend on each Field:

  • I've been told by Nika that some fields should not be forward to remote iframes because of privacy reasons.
    An example is nsIPrincipal, which contains URLs. But the current list of attribute I gather do not seem to surface private data.
  • Then, there is the actual location of the attribute in the BrowsingContext classes. There isn't only BrowsingContext. There will also be a WindowContext class, which is still underway in bug 1583863. If an attribute is per-window, we may have to wait for this work to be done.
  • If it is per browsing context, then we could use the synced BrowsingContext attribute code which is already in place.

Nika also gave me a few pointers to understand these synced attributes:

Emilio, I was wondering if you were having any goal around Fission, or if anyone in your team is having some, and would be worth including in this discussion?

(In reply to Brian Hackett (:bhackett) from comment #1)

The main issue why I think the BrowsingContext fields are inappropriate, though, is that they are only accessible from C++. Any field we wanted to expose to JS would need a fair amount of glue so that JS could access the field and respond to changes to the field in content processes.

I also questioned Nika about exposing BrowsingContext attributes to JS:

Making JS able to access a field in most cases should be as simple as adding the field to the webidl file, and perhaps adding some overloads to BrowsingContext.h to make it visible

(In reply to Alexandre Poirot [:ochameau] from comment #3)

Here, we are talking about:

  • nsIContentViewer::emulatePrefersColorScheme()

DevTools is also setting:
[snip]

I looked for docshell, contentViewer and windowUtils. Would there be any other typical class to look for?

Those look about right to me.

I imagine we will have to make a few attributes on nsIDocShell, nsIContentViewer and nsIWindowUtils Fission compatible.
I mean, outside of DevTools scope. When the DevTools are off, we would expect the color-scheme to be correctly applied to all iframes, including the remote one trigerred by Fission. And I imagine similar pattern would apply to a few other atributes.

For color-scheme that already happens correctly. The main difference here is that the system value is global, but the devtools value is scoped per docshell. So when the value changes we only need to forward it to all live processes, it's a somewhat simpler mechanism.

The only similar thing that layout needs to deal with, I think, is printing. I don't know what the plan is for Fission printing, but Jonathan Watt would know. This is, again, somewhat different from devtools in the sense that devtools applies the printing state to a "live" document, while print preview clones the documents and may be able to send initialization data, plus knows that the value won't change for the lifetime of a document.

So not sure about the service-workers and such, but the rest of them seem pretty devtools-specific, and we should do something that makes sense for devtools IMO. Moving these to BrowsingContext from nsIContentViewer may make sense, for most of these boolean and emulation flags as long as we get notified properly for changes to those fields in the content processes.

  • Then, there is the actual location of the attribute in the BrowsingContext classes. There isn't only BrowsingContext. There will also be a WindowContext class, which is still underway in bug 1583863. If an attribute is per-window, we may have to wait for this work to be done.
  • If it is per browsing context, then we could use the synced BrowsingContext attribute code which is already in place.

All these attributes right now work per browsing context, right? It may be tricky to deal with subresource documents and all these, too...

Nika also gave me a few pointers to understand these synced attributes:
[snip]

That looks pretty neat.

Emilio, I was wondering if you were having any goal around Fission, or if anyone in your team is having some, and would be worth including in this discussion?

Jwatt is more involved with Fission, afaik. I don't personally have, but I'm always happy to help :)

(In reply to Brian Hackett (:bhackett) from comment #1)

The main issue why I think the BrowsingContext fields are inappropriate, though, is that they are only accessible from C++. Any field we wanted to expose to JS would need a fair amount of glue so that JS could access the field and respond to changes to the field in content processes.

I also questioned Nika about exposing BrowsingContext attributes to JS:

Making JS able to access a field in most cases should be as simple as adding the field to the webidl file, and perhaps adding some overloads to BrowsingContext.h to make it visible

If exposing them to JS is simple, then they seem the most obvious way to go about this, I'd think.

Jonathan, I would appreciate your feedback here. You may find extensive context in comment 3 and 4.

It relates to my question during the last DOM Fission meeting on how to proceed regarding a few attributes of DocShell/ContentViewer/WindowUtils:
Should we migrate them to BrowsingContext? Or rather workaround via DevTools actors (which is kinda equivalent to working around via a JS Window Actor to propagate the state across remote frames)?

It looks like there is an agreement on migrating them to BrowsingContext. Do you agree?
If yes:

  • do you have any plan to do that for any of these attributes? If not, would that fix anything for you/your team/firefox?
  • would you do that for some other similar attributes of these classes?
  • do we need any coordination? Anything we should know which may help us do that? Nika's pointers highlighted in comment 3 seems to suggest it would be easy to do.
Flags: needinfo?(jwatt)
See Also: → 1620966

Tracking for Fission Nightly (M6)

Fission Milestone: --- → M6
Component: DOM: Core & HTML → Layout

Hi Alexandre: I'm following up on some of these older Fission bugs. Per comment 8, are you still waiting on a response here, or have you since gone with DevTools actors?

Severity: normal → N/A
Fission Milestone: M6 → M6c
Flags: needinfo?(poirot.alex)

Sean, nothing happened. So that features relying on these attributes are flaky when having remoted fission iframes.
I only converted one similar flag, this time very specific to DevTools, watchedByDevTools, via bug 1620966.
I moved this from DocShell to BrowsingContext.

I ended up opening bugs for each of these attributes so that we can track this work:

  • nsIContentViewer::emulatePrefersColorScheme (this bug)
  • nsIContentViewer::overrideDPPX (bug 1655959)
  • nsIContentViewer::emulateMedium() (bug 1655960)
  • nsIDocShell::touchEventsOverride (bug 1655962)
  • nsIDocShell::metaViewportOverride (bug 1655963)
  • nsIDocShell::allowJavascript (bug 1655964)
  • nsIWindowUtils::paintFlashing (bug 1655966)
  • nsIWindowUtils::serviceWorkersTestingEnabled (bug 1655967)
  • nsIDocShell::defaultLoadFlags (bug 1655968)
  • nsIDocShell::cssErrorReportingEnabled (bug 1655969)
    Sorry if I mis-categorized them all in Core::Layout, I couldn't find which product and category was best matchng for DocShell and WindowUtils attributes.
Flags: needinfo?(poirot.alex)

I am not sure dt-fission-frame is the right bug to be blocked, but adding it anyway.

Depends on: dt-fission-framework
Whiteboard: dt-fission-m2-reserve

Moving M7 since the others are also M7.

Fission Milestone: M6c → M7
Flags: needinfo?(jwatt)

Bulk move of all dt-fission-m2-reserve bugs to Fission MVP milestone.

Fission Milestone: M7 → MVP
Whiteboard: dt-fission-m2-reserve → dt-fission-m2-reserve, dt-fission-m3-mvp
Whiteboard: dt-fission-m2-reserve, dt-fission-m3-mvp → dt-fission-m3-mvp

Moving some dt-fission-m3-mvp bugs from Fission MVP to M7 (blocking Beta experiment).

Fission Milestone: MVP → M7

Daniel, please find an assignee for this Fission M7 (Fx88) bug.

Flags: needinfo?(dholbert)

Sure. emilio, maybe you have cycles to take this?

(Hoping you might be a good person to point at this, since you added/implemented emulatePrefersColorScheme, and I recall you having done some fission work, and comment 4 sounds like you're open to working on this. :) )

Flags: needinfo?(dholbert) → needinfo?(emilio)

Will poke after I'm done with the non-native theme stuff.

Assignee: nobody → emilio
Status: NEW → ASSIGNED

We keep mMedium in nsPresContext rather than just looking it up in the
browsing context because that's used quite more frequently.

Pushed by ealvarez@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/11263420ea88
Move print and color-scheme simulation to browsingContext. r=ochameau,nika,devtools-backward-compat-reviewers

https://hg.mozilla.org/mozilla-central/rev/11263420ea88
https://hg.mozilla.org/mozilla-central/rev/203b0106b320

Status: ASSIGNED → RESOLVED
Closed: 3 years ago
status-firefox87: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → 87 Branch
Flags: needinfo?(emilio)
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: