[e10s] Get responsive design mode working in e10s

RESOLVED FIXED in Firefox 38

Status

RESOLVED FIXED
4 years ago
6 months ago

People

(Reporter: trishul.goel, Assigned: paul)

Tracking

(Blocks: 1 bug)

35 Branch
Firefox 38
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(e10s+)

Details

(Whiteboard: [e10s-m6])

Attachments

(1 attachment, 9 obsolete attachments)

(Reporter)

Description

4 years ago
User Agent: Mozilla/5.0 (X11; Linux i686; rv:35.0) Gecko/20100101 Firefox/35.0
Build ID: 20140913030206

Steps to reproduce:

Entered responsive mode (Ctrl + shift + M)  
Pressed the close button


Actual results:

Responsive design did not close 


Expected results:

Should switch back to normal mode

Comment 1

4 years ago
In fact, I feel they can be integrated into a bug.
Status: UNCONFIRMED → NEW
Component: Untriaged → Developer Tools: Responsive Mode
Ever confirmed: true
OS: Linux → All
Hardware: x86 → All
See Also: → bug 1067114
Summary: unable to quit responsive design → [e10s] unable to quit responsive design
Blocks: 875871
tracking-e10s: --- → m6+

Comment 2

4 years ago
I'm seeing something similar:

1. Enable e10s 
2. View a site with Responsive Design View
3. Press the little 'x' in the upper left to leave Responsive Design View - nothing happens.
4. Disable e10s and the little 'x' works.
There's an error thrown from gcli: "this.browser.contentWindow is null"
Paul, I know you had this working at the meetup a couple weeks ago. Any way you can upload a patch for it? We can worry about enabling the tests later.

Just having this able to close is a big deal, as right now you have to close the whole tab to get out of responsive mode with e10s enabled.
Flags: needinfo?(paul)
(Assignee)

Comment 5

4 years ago
Coming soon. A couple of other bugs to fix first.
Flags: needinfo?(paul)
Duplicate of this bug: 1067038
Duplicate of this bug: 1068916
Relatedly, clicking “Responsive Design Mode” button (or ^M; not ^m) toggles the responsive design view. In e10s additional responsive design toolbars will appear instead of closing the already enabled mode.

Comment 9

4 years ago
In last nightly (36.0a1 2014-10-21), an error is thrown in the browser console:

responsivedesign.jsm.js:186
TypeError: this.browser.contentWindow is null
(Assignee)

Comment 10

4 years ago
Created attachment 8508699 [details] [diff] [review]
wip
Assignee: nobody → paul
Status: NEW → ASSIGNED
(Assignee)

Comment 11

4 years ago
Created attachment 8508945 [details] [diff] [review]
wip
Attachment #8508699 - Attachment is obsolete: true
Blocks: 937169
Duplicate of this bug: 1081585
devtools bugs will block the e10s merge to Aurora, but blassey would like them to be tracked by dte10s meta bug 875871, not the tracking-e10s=m6+ flag.
tracking-e10s: m6+ → +
Whiteboard: [e10s-m6]
Created attachment 8529505 [details] [diff] [review]
Rebased WIP

Rebased from previous patch. Seems to be working well: opening and closing works, screenshot button works, and touch events are disabled on e10s.

There's one test failing under regular mode: browser_responsiveuiaddcustompreset.js, and some tests disabled under e10s. My guess is that the only thing left to do is get the test passing. Is that right, Paul? I'm willing to check it out.
Attachment #8508945 - Attachment is obsolete: true
Flags: needinfo?(paul)
(Assignee)

Comment 15

4 years ago
Sorry for the long delay. Looking at this now.
Flags: needinfo?(paul)
(Assignee)

Comment 16

4 years ago
Created attachment 8530262 [details] [diff] [review]
v1

Alex, can you make sure I didn't break the mulet?
Attachment #8529505 - Attachment is obsolete: true
Attachment #8530262 - Flags: review?(poirot.alex)
Summary: [e10s] unable to quit responsive design → [e10s] Get responsive design mode working in e10s
(Assignee)

Updated

4 years ago
Duplicate of this bug: 1108533
Comment on attachment 8530262 [details] [diff] [review]
v1

Review of attachment 8530262 [details] [diff] [review]:
-----------------------------------------------------------------

Yes, mulet is broken!
In b2g/chrome/content/desktop.js, you should convert tab.__responsiveUI to ResponsiveUIManger.getResponsiveUIForTab(tab).
Same thing in b2g/chrome/content/screen.js.

There is still some failures in last try, in browser/devtools/responsivedesign/test/browser_responsiveui.js.

Otherwise the patch looks good, I made some random comments.

::: browser/devtools/responsivedesign/responsivedesign-child.jsm
@@ +17,5 @@
> +    return;
> +  }
> +  addMessageListener("ResponsiveMode:RequestScreenshot", screenshot);
> +  addMessageListener("ResponsiveMode:NotifyOnResize", notifiyOnResize);
> +  addEventListener("load", makeScrollbarsFloating);

By listening to `load` you are also going to trigger makeScrollbarsFloating for iframes.
Either call makeScrollbarsFloating only for top level document load event,
or make it so that makeScrollbarsFloating only applies the stylesheet for the document which dispatched the load event.
(The second option looks better as the first one would miss applying to iframe created after top level document load event)

@@ +20,5 @@
> +  addMessageListener("ResponsiveMode:NotifyOnResize", notifiyOnResize);
> +  addEventListener("load", makeScrollbarsFloating);
> +  docShell.deviceSizeIsPageSize = true;
> +  gRequiresFloatingScrollbars = data.requiresFloatingScrollbars;
> +  if (docShell.contentViewer) {

A comment about this condition could be helpful.

@@ +54,5 @@
> +    try {
> +      winUtils.removeSheet(gFloatingScrollbarsStylesheet, win.AGENT_SHEET);
> +    } catch(e) { }
> +  }
> +  flushStyle();

Could you move all the code related to floating scrollbars to a dedicated cleanup function?

@@ +91,5 @@
> +  let canvas = content.document.createElementNS("http://www.w3.org/1999/xhtml", "canvas");
> +  let width = content.innerWidth;
> +  let height = content.innerHeight;
> +  canvas.mozOpaque = true;
> +  canvas.mozImageSmoothingEnabled = true;

Could you justify the usage of these properties in comments?

::: browser/devtools/responsivedesign/responsivedesign.jsm
@@ +139,5 @@
> +  }
> +  this.mm.addMessageListener("ResponsiveMode:Start:Done", childOn);
> +
> +  let requiresFloatingScrollbars = !this.mainWindow.matchMedia("(-moz-overlay-scrollbars)").matches;
> +  this.mm.loadFrameScript("resource:///modules/devtools/responsivedesign-child.jsm", true);

Frame scripts are named .js and not .jsm.
Attachment #8530262 - Flags: review?(poirot.alex)
(Assignee)

Comment 20

4 years ago
(In reply to Alexandre Poirot [:ochameau] from comment #19)
> Comment on attachment 8530262 [details] [diff] [review]
> v1
> 
> Review of attachment 8530262 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Yes, mulet is broken!
> In b2g/chrome/content/desktop.js, you should convert tab.__responsiveUI to
> ResponsiveUIManger.getResponsiveUIForTab(tab).
> Same thing in b2g/chrome/content/screen.js.

Ok.

> There is still some failures in last try, in
> browser/devtools/responsivedesign/test/browser_responsiveui.js.

Fixed: https://tbpl.mozilla.org/?tree=Try&rev=3190dc10d1fd

> Otherwise the patch looks good, I made some random comments.
> 
> ::: browser/devtools/responsivedesign/responsivedesign-child.jsm
> @@ +17,5 @@
> > +    return;
> > +  }
> > +  addMessageListener("ResponsiveMode:RequestScreenshot", screenshot);
> > +  addMessageListener("ResponsiveMode:NotifyOnResize", notifiyOnResize);
> > +  addEventListener("load", makeScrollbarsFloating);
> 
> By listening to `load` you are also going to trigger makeScrollbarsFloating
> for iframes.
> Either call makeScrollbarsFloating only for top level document load event,
> or make it so that makeScrollbarsFloating only applies the stylesheet for
> the document which dispatched the load event.
> (The second option looks better as the first one would miss applying to
> iframe created after top level document load event)

Actually, listening to "load" is useless because of the webprogress listener
that does the same job.

But your comment still applies. I'm not sure we can just add the ua stylesheet
just to the loaded document  (there are some corners cases I don't understand).
No matter what, you need to flush the top level document, and inserting the stylesheet
twice is just a no-op operation. It works well this way, there are some
useless sheet injection, but I think it's safer that way. If you know precisely
when to insert the sheet and invalidate the top level document, I'd be happy to
implement it the right way.

> @@ +20,5 @@
> > +  addMessageListener("ResponsiveMode:NotifyOnResize", notifiyOnResize);
> > +  addEventListener("load", makeScrollbarsFloating);
> > +  docShell.deviceSizeIsPageSize = true;
> > +  gRequiresFloatingScrollbars = data.requiresFloatingScrollbars;
> > +  if (docShell.contentViewer) {
> 
> A comment about this condition could be helpful.

Ok.

> @@ +54,5 @@
> > +    try {
> > +      winUtils.removeSheet(gFloatingScrollbarsStylesheet, win.AGENT_SHEET);
> > +    } catch(e) { }
> > +  }
> > +  flushStyle();
> 
> Could you move all the code related to floating scrollbars to a dedicated
> cleanup function?

Ok.

> @@ +91,5 @@
> > +  let canvas = content.document.createElementNS("http://www.w3.org/1999/xhtml", "canvas");
> > +  let width = content.innerWidth;
> > +  let height = content.innerHeight;
> > +  canvas.mozOpaque = true;
> > +  canvas.mozImageSmoothingEnabled = true;
> 
> Could you justify the usage of these properties in comments?

Removed mozImageSmoothingEnabled, kept mozOpaque that is self explanatory.

> ::: browser/devtools/responsivedesign/responsivedesign.jsm
> @@ +139,5 @@
> > +  }
> > +  this.mm.addMessageListener("ResponsiveMode:Start:Done", childOn);
> > +
> > +  let requiresFloatingScrollbars = !this.mainWindow.matchMedia("(-moz-overlay-scrollbars)").matches;
> > +  this.mm.loadFrameScript("resource:///modules/devtools/responsivedesign-child.jsm", true);
> 
> Frame scripts are named .js and not .jsm.

Meh. Fixed.
Still broken for me in
37.0a1 (2014-12-14)

12:11:04.995 TypeError: this.browser.contentWindow is null responsivedesign.jsm:187:2
12:11:09.963 TypeError: this.touchEventHandler is undefined responsivedesign.jsm:825:0
12:11:23.729 TypeError: this._deviceSizeWasPageSize is undefined responsivedesign.jsm:273:4
Comment on attachment 8536502 [details] [diff] [review]
v1.1

Review of attachment 8536502 [details] [diff] [review]:
-----------------------------------------------------------------

(In reply to Paul Rouget [:paul] (slow to respond. Ping me on IRC) from comment #20)
> > ::: browser/devtools/responsivedesign/responsivedesign-child.jsm
> > @@ +17,5 @@
> > > +    return;
> > > +  }
> > > +  addMessageListener("ResponsiveMode:RequestScreenshot", screenshot);
> > > +  addMessageListener("ResponsiveMode:NotifyOnResize", notifiyOnResize);
> > > +  addEventListener("load", makeScrollbarsFloating);
> > 
> > By listening to `load` you are also going to trigger makeScrollbarsFloating
> > for iframes.
> > Either call makeScrollbarsFloating only for top level document load event,
> > or make it so that makeScrollbarsFloating only applies the stylesheet for
> > the document which dispatched the load event.
> > (The second option looks better as the first one would miss applying to
> > iframe created after top level document load event)
> 
> Actually, listening to "load" is useless because of the webprogress listener
> that does the same job.
> 
> But your comment still applies. I'm not sure we can just add the ua
> stylesheet
> just to the loaded document  (there are some corners cases I don't
> understand).

Do you have test case for these corner cases, I have one last suggestion mentioned after...


Otherwise, it no longer breaks the mulet!

::: browser/devtools/responsivedesign/responsivedesign-child.js
@@ +110,5 @@
> +                              .getInterface(Ci.nsIWebProgress);
> +    webProgress.addProgressListener(this, Ci.nsIWebProgress.NOTIFY_ALL);
> +  },
> +  onLocationChange: function onLocationChange() {
> +    makeScrollbarsFloating();

What about passing the loading docshell to makeScrollbarsFloating and only inject the sheet to it, while always flushing toplevel?
onLocationChange: function onLocationChange(webProgress) {
  webProgress.QueryInterface(Ci.nsIDocShell);
  makeScrollbarsFloading(webProgress);
}
function makeScrollbarsFloating(targetDocshell) {
  ...
  let allDocShells = [];
  if (targetDocshell)
    allDocShells.push(targetDocshell);
  else {
    allDocShell.push(docShell);
    for (let i = 0; i < docShell.childCount; i++) {
      let child = docShell.getChildAt(i).QueryInterface(Ci.nsIDocShell);
      allDocShells.push(child);
    }
  }
  ...
  flushStyle();
}
Attachment #8536502 - Flags: review?(poirot.alex) → review+
(Assignee)

Comment 24

4 years ago
(In reply to Alexandre Poirot [:ochameau] from comment #23)
> Do you have test case for these corner cases

I tried to do like you describe. The issues I run into: scrollbars not always being restored (with codepen.io), and some deep docshells (tab > iframe > iframe) to not be affected in jsbin. Also - width and height of the page not being  properly recorded when the responsive mode is re-opened.

I'm sure I could figure out what's going on, but it's going to take even more time. I prefer to land this working patch first.
(Assignee)

Comment 25

4 years ago
Created attachment 8543787 [details] [diff] [review]
v1.2
Attachment #8543787 - Flags: review+
Depends on: 1074180
Depends on: 1098984
(Assignee)

Updated

4 years ago
Blocks: 1074835
(Assignee)

Comment 27

4 years ago
Created attachment 8545710 [details] [diff] [review]
1.2

Fixed test failure (hopefuly).
Attachment #8530262 - Attachment is obsolete: true
Attachment #8536502 - Attachment is obsolete: true
Attachment #8543787 - Attachment is obsolete: true
Attachment #8545710 - Flags: review+
(Assignee)

Updated

4 years ago
Keywords: checkin-needed
https://hg.mozilla.org/integration/fx-team/rev/66ec04bc10eb
Flags: in-testsuite+
Keywords: checkin-needed
Whiteboard: [e10s-m6] → [e10s-m6][fixed-in-fx-team]

Comment 31

4 years ago
In case you didn't see that: currently (Fx 38a01 2015-01-14) if you press several times on cmd+alt+m to trigger responsive design mode, it create multiples responsive toolbar.
(Assignee)

Comment 32

4 years ago
Created attachment 8549543 [details] [diff] [review]
v1.2

Fixes leak.

https://tbpl.mozilla.org/?tree=Try&rev=b33870d7c5f5
Attachment #8545710 - Attachment is obsolete: true
Attachment #8549543 - Flags: review+
(Assignee)

Updated

4 years ago
Keywords: checkin-needed
https://hg.mozilla.org/integration/fx-team/rev/553cdfa00c2b
Keywords: checkin-needed
Whiteboard: [e10s-m6] → [e10s-m6][fixed-in-fx-team]
No longer blocks: 1045689
(Assignee)

Comment 35

4 years ago
Created attachment 8551675 [details] [diff] [review]
patch.diff

Alright. I think I go it right this time: https://treeherder.mozilla.org/#/jobs?repo=try&revision=ad673a194a27
Attachment #8549543 - Attachment is obsolete: true
Attachment #8551675 - Flags: review+
(Assignee)

Updated

4 years ago
Keywords: checkin-needed
(Assignee)

Comment 36

4 years ago
Created attachment 8551676 [details] [diff] [review]
v1.2

Better commit message.
Attachment #8551675 - Attachment is obsolete: true
Attachment #8551676 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/ae9cc9b5f5dd
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Whiteboard: [e10s-m6][fixed-in-fx-team] → [e10s-m6]
Target Milestone: --- → Firefox 38
Depends on: 1126639
Depends on: 1145010

Updated

4 years ago
Depends on: 1141760
Depends on: 1189702
No longer depends on: 1249967

Updated

6 months ago
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.