1417446 - Remove the MESSAGE_ADD action

Status: RESOLVED FIXED

(DevTools :: Console, enhancement, P1)

Description

[Nicolas Chevobbe [:nchevobbe]]

In Bug 1371721, we created a new action, MESSAGES_ADD [1] (note the plural form) to batch our messages addition into the store.
This means that we are no longer using the MESSAGE_ADD [2] (note the single form) action in our code, except in tests [3].

We should get rid of this action and only use messagesAdd in all the tests, which represent better what the console is doing.

Do be more specific, the following :

> messageAdd(packet)

should be tranformed to :

> messagesAdd([packet])

In some places we were dispatching multiple messageAdd sequentially. These could be merged into one messagesAdd.

When this patch is done, all the mocha tests need to be successful.
The tests can be run with the following command: 

> cd devtools/client/webconsole && npm test


The constant [4], as well as the reducer handling this action [5] should be removed as well.


[1] https://searchfox.org/mozilla-central/rev/a662f122c37704456457a526af90db4e3c0fd10e/devtools/client/webconsole/new-console-output/actions/messages.js#29-52
[2] https://searchfox.org/mozilla-central/rev/a662f122c37704456457a526af90db4e3c0fd10e/devtools/client/webconsole/new-console-output/actions/messages.js#54-71
[3] https://searchfox.org/mozilla-central/search?q=messageAdd(&case=true&regexp=false&path=
[4] https://searchfox.org/mozilla-central/rev/a662f122c37704456457a526af90db4e3c0fd10e/devtools/client/webconsole/new-console-output/constants.js#16
[5] https://searchfox.org/mozilla-central/rev/a662f122c37704456457a526af90db4e3c0fd10e/devtools/client/webconsole/new-console-output/reducers/messages.js#183-185

Comment 1

[Pradeep Gangwar [:pradeepgangwar]]

Can I work on this bug and try fixing it?

Comment 2

[Nicolas Chevobbe [:nchevobbe]]

Hello Pradeep, 

Of course you can start working on this :)
If you have any questions, don't hesitate to hit me here or on Slack https://devtools-html-slack.herokuapp.com/

Comment 3

[Nicolas Chevobbe [:nchevobbe]]

Pradeep, did you start working on this ?
I'm afraid I did remove the action in Bug 1368100.
But if you have already a patch ready for review, I'll review it and do the rebase on my patch.

Comment 4

[Pradeep Gangwar [:pradeepgangwar]]

Attachment: Bug-1417446.patch

I tried solving it and after running npm test, I get some error all of which are associated with length property. It says cannot read property length of undefined. I was not getting any idea of error so i decided to make a WIP patch and ask for feedback.

Comment 5

[Pradeep Gangwar [:pradeepgangwar]]

The file associated is https://searchfox.org/mozilla-central/source/devtools/client/webconsole/new-console-output/reducers/messages.js#159

Comment 6

[Pradeep Gangwar [:pradeepgangwar]]

Attachment: Bug-1417446.patch

Comment 7

[Nicolas Chevobbe [:nchevobbe]]

Comment on attachment 8929611 [details] [diff] [review]
Bug-1417446.patch

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

There are some errors in the patch that might explain the test failures that you see.
Also, I am sorry to say that all of this is mostly covered in a working patch for Bug 1368100. Do you mind if we close this bug ?

::: devtools/client/webconsole/new-console-output/actions/messages.js
@@ +51,4 @@
>    };
>  }
>  
> +function messagesAdd(packet, idGenerator = null) {

This function should be removed. messagesAdd already exists

@@ +142,4 @@
>  }
>  
>  module.exports = {
> +  messagesAdd,

we shouldn't export the same function twice

::: devtools/client/webconsole/new-console-output/reducers/messages.js
@@ +180,4 @@
>  
>        return limitTopLevelMessageCount(newState, logLimit);
>  
> +    case constants.MESSAGES_ADD:

this whole block should be removed, we already have a `case` for MESSAGES_ADD

::: devtools/client/webconsole/new-console-output/test/helpers.js
@@ +27,4 @@
>  
>    const idGenerator = new IdGenerator();
>    wrappedActions.messageAdd = (packet) => {
> +    return actions.messagesAdd(packet, idGenerator);

messagesAdd takes an array : actions.messagesAdd([packet])

@@ +41,4 @@
>  
>    // Add the messages from the input commands to the store.
>    input.forEach((cmd) => {
> +    store.dispatch(actions.messagesAdd(stubPackets.get(cmd)));

same here, messagesAdd takes an array

Comment 8

[Pradeep Gangwar [:pradeepgangwar]]

There is no problem closing this bug if this has been solved in Bug 1368100. :-)

Comment 9

[Nicolas Chevobbe [:nchevobbe]]

Thank you for your understanding :)

Comment 10

[Nicolas Chevobbe [:nchevobbe]]

Attachment: Bug 1417446 - Remove MESSAGE_ADD action;.

This action is not used anywhere since we batch messages and thus use MESSAGES_ADD action.
The tests that were using it now use messagesAdd.

Review commit: https://reviewboard.mozilla.org/r/213860/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/213860/

Comment 11

[Brian Grinstead [:bgrins]]

Comment on attachment 8943519 [details]
Bug 1417446 - Remove MESSAGE_ADD action;.

https://reviewboard.mozilla.org/r/213860/#review219738

Thanks!

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/fccc4ee24bd6
Remove MESSAGE_ADD action;r=bgrins.

Comment 13

[Andrei Ciure[:aciure]]

https://hg.mozilla.org/mozilla-central/rev/fccc4ee24bd6

Comment 14

[Alexandre Poirot [:ochameau]]

Were you expecting a perf improvement here?
We got a 9% speedup on the mutations test while this bug and bug 1431334 landed:
http://firefox-dev.tools/performance-dashboard/perfherder/?test=inspector.mutations&days=14&filterstddev=true

Comment 15

[Alexandre Poirot [:ochameau]]

Sorry, I thought I was on the console DAMP page. But this is an improvement related to inspector.
Nevermind my previous comment!