1655963 - Move nsIDocShell::metaViewportOverride to BrowsingContext

Status: REOPENED

(DevTools :: Responsive Design Mode, task)

Description

[Alexandre Poirot [:ochameau]]

+++ This bug was initially created as a clone of Bug #1655959 +++

This attribute is used by DevTools and is expected to be also applied to remoted Fission iframe.
By setting this attribute on nsIDocShell, we don't ensure applying it to remote iframe, which spawn another nsIDocShell in their distinct process.
The typical way to fix that is to move such attribute up to BrowsingContext class.
You can take example on bug 1593708, which moved inRDMPanel from PresShell up to BrowsingContext. Or bug 1620966, which moved watchedByDevTools from DocShell up to BrowsingContext.

I imagine that it may also fix non-devtools usecase if this attribute is toggled outside of DevTools.
See bug 1591120 for extensive context.

Comment 1

[Chris Peterson [:cpeterson]]

Tracking dt-fission-m2-reserve bugs for Fission Beta milestone (M7).

Comment 2

[Hiroyuki Ikezoe (:hiro)]

I don't think the flag needs to be propagated into descendant documents, in my understandings it's just necessary only for the top level content document.

Closing as WORKSFORME. (CCing Brad just in case I am missing something)

Comment 3

[Brad Werth [:bradwerth]]

Micah knows better than I do if this will lead to problems down the road with RDM. For my part, this sounds like a fine decision, unless it's important that all of the RDM-relevant flags travel together.

Comment 4

[Micah [:mtigley] (she/her)]

Thanks for the ping, Brad! AFAIK, I don't think this flag needs to be propagated into its descendent documents for meta viewport correctness, nor should it cause any problems with RDM if it were to. So the decision to move this flag to the browsing context should be fine.

The motivation behind Bug 1593708 was due to a new nsIDocShell being created during a page reload or navigation. This had the effect of clearing RDM-specific flags set in https://searchfox.org/mozilla-central/source/devtools/client/responsive/ui.js#732. But this has been remedied by having the responsive front set the flags again when a new target is available.

For consistency, I think RDM-specific flags should be set on the browsing context where possible. This would help streamline how RDM-specific flags are set by having them done in one place. I believe the involved flags are nsIDocShell::touchEventsOverride (Bug 1655962), nsIDocShell::metaViewportOverride, and nsIContentViewer::overrideDPPX (Bug 1655959).

Comment 5

[Brad Werth [:bradwerth]]

Based on comment 4, this is still necessary.

Comment 6

[Hiroyuki Ikezoe (:hiro)]

Hi Micah, I don't quite understand what the outcome expected in this bug. Is this what you guys are going to change is a blocker for Fission?

Also just moving nsIDocShell::metaViewportOverride to (top level) BrowsingContext is not a Layout stuff at all, AFAICT. So please move to an appropriate component. Thanks!

Comment 7

[Micah [:mtigley] (she/her)]

(In reply to Hiroyuki Ikezoe (:hiro) from comment #6)

Hi Micah, I don't quite understand what the outcome expected in this bug. Is this what you guys are going to change is a blocker for Fission?

Also just moving nsIDocShell::metaViewportOverride to (top level) BrowsingContext is not a Layout stuff at all, AFAICT. So please move to an appropriate component. Thanks!

This wouldn't be a blocker for Fission. We'd move nsIDocShell::metaViewportOverride to BrowsingContext just for consistency with how other flags in RDM are set.

Right, since this flag is specifically used by RDM in DevTools, let's move this issue to the Responsive Design Mode component for DevTools.

Comment 8

[Neha Kochar [:neha]]

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

Comment 9

[Nicolas Chevobbe [:nchevobbe]]

Looks like a refactoring to me, so moving out of Fission

Comment 10

[Chris Peterson [:cpeterson]]

Not a Fission bug