1250084 - The 'Responsive Design View' button from the Menu is not checked while the RDM is opened

Status: VERIFIED FIXED

(DevTools :: Responsive Design Mode, defect, P1)

Description

[Mihai Boldan, Desktop QA [:mboldan]]

[Note]:
- Logged bug based on Bug 1239437 Comment 50 

[Affected versions]:
- Firefox 47.0a1 (2016-02-21)

[Affected platforms]:
- WIndows 10 x86, Ubuntu 14.04 x86, Mac OS X 10.9.5

[Steps to reproduce]:
1. Launch Firefox.
2. From about:config, enable the devtools.responsive.html.enabled pref.
3. Open RDM from the Menu or by using keyboard shortcut.
4. Go to Menu -> Developer.

[Expected result]:
- The 'Responsive Design View' button from the Menu is checked.

[Actual result]:
- The 'Responsive Design View' button from the Menu is not checked.

[Regression range]:
- This is not a regression since this is a new implementation.

Comment 1

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

It seems the menu item state is managed manually by responsivedesign.jsm[1].

I think this would belong at the manager level in the new version.

[1]: https://dxr.mozilla.org/mozilla-central/source/devtools/client/responsivedesign/responsivedesign.jsm#406-418

Comment 2

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

To clarify, I believe we should put this in the ResponsiveUI object (which is specific to a particular tab) which lives in the manager.js file.

Here's an updated link to the legacy code since it moved around:

https://dxr.mozilla.org/mozilla-central/rev/494289c72ba3997183e7b5beaca3e0447ecaf96d/devtools/client/responsivedesign/responsivedesign.jsm#431-437

Comment 3

[Matteo Ferretti [:zer0] [:matteo]]

Attachment: MozReview Request: Bug 1250084 - The 'Responsive Design View' button from the Menu is not checked while the RDM is opened; r=jryans

Review commit: https://reviewboard.mozilla.org/r/44097/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/44097/

Comment 4

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Comment on attachment 8737722 [details]
MozReview Request: Bug 1250084 - The 'Responsive Design View' button from the Menu is not checked while the RDM is opened; r=jryans

https://reviewboard.mozilla.org/r/44097/#review40861

Seems fine overall, but would like to re-test with the corrected element ID.

::: devtools/client/responsive.html/events.js:7
(Diff revision 1)
> + * License, v. 2.0. If a copy of the MPL was not distributed with this
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +"use strict";
> +
> +const { filter, map, merge } = require("sdk/event/utils");

This file screams "author worked on SDK" to me. :)

::: devtools/client/responsive.html/manager.js:209
(Diff revision 1)
>     *
>     * In the future, we can do better by using swapFrameLoaders to preserve the
>     * state.  Platform discussions are in progress to make this happen.  See
>     * bug 1238160 about <iframe mozbrowser> for more details.
>     */
> -  init: Task.async(function*() {
> +  init: Task.async(function* () {

`function*(){` seems like the prevailing DevTools style, but maybe we should have some ESLint rule for it?

::: devtools/client/responsive.html/manager.js:263
(Diff revision 1)
>    return deferred.promise;
>  }
> +
> +const onActivate = (tab) => setMenuCheckFor(tab);
> +
> +const onClose = ({window, tabs}) => {

Nit: ({ window, tab })

::: devtools/client/responsive.html/manager.js:270
(Diff revision 1)
> +    ResponsiveUIManager.closeIfNeeded(window, tab);
> +  }
> +};
> +
> +const setMenuCheckFor = (tab, window = getOwnerWindow(tab)) => {
> +  let menu = window.document.getElementById("Tools:ResponsiveUI");

:ochameau recently changed all the menus around, so this element no longer exists for me.

I think you can use "menu_responsiveUI" now.

https://dxr.mozilla.org/mozilla-central/rev/9bd90088875399347b05d87c67d3709e31539dcd/devtools/client/menus.js#140

Comment 5

[Matteo Ferretti [:zer0] [:matteo]]

Comment on attachment 8737722 [details]
MozReview Request: Bug 1250084 - The 'Responsive Design View' button from the Menu is not checked while the RDM is opened; r=jryans

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/44097/diff/1-2/

Comment 6

[Matteo Ferretti [:zer0] [:matteo]]

Comment on attachment 8737722 [details]
MozReview Request: Bug 1250084 - The 'Responsive Design View' button from the Menu is not checked while the RDM is opened; r=jryans

Whoops, published the wrong branch, this still missing one thing, sorry.

Comment 7

[Matteo Ferretti [:zer0] [:matteo]]

Comment on attachment 8737722 [details]
MozReview Request: Bug 1250084 - The 'Responsive Design View' button from the Menu is not checked while the RDM is opened; r=jryans

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/44097/diff/2-3/

Comment 8

[Matteo Ferretti [:zer0] [:matteo]]

Ryan, as you can see from try server:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=68ac36697683&selectedJob=19335911

We still have the issue on Win Debug.
You can take a look at the code / unit test and if you have a suggestion let me know. Otherwise, as said during the yesterday's stand up, I would disable this test for windows or remove the menu_02 unit test at all, and file a follow up bug.

Comment 9

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Comment on attachment 8737722 [details]
MozReview Request: Bug 1250084 - The 'Responsive Design View' button from the Menu is not checked while the RDM is opened; r=jryans

https://reviewboard.mozilla.org/r/44097/#review42441

::: devtools/client/responsive.html/events.js:17
(Diff revision 3)
> +const tabSelect = filter(tabEvents, e => e.type === "TabSelect");
> +const tabClose = filter(tabEvents, e => e.type === "TabClose");
> +const windowOpen = filter(browserEvents, e => e.type === "load");
> +const windowClose = filter(browserEvents, e => e.type === "close");
> +
> +const activate = merge([

Add some comments to each of these exports to describe which events they are... like that this is tabs from any window, etc.

::: devtools/client/responsive.html/manager.js:83
(Diff revision 3)
>     * @param tab
>     *        The browser tab.
>     * @return Promise
>     *         Resolved (with no value) when closing is complete.
>     */
>    closeIfNeeded: Task.async(function* (window, tab) {

I was able to reproduce the Windows debug issue locally.  In fact, it may actually reproduce for you as well, but it does not register as a "failure" in the test harness.  You have to scroll up a while and look for the TEST-UNEXPECTED-FAIL lines.

Anyway, it seems like this issue is we're leaving these handlers bound.  Try something like:

```
     if (this.isActiveForTab(tab)) {
-      yield this.activeTabs.get(tab).destroy();
+      let ui = this.activeTabs.get(tab);
       this.activeTabs.delete(tab);
-      this.emit("off", { tab });

       if (!this.activeTabs.size) {
         off(events.activate, "data", onActivate);
         off(events.close, "data", onClose);
       }

+      yield ui.destroy();
+      this.emit("off", { tab });
```

::: devtools/client/responsive.html/manager.js:334
(Diff revision 3)
> +
> +const onActivate = (tab) => setMenuCheckFor(tab);
> +
> +const onClose = ({ window, tabs }) => {
> +  for (let tab of tabs) {
> +    ResponsiveUIManager.closeIfNeeded(window, tab.outerHTML);

Why `outerHTML`?  `closeIfNeeded` expects a real tab object.

::: devtools/client/responsive.html/test/browser/browser_menu_item_02.js:15
(Diff revision 3)
> +const { getActiveTab } = require("sdk/tabs/utils");
> +const { getMostRecentBrowserWindow } = require("sdk/window/utils");
> +const { open, close, startup } = require("sdk/window/helpers");
> +const { partial } = require("sdk/lang/functional");
> +
> +const openBrowserWindow = partial(open, null, {features: {toolbar: true}});

Nit: { space

Comment 10

[Matteo Ferretti [:zer0] [:matteo]]

https://reviewboard.mozilla.org/r/44097/#review42441

> I was able to reproduce the Windows debug issue locally.  In fact, it may actually reproduce for you as well, but it does not register as a "failure" in the test harness.  You have to scroll up a while and look for the TEST-UNEXPECTED-FAIL lines.
> 
> Anyway, it seems like this issue is we're leaving these handlers bound.  Try something like:
> 
> ```
>      if (this.isActiveForTab(tab)) {
> -      yield this.activeTabs.get(tab).destroy();
> +      let ui = this.activeTabs.get(tab);
>        this.activeTabs.delete(tab);
> -      this.emit("off", { tab });
> 
>        if (!this.activeTabs.size) {
>          off(events.activate, "data", onActivate);
>          off(events.close, "data", onClose);
>        }
> 
> +      yield ui.destroy();
> +      this.emit("off", { tab });
> ```

Unfortunately on my machine it doesn't show any TST-UNEXPECTED-FAIL on this test. How you were able to reproduce it? I'm on Windows 8.1, I tried both to run just this test and both the whole test suite for responsive.html. I can blindly adapt your changes – I believe you tested them and fixed the issue locally – and push on try to see what's happening, but if I can actually reproduce the failure that would be better.

> Why `outerHTML`?  `closeIfNeeded` expects a real tab object.

It was a left over from the previous code; there was a `console.log` there, that dumped the `outerHTML`, and during the removal is ended up there.

Comment 11

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

https://reviewboard.mozilla.org/r/44097/#review42441

> Unfortunately on my machine it doesn't show any TST-UNEXPECTED-FAIL on this test. How you were able to reproduce it? I'm on Windows 8.1, I tried both to run just this test and both the whole test suite for responsive.html. I can blindly adapt your changes – I believe you tested them and fixed the issue locally – and push on try to see what's happening, but if I can actually reproduce the failure that would be better.

Here are some more details about my setup:

* Host Machine: MacBook Pro 15" Retina (2013) with OS X 10.11.4
* VM Application: VMWare Fusion 7.1.3
* Guest Machine: Windows 10 Pro (10586.164)
* Compiler: Visual Studio Community 2015 (14.0.24720.00 Update 1)
* .mozconfig: https://gist.github.com/jryans/fb99b138026d90497bc9747a989a30eb
* Failing Test Command: `./mach mochitest devtools/client/responsive.html/test/browser/browser_menu_item_*`
* Log from Failed Run: https://gist.github.com/jryans/afd7454697f98270951199018b16142c

Running just the one single test did not seem to work for me, as the test suite would just hang.  Running both menu_item tests together seemed to work.

Comment 12

[Matteo Ferretti [:zer0] [:matteo]]

Comment on attachment 8737722 [details]
MozReview Request: Bug 1250084 - The 'Responsive Design View' button from the Menu is not checked while the RDM is opened; r=jryans

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/44097/diff/3-4/

Comment 13

[Matteo Ferretti [:zer0] [:matteo]]

As we discussed, I disabled for the time being the test on Windows Debug: https://treeherder.mozilla.org/#/jobs?repo=try&revision=da7c45b9be77&selectedJob=19928907

If it's okay that way, we can land as is and I will file a follow up bug.

Comment 14

[Matteo Ferretti [:zer0] [:matteo]]

ryan, as mentioned in the stand up today, here the try build with the changes but without the test skipped: https://treeherder.mozilla.org/#/jobs?repo=try&revision=9682626bf2ab&selectedJob=19903988

As you can see, there is still a window that leaks.

Comment 15

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Comment on attachment 8737722 [details]
MozReview Request: Bug 1250084 - The 'Responsive Design View' button from the Menu is not checked while the RDM is opened; r=jryans

https://reviewboard.mozilla.org/r/44097/#review45529

::: devtools/client/responsive.html/test/browser/browser.ini:18
(Diff revisions 3 - 4)
>  [browser_device_width.js]
> -skip-if = (e10s && debug) # Bug 1262432 - crashes at nsLayoutUtils::HasDisplayPort(content)
>  [browser_exit_button.js]
>  [browser_menu_item_01.js]
>  [browser_menu_item_02.js]
> +skip-if = os == "win" && debug #debug-only failure

This comment should reference the bug to fix the failure.  So please file it, and mention the bug number here.

Comment 16

[Pulsebot]

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

Comment 17

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/352d365eb4ed

Comment 18

[Mihai Boldan, Desktop QA [:mboldan]]

The issue is no longer reproducible on Firefox 49.0a1 (2016-04-28).
The tests were performed on Windows 10 x86, Ubuntu 14.04 x86 and on Mac OS X 10.9.5.
I am marking this issue Verified-FIxed.