1421213 - Clicking on the request status code should open the corresponding MDN page

Status: RESOLVED FIXED

(DevTools :: Console, enhancement)

Description

[Nicolas Chevobbe [:nchevobbe]]

In Bug 1417805, we added a dedicated, background-colored element for the status code.
It would be useful if this element open the MDN page about this status code.

We already have some similar mechanism in Netmonitor [1] (in the Header Panel) which we could borrow from.


[1] https://searchfox.org/mozilla-central/rev/7a8c667bdd2a4a32746c9862356e199627c0896d/devtools/client/netmonitor/src/components/HeadersPanel.js#193,216-220

Comment 1

[Abhinav Koppula]

Attachment: bz-1421213.diff

Hi Nicolas,
I have created a patch for this issue. I couldn't create a moz-review request since Bug 1417805 hasn't landed on mc I think but I have created my patch on top of the patch of Bug 1417805.
I hope its ok.

Comment 2

[Nicolas Chevobbe [:nchevobbe]]

Hello Abhinav,
Bug 1417805 landed in mozilla-central, so can you update, rebase your patch on top of it and push to mozreview ? Thanks !

Comment 3

[Nicolas Chevobbe [:nchevobbe]]

Comment on attachment 8932575 [details] [diff] [review]
bz-1421213.diff

The code looks good, thanks Abhinav.
I only have a small comment for the onStatusCodeClick, we should use the existing function in https://searchfox.org/mozilla-central/rev/9f3bd430c2b132c86c46126a0548661de876799a/devtools/client/webconsole/new-console-output/new-console-output-wrapper.js#91-93 (and make it better if we need to handle different type of button click)

Comment 4

[Abhinav Koppula]

Attachment: Bug 1421213 - Clicking on the request status code should open the corresponding MDN page.

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

Comment 5

[Abhinav Koppula]

Hi Nicolas,
I have created a moz-review request and have addressed the nit.

Comment 6

[Nicolas Chevobbe [:nchevobbe]]

Comment on attachment 8932992 [details]
Bug 1421213 - Clicking on the request status code should open the corresponding MDN page.

https://reviewboard.mozilla.org/r/203990/#review209812

Hello Abhinav, thanks again for working on this, you're extremly helpful to us !
I think there is an issue with this patch, I can't get it to work at all. Was it working on your machine ?

Also, it would be nice to have a mochitest here to make sure we do navigate to the expected mdn page.

::: devtools/client/webconsole/new-console-output/components/message-types/NetworkEventMessage.js:90
(Diff revision 1)
> +  const onStatusCodeClick = (e, url) => {
> +    e.stopPropagation();
> +    e.preventDefault();
> +    serviceContainer.openLink(url);
> +  };

I think we can inline it in onClick: directly

Comment 7

[Sagar Bharadwaj]

Hello, 
I would like to work on this.
This is my first time contributing to Mozilla. Also my first time using mercurial.
I made the required changes in devtools module.
However, the changes are not reflected in the build when I run './mach run'
Should I rebuild the whole project again? I tried './mach build devtools/*'. That didn't help me.

Comment 8

[Nicolas Chevobbe [:nchevobbe]]

Hello sagar, Sorry, i did not updated the bug but it is already assigned to abhinav, and he already submitted a patch
You can have a look at http://bugs.firefox-dev.tools/?easy&tool=all to see available easy bug to start constributing. Also, you might want to have a look at http://docs.firefox-dev.tools/getting-started/ to setup your machine

Comment 9

[Sagar Bharadwaj]

Alright, no problem.
Nevertheless, which directories should I rebuild to reflect my changes in the devtools source code when I 'mach run'?

Comment 10

[Abhinav Koppula]

Comment on attachment 8932992 [details]
Bug 1421213 - Clicking on the request status code should open the corresponding MDN page.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/203990/diff/1-2/

Comment 11

[Abhinav Koppula]

Comment on attachment 8932992 [details]
Bug 1421213 - Clicking on the request status code should open the corresponding MDN page.

https://reviewboard.mozilla.org/r/203990/#review209812

Hi Nicolas,
Have updated my PR with the following points after our discussion on slack:
- addressed review comment of inlining in `onclick` directly
- title of `Learn More` added
- enhanced the behavior to make sure middle click on status-code works correctly(open the tab without making it active)
- added test for left click, middle click and right click on status-code, by enhancing head.js -> simulateLinkClick

Comment 12

[Nicolas Chevobbe [:nchevobbe]]

(In reply to Sagar Bharadwaj from comment #9)
> Alright, no problem.
> Nevertheless, which directories should I rebuild to reflect my changes in
> the devtools source code when I 'mach run'?

You need to do `./mach build faster` from your repo root.

Comment 13

[Nicolas Chevobbe [:nchevobbe]]

Comment on attachment 8932992 [details]
Bug 1421213 - Clicking on the request status code should open the corresponding MDN page.

https://reviewboard.mozilla.org/r/203990/#review211778

Thanks abhinav, we're almost done here.
Still r- because we need to handle Cmd/Ctrl click, and i'd like the test to be a bit cleaner for dispatching the click event.

::: devtools/client/webconsole/hudservice.js:425
(Diff revision 2)
>     * @param string aLink
>     *        The URL you want to open in a new tab.
>     */
> -  openLink: function WC_openLink(aLink)
> +  openLink: function WC_openLink(aLink, e)
>    {
> +    if (e != null && e.button === 1) {

does it handle Cmd/Ctrl + click ?

::: devtools/client/webconsole/new-console-output/components/message-types/NetworkEventMessage.js:86
(Diff revision 2)
> +      className: "status-code",
> +      "data-code": status,
> +      title: LEARN_MORE,
> +      onClick: (e) => {
> +        e.stopPropagation();
> +        e.preventDefault();

why do we need to prevent default ?

::: devtools/client/webconsole/new-console-output/test/mochitest/browser_webconsole_network_messages_expand.js:46
(Diff revision 2)
> +  ok(statusCodeNode.title, l10n.getStr("webConsoleMoreInfoLabel"));
> +  let values = [
> +    { middleMouse: true, where: "tabshifted" },
> +    { middleMouse: false, where: "tab" }
> +  ];
> +
> +  for (let i = 0; i < values.length; i++) {
> +    let { link, tab } = await simulateLinkClick(statusCodeNode, values[i].middleMouse);
> +    is(link, LEARN_MORE_URI, `Clicking the provided link opens ${link}`);
> +    is(tab, values[i].where, `Link opened in correct tab`);
> +  }
> +
> +  info("Check if context menu is opened on right clicking the status-code");
> +  await openContextMenu(hud, statusCodeNode);
> +

can we create a new test for this ? this should be only about expanding a network call here. The new test should only be about clicking on the status code

::: devtools/client/webconsole/new-console-output/test/mochitest/head.js:26
(Diff revision 2)
>  var WCUL10n = require("devtools/client/webconsole/webconsole-l10n");
>  const DOCS_GA_PARAMS = "?utm_source=mozilla" +
>                         "&utm_medium=firefox-console-errors" +
>                         "&utm_campaign=default";
> +const STATUS_CODES_GA_PARAMS = "?utm_source=mozilla" +
> +                               "&utm_medium=devtools-netmonitor" +

oh, i didn't noticerd this parameter before, that's interesting.
In the case of the console, it should say devtools-webconsole I guess, which means we need to modify the  getHTTPStatusCodeURL function to be able to pass a panel id or something

::: devtools/client/webconsole/new-console-output/test/mochitest/head.js:426
(Diff revision 2)
>   * @param ElementNode element
>   *        The <a> element we want to simulate click on.
>   * @returns Promise
> - *          A Promise that resolved when the link clik simulation occured.
> + *          A Promise that resolved when the link click simulation occured.
>   */
> -function simulateLinkClick(element) {
> +function simulateLinkClick(element, middleMouseButton = false) {

we need to handle Cmd/Ctrl click as well, so I think we should replace the new argument with something like clickEventProps, which would accept an object. Then if the argument is falsy, call .click(), otherwise, use dispatch event with the clickEventProps

::: devtools/client/webconsole/new-console-output/test/mochitest/head.js:437
(Diff revision 2)
> +      element.dispatchEvent(getMouseClickEvent(element, "click",
> +        { ctrlKey: false, altKey: false, shiftKey: false, metaKey: false, button: 1 }));

so here, hopefully we can have something like
element.dispatchEvent(element, "click", getMouseEvent(clickEventProps))

::: devtools/client/webconsole/new-console-output/test/mochitest/head.js:446
(Diff revision 2)
> -    element.click();
> +      element.click();
> +    }
>    });
>  }
> +
> +function getMouseClickEvent(element, type, { ctrlKey, altKey, shiftKey, metaKey, button}) {

i don't think element is used in the function
we should have getMouseEvent(type, overrides)

::: devtools/client/webconsole/new-console-output/test/mochitest/head.js:449
(Diff revision 2)
> +    bubbles: true,
> +    cancelable: (type != "mousemove"),
> +    view: window,
> +    detail: 0,
> +    screenX: 0,
> +    screenY: 0,
> +    clientX: 0,
> +    clientY: 0,
> +    ctrlKey: ctrlKey,
> +    altKey: altKey,
> +    shiftKey: shiftKey,
> +    metaKey: metaKey,
> +    button: button,

do we really need all of this ?
Looking at the example from https://developer.mozilla.org/en-US/docs/Web/API/MouseEvent , i think we can simplify this a lot more. Ideally we would create the MouseEvent with the overrides object only

Comment 14

[Abhinav Koppula]

Comment on attachment 8932992 [details]
Bug 1421213 - Clicking on the request status code should open the corresponding MDN page.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/203990/diff/2-3/

Comment 15

[Abhinav Koppula]

Comment on attachment 8932992 [details]
Bug 1421213 - Clicking on the request status code should open the corresponding MDN page.

https://reviewboard.mozilla.org/r/203990/#review211778

> why do we need to prevent default ?

No specific reason why I added prevent default, just that I see preventDefault and stopPropagation being used together. Let me know if I should remove it.

Comment 16

[Abhinav Koppula]

Comment on attachment 8932992 [details]
Bug 1421213 - Clicking on the request status code should open the corresponding MDN page.

https://reviewboard.mozilla.org/r/203990/#review211778

Hi Nicolas,
I have updated my patch and have also added the Ctrl click functionality and have cleaned up the test.

Comment 17

[Nicolas Chevobbe [:nchevobbe]]

Comment on attachment 8932992 [details]
Bug 1421213 - Clicking on the request status code should open the corresponding MDN page.

https://reviewboard.mozilla.org/r/203990/#review214314

Thanks abhinav, this is looking good.
r- because we may not want to open on right click

::: devtools/client/netmonitor/src/components/MdnLink.js:39
(Diff revision 3)
>    e.stopPropagation();
>    e.preventDefault();
>  
>    let win = Services.wm.getMostRecentWindow(gDevTools.chromeWindowType);
> -  if (e.button === 1) {
> +  let { button, ctrlKey } = e;
> +  if (button === 1 || ctrlKey) {

i think we should have `button === 1 || (button === 0 && ctrlKey)` , since we don't want to navigate on right click for example

::: devtools/client/netmonitor/src/utils/mdn-utils.js:144
(Diff revision 3)
> -const GA_PARAMS =
> -  "?utm_source=mozilla&utm_medium=devtools-netmonitor&utm_campaign=default";
> +const GA_PARAMS = (panelId = "netmonitor") => {
> +  return `?utm_source=mozilla&utm_medium=devtools-${panelId}&utm_campaign=default`;
> +};

nice

::: devtools/client/netmonitor/src/utils/mdn-utils.js:144
(Diff revision 3)
>      "505",
>      "511"
>  ];
>  
>  const MDN_URL = "https://developer.mozilla.org/docs/";
> -const GA_PARAMS =
> +const GA_PARAMS = (panelId = "netmonitor") => {

we could rename it getGAParams maybe ?

::: devtools/client/webconsole/hudservice.js:425
(Diff revision 3)
>     * @param string aLink
>     *        The URL you want to open in a new tab.
>     */
> -  openLink: function WC_openLink(aLink)
> +  openLink: function WC_openLink(aLink, e)
>    {
> +    if (e != null && (e.button === 1 || e.ctrlKey)) {

same as previously, we only want to open in background on middle click or left-click + ctrl

::: devtools/client/webconsole/new-console-output/test/mochitest/browser_webconsole_network_messages_status_code.js:15
(Diff revision 3)
> +Services.prefs.setBoolPref(NET_PREF, false);
> +Services.prefs.setBoolPref(XHR_PREF, true);
> +registerCleanupFunction(() => {
> +  Services.prefs.clearUserPref(NET_PREF);
> +  Services.prefs.clearUserPref(XHR_PREF);
> +});

instead of this block, i think you can do : 

pushPref(NET_PREF, true);
pushPref(XHR_PREF, true);

since it does the same thing

::: devtools/client/webconsole/new-console-output/test/mochitest/browser_webconsole_network_messages_status_code.js:55
(Diff revision 3)
> +  let ctrlKeyMouseEvent = new MouseEvent("click", {
> +    bubbles: true,
> +    ctrlKey: true,
> +    view: window
> +  });
> +  let values = [

let's call this testCases so it's more obvious what it does

::: devtools/client/webconsole/new-console-output/test/mochitest/browser_webconsole_network_messages_status_code.js:55
(Diff revision 3)
> +  let values = [
> +    { clickEvent: middleMouseEvent, where: "tabshifted" },
> +    { clickEvent: null, where: "tab" },
> +    { clickEvent: ctrlKeyMouseEvent, where: "tabshifted" },
> +  ];

we should also test that right click does NOT open the page

::: devtools/client/webconsole/new-console-output/test/mochitest/browser_webconsole_network_messages_status_code.js:71
(Diff revision 3)
> +
> +  info("Check if context menu is opened on right clicking the status-code");
> +  await openContextMenu(hud, statusCodeNode);
> +});
> +
> +async function waitForRequestUpdates(toolbox) {

we don't use await in this test, so we can remove the async keyword, and we'll return the promise, which is fine

::: devtools/client/webconsole/new-console-output/test/mochitest/browser_webconsole_network_messages_status_code.js:73
(Diff revision 3)
> +  return new Promise(resolve => {
> +    ui.jsterm.hud.on("network-message-updated", () => {
> +      info("network-message-updated received");
> +      resolve();
> +    });
> +  });

i think we can do : 

return ui.jsterm.hud.once("network-message-updated")


once does return a promise that resolves on the event call.

::: devtools/client/webconsole/new-console-output/test/mochitest/head.js:428
(Diff revision 3)
>    return new Promise((resolve) => {
>      // Override openUILinkIn to prevent navigating.
>      let oldOpenUILinkIn = window.openUILinkIn;
> -    window.openUILinkIn = function (link) {
> +    window.openUILinkIn = function (link, where) {
>        window.openUILinkIn = oldOpenUILinkIn;
> -      resolve(link);
> +      resolve({link: link, tab: where});

why not call `tab` `where` instead ?

Comment 18

[Abhinav Koppula]

Comment on attachment 8932992 [details]
Bug 1421213 - Clicking on the request status code should open the corresponding MDN page.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/203990/diff/3-4/

Comment 19

[Abhinav Koppula]

Comment on attachment 8932992 [details]
Bug 1421213 - Clicking on the request status code should open the corresponding MDN page.

https://reviewboard.mozilla.org/r/203990/#review214314

Hi Nicolas,
Thanks for the review. I have added the test-case for right-click and right-click + ctrl key as well

Comment 20

[Nicolas Chevobbe [:nchevobbe]]

Comment on attachment 8932992 [details]
Bug 1421213 - Clicking on the request status code should open the corresponding MDN page.

https://reviewboard.mozilla.org/r/203990/#review214514

Thanks Abhinav, this is almost complete !
One thing I forgot to tell you, in OSX, we should open in background on Command + click, not Ctrl + click, to match Firefox behavior.
I tested it manually and it does not work for me.
Could you have a look ? I think there are places where we check for the OS (e.g. https://searchfox.org/mozilla-central/rev/ff462a7f6b4dd3c3fdc53c9bc0b328f42a5b3d2b/devtools/client/webconsole/new-console-output/test/mochitest/browser_webconsole_closing_after_completion.js#34)

::: devtools/client/webconsole/new-console-output/test/mochitest/head.js:433
(Diff revisions 3 - 4)
> +    new Promise((resolve) => {
> -    // Override openUILinkIn to prevent navigating.
> +      // Override openUILinkIn to prevent navigating.
> -    let oldOpenUILinkIn = window.openUILinkIn;
> +      let oldOpenUILinkIn = window.openUILinkIn;
> -    window.openUILinkIn = function (link, where) {
> +      window.openUILinkIn = function (link, where) {
> -      window.openUILinkIn = oldOpenUILinkIn;
> +        window.openUILinkIn = oldOpenUILinkIn;
> -      resolve({link: link, tab: where});
> +        resolve({link: link, where: where});

we can simplify `where: where` to `where`

Comment 21

[Abhinav Koppula]

Comment on attachment 8932992 [details]
Bug 1421213 - Clicking on the request status code should open the corresponding MDN page.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/203990/diff/4-5/

Comment 22

[Abhinav Koppula]

Hi Nicolas,
I have updated the patch to make it work for OSX as well and have updated the tests as well.

For OSX i triggered a try run - https://treeherder.mozilla.org/#/jobs?repo=try&revision=8a4c44e6fc2831123204e1ec615a8b316e9bcd96

Are the failures related to my changes?

Comment 23

[Nicolas Chevobbe [:nchevobbe]]

Comment on attachment 8932992 [details]
Bug 1421213 - Clicking on the request status code should open the corresponding MDN page.

https://reviewboard.mozilla.org/r/203990/#review216116

Thanks Abhinav, I tested it on OSX and it works very well.
I have a few nits to make things more readable, and some questions in tests, but this should be the last review round :)

::: devtools/client/netmonitor/src/utils/mdn-utils.js:144
(Diff revision 5)
> -const GA_PARAMS =
> -  "?utm_source=mozilla&utm_medium=devtools-netmonitor&utm_campaign=default";
> +const getGAParams = (panelId = "netmonitor") => {
> +  return `?utm_source=mozilla&utm_medium=devtools-${panelId}&utm_campaign=default`;
> +};

I did not think of this earlier, but we could use URLSearchParams here: 

const getGAParams = (panelId = "netmonitor") => {
  return `?${new URLSearchParams({
    utm_source: "mozilla",
    utm_medium: "devtools-" + panelId,
    utm_campaign: "default"
  }).toString()}`;
};

It makes things a bit cleaner and maintanable

::: devtools/client/webconsole/new-console-output/test/mochitest/browser_webconsole_allow_mixedcontent_securityerrors.js:46
(Diff revision 5)
>    ok(true, "Mixed display content warning message is visible");
>  
>    const mixedActiveContentMessage = await onMixedActiveContent;
>    ok(true, "Mixed active content warning message is visible");
>  
> +  const checkLink = ({ link, where }, { expectedLink, expectedTab }) => {

I think we could only pass a single object to this function.

::: devtools/client/webconsole/new-console-output/test/mochitest/browser_webconsole_allow_mixedcontent_securityerrors.js:53
(Diff revision 5)
> -  const url = await simulateLinkClick(learnMoreLink);
> -  is(url, LEARN_MORE_URI, `Clicking the provided link opens ${url}`);
> +  checkLink(await simulateLinkClick(learnMoreLink),
> +    { expectedLink: LEARN_MORE_URI, expectedTab: "tab" });

this is terse for sure, but a bit hard to read, can we make this in 2 steps : 

```js
let linkSimulation = await simulateLinkClick(learnMoreLink);
checkLink({
  ...linkSimulation,
  expectedLink: LEARN_MORE_URI, 
  expectedTab: "tab",
});
```

::: devtools/client/webconsole/new-console-output/test/mochitest/browser_webconsole_allow_mixedcontent_securityerrors.js:62
(Diff revision 5)
> +  checkLink(await simulateLinkClick(learnMoreLink, ctrlOrCmdKeyMouseEvent),
> +    { expectedLink: LEARN_MORE_URI, expectedTab: "tabshifted" });

same here, split it into 2 steps

::: devtools/client/webconsole/new-console-output/test/mochitest/browser_webconsole_network_messages_status_code.js:26
(Diff revision 5)
> +  await ContentTask.spawn(gBrowser.selectedBrowser, null, function () {
> +    content.wrappedJSObject.testXhrPost();
> +  });
> +
> +  info("XHR executed");
> +
> +  await waitForRequestUpdates(toolbox);

maybe we should wait for the update before executing testXhrPost ?

Here we may have a race condition if the requestUpdates are done before we hit l.32

::: devtools/client/webconsole/new-console-output/test/mochitest/browser_webconsole_network_messages_status_code.js:43
(Diff revision 5)
> +  let middleMouseEvent = new MouseEvent("click", {
> +    bubbles: true,
> +    button: 1,
> +    view: window
> +  });
> +  let ctrlOrCmdKeyMouseEvent = new MouseEvent("click", {
> +    bubbles: true,
> +    [isOSX ? "metaKey" : "ctrlKey"]: true,
> +    view: window
> +  });
> +  let rightClickMouseEvent = new MouseEvent("click", {
> +    bubbles: true,
> +    button: 2,
> +    view: window
> +  });
> +  let rightClickCtrlOrCmdKeyMouseEvent = new MouseEvent("click", {
> +    bubbles: true,
> +    button: 2,
> +    [isOSX ? "metaKey" : "ctrlKey"]: true,
> +    view: window
> +  });

can the construction of the event be made in a separate function to make it easier following the test ?

```js
let {
  middleMouseEvent,
  ctrlOrCmdKeyMouseEvent,
  rightClickMouseEvent,
  rightClickCtrlOrCmdKeyMouseEvent,
} = getMouseEvents();

…

function getMouseEvents() {
 let middleMouseEvent = new MouseEvent("click", {
    bubbles: true,
    button: 1,
    view: window
  });
  let ctrlOrCmdKeyMouseEvent = new MouseEvent("click", {
    bubbles: true,
    [isOSX ? "metaKey" : "ctrlKey"]: true,
    view: window
  });
  let rightClickMouseEvent = new MouseEvent("click", {
    bubbles: true,
    button: 2,
    view: window
  });
  let rightClickCtrlOrCmdKeyMouseEvent = new MouseEvent("click", {
    bubbles: true,
    button: 2,
    [isOSX ? "metaKey" : "ctrlKey"]: true,
    view: window
  });
  
  return {
    middleMouseEvent,
    ctrlOrCmdKeyMouseEvent,
    rightClickMouseEvent,
    rightClickCtrlOrCmdKeyMouseEvent,
  };
}
```

::: devtools/client/webconsole/new-console-output/test/mochitest/browser_webconsole_network_messages_status_code.js:73
(Diff revision 5)
> +    { clickEvent: ctrlOrCmdKeyMouseEvent, link: LEARN_MORE_URI, where: "tabshifted" },
> +    { clickEvent: rightClickMouseEvent, link: null, where: null },
> +    { clickEvent: rightClickCtrlOrCmdKeyMouseEvent, link: null, where: null }
> +  ];
> +
> +  for (let i = 0; i < testCases.length; i++) {

we could use `testCases.forEach`, or `for (let testCase of testCases)` to be a bit cleaner

::: devtools/client/webconsole/new-console-output/test/mochitest/browser_webconsole_network_messages_status_code.js:79
(Diff revision 5)
> +  info("Check if context menu is opened on right clicking the status-code");
> +  await openContextMenu(hud, statusCodeNode);

shouldn't this go into the for loop ?
There is 2 right click event, and we should make sure that both of them trigger the context menu.

Also, openContextMenu , as its name suggests, does open the context menu, and not wait for the menu to be open. So here we are not really testing that it is our right click that open the context menu.

To do such thing, we should do like in the function, i.e. wait for the menu-open event: 

let onConsoleMenuOpened = hud.ui.newConsoleOutput.once("menu-open");
await simulateLinkClick(…)
await onConsoleMenuOpened;

This should be done in the loop, if we expect the context menu to be opened.

::: devtools/client/webconsole/new-console-output/test/mochitest/head.js:18
(Diff revision 5)
>  const DOCS_GA_PARAMS = "?utm_source=mozilla" +
>                         "&utm_medium=firefox-console-errors" +
>                         "&utm_campaign=default";
> +const STATUS_CODES_GA_PARAMS = "?utm_source=mozilla" +
> +                               "&utm_medium=devtools-webconsole" +
> +                               "&utm_campaign=default";

could we clean this up using URLSearchParams ?

::: devtools/client/webconsole/new-console-output/test/mochitest/head.js:411
(Diff revision 5)
>   * Fake clicking a link and return the URL we would have navigated to.
>   * This function should be used to check external links since we can't access
>   * network in tests.

We should add a sentence to say that it can also be used to test that a click will not be fired

::: devtools/client/webconsole/new-console-output/test/mochitest/head.js:420
(Diff revision 5)
>   * @param ElementNode element
>   *        The <a> element we want to simulate click on.
> + * @param Object clickEventProps
> + *        The custom properties which would be used to dispatch a click event
>   * @returns Promise
> - *          A Promise that resolved when the link clik simulation occured.
> + *          A Promise that resolved when the link click simulation occured.

we should say what the Promise resolves with (an object with link and where), and say what it returns when the clikc did not happen.

::: devtools/client/webconsole/new-console-output/test/mochitest/head.js:423
(Diff revision 5)
> -  return new Promise((resolve) => {
> +  // wait for 1 sec before resolving if none of the other promises resolve
> +  let promises = [
> +    new Promise(function(resolve, reject) {
> +      setTimeout(resolve, 1000, {link: null, where: null});
> +    }),
> +    new Promise((resolve) => {
> -    // Override openUILinkIn to prevent navigating.
> +      // Override openUILinkIn to prevent navigating.
> -    let oldOpenUILinkIn = window.openUILinkIn;
> +      let oldOpenUILinkIn = window.openUILinkIn;
> -    window.openUILinkIn = function (link) {
> +      window.openUILinkIn = function (link, where) {
> -      window.openUILinkIn = oldOpenUILinkIn;
> +        window.openUILinkIn = oldOpenUILinkIn;
> -      resolve(link);
> +        resolve({link: link, where});
> -    };
> +      };
>  
> +      if (clickEventProps) {
> +        // Click on the link using the event properties.
> +        element.dispatchEvent(clickEventProps);
> +      } else {
> -    // Click on the link.
> +        // Click on the link.
> -    element.click();
> +        element.click();
> -  });
> -}
> +      }
> +    })
> +  ];
> +  return Promise.race(promises);

This is good. I think we could declare things separately so it is easier to read : 

```js
function simulateLinkClick(element, clickEventProps) {
  const onOpenLink = new Promise((resolve) => {
    // Override openUILinkIn to prevent navigating.
    let oldOpenUILinkIn = window.openUILinkIn;
    window.openUILinkIn = function (link, where) {
      window.openUILinkIn = oldOpenUILinkIn;
      resolve({link: link, where});
    };

    if (clickEventProps) {
      // Click on the link using the event properties.
      element.dispatchEvent(clickEventProps);
    } else {
      // Click on the link.
      element.click();
    }
  });
  
  // Declare a timeout Promise that we can use to make sure openUILinkIn was not called.
  const onTimeout = new Promise(function(resolve, reject) {
    setTimeout(resolve, 1000, {link: null, where: null});
  });
  

  return Promise.race([onOpenLink, onTimeout]);

```

Comment 24

[Abhinav Koppula]

Comment on attachment 8932992 [details]
Bug 1421213 - Clicking on the request status code should open the corresponding MDN page.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/203990/diff/5-6/

Comment 25

[Abhinav Koppula]

Comment on attachment 8932992 [details]
Bug 1421213 - Clicking on the request status code should open the corresponding MDN page.

https://reviewboard.mozilla.org/r/203990/#review216116

Hi Nicolas,
Thanks for the review. I have updated my patch addressing the nits.

Comment 26

[Nicolas Chevobbe [:nchevobbe]]

Comment on attachment 8932992 [details]
Bug 1421213 - Clicking on the request status code should open the corresponding MDN page.

https://reviewboard.mozilla.org/r/203990/#review216578

This looks good, thanks Abhinav.
Let's push to TRY

Comment 27

[Nicolas Chevobbe [:nchevobbe]]

Seems like there is an issue with the test leaking : 
https://treeherder.mozilla.org/#/jobs?repo=try&revision=772753d8bfa2fdd4961b20a33d8db69cc6a12a1d&selectedJob=154692004

Comment 28

[Julian Descottes [:jdescottes] (PTO -> Jan 5th)]

The leaks are caused by simulateLinkClick, when the timeout finishes first. We should still cleanup the openUILinkIn override.

For instance, you can modify the timeout as follows:

  const onTimeout = new Promise(function (resolve) {
    setTimeout(() => {
      window.openUILinkIn = oldOpenUILinkIn;
      resolve({link: null, where: null});
    }, 1000);
  });

(and oldOpenUILinkIn needs to be declared in the upper scope)

There are several other things that looked suspicious: timeout not cleared if the promise finishes first, context menu not hidden, event listeners once("menu-open") attached even when no context-menu is expected. None of them actually leak, but might be nice to clean them up :)

Comment 29

[Abhinav Koppula]

Comment on attachment 8932992 [details]
Bug 1421213 - Clicking on the request status code should open the corresponding MDN page.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/203990/diff/6-7/

Comment 30

[Abhinav Koppula]

Hi Julian,
Thanks for the suggestions.

Hi Nicolas,
I have updated the patch and pushed to TRY which looks green.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=c0d7a4f7438c3b8ddaf452afb2e8c67eb3bf267a

Can you also check once from your side?

Comment 31

[Nicolas Chevobbe [:nchevobbe]]

Comment on attachment 8932992 [details]
Bug 1421213 - Clicking on the request status code should open the corresponding MDN page.

https://reviewboard.mozilla.org/r/203990/#review218768

It looks good, thanks Abhinav.
Let's see if we can clean this up a bit further, and then land it :)

::: devtools/client/webconsole/new-console-output/test/mochitest/browser_webconsole_network_messages_status_code.js:58
(Diff revision 7)
> +    { clickEvent: rightClickMouseEvent, link: null, where: null },
> +    { clickEvent: rightClickCtrlOrCmdKeyMouseEvent, link: null, where: null }
> +  ];
> +
> +  for (let testCase of testCases) {
> +    let onConsoleMenuOpened = hud.ui.newConsoleOutput.once("menu-open");

let's declare this only if we'll check for the context menu: 

```js
const {clickEvent} = testCase;
const onConsoleMenuOpened = [rightClickMouseEvent, rightClickCtrlOrCmdKeyMouseEvent].includes(clickEvent)
  ? hud.ui.newConsoleOutput.once("menu-open")
  : null;

let { link, where } = await simulateLinkClick(statusCodeNode, clickEvent);
is(link, testCase.link, `Clicking the provided link opens ${link}`);
is(where, testCase.where, `Link opened in correct tab`);

if (onConsoleMenuOpened) {
  info("Check if context menu is opened on right clicking the status-code");
  await onConsoleMenuOpened;
}
```

::: devtools/client/webconsole/new-console-output/test/mochitest/head.js:455
(Diff revision 7)
> +  const onTimeout = new Promise(function(resolve, reject) {
> +    setTimeout(() => {
> +      window.openUILinkIn = oldOpenUILinkIn;
> +      resolve({link: null, where: null});
> +    }, 1000);
>    });

Like Julian said in a previous comment, we could cancel the timeout if onOpenLink reolves first.

```js
let timeoutId;
const onTimeout = new Promise(function(resolve, reject) {
  timeoutId = setTimeout(() => {
    window.openUILinkIn = oldOpenUILinkIn;
    timeoutId = null;
    resolve({link: null, where: null});
  }, 1000);
});

onOpenLink.then(() = {
  if (timeoutId) {
    clearTimeout(timeoutId);
  }
});

return Promise.race([onOpenLink, onTimeout]);
```

Comment 32

[Abhinav Koppula]

Comment on attachment 8932992 [details]
Bug 1421213 - Clicking on the request status code should open the corresponding MDN page.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/203990/diff/7-8/

Comment 33

[Abhinav Koppula]

Updated the patch
TRY link:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=8003c42156aeae4d472cc742b66c772738a0dee7

Comment 34

[Nicolas Chevobbe [:nchevobbe]]

Looks good to me, thanks for doing those changes Abhinav

Comment 35

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/9029eac178ab
Clicking on the request status code should open the corresponding MDN page. r=nchevobbe

Comment 36

[Nicolas Chevobbe [:nchevobbe]]

Comment on attachment 8932992 [details]
Bug 1421213 - Clicking on the request status code should open the corresponding MDN page.

https://reviewboard.mozilla.org/r/203990/#review219202

Comment 37

[Narcis Beleuzu [:NarcisB]]

Backed out for devtools failures on /browser_webconsole_hpkp_invalid-headers.js 

Log: https://treeherder.mozilla.org/logviewer.html#?job_id=156803118&repo=autoland&lineNumber=15784
Rev: https://hg.mozilla.org/integration/autoland/rev/a74949ee178297231f37a98687e83038568d865f

Comment 38

[Nicolas Chevobbe [:nchevobbe]]

yes, the patch needs to be rebased against mozilla-central

Comment 39

[Abhinav Koppula]

Attachment: Bug 1421213 - Fix tests broken due to simulateClickLink changes.

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

Comment 40

[Abhinav Koppula]

Comment on attachment 8932992 [details]
Bug 1421213 - Clicking on the request status code should open the corresponding MDN page.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/203990/diff/8-9/

Comment 41

[Abhinav Koppula]

TRY push:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=d56ad7909ab254c4b5b69b4ba0c473a1f9c3ef80

Comment 42

[Nicolas Chevobbe [:nchevobbe]]

Comment on attachment 8943284 [details]
Bug 1421213 - Fix tests broken due to simulateClickLink changes.

https://reviewboard.mozilla.org/r/213606/#review219328

Looks good, thanks Abhinav !

Comment 43

[Mozilla Autoland]

hg error in cmd: hg push -r tip ssh://hg.mozilla.org/integration/autoland: pushing to ssh://hg.mozilla.org/integration/autoland
searching for changes
remote: adding changesets
remote: adding manifests
remote: adding file changes
remote: added 2 changesets with 13 changes to 13 files
remote: 
remote: 
remote: ************************** ERROR ****************************
remote: Rev b7ec862acce7 needs "Bug N" or "No bug" in the commit message.
remote: abhinav <[e-mail]>
remote: Fix tests broken due to simulateClickLink changes r=nchevobbe
remote: 
remote: MozReview-Commit-ID: 6JYBSzgMzR
remote: *************************************************************
remote: 
remote: 
remote: transaction abort!
remote: rollback completed
remote: pretxnchangegroup.c_commitmessage hook failed
abort: push failed on remote

Comment 44

[Abhinav Koppula]

Comment on attachment 8943284 [details]
Bug 1421213 - Fix tests broken due to simulateClickLink changes.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/213606/diff/1-2/

Comment 45

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/76f2c6767866
Clicking on the request status code should open the corresponding MDN page. r=nchevobbe
https://hg.mozilla.org/integration/autoland/rev/64c447e34577
Fix tests broken due to simulateClickLink changes. r=nchevobbe

Comment 46

[Tiberius Oros[:tiberius_oros]]

https://hg.mozilla.org/mozilla-central/rev/76f2c6767866
https://hg.mozilla.org/mozilla-central/rev/64c447e34577