1067145 - [e10s] Get responsive design mode working in e10s

Status: RESOLVED FIXED

(DevTools :: Responsive Design Mode, defect)

Description

[Trishul]

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

[YF (Yang)]

In fact, I feel they can be integrated into a bug.

Comment 2

[znmeb]

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.

Comment 3

[Heather Arthur [:harth]]

There's an error thrown from gcli: "this.browser.contentWindow is null"

Comment 4

[Heather Arthur [:harth]]

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.

Comment 5

[Paul Rouget [:paul]]

Coming soon. A couple of other bugs to fix first.

Comment 8

[Simonas Kazlauskas [:nagisa/:simukis]]

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

[Aurélien MANCA]

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

Comment 10

[Paul Rouget [:paul]]

Attachment: wip

Comment 11

[Paul Rouget [:paul]]

Attachment: wip

Comment 13

[Chris Peterson [:cpeterson]]

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.

Comment 14

[Heather Arthur [:harth]]

Attachment: 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.

Comment 15

[Paul Rouget [:paul]]

Sorry for the long delay. Looking at this now.

Comment 16

[Paul Rouget [:paul]]

Attachment: v1

Alex, can you make sure I didn't break the mulet?

Comment 17

[Paul Rouget [:paul]]

https://tbpl.mozilla.org/?tree=Try&rev=0dabaa67ffdc

Comment 19

[Alexandre Poirot [:ochameau]]

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.

Comment 20

[Paul Rouget [:paul]]

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

Comment 21

[Paul Rouget [:paul]]

Attachment: v1.1

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

Comment 22

[Adrian Aichner [:anaran]]

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 23

[Alexandre Poirot [:ochameau]]

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();
}

Comment 24

[Paul Rouget [:paul]]

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

Comment 25

[Paul Rouget [:paul]]

Attachment: v1.2

Comment 26

[Paul Rouget [:paul]]

https://tbpl.mozilla.org/?tree=Try&rev=70e568036a3f

Comment 27

[Paul Rouget [:paul]]

Attachment: 1.2

Fixed test failure (hopefuly).

Comment 28

[Paul Rouget [:paul]]

https://tbpl.mozilla.org/?tree=Try&rev=bfd24dea051a

Comment 29

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/fx-team/rev/66ec04bc10eb

Comment 30

[Ryan VanderMeulen [:RyanVM]]

Backed out for leaks.
https://hg.mozilla.org/integration/fx-team/rev/b7fb384c126f

https://treeherder.mozilla.org/ui/logviewer.html#?job_id=1620725&repo=fx-team

Comment 31

[Maxime Thirouin]

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.

Comment 32

[Paul Rouget [:paul]]

Attachment: v1.2

Fixes leak.

https://tbpl.mozilla.org/?tree=Try&rev=b33870d7c5f5

Comment 33

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/fx-team/rev/553cdfa00c2b

Comment 34

[Ryan VanderMeulen [:RyanVM]]

Backed out for browser_responsiveui.js failures.
https://hg.mozilla.org/integration/fx-team/rev/cbdda39e8e5a

https://treeherder.mozilla.org/logviewer.html#?job_id=1704440&repo=fx-team

Comment 35

[Paul Rouget [:paul]]

Attachment: patch.diff

Alright. I think I go it right this time: https://treeherder.mozilla.org/#/jobs?repo=try&revision=ad673a194a27

Comment 36

[Paul Rouget [:paul]]

Attachment: v1.2

Better commit message.

Comment 37

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/fx-team/rev/ae9cc9b5f5dd

Comment 38

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/ae9cc9b5f5dd