Closed
Bug 1132203
Opened 9 years ago
Closed 9 years ago
JSON response viewer
Categories
(DevTools :: JSON Viewer, defect, P1)
Tracking
(firefox44 fixed, relnote-firefox 44+)
RESOLVED
FIXED
Firefox 44
People
(Reporter: canuckistani, Assigned: Honza)
References
(Blocks 1 open bug)
Details
(Keywords: dev-doc-complete, Whiteboard: [polish-backlog])
Attachments
(8 files, 33 obsolete files)
4.20 KB,
patch
|
bgrins
:
review+
RyanVM
:
checkin+
|
Details | Diff | Splinter Review |
84.15 KB,
patch
|
Honza
:
review+
|
Details | Diff | Splinter Review |
117.94 KB,
patch
|
Honza
:
review+
|
Details | Diff | Splinter Review |
25.83 KB,
patch
|
Honza
:
review+
|
Details | Diff | Splinter Review |
15.32 KB,
patch
|
Honza
:
review+
|
Details | Diff | Splinter Review |
10.89 KB,
patch
|
bzbarsky
:
review+
|
Details | Diff | Splinter Review |
8.16 KB,
patch
|
jryans
:
review+
|
Details | Diff | Splinter Review |
2.58 KB,
patch
|
billm
:
review+
|
Details | Diff | Splinter Review |
When I open a JSON xhr response in a new tab, I want to see by default a formatted / highlighted version by default, with the following options: * view raw ouput * filter results ( by property name or value ) with live updates to the tree as I type * view netmonitor-ish request info ( headers, timings, etc )
Reporter | ||
Comment 1•9 years ago
|
||
* consider a view that, when a JSON parsing error occurs, tries to provide the exact place in the file where the problem occurred.
Reporter | ||
Updated•9 years ago
|
Whiteboard: [devedition-40]
Assignee | ||
Updated•9 years ago
|
Assignee: nobody → odvarko
Comment 3•9 years ago
|
||
Let's also consider being able to JSON parse websocket raws (send/receives) and ASCII based protocol upgrades like STOMP of socketIO. This would depend on whatever websocket implementation we have in the network panel.
Reporter | ||
Comment 4•9 years ago
|
||
Assigning P1 priority for some devedition-40 bugs. Filter on '148DD49E-D162-440B-AE28-E22632FC20AC'
Priority: -- → P1
Comment 5•9 years ago
|
||
(In reply to Jeff Griffiths (:canuckistani) from comment #0) > When I open a JSON xhr response in a new tab, I want to see by default a > formatted / highlighted version by default, with the following options: Is the intention that any tab with a JSON content type is loaded in this new view, or only when the tab was opened from the Developer Tools?
Assignee | ||
Comment 6•9 years ago
|
||
(In reply to Brian Grinstead [:bgrins] from comment #5) > Is the intention that any tab with a JSON content type is loaded in this new > view, or only when the tab was opened from the Developer Tools? See this thread: https://groups.google.com/forum/#!topic/mozilla.dev.platform/QmQSsujQSaA Also, here is github repo with the current prototype: https://github.com/janodvarko/prototypes/tree/master/json-viewer (+ a screenshot: http://snag.gy/BdUN5.jpg) Honza
Assignee | ||
Comment 7•9 years ago
|
||
Also, not sure if this is a dup: https://bugzilla.mozilla.org/show_bug.cgi?id=554025 Honza
Assignee | ||
Comment 9•9 years ago
|
||
Just a note that a prototype of this tool is available as an extension: https://github.com/janodvarko/prototypes/tree/master/json-viewer Honza
Assignee | ||
Comment 10•9 years ago
|
||
Attachment #8598014 -
Flags: review?(bgrinstead)
Assignee | ||
Comment 11•9 years ago
|
||
Attachment #8598015 -
Flags: review?(bgrinstead)
Assignee | ||
Comment 12•9 years ago
|
||
Brian, I've attached two patches: 1. bug1132203-libs-1.patch: contains RequireJS + ReactJS 2. bug1132203-viewer-1.patch: JSON Viewer implementation add #1: - Not sure whether we should use an existin instance of these libs in the tree add #2: - The patch might be still quite big. We can have 1:1 and I can introduce you the code structure. - I wasn't sure where should be the entry point that loads the entire feature (root module). I used devtools/main, but there might be a better place. - There are a few places where a new prefs should be introduced (limit for cropping strings, max number of displayed items in an array). We can introduce new prefs or use existing if possible. - We should make sure the localized strings land in time (perhaps in advance if necessary) - Two themese are supported (Light and Firebug) Dark is TBD - I didn't pay too much attention to invalid JSON yet - e10s is supported - I have been using this test page: https://api.github.com/orgs/firebug/repos Honza
Comment 13•9 years ago
|
||
Do we need to include the require.js library in tree? Don't we have similar functionality already available?
Flags: needinfo?(odvarko)
Comment 14•9 years ago
|
||
I was wondering about landing react in tree with regards to a license and security review, but I see that the loop team has already landed it (originally in Bug 1033841). It's here: https://dxr.mozilla.org/mozilla-central/source/browser/components/loop/content/shared/libs/react-0.12.2.js.
See Also: → 1033841
Comment 15•9 years ago
|
||
Comment on attachment 8598015 [details] [diff] [review] bug1132203-viewer-1.patch Review of attachment 8598015 [details] [diff] [review]: ----------------------------------------------------------------- I'm not sure how difficult this will be to untangle, but is it possible to split this up into two patches - one that just has the stream converter stuff and the other that implements the json viewer frontend? That'd make it easier to review, and I'd like to get a browser peer to review the converter stuff.
Attachment #8598015 -
Flags: review?(bgrinstead)
Comment 16•9 years ago
|
||
(In reply to Brian Grinstead [:bgrins] from comment #15) > Comment on attachment 8598015 [details] [diff] [review] > bug1132203-viewer-1.patch > > Review of attachment 8598015 [details] [diff] [review]: > ----------------------------------------------------------------- > > I'm not sure how difficult this will be to untangle, but is it possible to > split this up into two patches - one that just has the stream converter > stuff and the other that implements the json viewer frontend? That'd make > it easier to review, and I'd like to get a browser peer to review the > converter stuff. Never mind, looking again I see the converter stuff is pretty much isolated to convertor-child.js.
Comment 17•9 years ago
|
||
Comment on attachment 8598015 [details] [diff] [review] bug1132203-viewer-1.patch Review of attachment 8598015 [details] [diff] [review]: ----------------------------------------------------------------- This is in response to this thread: https://groups.google.com/forum/#!topic/mozilla.dev.platform/QmQSsujQSaA/discussion. There's a file in this patch called convertor-child.js that implements the JSON viewer directly in the page when loading an application/json url (like https://api.github.com/orgs/firebug/repos). What do you think about this approach?
Attachment #8598015 -
Flags: feedback?(fbraun)
Comment 18•9 years ago
|
||
Comment on attachment 8598015 [details] [diff] [review] bug1132203-viewer-1.patch I'm afraid I don't know our code well enough to give meaningful feedback on the exact implementation. What I did notice was that it looks like the `htmlEncode` function should be called more often (e.g., in toHTML).
Attachment #8598015 -
Flags: feedback?(fbraun)
Assignee | ||
Comment 19•9 years ago
|
||
(In reply to Brian Grinstead [:bgrins] from comment #13) > Do we need to include the require.js library in tree? Don't we have similar > functionality already available? I don't know if we have something similar, but note that modules are loaded in content scope with limited privileges. (In reply to Brian Grinstead [:bgrins] from comment #14) > I was wondering about landing react in tree with regards to a license and > security review, but I see that the loop team has already landed it > (originally in Bug 1033841). It's here: > https://dxr.mozilla.org/mozilla-central/source/browser/components/loop/ > content/shared/libs/react-0.12.2.js. Yep, I know about this one. So, do you think we should refer it? I noticed that the URL changes with new version... (In reply to Brian Grinstead [:bgrins] from comment #17) > Comment on attachment 8598015 [details] [diff] [review] > bug1132203-viewer-1.patch > > Review of attachment 8598015 [details] [diff] [review]: > ----------------------------------------------------------------- > > This is in response to this thread: > https://groups.google.com/forum/#!topic/mozilla.dev.platform/QmQSsujQSaA/ > discussion. There's a file in this patch called convertor-child.js that > implements the JSON viewer directly in the page when loading an > application/json url (like https://api.github.com/orgs/firebug/repos). What > do you think about this approach? That's why I started the thread in the first place and the feedback is rather positive. So, my gut feeling is that we can go ahead and do it this way (if security review is positive) Honza
Flags: needinfo?(odvarko)
Assignee | ||
Comment 20•9 years ago
|
||
Btw. I am using github and bug1132203 branch to track changes https://github.com/janodvarko/gecko-dev/commits/bug1132203 Honza
Comment 21•9 years ago
|
||
Comment on attachment 8598015 [details] [diff] [review] bug1132203-viewer-1.patch Review of attachment 8598015 [details] [diff] [review]: ----------------------------------------------------------------- Even with the positive feedback from the mailing list I'd still rather have Gijs take a look at the converter-child implementation. I'll flag him when he gets back and is accepting review requests. ::: browser/devtools/jsonview/convertor-child.js @@ +158,5 @@ > + defineAs: "postChromeMessage" > + }); > + }) > + > + // This regex attempts to match a JSONP structure: I don't think we should handle JSONP (at least for v1). It adds some complexity and I don't think this function will even run with a JSONP result anyway, since JSONP should be returned with the `application/javascript` mime type.
Comment 22•9 years ago
|
||
(In reply to Jan Honza Odvarko [:Honza] from comment #12) > - The patch might be still quite big. We can have 1:1 and I can introduce > you the code structure. That would be helpful. Or if time is of the essence, we should find someone who already knows react to do the code review. > - I wasn't sure where should be the entry point that loads the entire > feature (root module). I used devtools/main, but there might be a better > place. I'm assuming this entry point should live somewhere outside of devtools (maybe wherever the xml pretty printing is set up)? > - We should make sure the localized strings land in time (perhaps in advance > if necessary) We should do this. Can you split out the strings as a separate patch?
Updated•9 years ago
|
Status: NEW → ASSIGNED
Comment 23•9 years ago
|
||
Comment on attachment 8598015 [details] [diff] [review] bug1132203-viewer-1.patch Gijs, this patch is implementing the JSON previewer as discussed in https://groups.google.com/forum/#!topic/mozilla.dev.platform/QmQSsujQSaA/discussion. There's a file in called convertor-child.js that implements "nsIStreamConverter", "nsIStreamListener", and "nsIRequestObserver" to show this preview when loading application/json url (like https://api.github.com/orgs/firebug/repos). I don't know enough about these interfaces to review that file, can you please take a look at it and give feedback on the implementation?
Attachment #8598015 -
Flags: feedback?(gijskruitbosch+bugs)
Comment 24•9 years ago
|
||
Comment on attachment 8598015 [details] [diff] [review] bug1132203-viewer-1.patch Review of attachment 8598015 [details] [diff] [review]: ----------------------------------------------------------------- The code style here is very foreign to me, and I don't know anything about stream conversion. 302 bsmedberg, who may redirect again. Sorry. :-\ I'll note that all the SVG is screaming for cleanup, and that all of this feels like about 10 times as much code as I would have thought we'd need for a feature like this. What is "reps" and why do we need it? ::: browser/devtools/jsonview/moz.build @@ +9,5 @@ > + 'css', > + 'lib' > +] > + > +EXTRA_JS_MODULES.devtools.jsonview += [ ehm... this looks very wrong, but ask a build peer.
Attachment #8598015 -
Flags: feedback?(gijskruitbosch+bugs) → feedback?(benjamin)
Comment 25•9 years ago
|
||
(In reply to :Gijs Kruitbosch from comment #24) > The code style here is very foreign to me, and I don't know anything about > stream conversion. 302 bsmedberg, who may redirect again. Sorry. :-\ And to be clear, I'm specifically wanting feedback on the convertor-child.js file which sets up the conversion.
Comment 26•9 years ago
|
||
Comment on attachment 8598015 [details] [diff] [review] bug1132203-viewer-1.patch I really hope I'm not still listed as a build peer. I don't know what all the dots are for but there's certainly precedent for that in the tree already.
Attachment #8598015 -
Flags: feedback?(benjamin)
Comment 27•9 years ago
|
||
(In reply to Benjamin Smedberg [:bsmedberg] from comment #26) > Comment on attachment 8598015 [details] [diff] [review] > bug1132203-viewer-1.patch > > I really hope I'm not still listed as a build peer. I don't know what all > the dots are for but there's certainly precedent for that in the tree > already. I believe that he was forwarding on my request about the stream conversion stuff in the convertor-child.js file in the patch, not the build changes. Can you take a look at those changes, or reassign to someone who could?
Flags: needinfo?(benjamin)
Comment 29•9 years ago
|
||
Gavin, can you recommend an appropriate reviewer for the stream converter stuff?
Flags: needinfo?(gavin.sharp)
Comment 30•9 years ago
|
||
bz, but he's apparently not doing reviews. Perhaps the "other honza", a.k.a. mayhemer?
Flags: needinfo?(gavin.sharp)
Comment 31•9 years ago
|
||
Comment on attachment 8598015 [details] [diff] [review] bug1132203-viewer-1.patch Honza, this patch is implementing the JSON previewer as discussed in https://groups.google.com/forum/#!topic/mozilla.dev.platform/QmQSsujQSaA/discussion. There's a file here called convertor-child.js that implements "nsIStreamConverter", "nsIStreamListener", and "nsIRequestObserver" to show this preview when loading application/json url (like https://api.github.com/orgs/firebug/repos). I don't know enough about these interfaces to review that file, can you please take a look at it and give feedback on the implementation?
Attachment #8598015 -
Flags: feedback?(honzab.moz)
Assignee | ||
Comment 32•9 years ago
|
||
(In reply to Brian Grinstead [:bgrins]) > I don't think we should handle JSONP (at least for v1). Agree, removed > I'm assuming this entry point should live somewhere outside of devtools > (maybe wherever the xml pretty printing is set up)? Yes, precisely. The feature should be available even if the developer Toolbox isn't even opened. Any tips where to load the main json viewer module? > We should do this. Can you split out the strings as a separate patch? Done (In reply to :Gijs Kruitbosch from comment #24) > What is "reps" and why do we need it? This directory contains list of generic ReactJS templates that are used to render various JS types (e.g. string, array, null, undefined, etc.) Yes, it's more code that is necessary for now, but this repository of templates should be shared and used also at other places within the developer Toolbox UI in the future (possible anywhere we need to render JS object). --- I am also updating patches. - Displaying an error when JSON parsing breaks - CSS cleaned up a bit - JSONP support removed There are now three patches. * bug1132203-libs: ReactJS and RequireJS libs * bug1132203-locales: strings used in the UI (needs to land till 5/11) * bug1132203-viewer: the viewer implementation Honza
Attachment #8598015 -
Attachment is obsolete: true
Attachment #8598015 -
Flags: feedback?(honzab.moz)
Assignee | ||
Comment 33•9 years ago
|
||
Brian, locales should land asap (till Monday) Honza
Attachment #8602696 -
Flags: review?(bgrinstead)
Assignee | ||
Comment 34•9 years ago
|
||
(In reply to Brian Grinstead [:bgrins] from comment #31) > Comment on attachment 8598015 [details] [diff] [review] > bug1132203-viewer-1.patch > > Honza, this patch is implementing the JSON previewer as discussed in > https://groups.google.com/forum/#!topic/mozilla.dev.platform/QmQSsujQSaA/ > discussion. There's a file here called convertor-child.js that implements > "nsIStreamConverter", "nsIStreamListener", and "nsIRequestObserver" to show > this preview when loading application/json url (like > https://api.github.com/orgs/firebug/repos). I don't know enough about these > interfaces to review that file, can you please take a look at it and give > feedback on the implementation? Honza, the patch is updated and there is version 2 (bug1132203-viewer-2.patch), but the convertor-child remains the same. Honza
Assignee | ||
Comment 35•9 years ago
|
||
James, the JSON Viewer is based on ReactJS and I was hoping you could take a look and provide some feedback (or do a review). Some comments: * The entire app is injected into the (application/json) page in convertor-child.js using RequireJS * All related app files (ReactJS components) are in components directory * There is also 'reps' dir that contains set of general templates for basic JS data types. I am hoping we can share it on other places (in Toolbox UI) where JS objects should be rendered. * The root component is main-tabbed-area. Honza Honza
Flags: needinfo?(jlong)
Comment 36•9 years ago
|
||
(In reply to Jan Honza Odvarko [:Honza] from comment #32) > (In reply to :Gijs Kruitbosch from comment #24) > > What is "reps" and why do we need it? > This directory contains list of generic ReactJS templates that > are used to render various JS types (e.g. string, array, null, undefined, > etc.) > Yes, it's more code that is necessary for now, but this repository > of templates should be shared and used also at other places within the > developer Toolbox UI in the future (possible anywhere we need to render JS > object). But surely we already have a lot of code for this that's used in the Object Viewer?
Assignee | ||
Comment 37•9 years ago
|
||
(In reply to :Gijs Kruitbosch from comment #36) > But surely we already have a lot of code for this that's used in the Object > Viewer? Do you mean Variables View? Note that this is ReactJS based. Honza
Comment 38•9 years ago
|
||
(In reply to Jan Honza Odvarko [:Honza] from comment #37) > (In reply to :Gijs Kruitbosch from comment #36) > > But surely we already have a lot of code for this that's used in the Object > > Viewer? > Do you mean Variables View? "The thing that opens when you click a JS object that shows in the console" Bugzilla calls it "Object Viewer". Whatever it's called - that thing. > Note that this is ReactJS based. That by itself does not seem a good enough reason to have two (big) chunks of code that do identical things.
Assignee | ||
Comment 39•9 years ago
|
||
(In reply to :Gijs Kruitbosch from comment #38) > (In reply to Jan Honza Odvarko [:Honza] from comment #37) > > (In reply to :Gijs Kruitbosch from comment #36) > > > But surely we already have a lot of code for this that's used in the Object > > > Viewer? > > Do you mean Variables View? > > "The thing that opens when you click a JS object that shows in the console" > > Bugzilla calls it "Object Viewer". Whatever it's called - that thing. Yep, that's the thing > > > Note that this is ReactJS based. > > That by itself does not seem a good enough reason to have two (big) chunks > of code that do identical things. Well, one of the goals in this case was trying out ReactJS. Honza
Comment 40•9 years ago
|
||
Comment on attachment 8602694 [details] [diff] [review] bug1132203-viewer-2.patch Looks like the feedback from Comment 31 got cleared
Attachment #8602694 -
Flags: feedback?(honzab.moz)
Comment 41•9 years ago
|
||
(In reply to Jan Honza Odvarko [:Honza] from comment #39) > (In reply to :Gijs Kruitbosch from comment #38) > > (In reply to Jan Honza Odvarko [:Honza] from comment #37) > > > (In reply to :Gijs Kruitbosch from comment #36) > > > > But surely we already have a lot of code for this that's used in the Object > > > > Viewer? > > > Do you mean Variables View? > > > > "The thing that opens when you click a JS object that shows in the console" > > > > Bugzilla calls it "Object Viewer". Whatever it's called - that thing. > Yep, that's the thing > > > > Note that this is ReactJS based. > > > > That by itself does not seem a good enough reason to have two (big) chunks > > of code that do identical things. Is part of the plan to eventually make this new object viewer be the default variable view, or would these two remain distinct? One difference here is that the data source is a plain old object, while the variable view deals with value grips from the server. There are some features that the current VariablesView has that would need to be added before a switch could happen (like change flashing, inline editing, tooltips, and upcoming large object handling in Bug 1023386). I wonder if there's a way to adapt some of the new frontend ideas into that existing widget so we can share any future improvements across the features - sort of swapping out the 'Scope' and 'Variable' classes. I'm not familiar enough with either code base to say whether that'd be sane.
Comment 42•9 years ago
|
||
I feel like we're pretty far away from replacing something like the variable view with a React component. I'm going to look closer at this code soon, but I think we need to take the approach of slowly applying React where it makes sense, and eventually replace the variables view all at once. I'm not entirely sure where the current React work fits in, but I'm going to take a look at the code and get more familiar with it.
Comment 43•9 years ago
|
||
(In reply to Brian Grinstead [:bgrins] from comment #41) > One difference here is > that the data source is a plain old object, while the variable view deals > with value grips from the server. FWIW, the current variables view can deal with both plain js objects and grips.
Comment 44•9 years ago
|
||
To Gijs' point, why should we use this frontend today instead of just plugging in a VariablesView widget into the content? Is there a technical limitation (ie are we unable to use XUL), or some missing feature / shortcoming with it's UI that prevents it from being useful for this kind of display? I'm not necessarily opposed to doing something new if we can share it across the tools and the new thing will make that easier or provide some other benefit. In particular, moving to an HTML implementation would be nice. But adding something completely new and building good accessibility, keyboard shortcuts, context menu items, test coverage, etc is a lot of work. (In reply to James Long (:jlongster) from comment #42) > I'm not entirely sure where the current React work fits in, but I'm going to > take a look at the code and get more familiar with it. OK that would help, thanks.
Comment 45•9 years ago
|
||
Just to help me put things in context: is the plan that this will eventually become a new and improved Variables View? That's what I'm getting from Comment 32.
Flags: needinfo?(odvarko)
Comment 46•9 years ago
|
||
Comment on attachment 8602696 [details] [diff] [review] bug1132203-locales-1.patch Review of attachment 8602696 [details] [diff] [review]: ----------------------------------------------------------------- Strings look fine, these will need to land before 5-11
Attachment #8602696 -
Flags: review?(bgrinstead) → review+
Comment 47•9 years ago
|
||
A few comments: * Uppercasing the DOM elements unnecessary. No React style guide is in favor of that, and I would advise against it * Do not export the component wrapped in React.createFactory. This is bad for tests and you need access to the component class at various times. Wrap it when you import the component, and you can make it easier by creating a utility function so you can use destructuring (`var { Foo, Bar } = Elements(require('./components'))`). * Nit: the `render` method usually goes at the bottom of the component * Avoid refs at all costs. I don't see any need to use refs in tabs.js, or search-box.js. There might be other places. * There is inconsistent usage of dom elements. Some use the refs.LI, others use React.createElement('li). I would recommend always doing `var dom = React.DOM` and just using `dom.li` everywhere. * There's also a lot of style inconsistencies and things that conflict with our styles. I saw several places like this: if(newProps.tabActive){ this.setState({tabActive: newProps.tabActive}) } needs a space after if, and the code inside really should be bumped down to the next line. * I don't see any tests? That's what I see from a quick glance. This is a cool endeavor, but to be honest I'm sad that we weren't involved earlier because not only would I have liked to help develop this, but I'm working on React stuff right now. There are several things that need to be improved in these React components. More importantly I'm developing a careful strategy for introducing React into devtools and this doesn't really jive with it. There's no reason to replace the variables view right now, you could easily wrap it in a single React component and be done with it. Far down the road we could replace the internals with an actual React component, but I don't see the need to do that right now (there is a lot of functionality in there). The more important thing is to structure our code in a way where using React in our tools is even possible. We'll get the most out of React when we use it to layout and control each tool's UI and state, not just replacing individual components. So I'm not exactly sure where this fits into the picture; I don't want to spend months rewriting our individual components because everyone will wonder what the actual benefit was. However, when we use React to *compose* various components and control the state of our debugger UI, you'll see the benefits very quickly.
Updated•9 years ago
|
Flags: needinfo?(jlong)
Assignee | ||
Comment 48•9 years ago
|
||
(In reply to James Long (:jlongster) from comment #47) Thanks for the feedback James. See my comments, I am starting from your bottom note. > That's what I see from a quick glance. This is a cool endeavor, but to be > honest I'm sad that we weren't involved earlier because not only would I > have liked to help develop this, This is the beginning and that's why I CCed you here in the first place. It's great if we cooperate on this. I used ReactJS when implementing couple for developer tools extensions [1] (on top of native dev tools). Not only to ship new tools, but also show other extension developers how to integrate and use 3rd party web libs in add-ons. It's been quite common requirement to render bits and pieces of JSON in the UI, for example, RDP inspector renders RDP packet content, FireQuery renders jQuery data (JSON) associated with HTML elements, etc. It's about rendering plain JSON, i.e. use JSON as an input and get nice and (colored) HTML markup that represents it. Markup that can be simply themed and we can do support for Light, Dark and also for other custom themes like Firebug. It isn't about replacing VariablesView. Here is an example: rendering preview of jQuery data in a tooltip, which is displayed within the Markup view. Content of the tooltip should display syntax-colored JSON object preview. The tooltip (shared/widgets/tooltip) already knows how to render content using HTML (through setIFrameContent [2]) and all which is needed to do is just provide the markup. It's just straightforward to use ReactJS for the transformation JSON -> HTML. Also my gut feeling is that using VariablesView in the tooltip wouldn't fit so well. Also note that contents of the tooltip is rendered within an iframe with type="content" (as a step towards UI that is entirely running inside type="cotent" scope) VariablesView needs chrome privileges. > but I'm working on React stuff right now. > There are several things that need to be improved in these React components. > More importantly I'm developing a careful strategy for introducing React > into devtools and this doesn't really jive with it. I'd love to know more about the strategy you are working on and see how things should be done, so they fit into it. Perhaps we could have a meeting? > There's no reason to replace the variables view right now Agreed. Rewriting existing and working components wouldn't be an effective strategy at this point. > , you could easily wrap it in a single > React component and be done with it. Far down the road we could replace the > internals with an actual React component, but I don't see the need to do > that right now (there is a lot of functionality in there). > > The more important thing is to structure our code in a way where using React > in our tools is even possible. I did several experiments when extending devtools and rendering additional information in the existing UI using React. E.g. customizing the Console or Markup View - by injecting React into the appropriate panel frames and using it's API or creating new frame (e.g. for the aforementioned tooltip). > We'll get the most out of React when we use > it to layout and control each tool's UI and state, not just replacing > individual components. Sounds good to me. > So I'm not exactly sure where this fits into the picture; I don't want to > spend months rewriting our individual components because everyone will > wonder what the actual benefit was. However, when we use React to *compose* > various components and control the state of our debugger UI, you'll see the > benefits very quickly. To finish the picture I started describing above. In order to render JSON bits (in all those mentioned add-ons) I created set of small React templates (that's what you could see in the 'reps' directory). It works even in content scope with no privileges (no XUL no chrome API). And since the "JSON Viewer" is based on stream-convertor approach where the logic lives in-content (under the same URL from where the original JSON comes from), it feels like a fitting approach to use React and existing templates since it doesn't need chrome privileges (VariablesView requires chrome scope). Another example: The same set of templates can be later use for rendering objects within the Console panel or anywhere else in the UI - at some point we could have, step by step, one unified way how objects are rendered within the Toolbox UI. VariablesView can use it to render object's values (it's currently just plain text) or even for the layout in the future when we decide that it's the right time. In any case, I am open for any suggestions or better approaches! > A few comments: > > * Uppercasing the DOM elements unnecessary. No React style guide is in favor > of that, and I would advise against it > > * Do not export the component wrapped in React.createFactory. This is bad > for tests and you need access to the component class at various times. Wrap > it when you import the component, and you can make it easier by creating a > utility function so you can use destructuring (`var { Foo, Bar } = > Elements(require('./components'))`). > > * Nit: the `render` method usually goes at the bottom of the component > > * Avoid refs at all costs. I don't see any need to use refs in tabs.js, or > search-box.js. There might be other places. > > * There is inconsistent usage of dom elements. Some use the refs.LI, others > use React.createElement('li). I would recommend always doing `var dom = > React.DOM` and just using `dom.li` everywhere. > > * There's also a lot of style inconsistencies and things that conflict with > our styles. I saw several places like this: > > if(newProps.tabActive){ this.setState({tabActive: newProps.tabActive}) } > > needs a space after if, and the code inside really should be bumped down to > the next line. > > * I don't see any tests? The current patch is a prototype I quickly built, so I didn't bother writing tests. I'll start writing tests as soon as we agree on the concept we want to use to build it. Btw. I agree with all the points above, I can fix it. Honza [1] Developer Tools Extension: * FireQuery: https://github.com/firebug/firequery * RDP Inspector: https://hacks.mozilla.org/2015/05/rdp-inspector-an-extension-for-firefox-developer-tools/ * Pixel Perfect: https://hacks.mozilla.org/2015/03/pixel-perfect-2-extension-for-firefox-developer-tools/ * Firebug.next: https://github.com/firebug/firebug.next [2] jQuery Tooltip: https://github.com/firebug/firequery/blob/master/lib/data-tooltip.js#L52
Flags: needinfo?(odvarko)
Comment 49•9 years ago
|
||
(In reply to Jan Honza Odvarko [:Honza] from comment #48) > (In reply to James Long (:jlongster) from comment #47) > > Thanks for the feedback James. See my comments, I am starting from your > bottom note. > > > > That's what I see from a quick glance. This is a cool endeavor, but to be > > honest I'm sad that we weren't involved earlier because not only would I > > have liked to help develop this, > > This is the beginning and that's why I CCed you here in the first place. > It's great if we cooperate on this. Ok, sounds good. This seemed like a lot of code at first. It's great that you all jumped into React and made this! > > I used ReactJS when implementing couple for developer tools extensions [1] > (on top of native dev tools). Not only to ship new tools, but also show > other extension developers how to integrate and use 3rd party web libs in > add-ons. > > It's been quite common requirement to render bits and pieces of JSON in the > UI, for example, RDP inspector renders RDP packet content, FireQuery renders > jQuery data (JSON) associated with HTML elements, etc. > > It's about rendering plain JSON, i.e. use JSON as an input and get nice and > (colored) HTML markup that represents it. Markup that can be simply themed > and we can do support for Light, Dark and also for other custom themes like > Firebug. It isn't about replacing VariablesView. I see now. I suppose we have bits of code around to do that, but that is a lot easier than VariablesView. It's fine to make a new thing for sure, but I'm still hesitant to introduce React at the bottom of the stack like this. If you want to offer it for devtools extensions, that's awesome. When we get to the point of replacing parts of our individual components, this will exist and we can take it and use it. My worry is that we have limited resources, and I want them to be spent on refactoring our code so that React can be front and center, not just for individual components. I know that I don't really have to help develop this because I'm working on cleaning up the debugger for a future react/flux architecture. If I start working on this, I will have little time to work on that. > > > > but I'm working on React stuff right now. > > There are several things that need to be improved in these React components. > > More importantly I'm developing a careful strategy for introducing React > > into devtools and this doesn't really jive with it. > > I'd love to know more about the strategy you are working on and see how > things should be done, > so they fit into it. Perhaps we could have a meeting? Sure, I'm happy to meet. Ping me in IRC? I'm generally free. I outlined what I'm thinking above. I'm hesitant to land this in our codebase because that means we have to maintain it. I'd much rather wrap our existing code in a React component, though I see what you mean about requiring Chrome privileges. > > , you could easily wrap it in a single > > React component and be done with it. Far down the road we could replace the > > internals with an actual React component, but I don't see the need to do > > that right now (there is a lot of functionality in there). > > > > The more important thing is to structure our code in a way where using React > > in our tools is even possible. > > I did several experiments when extending devtools and rendering additional > information in the existing UI using React. E.g. customizing the Console or > Markup View - by injecting React into the appropriate panel frames and using > it's API or creating new frame (e.g. for the aforementioned tooltip). Yeah, while I think there are places that might help, I'm super interested in just figuring out how to use React to help with comprehending our tools in general, and provide interfaces to composing our "widgets". I was planning on wrapping a lot of our existing widgets in React components, because you could get far really fast. > > Another example: The same set of templates can be later use for rendering > objects within the Console panel or anywhere else in the UI - at some point > we could have, step by step, one unified way how objects are rendered within > the Toolbox UI. VariablesView can use it to render object's values (it's > currently just plain text) or even for the layout in the future when we > decide that it's the right time. I absolutely want to get there, but I think we first need to establish some standard practices for how tools are generally written. Having a bunch of low-level components in React isn't really going to help our current tools, if anything it might make it more awkward because if a tool doesn't leverage React itself it's going to be awkward to compose React components. We need to experiment with various architectures first which I'm (in a careful way) doing with the debugger, where React has a role within the tool itself. I've discovered that it's actually more important to adopt a Flux-style architecture first and then we can talk about React. But even deciding on that feels a good ways off, which is why I'm hesitant to even begin to start adopting low-level React components. Let's definitely meet and talk about it.
Comment 50•9 years ago
|
||
Shoot, when I wrote "I know that I don't really have to help develop this because I'm working", I meant "I don't have really have *time* to help"
Reporter | ||
Comment 51•9 years ago
|
||
(In reply to James Long (:jlongster) from comment #47) > So I'm not exactly sure where this fits into the picture; I don't want to > spend months rewriting our individual components because everyone will > wonder what the actual benefit was. However, when we use React to *compose* > various components and control the state of our debugger UI, you'll see the > benefits very quickly. I asked for this feature and we got Honza working on it. It's a very scoped feature: - it's not part of the toolbox - it's not going to be enabled by default except on dev edition - for our users, anything we do here is going to be better than what we ( and chrome ) currently do, so it can be a prototype with some known issues and still be useful. It's really just a cool developer-oriented browser based on user feedback[1] and especially the dominance of addons that implement this feature in both Firefox and Chrome. Chrome's json viewer addons have millions of viewers[2]. For what it's worth, I think of this project as an experiment in the small for doing UI differently, based on Honza's work in addons. It's not meant to be a project that leverages existing code or development methods. I do really appreciate getting Jame's feedback on the use of react though, and generally everyone's feedback as we go along. Honza: when it gets to be time, I'd love to have try builds to test. [1] http://ffdevtools.uservoice.com/forums/246087-firefox-developer-tools-ideas/suggestions/6413798-show-json-as-structured-data-just-like-xml-files [2] https://chrome.google.com/webstore/search/JSON?_category=extensions
Comment 52•9 years ago
|
||
Ah ok, I didn't realize this wasn't going into the toolbox. Thanks for clarifying Jeff, those 3 points make a lot of sense. Alright then, for my general feedback, I'd say the two most important parts are: don't export components wrapped in React.createFactory, and avoid refs. Not sure I'll have time to take a much closer look, but let me know if you want me more involved.
Assignee | ||
Comment 53•9 years ago
|
||
Comment on attachment 8602696 [details] [diff] [review] bug1132203-locales-1.patch This patch introduces some new UI string and should land today. The bug stays open to finish the implementation... Honza
Attachment #8602696 -
Flags: checkin?(cbook)
Assignee | ||
Comment 54•9 years ago
|
||
Jeff, here is a try build: Results: https://treeherder.mozilla.org/#/jobs?repo=try&revision=d6f3ec09cfca Builds: https://ftp-ssl.mozilla.org/pub/mozilla.org/firefox/try-builds/jodvarko@mozilla.com-d6f3ec09cfca Honza
Comment 55•9 years ago
|
||
(In reply to Jan Honza Odvarko [:Honza] from comment #53) > Comment on attachment 8602696 [details] [diff] [review] > bug1132203-locales-1.patch > > This patch introduces some new UI string and should land today. > > The bug stays open to finish the implementation... > > Honza Ryan can you land this today when fx-team is open again ?
Flags: needinfo?(ryanvm)
Comment 56•9 years ago
|
||
Comment on attachment 8602696 [details] [diff] [review] bug1132203-locales-1.patch https://hg.mozilla.org/mozilla-central/rev/0574b7cd8e72
Flags: needinfo?(ryanvm)
Attachment #8602696 -
Flags: checkin?(cbook) → checkin+
Assignee | ||
Comment 57•9 years ago
|
||
I am attaching a patch with changes I mentioned yesterday: - CSP warning fixed - JSON tree optimized - Enabled only in devedition Honza
Attachment #8602694 -
Attachment is obsolete: true
Attachment #8602694 -
Flags: feedback?(honzab.moz)
Attachment #8605264 -
Flags: review?(bgrinstead)
Assignee | ||
Updated•9 years ago
|
Attachment #8605264 -
Flags: feedback?(honzab.moz)
Updated•9 years ago
|
Attachment #8605264 -
Flags: review?(bgrinstead) → review?(jlong)
Assignee | ||
Comment 58•9 years ago
|
||
I am attaching a new patch with fixes for Jeff's feedback. - Fix Save action in e10s - Copy headers in raw format - Fix pretty print - CSS style for clicked toolbar buttons @James: please let me know if I shouldn't update the patch ATM (review in progress). In any case, you can follow my changes (individual commits) also here: https://github.com/janodvarko/gecko-dev/commits/bug1132203 Honza
Attachment #8605264 -
Attachment is obsolete: true
Attachment #8605264 -
Flags: review?(jlong)
Attachment #8605264 -
Flags: feedback?(honzab.moz)
Attachment #8605834 -
Flags: review?(jlong)
Attachment #8605834 -
Flags: feedback?(honzab.moz)
Assignee | ||
Comment 59•9 years ago
|
||
Try build: Results will be displayed on Treeherder as they come in: https://treeherder.mozilla.org/#/jobs?repo=try&revision=5b294d861bb4 Once completed, builds and logs will be available at: https://ftp-ssl.mozilla.org/pub/mozilla.org/firefox/try-builds/jodvarko@mozilla.com-5b294d861bb4 Can anyone check out the tests? There are two failures, but seem to be unrelated... Honza
Comment 60•9 years ago
|
||
I haven't started my review yet (probably will today) so you can still attach new patches. The test failures do seem unrelated (but the `eventLoop is undefined` looks a little scary, but I don't see how it could be related).
Comment 61•9 years ago
|
||
(In reply to Jan Honza Odvarko [:Honza] from comment #59) > Can anyone check out the tests? There are two failures, but seem to be > unrelated... (In reply to James Long (:jlongster) from comment #60) > I haven't started my review yet (probably will today) so you can still > attach new patches. The test failures do seem unrelated (but the `eventLoop > is undefined` looks a little scary, but I don't see how it could be related). I've seen this in other try pushes recently, it's not related to this patch. In any case, you can always retrigger a failed build by clicking on the orange test then clicking the 'refresh' icon on the bottom left of the page. This can help figure out if it's a permaorange or an intermittent.
Updated•9 years ago
|
Attachment #8598014 -
Flags: review?(bgrinstead)
Comment 62•9 years ago
|
||
Comment on attachment 8605834 [details] [diff] [review] bug1132203-viewer-4.patch Review of attachment 8605834 [details] [diff] [review]: ----------------------------------------------------------------- I have no idea what exactly I should give a feedback on here. This is not at all a code I know.
Attachment #8605834 -
Flags: feedback?(honzab.moz)
Assignee | ||
Comment 63•9 years ago
|
||
(In reply to Honza Bambas (:mayhemer) from comment #62) > Comment on attachment 8605834 [details] [diff] [review] > bug1132203-viewer-4.patch > > Review of attachment 8605834 [details] [diff] [review]: > ----------------------------------------------------------------- > > I have no idea what exactly I should give a feedback on here. Only the convertor-child.js file Honza
Comment 64•9 years ago
|
||
Comment on attachment 8605834 [details] [diff] [review] bug1132203-viewer-4.patch Review of attachment 8605834 [details] [diff] [review]: ----------------------------------------------------------------- ::: browser/devtools/jsonview/convertor-child.js @@ +87,5 @@ > + var bytesRead = 1; > + > + while (totalBytesRead < aCount && bytesRead > 0) { > + var str = {}; > + bytesRead = is.readString(-1, str); don't go over aCount. It could confuse the underlying pump code. @@ +176,5 @@ > + // See http://www.mail-archive.com/mozilla-xpcom@mozilla.org/msg04194.html > + var storage = Cc["@mozilla.org/storagestream;1"].createInstance(Ci.nsIStorageStream); > + > + // I have no idea what to pick for the first parameter (segments) > + storage.init(4, 0xffffffff, null); according http://hg.mozilla.org/mozilla-central/annotate/8d8df22fe72d/xpcom/io/nsIStorageStream.idl#l20 the first arg is size of a single segment. Hence 4 is really not the number ;) I suggest 128k, depending on how large the content you expect. @@ +187,5 @@ > + binout.writeUtf8Z(outputDoc); > + binout.close(); > + > + // I can't explain it, but we need to trim 4 bytes off the front or > + // else it includes random crap BOM? or a bug? or a misuse? @@ +193,5 @@ > + var instream = storage.newInputStream(trunc); > + > + // Pass the data to the main content listener > + this.listener.onDataAvailable(this.channel, aContext, instream, 0, > + storage.length - trunc); s/storage.length - trunc/instream.available()/ @@ +200,5 @@ > + }, > + > + htmlEncode: function(t) { > + return t !== null ? t.toString().replace(/&/g,"&"). > + replace(/"/g,""").replace(/</g,"<").replace(/>/g,">") : ''; wish there were a better way... @@ +213,5 @@ > + '<base href="' + baseUrl + '">' + > + '<link rel="stylesheet" type="text/css" href="css/main.css">' + > + '<link rel="stylesheet" type="text/css" href="chrome://browser/skin/devtools/light-theme.css">' + > + '<script data-main="viewer-config" src="lib/require.js"></script>' + > + '</head><body class="' + themeClassName + '">' + is JsonViewUtils.getCurrentTheme() fully under our control? @@ +216,5 @@ > + '<script data-main="viewer-config" src="lib/require.js"></script>' + > + '</head><body class="' + themeClassName + '">' + > + '<div id="content"></div>' + > + '<div id="json">' + json + '</div>' + > + '<div id="headers">' + headers + '</div>' + shouldn't json and headers be escaped? not very secured this way.. this is chrome as I understand, right? so any code would run with chrome privs? @@ +238,5 @@ > + return '<!DOCTYPE html>\n' + > + '<html><head><title>' + this.htmlEncode(uri + ' - Error') + '</title>' + > + '<base href="' + self.data.url() + '">' + > + '</head><body>' + > + output + as well here, escape it
Assignee | ||
Comment 65•9 years ago
|
||
(In reply to Honza Bambas (:mayhemer) from comment #64) > Comment on attachment 8605834 [details] [diff] [review] > bug1132203-viewer-4.patch > > Review of attachment 8605834 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: browser/devtools/jsonview/convertor-child.js > @@ +87,5 @@ > > + var bytesRead = 1; > > + > > + while (totalBytesRead < aCount && bytesRead > 0) { > > + var str = {}; > > + bytesRead = is.readString(-1, str); > > don't go over aCount. It could confuse the underlying pump code. I don't understand what you mean here. What exactly should I fix? > > @@ +176,5 @@ > > + // See http://www.mail-archive.com/mozilla-xpcom@mozilla.org/msg04194.html > > + var storage = Cc["@mozilla.org/storagestream;1"].createInstance(Ci.nsIStorageStream); > > + > > + // I have no idea what to pick for the first parameter (segments) > > + storage.init(4, 0xffffffff, null); > > according > http://hg.mozilla.org/mozilla-central/annotate/8d8df22fe72d/xpcom/io/ > nsIStorageStream.idl#l20 the first arg is size of a single segment. Hence 4 > is really not the number ;) I suggest 128k, depending on how large the > content you expect. Fixed > > @@ +187,5 @@ > > + binout.writeUtf8Z(outputDoc); > > + binout.close(); > > + > > + // I can't explain it, but we need to trim 4 bytes off the front or > > + // else it includes random crap > > BOM? or a bug? or a misuse? I can't explain it > > @@ +193,5 @@ > > + var instream = storage.newInputStream(trunc); > > + > > + // Pass the data to the main content listener > > + this.listener.onDataAvailable(this.channel, aContext, instream, 0, > > + storage.length - trunc); > > s/storage.length - trunc/instream.available()/ Fixed > > @@ +200,5 @@ > > + }, > > + > > + htmlEncode: function(t) { > > + return t !== null ? t.toString().replace(/&/g,"&"). > > + replace(/"/g,""").replace(/</g,"<").replace(/>/g,">") : ''; > > wish there were a better way... > > @@ +213,5 @@ > > + '<base href="' + baseUrl + '">' + > > + '<link rel="stylesheet" type="text/css" href="css/main.css">' + > > + '<link rel="stylesheet" type="text/css" href="chrome://browser/skin/devtools/light-theme.css">' + > > + '<script data-main="viewer-config" src="lib/require.js"></script>' + > > + '</head><body class="' + themeClassName + '">' + > > is JsonViewUtils.getCurrentTheme() fully under our control? > > @@ +216,5 @@ > > + '<script data-main="viewer-config" src="lib/require.js"></script>' + > > + '</head><body class="' + themeClassName + '">' + > > + '<div id="content"></div>' + > > + '<div id="json">' + json + '</div>' + > > + '<div id="headers">' + headers + '</div>' + > > shouldn't json and headers be escaped? not very secured this way.. this is > chrome as I understand, right? so any code would run with chrome privs? Fixed, but note that the code run in content scope (no chrome privs) > > @@ +238,5 @@ > > + return '<!DOCTYPE html>\n' + > > + '<html><head><title>' + this.htmlEncode(uri + ' - Error') + '</title>' + > > + '<base href="' + self.data.url() + '">' + > > + '</head><body>' + > > + output + > > as well here, escape it Fixed Thanks for the review! Honza
Attachment #8605834 -
Attachment is obsolete: true
Attachment #8605834 -
Flags: review?(jlong)
Attachment #8612846 -
Flags: review?(jlong)
Attachment #8612846 -
Flags: review?(honzab.moz)
Comment 66•9 years ago
|
||
(In reply to Jan Honza Odvarko [:Honza] from comment #65) > > don't go over aCount. It could confuse the underlying pump code. > I don't understand what you mean here. What exactly should I fix? > The -1 means to read whatever is available in the stream, but you must limit it to aCount. I'll recheck the code if I just don't need new glasses ;)
Comment 67•9 years ago
|
||
Comment on attachment 8612846 [details] [diff] [review] bug1132203-viewer-5.patch Review of attachment 8612846 [details] [diff] [review]: ----------------------------------------------------------------- r=honzab ::: browser/devtools/jsonview/convertor-child.js @@ +94,5 @@ > + var str = {}; > + bytesRead = is.readString(-1, str); > + totalBytesRead += bytesRead; > + this.data += str.value; > + } The correct loop should be: while (aCount) { var str = {}; var bytesRead = is.readString(aCount, str); if (!bytesRead) { break; // actually, should never happen } aCount -= bytesRead; this.data += str.value; }
Attachment #8612846 -
Flags: review?(honzab.moz) → review+
Assignee | ||
Comment 68•9 years ago
|
||
(In reply to Honza Bambas (:mayhemer) from comment #67) > Comment on attachment 8612846 [details] [diff] [review] > bug1132203-viewer-5.patch > > Review of attachment 8612846 [details] [diff] [review]: > ----------------------------------------------------------------- > > r=honzab Included in the commit message now > ::: browser/devtools/jsonview/convertor-child.js > @@ +94,5 @@ > > + var str = {}; > > + bytesRead = is.readString(-1, str); > > + totalBytesRead += bytesRead; > > + this.data += str.value; > > + } > > The correct loop should be: > > while (aCount) { > var str = {}; > var bytesRead = is.readString(aCount, str); > if (!bytesRead) { > break; // actually, should never happen > } > aCount -= bytesRead; > this.data += str.value; > } Fixed Thanks! Honza
Attachment #8612846 -
Attachment is obsolete: true
Attachment #8612846 -
Flags: review?(jlong)
Attachment #8615188 -
Flags: review?(jlong)
Comment 69•9 years ago
|
||
Comment on attachment 8615188 [details] [diff] [review] bug1132203-viewer-6.patch Review of attachment 8615188 [details] [diff] [review]: ----------------------------------------------------------------- ::: browser/devtools/jsonview/convertor-child.js @@ +93,5 @@ > + while (aCount) { > + var str = {}; > + var bytesRead = is.readString(aCount, str); > + if (!bytesRead) { > + break; // actually, should never happen nit: here you may even want to throw, since not all of aCount has been successfully read from the input stream. This error would be otherwise lost and corrupted data would be sent to the consumer with NS_OK result in OnStopRequest.
Assignee | ||
Comment 70•9 years ago
|
||
(In reply to Honza Bambas (:mayhemer) from comment #69) > Comment on attachment 8615188 [details] [diff] [review] > bug1132203-viewer-6.patch > > Review of attachment 8615188 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: browser/devtools/jsonview/convertor-child.js > @@ +93,5 @@ > > + while (aCount) { > > + var str = {}; > > + var bytesRead = is.readString(aCount, str); > > + if (!bytesRead) { > > + break; // actually, should never happen > > nit: here you may even want to throw, since not all of aCount has been > successfully read from the input stream. This error would be otherwise lost > and corrupted data would be sent to the consumer with NS_OK result in > OnStopRequest. Fixed + updated patch attached. Honza
Attachment #8615188 -
Attachment is obsolete: true
Attachment #8615188 -
Flags: review?(jlong)
Comment 71•9 years ago
|
||
Comment on attachment 8617277 [details] [diff] [review] bug1132203-viewer-7.patch I will carve out some time next week to look at this.
Attachment #8617277 -
Flags: review?(jlong)
Reporter | ||
Comment 72•9 years ago
|
||
Honza - can I get a new try build with the latest patch to review usability?
Flags: needinfo?(odvarko)
Assignee | ||
Comment 73•9 years ago
|
||
Sure, here it goes: https://treeherder.mozilla.org/#/jobs?repo=try&revision=1a5af56716eb https://ftp-ssl.mozilla.org/pub/mozilla.org/firefox/try-builds/jodvarko@mozilla.com-1a5af56716eb Do not forget to enable: devtools.jsonview.enabled Honza
Flags: needinfo?(odvarko)
Reporter | ||
Comment 74•9 years ago
|
||
(In reply to Jan Honza Odvarko [:Honza] from comment #73) > Sure, here it goes: > https://treeherder.mozilla.org/#/jobs?repo=try&revision=1a5af56716eb > https://ftp-ssl.mozilla.org/pub/mozilla.org/firefox/try-builds/ > jodvarko@mozilla.com-1a5af56716eb Awesome, looking good. Some notes: * when I copy the raw response using the copy command, the text when pasted is pretty-printed. Do you think it should instead be whatever the state is in the raw response view? Meaning if the JSON is all on a single line, we should copy that to the clipboard, but if the JSON has been pretty-printed, we should instead copy the pretty-printed text. My idea here is that when the user clicks on the copy button they get the current state that they see in front of them * do you think clicking on 'pretty print' a second time should revert to the non-pretty-printed text? * I thought it was strange that the headers are shown Response first, then Request, but when I click copy they are request, then Response. They should at least be consistent with each other. I noticed the netmonitor also shows headers response first, then request. * there is no keyboard shortcut for focusing the filter field. When you implement this ( could be a followup, it's unclear to me which shortcut we could possibly use ) we should indicate what the shortcut is in the filter field's placeholder text.
Assignee | ||
Comment 75•9 years ago
|
||
Attachment #8598014 -
Attachment is obsolete: true
Assignee | ||
Comment 76•9 years ago
|
||
Attachment #8617277 -
Attachment is obsolete: true
Attachment #8617277 -
Flags: review?(jlong)
Assignee | ||
Comment 77•9 years ago
|
||
Updated patches attached. (In reply to Jeff Griffiths (:canuckistani) from comment #74) > Awesome, looking good. Some notes: Jeff, all your notes make sense to me, see below. Changes: 1) The JSON "Copy" command respects Pretty print state. I.e. if JSON text on the screen is pretty printed the copy is also pretty printed and vice versa. 2) Clicking "Pretty print" button the second time reverts to non-pretty-printed. 3) Copying headers to the clipboard respect the order of data on the screen (first response headers followed by request headers). Response headers are first - just like in the Net panel. 4) Keyboard shortcut: bug 1178771 5) Support for the Dark Theme 6) A few minor fixes (CSS and couple of React warnings) 7) React code style fixed Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=82ef39aedb27 Honza
Assignee | ||
Updated•9 years ago
|
Attachment #8627679 -
Flags: review?(jlong)
Assignee | ||
Updated•9 years ago
|
Attachment #8627680 -
Flags: review?(jlong)
Assignee | ||
Comment 78•9 years ago
|
||
Just to note that devtools.jsonview.enabled pref needs to be true to see the feature. Honza
Comment 79•9 years ago
|
||
Comment on attachment 8627679 [details] [diff] [review] bug1132203-libs-2.patch Review of attachment 8627679 [details] [diff] [review]: ----------------------------------------------------------------- I think we need to go ahead and commit React into our repo. We're at least going to refactor some of the debugger to use it (after my first pass refactoring lands, which is sooner than you think). Long term we can remove it for some reason we decide to go without it, but after Whistler it seems like most people are comfortable with it. I'm going to file a new bug to get it committed. There are a few things to figure out, like loading the dev version for local builds, but I talked with a few people today and we know how to do that, it's just a matter to putting it together. I'll make sure that lands soon so this isn't blocked on it. If we run into problems with that, we can land it locally for this feature.
Attachment #8627679 -
Flags: review?(jlong) → review-
Comment 80•9 years ago
|
||
Why would we use our own copy of react instead of sharing with loop/hello who also use react? No need to bloat the tree / firefox download with duplicates.
Comment 81•9 years ago
|
||
FWIW, we're currently on React 0.12.2, which is a bit dated. We're hoping to switch to 0.13.3 soon, and 0.14 is in beta. I'd probably suggest going with latest stable (eg 0.13.3) for now to avoid having to retool your code.
Comment 82•9 years ago
|
||
Yeah, that's why I don't want to share it completely. We want to share across our tools of course, but I don't have to have to track versions across major projects. For production, having 2 prod versions of React really barely extra weight. React is still changing a decent bit between versions. Not enough to be too worried, but enough that I want to move fast and upgrade our stuff quickly, and not require a lot of coordination with unrelated projects.
Comment 83•9 years ago
|
||
Eventually we probably can coordinate, but there's enough to already figure out right now that it's easiest to track it ourselves.
Comment 84•9 years ago
|
||
Honza, I will do a full review of all this code, but first can you update the code to use the version of React add in bug 1181646? We want to share it. It looks like currently you are just using the dev version unconditionally. You can get it here: resource:///modules/devtools/shared/browser/react-dev.js. The prod one is available at just `react.js`. I know you wanted to develop this as a normal webpage, which is great. You should be able to configure require.js to load react from your local copy if developing in a webpage, otherwise (if compiled into the browser), load it from our tools. I'm not sure how you want to detect the context you are running in. In some ways, this is the same dev/prod flag, so if you can figure that out, you can also just load the production `react.js` one in release.
Comment 85•9 years ago
|
||
Comment on attachment 8627680 [details] [diff] [review] bug1132203-viewer-8.patch Review of attachment 8627680 [details] [diff] [review]: ----------------------------------------------------------------- I'm going to review this in multiple steps, and won't change the r? status until after I get through most of the patch. ::: browser/devtools/jsonview/main.js @@ +34,5 @@ > +// Register for messages coming from the child process. > +let parentProcessMessageManager = Cc["@mozilla.org/parentprocessmessagemanager;1"]. > + getService(Ci.nsIMessageBroadcaster); > + > +parentProcessMessageManager.addMessageListener("devtools:jsonview:save", onSave); As mentioned in my other comment, can this file be loaded multiple times? If so, do you need to watch for the unload event and remove this event listener from the parentProcessMessageManager? Just want to make sure I understand how this is all loaded and we're not introducing a leak. ::: browser/devtools/main.js @@ +13,5 @@ > defaultTools.forEach(definition => gDevTools.registerTool(definition)); > defaultThemes.forEach(definition => gDevTools.registerTheme(definition)); > > +// JSON View in-content tool > +if (Services.prefs.getBoolPref("devtools.jsonview.enabled")) { Since it's loaded in this devtools file, does that mean it's only going to work after you've opened the devtools? How is browser/devtools/main.js loaded? It looks like it's meant to be loaded any time the tools are opened. If that's the case, couldn't devtools/json/main be evaluated multiple times?
Comment on attachment 8627680 [details] [diff] [review] bug1132203-viewer-8.patch Review of attachment 8627680 [details] [diff] [review]: ----------------------------------------------------------------- ::: browser/devtools/main.js @@ +15,5 @@ > > +// JSON View in-content tool > +if (Services.prefs.getBoolPref("devtools.jsonview.enabled")) { > + try { > + require("devtools/jsonview/main"); This is not the right file for this sort of thing. browser/devtools/main.js is loaded when a DevTools loader is first constructed. James's concern is valid: this will be loaded for each DevTools loader. If you start Browser Toolbox, there are two loaders active at once, and now two instances of this module. I think you may want to coordinate register this JSON View stuff from gDevTools.jsm instead. This JSM is loaded once per browsing session, so it's likely an easier spot. Also, like with my HAR review, I dislike require calls that don't return anything. Exporting some kind of "register" function would be more obvious, I think.
Assignee | ||
Comment 87•9 years ago
|
||
(In reply to James Long (:jlongster) from comment #84) > Honza, I will do a full review of all this code, but first can you update > the code to use the version of React add in bug 1181646? We want to share it. > > It looks like currently you are just using the dev version unconditionally. > You can get it here: > resource:///modules/devtools/shared/browser/react-dev.js. The prod one is > available at just `react.js`. > > I know you wanted to develop this as a normal webpage, which is great. You > should be able to configure require.js to load react from your local copy if > developing in a webpage, otherwise (if compiled into the browser), load it > from our tools. I'm not sure how you want to detect the context you are > running in. In some ways, this is the same dev/prod flag, so if you can > figure that out, you can also just load the production `react.js` one in > release. Done, the viewer is now using DevTools (shared) ReactJS from: resource:///modules/devtools/shared/content/react.js (or react-dev.js in case of 'browser/branding/unofficial') Honza
Attachment #8627679 -
Attachment is obsolete: true
Attachment #8634695 -
Flags: review?(jlong)
Assignee | ||
Comment 88•9 years ago
|
||
(In reply to James Long (:jlongster) from comment #85) > How is browser/devtools/main.js loaded? It looks like it's meant to be > loaded any time the tools are opened. If that's the case, couldn't > devtools/json/main be evaluated multiple times? (In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #86) > I think you may want to coordinate register this JSON View stuff from > gDevTools.jsm instead. This JSM is loaded once per browsing session, so > it's likely an easier spot. > > Also, like with my HAR review, I dislike require calls that don't return > anything. Exporting some kind of "register" function would be more obvious, > I think. Done, JSON View is now loaded from gDevTools.jsm and explicitly initialized. Thanks for the reviews guys! Honza
Attachment #8627680 -
Attachment is obsolete: true
Attachment #8627680 -
Flags: review?(jlong)
Attachment #8634697 -
Flags: review?(jlong)
Assignee | ||
Comment 89•9 years ago
|
||
Forgot to add, JSON View is now based on the patch from bug 1181646, so make sure you have it applied (the patch isn't yet landed in the tree atm) Honza
Reporter | ||
Updated•9 years ago
|
Whiteboard: [devedition-40] → [polish-backlog]
Reporter | ||
Updated•9 years ago
|
Target Milestone: --- → Firefox 43
Assignee | ||
Comment 90•9 years ago
|
||
Fixed memory leak Honza
Attachment #8634697 -
Attachment is obsolete: true
Attachment #8634697 -
Flags: review?(jlong)
Assignee | ||
Comment 91•9 years ago
|
||
Tests https://treeherder.mozilla.org/#/jobs?repo=try&revision=7f612c9c7c4e Waiting for results Honza
Comment 92•9 years ago
|
||
Comment on attachment 8648065 [details] [diff] [review] bug1132203-tests-1.patch Review of attachment 8648065 [details] [diff] [review]: ----------------------------------------------------------------- Eventually I'd love to see more component unit tests, where you just test a specific component by giving it some props and checking what it rendered (not even touching the DOM). But these look good. I'm going to take a closer look at them to actually flip the r? flag, just glancing through the patches now and asking some general questions. ::: browser/devtools/jsonview/test/simple_json.json^headers^ @@ +1,1 @@ > +Content-Type: application/json; charset=utf-8 \ No newline at end of file Is there some magic in our testing infrastructure to use these files named with ^headers^ as additional headers?
Comment 93•9 years ago
|
||
(In reply to Jan Honza Odvarko [:Honza] from comment #87) > > Done, the viewer is now using DevTools (shared) ReactJS from: > resource:///modules/devtools/shared/content/react.js (or react-dev.js in > case of 'browser/branding/unofficial') > > Honza Are you specifically using the MOZ_BRANDING_DIRECTORY constant? We don't use that anymore. In bug 1181646 we eventually just introduced a new one: DEBUG_JS_MODULES. It's just a boolean that you should check, and if true load the dev react version. You'll want to add this to your build config if you want to load dev react then: `ac_add_options --enable-debug-js-modules`
Comment 94•9 years ago
|
||
Comment on attachment 8648063 [details] [diff] [review] bug1132203-viewer-10.patch Review of attachment 8648063 [details] [diff] [review]: ----------------------------------------------------------------- This looks really good! I would consider this my full review, sorry it took so long to actually do :( I did r- only because I think the Reps class could be cleaned up a lot, and actually would be simpler as just a component. Read my comment and let me know. That's the only major change. Once we address that and some other nits are address, I'll definitely r+ this. My next review should be really fast 'cause I'll just be looking at the tweaks. ::: browser/devtools/jsonview/components/reps/repository.js @@ +32,5 @@ > +require("reps/null"); > +require("reps/string"); > +require("reps/number"); > +require("reps/array"); > +require("reps/object"); I think it would be good if all of this modules knew nothing about `Reps`, and just exported the component, and you called `Reps.registerRep` here for each of the imports (it could also export the `supportsObject` method). Modules that only export stuff are easier to test and manage. @@ +35,5 @@ > +require("reps/array"); > +require("reps/object"); > + > +// Exports from this module > +exports.Reps = require("reps/reps").Reps; Please do relative includes like `require("./reps")`, and remove that mapping like I mentioned before. ::: browser/devtools/jsonview/components/reps/reps.js @@ +17,5 @@ > + > +/** > + * Repository of all registered templates (reps). > + */ > +var Reps = { There's a few functions in here that I don't know what the are used for, like`getTargetRepObject`, `getRepNode`, etc. Honestly I can't find anywhere those are used. I would remove them if they aren't used anywhere. I'd rather start with a minimal API. The bigger issue though is I think the primary purpose of `Reps` should be available as a component, not a `getRep` method. Here's a more React-like way to do it: 1. At the top of reps.js, import all of the components that render out types (undefined, null, array, etc). You already do that in `repository.js`. Get rid of `repository.js` and just do all those imports at the top of the file here. This assumes that you've already changed those components to not register themselves anywhere; they just export a component and a `supportsObject` function. 2. Group the above imported components into an array. So, it would look like this so far: const Undefined = require('./undefined'); const Null = require('./null'); ... const tags = [Undefined, Null, ...]; 3. Create a `Rep` component that takes a `value` property. Within the render method, find the appropriate tag for that value. You would basically implement the same code in `getRep` here but reference `tags` instead. Just render that component with the current props; const Rep = React.createClass({ getRep: function(value) { /* look through tags, etc */ }; render: function() { const rep = this.getRep(this.props.value); return rep(this.props); } }); 4. Then, whenever you want to render a value, just do `Rep({ value: value, <any other props> })` There's most likely errors in the above code, but it's just to give you the idea. Let me know if I missed something. @@ +111,5 @@ > + } > + } > + }, > + > + createFactory: function(args) { This function should be named `createFactories`, since it creates multiple factories. ::: browser/devtools/jsonview/converter-child.js @@ +51,5 @@ > + ], > + > + get wrappedJSObject() this, > + > + initialize: function() { nit: empty method. is this required for the XPCOM interface? @@ +82,5 @@ > + // it to crash Firefox on OSX/Win32 (but not Win64) > + // It seems just reading once with -1 (default buffer size) gets the file done. > + // However, *not* reading in a loop seems to cause problems with Firebug > + // So I read in a loop, but do whatever I can to avoid infinite-looping. > + var totalBytesRead = 0; This variable is never used anywhere, and I don't see where the comment applies. Seems like this is leftover from a previous attempt? @@ +151,5 @@ > + defineAs: "postChromeMessage" > + }); > + }) > + > + let cleanData = this.data; why declare this variable? I expected it to be different than `this.data`. I'd say just use `this.data` everywhere (you already use it when calling `toErrorPage` below, but use `cleanData` when calling `toHTML`, which is confusing) @@ +209,5 @@ > + var themeClassName = "theme-" + JsonViewUtils.getCurrentTheme(); > + var baseUrl = "resource:///modules/devtools/jsonview/"; > + var theme = (themeClassName == "theme-light") ? "light" : "dark"; > + var themeUrl = '<link rel="stylesheet" type="text/css" ' + > + 'href="chrome://browser/skin/devtools/' + theme + '-theme.css">'; Nice theme switching @@ +312,5 @@ > + getService(Ci.nsICategoryManager); > +categoryManager.deleteCategoryEntry(GECKO_VIEWER, JSON_TYPE, false); > + > +categoryManager.addCategoryEntry("ext-to-type-mapping", "json", > + "application/json", false, true); Is there a reason you don't also use JSON_TYPE here? ::: browser/devtools/jsonview/main.js @@ +18,5 @@ > +const parentProcessMessageManager = Cc["@mozilla.org/parentprocessmessagemanager;1"]. > + getService(Ci.nsIMessageBroadcaster); > + > +// Constants > +const PREF = "devtools.jsonview.enabled"; I'd prefer a clearer name. Something like ENABLED_PREF_NAME, or JSON_VIEW_PREF. Up to you. @@ +26,5 @@ > + * It has the same lifetime as the browser. Initialization done by > + * DevTools() object from gDevTools.jsm > + */ > +var JsonView = { > + initialize: makeInfallible(function() { I like the way this is initialized now; clearer than the last patch. ::: browser/devtools/jsonview/viewer-config.js @@ +19,5 @@ > + "react": [ > + "resource:///modules/devtools/shared/content/react-dev", > + "resource:///modules/devtools/shared/content/react" > + ], > + "reps": "components/reps", I think this is confusing. There's a strong effort to map all paths used in `require` statements to literal paths on the filesystem. This doesn't save much hassle (there's only one file outside of `components` that requires this (json-view.js), and you could just do `require("./components/reps/repository")` instead. Instead of components you can just require it relatively. I'd prefer if this mapping was removed.
Attachment #8648063 -
Flags: review-
Comment 95•9 years ago
|
||
Comment on attachment 8634695 [details] [diff] [review] bug1132203-libs-3.patch Review of attachment 8634695 [details] [diff] [review]: ----------------------------------------------------------------- Eventually, I think we are going to need this to use our CommonJS loader. We just can't add another type of loader into the system, as there are already too many things going on. However, for now I think it's fine as an isolated feature. I actually wrote a browser loader for the debugger that I think is exactly the solution here. The browser loader basically is a special SDK loader that will load all of your files with the full browser API available. Basically, you can do what you are doing here but without the RequireJS boilerplate.
Attachment #8634695 -
Flags: review?(jlong) → review+
Assignee | ||
Comment 96•9 years ago
|
||
Thanks for the review James! (In reply to James Long (:jlongster) from comment #92) > ::: browser/devtools/jsonview/test/simple_json.json^headers^ > @@ +1,1 @@ > > +Content-Type: application/json; charset=utf-8 > \ No newline at end of file Fixed > Is there some magic in our testing infrastructure to use these files named > with ^headers^ as additional headers? Yes, see: https://developer.mozilla.org/en-US/docs/Mochitest#How_do_I_change_the_HTTP_headers_or_status_sent_with_a_file_used_in_a_Mochitest.3F (In reply to James Long (:jlongster) from comment #93) > (In reply to Jan Honza Odvarko [:Honza] from comment #87) > Are you specifically using the MOZ_BRANDING_DIRECTORY constant? We don't use > that anymore. In bug 1181646 we eventually just introduced a new one: > DEBUG_JS_MODULES. It's just a boolean that you should check, and if true > load the dev react version. > > You'll want to add this to your build config if you want to load dev react > then: `ac_add_options --enable-debug-js-modules` I am using ReactJS path fallback (trying to load react-dev first) See also: http://requirejs.org/docs/api.html#pathsfallbacks But I've updated a comment to mention `ac_add_options --enable-debug-js-modules` (in viewer-config.js file) (In reply to James Long (:jlongster) from comment #94) > Comment on attachment 8648063 [details] [diff] [review] > bug1132203-viewer-10.patch > > ::: browser/devtools/jsonview/components/reps/repository.js > @@ +32,5 @@ > > +require("reps/null"); > > +require("reps/string"); > > +require("reps/number"); > > +require("reps/array"); > > +require("reps/object"); > > I think it would be good if all of this modules knew nothing about `Reps`, > and just exported the component Done > @@ +35,5 @@ > > +require("reps/array"); > > +require("reps/object"); > > + > > +// Exports from this module > > +exports.Reps = require("reps/reps").Reps; > > Please do relative includes like `require("./reps")`, and remove that > mapping like I mentioned before. Done > ::: browser/devtools/jsonview/components/reps/reps.js > @@ +17,5 @@ > > + > > +/** > > + * Repository of all registered templates (reps). > > + */ > > +var Reps = { > > There's a few functions in here that I don't know what the are used for, > like`getTargetRepObject`, `getRepNode`, etc. Removed > The bigger issue though is I think the primary purpose of `Reps` should be > available as a component, not a `getRep` method. Yes, I like that, it's nicely fits with the idea of reps. Done. > > + createFactory: function(args) { > > This function should be named `createFactories`, since it creates multiple > factories. Done > ::: browser/devtools/jsonview/converter-child.js > @@ +51,5 @@ > > + ], > > + > > + get wrappedJSObject() this, > > + > > + initialize: function() { > > nit: empty method. is this required for the XPCOM interface? Removed > @@ +82,5 @@ > > + // it to crash Firefox on OSX/Win32 (but not Win64) > > + // It seems just reading once with -1 (default buffer size) gets the file done. > > + // However, *not* reading in a loop seems to cause problems with Firebug > > + // So I read in a loop, but do whatever I can to avoid infinite-looping. > > + var totalBytesRead = 0; > > This variable is never used anywhere, and I don't see where the comment > applies. Seems like this is leftover from a previous attempt? Yes, removed. > @@ +151,5 @@ > > + defineAs: "postChromeMessage" > > + }); > > + }) > > + > > + let cleanData = this.data; > > why declare this variable? I expected it to be different than `this.data`. > I'd say just use `this.data` everywhere (you already use it when calling > `toErrorPage` below, but use `cleanData` when calling `toHTML`, which is > confusing) Done > @@ +312,5 @@ > > + getService(Ci.nsICategoryManager); > > +categoryManager.deleteCategoryEntry(GECKO_VIEWER, JSON_TYPE, false); > > + > > +categoryManager.addCategoryEntry("ext-to-type-mapping", "json", > > + "application/json", false, true); > > Is there a reason you don't also use JSON_TYPE here? Done > ::: browser/devtools/jsonview/main.js > @@ +18,5 @@ > > +const parentProcessMessageManager = Cc["@mozilla.org/parentprocessmessagemanager;1"]. > > + getService(Ci.nsIMessageBroadcaster); > > + > > +// Constants > > +const PREF = "devtools.jsonview.enabled"; > > I'd prefer a clearer name. Something like ENABLED_PREF_NAME, or > JSON_VIEW_PREF. Up to you. JSON_VIEW_PREF it is (In reply to James Long (:jlongster) from comment #95) > Comment on attachment 8634695 [details] [diff] [review] > bug1132203-libs-3.patch > > Review of attachment 8634695 [details] [diff] [review]: > ----------------------------------------------------------------- > > Eventually, I think we are going to need this to use our CommonJS loader. We > just can't add another type of loader into the system, as there are already > too many things going on. However, for now I think it's fine as an isolated > feature. > > I actually wrote a browser loader for the debugger that I think is exactly > the solution here. The browser loader basically is a special SDK loader that > will load all of your files with the full browser API available. Basically, > you can do what you are doing here but without the RequireJS boilerplate. I like the idea. Just to note that JSON Viewer UI is entirely running inside the content scope. Can your loader operate within non privileged scope? Anyway I filled a follow up for further discussion: Bug 1195703 Honza
Attachment #8648063 -
Attachment is obsolete: true
Attachment #8649217 -
Flags: review?(jlong)
Assignee | ||
Comment 97•9 years ago
|
||
A few small modifications to the test files (+ one license note) Honza
Attachment #8648065 -
Attachment is obsolete: true
Attachment #8649219 -
Flags: review?(jlong)
Assignee | ||
Comment 98•9 years ago
|
||
Push to try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=fae2838f217a Honza
Assignee | ||
Comment 99•9 years ago
|
||
Fixing test failures reported by the try server. New push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=8d094658be09 Honza
Attachment #8649219 -
Attachment is obsolete: true
Attachment #8649219 -
Flags: review?(jlong)
Attachment #8650297 -
Flags: review?(jlong)
Comment 100•9 years ago
|
||
Comment on attachment 8649217 [details] [diff] [review] bug1132203-viewer-11.patch Review of attachment 8649217 [details] [diff] [review]: ----------------------------------------------------------------- I don't see any other major issues, thanks for responding to all the reviews.
Attachment #8649217 -
Flags: review?(jlong) → review+
Comment 101•9 years ago
|
||
Comment on attachment 8650297 [details] [diff] [review] bug1132203-tests-3.patch Review of attachment 8650297 [details] [diff] [review]: ----------------------------------------------------------------- Looks good to me, but might be good to have something who knows the testing infrastructure better than me to take a quick glance. jryans or bgrins, can you take a glance here and see if there's any way we could reduce duplicate code? (seems like Honza had to redefine a few commonly used functions) Both of you don't have to look at it, just whoever is first.
Attachment #8650297 -
Flags: review?(jryans)
Attachment #8650297 -
Flags: review?(jlong)
Attachment #8650297 -
Flags: review?(bgrinstead)
Comment on attachment 8650297 [details] [diff] [review] bug1132203-tests-3.patch Review of attachment 8650297 [details] [diff] [review]: ----------------------------------------------------------------- It seems generally fine to me. As the new head.js states, we should move common APIs to a shared file, but I'm fine with it happening in a follow up myself. I did not notice too many duplicate test functions, but I also scanned quickly. ::: browser/devtools/jsonview/test/browser_jsonview_copy_headers.js @@ +1,3 @@ > +/* -*- indent-tabs-mode: nil; js-indent-level: 2 -*- */ > +/* vim: set ts=2 et sw=2 tw=80: */ > +/* This Source Code Form is subject to the terms of the Mozilla Public Nit: We try our best to license test files as Public Domain. See the header[1] guide. [1]: https://www.mozilla.org/MPL/headers/ ::: browser/devtools/jsonview/test/head.js @@ +8,5 @@ > +// shared-head.js handles imports, constants, and utility functions > +Services.scriptloader.loadSubScript( > + "chrome://mochitests/content/browser/browser/devtools/framework/test/head.js", this); > + > +// XXX move some API into devtools/framework/test/shared-head.js Please file a follow up to do this, and place the bug number here.
Attachment #8650297 -
Flags: review?(jryans)
Attachment #8650297 -
Flags: review?(bgrinstead)
Attachment #8650297 -
Flags: review+
Assignee | ||
Comment 103•9 years ago
|
||
(In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #102) > Comment on attachment 8650297 [details] [diff] [review] > bug1132203-tests-3.patch > > Review of attachment 8650297 [details] [diff] [review]: > ----------------------------------------------------------------- > > It seems generally fine to me. As the new head.js states, we should move > common APIs to a shared file, but I'm fine with it happening in a follow up > myself. > > I did not notice too many duplicate test functions, but I also scanned > quickly. > > ::: browser/devtools/jsonview/test/browser_jsonview_copy_headers.js > @@ +1,3 @@ > > +/* -*- indent-tabs-mode: nil; js-indent-level: 2 -*- */ > > +/* vim: set ts=2 et sw=2 tw=80: */ > > +/* This Source Code Form is subject to the terms of the Mozilla Public > > Nit: We try our best to license test files as Public Domain. See the > header[1] guide. Fixed > > [1]: https://www.mozilla.org/MPL/headers/ > > ::: browser/devtools/jsonview/test/head.js > @@ +8,5 @@ > > +// shared-head.js handles imports, constants, and utility functions > > +Services.scriptloader.loadSubScript( > > + "chrome://mochitests/content/browser/browser/devtools/framework/test/head.js", this); > > + > > +// XXX move some API into devtools/framework/test/shared-head.js > > Please file a follow up to do this, and place the bug number here. Bug 1197886 Thanks for the review Ryan! Honza
Attachment #8650297 -
Attachment is obsolete: true
Attachment #8651853 -
Flags: review+
Assignee | ||
Updated•9 years ago
|
Keywords: checkin-needed
Comment 104•9 years ago
|
||
https://hg.mozilla.org/integration/fx-team/rev/2429f203ecc7 https://hg.mozilla.org/integration/fx-team/rev/307682f73f2d https://hg.mozilla.org/integration/fx-team/rev/e059e455e5b3
Keywords: checkin-needed
Comment 105•9 years ago
|
||
Backed out for test_bug667533.html timeouts. https://treeherder.mozilla.org/logviewer.html#?job_id=4353826&repo=fx-team https://hg.mozilla.org/integration/fx-team/rev/66a87937a5c0
Assignee | ||
Comment 106•9 years ago
|
||
Patch for the test failure. Try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=662ad0192120 Honza
Comment 107•9 years ago
|
||
Comment on attachment 8649217 [details] [diff] [review] bug1132203-viewer-11.patch Review of attachment 8649217 [details] [diff] [review]: ----------------------------------------------------------------- Please read the SVG cleanup guidelines before shipping some : https://etherpad.mozilla.org/svg-guidelines
Comment 108•9 years ago
|
||
(In reply to Tim Nguyen [:ntim] (mostly away until 26 August) from comment #107) > Comment on attachment 8649217 [details] [diff] [review] > bug1132203-viewer-11.patch > > Review of attachment 8649217 [details] [diff] [review]: > ----------------------------------------------------------------- > > Please read the SVG cleanup guidelines before shipping some : > https://etherpad.mozilla.org/svg-guidelines Can you move this information to MDN? Maybe to something like https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/SVG_Guidlines. You already posted to dev-platform about this and didn't get any objections, so I'd say just go ahead and do it and then follow up with a post to firefox-dev.
Assignee | ||
Comment 109•9 years ago
|
||
Patch updated Try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=3d10b0be9765 Honza
Attachment #8652326 -
Attachment is obsolete: true
Assignee | ||
Comment 110•9 years ago
|
||
(In reply to Ryan VanderMeulen [:RyanVM UTC-4] from comment #105) > Backed out for test_bug667533.html timeouts. > https://treeherder.mozilla.org/logviewer.html#?job_id=4353826&repo=fx-team > > https://hg.mozilla.org/integration/fx-team/rev/66a87937a5c0 The timeout (test test_bug667533.html) should be fixed by this patch (see try push in comment #109) - JSON Viewer feature is now dynamically activated when the preference is set (by changing "devtools.jsonview.enabled" through about:config) or deactivated when the pref is set to false. No browser restart needed. Ryan, can you please do a review? Honza
Assignee | ||
Updated•9 years ago
|
Attachment #8654141 -
Flags: review?(jryans)
Assignee | ||
Comment 111•9 years ago
|
||
Ah, forgot to set the flag. Ryan, it's quite small patch, just a few changes. Honza
Comment on attachment 8654141 [details] [diff] [review] bug1132203-loadondemand-2.patch Review of attachment 8654141 [details] [diff] [review]: ----------------------------------------------------------------- ::: browser/devtools/jsonview/main.js @@ +28,5 @@ > + // Load JSON converter module. This converter is responsible > + // for handling 'application/json' documents and converting > + // them into a simple web-app that allows easy inspection > + // of the JSON data. > + globalMessageManager.loadFrameScript( When I worked on view source tabs, loading an extra frame script for all windows by default regressed Talos tests after landing. Please add Talos to your try runs and compare with other runs using the perfherder links that appear in treeherder for Talos.
Attachment #8654141 -
Flags: review?(jryans) → review+
Assignee | ||
Comment 113•9 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=d9c6aa2937d7 https://treeherder.mozilla.org/perf.html#/comparechooser?newProject=try&newRevision=d9c6aa2937d7 Honza
Updated•9 years ago
|
Component: Developer Tools → Developer Tools: JSON Viewer
Assignee | ||
Comment 114•9 years ago
|
||
(In reply to Jan Honza Odvarko [:Honza] from comment #113) > https://treeherder.mozilla.org/#/jobs?repo=try&revision=d9c6aa2937d7 Ryan, any ideas why the same try (the same patch) has passed in comment #109, is it all intermittent now? Honza
Flags: needinfo?(jryans)
(In reply to Jan Honza Odvarko [:Honza] from comment #114) > (In reply to Jan Honza Odvarko [:Honza] from comment #113) > > https://treeherder.mozilla.org/#/jobs?repo=try&revision=d9c6aa2937d7 > Ryan, any ideas why the same try (the same patch) has passed in > comment #109, is it all intermittent now? > > Honza The try errors look unrelated to your work. If you try rebasing to latest fx-team and run again, they may go away.
Flags: needinfo?(jryans)
Assignee | ||
Comment 116•9 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=97af6187fd6a https://treeherder.mozilla.org/perf.html#/comparechooser?newProject=try&newRevision=97af6187fd6a Ryan, not sure how to properly compare with other runs...? Honza
Flags: needinfo?(jryans)
(In reply to Jan Honza Odvarko [:Honza] from comment #116) > https://treeherder.mozilla.org/#/jobs?repo=try&revision=97af6187fd6a > https://treeherder.mozilla.org/perf.html#/ > comparechooser?newProject=try&newRevision=97af6187fd6a > > Ryan, not sure how to properly compare with other runs...? I picked a recent m-i run to compare[1]. The sessionrestore opt scores worry me, as they suggest regressions on all platforms. I've triggered more runs on the base rev and your try so we can get more data. Let's check the comparison again once those complete. [1]: https://treeherder.mozilla.org/perf.html#/compare?originalProject=mozilla-inbound&originalRevision=e110acdf8e64&newProject=try&newRevision=97af6187fd6a
Flags: needinfo?(jryans)
Assignee | ||
Comment 118•9 years ago
|
||
The sessionrestore opt seems to be still red. If loading the frame script into every window is not the right way, we can remove the support for dynamic registration/unregistration of the viewer (introduced in the loadondemand patch). The only questions is how to fix the test: parser/htmlparser/tests/mochitest/test_bug667533.html This test expects text output for application/json and fails because the JSON Viewer is active. Note that the "devtools.jsonview.enabled" pref needs to be true for entire test suite... Can we just change that one failing test? Honza
Flags: needinfo?(jryans)
Mossop, maybe you have some good advice? I am not sure what to recommend here. JSON viewer wants to load an extra frame script, but this is leading to sessionrestore Talos regressions, similar to when I worked on view source in tab. Is there a good way for Honza to only load his frame script into browsers of interest (with JSON content)? In bug 1171720, we've discussed potential frame script loading ideas surrounding view source based on URL patterns, but I'm not sure that would help here.
Flags: needinfo?(jryans) → needinfo?(dtownsend)
Comment 120•9 years ago
|
||
(In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #119) > Mossop, maybe you have some good advice? I am not sure what to recommend > here. JSON viewer wants to load an extra frame script, but this is leading > to sessionrestore Talos regressions, similar to when I worked on view source > in tab. My main thought is to wonder why on earth just loading a frame script is regressing that test so much. It doesn't seem like it should be an expensive operation. Have we verified that even an empty framescript causes the regression? If not then making sure that anything the framescript does happens asynchronously would be a way to go. If it is really loading a framescript that does it then maybe we can just have the main browser frame script load the devtools frame scripts. > Is there a good way for Honza to only load his frame script into browsers of > interest (with JSON content)? None that I know of right now, billm might have ideas. Looks like Yoric wrote the sessionrestore test so maybe he has some ideas about why this is affecting it so much.
Flags: needinfo?(dtownsend) → needinfo?(dteller)
Could this be simply because the sessionrestore test creates lots of tabs?
Flags: needinfo?(dteller)
Assignee | ||
Comment 122•9 years ago
|
||
Could it be that some add-on sdk modules are loaded? (and they could load dependencies) Honza
The sessionrestore test doesn't use any add-on.
Assignee | ||
Comment 124•9 years ago
|
||
(In reply to David Rajchenbach-Teller [:Yoric] (use "needinfo") from comment #123) > The sessionrestore test doesn't use any add-on. I mean the frame-script is loading some Add-on SDK modules and perhaps there is a lot of dependencies and loading them might take the time... (I don't now just guessing) Honza
Comment 125•9 years ago
|
||
(In reply to Jan Honza Odvarko [:Honza] from comment #124) > (In reply to David Rajchenbach-Teller [:Yoric] (use "needinfo") from comment > #123) > > The sessionrestore test doesn't use any add-on. > I mean the frame-script is loading some Add-on SDK modules and perhaps there > is a lot of dependencies and loading them might take the time... (I don't > now just guessing) > > Honza Would it be possible to wait to import those modules until a JSON page is loaded?
(In reply to Dave Townsend [:mossop] from comment #120) > (In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #119) > > Mossop, maybe you have some good advice? I am not sure what to recommend > > here. JSON viewer wants to load an extra frame script, but this is leading > > to sessionrestore Talos regressions, similar to when I worked on view source > > in tab. > > My main thought is to wonder why on earth just loading a frame script is > regressing that test so much. It doesn't seem like it should be an expensive > operation. Have we verified that even an empty framescript causes the > regression? I think this information would be good to have: if we just add an empty frame script that does nothing interesting, is that enough to regress the Talos test? (We'd need to be careful in case there is some code path that realizes it is empty and just does nothing with the script...)
Assignee | ||
Comment 127•9 years ago
|
||
(In reply to Brian Grinstead [:bgrins] from comment #125) > Would it be possible to wait to import those modules until a JSON page is > loaded? Yep, that's my next step (in figuring out what's actually slow in the frame script). (In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #126) > I think this information would be good to have: if we just add an empty > frame script that does nothing interesting, is that enough to regress the > Talos test? (We'd need to be careful in case there is some code path that > realizes it is empty and just does nothing with the script...) Agree (I already have come try pushes, I am currently watching) Honza
Assignee | ||
Comment 128•9 years ago
|
||
So, here is my test: - there is a new frame-script called: converter-observer.js, it's loaded into all frames. - the script defines a few lazy getters and one object, but nothing is really used. It should be fairly quick to load it. - This script is supposed to call register/unregister JSON View and load the converter-child.js that when necessary. The converter-child.js contains all the viewer XPCOM code. But, the Talos still shows reds for session restore tests! https://treeherder.mozilla.org/#/jobs?repo=try&revision=75d2ba940e10 https://treeherder.mozilla.org/perf.html#/comparechooser?newProject=try&newRevision=75d2ba940e10 Here is another try-push with converter-observer.js script almost empty (just one, not used, lazy getter) https://treeherder.mozilla.org/#/jobs?repo=try&revision=2b95b3d44786 https://treeherder.mozilla.org/perf.html#/comparechooser?newProject=tr y&newRevision=2b95b3d44786 Btw. when comparing revisions in comparechooser, what base revision should I pick? Honza
Attachment #8654141 -
Attachment is obsolete: true
Flags: needinfo?(jryans)
Flags: needinfo?(bgrinstead)
Assignee | ||
Comment 129•9 years ago
|
||
(In reply to Jan Honza Odvarko [:Honza] from comment #128) > Here is another try-push with converter-observer.js script almost empty > (just one, not used, lazy getter) > > https://treeherder.mozilla.org/#/jobs?repo=try&revision=2b95b3d44786 > https://treeherder.mozilla.org/perf.html#/comparechooser?newProject=tr > y&newRevision=2b95b3d44786 When I compare these results (an empty script) it looks ok https://treeherder.mozilla.org/perf.html#/compare?originalProject=mozilla-inbound&originalRevision=e110acdf8e64&newProject=try&newRevision=2b95b3d44786 Honza
:bgrins helped Honza with this over IRC.
Flags: needinfo?(jryans)
Comment 131•9 years ago
|
||
I believe the right comparison page for Comment 129 is this (based off of parent fx-team commit): https://treeherder.mozilla.org/perf.html#/compare?originalProject=fx-team&originalRevision=65e92b72d584&newProject=try&newRevision=2b95b3d44786. Which means we need retriggers on T-o for opt builds on these two commits: https://treeherder.mozilla.org/#/jobs?repo=try&revision=2b95b3d44786 https://treeherder.mozilla.org/#/jobs?repo=fx-team&revision=65e92b72d584 I've gone through and done the retriggers, so let's see how that turns out
Flags: needinfo?(bgrinstead)
Comment 132•9 years ago
|
||
(In reply to Brian Grinstead [:bgrins] from comment #131) > I believe the right comparison page for Comment 129 is this (based off of > parent fx-team commit): > https://treeherder.mozilla.org/perf.html#/compare?originalProject=fx- > team&originalRevision=65e92b72d584&newProject=try&newRevision=2b95b3d44786. Looks like there is at least a regression on winxp sessionrestore (1.60%). So if that is a commit that has almost nothing in the frame script, then the regression might be caused by just introducing the frame script.
Comment 133•9 years ago
|
||
I also wonder if the session restore test is having some issues. I just saw a 5.9% regression alert from this changeset, which can't be right: http://hg.mozilla.org/integration/fx-team/rev/9d35eca4bc3a.
Comment 134•9 years ago
|
||
(In reply to Brian Grinstead [:bgrins] from comment #133) > I also wonder if the session restore test is having some issues. I just saw > a 5.9% regression alert from this changeset, which can't be right: > http://hg.mozilla.org/integration/fx-team/rev/9d35eca4bc3a. Can't it? If that test creates lots of tabs that load about:newtab, and that cset causes style changes... (I mean, I'd need to look at it in more detail to be sure, but it's not like it's a test-only change or in code entirely unrelated) Anyway, there's also a substantial ts_paint regression on winxp, and a number of other tests that don't have enough retriggers.
Comment 135•9 years ago
|
||
(In reply to :Gijs Kruitbosch from comment #134) > (In reply to Brian Grinstead [:bgrins] from comment #133) > > I also wonder if the session restore test is having some issues. I just saw > > a 5.9% regression alert from this changeset, which can't be right: > > http://hg.mozilla.org/integration/fx-team/rev/9d35eca4bc3a. > > Can't it? If that test creates lots of tabs that load about:newtab, and that > cset causes style changes... (I mean, I'd need to look at it in more detail > to be sure, but it's not like it's a test-only change or in code entirely > unrelated) Hm, that's possible. I understood that `titleBgColor` wouldn't be set in normal cases after talking to one of the devs, so the call to style.backgroundColor wouldn't run in the test. Maybe it does though, I'm not sure.
Assignee | ||
Comment 136•9 years ago
|
||
Comparing Talos test results [1] even for minimal frame script still reports regressions, so I changed the patch and converted the frame script into process script (I believe this is the right way to go anyway). Updated patch attached. Here is try-push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=6c86a6f34026 Talos comparing results: https://treeherder.mozilla.org/perf.html#/compare?originalProject=mozilla-central&originalRevision=b23b2fa33a9d&newProject=try&newRevision=6c86a6f34026 Can anyone check out the results? I don't see any 'red' results, but at the same time I am not sure whether I picked the right base revision (still not sure about how to pick the right base revision that has Talos tests results). Honza [1] https://treeherder.mozilla.org/perf.html#/compare?originalProject=fx-team&originalRevision=65e92b72d584&newProject=try&newRevision=2b95b3d44786.
Attachment #8657900 -
Attachment is obsolete: true
Attachment #8659108 -
Flags: review?(jryans)
Assignee | ||
Comment 137•9 years ago
|
||
Here is proper base revision I guess: https://treeherder.mozilla.org/perf.html#/compare?originalProject=fx-team&originalRevision=8968d387064d&newProject=try&newRevision=6c86a6f34026 (looks good so far, but tests still running atm) Honza
(In reply to Jan Honza Odvarko [:Honza] from comment #137) > Here is proper base revision I guess: > https://treeherder.mozilla.org/perf.html#/compare?originalProject=fx- > team&originalRevision=8968d387064d&newProject=try&newRevision=6c86a6f34026 > > (looks good so far, but tests still running atm) Yes, this version looks good. Seems you figured out how to pick the base: it should be whatever changeset landed just before your patches. Sometimes you have to walk back a few more though, if Talos never ran at all on that changeset. Anyway, this seems like the correct comparison to me.
Assignee | ||
Comment 139•9 years ago
|
||
(In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #138) > Yes, this version looks good. Seems you figured out how to pick the base: > it should be whatever changeset landed just before your patches. Sometimes > you have to walk back a few more though, if Talos never ran at all on that > changeset. So, only sheriffs can trigger Talos tests (or any other tests) explicitly for an existing revision, right? Honza
(In reply to Jan Honza Odvarko [:Honza] from comment #139) > (In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #138) > > Yes, this version looks good. Seems you figured out how to pick the base: > > it should be whatever changeset landed just before your patches. Sometimes > > you have to walk back a few more though, if Talos never ran at all on that > > changeset. > > So, only sheriffs can trigger Talos tests (or any other tests) explicitly > for an existing revision, right? Hmm, if they never ran before for a revision, it's a bit tricky... There are tools like https://secure.pub.build.mozilla.org/buildapi/self-serve that you can use (click "arrow" from top-right on a changeset in treeherder, choose BuildAPI), but it's not easy to know how trigger them. I think something like bug 1202718 would be easiest, assuming it can help even for revision that never ran Talos yet.
Comment on attachment 8659108 [details] [diff] [review] bug1132203-loadondemand-4.patch Review of attachment 8659108 [details] [diff] [review]: ----------------------------------------------------------------- Seems mostly good, but a few things I did not follow. ::: browser/devtools/jsonview/converter-child.js @@ +17,2 @@ > > +DevToolsUtils.defineLazyGetter(this, "NetworkHelper", () => Can you use loader.lazyRequireGetter? @@ -301,2 @@ > > -categoryManager.addCategoryEntry("ext-to-type-mapping", "json", Do you still need to do this step? It doesn't seem to happen anymore in the new version. @@ +305,5 @@ > > + return false; > +} > + > +exports.JsonViewService = { Who uses this export?
Attachment #8659108 -
Flags: review?(jryans)
Assignee | ||
Comment 142•9 years ago
|
||
(In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #141) > Comment on attachment 8659108 [details] [diff] [review] > bug1132203-loadondemand-4.patch > > Review of attachment 8659108 [details] [diff] [review]: > ----------------------------------------------------------------- > > Seems mostly good, but a few things I did not follow. > > ::: browser/devtools/jsonview/converter-child.js > @@ +17,2 @@ > > > > +DevToolsUtils.defineLazyGetter(this, "NetworkHelper", () => > > Can you use loader.lazyRequireGetter? Done > > @@ -301,2 @@ > > > > -categoryManager.addCategoryEntry("ext-to-type-mapping", "json", > > Do you still need to do this step? It doesn't seem to happen anymore in the > new version. It's done in converter-observer.js module > > @@ +305,5 @@ > > > > + return false; > > +} > > + > > +exports.JsonViewService = { > > Who uses this export? converter-observer module. This module is observing the devtools.jsonview.enabled pref and calling JsonViewService.register/unregister() as appropriate. Honza
Attachment #8659108 -
Attachment is obsolete: true
Attachment #8659287 -
Flags: review?(jryans)
Assignee | ||
Comment 143•9 years ago
|
||
(In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #141) > @@ -301,2 @@ > > > > -categoryManager.addCategoryEntry("ext-to-type-mapping", "json", > > Do you still need to do this step? It doesn't seem to happen anymore in the > new version. I see now what you mean, now it's back in the patch. Thanks for the review Ryan! Honza
Attachment #8659287 -
Attachment is obsolete: true
Attachment #8659287 -
Flags: review?(jryans)
Attachment #8659362 -
Flags: review?(jryans)
Comment on attachment 8659362 [details] [diff] [review] bug1132203-loadondemand-6.patch Review of attachment 8659362 [details] [diff] [review]: ----------------------------------------------------------------- Okay, I think it seems good!
Attachment #8659362 -
Flags: review?(jryans) → review+
Assignee | ||
Comment 145•9 years ago
|
||
Thanks Ryan! I've yet added one small thing. The channel doesn't always have to be nsIHttpChannel, and so iterating headers isn't always possible. One condition in the patch is changed. Honza
Attachment #8659362 -
Attachment is obsolete: true
Attachment #8659827 -
Flags: review?(jryans)
Assignee | ||
Comment 146•9 years ago
|
||
@bz: we've implemented a JSON Response Viewer that nicely renders application/json document types (useful especially for web developers). The feature is hidden behind devtools.jsonview.enabled pref that is set to false by default in all Fx channels except of the DevEdition. This test expects application/json types to be rendered as a plain text, and so it also needs to make sure the pref is false now. I've seen that you did the last review for this test, so I figured that you could do it again, thanks! Honza
Attachment #8659834 -
Flags: review?(bzbarsky)
Attachment #8659827 -
Flags: review?(jryans) → review+
Comment 147•9 years ago
|
||
Comment on attachment 8659834 [details] [diff] [review] bug1132203-parser-test-1.patch Please use pushPrefEnv instead of setBoolPref....
Attachment #8659834 -
Flags: review?(bzbarsky) → review-
Comment 148•9 years ago
|
||
Also, why does that test even fail? Is the DOM actually changed with the viewer in place? If so, it would be better to skip the test on devedition, with comments about how our behavior there is not necessarily web-compatible or something. Otherwise someone can flip the pref on for release and not get a test failure, even though the test is meant to ensure that on release we in fact render JSON as text for script purposes...
Flags: needinfo?(odvarko)
Assignee | ||
Comment 149•9 years ago
|
||
(In reply to Boris Zbarsky [:bz] from comment #148) > Also, why does that test even fail? Is the DOM actually changed with the > viewer in place? If so, it would be better to skip the test on devedition, > with comments about how our behavior there is not necessarily web-compatible > or something. Otherwise someone can flip the pref on for release and not > get a test failure, even though the test is meant to ensure that on release > we in fact render JSON as text for script purposes... I see, makes sense to me. I was also discussing this with Gijs and we agreed that enabling this feature for top level loads only is even better for web compatibility. This also means that the existing parser test can stay as is. New patch attached. I am assigning Ryans for the review since he's been mostly reviewing the stuff. Thanks Boris for the input! Honza
Attachment #8659834 -
Attachment is obsolete: true
Flags: needinfo?(odvarko)
Attachment #8660701 -
Flags: review?(jryans)
Comment on attachment 8660701 [details] [diff] [review] bug1132203-toplevel-only-1.patch Review of attachment 8660701 [details] [diff] [review]: ----------------------------------------------------------------- I would suggest making sure :bz is okay with the perf of this on really large files (since we now go through this JS conversion and encoding, instead of displaying straight from the parser in plain text mode). I know he's been worried about this for other view source changes I've made in the past. Having said that, how do you even "view" JSON for non-top level loads anyway...? I think I'm missing some pieces here before I could give r+. ::: browser/devtools/jsonview/converter-child.js @@ +150,5 @@ > + return t !== null ? t.toString().replace(/&/g,"&"). > + replace(/"/g,""").replace(/</g,"<").replace(/>/g,">") : ''; > + }, > + > + toPlainText: function(json, title) { I guess add a note that this the same output you would get from the parser's plain view source[1]. [1]: https://dxr.mozilla.org/mozilla-central/rev/9ed17db42e3e46f1c712e4dffd62d54e915e0fac/parser/html/nsHtml5TreeBuilderCppSupplement.h#1162 @@ +288,5 @@ > }); > > +// Helpers > + > +function isTopLevelLoad(aRequest) { This seems like a useful network helper itself, maybe move it there?
Attachment #8660701 -
Flags: review?(jryans) → feedback+
Assignee | ||
Comment 151•9 years ago
|
||
@Boris, does the solution suggested in comment #149 look ok to you? Honza
Flags: needinfo?(bzbarsky)
Comment 152•9 years ago
|
||
(In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #150) > Having said that, how do you even "view" JSON for non-top level loads > anyway...? Like the test referenced in comment 146 and later (so by loading in a(n i)frame, for instance).
(In reply to :Gijs Kruitbosch from comment #152) > (In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #150) > > Having said that, how do you even "view" JSON for non-top level loads > > anyway...? > > Like the test referenced in comment 146 and later (so by loading in a(n > i)frame, for instance). Okay, I mean I get you can put in an iframe, I just don't really follow what the purpose of doing so would be... I would have assumed most sub-requests to load JSON in the real world are XHR requests. The plain view source presentation is only meant human consumption, I would have thought? Maybe thinking about reality is getting me off topic here. :)
Assignee | ||
Comment 154•9 years ago
|
||
Try push including the last patch (from comment #149) https://treeherder.mozilla.org/#/jobs?repo=try&revision=0bb180b2470c https://treeherder.mozilla.org/perf.html#/comparechooser?newProject=try&newRevision=0bb180b2470c Honza
Comment 155•9 years ago
|
||
Why would you even install this stream converter in the non-toplevel case and then manually duplicate work that Gecko would do anyway? That seems pretty weird...
Flags: needinfo?(bzbarsky)
Assignee | ||
Comment 156•9 years ago
|
||
(In reply to Boris Zbarsky [:bz] from comment #155) > Why would you even install this stream converter in the non-toplevel case > and then manually duplicate work that Gecko would do anyway? That seems > pretty weird... Yes, but what is the other option? I don't know how register the XPCOM converter service just for top level requests/loads. (and unregistering in the middle of the request doesn't seem to be feasible) Or is it possible to use the default gecko-viewer component from JS and forward the processing to it? Honza
Flags: needinfo?(bzbarsky)
Comment 157•9 years ago
|
||
The "normal" way to do this (e.g. the way the feed reader works) is to install something that sniffs the load and changes its MIME type to some internal type that you then install a stream converter for, instead of unregistering Gecko's built-in handler. Then the sniffer can decide whether this is a toplevel load or not. If it is, it sniffs it to the internal type and your converter kicks in. IF it's not, it leaves it as application/json and the internal Gecko handling happens.
Flags: needinfo?(bzbarsky)
Assignee | ||
Comment 158•9 years ago
|
||
(In reply to Boris Zbarsky [:bz] from comment #157) > The "normal" way to do this (e.g. the way the feed reader works) is to > install something that sniffs the load and changes its MIME type to some > internal type that you then install a stream converter for, instead of > unregistering Gecko's built-in handler. Then the sniffer can decide whether > this is a toplevel load or not. If it is, it sniffs it to the internal type > and your converter kicks in. IF it's not, it leaves it as application/json > and the internal Gecko handling happens. Sounds reasonable. Any tips how to sniff and change the content type?. Honza
Flags: needinfo?(bzbarsky)
Comment 159•9 years ago
|
||
Sure. You register an implementation of nsIContentSniffer in NS_CONTENT_SNIFFER_CATEGORY. Then if your GetMIMETypeFromContent returns a nonempty type, that type will override the type the server sent. Note that you'll only want to do this for cases that have no content-disposition or inline content-disposition, of course.
Flags: needinfo?(bzbarsky)
Reporter | ||
Comment 160•9 years ago
|
||
(In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #153) > (In reply to :Gijs Kruitbosch from comment #152) > > (In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #150) > > > Having said that, how do you even "view" JSON for non-top level loads > > > anyway...? > > > > Like the test referenced in comment 146 and later (so by loading in a(n > > i)frame, for instance). > > Okay, I mean I get you can put in an iframe, I just don't really follow what > the purpose of doing so would be... I would have assumed most sub-requests > to load JSON in the real world are XHR requests. The plain view source > presentation is only meant human consumption, I would have thought? The explicit use case is right-click on a JSON network request in the net monitor and open in a new tab. The JSON response from the server is a top-level load. Anything else is outside our scope from my point of view.
Assignee | ||
Comment 161•9 years ago
|
||
(In reply to Boris Zbarsky [:bz] from comment #159) > Sure. You register an implementation of nsIContentSniffer in > NS_CONTENT_SNIFFER_CATEGORY. Then if your GetMIMETypeFromContent returns a > nonempty type, that type will override the type the server sent. > > Note that you'll only want to do this for cases that have no > content-disposition or inline content-disposition, of course. Excellent, thanks Boris, patch attached. Push to try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=0b99c1dede6c https://treeherder.mozilla.org/perf.html#/comparechooser?newProject=try&newRevision=0b99c1dede6c Honza
Attachment #8660701 -
Attachment is obsolete: true
Attachment #8662381 -
Flags: review?(bzbarsky)
Comment 162•9 years ago
|
||
Comment on attachment 8662381 [details] [diff] [review] bug1132203-toplevel-only-2.patch > if (JsonViewService.register()) { >+ categoryManager.addCategoryEntry(CONTENT_SNIFFER_CATEGORY, JSON_VIEW_TYPE, Why? At the very least this needs a comment, but it looks wrong to me. > this.geckoMapping = categoryManager.addCategoryEntry(GECKO_TYPE_MAPPING, Why wouldn't we want this normally, even when this service is not registered? Seems like we should just add it to extraMimeEntries in nsExternalHelperAppService.cpp >+ if (NetworkHelper.getResponseHeader(aRequest, "content-disposition")) { >+ return; No, that's not what you want, for several reasons. Non-HTTP channels can have content-disposition, and just having the header doesn't mean you should skip your JSON viewer (e.g. it could be "content-disposition: inline"). What you want is this: if (aRequest instanceof nsIChannel) { try { if (aRequest.contentDisposition == nsIChannel.DISPOSITION_ATTACHMENT) { return ""; } } catch (e) { // Channel doesn't support content dispositions } } >+ if (name && name.startsWith("data:" + JSON_TYPE)) { And this part is bogus too. Just use aRequest.contentType if instanceof nsIChannel. If not, do nothing (though there are no toplevel non-channel requests anyway). You need to return empty string at the end of your function, no? Otherwise the caller will think you sniffed the type to "undefined", I expect. I assume you got all the JS xpcom goop right; it looks reasonable to me, but I don't have much confidence of catching a bug in it if there is one. >+ let win = this.getWindowForRequest(aRequest); This will end up returning true from isTopLevelLoad for _any_ load in a toplevel window, not just a navigation. What you want to do, in addition to checking for win.top == win, is to check for the nsIChannel.LOAD_DOCUMENT_URI flag on the request, right? Also, how does this part work in e10s mode? Which process is it running in?
Attachment #8662381 -
Flags: review?(bzbarsky) → review-
Assignee | ||
Comment 163•9 years ago
|
||
(In reply to Boris Zbarsky [:bz] from comment #162) > Comment on attachment 8662381 [details] [diff] [review] > bug1132203-toplevel-only-2.patch Thanks for the review Boris, I fixed most of your comments. The only issue I am seeing is that the registered sniffer component is actually instantiated in the parent process (done automatically since it's used through the category manager) and so doesn't have access to the request's parent window (the getWindowForRequest() returns null). And nsIChannel.LOAD_DOCUMENT_URI is set for both the top level request document as well as an iframe document. Note that both the stream converter and sniffer components are registered in the child process... Honza
Flags: needinfo?(bzbarsky)
Comment 164•9 years ago
|
||
You want to check both LOAD_DOCUMENT_URI _and_ being in a toplevel window. I assume we have _some_ way to tell something useful about the context of a channel in the parent process... but I have no idea what it is. Mike, do you know?
Flags: needinfo?(bzbarsky) → needinfo?(mconley)
Comment 165•9 years ago
|
||
I'm afraid that the inner-workings and structure of e10s-mode's network stuff is still a bit of a mystery to me. Redirecting to billm?
Flags: needinfo?(mconley) → needinfo?(wmccloskey)
Assignee | ||
Comment 166•9 years ago
|
||
Bill, according to our conversion on IRC, I've filled a new report asking for a top-level-load-flag in nsILoadInfo structure, bug 1207278 Honza
Reporter | ||
Comment 167•9 years ago
|
||
Honza - should bug 1207278 really block landing, or is it instead a followup to switch once that bug is resolved?
Flags: needinfo?(odvarko)
Assignee | ||
Comment 168•9 years ago
|
||
(In reply to Jeff Griffiths (:canuckistani) (:⚡︎) from comment #167) > Honza - should bug 1207278 really block landing, or is it instead a followup > to switch once that bug is resolved? It's a blocker unfortunately :(. It's needed for enabling the JSON Viewer for top level loads only. (Bill promised to have the API ready in couple of days). Honza
Flags: needinfo?(odvarko)
Assignee | ||
Comment 169•9 years ago
|
||
Attachment #8634695 -
Attachment is obsolete: true
Attachment #8666652 -
Flags: review+
Assignee | ||
Comment 170•9 years ago
|
||
Attachment #8649217 -
Attachment is obsolete: true
Attachment #8666653 -
Flags: review+
Assignee | ||
Comment 171•9 years ago
|
||
Attachment #8651853 -
Attachment is obsolete: true
Attachment #8666654 -
Flags: review+
Assignee | ||
Comment 172•9 years ago
|
||
Attachment #8659827 -
Attachment is obsolete: true
Attachment #8666655 -
Flags: review+
Assignee | ||
Comment 173•9 years ago
|
||
(In reply to Boris Zbarsky [:bz] from comment #164) > You want to check both LOAD_DOCUMENT_URI _and_ being in a toplevel window. > > I assume we have _some_ way to tell something useful about the context of a > channel in the parent process... but I have no idea what it is. Mike, do > you know? Boris, the attached patch fixes all your review notes (from comment #162) and also solves the issue with top level loads (I am using aRequest.loadInfo.parentOuterWindowID == aRequest.loadInfo.outerWindowID condition now). Honza
Attachment #8662381 -
Attachment is obsolete: true
Attachment #8666657 -
Flags: review?(bzbarsky)
Assignee | ||
Updated•9 years ago
|
Flags: needinfo?(wmccloskey)
Comment 174•9 years ago
|
||
Comment on attachment 8666657 [details] [diff] [review] bug1132203-toplevel-only-3.patch >+ if (loadInfo && loadInfo.parentOuterWindowID == loadInfo.outerWindowID) { I assume this code will never be used in any situation where <iframe mozbrowser> might be used? Because it will do the wrong thing in that situation.... r=me modulo that. And it would be good to file a followup to fix the mozbrowser case too; loadinfo should not be using GetParent to get the parent window if it wants to respect mozbrowser.
Attachment #8666657 -
Flags: review?(bzbarsky) → review+
Assignee | ||
Comment 175•9 years ago
|
||
(In reply to Boris Zbarsky [:bz] from comment #174) > Comment on attachment 8666657 [details] [diff] [review] > bug1132203-toplevel-only-3.patch > > >+ if (loadInfo && loadInfo.parentOuterWindowID == loadInfo.outerWindowID) { > > I assume this code will never be used in any situation where <iframe > mozbrowser> might be used? Because it will do the wrong thing in that > situation.... > > r=me modulo that. And it would be good to file a followup to fix the > mozbrowser case too; loadinfo should not be using GetParent to get the > parent window if it wants to respect mozbrowser. Bill, what do you think? Does the usage of loadInfo seems ok to you? Honza
Flags: needinfo?(wmccloskey)
bz is right, we should be using GetScriptableParent instead. I filed bug 1209243 for this. I think it's fine to land this code now though. It just means that the JSON viewer won't activate in b2g until we fix bug 1209243.
Flags: needinfo?(wmccloskey)
Reporter | ||
Comment 177•9 years ago
|
||
(In reply to Bill McCloskey (:billm) from comment #176) > bz is right, we should be using GetScriptableParent instead. I filed bug > 1209243 for this. I think it's fine to land this code now though. It just > means that the JSON viewer won't activate in b2g until we fix bug 1209243. Sounds reasonable. Let's ship this thing :)
Assignee | ||
Comment 178•9 years ago
|
||
Try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=720e74aec9a7 https://treeherder.mozilla.org/perf.html#/comparechooser?newProject=try&newRevision=720e74aec9a7 Honza
Keywords: checkin-needed
Comment 179•9 years ago
|
||
https://hg.mozilla.org/integration/fx-team/rev/d618bbcee986 https://hg.mozilla.org/integration/fx-team/rev/f8edd4fa4d00 https://hg.mozilla.org/integration/fx-team/rev/ce3ca3b43597 https://hg.mozilla.org/integration/fx-team/rev/f6fa722a8636 https://hg.mozilla.org/integration/fx-team/rev/ed8ce0300161
Keywords: checkin-needed
Backed out in https://hg.mozilla.org/integration/fx-team/rev/288d12e9236b for dt2 orange (which was in the try run): https://treeherder.mozilla.org/logviewer.html#?job_id=4891631&repo=fx-team
Flags: needinfo?(odvarko)
I'm also seeing https://treeherder.mozilla.org/logviewer.html#?job_id=4888440&repo=fx-team frequently but not constantly ever since this landed.
Assignee | ||
Comment 182•9 years ago
|
||
> https://treeherder.mozilla.org/logviewer.html#?job_id=4891631&repo=fx-team
This is weird...
Ryan, any idea why this test would fail due to the JSON Viewer?
I don't see any connection...
Honza
Flags: needinfo?(odvarko) → needinfo?(jryans)
(In reply to Jan Honza Odvarko [:Honza] from comment #182) > > https://treeherder.mozilla.org/logviewer.html#?job_id=4891631&repo=fx-team > This is weird... > > Ryan, any idea why this test would fail due to the JSON Viewer? > I don't see any connection... I can't see why either... maybe :past knows?
Flags: needinfo?(jryans) → needinfo?(past)
Comment 184•9 years ago
|
||
The culprit seems to be this error: TypeError: object in compartment marked as invisible to Debugger Other than that I don't have any clues. More in-depth debugging is needed.
Flags: needinfo?(past)
Assignee | ||
Comment 185•9 years ago
|
||
Ryan, the attached patch includes: - Fix for themes - Fix for the intermittent test (json viewer filter) Try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=210083c06aaf Btw. I don't see the dt2 orange anymore. Honza
Attachment #8668514 -
Flags: review?(jryans)
I don't understand the purpose of these changes. Can you explain in more detail?
Flags: needinfo?(odvarko)
Assignee | ||
Comment 187•9 years ago
|
||
(In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #186) > I don't understand the purpose of these changes. Can you explain in more > detail? - Fix for theme The CSS file for dark and light theme has been inserted into the page using the chrome:// protocol URL. I am not sure why this worked before (platform changes?), but including chrome:// resources into content scope doesn't work for the viewer anymore, so I am directly including the CSS source instead. - Fix for the intermittent test (json viewer filter) The filter test "browser_jsonview_filter.js" failed sometimes and I believe it was because a mutation observer has been waiting for changes that already happened. So, there is code that checks the UI first and then waits for changes. Honza
Flags: needinfo?(odvarko)
(In reply to Jan Honza Odvarko [:Honza] from comment #187) > - Fix for theme > The CSS file for dark and light theme has been inserted into the page using > the chrome:// protocol URL. I am not sure why this worked before (platform > changes?), but including chrome:// resources into content scope doesn't work > for the viewer anymore, so I am directly including the CSS source instead. Ah, this is a subtle change from the file migration. Previously our chrome:// files were in browser.jar which was marked as client accessible. Now, they are in devtools.jar, which does not do this. I think it's better to republish them as resource:/// URIs too, which are content accessible. I'll attach a patch. Then you can reference them as resource:///modules/devtools/client/themes/*.css.
Attachment #8668544 -
Flags: review?(odvarko)
(In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #188) > (In reply to Jan Honza Odvarko [:Honza] from comment #187) > > - Fix for theme > > The CSS file for dark and light theme has been inserted into the page using > > the chrome:// protocol URL. I am not sure why this worked before (platform > > changes?), but including chrome:// resources into content scope doesn't work > > for the viewer anymore, so I am directly including the CSS source instead. > > Ah, this is a subtle change from the file migration. Previously our > chrome:// files were in browser.jar which was marked as client accessible. > Now, they are in devtools.jar, which does not do this. I meant "content accessible" here.
Comment on attachment 8668514 [details] [diff] [review] bug1132203-theme-fix-1.patch Review of attachment 8668514 [details] [diff] [review]: ----------------------------------------------------------------- Test fix seems okay, but let's look at this again after the patch I attached.
Attachment #8668514 -
Flags: review?(jryans)
Assignee | ||
Comment 192•9 years ago
|
||
Comment on attachment 8668544 [details] [diff] [review] bug1132203-expose-themes-1.patch Review of attachment 8668544 [details] [diff] [review]: ----------------------------------------------------------------- Excellent, works for me. Honza
Attachment #8668544 -
Flags: review?(odvarko) → review+
Assignee | ||
Comment 193•9 years ago
|
||
Now using resource:/// for exposed themes. Honza
Attachment #8668514 -
Attachment is obsolete: true
Attachment #8668926 -
Flags: review?(jryans)
Assignee | ||
Comment 194•9 years ago
|
||
Try push with all current patches: https://treeherder.mozilla.org/#/jobs?repo=try&revision=f8ff3b25ba99 Honza
Comment on attachment 8668926 [details] [diff] [review] bug1132203-theme-fix-2.patch Review of attachment 8668926 [details] [diff] [review]: ----------------------------------------------------------------- ::: devtools/client/jsonview/converter-child.js @@ +198,5 @@ > var themeClassName = "theme-" + JsonViewUtils.getCurrentTheme(); > var baseUrl = "resource:///modules/devtools/client/jsonview/"; > var theme = (themeClassName == "theme-light") ? "light" : "dark"; > var themeUrl = '<link rel="stylesheet" type="text/css" ' + > + 'href="resource:///modules//devtools/client/themes/' + theme + '-theme.css">'; "modules//devtools" should be "modules/devtools"
Attachment #8668926 -
Flags: review?(jryans) → review+
Assignee | ||
Comment 196•9 years ago
|
||
(In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #195) > Comment on attachment 8668926 [details] [diff] [review] > bug1132203-theme-fix-2.patch > > Review of attachment 8668926 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: devtools/client/jsonview/converter-child.js > @@ +198,5 @@ > > var themeClassName = "theme-" + JsonViewUtils.getCurrentTheme(); > > var baseUrl = "resource:///modules/devtools/client/jsonview/"; > > var theme = (themeClassName == "theme-light") ? "light" : "dark"; > > var themeUrl = '<link rel="stylesheet" type="text/css" ' + > > + 'href="resource:///modules//devtools/client/themes/' + theme + '-theme.css">'; > > "modules//devtools" should be "modules/devtools" Fixed Honza
Attachment #8668926 -
Attachment is obsolete: true
Attachment #8669099 -
Flags: review+
Assignee | ||
Comment 197•9 years ago
|
||
I am seeing a lot of test failures: INFO TEST-UNEXPECTED-FAIL | browser/base/content/test/general/browser_parsable_css.js | Got error message for jar:file:///builds/slave/test/build/application/NightlyDebug.app/Contents/Resources/browser/omni.ja!/modules/devtools/client/themes/light-theme.css: Selector expected. Ruleset ignored due to bad selector. - in the try push from comment #194. Could that be related to the theme files exposing? Honza
Flags: needinfo?(jryans)
Okay, the failure is because the file uses preprocessing, and from resource:// it's not preprocessed yet. We want to convert away from preprocessing (bug 1183280) anyway, but I don't want to make us wait for that here... Where are you actually using any of theme class names...? I couldn't find them your patches.
Flags: needinfo?(jryans) → needinfo?(odvarko)
I think the simplest answer for now might be to just not use the theme file for now and we can improve the design in a follow up after you land this bug.
Assignee | ||
Comment 200•9 years ago
|
||
(In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #199) > I think the simplest answer for now might be to just not use the theme file > for now and we can improve the design in a follow up after you land this bug. We can use the way I implemented it before (direct CSS source injection) and improve it in a follow up. I've already test is and it works well. Honza
Flags: needinfo?(odvarko)
Comment 201•9 years ago
|
||
(In reply to Jan Honza Odvarko [:Honza] from comment #200) > (In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #199) > > I think the simplest answer for now might be to just not use the theme file > > for now and we can improve the design in a follow up after you land this bug. > We can use the way I implemented it before (direct CSS source injection) > and improve it in a follow up. I've already test is and it works well. > > Honza I think the specific question is what things from the theme are needed for the JSON viewer to work. i.e. is it using all sorts of styling from the toolbox, or just a background color? It may be simpler to not include any toolbox CSS at all if it's not strictly necessary. It'd also make it less likely that CSS changes to the toolbox cause some weird regression to this page.
See :bgrins comment 201, that info would help determine how to best proceed.
Flags: needinfo?(odvarko)
Assignee | ||
Comment 203•9 years ago
|
||
(In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #202) > See :bgrins comment 201, that info would help determine how to best proceed. I agree that it really looks simpler to not include any toolbox CSS at all (at least now). The only things the viewer is actually using from the theme files are the variables. See my patch, I copied those vars the viewer actually needs and put them into two CSS files within the jsonviewer/css directory - and the viewer is using them instead of the original theme files. Perhaps this is the way to go? (btw. the patch also fixes a few button class names) Honza
Attachment #8669099 -
Attachment is obsolete: true
Flags: needinfo?(odvarko)
Attachment #8669598 -
Flags: review?(jryans)
Comment on attachment 8669598 [details] [diff] [review] bug1132203-theme-fix-4.patch Okay, I can live with this, but please file a follow up bug to clean this up after bug 1210954 lands. :bgrins is splitting out the "official" themes into just variables over there, so you should be able to use that from a resource:// URL in the follow up bug.
Attachment #8669598 -
Flags: review?(jryans) → review+
Assignee | ||
Comment 205•9 years ago
|
||
(In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #204) > Comment on attachment 8669598 [details] [diff] [review] > bug1132203-theme-fix-4.patch > > Okay, I can live with this, but please file a follow up bug to clean this up > after bug 1210954 lands. :bgrins is splitting out the "official" themes > into just variables over there, so you should be able to use that from a > resource:// URL in the follow up bug. Yes, that bug fix will help. Try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=21196064e071 Honza
Assignee | ||
Updated•9 years ago
|
Attachment #8668544 -
Attachment is obsolete: true
Assignee | ||
Comment 206•9 years ago
|
||
Ryan, I marked the 'bug1132203-expose-themes-1.patch' patch as obsolete since it isn't needed for this report. I don't know what it your plan with exposing themes, so let me know if you want me to create another bug and lend it separately! The try push from comment #205 looks good to me, marking as checkin-needed. Honza
Keywords: checkin-needed
Comment 207•9 years ago
|
||
so all patches here need checkin ?
Assignee | ||
Comment 208•9 years ago
|
||
Yes, here is the list (in the same order): bug1132203-libs-4.patch bug1132203-viewer-12.patch bug1132203-tests-5.patch bug1132203-loadondemand-8.patch bug1132203-toplevel-only-3.patch bug1132203-theme-fix-4.patch Honza
(In reply to Jan Honza Odvarko [:Honza] from comment #206) > Ryan, I marked the 'bug1132203-expose-themes-1.patch' patch as obsolete > since it isn't needed for this report. I don't know what it your plan with > exposing themes, so let me know if you want me to create another bug and > lend it separately! Can you file a bug that depends on bug 1210954 (and this one maybe?) to de-duplicate the theme vars for JSON viewer by exposing them? Don't worry about my patch, it would be different anyway since :bgrins has made new files.
Flags: needinfo?(odvarko)
Assignee | ||
Comment 210•9 years ago
|
||
(In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #209) > Can you file a bug that depends on bug 1210954 (and this one maybe?) to > de-duplicate the theme vars for JSON viewer by exposing them? Sure, here it is: bug 1211918 Honza
Flags: needinfo?(odvarko)
Comment 211•9 years ago
|
||
https://hg.mozilla.org/integration/fx-team/rev/2a6416177736 https://hg.mozilla.org/integration/fx-team/rev/f00c53c8ffd4 https://hg.mozilla.org/integration/fx-team/rev/f3023ebc9881 https://hg.mozilla.org/integration/fx-team/rev/0677c5169e05 https://hg.mozilla.org/integration/fx-team/rev/8a90df6a9ae7 https://hg.mozilla.org/integration/fx-team/rev/d6e2d567d8c3
Keywords: checkin-needed
Comment 212•9 years ago
|
||
sorry had to back this out for test failures like https://treeherder.mozilla.org/logviewer.html#?job_id=5016968&repo=fx-team
Flags: needinfo?(odvarko)
Comment 213•9 years ago
|
||
Backout: https://hg.mozilla.org/integration/fx-team/rev/544a40b58d55
Assignee | ||
Comment 214•9 years ago
|
||
(In reply to Carsten Book [:Tomcat] from comment #212) > sorry had to back this out for test failures like > https://treeherder.mozilla.org/logviewer.html#?job_id=5016968&repo=fx-team Sorry about that failure, seemed like not related, I am working on it. Ryan, Panos: I have been hunting this problem: TypeError: object in compartment marked as invisible to Debugger ... and it's caused by loading the loader module: const {devtools} = Cu.import("resource://gre/modules/devtools/shared/Loader.jsm", {}); ... within converter-observer.js script that is loaded using loadProcessScript as follows: Services.ppmm.loadProcessScript( "resource:///modules/devtools/client/jsonview/converter-observer.js", true); The code that fires the TypeError above is in actors/webconsole.js (line 446): let dbgGlobal = this.dbg.makeGlobalObjectReference(global); The |global| is a Sandbox object. Why loading Loader.jsm module would cause that? Honza
Flags: needinfo?(past)
Flags: needinfo?(odvarko)
Flags: needinfo?(jryans)
This seems quite strange so far! I haven't been able to find the core issue yet. Here's what I know so far: * The process script converter-observer.js gets loaded 1 time in the parent, and 2 times in the child * The extra time in the child is triggered by GCLI which loads toolbox.js to get ToolboxButtons, which in turn loads gDevTools.jsm in the child (filed bug 1212689 to fix this) * However, fixing this so the process script is only loaded 1 time in the child does not seem to fix the core issue here * As Honza said, if you remove the Loader.jsm import from content-observer, the issue goes away * If I copy Loader.jsm to a separate name like TestLoader.jsm, and content-observer instead loads this new file, the issue goes away * Flipping the "invisibleToDebugger" setting inside Loader.jsm seems to have no impact on this issue
Assignee | ||
Comment 216•9 years ago
|
||
Note that it also helps if the module is loaded asynchronously using e.g. Services.tm.mainThread.dispatch (however this can't be used as a workaround as the first default page would be loaded before the json viewer service is registered). Honza
Okay, in my testing, the error is fixed by loading Loader.jsm lazily from converter-observer.js: XPCOMUtils.defineLazyGetter(this, "devtools", function() { const {devtools} = Cu.import("resource://gre/modules/devtools/shared/Loader.jsm", {}); return devtools; }); However, I still do not really understand the core issue of why webconsole actor was failing. Anyway, I hope this will be enough to land your work!
Flags: needinfo?(jryans)
Assignee | ||
Comment 218•9 years ago
|
||
(In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #217) > Okay, in my testing, the error is fixed by loading Loader.jsm lazily from > converter-observer.js: > > XPCOMUtils.defineLazyGetter(this, "devtools", function() { > > const {devtools} = > Cu.import("resource://gre/modules/devtools/shared/Loader.jsm", {}); > return devtools; > > }); > > However, I still do not really understand the core issue of why webconsole > actor was failing. Anyway, I hope this will be enough to land your work! I did exactly the same, but this works only if the devtools.jsonview.enabled pref is set to false, which is by default for non dev-edition channels. In dev-edition, the ConverterObserver.register() method is executed immediately when the converter-observer.js is loaded and so, the lazy getters as well -> causing the issue to appear. I think we need to figure out the core problem or clone the Loader.jsm into temp module. Would this be acceptable for you as a workaround? Honza
Flags: needinfo?(jryans)
Assignee | ||
Comment 219•9 years ago
|
||
@Jim: do you know what the problem could be? (start reading from comment #214) Honza
Flags: needinfo?(jimb)
(In reply to Jan Honza Odvarko [:Honza] from comment #218) > (In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #217) > > Okay, in my testing, the error is fixed by loading Loader.jsm lazily from > > converter-observer.js: > > > > XPCOMUtils.defineLazyGetter(this, "devtools", function() { > > > > const {devtools} = > > Cu.import("resource://gre/modules/devtools/shared/Loader.jsm", {}); > > return devtools; > > > > }); > > > > However, I still do not really understand the core issue of why webconsole > > actor was failing. Anyway, I hope this will be enough to land your work! > I did exactly the same, but this works only if the devtools.jsonview.enabled > pref is set to false, which is by default for non dev-edition channels. In > dev-edition, the ConverterObserver.register() method is executed immediately > when the converter-observer.js is loaded and so, the lazy getters as well -> > causing the issue to appear. Ah sorry, I had hacked up a lot of things in debugging. I am seeing this as well. > I think we need to figure out the core problem or clone the Loader.jsm into > temp module. Would this be acceptable for you as a workaround? Unfortunately, I don't think we should accept cloning Loader.jsm. That file is at the heart of how we load the tools, and is already confusing enough with just one version. Keeping track of another sounds dangerous to me. Anyway, I've made some more discoveries. In Loader.jsm, we set the following: invisibleToDebugger: Services.appinfo.name !== "Firefox" So, since we are running Firefox, it should be set to "false". That's indeed what used to happen. However, your changes now trigger Loader.jsm from a process script, which is the first time we've done that. It appears that the child process does not have any `appinfo` data until **after** process scripts are run[1]. So, since we're now first invoking Loader.jsm in child from a process script, it's set to true there. It seems like a bug, either in the test or in functionality, that the web console test fails when `invisibleToDebugger === true`. I filed bug 1213427 to investigate why this failure happens. However, it does not seem appropriate to just disable the test. It also does not seem good to just allow it to become `invisibleToDebugger === true` in the child process, since this break debugging of DevTools files that are loaded in the child. It seems we need to construct a way to set `invisibleToDebugger` to false for all desktop Firefox processes, but without using `appinfo`. [1]: https://dxr.mozilla.org/mozilla-central/rev/d01dd42e654b8735d86f9e7c723cc869a3b56798/dom/ipc/ContentParent.cpp#2538
Flags: needinfo?(past)
Flags: needinfo?(jryans)
Flags: needinfo?(jimb)
Ryan, can you try switching the order of this line: https://dxr.mozilla.org/mozilla-central/rev/d01dd42e654b8735d86f9e7c723cc869a3b56798/dom/ipc/ContentParent.cpp#2519 with this block: https://dxr.mozilla.org/mozilla-central/rev/d01dd42e654b8735d86f9e7c723cc869a3b56798/dom/ipc/ContentParent.cpp#2529-2539 I think that might fix this problem.
(In reply to Bill McCloskey (:billm) from comment #221) > Ryan, can you try switching the order of this line: > https://dxr.mozilla.org/mozilla-central/rev/ > d01dd42e654b8735d86f9e7c723cc869a3b56798/dom/ipc/ContentParent.cpp#2519 > with this block: > https://dxr.mozilla.org/mozilla-central/rev/ > d01dd42e654b8735d86f9e7c723cc869a3b56798/dom/ipc/ContentParent.cpp#2529-2539 > > I think that might fix this problem. I considered that, but there seems to be a more local solution: I can switch to using AppConstants.jsm which is always available.
OK, but we should fix this anyway. It'll take 15 minutes and maybe save someone a few hours down the road.
(In reply to Bill McCloskey (:billm) from comment #223) > OK, but we should fix this anyway. It'll take 15 minutes and maybe save > someone a few hours down the road. Alright, I'll try it your way. :) The comments suggested to me it would not work... I'll test it now.
Okay, it did indeed work! Thanks for the nudge in the right direction, Bill. Try with new patch: https://treeherder.mozilla.org/#/jobs?repo=try&revision=40a01df8cd0b
Attachment #8672140 -
Flags: review?(wmccloskey)
This time with the right file...
Attachment #8672140 -
Attachment is obsolete: true
Attachment #8672140 -
Flags: review?(wmccloskey)
Attachment #8672141 -
Flags: review?(wmccloskey)
Comment on attachment 8672141 [details] [diff] [review] appinfo-process-script Review of attachment 8672141 [details] [diff] [review]: ----------------------------------------------------------------- Thanks!
Attachment #8672141 -
Flags: review?(wmccloskey) → review+
Assignee | ||
Comment 228•9 years ago
|
||
All look good, let's try to land it! Honza
Keywords: checkin-needed
Comment 229•9 years ago
|
||
https://hg.mozilla.org/integration/fx-team/rev/7a430de890ef https://hg.mozilla.org/integration/fx-team/rev/1c2a1cba58b5 https://hg.mozilla.org/integration/fx-team/rev/3237d79fe9ab https://hg.mozilla.org/integration/fx-team/rev/173e398925e3 https://hg.mozilla.org/integration/fx-team/rev/2bd39f632ac5 https://hg.mozilla.org/integration/fx-team/rev/05740dc2f009 https://hg.mozilla.org/integration/fx-team/rev/2a20ae6dda0e
Keywords: checkin-needed
Comment 230•9 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/7a430de890ef https://hg.mozilla.org/mozilla-central/rev/1c2a1cba58b5 https://hg.mozilla.org/mozilla-central/rev/3237d79fe9ab https://hg.mozilla.org/mozilla-central/rev/173e398925e3 https://hg.mozilla.org/mozilla-central/rev/2bd39f632ac5 https://hg.mozilla.org/mozilla-central/rev/05740dc2f009 https://hg.mozilla.org/mozilla-central/rev/2a20ae6dda0e
Comment 231•9 years ago
|
||
Given that Firefox doesn't have any better way to display pages with JSON content, have we considered enabling this by default in all version of Firefox?
Flags: needinfo?(odvarko)
Comment 232•9 years ago
|
||
Note that the current implementation is a spec violation (in that it changes the observable DOM from what the spec says it should be). The fact that we're restricting it to toplevel documents only probably helps some, so we might be OK with shipping it, but it's technically not the right behavior, and could in theory cause compat problems.
Assignee | ||
Comment 233•9 years ago
|
||
(In reply to Panos Astithas [:past] from comment #231) > Given that Firefox doesn't have any better way to display pages with JSON > content, have we considered enabling this by default in all version of > Firefox? No, at least not for now. The feature has been planned for DevEdition only, but let's see what the feedback says and we can discuss again. In any case, we should keep in mind the comment #232 from Boris. Honza
Flags: needinfo?(odvarko)
Should we relnote this feature in Dev. Ed.? Also is there a bug for enabling this outside of Dev. Ed? Should the relnote wait until it's available everywhere?
Flags: needinfo?(odvarko)
Keywords: dev-doc-needed
Assignee | ||
Comment 235•9 years ago
|
||
(In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #234) > Should we relnote this feature in Dev. Ed.? Yes > Also is there a bug for enabling this outside of Dev. Ed? AFAIK no > Should the relnote wait until it's available everywhere? No, the feature should be noted in DE 44 release notes. Honza
Flags: needinfo?(odvarko)
Comment 236•9 years ago
|
||
Added to FF44 release notes. Wording used: "Application/JSON documents can be easily inspected using interactive viewer"
relnote-firefox:
--- → 44+
Comment 237•8 years ago
|
||
Honza, I've drafted a page on this. Please let me know if I'm missing anything.
Flags: needinfo?(odvarko)
Updated•8 years ago
|
Keywords: dev-doc-needed → dev-doc-complete
Assignee | ||
Comment 238•8 years ago
|
||
(In reply to Will Bamberg [:wbamberg] from comment #237) > Honza, I've drafted a page on this. Please let me know if I'm missing > anything. Sorry for the delay (I've been on PTO). Where I can see the page? (if it isn't too late already) Honza
Flags: needinfo?(odvarko)
Comment 239•8 years ago
|
||
Oops :) https://developer.mozilla.org/en-US/docs/Tools/JSON_viewer
Assignee | ||
Comment 240•8 years ago
|
||
I added a comment about `devtools.jsonview.enabled` pref that allows to enable this features in other releases channels. Some improvements are coming and I'll update the page when they are landed. Thanks for creating the page! Honza
Target Milestone: Firefox 43 → Firefox 44
Updated•7 years ago
|
Depends on: CVE-2017-5390
Comment 241•7 years ago
|
||
> Some improvements are coming and I'll update the page when they are landed.
Been a year since you said that, Jan. Any movement on this yet?
(In reply to Andy Mercer from comment #241) > > Some improvements are coming and I'll update the page when they are landed. > > Been a year since you said that, Jan. Any movement on this yet? Are you asking about particular improvements? Or the page content? Or something else...?
(In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #242) > (In reply to Andy Mercer from comment #241) > > > Some improvements are coming and I'll update the page when they are landed. > > > > Been a year since you said that, Jan. Any movement on this yet? > > Are you asking about particular improvements? Or the page content? Or > something else...? If you were wondering when it will ship in release, please follow bug 1243951 for this. (Hopefully in 53.)
Comment 244•7 years ago
|
||
(In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #243) > If you were wondering when it will ship in release, please follow bug > 1243951 for this. (Hopefully in 53.) Yes, that is what I was wondering, thanks!
Updated•6 years ago
|
Product: Firefox → DevTools
You need to log in
before you can comment on or make changes to this bug.
Description
•