Open Bug 1655963 Opened 4 years ago Updated 4 years ago

Move nsIDocShell::metaViewportOverride to BrowsingContext

Categories

(DevTools :: Responsive Design Mode, task)

task

Tracking

(Not tracked)

REOPENED

People

(Reporter: ochameau, Unassigned)

Details

(Whiteboard: [not-a-fission-bug])

+++ 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.

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

Fission Milestone: --- → M7

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)

Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → WORKSFORME

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.

Flags: needinfo?(mtigley)

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).

Flags: needinfo?(mtigley)

Based on comment 4, this is still necessary.

Status: RESOLVED → REOPENED
Resolution: WORKSFORME → ---

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!

Flags: needinfo?(mtigley)

(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.

Flags: needinfo?(mtigley)
Component: Layout → Responsive Design Mode
Product: Core → DevTools

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

Fission Milestone: M7 → MVP
Severity: -- → S3

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

No longer blocks: dt-rdm-fission
Type: enhancement → task
Fission Milestone: MVP → ---
Whiteboard: dt-fission-m2-reserve
Whiteboard: [fission-]

Not a Fission bug

Whiteboard: [fission-] → [not-a-fission-bug]
You need to log in before you can comment on or make changes to this bug.