Load new console frontend in an xhtml document instead of xul

VERIFIED FIXED in Firefox 56

Status

defect
P1
normal
VERIFIED FIXED
2 years ago
10 months ago

People

(Reporter: arni2033, Assigned: bgrins)

Tracking

(Blocks 1 bug, {regression})

Trunk
Firefox 56
Dependency tree / graph
Bug Flags:
in-qa-testsuite +

Firefox Tracking Flags

(firefox-esr52 disabled)

Details

(Whiteboard: [console-html])

Attachments

(4 attachments, 9 obsolete attachments)

59 bytes, text/x-review-board-request
bgrins
: review+
Details
59 bytes, text/x-review-board-request
bgrins
: review+
Details
59 bytes, text/x-review-board-request
bgrins
: review+
Details
59 bytes, text/x-review-board-request
bgrins
: review+
Details
Reporter

Description

2 years ago
>>>   My Info:   Win7_64, Nightly 52, 32bit, ID 20161001030430 (2016-10-01)
STR_1:
1. Open https://www.mozilla.org/en-US/firefox/nightly/firstrun/
2. Open devtools -> debugger
3. Press Escape to open split console. Enable filter bar. Close split console
4. Open split console.

AR:
 I see all buttons briefly displayed vertically, one under another. After 0.1s they align horizontally
 If performed on a new profile, STR_1 will leave a blank space on the right side of console.
ER:
 All buttons should be immediately aligned horizontally, shouldn't twitch/blink. No scrollbar or
 free space on the right side of console. Yes, the blank space appears when scrollbar hides   v_v

This is regression from bug 1304178. Regression range:
> https://hg.mozilla.org/integration/fx-team/pushloghtml?fromchange=37f78aca862224d7151c0fcae1ed8373fe11c83b&tochange=a5510966f80b9b2f5abf59ab32cf4c92d66c60de
Reporter

Updated

2 years ago
No longer blocks: 1277113
Reporter

Updated

2 years ago
Component: Untriaged → Developer Tools: Console
Assignee

Updated

2 years ago
Priority: -- → P3
Whiteboard: [new-console]
Flags: qe-verify?
Priority: P3 → P2
Flags: qe-verify? → qe-verify+
QA Contact: iulia.cristescu
No longer blocks: console-perf
Priority: P2 → P1
Assignee: nobody → jdescottes
Status: NEW → ASSIGNED
Iteration: --- → 54.2 - Feb 20
Iteration: 54.2 - Feb 20 → ---
Priority: P1 → P2
Assignee: jdescottes → nobody
Status: ASSIGNED → NEW
Whiteboard: [new-console] → [console-html]
Assignee: nobody → bgrinstead
Status: NEW → ASSIGNED
Iteration: --- → 55.4 - May 1
Priority: P2 → P1
See Also: → 1358393
Assignee

Comment 1

2 years ago
Posted patch console-html-WIP.patch (obsolete) — Splinter Review
Early WIP.  This creates a more simple HTML page and copies over relevant CSS and JS into new files
Assignee

Updated

2 years ago
Depends on: 1359597
Assignee

Updated

2 years ago
Summary: (twitching at startup) Buttons in console -> filter bar briefly aligned vertically when I open split console, causing blank space at the right → Load new console frontend in an xhtml document instead of xul
Assignee

Updated

2 years ago
Blocks: 1307765
Comment hidden (mozreview-request)
Assignee

Updated

2 years ago
Attachment #8861134 - Attachment is obsolete: true
Iteration: 55.4 - May 1 → 55.5 - May 15
Assignee

Updated

2 years ago
Blocks: 1362023
Assignee

Updated

2 years ago
Duplicate of this bug: 1341420
Iteration: 55.5 - May 15 → 55.6 - May 29
Iteration: 55.6 - May 29 → 55.7 - Jun 12
Comment hidden (mozreview-request)
Assignee

Comment 5

2 years ago
Honza, I've pushed up a rebased version of the patch.  It still has the double event issue we discussed, likely due to React being loaded twice as you mentioned
Fixed double load of the React module.

Related info:
Honza: https://bugzilla.mozilla.org/show_bug.cgi?id=1341159#c7
Jarda: https://bugzilla.mozilla.org/show_bug.cgi?id=1309866#c80

Final solution Bug 1342297 - Remove ReactDOM event system patch

Honza
Iteration: 55.7 - Jun 12 → 56.1 - Jun 26
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Assignee

Comment 11

2 years ago
mozreview-review
Comment on attachment 8877096 [details]
Bug 1326937 - Patch react-dom to avoid double load

https://reviewboard.mozilla.org/r/148432/#review152950

Hope we can use a non patched react-dom someday
Attachment #8877096 - Flags: review?(bgrinstead) → review+
Try push:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=d8d4714714cad14928dea267a98c95dec8892aec&selectedJob=106673800

I am seeing a lot of failures, hope these could help us to fix remaining problems here.

Honza
@brian: looks like many tests are failing because of missing `experimentalOutputNode`, what's that?
http://searchfox.org/mozilla-central/rev/d67ef71097da4d1aa344c9d9c672e49a7228e765/devtools/client/webconsole/webconsole.js#569

Honza
Flags: needinfo?(bgrinstead)
Test try push:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=03c9a7a220ed74637427506bef5da1eca77704f8

Patch that sets the `experimentalOutputNode` included. Just to see test failure not related to the output node being used in tests.


Honza
Assignee

Comment 15

2 years ago
(In reply to Jan Honza Odvarko [:Honza] from comment #13)
> @brian: looks like many tests are failing because of missing
> `experimentalOutputNode`, what's that?
> http://searchfox.org/mozilla-central/rev/
> d67ef71097da4d1aa344c9d9c672e49a7228e765/devtools/client/webconsole/
> webconsole.js#569
> 
> Honza

This is set when the new frontend is loaded in the old document.  With this patch applied:

* All references to experimentalOutputNode in the the new-console-output mochitests should be able to change to just `outputNode` (as set here: https://dxr.mozilla.org/mozilla-central/source/devtools/client/webconsole/new-webconsole.js#184)
* All references to experimentalOutputNode can be removed in the old webconsole.js (in fact any conditions with this.NEW_CONSOLE_OUTPUT_ENABLED can be be removed since in this case we are now loading a new document instead).
Flags: needinfo?(bgrinstead)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Try push:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=42e9f4cc28f57dedb72af25e729857fc25c58e02

I am attaching further patch with fixes for test failures. Now memory leaks are fixed too.
A lot less test failures and some of them might be unrelated.

Honza
Assignee

Comment 20

2 years ago
Since we don't want to deal with the variable view splitter in an html doc, I'm going to block this on Bug 1308566. Although I'm sure there's still more to do here besides waiting on that.
Depends on: 1308566
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
@Nicolas: I am seeing errors related to reps.css:

INFO - TEST-UNEXPECTED-FAIL | browser/base/content/test/static/browser_parsable_css.js | Got error message for resource://devtools/client/shared/components/reps/reps.css: Unknown property ‘user-select’.  Declaration dropped.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=e2968fb9094af8849db5871ecac01f96637700f5&selectedJob=111211090

What could be wrong here?

Honza
Flags: needinfo?(nchevobbe)
Iteration: 56.1 - Jun 26 → 56.2 - Jul 10
I don't really see why it fails now, the reps.css file wasn't changed since a long time (https://hg.mozilla.org/mozilla-central/log/tip/devtools/client/shared/components/reps/reps.css , in February).

It looks more like an error in the test that is failing for me. There's some previous bugs for intermittents in this test.
Flags: needinfo?(nchevobbe)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Iteration: 56.2 - Jul 10 → 56.3 - Jul 24
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment on attachment 8877095 [details]
Bug 1326937 - Provide HTML page when running new console frontend;

https://reviewboard.mozilla.org/r/148430/#review161582
Attachment #8877095 - Flags: review?(odvarko) → review+
Assignee

Comment 46

2 years ago
mozreview-review
Comment on attachment 8878041 [details]
Bug 1326937 - Fix CSS;

https://reviewboard.mozilla.org/r/149456/#review161622

::: devtools/client/webconsole/new-console-output/test/mochitest/browser_webconsole_input_focus.js:34
(Diff revision 5)
>    inputNode = hud.jsterm.inputNode;
>    ok(inputNode.getAttribute("focused"), "input node is focused, first");
>  
>    yield waitForBlurredInput(inputNode);
>  
> -  EventUtils.sendMouseEvent({type: "click"}, hud.outputNode);
> +  inputNode.focus();

Should this be instead clicking on the output node?  I believe the intent here is that when you click on the output node it focuses the jsterm.

Maybe `hud.outputNode.click()`?

::: devtools/client/webconsole/webconsole.js:556
(Diff revision 5)
>      this.jsterm = new JSTerm(this);
>      this.jsterm.init();
>  
>      let toolbox = gDevTools.getToolbox(this.owner.target);
>  
>      if (this.NEW_CONSOLE_OUTPUT_ENABLED) {

We should be able to delete this entire condition (and any other references to NEW_CONSOLE_OUTPUT_ENABLED in the file), since we are never running the new console output anymore in this file
Attachment #8878041 - Flags: review?(bgrinstead)
Assignee

Comment 47

2 years ago
mozreview-review
Comment on attachment 8880786 [details]
Bug 1326937 - Fix CSS;

https://reviewboard.mozilla.org/r/152156/#review161882

::: devtools/client/themes/webconsole.css:872
(Diff revision 4)
> +  overflow: auto;
> +  -moz-user-select: text;
> +  position: relative;
> +}
> +
> +:root,

Styles on jsterm, body, :root, and output-container will affect the old frontend as well and I'd prefer to not have to worry about breaking that while it's in the Browser Console.

Can you add a class to the root node in webconsole.xhtml and prefix any specific styles with that class?
Attachment #8880786 - Flags: review?(bgrinstead)
Assignee

Comment 48

2 years ago
mozreview-review
Comment on attachment 8882763 [details]
Bug 1326937 - Avoid an exception due to a missing sidebar;

https://reviewboard.mozilla.org/r/153870/#review161890

::: devtools/client/webconsole/jsterm.js:588
(Diff revision 3)
>     *        the variables view window after open, |false| otherwise.
>     * @return object
>     *         A promise object that is resolved when the variables view has
>     *         opened. The new variables view instance is given to the callbacks.
>     */
>    openVariablesView: function (options) {

Nit: does it work to bail out at the top of this function with something like: 

```
if (!this.hud.document.querySelector("#webconsole-sidebar") {
  return Promise.resolve(null);
}
```

instead of propogating the conditions into `onContainerReady`, `_createSidebar`, and `_addVariablesViewSidebarTab`?
Attachment #8882763 - Flags: review?(bgrinstead) → review+
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(In reply to Brian Grinstead [:bgrins] from comment #46)
> Comment on attachment 8878041 [details]
> Bug 1326937 - Fix test failures;
> > -  EventUtils.sendMouseEvent({type: "click"}, hud.outputNode);
> > +  inputNode.focus();
> 
> Should this be instead clicking on the output node?  I believe the intent
> here is that when you click on the output node it focuses the jsterm.
I found real bug and the test now passes without any modifications.

> >      if (this.NEW_CONSOLE_OUTPUT_ENABLED) {
> 
> We should be able to delete this entire condition (and any other references
> to NEW_CONSOLE_OUTPUT_ENABLED in the file), since we are never running the
> new console output anymore in this file
Done 

(In reply to Brian Grinstead [:bgrins] from comment #47)
> Comment on attachment 8880786 [details]
> Bug 1326937 - Fix CSS;
> ::: devtools/client/themes/webconsole.css:872
> (Diff revision 4)
> > +  overflow: auto;
> > +  -moz-user-select: text;
> > +  position: relative;
> > +}
> > +
> > +:root,
> 
> Styles on jsterm, body, :root, and output-container will affect the old
> frontend as well and I'd prefer to not have to worry about breaking that
> while it's in the Browser Console.
> 
> Can you add a class to the root node in webconsole.xhtml and prefix any
> specific styles with that class?
I used body for this purpose since it's only available in .xhtml, no?
Anyway, I replaced :root by html.


(In reply to Brian Grinstead [:bgrins] from comment #48)
> Comment on attachment 8882763 [details]
> Bug 1326937 - Avoid an exception due to a missing sidebar;
> Nit: does it work to bail out at the top of this function with something
> like: 
> 
> ```
> if (!this.hud.document.querySelector("#webconsole-sidebar") {
>   return Promise.resolve(null);
> }
> ```
> 
> instead of propogating the conditions into `onContainerReady`,
> `_createSidebar`, and `_addVariablesViewSidebarTab`?
Good point, done.

Thanks!
Honza
Assignee

Comment 56

2 years ago
mozreview-review
Comment on attachment 8878041 [details]
Bug 1326937 - Fix CSS;

https://reviewboard.mozilla.org/r/149456/#review162102

Nice. Can you fold this into the first patch before we land?
Attachment #8878041 - Flags: review?(bgrinstead) → review+
Assignee

Comment 57

2 years ago
mozreview-review
Comment on attachment 8877095 [details]
Bug 1326937 - Provide HTML page when running new console frontend;

https://reviewboard.mozilla.org/r/148430/#review162106

::: testing/talos/talos/tests/devtools/addon/content/damp.js:27
(Diff revision 8)
>    this._heapSnapshotFilePath = null;
>    // HeapSnapshot instance. Set by readHeapSnapshot, used by takeCensus.
>    this._snapshot = null;
>  
>    // Use the old console for now: https://bugzilla.mozilla.org/show_bug.cgi?id=1306780
> -  Services.prefs.setBoolPref("devtools.webconsole.new-frontend-enabled", false);
> +  // Services.prefs.setBoolPref("devtools.webconsole.new-frontend-enabled", false);

Please revert the change to this file
Assignee

Comment 58

2 years ago
mozreview-review
Comment on attachment 8882763 [details]
Bug 1326937 - Avoid an exception due to a missing sidebar;

https://reviewboard.mozilla.org/r/153870/#review162108

Can you fold this into the first patch (along with the test fixes)?
Assignee

Comment 59

2 years ago
mozreview-review
Comment on attachment 8880786 [details]
Bug 1326937 - Fix CSS;

https://reviewboard.mozilla.org/r/152156/#review162104

::: devtools/client/themes/webconsole.css:406
(Diff revision 5)
>    background-position: -60px -36px;
>  }
>  
>  /* JSTerm Styles */
> +
> +#jsterm-wrapper,

Please prefix all of these with `html` so that it doesn't affect the old frontend

::: devtools/client/themes/webconsole.css
(Diff revision 5)
>  
> -/*
> - * Open DOMNode in inspector button. Style need to be reset in the new
> - * console since its style is already defined in reps.css .
> - */
> -.webconsole-output-wrapper .open-inspector {

Why is this workaround not needed anymore?
Attachment #8880786 - Flags: review?(bgrinstead)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(In reply to Brian Grinstead [:bgrins] from comment #56)
> Comment on attachment 8878041 [details]
> Bug 1326937 - Fix CSS;
> Nice. Can you fold this into the first patch before we land?
Done

(In reply to Brian Grinstead [:bgrins] from comment #57)
> Comment on attachment 8877095 [details]
> Bug 1326937 - Provide HTML page when running new console frontend;
> > -  Services.prefs.setBoolPref("devtools.webconsole.new-frontend-enabled", false);
> > +  // Services.prefs.setBoolPref("devtools.webconsole.new-frontend-enabled", false);
> 
> Please revert the change to this file
Done

Shouldn't we enable damp tests for the new front end?

(In reply to Brian Grinstead [:bgrins] from comment #58)
> Comment on attachment 8882763 [details]
> Bug 1326937 - Avoid an exception due to a missing sidebar;
> Can you fold this into the first patch (along with the test fixes)?
Done

(In reply to Brian Grinstead [:bgrins] from comment #59)
> Comment on attachment 8880786 [details]
> Bug 1326937 - Fix CSS;
> > +
> > +#jsterm-wrapper,
> 
> Please prefix all of these with `html` so that it doesn't affect the old
> frontend
Done

> > -/*
> > - * Open DOMNode in inspector button. Style need to be reset in the new
> > - * console since its style is already defined in reps.css .
> > - */
> > -.webconsole-output-wrapper .open-inspector {
> 
> Why is this workaround not needed anymore?
This caused the inspector icon to be huge and covering most of the console content. But I am not seeing the problem anymore, so I reverted the change.

Honza
Assignee

Comment 64

2 years ago
(In reply to Jan Honza Odvarko [:Honza] from comment #63)
> (In reply to Brian Grinstead [:bgrins] from comment #57)
> > Comment on attachment 8877095 [details]
> > Bug 1326937 - Provide HTML page when running new console frontend;
> > > -  Services.prefs.setBoolPref("devtools.webconsole.new-frontend-enabled", false);
> > > +  // Services.prefs.setBoolPref("devtools.webconsole.new-frontend-enabled", false);
> > 
> > Please revert the change to this file
> Done
> 
> Shouldn't we enable damp tests for the new front end?

Yes, but let's wait until we flip the pref for non-nightly builds (or at least, let's do it in a different bug than this)
Assignee

Comment 65

2 years ago
mozreview-review
Comment on attachment 8877095 [details]
Bug 1326937 - Provide HTML page when running new console frontend;

https://reviewboard.mozilla.org/r/148430/#review162620

Thank you, this looks great!
Attachment #8877095 - Flags: review?(bgrinstead) → review+
Assignee

Comment 66

2 years ago
This should be ready to land, with a good try push
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
One failing test fixed:
devtools/client/webconsole/new-console-output/test/mochitest/browser_webconsole_console_dir.js

https://treeherder.mozilla.org/#/jobs?repo=try&revision=97c2fe9f5d25357510cb7864f8960502444c47e2

Honza

Comment 73

2 years ago
Pushed by jodvarko@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/227ff88b9848
Provide HTML page when running new console frontend; r=bgrins
https://hg.mozilla.org/integration/autoland/rev/57701c371c8d
Patch react-dom to avoid double load r=bgrins
https://hg.mozilla.org/integration/autoland/rev/d99559a7adc7
Fix CSS; r=bgrins
Backed out for timing out in browser_ext_devtools_inspectedWindow_eval_bindings.js:

https://hg.mozilla.org/integration/autoland/rev/99c1d1e02a53d0f1e44fdca3057c50c3a6e3fc18
https://hg.mozilla.org/integration/autoland/rev/8418d3ebb3a0d47686c61e62a68c0fb0da7b5c25
https://hg.mozilla.org/integration/autoland/rev/6a3d7b1a87e0243e2240fee60ca1f31795995d39

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=d99559a7adc7cc556575905fc6ddfe9b650ea6bb&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=retry&filter-resultStatus=usercancel&filter-resultStatus=runnable
Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=114927927&repo=autoland

[task 2017-07-17T18:02:43.340796Z] 18:02:43     INFO - TEST-PASS | browser/components/extensions/test/browser/browser_ext_devtools_inspectedWindow_eval_bindings.js | Got the expected eval result - {"evalResult":"BODY","errorResult":"undefined"} deepEqual {"evalResult":"BODY","errorResult":"undefined"} - 
[task 2017-07-17T18:02:43.345182Z] 18:02:43     INFO - Toolbox switched back to the styleeditor panel
[task 2017-07-17T18:02:43.349233Z] 18:02:43     INFO - Test inspectedWindow.eval inspect() binding called for a DOM element
[task 2017-07-17T18:02:43.351046Z] 18:02:43     INFO - Buffered messages logged at 18:02:01
[task 2017-07-17T18:02:43.356386Z] 18:02:43     INFO - Wait for the toolbox to switch to the inspector and the expected node has been selected
[task 2017-07-17T18:02:43.362235Z] 18:02:43     INFO - TEST-PASS | browser/components/extensions/test/browser/browser_ext_devtools_inspectedWindow_eval_bindings.js | The expected DOM node has been selected in the inspector - 
[task 2017-07-17T18:02:43.372608Z] 18:02:43     INFO - Toolbox has been switched to the inspector as expected
[task 2017-07-17T18:02:43.376195Z] 18:02:43     INFO - Test inspectedWindow.eval inspect() binding called for a JS object
[task 2017-07-17T18:02:43.378137Z] 18:02:43     INFO - Wait for the split console to be opened and the JS object inspected
[task 2017-07-17T18:02:43.383217Z] 18:02:43     INFO - Console message: [JavaScript Warning: "Unknown property ‘user-select’.  Declaration dropped." {file: "resource://devtools/client/shared/components/reps/reps.css" line: 212 column: 13 source: "  user-select: none;"}]
[task 2017-07-17T18:02:43.385301Z] 18:02:43     INFO - Buffered messages finished
[task 2017-07-17T18:02:43.391350Z] 18:02:43     INFO - TEST-UNEXPECTED-FAIL | browser/components/extensions/test/browser/browser_ext_devtools_inspectedWindow_eval_bindings.js | Test timed out - 
[task 2017-07-17T18:02:43.392912Z] 18:02:43     INFO - Not taking screenshot here: see the one that was previously logged
[task 2017-07-17T18:02:43.401262Z] 18:02:43     INFO - TEST-UNEXPECTED-FAIL | browser/components/extensions/test/browser/browser_ext_devtools_inspectedWindow_eval_bindings.js | Extension left running at test shutdown - 
[task 2017-07-17T18:02:43.402852Z] 18:02:43     INFO - Stack trace:
[task 2017-07-17T18:02:43.404390Z] 18:02:43     INFO -     chrome://mochikit/content/tests/SimpleTest/ExtensionTestUtils.js:ExtensionTestUtils.loadExtension/<:109
[task 2017-07-17T18:02:43.405936Z] 18:02:43     INFO -     chrome://mochikit/content/browser-test.js:Tester.prototype.nextTest<:390
[task 2017-07-17T18:02:43.407515Z] 18:02:43     INFO -     timeoutFn@chrome://mochikit/content/browser-test.js:858:9
[task 2017-07-17T18:02:43.409738Z] 18:02:43     INFO -     setTimeout handler*Tester_execTest@chrome://mochikit/content/browser-test.js:820:9
[task 2017-07-17T18:02:43.411277Z] 18:02:43     INFO -     Tester.prototype.nextTest</<@chrome://mochikit/content/browser-test.js:670:7
[task 2017-07-17T18:02:43.413370Z] 18:02:43     INFO -     SimpleTest.waitForFocus/waitForFocusInner/focusedOrLoaded/<@chrome://mochikit/content/tests/SimpleTest/SimpleTest.js:795:59
Flags: needinfo?(odvarko)
Assignee

Comment 75

2 years ago
Since we need to re-land anyway: I noticed that the first patch has me as the author (I think since we folded your work into the original WIP). Honza, can you update that to be you?
Assignee

Updated

2 years ago
Blocks: 1381636
Assignee

Updated

2 years ago
Blocks: 1381686
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(In reply to Brian Grinstead [:bgrins] from comment #75)
> Since we need to re-land anyway: I noticed that the first patch has me as
> the author (I think since we folded your work into the original WIP). Honza,
> can you update that to be you?
Sure, done.

Honza
Flags: needinfo?(odvarko)
Assignee

Comment 81

2 years ago
mozreview-review
Comment on attachment 8887458 [details]
Bug 1326937 - Fix persistent logs;

https://reviewboard.mozilla.org/r/158294/#review163546

::: browser/components/extensions/test/browser/browser_ext_devtools_inspectedWindow_eval_bindings.js
(Diff revision 1)
>      is(objectType, "object", "The inspected object has the expected type");
>      Assert.deepEqual(Object.keys(objectPreviewProperties), ["testkey"],
>                       "The inspected object has the expected preview properties");
>    })();
>  
> -  const inspectJSObjectPromise = extension.awaitMessage(`inspectedWindow-eval-result`);

I don't think removing this code is the right solution.  I tested locally and I'm not seeing the object in the console (I do see the split console get opened though).

The problem here I think is that 'inspect()' isn't working in the xhtml doc (due to lack of vview). We need to at least wait for Bug 1379570 (currently on autoland), and confirm this test works with that patch applied.
Attachment #8887458 - Flags: review?(bgrinstead) → review-
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Attachment #8888277 - Attachment is obsolete: true
Attachment #8888277 - Flags: review?(bgrinstead)
Attachment #8888278 - Attachment is obsolete: true
Attachment #8888279 - Attachment is obsolete: true
Attachment #8888279 - Flags: review?(bgrinstead)
Attachment #8888280 - Attachment is obsolete: true
Attachment #8888280 - Flags: review?(bgrinstead)
Attachment #8888281 - Attachment is obsolete: true
Attachment #8888281 - Flags: review?(lgreco)
Attachment #8888281 - Flags: review?(bgrinstead)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Assignee

Comment 92

2 years ago
mozreview-review
Comment on attachment 8887458 [details]
Bug 1326937 - Fix persistent logs;

https://reviewboard.mozilla.org/r/158294/#review164652

::: browser/components/extensions/test/browser/browser_ext_devtools_inspectedWindow_eval_bindings.js:140
(Diff revision 3)
>      });
>  
> -    const objectType = options.objectActor.type;
> -    const objectPreviewProperties = options.objectActor.preview.ownProperties;
> -    is(objectType, "object", "The inspected object has the expected type");
> -    Assert.deepEqual(Object.keys(objectPreviewProperties), ["testkey"],
> +    let objectInspectors = [...messageNode.querySelectorAll(".tree")];
> +    is(objectInspectors.length, 1, "There is the expected number of object inspectors");
> +
> +    const [objectInspector] = objectInspectors;

IMO all of the changes after this can be removed - we have devtools tests making sure inspect/OI work so here we can be satisfied once we know the OI exists

::: devtools/client/webconsole/jsterm.js:413
(Diff revision 3)
>      if (WebConsoleUtils.isActorGrip(result)) {
>        msg._objectActors.add(result.actor);
>      }
>    },
>  
>    inspectObjectActor: function (objectActor) {

Can you also revert the changes to executeResultCallback from https://hg.mozilla.org/mozilla-central/rev/4dd3191931e0#l1.2 so that we call inspectObjectActor there as well?
Attachment #8887458 - Flags: review?(bgrinstead) → review-
Depends on: 1382690
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(In reply to Brian Grinstead [:bgrins] from comment #92)
> Comment on attachment 8887458 [details]
> Bug 1326937 - Fix persitent logs;
> 
> https://reviewboard.mozilla.org/r/158294/#review164652
This patch will be handled separately in bug Bug 1382690 

Honza
Assignee

Comment 98

2 years ago
mozreview-review
Comment on attachment 8887458 [details]
Bug 1326937 - Fix persistent logs;

https://reviewboard.mozilla.org/r/158294/#review164662
Attachment #8887458 - Flags: review?(bgrinstead) → review+
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 107

2 years ago
Pushed by bgrinstead@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/2f2e14280fab
Provide HTML page when running new console frontend; r=bgrins
https://hg.mozilla.org/integration/autoland/rev/5612970bf698
Patch react-dom to avoid double load r=bgrins
https://hg.mozilla.org/integration/autoland/rev/499fa676cb6d
Fix CSS; r=bgrins
https://hg.mozilla.org/integration/autoland/rev/d941b592133f
Fix persistent logs; r=bgrins
Reproduced the initial issue on 53.0a1 (2017-01-01). I can confirm the bug is verified fixed on latest Nightly 56.0a1 (2017-07-24), using Windows 10 x64, Ubuntu 16.04 x64 and Mac OS X 10.11.6.
Status: RESOLVED → VERIFIED
Flags: qe-verify+
Depends on: 1385157
Depends on: 1334683
Depends on: 1388104

Updated

a year ago
Product: Firefox → DevTools
Flags: in-qa-testsuite+
You need to log in before you can comment on or make changes to this bug.