Closed Bug 1308566 Opened 8 years ago Closed 7 years ago

Use the object inspector instead of variables view

Categories

(DevTools :: Console, enhancement, P1)

Product:
DevTools
Component:
Console
Type:
enhancement

Tracking

(firefox56 verified)

Status:
VERIFIED FIXED
Milestone:
Firefox 56
Iteration:
56.3 - Jul 24
Tracking Flags:
Tracking Status
firefox56 --- verified

People

(Reporter: linclark, Assigned: nchevobbe)

References

(Depends on 5 open bugs)

Details

(Whiteboard: [console-html])

Attachments

(8 files)

Bug 1308566 - Update reps bundle to v0.9.
(deleted), text/x-review-board-request
bgrins
: review+
Details
Bug 1308566 - Use Object Inspector in Console.
(deleted), text/x-review-board-request
bgrins
: review+
Honza
: review+
Details
Bug 1308566 - Release actors from loaded properties.
(deleted), text/x-review-board-request
bgrins
: review+
Honza
: review+
Details
Bug 1308566 - Fixes broken tests due to the ObjectInspector switch.
(deleted), text/x-review-board-request
bgrins
: review+
Details
Bug 1308566 - Add a mochitest for the ObjectInspector in the console.
(deleted), text/x-review-board-request
bgrins
: review+
Details
Bug 1308566 - Fix Reps README.
(deleted), text/x-review-board-request
bgrins
: review+
Details
Bug 1308566 - Remove reps mochitests.
(deleted), text/x-review-board-request
bgrins
: review+
Details
Bug 1308566 - Exclude reps.css from browser_parsable_css test.
(deleted), text/x-review-board-request
bgrins
: review+
Details
We should use the object inspector from the debugger instead of variables view.
Priority: -- → P2
Whiteboard: new-console
Blocks: 1307922
Blocks: enable-new-console
Flags: qe-verify+
Priority: P2 → P3
QA Contact: iulia.cristescu
Whiteboard: new-console → [reserve-new-console]
Priority: P3 → P2
Whiteboard: [reserve-new-console] → [new-console]
Including the object inspector in reps is being tracked here: https://github.com/devtools-html/devtools-reps/issues/11
No longer blocks: 1307922
Priority: P2 → P3
Whiteboard: [new-console] → [reserve-new-console]
Blocks: 1326837
Blocks: 1258584
Blocks: 1137281
No longer blocks: 1326837
Whiteboard: [reserve-new-console] → [reserve-console-html]
Priority: P3 → P2
Whiteboard: [reserve-console-html] → [console-html]
Here's a patch of what ObjectInspector integration could look like in the console: https://reviewboard.mozilla.org/r/152874/diff/1#index_header This is not ready for proper review, since the reps bundle hasn't landed yet, there are no tests and there need to be some cleanup here. I just want to make sure we're heading in the right direction with this. So, Brian, Honza, any feedback is welcomed so I can fix possible issues and be ready when the reps bundle land.
Flags: needinfo?(odvarko)
Flags: needinfo?(bgrinstead)
Comment on attachment 8881799 [details] Bug 1308566 - Use Object Inspector in Console. https://reviewboard.mozilla.org/r/152874/#review158222 Looks greate, couple of inline comments. Honza ::: devtools/client/webconsole/new-console-output/actions/messages.js:109 (Diff revision 1) > type: NETWORK_MESSAGE_UPDATE, > message, > }; > } > > +function messageObjectPropertiesLoad(id, client, grip) { Some comments explaining the new actions would be great ::: devtools/client/webconsole/new-console-output/reducers/messages.js:203 (Diff revision 1) > return state.set("messagesTableDataById", messagesTableDataById.set(id, data)); > + > + case constants.MESSAGE_OBJECT_PROPERTIES_RECEIVE: > + return state.set( > + "messagesObjectPropertiesById", > + messagesObjectPropertiesById.set( Clean up of the `messageObjectPropertiesById` is also needed.
Flags: needinfo?(odvarko)
Comment on attachment 8881799 [details] Bug 1308566 - Use Object Inspector in Console. https://reviewboard.mozilla.org/r/152874/#review158552 ::: devtools/client/themes/webconsole.css:154 (Diff revision 2) > } > > .message-body > * { > white-space: pre-wrap; > word-wrap: break-word; > + flex-shrink: 0; Please make this property (and the one below) only apply on the new frontend
Assignee: nobody → nchevobbe
Status: NEW → ASSIGNED
Iteration: --- → 56.2 - Jul 10
Priority: P2 → P1
Comment on attachment 8882229 [details] Bug 1308566 - Fixes broken tests due to the ObjectInspector switch. https://reviewboard.mozilla.org/r/153328/#review160420
Attachment #8882229 - Flags: review?(bgrinstead) → review+
Comment on attachment 8884328 [details] Bug 1308566 - Fix Reps README. https://reviewboard.mozilla.org/r/155258/#review160422
Attachment #8884328 - Flags: review?(bgrinstead) → review+
Comment on attachment 8884329 [details] Bug 1308566 - Remove reps mochitests. https://reviewboard.mozilla.org/r/155260/#review160424
Attachment #8884329 - Flags: review?(bgrinstead) → review+
Blocks: 1379570
Attachment #8881799 - Flags: review?(bgrinstead)
Attachment #8882030 - Flags: review?(odvarko)
Attachment #8881799 - Flags: review?(odvarko)
Comment on attachment 8883499 [details] Bug 1308566 - Add a mochitest for the ObjectInspector in the console. https://reviewboard.mozilla.org/r/154392/#review160998 Looks good ::: devtools/client/webconsole/new-console-output/test/mochitest/browser_webconsole_object_inspector.js:20 (Diff revision 3) > + const store = hud.ui.newConsoleOutput.getStore(); > + // Adding logging each time the store is modified in order to check > + // the store state in case of failure. > + store.subscribe(() => { > + const messages = store.getState().messages.messagesById > + .reduce(function (res, message) { Could be shortened with: ``` function(res, { id, type, parameters, messageText }) { res.push({ id, type, parameters, messageText }); return res; } ```
Attachment #8883499 - Flags: review?(bgrinstead) → review+
These look good overall! I want Honza to have a look at the two remaining patches before we land. One usage note: when evaluating `window.location` I see "undefined" as the preview text in OI (renders "Location" without the patches applied)
Flags: needinfo?(bgrinstead)
Iteration: 56.2 - Jul 10 → 56.3 - Jul 24
(In reply to Brian Grinstead [:bgrins] from comment #34) > One usage note: when evaluating `window.location` I see "undefined" as the > preview text in OI (renders "Location" without the patches applied) Thanks for spotting that. https://github.com/devtools-html/devtools-core/pull/488 will fix this.
No longer blocks: 1379570
Comment on attachment 8882030 [details] Bug 1308566 - Release actors from loaded properties. https://reviewboard.mozilla.org/r/153104/#review161200 Looks good to me, thanks for the patch Nicolas! R+ assuming Try is green Honza
Attachment #8882030 - Flags: review+
Attachment #8881799 - Flags: review?(bgrinstead)
Comment on attachment 8881798 [details] Bug 1308566 - Update reps bundle to v0.9. https://reviewboard.mozilla.org/r/152872/#review161204 I quickly scanned through the patch and haven't seen anything suspicious. All these changes went through a review on github. Honza
Attachment #8881798 - Flags: review+
Attachment #8881799 - Flags: review?(odvarko)
Attachment #8881799 - Flags: review?(bgrinstead)
Attachment #8881799 - Flags: review?(odvarko)
Attachment #8881799 - Flags: review?(bgrinstead)
Comment on attachment 8881799 [details] Bug 1308566 - Use Object Inspector in Console. https://reviewboard.mozilla.org/r/152874/#review161206 Thanks for workin on this! Please see couple of inline comments. Honza ::: devtools/client/themes/webconsole.css:759 (Diff revision 8) > +} > + > +.webconsole-output-wrapper .message-flex-body > .message-body { > + display: flex; > + flex-wrap: wrap; > + max-width: 100%; Are these style needed? They are overriden/duplicated later in the same file (line 895) ::: devtools/client/webconsole/new-console-output/actions/messages.js:109 (Diff revision 8) > type: NETWORK_MESSAGE_UPDATE, > message, > }; > } > > +function messageObjectPropertiesLoad(id, client, grip) { nit: a comment explaining new actions & logic would be nice.
Attachment #8881799 - Flags: review?(odvarko)
Comment on attachment 8881799 [details] Bug 1308566 - Use Object Inspector in Console. https://reviewboard.mozilla.org/r/152874/#review161206 > Are these style needed? They are overriden/duplicated later in the same file (line 895) Can you point me to where you see those styles are overriden ? I can't go to line 895 since the file is only ~860 lines, and I can't find where we have similar rules after here. > nit: a comment explaining new actions & logic would be nice. sure
(In reply to Nicolas Chevobbe [:nchevobbe] from comment #62) > Comment on attachment 8881799 [details] > Bug 1308566 - Use Object Inspector in Console. > > https://reviewboard.mozilla.org/r/152874/#review161206 > > > Are these style needed? They are overriden/duplicated later in the same file (line 895) Ah sorry, this came from different patch (also applied). Honza
Comment on attachment 8881799 [details] Bug 1308566 - Use Object Inspector in Console. https://reviewboard.mozilla.org/r/152874/#review161254
Attachment #8881799 - Flags: review?(odvarko) → review+
Comment on attachment 8881798 [details] Bug 1308566 - Update reps bundle to v0.9. https://reviewboard.mozilla.org/r/152872/#review161266 Does this include the fix from https://github.com/devtools-html/devtools-core/pull/488 ?
Attachment #8881798 - Flags: review?(bgrinstead) → review+
Comment on attachment 8881799 [details] Bug 1308566 - Use Object Inspector in Console. https://reviewboard.mozilla.org/r/152874/#review161206 > Can you point me to where you see those styles are overriden ? I can't go to line 895 since the file is only ~860 lines, and I can't find where we have similar rules after here. Honza had another patch applied that added rules. This was not an issue from this patch. > sure done
Comment on attachment 8882030 [details] Bug 1308566 - Release actors from loaded properties. https://reviewboard.mozilla.org/r/153104/#review161274
Attachment #8882030 - Flags: review?(bgrinstead) → review+
(In reply to Brian Grinstead [:bgrins] from comment #76) > Comment on attachment 8881798 [details] > Bug 1308566 - Update reps bundle to v0.9. > > https://reviewboard.mozilla.org/r/152872/#review161266 > > Does this include the fix from > https://github.com/devtools-html/devtools-core/pull/488 ? Yes, it does :) Here is the hopefully last try push https://treeherder.mozilla.org/#/jobs?repo=try&revision=9d92e0882a0f4896a6ef05e2d121ee1548c9eef3
Comment on attachment 8881799 [details] Bug 1308566 - Use Object Inspector in Console. https://reviewboard.mozilla.org/r/152874/#review161276
Attachment #8881799 - Flags: review?(bgrinstead) → review+
Comment on attachment 8885359 [details] Bug 1308566 - Exclude reps.css from browser_parsable_css test. https://reviewboard.mozilla.org/r/156206/#review161364 Capturing our discussion - since reps is shared with the debugger ui it's using -webkit-user-select. debugger.css is already excluded in this file as well
Attachment #8885359 - Flags: review?(bgrinstead) → review+
Pushed by nchevobbe@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/c8237d11b29c Update reps bundle to v0.9. r=bgrins,Honza https://hg.mozilla.org/integration/autoland/rev/fc543151ddfe Fix Reps README. r=bgrins https://hg.mozilla.org/integration/autoland/rev/50f96501291a Remove reps mochitests. r=bgrins https://hg.mozilla.org/integration/autoland/rev/f727b5fb76e3 Use Object Inspector in Console. r=bgrins,Honza https://hg.mozilla.org/integration/autoland/rev/cbf450d41bc4 Release actors from loaded properties. r=bgrins,Honza https://hg.mozilla.org/integration/autoland/rev/3d4b8196f740 Fixes broken tests due to the ObjectInspector switch. r=bgrins https://hg.mozilla.org/integration/autoland/rev/aae50b4ec136 Add a mochitest for the ObjectInspector in the console. r=bgrins https://hg.mozilla.org/integration/autoland/rev/42fbf03c91b7 Exclude reps.css from browser_parsable_css test. r=bgrins
Depends on: 1380494
Depends on: 1380499
Depends on: 1380501
Depends on: 1380502
Depends on: 1380504
Depends on: 1380506
Depends on: 1380707
Depends on: 1380709
Depends on: 1380711
I can confirm the bug is verified fixed on latest Nightly 56.0a1 (2017-07-17), using Windows 10 x64, Ubuntu 16.04 x64 and Mac OS X 10.11.6.
Status: RESOLVED → VERIFIED
status-firefox56: fixed → verified
Flags: qe-verify+
Depends on: 1387823
Depends on: 1387829
No longer depends on: 1380506
Depends on: 1388831
Depends on: 1390027
Depends on: 1391077
Depends on: 1391649
Blocks: 1367467
Depends on: 1405900
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: