1255908 - [FrameComponent] Provide more useful name other than "/"

Status: RESOLVED FIXED

(DevTools :: Shared Components, defect, P1)

Description

[Jordan Santell [:jsantell] [@jsantell]]

Webconsole would previously render sources from "http://www.cnn.com/" as "www.cnn.com" -- Now the frame component renders it as "/". The previously logic for rendering the source didn't have a special case for this, it just seemed like a side effect, so not sure of all the rules we had in place previously.

We should try to be as helpful as possible when abbreviating.

Comment 1

[Lin Clark [:linclark]]

Is this a recent regression? Or has this been happening for a while? 

If it's a recent regression, I think this should be a P1. Going to mark as P2 for now, though.

Comment 2

[Jordan Santell [:jsantell] [@jsantell]]

This is a known regression due to the way we parse URIs for displaying, bumping to P1

Comment 3

[Jordan Santell [:jsantell] [@jsantell]]

We previously would render "/" for some sites like "http://www.cnn.com/" for warnings/errors -- it doesn't seem to be consistent on when we just render "/" versus a domain previously -- either way, they're all going through the same path now, so let's improve it to be as helpful as possible. So, partial regression.

Comment 4

[James Willcox (:snorp) ([e-mail]) (he/him)]

Joe this is a new regression in 48, do you plan on fixing it in that release?

Comment 5

[Brian Grinstead [:bgrins]]

Jaideep is looking at this, will evaluate uplift plan depending on the fix

Comment 6

[Jaideep Bhoosreddy [:jbhoosreddy]]

Attachment: 1255908.patch

Replaced the "/" with hostname, as per discussion.

Comment 7

[Brian Grinstead [:bgrins]]

Comment on attachment 8764017 [details] [diff] [review]
1255908.patch

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

Can you also add a test case for this to devtools/client/shared/test/test_source-utils.js?

::: devtools/client/shared/source-utils.js
@@ +75,5 @@
>      // `hostname`: "foo.com"
>      // `host`: "foo.com:8888"
>      let isChrome = isChromeScheme(location);
>  
>      url.fileName = url.pathname ?

I think changing this statement would be roughly equivalent without needing a new one (replacing the hardcoded '/' with url.hostname). Feel free to disregard if I'm wrong about that:

url.fileName = url.pathname ?
  (url.pathname.slice(url.pathname.lastIndexOf("/") + 1) || url.hostname) :
  url.hostname;

Comment 8

[Jarda Snajdr [:jsnajdr]]

Changing the behavior of the parseURL in this way doesn't look like a good idea. Do really all the clients of this function (getSourceNames, the Frame component, its usages in Console, the Performance and Memory tool...) expect and desire the fileName component to be replaced with host in case it's '/'? Probably not.

A better approach would be to add a new option to the Frame component (something like showEmptyPathAsHost) and set it to true where desired. Then, inside the component, use the 'host' property returned from getSourceNames accordingly.

Comment 9

[Jaideep Bhoosreddy [:jbhoosreddy]]

Attachment: 1255908.patch [1.0]

Thanks for the comment Jarda. I noticed it when I started writing the test for source_utils as Brian said.

But since I removed my changes to source_utils, and I didn't find any tests for React components, let me know how to proceed.

Comment 10

[Jarda Snajdr [:jsnajdr]]

Comment on attachment 8764099 [details] [diff] [review]
1255908.patch [1.0]

Hello Jaideep, this approach looks much better. Few things that are missing to make it perfect:

- mochitest for the frame component is at devtools/client/shared/components/test/mochitest/test_frame_01.html. Let's add some new test cases to it.
- the showEmptyPathAsRoot should be a part of the component props, and its default value should be false - let's enable it only where wanted. Please add the right code to propTypes and getDefaultProps.
- can it happen that the short path is an empty string? Then I'd add a check for it: if (short === "" || short === "/")

Comment 11

[Jaideep Bhoosreddy [:jbhoosreddy]]

Attachment: 1255908.patch [2.0]

Thanks for the detailed explanation Jarda. That was really helpful, especially gave me a better understanding of shared components in devtools.

Comment 12

[Jaideep Bhoosreddy [:jbhoosreddy]]

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

Comment 13

[Jarda Snajdr [:jsnajdr]]

Comment on attachment 8764151 [details] [diff] [review]
1255908.patch [2.0]

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

r+ if you fix the tests and have a green try.

Also, please rewrite the commit message - it's no longer true that you 'Replaced "/" with hostname when parsing source in SourceUtils'

::: devtools/client/shared/components/frame.js
@@ +79,5 @@
>        );
>      }
>  
> +    let displaySource = short;
> +    if ((short === "" || short === "/") && showEmptyPathAsHost) {

nit: it's better to write the condition as "showEmptyPathAsHost && (the rest)" - evaluate the cheap condition first, and allow the short-circuit evaluation to skip the more expensive check.

::: devtools/client/shared/components/test/mochitest/test_frame_01.html
@@ +155,5 @@
> +    // Check if file is rendered with hostname for root documents when showEmptyPathAsString is true
> +    yield checkFrameComponent({
> +      source: "http://www.cnn.com/",
> +      line: "1",
> +      showEmptyPathAsHost: false,

The showEmptyPathAsHost property should be true here.

@@ +158,5 @@
> +      line: "1",
> +      showEmptyPathAsHost: false,
> +    }, {
> +      file: "www.cnn.com",
> +      shouldLink: true,

You need to specify an expected "line" property here to make the tests pass.

Comment 14

[Jaideep Bhoosreddy [:jbhoosreddy]]

Attachment: 1255908.patch [3.0]

Comment 15

[Jaideep Bhoosreddy [:jbhoosreddy]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=2b9b790a97b1

Comment 16

[Jaideep Bhoosreddy [:jbhoosreddy]]

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

Comment 17

[Jaideep Bhoosreddy [:jbhoosreddy]]

Attachment: 1255908.patch [4.0]

Comment 18

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/fx-team/rev/437fc937572d
Replaced "/" with hostname when for root document in webconsole; r=jsnajdr

Comment 19

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/437fc937572d

Comment 20

[Joe Walker [:jwalker]]

As a P1 regression, it would probably be good to get this uplifted. Are you OK to do that Jaideep?

Comment 21

[Jaideep Bhoosreddy [:jbhoosreddy]]

Comment on attachment 8764456 [details] [diff] [review]
1255908.patch [4.0]

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

Approval Request Comment - Bug 1255908
[Feature/Regression]: Regression affecting 48 & 49

[User impact if declined]: Console error for root document would display "/" instead of hostname.
[Describe test coverage new/current, TreeHerder]: Added test cases in the test suite to test the affected component.
[Risks and why]: Medium, minor JavaScript change with tests.
[String/UUID change made/needed]: None

Comment 22

[Jaideep Bhoosreddy [:jbhoosreddy]]

task done; removing needinfo request.

Comment 23

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8764456 [details] [diff] [review]
1255908.patch [4.0]

Fix a recent regression, taking it.
Should be in 48 beta 4

Comment 24

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/cdb052e0c1e4

Comment 25

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-beta/rev/fab02e732bcd