New console frontend: Add support for network event message #262

rickychien · 2016-09-08T09:20:36Z

Fixes #195

Support for network event message

linclark · 2016-09-08T16:28:32Z

devtools/client/themes/webconsole.css

@@ -140,7 +140,6 @@ a {
 }

 .message-body > * {
-  white-space: pre-wrap;


We want to be careful about changes to the CSS in this file. This gets applied to the old console as well.

You should be able to add overrides to the NEW CONSOLE OUTPUT section, and scope them using one of the new-console specific classes for now.

I'll move this part into NEW CONSOLE OUTPUT section :)

BTW, I don't know why white-space: pre-wrap; doesn't seem to take effect to render a space after a method. Thus, I used this workaround to produce a space for new UI and it seems to work great.

linclark · 2016-09-08T16:35:59Z

I just downloaded this and tested it out. It looks really good!

I'm going to do another pass through the code tomorrow morning, so I will likely have more specific comments. But I think this is basically doing everything right.

linclark · 2016-09-08T16:37:48Z

@rickychien One thing that we will need for this is tests. You will probably need to add another stub generator in order to get the necessary stubs. That can be challenging, so let me know if you run into any problems with that.

linclark · 2016-09-09T11:05:57Z

devtools/client/webconsole/new-console-output/components/message-types/network-event-message.js

+  let Xhr = l10n.getStr("webConsoleXhrIndicator");
+
+  function onUrlClick(evt) {
+    evt.preventDefault();


This preventDefault is required because we're using the a tag for the URLs, right? We can probably just change the a to a span instead. In the current console, it doesn't seem like you can click on the link to navigate to the URL anyway, so there's no reason that I can see for it being an a tag. @bgrins, what do you think?

It's OK to go away from an <a> tag if we do a <span> with tabindex of 0: https://bugzilla.mozilla.org/show_bug.cgi?id=987373#c39. If it will require re-styling things we might consider waiting, but I'd be fine with making the change now

Can we keep a tag here but remove its href? It seems to work for me after removing evt.preventDefault() and also keep a's underline and hover styling as it is.

~~@bgrins the thing is, I don't think that we actually do want a tabindex if clicking on it does nothing... do we? In the current console, clicking on the link doesn't take you there, AFAICT.~~

@rickychien I don't think the styling matters too much. I believe the only styling that we might want is the underline, but I'm fine with leaving that out for now. If clicking on the link doesn't do anything, then a hover state is confusing anyway.

UPDATE: I realized I didn't have network panel turned on when I was testing this in old console.

rickychien · 2016-09-13T09:15:14Z

@linclark
Do you think it makes sense to simulate("click") to test for a url clicking? Since behavior of url clicking is pretty simple and I'm not sure what things we're able to ensure for this.

linclark · 2016-09-13T18:26:07Z

devtools/client/webconsole/new-console-output/test/fixtures/stubs/networkEvent.js

+	"type": "log"
+}));
+
+stubPreparedMessages.set("undefined", new NetworkEventMessage({


It looks like it's generating a stub with the key undefined here. This probably means the event listener is getting two packets but only has one key.

linclark · 2016-09-13T18:40:59Z

@rickychien It's so awesome to see the test in here! Thanks for taking care of all that.

A few minor issues, but once those are resolved I think this is ready. Once it's in, we can start on #196... would you be interested in taking on that issue?

linclark · 2016-09-13T18:43:19Z

Do you think it makes sense to simulate("click") to test for a url clicking? Since behavior of url clicking is pretty simple and I'm not sure what things we're able to ensure for this.

You're right, we probably can't ensure much for this in the unit tests. I'll create an issue to make sure that we have an end-to-end test for it.

rickychien · 2016-09-14T07:26:25Z

devtools/client/webconsole/new-console-output/components/message-types/network-event-message.js

+      dom.span({ className: "method" }, method),
+      isXHR ? dom.span({ className: "xhr" }, xhr) : null,
+      dom.a({ className: "url", title: url, onClick: onUrlClick },
+        getURLDisplayString(url)),


@linclark
Could you check the display url on webconsole? I followed you suggestion to use this API from reps and url string is limited by 50 characters. It looks weird to me.

Ah, yeah. I'd say just keep it like you had it then.

rickychien · 2016-09-15T04:01:10Z

Github's new pull request reviews looks pretty cool!

@linclark Last issues has been addressed and I can't wait to see your PR approval.

linclark

Unfortunately there are still a few outstanding changes from the previous reviews. Once those are cleared up, this will be ready.

linclark · 2016-09-15T14:48:15Z

devtools/client/webconsole/new-console-output/components/console-output.js

@@ -13,15 +13,20 @@ const ReactDOM = require("devtools/client/shared/vendor/react-dom");
 const { connect } = require("devtools/client/shared/vendor/react-redux");

 const { getAllMessages, getAllMessagesUiById } = require("devtools/client/webconsole/new-console-output/selectors/messages");
+const { getAllUi } = require("devtools/client/webconsole/new-console-output/selectors/ui");


I believe this should have been removed when the currentLocation code was removed. This component is no longer using anything from the UI selector.

When we do implement the location stuff, the UI reducer/selector is probably not the best place for it. Instead, I'm thinking we'll probably want to create a part of the state tree that is just in charge of backend state.

linclark · 2016-09-15T14:48:29Z

devtools/client/webconsole/new-console-output/components/console-output.js

    sourceMapService: PropTypes.object,
    onViewSourceInDebugger: PropTypes.func.isRequired,
+    openNetworkPanel: PropTypes.func.isRequired,
+    openLink: PropTypes.func.isRequired,
+    ui: PropTypes.object.isRequired,


Same as above.

linclark · 2016-09-15T14:50:18Z

devtools/client/webconsole/new-console-output/components/console-output.js

+      onViewSourceInDebugger,
+      openNetworkPanel,
+      openLink,
+      ui,


Same as above.

linclark · 2016-09-15T14:50:26Z

devtools/client/webconsole/new-console-output/components/console-output.js

@@ -77,7 +87,8 @@ function isScrolledToBottom(outputNode, scrollNode) {
 function mapStateToProps(state) {
  return {
    messages: getAllMessages(state),
-    messagesUi: getAllMessagesUiById(state)
+    messagesUi: getAllMessagesUiById(state),
+    ui: getAllUi(state),


Same as above.

linclark · 2016-09-15T14:52:03Z

devtools/client/webconsole/new-console-output/new-console-output-wrapper.js

@@ -8,15 +8,16 @@ const React = require("devtools/client/shared/vendor/react");
 const ReactDOM = require("devtools/client/shared/vendor/react-dom");
 const { Provider } = require("devtools/client/shared/vendor/react-redux");

-const actions = require("devtools/client/webconsole/new-console-output/actions/messages");
+const messageActions = require("devtools/client/webconsole/new-console-output/actions/messages");
+const uiActions = require("devtools/client/webconsole/new-console-output/actions/ui");


Same as above (regarding UI selectors/reducers).

linclark · 2016-09-15T14:52:27Z

devtools/client/webconsole/new-console-output/new-console-output-wrapper.js

@@ -8,15 +8,16 @@ const React = require("devtools/client/shared/vendor/react");
 const ReactDOM = require("devtools/client/shared/vendor/react-dom");
 const { Provider } = require("devtools/client/shared/vendor/react-redux");

-const actions = require("devtools/client/webconsole/new-console-output/actions/messages");
+const messageActions = require("devtools/client/webconsole/new-console-output/actions/messages");


Message actions can be renamed to actions again.

linclark · 2016-09-15T14:54:41Z

devtools/client/webconsole/new-console-output/utils/messages.js

+        isXHR: networkEvent.isXHR,
+        request: networkEvent.request,
+        response: networkEvent.response,
+        source: MESSAGE_SOURCE.NETWORK,


When I said these could be hardcoded, I meant you could move them to the definition of NetworkEventMessage in types.js.

linclark · 2016-09-15T14:59:35Z

@rickychien agreed, the new review system is awesome!

MozReview-Commit-ID: 6RcVt7BZFr2

rickychien · 2016-09-15T15:56:33Z

Issues addressed again. @linclark thanks for your carefully review.

linclark · 2016-09-15T16:07:00Z

This looks great. Thanks so much for taking the time on it.

linclark reviewed Sep 8, 2016
View reviewed changes

linclark reviewed Sep 9, 2016
View reviewed changes

linclark reviewed Sep 13, 2016
View reviewed changes

rickychien reviewed Sep 14, 2016
View reviewed changes

linclark suggested changes Sep 15, 2016

View reviewed changes

Ricky Chien added 2 commits September 15, 2016 23:54

New console frontend: Add support for network event message

1ffc891

New console frontend: Add tests for network event message

a1765ae

MozReview-Commit-ID: 6RcVt7BZFr2

linclark approved these changes Sep 15, 2016

View reviewed changes

linclark merged commit 9000dbc into firefox-devtools:console-frontend Sep 15, 2016

rickychien deleted the network-event-messages branch September 16, 2016 02:48

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

New console frontend: Add support for network event message #262

New console frontend: Add support for network event message #262

rickychien commented Sep 8, 2016 •

edited

linclark Sep 8, 2016

rickychien Sep 9, 2016 •

edited

linclark commented Sep 8, 2016

linclark commented Sep 8, 2016

linclark Sep 9, 2016

bgrins Sep 9, 2016 •

edited

rickychien Sep 12, 2016 •

edited

linclark Sep 12, 2016 •

edited

rickychien commented Sep 13, 2016

linclark Sep 13, 2016

linclark commented Sep 13, 2016

linclark commented Sep 13, 2016

rickychien Sep 14, 2016

linclark Sep 14, 2016

rickychien commented Sep 15, 2016 •

edited

linclark left a comment

linclark Sep 15, 2016 •

edited

linclark Sep 15, 2016

linclark Sep 15, 2016

linclark Sep 15, 2016

linclark Sep 15, 2016 •

edited

linclark Sep 15, 2016

linclark Sep 15, 2016

linclark commented Sep 15, 2016

rickychien commented Sep 15, 2016

linclark commented Sep 15, 2016

New console frontend: Add support for network event message #262

New console frontend: Add support for network event message #262

Conversation

rickychien commented Sep 8, 2016 • edited

Support for network event message

Choose a reason for hiding this comment

rickychien Sep 9, 2016 • edited

Choose a reason for hiding this comment

linclark commented Sep 8, 2016

linclark commented Sep 8, 2016

Choose a reason for hiding this comment

bgrins Sep 9, 2016 • edited

Choose a reason for hiding this comment

rickychien Sep 12, 2016 • edited

Choose a reason for hiding this comment

linclark Sep 12, 2016 • edited

Choose a reason for hiding this comment

rickychien commented Sep 13, 2016

Choose a reason for hiding this comment

linclark commented Sep 13, 2016

linclark commented Sep 13, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rickychien commented Sep 15, 2016 • edited

linclark left a comment

Choose a reason for hiding this comment

linclark Sep 15, 2016 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

linclark Sep 15, 2016 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

linclark commented Sep 15, 2016

rickychien commented Sep 15, 2016

linclark commented Sep 15, 2016

rickychien commented Sep 8, 2016 •

edited

rickychien Sep 9, 2016 •

edited

bgrins Sep 9, 2016 •

edited

rickychien Sep 12, 2016 •

edited

linclark Sep 12, 2016 •

edited

rickychien commented Sep 15, 2016 •

edited

linclark Sep 15, 2016 •

edited

linclark Sep 15, 2016 •

edited