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

RESOLVED FIXED in Firefox 48

Status

DevTools
Shared Components
P1
normal
RESOLVED FIXED
2 years ago
9 days ago

People

(Reporter: jsantell, Assigned: jbhoosreddy)

Tracking

(Blocks: 1 bug, {regression})

48 Branch
Firefox 50
regression

Firefox Tracking Flags

(firefox47 unaffected, firefox48 fixed, firefox49 fixed, firefox50 fixed)

Details

(Whiteboard: [btpp-fix-later])

Attachments

(1 attachment, 4 obsolete attachments)

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.
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.
Flags: needinfo?(jsantell)
Priority: -- → P2
Whiteboard: [btpp-fix-later]
This is a known regression due to the way we parse URIs for displaying, bumping to P1
Flags: needinfo?(jsantell)
Priority: P2 → P1

Updated

2 years ago
Keywords: regression

Updated

2 years ago
status-firefox47: --- → unaffected
status-firefox48: --- → affected
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.
Joe this is a new regression in 48, do you plan on fixing it in that release?
status-firefox49: --- → affected
status-firefox50: --- → affected
Flags: needinfo?(jwalker)
Jaideep is looking at this, will evaluate uplift plan depending on the fix
Assignee: nobody → jaideepb
Flags: needinfo?(jwalker)
Status: NEW → ASSIGNED
(Assignee)

Comment 6

2 years ago
Created attachment 8764017 [details] [diff] [review]
1255908.patch

Replaced the "/" with hostname, as per discussion.
Attachment #8764017 - Flags: review?(bgrinstead)
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;
Attachment #8764017 - Flags: review?(bgrinstead)
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.
(Assignee)

Comment 9

2 years ago
Created attachment 8764099 [details] [diff] [review]
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.
Attachment #8764017 - Attachment is obsolete: true
Flags: needinfo?(bgrinstead)
Attachment #8764099 - Flags: review?(bgrinstead)
Attachment #8764099 - Flags: feedback?(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 === "/")
Attachment #8764099 - Flags: feedback?(jsnajdr) → feedback+
(Assignee)

Comment 11

2 years ago
Created attachment 8764151 [details] [diff] [review]
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.
Attachment #8764099 - Attachment is obsolete: true
Attachment #8764099 - Flags: review?(bgrinstead)
Flags: needinfo?(bgrinstead)
Attachment #8764151 - Flags: review?(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.
Attachment #8764151 - Flags: review?(jsnajdr) → review+
(Assignee)

Comment 14

2 years ago
Created attachment 8764358 [details] [diff] [review]
1255908.patch [3.0]
Attachment #8764151 - Attachment is obsolete: true
Attachment #8764358 - Flags: review+
(Assignee)

Comment 17

2 years ago
Created attachment 8764456 [details] [diff] [review]
1255908.patch [4.0]
Attachment #8764358 - Attachment is obsolete: true
Attachment #8764456 - Flags: review+
(Assignee)

Updated

2 years ago
Keywords: checkin-needed

Comment 18

2 years ago
Pushed by cbook@mozilla.com:
https://hg.mozilla.org/integration/fx-team/rev/437fc937572d
Replaced "/" with hostname when for root document in webconsole; r=jsnajdr
Keywords: checkin-needed

Comment 19

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/437fc937572d
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox50: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 50
As a P1 regression, it would probably be good to get this uplifted. Are you OK to do that Jaideep?
Flags: needinfo?(jaideepb)
(Assignee)

Comment 21

2 years ago
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
Attachment #8764456 - Flags: approval-mozilla-beta?
Attachment #8764456 - Flags: approval-mozilla-aurora?
(Assignee)

Comment 22

2 years ago
task done; removing needinfo request.
(Assignee)

Updated

2 years ago
Flags: needinfo?(jaideepb)
Comment on attachment 8764456 [details] [diff] [review]
1255908.patch [4.0]

Fix a recent regression, taking it.
Should be in 48 beta 4
Attachment #8764456 - Flags: approval-mozilla-beta?
Attachment #8764456 - Flags: approval-mozilla-beta+
Attachment #8764456 - Flags: approval-mozilla-aurora?
Attachment #8764456 - Flags: approval-mozilla-aurora+

Comment 24

2 years ago
bugherderuplift
https://hg.mozilla.org/releases/mozilla-aurora/rev/cdb052e0c1e4
status-firefox49: affected → fixed

Comment 25

2 years ago
bugherderuplift
https://hg.mozilla.org/releases/mozilla-beta/rev/fab02e732bcd
status-firefox48: affected → fixed
Version: unspecified → 48 Branch

Updated

9 days ago
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.