Closed Bug 1239437 Opened 8 years ago Closed 8 years ago

Land basic shell for Responsive Design rewrite

Categories

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

Product:
DevTools
Component:
Responsive Design Mode
Type:
defect

Tracking

(firefox46 affected, firefox47 verified)

Status:
VERIFIED FIXED
Milestone:
Firefox 47
Iteration:
47.1 - Feb 8
Tracking Flags:
Tracking Status
firefox46 --- affected
firefox47 --- verified

People

(Reporter: jryans, Assigned: jryans)

References

Details

(Whiteboard: [multiviewport] [mvp-rdm])

Attachments

(8 files)

MozReview Request: Bug 1239437 - Delegate to empty, new responsive UI tool if enabled. r=pbrosset
58 bytes, text/x-review-board-request
pbro
: review+
Details
MozReview Request: Bug 1239437 - Allow linting for responsive.html directory. r=pbrosset
58 bytes, text/x-review-board-request
pbro
: review+
Details
MozReview Request: Bug 1239437 - Add a basic viewport frame. r=pbrosset
58 bytes, text/x-review-board-request
pbro
: review+
Details
MozReview Request: Bug 1239437 - Hide iframe border. r=pbrosset
58 bytes, text/x-review-board-request
pbro
: review+
Details
MozReview Request: Bug 1239437 - Stir in React and Redux. r=pbrosset,jlongster
58 bytes, text/x-review-board-request
jlong
: review+
pbro
: review+
Details
MozReview Request: Bug 1239437 - Allow run_test to be unused in xpcshell tests. r=pbrosset
58 bytes, text/x-review-board-request
pbro
: review+
Details
MozReview Request: Bug 1239437 - xpcshell tests for responsive.html. r=pbrosset
58 bytes, text/x-review-board-request
pbro
: review+
Details
MozReview Request: Bug 1239437 - Browser mochitests for responsive.html. r=pbrosset
58 bytes, text/x-review-board-request
pbro
: review+
Details 
We'll be rewriting RDM using HTML and React.

This bug should land a basic tool that can:

* Open and close RDM
* Show the page in a single viewport
* Grippers to resize, just so there's some amount of interactivity

Also this will setup the basic machinery for the project:

* Uses React and Redux
* Includes at least one test
Flags: qe-verify?
Flags: qe-verify? → qe-verify+
I decided to postpone the grippers / resize handles to bug 1241326, so this bug will have no interactivity.  There was enough complexity spinning up the tool at all, so it seems better to have that in a separate bug.

For this bug, all you get is a fixed, unchangeable viewport at a phone-like size.  There are many known issues that will be resolved in future bugs:

* The location bar is a surprising chrome:// URL (bug 1240900)
* The toolbox targets the tool, not the page content (bug 1240907)
* The toolbox closes when toggle in and out of the tool (bug 1240912)
* The page state is lost / reloaded (bug 1240913)
To enable the new UI, you'll need to set "devtools.responsive.html.enabled" to true.

To work around some of the known issues, I recommend using the RDM keyboard shortcut (Cmd-Opt-M on Mac / Ctrl-Shift-M on Windows / Linux) or menu item to toggle it on and off.  The toggle button in the toolbox would likely be confusing for now, since the toolbox aborts each time you toggle.
Comment on attachment 8710207 [details]
MozReview Request: Bug 1239437 - Allow linting for responsive.html directory. r=pbrosset

https://reviewboard.mozilla.org/r/31677/#review28633

As discussed, this will need to be removed since similar changes have happened on fx-team in the meantime.
Attachment #8710207 - Flags: review?(pbrosset) → review+
Attachment #8710206 - Flags: review?(pbrosset) → review+
Comment on attachment 8710206 [details]
MozReview Request: Bug 1239437 - Delegate to empty, new responsive UI tool if enabled. r=pbrosset

https://reviewboard.mozilla.org/r/31675/#review28635

::: devtools/client/moz.build:26
(Diff revision 1)
> +    'responsive.html',

Not sure about the name here to be honest.
I know it differs from the current repsonsivedesign because it's written in HTML as opposed to XUL, but I don't think this reason alone warrants calling it responsive.html.
Especially because at some stage the current responsivedesign will go away.
Let's try and find another name. In fact, maybe just 'responsive' is enough?

::: devtools/client/responsive.html/manager.js:11
(Diff revision 1)
> + * open and closing the responsive UI.

s/open/opening ?

::: devtools/client/responsive.html/manager.js:32
(Diff revision 1)
> +      this.runIfNeeded(window, tab);

Might be useful to return in both the if and else branch so that consumers can use the promises returned by runIfNeeded (and closeIfNeeded which is introduced in a later patch).
Comment on attachment 8710208 [details]
MozReview Request: Bug 1239437 - Add a basic viewport frame. r=pbrosset

https://reviewboard.mozilla.org/r/31679/#review28639

::: devtools/client/responsive.html/index.xhtml:7
(Diff revision 1)
> +  <head>

Should there be a `<title>` element in here? I know at some stage we want to make this tool "transparent" somehow and just have the url of the current page in the url bar and the title of the current page in the title bar, so maybe this is irrelevant.

::: devtools/client/responsive.html/manager.js:152
(Diff revision 1)
> +   * state.  Platform discussions are in progress to make this happen.

Maybe link to the relevant bug here.

::: devtools/client/responsive.html/manager.js:184
(Diff revision 1)
> +  tab.linkedBrowser.addEventListener("load", handle, true, true);

Why 2 times `true` here?
Attachment #8710208 - Flags: review?(pbrosset) → review+
Comment on attachment 8710209 [details]
MozReview Request: Bug 1239437 - Hide iframe border. r=pbrosset

https://reviewboard.mozilla.org/r/31681/#review28641

::: devtools/client/responsive.html/moz.build:8
(Diff revision 1)
> +    'themes',

This isn't the way we've been managing CSS so far. Can you explain what are the advantages of having a local themes folder, rather than putting the new CSS file into devtools/client/themes/?

Not against it, fwiw I think it makes more sense to have the CSS close to its markup, but just wondering why we're doing this now, and what's the story with the rest of our CSS.

Also, the name `themes` might not be the best. Is it really going to contain several "themes" for the tool in the end? I'm guessing that it's really just going to contain the various CSS files for the components in the tool, and then each file may contain theme-specific styles with selector prefixes like `.theme-dark`
Attachment #8710209 - Flags: review?(pbrosset)
Comment on attachment 8710210 [details]
MozReview Request: Bug 1239437 - Stir in React and Redux. r=pbrosset,jlongster

https://reviewboard.mozilla.org/r/31683/#review28643

Not sure if this is related to this commit or one after, but I didn't manage to start the RDM. Setting the right pref, opening a browser tab and then pressing ctrl+shift+M does load the right page, but it remains empty, and I have an error in the console:

```
ReferenceError: window is not defined
[63]</ReactEventListener.monitorScrollValue()
 react.js:10333
[28]</ReactBrowserEventEmitter<.ensureScrollValueMonitoring()
 react.js:4133
[72]</ReactMount._registerComponent()
 react.js:11573
[72]</ReactMount._renderNewRootComponent()
 react.js:11594
[72]</ReactMount._renderSubtreeIntoContainer()
 react.js:11674
[72]</ReactMount.render()
 react.js:11694
init()
 index.js:38
onLoad()
 index.js:61
```

Also, as noted below, I'd feel better if James gave a final look at this.
Anyway, from what I could see, the code is very easily read, and this looks very consistent with the rest of the react/redux code we have elsewhere in devtoos.

::: devtools/client/responsive.html/actions.js:4
(Diff revision 1)
> +

Just a thought, why not move this file as `index.js` (or something) inside the `actions` folder?

::: devtools/client/responsive.html/actions.js:12
(Diff revision 1)
> +function createEnum(array, target) {

The important part in this file that everyone working on this tool will have to read is the list of existing actions with their descriptions, so please move this helper function to the bottom of the file instead.

::: devtools/client/responsive.html/actions.js:19
(Diff revision 1)
> +createEnum([

And please add a comment before this list that says something like:

```
// List of all possible actions.
// Please add a self-explanatory comment for each new action you add
```

Oh wait, this file is really just for convenience, right? The actual actions that components are going to call are the functions in the actions folder.

If so, then I'm not convinced by the value that this file brings. Especially that we'll end up duplicating comments here and in action modules.
Does this really help?

::: devtools/client/responsive.html/app.js:11
(Diff revision 1)
> +const Models = require("./models");

I'm wondering if models is the right name here (sorry about being so nit picking with name on this bug ...). Maybe it's just me and my very very little react/redux experience, but seeing both store.js and models.js in the responsive.html folder confused me for a bit. 

Models here is a way to define and validate the type of the props being passed to react components, right? So something like prop-types.js would be better? Or something like this.
I just find model and store too similar in terms of meaning and got confused for a minute.

Talking about react and redux, I'll continue my review and note things I think should be changed as I go along, but please don't consider this as a final review, I simply have way too little experience with the framework to be considered a good reviewer. Especially now that the base architecture is being put in place. 
Can you please ask jlongster to give it a quick look too?

::: devtools/client/responsive.html/index.js:33
(Diff revision 1)
> +    // TODO: Should we track this as a separate tool from the old version?

At the very least we should track if people have the new tool enabled.
Attachment #8710210 - Flags: review?(pbrosset)
Attachment #8710211 - Flags: review?(pbrosset) → review+
Comment on attachment 8710211 [details]
MozReview Request: Bug 1239437 - Allow run_test to be unused in xpcshell tests. r=pbrosset

https://reviewboard.mozilla.org/r/31685/#review28647
Attachment #8710212 - Flags: review?(pbrosset) → review+
Comment on attachment 8710212 [details]
MozReview Request: Bug 1239437 - xpcshell tests for responsive.html. r=pbrosset

https://reviewboard.mozilla.org/r/31687/#review28649

::: devtools/client/responsive.html/test/unit/head.js:6
(Diff revision 1)
> +/* eslint-disable no-unused-vars */

This would be better:

`eslint no-unused-vars: [2, {"vars": "local"}]`

This way eslint won't complain about unused global vars and functions, but will still report locally unused variables as errors, and that's useful.

::: devtools/client/responsive.html/test/unit/head.js:21
(Diff revision 1)
> +function waitUntilState(store, predicate) {

Please add some jsdoc comment block for this function.

::: devtools/client/responsive.html/test/unit/head.js:40
(Diff revision 1)
> +function waitUntilAction(store, actionType) {

Please add some jsdoc comment block for this function.

::: devtools/client/responsive.html/test/unit/head.js:57
(Diff revision 1)
> +}

I just realized this 2 functions also exist in memory/test/unit/head.js.
Maybe worth putting them in common in, say, devtools/client/framework/test/shared-react-unit-test-head.js, or something better, and load that through loadSubScript?

::: devtools/client/responsive.html/test/unit/test_add_viewport.js:15
(Diff revision 1)
> +add_task(function*() {

I have so far not used add_task in xpcshell tests, but I know that in mochitests, they get called automatically. So, no need to call `run_next_test` from `run_test`. But maybe that's not the case here.
Comment on attachment 8710213 [details]
MozReview Request: Bug 1239437 - Browser mochitests for responsive.html. r=pbrosset

https://reviewboard.mozilla.org/r/31689/#review28653

::: devtools/client/responsive.html/manager.js:36
(Diff revision 1)
> -      this.runIfNeeded(window, tab);
> +      this.openIfNeeded(window, tab);

Back to a comment I made when reviewing the first commit: I think the promise returned by closeIfNeeded and openIfNeeded should be returned here too.

::: devtools/client/responsive.html/manager.js:47
(Diff revision 1)
> +   * @return ResponsiveUI

@return should be Promise instead of ResponsiveUI. So maybe something like:

```
@return Promise
        Resolves to the ResponsiveUI instance for this tab
```

::: devtools/client/responsive.html/manager.js:68
(Diff revision 1)
> +  closeIfNeeded(window, tab) {

Maybe this should return a promise for consistency with openIfNeeded.

::: devtools/client/responsive.html/manager.js:114
(Diff revision 1)
> -        this.runIfNeeded(window, tab);
> +        this.openIfNeeded(window, tab);

If there's a chance that ui.inited fails, then the promise rejection will end up here. So maybe add `.catch(e => console.error(e))`

::: devtools/client/responsive.html/test/browser/browser_viewport_basics.js:10
(Diff revision 1)
> +this.test = makeRDMTest(TEST_URL, function*({ ui }) {

I like `add_task` better now in mochitests, because:
- no need to call `finish` at the end
- no need to use `Task.async`
- no need to set anything on `this.test`

So, 2 options I think.

1) keep makeRDMTest but refactor it so it uses `add_task` instead of `Task.async`, and doesn't return anything.

2) change this test to:

```
add_task(function*() {
  let {ui, manager} = yield openTabAndRDM(url);
  ...
});
```

::: devtools/client/responsive.html/test/browser/head.js:6
(Diff revision 1)
> +/* eslint-disable no-unused-vars */

Same comment as for the xpcshell head.js file, you want to keep the rule for local variables.

::: devtools/client/responsive.html/test/browser/head.js:7
(Diff revision 1)
> +/* global addTab, removeTab, ResponsiveUI */

Instead of this comment, you can use:
`import-globals-from ../../../framework/test/shared-head.js`

Which will register all globals from the imported shared-head.js here. Then you don't have to manually maintain this list.
Attachment #8710213 - Flags: review?(pbrosset)
https://reviewboard.mozilla.org/r/31677/#review28633

Right, I've made it an empty commit now after rebasing, to avoid confusing MozReview.  Will remove when landing.
https://reviewboard.mozilla.org/r/31675/#review28635

> Not sure about the name here to be honest.
> I know it differs from the current repsonsivedesign because it's written in HTML as opposed to XUL, but I don't think this reason alone warrants calling it responsive.html.
> Especially because at some stage the current responsivedesign will go away.
> Let's try and find another name. In fact, maybe just 'responsive' is enough?

We agreed on IRC it was okay to keep the current name for now.  I wanted something semi-short but also clearly the "new" one.  When it's time to remove the legacy one, I'll rename it to just "responsive".
https://reviewboard.mozilla.org/r/31679/#review28639

> Should there be a `<title>` element in here? I know at some stage we want to make this tool "transparent" somehow and just have the url of the current page in the url bar and the title of the current page in the title bar, so maybe this is irrelevant.

Right, this tool (unlike most others) wants to "blend in" with the browser.  You bring up a good point though!  I added a note to bug 1240900 to mimic the title as well.

> Maybe link to the relevant bug here.

Okay, bug number added.

> Why 2 times `true` here?

Second true is a Gecko-specific `wantsUntrusted` arg, but anyway it seems unnecessary.  Removed.
https://reviewboard.mozilla.org/r/31681/#review28641

> This isn't the way we've been managing CSS so far. Can you explain what are the advantages of having a local themes folder, rather than putting the new CSS file into devtools/client/themes/?
> 
> Not against it, fwiw I think it makes more sense to have the CSS close to its markup, but just wondering why we're doing this now, and what's the story with the rest of our CSS.
> 
> Also, the name `themes` might not be the best. Is it really going to contain several "themes" for the tool in the end? I'm guessing that it's really just going to contain the various CSS files for the components in the tool, and then each file may contain theme-specific styles with selector prefixes like `.theme-dark`

After thinking about it more, the extra directory is not needed.  I'll move it up as "index.css" next to the document, similar to how some other tools have structural CSS next to markup.  So for the moment, there will just be the one file.

Our React component group has decided we'll change to component-specifc CSS, one file per component that lives next to the component JS file.  I'll leave that migration until later since I don't think the piece to load it has landed anywhere yet.
https://reviewboard.mozilla.org/r/31683/#review28643

Not sure what to say about the error message...  I tested with a clean profile and as well as after rebasing, and I did not see any errors.  Maybe you needed to rebuild more fully?  Also, are you using artifact builds?  I am not using those, not sure if it matters.

I'll add James as a reviewer for this piece.  If you didn't guess already, I based the organization heavily on the memory tool.

> Just a thought, why not move this file as `index.js` (or something) inside the `actions` folder?

That sounds reasonable to me.

> The important part in this file that everyone working on this tool will have to read is the list of existing actions with their descriptions, so please move this helper function to the bottom of the file instead.

Agreed, I moved it.

> And please add a comment before this list that says something like:
> 
> ```
> // List of all possible actions.
> // Please add a self-explanatory comment for each new action you add
> ```
> 
> Oh wait, this file is really just for convenience, right? The actual actions that components are going to call are the functions in the actions folder.
> 
> If so, then I'm not convinced by the value that this file brings. Especially that we'll end up duplicating comments here and in action modules.
> Does this really help?

I added a comment to explain the file.  You're right that there is some duplication between these action type constants and the "action creator" functions in ./actions.  However, I think it will be nice to have this central list of action types esp. as the tool grows, since you'll be able to see one list of all possible actions in a single place.

I described that in my added comment.

> I'm wondering if models is the right name here (sorry about being so nit picking with name on this bug ...). Maybe it's just me and my very very little react/redux experience, but seeing both store.js and models.js in the responsive.html folder confused me for a bit. 
> 
> Models here is a way to define and validate the type of the props being passed to react components, right? So something like prop-types.js would be better? Or something like this.
> I just find model and store too similar in terms of meaning and got confused for a minute.
> 
> Talking about react and redux, I'll continue my review and note things I think should be changed as I go along, but please don't consider this as a final review, I simply have way too little experience with the framework to be considered a good reviewer. Especially now that the base architecture is being put in place. 
> Can you please ask jlongster to give it a quick look too?

Nit picking names is fine, names are pretty critical, especially when starting something new! :)

The models validate the shape / types of data based down to components as props.  I do see what you mean about store vs. models.  The memory tool uses the same names for the store and models files, so maybe it's best to follow for consistency?

For the moment, I added a comment in the models.js file to explain its purpose a bit more.

> At the very least we should track if people have the new tool enabled.

I filed bug 1242057 to tune telemetry, and noted in the comment.
https://reviewboard.mozilla.org/r/31687/#review28649

> I just realized this 2 functions also exist in memory/test/unit/head.js.
> Maybe worth putting them in common in, say, devtools/client/framework/test/shared-react-unit-test-head.js, or something better, and load that through loadSubScript?

Okay, I've moved them to `shared-redux-head.js` (since they are about the Redux store), and memory imports them.

> I have so far not used add_task in xpcshell tests, but I know that in mochitests, they get called automatically. So, no need to call `run_next_test` from `run_test`. But maybe that's not the case here.

Looks like xpcshell doesn't need it either, removed.
https://reviewboard.mozilla.org/r/31689/#review28653

> If there's a chance that ui.inited fails, then the promise rejection will end up here. So maybe add `.catch(e => console.error(e))`

Okay, I saved the all off to `completed` and catch from there, since they all return promises now.

> I like `add_task` better now in mochitests, because:
> - no need to call `finish` at the end
> - no need to use `Task.async`
> - no need to set anything on `this.test`
> 
> So, 2 options I think.
> 
> 1) keep makeRDMTest but refactor it so it uses `add_task` instead of `Task.async`, and doesn't return anything.
> 
> 2) change this test to:
> 
> ```
> add_task(function*() {
>   let {ui, manager} = yield openTabAndRDM(url);
>   ...
> });
> ```

I think I like the overall pattern of `makeRDMTest`, since it gives you one place to write the common header and footer steps most tests will have.  So, I'll go with your option 1, and make it use `add_task`.
Comment on attachment 8710206 [details]
MozReview Request: Bug 1239437 - Delegate to empty, new responsive UI tool if enabled. r=pbrosset

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/31675/diff/1-2/
Comment on attachment 8710207 [details]
MozReview Request: Bug 1239437 - Allow linting for responsive.html directory. r=pbrosset

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/31677/diff/1-2/
Comment on attachment 8710208 [details]
MozReview Request: Bug 1239437 - Add a basic viewport frame. r=pbrosset

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/31679/diff/1-2/
Comment on attachment 8710209 [details]
MozReview Request: Bug 1239437 - Hide iframe border. r=pbrosset

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/31681/diff/1-2/
Comment on attachment 8710210 [details]
MozReview Request: Bug 1239437 - Stir in React and Redux. r=pbrosset,jlongster

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/31683/diff/1-2/
Attachment #8710210 - Attachment description: MozReview Request: Bug 1239437 - Stir in React and Redux. r=pbrosset → MozReview Request: Bug 1239437 - Stir in React and Redux. r=pbrosset,jlongster
Attachment #8710210 - Flags: review?(pbrosset)
Attachment #8710210 - Flags: review?(jlong)
Comment on attachment 8710211 [details]
MozReview Request: Bug 1239437 - Allow run_test to be unused in xpcshell tests. r=pbrosset

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/31685/diff/1-2/
Comment on attachment 8710212 [details]
MozReview Request: Bug 1239437 - xpcshell tests for responsive.html. r=pbrosset

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/31687/diff/1-2/
Comment on attachment 8710213 [details]
MozReview Request: Bug 1239437 - Browser mochitests for responsive.html. r=pbrosset

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/31689/diff/1-2/
Attachment #8710213 - Flags: review?(pbrosset)
Comment on attachment 8710209 [details]
MozReview Request: Bug 1239437 - Hide iframe border. r=pbrosset

https://reviewboard.mozilla.org/r/31681/#review28899
Attachment #8710209 - Flags: review?(pbrosset) → review+
Attachment #8710213 - Flags: review?(pbrosset) → review+
Comment on attachment 8710213 [details]
MozReview Request: Bug 1239437 - Browser mochitests for responsive.html. r=pbrosset

https://reviewboard.mozilla.org/r/31689/#review28901

::: devtools/client/responsive.html/test/browser/browser_viewport_basics.js:6
(Diff revision 2)
> +/* TODO: Find out why the comment below is needed. */

head.js globals are auto-imported, but you're using waitUntilState here which comes from this shared-redux-head.js file. The auto head.js global importer doesn't recursively go to import-globals-from instructions to get globals from there too.
Comment on attachment 8710210 [details]
MozReview Request: Bug 1239437 - Stir in React and Redux. r=pbrosset,jlongster

https://reviewboard.mozilla.org/r/31683/#review28903
Attachment #8710210 - Flags: review?(pbrosset) → review+
Comment on attachment 8710210 [details]
MozReview Request: Bug 1239437 - Stir in React and Redux. r=pbrosset,jlongster

https://reviewboard.mozilla.org/r/31683/#review29041

Looks great to me, just several nits. Mostly coming from personal preferences that we need to settle on and codify into a style guide.

::: devtools/client/responsive.html/actions/index.js:15
(Diff revision 2)
> +  "CHANGE_LOCATION",

I prefer simple `module.exports.CHANGE_LOCATION = "CHANGE_LOCATION"`, no need for anything more, but again this is fine for now and we need to just decide on something and write it into docs.

::: devtools/client/responsive.html/components/viewports.js:25
(Diff revision 2)
> +    let children = viewports.map((viewport, index) => {

Style nit: keep everything as one structure:

return dom.div(
  { id: "viewports" },
  viewports.map((viewport, index) => {
    return Viewport({
      key: index,
      location,
      viewport,
    });
  })
);

Hard to see the structure if it's split up (this isn't complicated yet, but it's hard to follow if you start creating multiple variables).

::: devtools/client/responsive.html/index.js:43
(Diff revision 2)
> +    ReactDOM.unmountComponentAtNode(document.querySelector("#app"));

Do you really need to do this? If the page is going way, that should take care of it. There's nothing this really does except destroy the DOM.

I know you probably got this from the memory tool, but I don't think we need this shutdown logic. The page will go away by itself.

::: devtools/client/responsive.html/models.js:4
(Diff revision 2)
> +

Not sure I love this "models.js" file. At the very least it should be renamed, because "models" is a convoluted word for this. Models usually implies MVC, and this is not that at all.

I would just call this "prop-types.js".

Usually it's best to colocate prop types right on the component.

EDIT: I see now that most prop types are colocated on the component, and this is just for some shared types, which is good. I'd prefer to avoid making these too specific, though: I would just export the `location` and `viewport` shape type:

module.exports.viewport = PropTypes.shape({
  width: PropTypes.number.isRequired,
  height: PropTypes.number.isRequired,
});

module.exports.location = PropTypes.string.isRequired;


Components should compose these how they want, like "an array of viewports". I'm not sure about forcing these to be `isRequired` either; seems like something the component should do.

::: devtools/client/responsive.html/reducers/location.js:10
(Diff revision 2)
> +

Nit: I prefer just keeping it simple: 

const INITIAL_STATE = "about:blank";

function update(state = INITIAL_STATE, action) {
  switch(action.type) {
  case CHANGE_LOCATION:
    return action.location;
  }

  return state;
}

But again these are styles that we need to settle on and make into a style guide. I think at least naming the first parameter "state" is important, makes it consistent across reducers what "state" is.

::: devtools/client/responsive.html/store.js:29
(Diff revision 2)
> +    store.history = history;

Hm, I don't think we should be patching the store like this (again, I know this came from memory). If you want to use the store enhancer pattern: https://github.com/rackt/redux/blob/master/docs/Glossary.md#store-enhancer

This is fine for now though, I need to finish docs and coordinate with Jordan on this stuff.
Attachment #8710210 - Flags: review?(jlong) → review+
https://reviewboard.mozilla.org/r/31689/#review28901

> head.js globals are auto-imported, but you're using waitUntilState here which comes from this shared-redux-head.js file. The auto head.js global importer doesn't recursively go to import-globals-from instructions to get globals from there too.

Ah right!  I've filed bug 1242584 to fix this in our ESLint helpers.
https://reviewboard.mozilla.org/r/31683/#review29041

> I prefer simple `module.exports.CHANGE_LOCATION = "CHANGE_LOCATION"`, no need for anything more, but again this is fine for now and we need to just decide on something and write it into docs.

I just have a really strong aversion to repeating the key and the value.  (Where are enums when you want them?!)  I'll keep it as-is until we decree something.

> Style nit: keep everything as one structure:
> 
> return dom.div(
>   { id: "viewports" },
>   viewports.map((viewport, index) => {
>     return Viewport({
>       key: index,
>       location,
>       viewport,
>     });
>   })
> );
> 
> Hard to see the structure if it's split up (this isn't complicated yet, but it's hard to follow if you start creating multiple variables).

Makes sense, I've made these changes.

> Do you really need to do this? If the page is going way, that should take care of it. There's nothing this really does except destroy the DOM.
> 
> I know you probably got this from the memory tool, but I don't think we need this shutdown logic. The page will go away by itself.

Right, seems fine without it.  Removed.

> Not sure I love this "models.js" file. At the very least it should be renamed, because "models" is a convoluted word for this. Models usually implies MVC, and this is not that at all.
> 
> I would just call this "prop-types.js".
> 
> Usually it's best to colocate prop types right on the component.
> 
> EDIT: I see now that most prop types are colocated on the component, and this is just for some shared types, which is good. I'd prefer to avoid making these too specific, though: I would just export the `location` and `viewport` shape type:
> 
> module.exports.viewport = PropTypes.shape({
>   width: PropTypes.number.isRequired,
>   height: PropTypes.number.isRequired,
> });
> 
> module.exports.location = PropTypes.string.isRequired;
> 
> 
> Components should compose these how they want, like "an array of viewports". I'm not sure about forcing these to be `isRequired` either; seems like something the component should do.

The models naming is also borrowed from memory, not sure what their reasoning was.  Anyway, I agree it's overloaded, :pbro was confused too.  I'll rename it to `prop-types`.

And yes, it's just some shared types.  I'll change to export just the singletons like you describe.

> Nit: I prefer just keeping it simple: 
> 
> const INITIAL_STATE = "about:blank";
> 
> function update(state = INITIAL_STATE, action) {
>   switch(action.type) {
>   case CHANGE_LOCATION:
>     return action.location;
>   }
> 
>   return state;
> }
> 
> But again these are styles that we need to settle on and make into a style guide. I think at least naming the first parameter "state" is important, makes it consistent across reducers what "state" is.

I think I prefer the memory tool style for this, with separate object for each type, mostly to avoid the switch - case syntax.

Also, I started out call the first parameter `state` like you mention, but it seems confusing with these "sub-reducers" that act on only a portion on the total app state.  Since they don't receive the top-level state, it seems better to use a specific name that says what it is in more detail.  Memory does the same in some files, like `memory/reducers/diffing`.

Anyway, more things we decide on later.

> Hm, I don't think we should be patching the store like this (again, I know this came from memory). If you want to use the store enhancer pattern: https://github.com/rackt/redux/blob/master/docs/Glossary.md#store-enhancer
> 
> This is fine for now though, I need to finish docs and coordinate with Jordan on this stuff.

Right, it's from memory.  Happy to replace this with whatever else we agree on using.
Seems like nothing from the push from https://hg.mozilla.org/mozilla-central/rev/7dc87559627e to https://hg.mozilla.org/mozilla-central/rev/1508986bb0cb got marked as fixed.
Flags: needinfo?(cbook)
Iteration: --- → 47.1 - Feb 8
Priority: -- → P1
Flags: needinfo?(cbook)
Just to discuss redux style some more, responding to the points above. Definitely need to check out the memory tool and talk with others to resolve these things, but it's not super urgent and totally fine to do what you want :)

(In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #40)
> https://reviewboard.mozilla.org/r/31683/#review29041
> 
> > I prefer simple `module.exports.CHANGE_LOCATION = "CHANGE_LOCATION"`, no need for anything more, but again this is fine for now and we need to just decide on something and write it into docs.
> 
> I just have a really strong aversion to repeating the key and the value. 
> (Where are enums when you want them?!)  I'll keep it as-is until we decree
> something.

I have an aversion to the amount of tiny util libs that we'll create for stuff like this :) but this is definitely not that big of a deal right now.

> 
> > Nit: I prefer just keeping it simple: 
> > 
> > const INITIAL_STATE = "about:blank";
> > 
> > function update(state = INITIAL_STATE, action) {
> >   switch(action.type) {
> >   case CHANGE_LOCATION:
> >     return action.location;
> >   }
> > 
> >   return state;
> > }
> > 
> > But again these are styles that we need to settle on and make into a style guide. I think at least naming the first parameter "state" is important, makes it consistent across reducers what "state" is.
> 
> I think I prefer the memory tool style for this, with separate object for
> each type, mostly to avoid the switch - case syntax.

The thing I really like about using a single function is that it's immediately clear what's going on: this function is called for every single action type and you can do anything you want with it. For example, for debugging I put a `console.log` at the beginning of the function a lot of the time to log everything. I know right now you have a small util at the bottom I could put this in, but we'll probably abstract this out so I can't just put it in there.

Another example is debugging: I can set a breakpoint at the beginning of my simple `update` function and step through it.

https://github.com/mozilla/gecko-dev/blob/master/devtools/client/debugger/content/reducers/sources.js

Again, just the whole thing about being averse to more abstractions that make it harder to debug, but we'll keep talking about this!

> 
> Also, I started out call the first parameter `state` like you mention, but
> it seems confusing with these "sub-reducers" that act on only a portion on
> the total app state.  Since they don't receive the top-level state, it seems
> better to use a specific name that says what it is in more detail.  Memory
> does the same in some files, like `memory/reducers/diffing`.

It's ok when the state is a simple string, but it'll quickly evolve to a more generic object that has several properties. And then you start typing things like `location.location` which is a little silly. When you're working in a reducer it's easy to know the `state` is just the shape that `initialState` is at the top of the file.
QA Contact: mihai.boldan
I managed to verify this issue on Firefox 47.0a1 (2016-02-18), across platforms[1].
After performing the tests, I can confirm that the RDM opens correctly, from the menu and also using the keyboard shortcut (Cmd-Opt-M on Mac / Ctrl-Shift-M on Windows / Linux).

During test I observed some issues:
1. The RDM does not close if it's opened in a new tab (Ctrl +T). If the close command is used once, nothing happens and if the command is used for the second time, both RDM paga and the new tab page are blank.
2. If the RDM is enabled, the 'Responsive Design View' button from the menu is not checked.  
3. On Windows and Mac platforms, if you go to (e.g. https://www.mozilla.org/en-US/teach/smarton/?utm_source=directory-tiles&utm_medium=tiles&utm_content=PrivacyV1&utm_campaign=desktop) and enable the Toogle Tools, opening or closing the RDM results the disable of the Toogle Tools. 
All the issues described above are reproducible if the RDM is enable/disable from Menu or using the keyboard shortcut.

Should I log the bugs described above? 
Is there anything that should be tested around this bug?    

[1] WIndows 10 x86, Ubuntu 14.04 x86, Mac OS X 10.9.5
Flags: qe-verify+ → needinfo?(jryans)
(In reply to Mihai Boldan, QA [:mboldan] from comment #47)
> I managed to verify this issue on Firefox 47.0a1 (2016-02-18), across
> platforms[1].
> After performing the tests, I can confirm that the RDM opens correctly, from
> the menu and also using the keyboard shortcut (Cmd-Opt-M on Mac /
> Ctrl-Shift-M on Windows / Linux).
> 
> During test I observed some issues:
> 1. The RDM does not close if it's opened in a new tab (Ctrl +T). If the
> close command is used once, nothing happens and if the command is used for
> the second time, both RDM paga and the new tab page are blank.
> 2. If the RDM is enabled, the 'Responsive Design View' button from the menu
> is not checked.  
> 3. On Windows and Mac platforms, if you go to (e.g.
> https://www.mozilla.org/en-US/teach/smarton/?utm_source=directory-
> tiles&utm_medium=tiles&utm_content=PrivacyV1&utm_campaign=desktop) and
> enable the Toogle Tools, opening or closing the RDM results the disable of
> the Toogle Tools. 
> All the issues described above are reproducible if the RDM is enable/disable
> from Menu or using the keyboard shortcut.
> 
> Should I log the bugs described above? 
> Is there anything that should be tested around this bug?    
> 
> [1] WIndows 10 x86, Ubuntu 14.04 x86, Mac OS X 10.9.5

Hi Mihai, If Ryan recommends logging bugs as you described please have the new bugs block Bug 1237360 and please add [multiviewport] to the whiteboard.  Thanks.
(In reply to Marco Mucci [:MarcoM] from comment #48)

> Hi Mihai, If Ryan recommends logging bugs as you described please have the
> new bugs block Bug 1237360 and please add [multiviewport] to the whiteboard.
> Thanks.

Sure, will do.
(In reply to Mihai Boldan, QA [:mboldan] from comment #47)
> During test I observed some issues:
> 1. The RDM does not close if it's opened in a new tab (Ctrl +T). If the
> close command is used once, nothing happens and if the command is used for
> the second time, both RDM paga and the new tab page are blank.

Sure, let's file this.  It's expected for the moment, since our page <-> RDM handling is not very good yet.  We'll improve it in bug 1240900 and others, but let's file the issue so we can track it.

> 2. If the RDM is enabled, the 'Responsive Design View' button from the menu
> is not checked.  

Huh!  Good catch.  Yes, let's file this.

> 3. On Windows and Mac platforms, if you go to (e.g.
> https://www.mozilla.org/en-US/teach/smarton/?utm_source=directory-
> tiles&utm_medium=tiles&utm_content=PrivacyV1&utm_campaign=desktop) and
> enable the Toogle Tools, opening or closing the RDM results the disable of
> the Toogle Tools. 

This sounds like bug 1240912, so I think we've got that one on file.

> Is there anything that should be tested around this bug?    

Sounds like you've covered it, thanks!
Flags: needinfo?(jryans)
I logged Bug 1250080 and Bug 1250084 based on the Comment 50 and also logged Bug 1250091, found while testing.
Since there are logged bugs for the found issues, I'm marking this bug Verified-Fixed.
Status: RESOLVED → VERIFIED
status-firefox47: --- → verified
Whiteboard: [multiviewport] → [multiviewport] [mvp-rdm]
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: