1201475 - Implement DOM Panel

Status: RESOLVED FIXED

(DevTools :: DOM, defect)

Description

[Jan Honza Odvarko [:Honza] (always need-info? me)]

One of the Firebug gaps is DOM panel displaying structure of the current window (DOM). The structure is displayed as an expandable tree and is useful for inspection. It's also possible to search for specific field.

Honza

Comment 1

[Patrick Brosset <:pbro>]

There's work that has been done a long time ago in bug 704094. It wasn't far from landing, but I never got to finish it. I thought I would mention it here.

Comment 2

[James Long (:jlongster)]

Obvious concern: I know you're considering using React, but I'd rather just reuse whatever the inspector currently uses. Seems like it should do exactly the same thing, but just in a sidebar, right?

I really don't want to be in a place where we are just adding to the number of ways to write components (now there's a 4th model in react, etc). I'd like to *replace* things. In the short term all that means is wrapping current stuff in React components, because it's so easy to do. So anything we add, we need to also take away. If that means it's a lot easier to just build on what we have for now, we should do that.

Comment 3

[Jan Honza Odvarko [:Honza] (always need-info? me)]

(In reply to James Long (:jlongster) from comment #2)
> Obvious concern: I know you're considering using React, but I'd rather just
> reuse whatever the inspector currently uses. Seems like it should do exactly
> the same thing, but just in a sidebar, right?
By saying "it should do exactly the same thing" you have the VariablesView in mind?

> I really don't want to be in a place where we are just adding to the number
> of ways to write components (now there's a 4th model in react, etc). I'd
> like to *replace* things. In the short term all that means is wrapping
> current stuff in React components, because it's so easy to do. So anything
> we add, we need to also take away. If that means it's a lot easier to just
> build on what we have for now, we should do that.
Yes, I see the point.

The thing is that the DOM panel and consequently also the Console logging stuff (e.g. support for inline XHR details previews) could be so much better. And btw. especially the XHR previews in the Console panel has been requested many times. Eventually, I'd love to *replace* the VariablesView by this as well and make it extensible for extension developers (e.g. Angular folks). It needs more steps though since VariablesView supports inline editing.

Yes, it's yet another way how to build UI, but every one is on board, no? Also, my personal experience with Domplate in Firebug (Domplate == ReactJS) is great. Firebug has been rendering 100% of its UI on top of it for many years and it helped to keep the structure clear and extensible.

I've already made set of steps towards having templates (reps) that are able to render various data types (primitives, objects, arrays, various DOM types, etc.). The code is used by the existing DOM panel (in Firebug.next) and also by RDP Inspector, FireQuery, JSON Viewer (and will be also in upcoming Web Sockets monitor prototype).

Just to be clear, I don't want to push on this. And it's great if we have a better approach. It just feels that keep using old components (i.e. VariablesView in this case) holds the innovation process. I personally don't like to duplicate things, but replacing bigger parts of the system in one step doesn't have to be always the best way to go.

Honza

Comment 4

[James Long (:jlongster)]

You know I'm obviously on board with using React ;) Sorry if I came off too negative. I want to be cautious, but not too cautious.

I wasn't thinking of the variables view. Can you paste a screenshot of what Firebug's functionality is here and we are copying? I was imagining literally the inspector view, but just in a sidebar and showing just part of the DOM (top-level being the DOM now you are inspecting).

Definitely agree things could be so much better. The more I think about it, I'm not concerned about the technical problems with migrating, but just make sure that everyone else who uses those components know what's going on, and have input. What I don't want to happen is start using React for component X while someone else needs something done in style #2 for component X and we start diverging.

I'd like to see a clear list of all the places you intend to change, and make sure that we talk to owners of that code to make sure everyone is in the loop (you may already be done this!).

The other thing is that we are talking about building a One True Tree Widget, and since you have a tree widget we might want to take this slowly and make sure everything lines up. It would be cool if we use React, which I think will be the best way to make something generic, but if we do we'll probably need to change your widget quite a bit to make it based off of something really generic.

Not against this at all, just want to make sure it aligns with everything else.

Comment 5

[Sebastian Zartner [:sebo]]

(In reply to James Long (:jlongster) from comment #4)
> I wasn't thinking of the variables view. Can you paste a screenshot of what
> Firebug's functionality is here and we are copying? I was imagining
> literally the inspector view, but just in a sidebar and showing just part of
> the DOM (top-level being the DOM now you are inspecting).

Note that Firebug has two DOM panels. One is the side panel within its HTML panel[1], which is covered in bug 704094, and a main DOM panel[2], which this issue is about.

> The other thing is that we are talking about building a One True Tree
> Widget, and since you have a tree widget we might want to take this slowly
> and make sure everything lines up. It would be cool if we use React, which I
> think will be the best way to make something generic, but if we do we'll
> probably need to change your widget quite a bit to make it based off of
> something really generic.

If a really generic tree widget should be created, it should be done in a separate bug report blocking this one.

Sebastian

[1] https://getfirebug.com/wiki/index.php/HTML_Panel#DOM
[2] https://getfirebug.com/wiki/index.php/DOM_Panel

Comment 6

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Attachment: dom-panel.png

(In reply to James Long (:jlongster) from comment #4)
> You know I'm obviously on board with using React ;) Sorry if I came off too
> negative. I want to be cautious, but not too cautious.
> 
> I wasn't thinking of the variables view. Can you paste a screenshot of what
> Firebug's functionality is here and we are copying? I was imagining
> literally the inspector view, but just in a sidebar and showing just part of
> the DOM (top-level being the DOM now you are inspecting).
A screenshot showing the DOM panel from Firebug.next is attached.
Note the it displays the DOM as a set of properties (so, it's different from what the Inspector panel/view shows).
- It's e10s enabled
- It's based on React
- It uses RDP and grips to get data from the backend

> Definitely agree things could be so much better. The more I think about it,
> I'm not concerned about the technical problems with migrating, but just make
> sure that everyone else who uses those components know what's going on, and
> have input. What I don't want to happen is start using React for component X
> while someone else needs something done in style #2 for component X and we
> start diverging.
Agree

> I'd like to see a clear list of all the places you intend to change, and
> make sure that we talk to owners of that code to make sure everyone is in
> the loop (you may already be done this!).
The plan I've in mind is:
- Port Firebug's DOM (main) panel into devtools
- Share React templates with JSON View
- Use the code to implement XHR inline preview in the Console panel (JSON/XML/HTML previews)
- Use the same code to implement DOM (side) panel for the Inspector.

So, it's mainly s used to implement 'Firebug gaps'.

Who else we should get in this thread?

The code (e.g. the tree-widget) could be pretty stable after it's used at three places and we can start thinking about using it also in different parts of the code base. I don't know what other features are requiring a tree-widget, but one of the obvious long-term candidates is the VariablesView.

Plan B: if it's too soon (or there is any other blocker) to start having ReactJS based features (like the DOM panel), I can keep maintaining the DOM panel (and other features like XHR preview in the Console) in Firebug 3 aka Firebug.next. The important goal here is that Firebug users have those features available when Firebug 2 stops working due to e10s.

> The other thing is that we are talking about building a One True Tree
> Widget, and since you have a tree widget we might want to take this slowly
> and make sure everything lines up.
Yes

> It would be cool if we use React, which I
> think will be the best way to make something generic, but if we do we'll
> probably need to change your widget quite a bit to make it based off of
> something really generic.
Yes.

I don't personally like spending a lot of time and building super-truper-generic widget that does everything. I prefer building rather simpler and flexible widget, used it in several places/scenarios, gather requirements and let it evolve.

> Not against this at all, just want to make sure it aligns with everything
> else.

I see and I think it's reasonable!


(In reply to Sebastian Zartner [:sebo] from comment #5)
> Note that Firebug has two DOM panels. One is the side panel within its HTML
> panel[1], 
Note that I am talking about existing Firebug.next features I want to port to devtools.


Honza

Comment 7

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Attachment: bug1201475.patch

First working prototype attached.

- you need a patch from bug 1211525 (xhr inspector, some needed changes there)
- you need a patch from bug 1247065 (the tree component)

Summary:
* There is a new DOM panel in the Toolbox (should be on by default in Firebug theme only TBD)

* The Panel uses a tree component from bug 1247065

* The implementation has two main parts: chrome and content

* The Chrome part (panel): it's the DomPanel object itself, the one registered with the Toolbox. Plus there is a main.js (just a few lines). The main.js also needs chrome privileges since it uses BrowserLoader to load all the content modules into 'dom.xhtml' file. 

* The Content part (view): it's the 90% of the implementation, running entirely in content scope. It doesn't need any extra privileges and is responsible for rendering the content of the panel. It's the 'View' of the panel.

* The View doesn't have direct access to the DOM it's rendering (due to e10s) and so, it uses Grips received from the backend (debugger server). The view understands grips and knows how to render them using reps [2].

* The UI is based on React & Redux. There are two following actions: 'FETCH_PROPERTIES' and 'SET_VISIBILITY_FILTER' 

* The Tree can be filtered by a filter at the top right corner. The exact tree-filtering algorithm needs to be specified yet (only first level of props is filtered, since complete object should be visible after filtering, see bug 1217131).

---

Both parts (chrome/content) are loosely coupled. The communication between them is based on two events [1].

1) "Get me the root grip": the view needs a root grip to display. It's usually the |window| object, but the DOM panel can display also different roots (we might want to support a concept of breadcrumbs).

2) "Get me prototype-and-properties": the view needs to display object properties as soon as it's expanded by the user. It sends 'prototypeAndProperties' event to the panel, which sends the packet further to the backend over RDP and returns response back to the view. The view understands grips and so, can render them as object properties. The view actually understands many types of grips and so, can render different types differently.

---

And here is the best part. Since the View (content scope) communicates with the chrome using events, we can simply replace these events by XHR and use HTTP server instead of the chrome. The HTTP server consequently forwards the request to the backend (debugger server) and sends the response packet back to the view. This way the entire 'View' (content scope) can run within a web page and our own devtools can be used for the development. For me this is great first step towards devtools.html -> make panels contents (views) pure.


I'd very much like to discuss this concept of how we can implement panels UI in the new world of React/Redux.


I am close to asking for a review (only code cleanup needed). I am thinking about: UI/UX: Helen, React/Redux: James/Lin?, integration: Brian?


Honza


[1] There is actually an exposed function from chrome to content atm, but I'd like to fix it.
[2] Reps is a set of components coming from the 'jsonview' directory. It'll be moved to shared/components/reps as soon as bug 1240494 is resolved

Comment 8

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Attachment: bug1201475-server.patch

I am also attaching the HTTP server that can be used to develop the panel-view as a webapp. Of course, it won't be part of the distribution, it's just for anyone interested.
STR: You need to build a bundle.js using webpack from within the dom directory (there is webpack.config.js), start Firefox, select the DOM panel and open dom/content/index.html in your web server.

I can provider more information if anyone is actually interested.

Honza

Comment 9

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Attachment: bug1201475.patch

Patch rebased on latest changes
Only code clean up needed and then ready for review.

Honza

Comment 10

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Helen, we'll need an icon for the new DOM panel.
Any tips? :-)

Honza

Comment 11

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Attachment: bug1201475.patch

James, since you are also involved in the tree-component, I am assigning you for the review here. The DOM panel is mostly based on the tree-component (bug 1247065) and shows nicely requirements (including API for customization) for the tree.

You might test in the Toolbox as well as in the Browser Toolbox!

Summary:
- There is a new DOM panel visualizing DOM of the current debugger target
- You can expand/collapse individual nodes
- You can filter (I'll file a followup for better algorithm that will also be used for the JSON Viewer).
- You can Refresh the panel by clicking on the Refresh button

* I need to figure out localization yet so, please ignore this in your review

The DOM panel UI (the View) is running inside a frame with no extra privileges. The only exception is main.js that's using the BrowserLoader -> to load DOM panel content (the View). This is great first step towards devtools.html
See also comment #7.

Thanks!

Honza

Comment 12

[Jan Honza Odvarko [:Honza] (always need-info? me)]

@Lin: I assigned you for the review here (mentioned this on our meeting).

Interesting things:
- Tree
- React/Redux
- Reps (could be reused in the Console panel)
- No chrome privileges

Honza

Comment 13

[Helen V. Holmes (:helenvholmes) (:✨)]

Attachment: dom-panel.svg

How would you feel about something like this Honza?

Pretty simple, but it gets the idea across (I think).

Comment 14

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Attachment: bug1201475.patch

Patch updated:

* Localization
* Code-style updated (using es6 classes instead of createClass)
* New panel icon

Of course, you also need a patch from bug 1247065 (the tree).

Honza

Comment 15

[Jan Honza Odvarko [:Honza] (always need-info? me)]

(In reply to Helen V. Holmes (:helenvholmes) (:✨) from comment #13)
> Created attachment 8724679 [details]
> dom-panel.svg
Thanks for the icon Helen, looks good to me!
What about emoji icon, should we also have one?

Honza

Comment 16

[Helen V. Holmes (:helenvholmes) (:✨)]

(In reply to Jan Honza Odvarko [:Honza] from comment #15)
> (In reply to Helen V. Holmes (:helenvholmes) (:✨) from comment #13)
> > Created attachment 8724679 [details]
> > dom-panel.svg
> Thanks for the icon Helen, looks good to me!
> What about emoji icon, should we also have one?
> 
> Honza

Comment 17

[Helen V. Holmes (:helenvholmes) (:✨)]

Bugzilla didn't recognize the emoji I tried to use, haha. This is what I meant to say: http://cl.ly/1V462a3V100x

Comment 18

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Attachment: bug1201475.patch

Patch updated, React.createClass() used.

(In reply to Helen V. Holmes (:helenvholmes) (:✨)(Unavailable until 3/14)) from comment #17)
> Bugzilla didn't recognize the emoji I tried to use, haha. This is what I
> meant to say: http://cl.ly/1V462a3V100x
Good, and where I can get the SVG file?

Honza

Comment 19

[James Long (:jlongster)]

Comment on attachment 8727490 [details] [diff] [review]
bug1201475.patch

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

::: devtools/client/dom/content/containers/main-frame.js
@@ +86,5 @@
> +  };
> +};
> +
> +// Exports from this module
> +exports.MainFrame = connect(mapStateToProps)(MainFrame);

Please change all the components in here too to only export a single component, not an object.

Comment 20

[James Long (:jlongster)]

Jan, you should really try hot reloading as well if you are working on components. Just set `devtools.loader.hotreload` to true and you should be able to change any React component (save the file) and it'll update automatically.

You can also do this with CSS. Unfortunately it does not work with @imports, which I see you use. imports aren't great to use anyway, because they block the world to go download and parse the CSS file. I'd recommend using <link> tags. It's worth it for hot reloading anyway.

I'm also working on a way to dynamically include a component's CSS automatically, so as the user you wouldn't have to do anything.

Comment 21

[James Long (:jlongster)]

While hacking on my patch for bug 1247065, I noticed several things about this tool:

* If the value in a row is quite large, it makes the whole row big, and the label is vertically centered. See here: http://jlongster.com/s/dom-panel/1.png

That is trying to display a huge object in the right column and making the row huge. However, if you expand that node, it no longer shows the value in the right column and is showing all the object as child nodes. Because the value goes away from the row, the row becomes a normal height again, seen here: http://jlongster.com/s/dom-panel/2.png

It's quite jarring to expand something and have the UI shift around like that.

* For large objects, there seems to be a delay when expanding the object. And during this delayed time (sometimes up to ~1 second), the label is gone and the value is "undefined". Worse, if there is an error fetching the expansion (which seems to happen more often than it should) the node is left in this state permanently: http://jlongster.com/s/dom-panel/3.png

* When fetching the expanded rows fails, the whole UI is broken (nothing responds). It seems quite easy for it to get in this state.

* The styles right now seem very Firebug-specific, but I'm sure you're going to fix that

* The top of the table seems to be slightly cut off: http://jlongster.com/s/dom-panel/4.png (can't scroll up any more)

* When I scrolled pretty far down, the value column started appearing on top of the toolbar: http://jlongster.com/s/dom-panel/4.png

Those are the things that stood out to me. To be quite honest, what is the goal of this panel? This is basically just a VariablesView of the builtin objects? I don't really know what I would use this for, but I understand that we need it for Firebug compatibility.

I'll do a close review of this patch (and the tree view patch) once it gets more stabilized.

Comment 22

[James Long (:jlongster)]

Oops, the last link about the values appearing on top of the toolbar should have linked here: http://jlongster.com/s/dom-panel/5.png

Comment 23

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Attachment: bug1201475.patch

(In reply to James Long (:jlongster) from comment #20)
> Jan, you should really try hot reloading as well if you are working on
> components. Just set `devtools.loader.hotreload` to true and you should be
> able to change any React component (save the file) and it'll update
> automatically.
I tried, but it doesn't work for me.
(changes I am doing in e.g. DomTree are not re-loaded)

I am seeing the following error:
Error: Stack overflow require.js %20line%20139%20%3E%20Function:364:0

But re my comment #7.
I like what you have done to support auto-reload!
But having the option to load panel (or later entire toolbox) content
in a web-page just like a web-app and use *our* tools (i.e. the Toolbox,
not the Browser Toolbox) for development (+hot reloading for free)
is just really really great for the development experience.

> You can also do this with CSS. Unfortunately it does not work with @imports,
> which I see you use. imports aren't great to use anyway, because they block
> the world to go download and parse the CSS file. I'd recommend using <link>
> tags. It's worth it for hot reloading anyway.
Agree, done!

> I'm also working on a way to dynamically include a component's CSS
> automatically, so as the user you wouldn't have to do anything.
Sounds great, is there anything I could see yet?


(In reply to James Long (:jlongster) from comment #21)
> While hacking on my patch for bug 1247065, I noticed several things about
> this tool:
> 
> * If the value in a row is quite large, it makes the whole row big, and the
> label is vertically centered. See here:
> http://jlongster.com/s/dom-panel/1.png
> 
> That is trying to display a huge object in the right column and making the
> row huge. However, if you expand that node, it no longer shows the value in
> the right column and is showing all the object as child nodes. Because the
> value goes away from the row, the row becomes a normal height again, seen
> here: http://jlongster.com/s/dom-panel/2.png
> 
> It's quite jarring to expand something and have the UI shift around like
> that.
This has been requested for the JSON Viewer (bug 1244912), but I
understand what you saying and I've removed the feature for now.

 
> * For large objects, there seems to be a delay when expanding the object.
> And during this delayed time (sometimes up to ~1 second), the label is gone
> and the value is "undefined". Worse, if there is an error fetching the
> expansion (which seems to happen more often than it should) the node is left
> in this state permanently: http://jlongster.com/s/dom-panel/3.png
Fixed (the fix will be part of the tree-component, bug 1247065)

> * When fetching the expanded rows fails, the whole UI is broken (nothing
> responds). It seems quite easy for it to get in this state.
I think I fixed it (attaching separate patch here for it)

> * The styles right now seem very Firebug-specific, but I'm sure you're going
> to fix that
What exactly do you think should be changed?

I already made some support for themes, here are two screenshots:
1) Firebug theme: http://janodvarko.cz/temp/dom-panel/firebug-theme.png
2) Light theme: http://janodvarko.cz/temp/dom-panel/light-theme.png

> 
> * The top of the table seems to be slightly cut off:
> http://jlongster.com/s/dom-panel/4.png (can't scroll up any more)
Fixed 

> 
> * When I scrolled pretty far down, the value column started appearing on top
> of the toolbar: http://jlongster.com/s/dom-panel/4.png
Fixed

> 
> Those are the things that stood out to me. To be quite honest, what is the
> goal of this panel? This is basically just a VariablesView of the builtin
> objects? I don't really know what I would use this for, but I understand
> that we need it for Firebug compatibility.
Yes, Firebug compatibility. Also, I believe we should use it for DOM side
panel in the Inspector panel and for new Variables view (built on top of React). 
I don't think it would be effective strategy to duplicate the code.

> I'll do a close review of this patch (and the tree view patch) once it gets
> more stabilized.
Thanks James.

Honza

Comment 24

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Attachment: bug1201475-fix-reps.patch

Here is the fix for the UI break.

Honza

Comment 25

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Attachment: bug1201475-fix-reps.patch

Yet fix commit message.

Honza

Comment 26

[James Long (:jlongster)]

(In reply to Jan Honza Odvarko [:Honza] from comment #23)
> Created attachment 8727826 [details] [diff] [review]
> bug1201475.patch
> 
> (In reply to James Long (:jlongster) from comment #20)
> > Jan, you should really try hot reloading as well if you are working on
> > components. Just set `devtools.loader.hotreload` to true and you should be
> > able to change any React component (save the file) and it'll update
> > automatically.
> I tried, but it doesn't work for me.
> (changes I am doing in e.g. DomTree are not re-loaded)

Because you're exporting the component attached to an object (`{ DomTree: DomTree }`). Just export the component itself and it'll work.

> 
> I am seeing the following error:
> Error: Stack overflow require.js %20line%20139%20%3E%20Function:364:0
> 
> But re my comment #7.
> I like what you have done to support auto-reload!
> But having the option to load panel (or later entire toolbox) content
> in a web-page just like a web-app and use *our* tools (i.e. the Toolbox,
> not the Browser Toolbox) for development (+hot reloading for free)
> is just really really great for the development experience.

Oh I completely agree there. You don't get hot reloading for free though, hot reloading is completely different. You can F5 to refresh, which is awesome and we totally need to get there. But hot reloading is even a step further, and reduces development time even more: just save the react file and it'll update the existing components on the page, but keep the state. This means you can expand the tree and then make changes to see how the expanded tree changes.

I did this when switching the tree from tables to divs and it's so much faster. Try it out, it's hard to go back! (It should work also just when developing it in a normal page, assuming you're still using BrowserLoader)

> 
> > You can also do this with CSS. Unfortunately it does not work with @imports,
> > which I see you use. imports aren't great to use anyway, because they block
> > the world to go download and parse the CSS file. I'd recommend using <link>
> > tags. It's worth it for hot reloading anyway.
> Agree, done!

Thanks for making these changes!

> 
> > I'm also working on a way to dynamically include a component's CSS
> > automatically, so as the user you wouldn't have to do anything.
> Sounds great, is there anything I could see yet?
> 

The only thing I have right now is a gist: https://gist.github.com/jlongster/a93aa8dabce06ac9392a But I'm working on the debugger now and will need this soon so I'll let you know.

> 
> (In reply to James Long (:jlongster) from comment #21)
> > While hacking on my patch for bug 1247065, I noticed several things about
> > this tool:
> > 
> > * If the value in a row is quite large, it makes the whole row big, and the
> > label is vertically centered. See here:
> > http://jlongster.com/s/dom-panel/1.png
> > 
> > That is trying to display a huge object in the right column and making the
> > row huge. However, if you expand that node, it no longer shows the value in
> > the right column and is showing all the object as child nodes. Because the
> > value goes away from the row, the row becomes a normal height again, seen
> > here: http://jlongster.com/s/dom-panel/2.png
> > 
> > It's quite jarring to expand something and have the UI shift around like
> > that.
> This has been requested for the JSON Viewer (bug 1244912), but I
> understand what you saying and I've removed the feature for now.

I'm not sure what the best UX is but as long as the arrow toggle doesn't move around it's ok.

> 
>  
> > * For large objects, there seems to be a delay when expanding the object.
> > And during this delayed time (sometimes up to ~1 second), the label is gone
> > and the value is "undefined". Worse, if there is an error fetching the
> > expansion (which seems to happen more often than it should) the node is left
> > in this state permanently: http://jlongster.com/s/dom-panel/3.png
> Fixed (the fix will be part of the tree-component, bug 1247065)

Thanks!

> 
> > * When fetching the expanded rows fails, the whole UI is broken (nothing
> > responds). It seems quite easy for it to get in this state.
> I think I fixed it (attaching separate patch here for it)

I'll try it out

> > * The styles right now seem very Firebug-specific, but I'm sure you're going
> > to fix that
> What exactly do you think should be changed?
> 
> I already made some support for themes, here are two screenshots:
> 1) Firebug theme: http://janodvarko.cz/temp/dom-panel/firebug-theme.png
> 2) Light theme: http://janodvarko.cz/temp/dom-panel/light-theme.png

I think it's mainly the font, we don't use that font anywhere right now.

> > 
> > * The top of the table seems to be slightly cut off:
> > http://jlongster.com/s/dom-panel/4.png (can't scroll up any more)
> Fixed 
> 
> > 
> > * When I scrolled pretty far down, the value column started appearing on top
> > of the toolbar: http://jlongster.com/s/dom-panel/4.png
> Fixed

Sweet!

> 
> > 
> > Those are the things that stood out to me. To be quite honest, what is the
> > goal of this panel? This is basically just a VariablesView of the builtin
> > objects? I don't really know what I would use this for, but I understand
> > that we need it for Firebug compatibility.
> Yes, Firebug compatibility. Also, I believe we should use it for DOM side
> panel in the Inspector panel and for new Variables view (built on top of
> React). 
> I don't think it would be effective strategy to duplicate the code.

Ok cool. How is this different from a variables view of the global scope though? Is there anything special about it?

Also, I think we shouldn't be too concerned about duplicating code. We are going to have components that are similar in nature but they don't actually duplicate code, and even if they do it's a small amount. Especially since we are at the beginning of writing shared components we should write the components we think make sense and then try to merge abstractions where we see fit. It's a lot better to try things out a few different ways first. For example, I'm working on the debugger and need several components, the variables view being on of them, and I don't know what I'm going to do yet but I might show a simple expandable list component. We'll work through how to abstract out behaviors into components so that we can share them, not sure what that looks like yet.

I'm not convinced this is appropriate for the VariablesView; that doesn't have any columns and is a simple expandable list.

Thanks for being so quick to make changes!

Comment 27

[James Long (:jlongster)]

Comment on attachment 8727831 [details] [diff] [review]
bug1201475-fix-reps.patch

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

Feel free to just merge this in with the other patch

Comment 28

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Attachment: bug1201475.patch

(In reply to James Long (:jlongster) from comment #26)
> Because you're exporting the component attached to an object (`{ DomTree:
> DomTree }`). Just export the component itself and it'll work.
Ah, I forgot to do this in the previous patch, it's done now.

Still not working for me and I am still seeing
Error: Stack overflow require.js %20line%20139%20%3E%20Function:364:0
(btw. I am on Win)
> Oh I completely agree there. You don't get hot reloading for free though,
> hot reloading is completely different. You can F5 to refresh, which is
> awesome and we totally need to get there. But hot reloading is even a step
> further,
I know, by saying for free, I meant when we are already using webpack
(which I am in my scenario described in comment #7) it's straightforward
to also use webpack-dev-server and plugins...

> The only thing I have right now is a gist:
> https://gist.github.com/jlongster/a93aa8dabce06ac9392a But I'm working on
> the debugger now and will need this soon so I'll let you know.
Yep, I've seen that gist.

> I think it's mainly the font, we don't use that font anywhere right now.
Fixed

> It's a lot better to try things out a few different ways first.
I agree with that.

(In reply to James Long (:jlongster) from comment #27)
> Feel free to just merge this in with the other patch
Done

Honza

Comment 29

[James Long (:jlongster)]

(In reply to Jan Honza Odvarko [:Honza] from comment #28)
> Created attachment 8728024 [details] [diff] [review]
> bug1201475.patch
> 
> (In reply to James Long (:jlongster) from comment #26)
> > Because you're exporting the component attached to an object (`{ DomTree:
> > DomTree }`). Just export the component itself and it'll work.
> Ah, I forgot to do this in the previous patch, it's done now.
> 
> Still not working for me and I am still seeing
> Error: Stack overflow require.js %20line%20139%20%3E%20Function:364:0
> (btw. I am on Win)

Oh, right it does not work on Windows right now because it doesn't use symlinks. Need to figure out something for that.

> > Oh I completely agree there. You don't get hot reloading for free though,
> > hot reloading is completely different. You can F5 to refresh, which is
> > awesome and we totally need to get there. But hot reloading is even a step
> > further,
> I know, by saying for free, I meant when we are already using webpack
> (which I am in my scenario described in comment #7) it's straightforward
> to also use webpack-dev-server and plugins...

Oh I didn't know you were using webpack. I see what you mean now! (I'm pretty sure react-hot-loader requires the same thing though about exporting the component itself)

Comment 30

[Jan Honza Odvarko [:Honza] (always need-info? me)]

(In reply to James Long (:jlongster) from comment #29)
> (In reply to Jan Honza Odvarko [:Honza] from comment #28)
> > Created attachment 8728024 [details] [diff] [review]
> > bug1201475.patch
> > 
> > (In reply to James Long (:jlongster) from comment #26)
> > > Because you're exporting the component attached to an object (`{ DomTree:
> > > DomTree }`). Just export the component itself and it'll work.
> > Ah, I forgot to do this in the previous patch, it's done now.
> > 
> > Still not working for me and I am still seeing
> > Error: Stack overflow require.js %20line%20139%20%3E%20Function:364:0
> > (btw. I am on Win)
> 
> Oh, right it does not work on Windows right now because it doesn't use
> symlinks. Need to figure out something for that.
OK, I see, let me know when I should test it again.

> > > Oh I completely agree there. You don't get hot reloading for free though,
> > > hot reloading is completely different. You can F5 to refresh, which is
> > > awesome and we totally need to get there. But hot reloading is even a step
> > > further,
> > I know, by saying for free, I meant when we are already using webpack
> > (which I am in my scenario described in comment #7) it's straightforward
> > to also use webpack-dev-server and plugins...
> 
> Oh I didn't know you were using webpack. I see what you mean now! (I'm
> pretty sure react-hot-loader requires the same thing though about exporting
> the component itself)
Yep, and when we are using all that we don't need to develop/maintain
any additional infrastructure (just focus on properly written React component).

Try push:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=5ff7e3bb68eb

Honza

Comment 31

[James Long (:jlongster)]

Comment on attachment 8728024 [details] [diff] [review]
bug1201475.patch

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

::: devtools/client/dom/content/actions/grips.js
@@ +37,5 @@
> + * Used to get properties from the backend and fire an action
> + * when they are received.
> + */
> +function fetchProperties(grip) {
> +  return function(dispatch) {

Arrow functions are a little cleaner: `dispatch => { ... }`

::: devtools/client/dom/content/containers/dom-tree.js
@@ +66,5 @@
> +        mode: "short",
> +        defaultRep: Grip,
> +        columns: columns,
> +        renderValue: renderValue,
> +        onFilter: this.onFilter.bind(this)

You don't need `bind`, `createClass` methods are auto-bound to `this` already

@@ +74,5 @@
> +});
> +
> +// This is the integration poin with Reps. The DomTree is using
> +// Reps to render all values.
> +var renderValue = props => {

Why have this outside the component? I'd just embed this small function in the `renderValue` property above inside `render`

::: devtools/client/dom/content/containers/main-toolbar.js
@@ +1,1 @@
> +/* -*- indent-tabs-mode: nil; js-indent-level: 2 -*- */

I recommend getting rid of the `containers` folder and moving everything to components. I think it's unnecessary and adds yet another term people have to learn. In your case there's hardly anything in the components directly. It might make sense it things get out of control, but I'd rather reduce the amount of terms we have floating around.

::: devtools/client/dom/content/reducers/filter.js
@@ +10,5 @@
> +/**
> + * Initial state definition
> + */
> +function getInitialState() {
> +  return "";

It's quite strange for the state of a reducer to be just a string. I think you should combine your reducers into a single reducer that is an object with properties representing this state. Your state is not very complex, so you probably don't need multiple reducers (yet). No need to split it up so much.

::: devtools/client/dom/content/store/configure-store.js
@@ +1,1 @@
> +/* -*- indent-tabs-mode: nil; js-indent-level: 2 -*- */

Why a whole separate directory for this? Just do this work somewhere in your init file. No need for a whole `store` directory. There's also a `thunk` file in here that you don't use.

::: devtools/client/dom/dom-panel.js
@@ +121,5 @@
> +    let deferred = defer();
> +
> +    // We need the thread object to get client object for grips
> +    // (using threadClient.pauseGrip).
> +    this.target.activeTab.attachThread({}, (response, threadClient) => {

Please don't attach the thread again! I'm actually not sure what happens when the thread is attached twice, but the toolbox has already attached it! You can get the current thread with `this._toolbox.threadClient`.

@@ +126,5 @@
> +      this.threadClient = threadClient;
> +
> +      // Attach Console. It might involve RDP communication, so wait
> +      // asynchronously for the result
> +      this.attachConsole(threadClient).then(consoleClient => {

Do you really need the console for this? There's no way to eval with the debugger API?

Does this use the existing console or attach another one? If the latter are there any implications of that?

Comment 32

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Attachment: bug1201475.patch

(In reply to James Long (:jlongster) from comment #31)
> Comment on attachment 8728024 [details] [diff] [review]
> bug1201475.patch
> 
> Review of attachment 8728024 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: devtools/client/dom/content/actions/grips.js
> @@ +37,5 @@
> > + * Used to get properties from the backend and fire an action
> > + * when they are received.
> > + */
> > +function fetchProperties(grip) {
> > +  return function(dispatch) {
> 
> Arrow functions are a little cleaner: `dispatch => { ... }`
Done

> 
> ::: devtools/client/dom/content/containers/dom-tree.js
> @@ +66,5 @@
> > +        mode: "short",
> > +        defaultRep: Grip,
> > +        columns: columns,
> > +        renderValue: renderValue,
> > +        onFilter: this.onFilter.bind(this)
> 
> You don't need `bind`, `createClass` methods are auto-bound to `this` already
Done

> 
> @@ +74,5 @@
> > +});
> > +
> > +// This is the integration poin with Reps. The DomTree is using
> > +// Reps to render all values.
> > +var renderValue = props => {
> 
> Why have this outside the component? I'd just embed this small function in
> the `renderValue` property above inside `render`
True, done.

> 
> ::: devtools/client/dom/content/containers/main-toolbar.js
> @@ +1,1 @@
> > +/* -*- indent-tabs-mode: nil; js-indent-level: 2 -*- */
> 
> I recommend getting rid of the `containers` folder and moving everything to
> components. I think it's unnecessary and adds yet another term people have
> to learn. In your case there's hardly anything in the components directly.
> It might make sense it things get out of control, but I'd rather reduce the
> amount of terms we have floating around.
Done

> 
> ::: devtools/client/dom/content/reducers/filter.js
> @@ +10,5 @@
> > +/**
> > + * Initial state definition
> > + */
> > +function getInitialState() {
> > +  return "";
> 
> It's quite strange for the state of a reducer to be just a string. I think
> you should combine your reducers into a single reducer that is an object
> with properties representing this state. Your state is not very complex, so
> you probably don't need multiple reducers (yet). No need to split it up so
> much.
I kept this as it is (after our conversation on IRC)

> 
> ::: devtools/client/dom/content/store/configure-store.js
> @@ +1,1 @@
> > +/* -*- indent-tabs-mode: nil; js-indent-level: 2 -*- */
> 
> Why a whole separate directory for this? Just do this work somewhere in your
> init file. No need for a whole `store` directory. There's also a `thunk`
> file in here that you don't use.
Dir removed

> 
> ::: devtools/client/dom/dom-panel.js
> @@ +121,5 @@
> > +    let deferred = defer();
> > +
> > +    // We need the thread object to get client object for grips
> > +    // (using threadClient.pauseGrip).
> > +    this.target.activeTab.attachThread({}, (response, threadClient) => {
> 
> Please don't attach the thread again! I'm actually not sure what happens
> when the thread is attached twice, but the toolbox has already attached it!
> You can get the current thread with `this._toolbox.threadClient`.
> 
> @@ +126,5 @@
> > +      this.threadClient = threadClient;
> > +
> > +      // Attach Console. It might involve RDP communication, so wait
> > +      // asynchronously for the result
> > +      this.attachConsole(threadClient).then(consoleClient => {
> 
> Do you really need the console for this? There's no way to eval with the
> debugger API?
> 
> Does this use the existing console or attach another one? If the latter are
> there any implications of that?
I used evaluateJSAsync and existing console client.

Thanks for the review!

Honza

Comment 33

[Helen V. Holmes (:helenvholmes) (:✨)]

Here's the Firefox OS bug emoji, which would be easiest: https://github.com/mozilla/fxemoji/blob/gh-pages/svgs/nature/u1F41B-bug.svg

Comment 34

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Attachment: bug1201475.patch

Just including the emoji icon.

Honza

Comment 35

[James Long (:jlongster)]

Going through this again now, but for something this big it's probably good to go ahead and get another person to look at it too. Not sure who, maybe bgrins, linclark, jryans?

Comment 36

[James Long (:jlongster)]

Comment on attachment 8717661 [details] [diff] [review]
bug1201475-server.patch

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

::: devtools/client/dom/httpd.js
@@ +1,1 @@
> +/* This Source Code Form is subject to the terms of the Mozilla Public

What is this patch, can you explain? I'm not sure this belongs in the tree.

Comment 37

[James Long (:jlongster)]

Comment on attachment 8730196 [details] [diff] [review]
bug1201475.patch

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

Looks generally fine to me, I hope I haven't missed something because it's a lot of code. Still a few nits but most of them aren't a big deal. My biggest worry is the lack of tests. I don't think we should over-test this early on, because we want to be open to changing things this early on, but we should have at least some tests of the basic functionality. Please write at least some mochitests before landing this (which you may be planning on doing since you created the test dir). I'll go ahead about r+.

::: devtools/client/dom/content/store.js
@@ +10,5 @@
> +
> +// WebSockets Monitor
> +const { rootReducer } = require("./reducers/index");
> +
> +function configureStore(initialState) {

I still don't really see the point of separating this out into a separate file. Why not just do this in `dom-view.js`? If you want to keep this file, "configure" doesn't really match what this is doing, this is actually creating the store. You could call this `createStore` like the shared lib does, although that might be confusing.

I think it's clearer just to have this in `dom-view.js` (and is common pattern that I think other tools do):

const createStore = require("devtools/client/shared/redux/create-store")({
  log: false
});
const { reducers } = require("./reducers/index");
const store = createStore(combineReducers(reducers));

This made me realize that you are also calling `combineReducers` in `reducers/index.js`. Please call it here instead; if anything that's the common pattern in the redux community. But you might also include `index.js` in tests and it should just export a simple object.

::: devtools/client/dom/test/browser.ini
@@ +1,5 @@
> +[DEFAULT]
> +tags = devtools
> +subsuite = devtools
> +support-files =
> +  head.js

Some tests would be nice :)

::: devtools/client/shared/components/reps/attribute.js
@@ +1,1 @@
> +/* -*- indent-tabs-mode: nil; js-indent-level: 2 -*- */

It worries me that none of the reps have xpcshell tests to test basic functionality. Maybe that's ok because we should use them a little bit more to make sure how they currently work will work across tools, but can you file a new bug to write at least some tests against them? No need to go too extreme, but having some would be good.

::: devtools/client/shared/components/reps/grip.js
@@ +85,5 @@
> +
> +    // Object members with non-empty values are preferred since it gives the
> +    // user a better overview of the object.
> +    let props = [];
> +    this.getProps(props, object, max, isInterestingProp);

Please avoid the C-style passing in of mutable props. Just return it. If this exists in the other reps, please update those as well. (seems like every rep is a similar style and there are a lot of new ones here so I'm not going into detail on every single one)

@@ +91,5 @@
> +    if (props.length <= max) {
> +      // There are not enough props yet (or at least, not enough props to
> +      // be able to know whether we should print "more..." or not).
> +      // Let's display also empty members and functions.
> +      this.getProps(props, object, max, function(t, value) {

nit: prefer arrow functions: `(t, value) => {...}` (not going to point out every place where this is applicable)

@@ +97,5 @@
> +      });
> +    }
> +
> +    if (props.length > max) {
> +      props.pop();

This doesn't seem right. If the length is greater than the max you only pop of a single item? Don't you need to do `slice` instead to make sure the new array is within the max length?

::: devtools/client/shared/components/reps/named-node-map.js
@@ +119,5 @@
> +let PropRep = React.createFactory(React.createClass({
> +  displayName: "PropRep",
> +
> +  render: function() {
> +    const { Rep } = createFactories(require("./rep"));

I'd rather not have a circular dependency (which forces a `require` in a render function). Can you instead take a `GenericRep` component as a property and the renderer should pass the Rep component in?

::: devtools/client/shared/components/reps/object.js
@@ +62,5 @@
>  
>      propIterator: function(object, max) {
>        function isInterestingProp(t, value) {
> +        // Do not pick objects, it could cause recursion.
> +        return (t == "boolean" || t == "number" || (t == "string" && value));

What does this change in the UI? Hard to tell. Are you sure this doesn't change anything important in the JSON viewer?

Comment 38

[Jan Honza Odvarko [:Honza] (always need-info? me)]

(In reply to James Long (:jlongster) from comment #36)
> Comment on attachment 8717661 [details] [diff] [review]
> bug1201475-server.patch
> 
> Review of attachment 8717661 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: devtools/client/dom/httpd.js
> @@ +1,1 @@
> > +/* This Source Code Form is subject to the terms of the Mozilla Public
> 
> What is this patch, can you explain? I'm not sure this belongs in the tree.
It was just an example of how we can develop panel/tool UI as a web app and access DevTools backend through HTTP server - I had really really good developer flow experience on top of it. Not intended for the tree tho, just an experiment, marked as obsolete now.

Honza

Comment 39

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Attachment: bug1201475.patch

(In reply to James Long (:jlongster) from comment #37)
> Comment on attachment 8730196 [details] [diff] [review]
> bug1201475.patch
> 
> Review of attachment 8730196 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Looks generally fine to me, I hope I haven't missed something because it's a
> lot of code. Still a few nits but most of them aren't a big deal. My biggest
> worry is the lack of tests. I don't think we should over-test this early on,
> because we want to be open to changing things this early on, but we should
> have at least some tests of the basic functionality. Please write at least
> some mochitests before landing this (which you may be planning on doing
> since you created the test dir).
Yes, I'll make some.

> I'll go ahead about r+.
> 
> ::: devtools/client/dom/content/store.js
> @@ +10,5 @@
> > +
> > +// WebSockets Monitor
> > +const { rootReducer } = require("./reducers/index");
> > +
> > +function configureStore(initialState) {
> 
> I still don't really see the point of separating this out into a separate
> file. Why not just do this in `dom-view.js`? If you want to keep this file,
> "configure" doesn't really match what this is doing, this is actually
> creating the store. You could call this `createStore` like the shared lib
> does, although that might be confusing.
> 
> I think it's clearer just to have this in `dom-view.js` (and is common
> pattern that I think other tools do):
> 
> const createStore = require("devtools/client/shared/redux/create-store")({
>   log: false
> });
> const { reducers } = require("./reducers/index");
> const store = createStore(combineReducers(reducers));
> 
> This made me realize that you are also calling `combineReducers` in
> `reducers/index.js`. Please call it here instead; if anything that's the
> common pattern in the redux community. But you might also include `index.js`
> in tests and it should just export a simple object.
Done

> 
> ::: devtools/client/dom/test/browser.ini
> @@ +1,5 @@
> > +[DEFAULT]
> > +tags = devtools
> > +subsuite = devtools
> > +support-files =
> > +  head.js
> 
> Some tests would be nice :)
I'll provide separate patch for them.


> ::: devtools/client/shared/components/reps/attribute.js
> @@ +1,1 @@
> > +/* -*- indent-tabs-mode: nil; js-indent-level: 2 -*- */
> 
> It worries me that none of the reps have xpcshell tests to test basic
> functionality. Maybe that's ok because we should use them a little bit more
> to make sure how they currently work will work across tools, but can you
> file a new bug to write at least some tests against them? No need to go too
> extreme, but having some would be good.
Agreed, let's do this in separate bug: Bug 1257552

> 
> ::: devtools/client/shared/components/reps/grip.js
> @@ +85,5 @@
> > +
> > +    // Object members with non-empty values are preferred since it gives the
> > +    // user a better overview of the object.
> > +    let props = [];
> > +    this.getProps(props, object, max, isInterestingProp);
> 
> Please avoid the C-style passing in of mutable props. Just return it. If
> this exists in the other reps, please update those as well. (seems like
> every rep is a similar style and there are a lot of new ones here so I'm not
> going into detail on every single one)
Fixed (and also in another place)

> 
> @@ +91,5 @@
> > +    if (props.length <= max) {
> > +      // There are not enough props yet (or at least, not enough props to
> > +      // be able to know whether we should print "more..." or not).
> > +      // Let's display also empty members and functions.
> > +      this.getProps(props, object, max, function(t, value) {
> 
> nit: prefer arrow functions: `(t, value) => {...}` (not going to point out
> every place where this is applicable)
Absolutely agree, fixed (and also at other places)

> 
> @@ +97,5 @@
> > +      });
> > +    }
> > +
> > +    if (props.length > max) {
> > +      props.pop();
> 
> This doesn't seem right. If the length is greater than the max you only pop
> of a single item? Don't you need to do `slice` instead to make sure the new
> array is within the max length?
This is correct. It's because the previous getProps() call can return `max+1` properties to indicate that there is more than `max` and the `more...` postfix is needed. So, one `props.pop()` call is just enough. I made a comment in the code explaining this.

> 
> ::: devtools/client/shared/components/reps/named-node-map.js
> @@ +119,5 @@
> > +let PropRep = React.createFactory(React.createClass({
> > +  displayName: "PropRep",
> > +
> > +  render: function() {
> > +    const { Rep } = createFactories(require("./rep"));
> 
> I'd rather not have a circular dependency (which forces a `require` in a
> render function). Can you instead take a `GenericRep` component as a
> property and the renderer should pass the Rep component in?
Totally agree. There is actually more to this, I'd like to have API allowing extension to register new reps for new data types (e.g. one for jQuery object). This new concept should avoid including all reps in rep.js and break the circular dependency. I filled a bug 1257548 and I'll fix this as part of it (or a follow up), if it's ok for you.

> 
> ::: devtools/client/shared/components/reps/object.js
> @@ +62,5 @@
> >  
> >      propIterator: function(object, max) {
> >        function isInterestingProp(t, value) {
> > +        // Do not pick objects, it could cause recursion.
> > +        return (t == "boolean" || t == "number" || (t == "string" && value));
> 
> What does this change in the UI? Hard to tell. Are you sure this doesn't
> change anything important in the JSON viewer?
This is alright. It only avoid infinite recursion when an object contains reference to itself and is trying to render its children i.e. itself. In any case, I'll be testing this as part of bug 1256757.

Thanks for the reviews James, I know it's a big patch.

Honza

Comment 40

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Comment on attachment 8731768 [details] [diff] [review]
bug1201475.patch

(In reply to James Long (:jlongster) from comment #35)
> Going through this again now, but for something this big it's probably good
> to go ahead and get another person to look at it too. Not sure who, maybe
> bgrins, linclark, jryans?

I was planning to ask Lin for review :)

Honza

Comment 41

[James Long (:jlongster)]

Cool, thanks for responding. About the last point, you are ignore all objects though, right? What does that mean for the UI? What is it not showing now? Just curious, I'm not familiar with that component to know.

Comment 42

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Attachment: bug1201475.patch

(In reply to James Long (:jlongster) from comment #41)
> Cool, thanks for responding. About the last point, you are ignore all
> objects though, right? What does that mean for the UI? What is it not
> showing now? Just curious, I'm not familiar with that component to know.
The change is in the way how object previews are generated. Btw. object preview is the right (second column) part in the JSON View or DOM panel. After this change, primitive values a prioritized for the preview (which actually makes the preview a bit easier to read). I believe that the user won't notice any difference, however, the fix is not perfect and we eventually need better solution. I've reported a bug 1257784 that covers the recursion problem and I'll fix it. Good call.

+ I simplified reps.css, removed font-family styling entirely. It should be up to the current theme to apply proper font. Currently .devtools-monospace class in the <body> element ensures correct monospace font light and dark themes. Firebug theme might apply its own font.

Honza

Comment 43

[Jan Honza Odvarko [:Honza] (always need-info? me)]

One problem I've found is related to encoding. When I load this page:
https://www.google.cz/?gfe_rd=cr&ei=xKnrVtnlM-ak8weN9IygAQ&gws_rd=ssl#q=zygozoospore

The DOM panel is empty and the Browser Console says:
Console Service ERROR [JavaScript Error: "not well-formed"] [JavaScript Error: "not well-formed"]
resource://devtools/client/shared/vendor/react-dev.js (19876) 

---

My feeling is that it's because there are Chinese characters in the prototypeAndProperties packet received from the backend and they break markup...? Note that the strings are directly used as properties to React components (used to generated the result markup). But it all works in RDP Inspector (I can properly see structure of the packet). The difference is that the DOM panel is running in XHML (RDPi uses HTML).

Any tips?

Honza

Comment 44

[James Long (:jlongster)]

I can't load that page; google redirects me to an english version. I tried http://www.archchinese.com/chinese_english_dictionary.html though which has Chinese characters, and it worked fine. 

You could go into react-dev.js and add a console.log statement to see what it's trying to set as `innerHTML`. I'm not sure without being able to reproduce. It sounds like it might be an XHTML-related issue.

Comment 45

[Jan Honza Odvarko [:Honza] (always need-info? me)]

(In reply to James Long (:jlongster) from comment #44)
> I can't load that page; google redirects me to an english version.

What about this URL?
https://www.google.com/?gfe_rd=cr&ei=cx7wVqPaPKzs8wf-zIHgCQ&gws_rd=ssl,cr&fg=1#q=zygozoospore

Or you might just look for 'zygozoospore', I guess.

Honza

Comment 46

[James Long (:jlongster)]

Can you rebase the attached patch? I've been applying it on fx-team whenever I want to test it.

Comment 47

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Attachment: bug1201475.patch

Sure, done!

Honza

Comment 48

[James Long (:jlongster)]

Jan, I dumped out the HTML that React is generating and ran it through a validator. There are a bunch of errors, but there is a pattern I see which might be the problem. I see elements like this:

<span data-reactid=".0.1.1.$/QS_4fa.$value.0.0.2:$

Comment 49

[James Long (:jlongster)]

Sigh, it cut off my message because of the unicode. Here's the element with the unicode substituted:

<span data-reactid=".0.1.1.$/QS_4fa.$value.0.0.2:$<UNICODE HERE>">

You are creating keys that include the unicode characters which are being set as attributes. I don't know exactly what step is causing the problem; if all unicode is allowed by HTML but React isn't inserting it correctly, if it's not allowed, or if the charset isn't set correctly or something. If anything, when you are generating keys for whatever markup this is you could sanitize the input.

Comment 50

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Thanks James!

Using dom.html (not dom.xhtml) for the DOM panel fixes the problem.

Is there any reason why we could *not* use .html files for our panels?

Honza

Comment 51

[James Long (:jlongster)]

(In reply to Jan Honza Odvarko [:Honza] from comment #50)
> Thanks James!
> 
> Using dom.html (not dom.xhtml) for the DOM panel fixes the problem.
> 
> Is there any reason why we could *not* use .html files for our panels?
> 
> Honza

I'm not the right person to ask. It probably has to do with our theme system and other things like that. bgrins, jryans, etc probably know more details about why XHTML might be required.

Comment 52

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Attachment: bug1201475.patch

Rebased on the current head.
Honza

Comment 53

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Attachment: bug1201475.patch

(In reply to James Long (:jlongster) from comment #51)
> I'm not the right person to ask. It probably has to do with our theme system
> and other things like that. bgrins, jryans, etc probably know more details
> about why XHTML might be required.
Talking to jryans and using .html is fine.

The new patch solves the unicode problem.

Honza

Honza

Comment 54

[Lin Clark [:linclark]]

Comment on attachment 8733990 [details] [diff] [review]
bug1201475.patch

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

Leaving this as needs review from me until I finish the review.

::: devtools/client/dom/content/components/dom-tree.js
@@ +51,5 @@
> +  /**
> +   * Render DOM panel content
> +   */
> +  render: function() {
> +    let store = this.context.store;

Why does this access the store directly from the context object? Generally when using Redux you shouldn't be accessing the context directly.

::: devtools/client/dom/content/components/main-toolbar.js
@@ +29,5 @@
> +
> +  displayName: "MainToolbar",
> +
> +  onRefresh: function() {
> +    this.props.actions.onRefresh();

This should be added to propTypes.

::: devtools/client/dom/content/components/search-box.js
@@ +36,5 @@
> +    if (this.searchTimeout) {
> +      win.clearTimeout(this.searchTimeout);
> +    }
> +
> +    let callback = this.doSearch.bind(this, searchBox);

Why do you need doSearch here? It seems like this could be

>  let callback = this.props.actions.onSearch(searchBox.value);

Comment 55

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Attachment: bug1201475.patch

(In reply to Lin Clark [:linclark] from comment #54)
> Comment on attachment 8733990 [details] [diff] [review]
> bug1201475.patch
> 
> Review of attachment 8733990 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Leaving this as needs review from me until I finish the review.
> 
> ::: devtools/client/dom/content/components/dom-tree.js
> @@ +51,5 @@
> > +  /**
> > +   * Render DOM panel content
> > +   */
> > +  render: function() {
> > +    let store = this.context.store;
> 
> Why does this access the store directly from the context object? Generally
> when using Redux you shouldn't be accessing the context directly.
I was following Dan Abramov tutorial:
https://egghead.io/lessons/javascript-redux-passing-the-store-down-implicitly-via-context
Do we want to do it differently?

But, there where two places where the store was used to only call dispatch().
I changed it from: 
store.dispatch(...)
to:
this.props.dispatch(...)

which seems to be a bit better...


> ::: devtools/client/dom/content/components/main-toolbar.js
> @@ +29,5 @@
> > +
> > +  displayName: "MainToolbar",
> > +
> > +  onRefresh: function() {
> > +    this.props.actions.onRefresh();
> 
> This should be added to propTypes.
Done

> 
> ::: devtools/client/dom/content/components/search-box.js
> @@ +36,5 @@
> > +    if (this.searchTimeout) {
> > +      win.clearTimeout(this.searchTimeout);
> > +    }
> > +
> > +    let callback = this.doSearch.bind(this, searchBox);
> 
> Why do you need doSearch here? It seems like this could be
> 
> >  let callback = this.props.actions.onSearch(searchBox.value);
It needs to be:
let callback = this.props.actions.onSearch.bind(this, searchBox.value);
Ok, done.

Honza

Comment 56

[Lin Clark [:linclark]]

> I was following Dan Abramov tutorial:
> https://egghead.io/lessons/javascript-redux-passing-the-store-down-
> implicitly-via-context
> Do we want to do it differently?

Ah yes, that tutorial is kind of confusing. What he's showing in that one is the internals of how Redux handles state internally. The API that you interact with (`connect`) is described over the next few videos, but it isn't really clear that what he's doing is replacing the dependency that all of the components have on the context object. By the end of the series, there is no `context` variable in the code anymore.

To wire it up in this case, you would add a connect function to the bottom

> const mapStateToProps = function(state) {
>  return {
>    state
>  }
>}
>
>module.exports = connect(mapStateToProps)(DomTree);

Which makes your GripProvider have this signature:

> provider: new GripProvider(state, this.props.dispatch),

Comment 57

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Attachment: bug1201475.patch

(In reply to Lin Clark [:linclark] from comment #56)
> To wire it up in this case, you would add a connect function to the bottom
I see, make sense, fixed.

Honza

Comment 58

[Lin Clark [:linclark]]

Comment on attachment 8735437 [details] [diff] [review]
bug1201475.patch

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

I am going to r- this for now just because of the lack of tests. :Honza and I talked about this in IRC last week and IIRC he's currently working on tests. Once we have test coverage, this will be r+ from me.

::: devtools/client/dom/.eslintrc
@@ +10,5 @@
> +  },
> +  "rules": {
> +    "indent": 0,
> +    "no-unused-vars": [2, {"vars": "all", "args": "none"}],
> +    "padded-blocks": 0,

The globals make sense, but why are these rules being added?

::: devtools/client/dom/content/components/dom-tree.js
@@ +54,5 @@
> +    let columns = [{
> +      "id": "value"
> +    }];
> +
> +    // This is the integration poin with Reps. The DomTree is using

s/poin/point

::: devtools/client/dom/content/components/main-frame.js
@@ +41,5 @@
> +    this.refresh(this.props.object);
> +  },
> +
> +  refresh: function(grip) {
> +    this.props.dispatch(fetchProperties(grip));

It seems like this doesn't need to be defined on this component, and could instead be defined on the toolbar. I'm guessing this is a holdover from before Redux was being used.

@@ +45,5 @@
> +    this.props.dispatch(fetchProperties(grip));
> +  },
> +
> +  onSearch: function(value) {
> +    this.props.dispatch(setVisibilityFilter(value));

It seems like this one could be in search-box.js, or it could be created in main-toolbar.js if you want to keep search-box.js more generic so it can be reused in different apps.

@@ +55,5 @@
> +  render: function() {
> +    return (
> +      div({className: "mainFrame"},
> +        MainToolbar({
> +          actions: this

So the whole component is being passed in as the actions prop here. Rather than passing the whole component, could we just pass down the action functions themselves?

This might also be a moot point given my comments above.

::: devtools/client/dom/content/components/search-box.js
@@ +37,5 @@
> +      win.clearTimeout(this.searchTimeout);
> +    }
> +
> +    let callback = this.props.actions.onSearch.bind(this, searchBox.value);
> +    this.searchTimeout = win.setTimeout(callback, searchDelay);

Should we be doing something here like react-timer-mixin does?

::: devtools/client/dom/content/reducers/filter.js
@@ +16,5 @@
> +
> +/**
> + * Filter displayed object properties.
> + */
> +function filter(state = getInitialState(), action) {

Unless the initial state will get more complex over time, it might be clearer just to set it directly here. But either way is fine.

::: devtools/client/dom/content/reducers/grips.js
@@ +16,5 @@
> +
> +/**
> + * Maintain a cache of received grip responses from the backend.
> + */
> +function grips(state = getInitialState(), action) {

Same as above.

@@ +25,5 @@
> +    return state;
> +  }
> +
> +  switch (action.status) {
> +    case "begin":

We might want to standardize this kind of thing across all the tools. Memory tool uses "start" and "end". Up to you whether you want to change it, though.

::: devtools/client/shared/components/reps/date-time.js
@@ +17,5 @@
> +  // Shortcuts
> +  const { span } = React.DOM;
> +
> +  /**
> +   * Used to render JS built-int Date() object.

s/built-int/built-in

Comment 59

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Attachment: bug1201475.patch

(In reply to Lin Clark [:linclark] from comment #58)
> Comment on attachment 8735437 [details] [diff] [review]
> bug1201475.patch
> 
> Review of attachment 8735437 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I am going to r- this for now just because of the lack of tests. :Honza and
> I talked about this in IRC last week and IIRC he's currently working on
> tests. Once we have test coverage, this will be r+ from me.
Yes

> 
> ::: devtools/client/dom/.eslintrc
> @@ +10,5 @@
> > +  },
> > +  "rules": {
> > +    "indent": 0,
> > +    "no-unused-vars": [2, {"vars": "all", "args": "none"}],
> > +    "padded-blocks": 0,
> 
> The globals make sense, but why are these rules being added?
Removed (it's now part of devtools\.eslintrc)

> 
> ::: devtools/client/dom/content/components/dom-tree.js
> @@ +54,5 @@
> > +    let columns = [{
> > +      "id": "value"
> > +    }];
> > +
> > +    // This is the integration poin with Reps. The DomTree is using
> 
> s/poin/point
Fixed

> 
> ::: devtools/client/dom/content/components/main-frame.js
> @@ +41,5 @@
> > +    this.refresh(this.props.object);
> > +  },
> > +
> > +  refresh: function(grip) {
> > +    this.props.dispatch(fetchProperties(grip));
> 
> It seems like this doesn't need to be defined on this component, and could
> instead be defined on the toolbar. I'm guessing this is a holdover from
> before Redux was being used.
Done

> 
> @@ +45,5 @@
> > +    this.props.dispatch(fetchProperties(grip));
> > +  },
> > +
> > +  onSearch: function(value) {
> > +    this.props.dispatch(setVisibilityFilter(value));
> 
> It seems like this one could be in search-box.js, or it could be created in
> main-toolbar.js if you want to keep search-box.js more generic so it can be
> reused in different apps.
Done (onSearch kept in main-toolbar.js)

> 
> @@ +55,5 @@
> > +  render: function() {
> > +    return (
> > +      div({className: "mainFrame"},
> > +        MainToolbar({
> > +          actions: this
> 
> So the whole component is being passed in as the actions prop here. Rather
> than passing the whole component, could we just pass down the action
> functions themselves?
OK, done (for the search-box)

> 
> This might also be a moot point given my comments above.
> 
> ::: devtools/client/dom/content/components/search-box.js
> @@ +37,5 @@
> > +      win.clearTimeout(this.searchTimeout);
> > +    }
> > +
> > +    let callback = this.props.actions.onSearch.bind(this, searchBox.value);
> > +    this.searchTimeout = win.setTimeout(callback, searchDelay);
> 
> Should we be doing something here like react-timer-mixin does?
Good point, the timeout is cleaned up in componentWillUnmount.

> 
> ::: devtools/client/dom/content/reducers/filter.js
> @@ +16,5 @@
> > +
> > +/**
> > + * Filter displayed object properties.
> > + */
> > +function filter(state = getInitialState(), action) {
> 
> Unless the initial state will get more complex over time, it might be
> clearer just to set it directly here. But either way is fine.
OK, I kept this as is (as an example).

> 
> ::: devtools/client/dom/content/reducers/grips.js
> @@ +16,5 @@
> > +
> > +/**
> > + * Maintain a cache of received grip responses from the backend.
> > + */
> > +function grips(state = getInitialState(), action) {
> 
> Same as above.
> 
> @@ +25,5 @@
> > +    return state;
> > +  }
> > +
> > +  switch (action.status) {
> > +    case "begin":
> 
> We might want to standardize this kind of thing across all the tools. Memory
> tool uses "start" and "end". Up to you whether you want to change it, though.
Totally on board with the standardization. Done.

We might want to also discuss 'Standard Actions' at our comp meeting.
https://github.com/acdlite/flux-standard-action

> 
> ::: devtools/client/shared/components/reps/date-time.js
> @@ +17,5 @@
> > +  // Shortcuts
> > +  const { span } = React.DOM;
> > +
> > +  /**
> > +   * Used to render JS built-int Date() object.
> 
> s/built-int/built-in
Fixed

Thanks Lin!

Tests in progress...
Honza

Comment 60

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Attachment: bug1201475-tests.patch

Tests...

Try push:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=1b6c96db9319

Honza

Comment 61

[Lin Clark [:linclark]]

Comment on attachment 8735809 [details] [diff] [review]
bug1201475.patch

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

It looks like there's a lot of orange in the try push. Some of them are intermittents, but it also looks like there are some legitimate fails in there.

Comment 62

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Attachment: bug1201475-tests.patch

Lin, I am attaching new version with fixes for tray failures.

There is only one timeout:
devtools/client/framework/test/browser_toolbox_window_reload_target.js
... that persists even if I am using requestLongerTimeout(10);

I need to find someone to get help with the test, but I think you might start with the review (this should go in 48 and the next merge is coming soon).

Honza

Comment 63

[Jan Honza Odvarko [:Honza] (always need-info? me)]

@jryans: do you have any tips why the
devtools/client/framework/test/browser_toolbox_window_reload_target.js
... times out?

Here is my try push:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=41fdae16b64b&selectedJob=19020181

I did use requestLongerTimeout(10); but the problem seems to be something else...

Honza

Comment 64

[Lin Clark [:linclark]]

Comment on attachment 8738170 [details] [diff] [review]
bug1201475-tests.patch

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

I'm not ready to r+ this yet, but I also don't want to block it, so just giving feedback for now.

I'm a little uncomfortable with the way the store is being used here. Is the store just being used for the waitForDispatch function? Is there any place we could put a test-only event instead?

In the long term, I'd like to see if we could add more unit/integration tests for this. Just for the sake of discussion, what functionality would we miss if these tests were structured like the following?

- 1 chrome test that tests the store directly. It calls dispatch on the store with an action. The store starts in state A and the test makes sure it ends up in state B
- 1 chrome test that tests the components. Given state A, it makes sure that it renders as expected. Same for B.
- 1 test that ensures the expected action is dispatched when a button is pressed

::: devtools/client/dom/content/dom-view.js
@@ +31,5 @@
>    addEventListener("devtools/chrome/message",
>      this.onMessage.bind(this), true);
> +
> +  // Make it local so, tests can access it.
> +  this.store = localStore;

You shouldn't need to pass in the store this way.

::: devtools/client/dom/test/head.js
@@ +40,5 @@
> +      loadCommonFrameScript(tab);
> +
> +      // Select the DOM panel and wait till it's initialized.
> +      initDOMPanel(tab).then(panel => {
> +        waitForDispatch(panel, "FETCH_PROPERTIES").then(() => {

Would it be possible to fire a test-only event instead of using waitForDispatch?

@@ +70,5 @@
> +    });
> +  });
> +}
> +
> +function loadCommonFrameScript(tab) {

This function seems to also be here: https://dxr.mozilla.org/mozilla-central/source/devtools/client/netmonitor/test/head.js#447

Rather than adding it in two heads, does it make sense to move it to a higher level head.js?

Comment 65

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

(In reply to Jan Honza Odvarko [:Honza] from comment #63)
> @jryans: do you have any tips why the
> devtools/client/framework/test/browser_toolbox_window_reload_target.js
> ... times out?
> 
> Here is my try push:
> https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=41fdae16b64b&selectedJob=19020181
> 
> I did use requestLongerTimeout(10); but the problem seems to be something
> else...

This test tries to reload the page while each built-in tool is open.

It fails in the new DOM panel:

"Reloaded from devtools window once and only for undocked devtools with tool dom, key #toolbox-force-reload-key2"

That key[1] is accel-F5.  The failure only happens on Windows and Linux, where accel maps to ctrl (it's cmd on Mac).  So, we're synthesizing ctrl-F5 with the DOM panel open.

(Aside: the file emoji-tool-dom.svg seems to contain... the contents of an entire GitHub HTML page...?)

Is the DOM panel doing anything to capture the sequence "ctrl-F5" that would prevent the toolbox from handling it?

[1]: https://dxr.mozilla.org/mozilla-central/source/devtools/client/framework/toolbox.xul#87

Comment 66

[Jan Honza Odvarko [:Honza] (always need-info? me)]

(In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #65)
> Is the DOM panel doing anything to capture the sequence "ctrl-F5" that would
> prevent the toolbox from handling it?
No, I don't know about anything that would block that (and it works when manually tested)

---

I changed the code so, the DOM panel is firing 'ready' event when its content is really initialized, i.e. when it receives response from the backend (for getPrototypeAndProperties packet).

Here is my latest try push:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=d48388eab5a1&selectedJob=19275287

It looks like there is less oranges, but still...

There is one failure I don't understand:
resource://devtools/client/framework/toolbox.js:1259 - TypeError: panel is undefined

(for me it's line: if (typeof panel.emit == "undefined") {

How this could happen?

Could it be that the panel is closed before this code is executed?

Honza

Comment 67

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

(In reply to Jan Honza Odvarko [:Honza] from comment #66)
> (In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #65)
> > Is the DOM panel doing anything to capture the sequence "ctrl-F5" that would
> > prevent the toolbox from handling it?
> No, I don't know about anything that would block that (and it works when
> manually tested)
> 
> ---
> 
> I changed the code so, the DOM panel is firing 'ready' event when its
> content is really initialized, i.e. when it receives response from the
> backend (for getPrototypeAndProperties packet).
> 
> Here is my latest try push:
> https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=d48388eab5a1&selectedJob=19275287
> 
> It looks like there is less oranges, but still...
> 
> There is one failure I don't understand:
> resource://devtools/client/framework/toolbox.js:1259 - TypeError: panel is
> undefined
> 
> (for me it's line: if (typeof panel.emit == "undefined") {
> 
> How this could happen?
> 
> Could it be that the panel is closed before this code is executed?

Which try run shows this error?  I wasn't able to find it.

Have you tried running the test locally on Windows or Linux debug builds?

Comment 68

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Attachment: bug1201475-tests.patch

(In reply to Lin Clark [:linclark] from comment #64)
> Comment on attachment 8738170 [details] [diff] [review]
> bug1201475-tests.patch
> 
> Review of attachment 8738170 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I'm not ready to r+ this yet, but I also don't want to block it, so just
> giving feedback for now.
> 
> I'm a little uncomfortable with the way the store is being used here. Is the
> store just being used for the waitForDispatch function? Is there any place
> we could put a test-only event instead?
Yes the store is exposed for waitForDispatch

I was inspired by James's API for debugger here:
https://github.com/mozilla/gecko-dev/blob/fx-team/devtools/client/debugger/test/mochitest/head.js#L1240

The store is exposed similarly for Debugger tests (James calls it DebuggerController) and my plan was to share the API as soon as we agree on the Action object structure (mainly on states like begin/done/error)

I think we could come up with more test API designed specifically for testing React/Redux based flow. Along these lines we can also improve the way how the store is exposed/used by these test API.

> > +function loadCommonFrameScript(tab) {
> 
> This function seems to also be here:
> https://dxr.mozilla.org/mozilla-central/source/devtools/client/netmonitor/
> test/head.js#447
> 
> Rather than adding it in two heads, does it make sense to move it to a
> higher level head.js?
Fixed (I used existing getFrameScript() API)

> In the long term, I'd like to see if we could add more unit/integration
> tests for this. Just for the sake of discussion, what functionality would we
> miss if these tests were structured like the following?
> 
> - 1 chrome test that tests the store directly. It calls dispatch on the
> store with an action. The store starts in state A and the test makes sure it
> ends up in state B
> - 1 chrome test that tests the components. Given state A, it makes sure that
> it renders as expected. Same for B.
> - 1 test that ensures the expected action is dispatched when a button is
> pressed
Definitely, I filled bug 1264002


Honza

Comment 69

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Attachment: bug1201475.patch

(In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #65)
> (Aside: the file emoji-tool-dom.svg seems to contain... the contents of an
> entire GitHub HTML page...?)
Ah, fixed

Honza

Comment 70

[James Long (:jlongster)]

FWIW the react/redux testing utilities of the debugger were heavily driven by the current design of the tests. The goal was to make it as easy as possible to update all of the tests. It's probably not the best testing API per-say. If we were in a pure react/redux world it would probably look different. (case in point: merging the store onto the DebuggerController was a hack and copied from Jordan doing the same thing in another tool where redux was slowly being integrated)

I like how the memory tool works. In debugger.html, we are mocking out the protocol actors and unit testing all the actions. I know Lin has other strategies for testing stuff. Anyway, until we're out of the mix of non-React & React code we probably won't standardize on testing utilities yet.

Comment 71

[Jan Honza Odvarko [:Honza] (always need-info? me)]

(In reply to James Long (:jlongster) from comment #70)
> FWIW the react/redux testing utilities of the debugger were heavily driven
> by the current design of the tests. The goal was to make it as easy as
> possible to update all of the tests. It's probably not the best testing API
> per-say. If we were in a pure react/redux world it would probably look
> different. (case in point: merging the store onto the DebuggerController was
> a hack and copied from Jordan doing the same thing in another tool where
> redux was slowly being integrated)
> 
> I like how the memory tool works. In debugger.html, we are mocking out the
> protocol actors and unit testing all the actions. I know Lin has other
> strategies for testing stuff. Anyway, until we're out of the mix of
> non-React & React code we probably won't standardize on testing utilities
> yet.
That make sense.

So, do you think API like waitForDispatch should be rather removed?
(I am thinking about mochitests now)

Honza

Comment 72

[James Long (:jlongster)]

Removed from this patch? I think the core idea behind it is solid, but in the debugger we had to use the global store in the tests because all of the tests depend on actual DOM changes. Ideally you would create a local store for each test itself. I think Lin didn't like exporting the real store for tests. So it's how the store is used, not specifically the `waitForDispatch` function. I think it's very useful to test actions by creating workflows and using helper functions that listens for actions.

I don't know if your code is structured in a way that you can create ad-hoc stores to be used in tests. I'd rather not emit various events just for tests, I think that's worse than just listening to the store itself. I don't really know what to recommend, glancing at this code it's hard to tell what the best strategy is. If it's too hard to create ad-hoc stores you might have to use the real store.

Comment 73

[Lin Clark [:linclark]]

Comment on attachment 8740511 [details] [diff] [review]
bug1201475-tests.patch

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

::: devtools/client/dom/test/head.js
@@ +132,5 @@
> +    store.dispatch({
> +      // Normally we would use `services.WAIT_UNTIL`, but use the
> +      // internal name here so tests aren't forced to always pass it
> +      // in
> +      type: "@@service/waitUntil",

One of my main concerns was that I find this waitUntil action kind of confusing. Could this be handled by subscribing to the store? That's what the memory tool seems to be doing for a lot of its waiting.

Comment 74

[James Long (:jlongster)]

(In reply to Lin Clark [:linclark] from comment #73)
> Comment on attachment 8740511 [details] [diff] [review]
> bug1201475-tests.patch
> 
> Review of attachment 8740511 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: devtools/client/dom/test/head.js
> @@ +132,5 @@
> > +    store.dispatch({
> > +      // Normally we would use `services.WAIT_UNTIL`, but use the
> > +      // internal name here so tests aren't forced to always pass it
> > +      // in
> > +      type: "@@service/waitUntil",
> 
> One of my main concerns was that I find this waitUntil action kind of
> confusing. Could this be handled by subscribing to the store? That's what
> the memory tool seems to be doing for a lot of its waiting.

No, because subscribing to the store only gives you the state. If you want to wait for an action you need something like this. This uses a middleware that a few pieces of code use to wait for a specific action to come through: https://github.com/mozilla/gecko-dev/blob/master/devtools/client/shared/redux/middleware/wait-service.js

For example, in an action creator if you see that you are already loading something you can use this to "queue" a function to be run when it's finished loading, so that you can reload it again.

You could create a custom store in each test that provides the ability to wait for a specific action, but it would be pretty much the same thing as WAIT_UNTIL.

The memory tool probably hasn't need something like this. The debugger does, especially for the current test suite, which frequently does something and then wants to wait until a certain async action if finished. I don't have access to the promise returned from `store.dispatch`. It's possible in the new refactor that we won't need this as much.

Note that I've seen several other libraries forced to implement something like this. IMHO it's where redux starts breaking down, but the solution isn't bad. It just doesn't necessarily fit into the "normal" redux way.

Comment 75

[Lin Clark [:linclark]]

If you want to r+ it, I'm ok with that. It's unfamiliar to me, but if it looks good to you then that works.

Comment 76

[James Long (:jlongster)]

I recommend reading the implementation of the middleware (called a service because it's stateful) and the usage here: https://github.com/jlongster/gecko-dev/blob/debugger-refactor-sources-4/devtools/client/debugger/content/actions/event-listeners.js#L21. I think it's worthwhile to get familiar with these patterns.

I don't actually see many tests here to know if it's appropriate or not. It made the debugger tests a lot easier.

Comment 77

[Lin Clark [:linclark]]

Comment on attachment 8740511 [details] [diff] [review]
bug1201475-tests.patch

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

I think my concerns about the testing strategy will be addressed with Honza's followup issue, and given the time constraint let's move ahead with this test.

Comment 78

[Lin Clark [:linclark]]

Comment on attachment 8740516 [details] [diff] [review]
bug1201475.patch

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

Just did some light manual testing. I don't know whether this has been noted, but the performance could be improved. It takes about 6 seconds to load on google.com. However, that can be a follow up.

This needs a passing try push before landing. Marking as r+ contingent on that.

Comment 79

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Attachment: bug1201475-opt.patch

(In reply to Lin Clark [:linclark] from comment #77)
> I think my concerns about the testing strategy will be addressed with
> Honza's followup issue, and given the time constraint let's move ahead with
> this test.
Yes

(In reply to Lin Clark [:linclark] from comment #78)
> Just did some light manual testing. I don't know whether this has been
> noted, but the performance could be improved. It takes about 6 seconds to
> load on google.com. However, that can be a follow up.
Yes, I know about this. It's because the |window| object has usually huge amount of properties and rendering takes time (it's a lot faster in non-debug non react-dev version though). I filled bug 1264908 (another reason why we need virtual tree-view).

> This needs a passing try push before landing. Marking as r+ contingent on
> that.
Yes, I am attaching one additional patch that:

- Fixes eslint errors
- Optimizes panel rendering, which now happens only if the panel is actually visible.

Try push seems to be good now:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=3847b2874c5f

Thanks for the reviews Lin!
Honza

Comment 80

[Lin Clark [:linclark]]

Comment on attachment 8741727 [details] [diff] [review]
bug1201475-opt.patch

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

Honza, are the ESLint errors in the try push expected?

Comment 81

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Ah, there are two from page_basic.html (I'll fix those), but all the other errors come from different directories.

Honza

Comment 82

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Attachment: bug1201475-opt.patch

Eslint errors fixed

Try push
https://treeherder.mozilla.org/#/jobs?repo=try&revision=64e8165a068a

Honza

Comment 83

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Thanks Lin!

Patches should be applied in this order:

1) bug1201475.patch
2) bug1201475-tests.patch
3) bug1201475-opt.patch

Honza

Comment 84

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/f80643f3c767
https://hg.mozilla.org/integration/fx-team/rev/b795087334cb
https://hg.mozilla.org/integration/fx-team/rev/50e5abd1f981

Comment 85

[Pulsebot]

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

Comment 86

[Carsten Book [:Tomcat]]

sorry had to back this out for test failures like https://treeherder.mozilla.org/logviewer.html#?job_id=8863964&repo=fx-team after this change here

Comment 87

[Pulsebot]

Backout:
https://hg.mozilla.org/integration/fx-team/rev/c788233efce1
https://hg.mozilla.org/integration/fx-team/rev/b790588251d7
https://hg.mozilla.org/integration/fx-team/rev/8f9694fc1184
https://hg.mozilla.org/integration/fx-team/rev/f789cf6c7a55

Comment 88

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Attachment: bug1201475-timeouts.patch

@jryans: the previous commit caused two tests to fail because of a timeout on linux32 debug. I was only testing with lin64, win and mac, which are fine.

It's because the DOM panel rendering is slow atm, but should be fixed as part of bug 1264908. In the meantime I increased the timeouts (using requestLongerTimeout), which seems to help.

Here is new try:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=6e347a3e9bf6

(hope it's ok, the dom panel should land in 48)

Honza

Comment 89

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

Comment on attachment 8744070 [details] [diff] [review]
bug1201475-timeouts.patch

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

Seems okay if it works!

Comment 90

[Jan Honza Odvarko [:Honza] (always need-info? me)]

@Tomcat: I believe I fixed the problem, let's try to land it again.

Patches should be applied in this order:
1) bug1201475.patch
2) bug1201475-tests.patch
3) bug1201475-opt.patch
4) bug1201475-timeouts.patch

Honza

Comment 91

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/6f83467f1a10
https://hg.mozilla.org/integration/fx-team/rev/60df1f524d7f
https://hg.mozilla.org/integration/fx-team/rev/bfda97a555e0
https://hg.mozilla.org/integration/fx-team/rev/4832ac37867f

Comment 92

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/6f83467f1a10
https://hg.mozilla.org/mozilla-central/rev/60df1f524d7f
https://hg.mozilla.org/mozilla-central/rev/bfda97a555e0
https://hg.mozilla.org/mozilla-central/rev/4832ac37867f

Comment 93

[Sebastian Zartner [:sebo]]

Release Note Request (optional, but appreciated)
[Why is this notable]: New panel within the DevTools allowing you to inspect any kind of DOM properties
[Suggested wording]: Added DOM Inspector
[Links (documentation, blog post, etc)]: Will be at https://developer.mozilla.org/en-US/docs/Tools/DOM_Inspector.

Sebastian

Comment 94

[Sebastian Zartner [:sebo]]

I've documented this now at https://developer.mozilla.org/en-US/docs/Tools/DOM_Inspector and mentioned it in https://developer.mozilla.org/en-US/Firefox/Releases/48.

Honza, please let me know if it is ok for you.

Will, I don't know how to add an entry to the sidebar. Can you please do that?

Sebastian

Comment 95

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

(In reply to Sebastian Zartner [:sebo] from comment #94)
> Will, I don't know how to add an entry to the sidebar. Can you please do
> that?

The sidebar content is (magically) maintained in the main Tools page[1].  If you edit the "Subnav" content at the bottom of this page, it should affect all the pages.

[1]: https://developer.mozilla.org/en-US/docs/Tools

Comment 96

[Sebastian Zartner [:sebo]]

(In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #95)
> (In reply to Sebastian Zartner [:sebo] from comment #94)
> > Will, I don't know how to add an entry to the sidebar. Can you please do
> > that?
> 
> The sidebar content is (magically) maintained in the main Tools page[1].  If
> you edit the "Subnav" content at the bottom of this page, it should affect
> all the pages.
> 
> [1]: https://developer.mozilla.org/en-US/docs/Tools

Cool, thanks for the hint!

Sebastian

Comment 97

[Jan Honza Odvarko [:Honza] (always need-info? me)]

(In reply to Sebastian Zartner [:sebo] from comment #94)
> I've documented this now at
> https://developer.mozilla.org/en-US/docs/Tools/DOM_Inspector and mentioned
> it in https://developer.mozilla.org/en-US/Firefox/Releases/48.
> 
> Honza, please let me know if it is ok for you.
Nice!

Honz

Comment 98

[Sebastian Zartner [:sebo]]

I've also added it to the main Tools page now:
https://developer.mozilla.org/en-US/docs/Tools

In the future we may also add videos to the DOM Inspector page (when the main remaining issues are fixed). Though for now I'll mark this as dev-doc-complete.

Sebastian

Comment 99

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

This is currently called "DOM Inspector" on MDN, which normally would seem okay, since we do document some other panels as "X Inspector", like with storage.  However, I think there is some potential for confusion with the "DOM Inspector" add-on[1] used by Gecko platform and add-on developers.

Will and Sebastian, any thoughts on what to do here?  Honza suggested on IRC calling it "DOM Panel" instead, but not sure if that works well with the rest of the pages we already have documented.

[1]: https://addons.mozilla.org/en-US/firefox/addon/dom-inspector-6622/

Comment 100

[Will Bamberg [:wbamberg]]

Sebastian, thanks for writing this page.

I'm not sure "DOM Panel" is better. I think there is also potential for confusion with the Inspector, as noted: https://hacks.mozilla.org/2016/05/developer-edition-48-firebug-features-editable-storage-inspector-improvements-and-more/#comment-19859. Might "DOM Tree View" be better?
(it's always a tree view, I know, but...)

Comment 101

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

(In reply to Will Bamberg [:wbamberg] from comment #100)
> Sebastian, thanks for writing this page.
> 
> I'm not sure "DOM Panel" is better. I think there is also potential for
> confusion with the Inspector, as noted:
> https://hacks.mozilla.org/2016/05/developer-edition-48-firebug-features-
> editable-storage-inspector-improvements-and-more/#comment-19859. Might "DOM
> Tree View" be better?
> (it's always a tree view, I know, but...)

Or perhaps "DOM Property View" to make it more specific to the content it shows, since it's showing the properties on the `window` object?

Comment 102

[Will Bamberg [:wbamberg]]

Yes, I think that would be better, too.

Comment 103

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

(In reply to Will Bamberg [:wbamberg] from comment #102)
> Yes, I think that would be better, too.

Great, could you make the change?  DOM Inspector is in the page title, so it would require a page move to fully update.

Comment 104

[Sebastian Zartner [:sebo]]

Will obviously made the changes already. Sorry for causing confusions! Simply forgot about the old DOM Inspector extension.

Sebastian

Comment 105

[Sylvestre Ledru [:Sylvestre]]

Sebastian, Do you know when/if this is going to be enabled by default?
I think we should relnote only when it is

Comment 106

[Sebastian Zartner [:sebo]]

(In reply to Sylvestre Ledru [:sylvestre] from comment #105)
> Sebastian, Do you know when/if this is going to be enabled by default?

I guess it may be enabled by default at some point, but I'm not the right person to ask. Forwarding the question to Honza.

> I think we should relnote only when it is

Some features within release notes are behind preferences, like e.g. the Firebug theme in 48. I think it's still worth providing a release note for this feature to let people know about it.

Sebastian

Comment 107

[Jan Honza Odvarko [:Honza] (always need-info? me)]

(In reply to Sebastian Zartner [:sebo] from comment #106)
> (In reply to Sylvestre Ledru [:sylvestre] from comment #105)
> > Sebastian, Do you know when/if this is going to be enabled by default?
> 
> I guess it may be enabled by default at some point, but I'm not the right
> person to ask. Forwarding the question to Honza.
Yes, it might be, but there is no such plan for now as far as I know.

Honza

Comment 108

[janekptacijarabaci]

See also bug 1320810

Comment 109

[Liz Henry (:lizzard) (relman/hg->git project)]

At this point, I don't think we still need to add a release note - the moment has passed.