Closed Bug 1448553 Opened 7 years ago Closed 7 years ago

Provide common implementations that make contents in developer tools show human-readable Unicode domain names and Unicode filenames instead of showing Punycode domain names and URI-encoded filenames

Categories

(DevTools :: General, defect)

Product:
DevTools
Component:
General
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(firefox61 fixed)

Status:
RESOLVED FIXED
Milestone:
Firefox 61
Tracking Flags:
Tracking Status
firefox61 --- fixed

People

(Reporter: zjz, Assigned: zjz)

References

Details

Attachments

(19 files, 5 obsolete files)

Screenshot.png
32.20 KB, image/png
Details
Screenshot after patching part 2.png
30.67 KB, image/png
Details
Screenshot1 before patching part 3.png
20.77 KB, image/png
Details
Screenshot2 before patching part 3.png
20.54 KB, image/png
Details
Screenshot1 after patching part 3.png
19.23 KB, image/png
Details
Screenshot2 after patching part 3.png
1.82 MB, image/png
Details
Screenshot before patching part 4.png
24.36 KB, image/png
Details
Screenshot after patching part 4.png
26.76 KB, image/png
Details
network-panel-punycode.png
65.79 KB, image/png
Details
better to keep the request URL in the header panel to be the raw URL rather than the decoded one.png
7.28 KB, image/png
Details
Bug 1448553 - Part 1: Defines common functions for getting a Unicode URL or a Unicode URL component
59 bytes, text/x-review-board-request
nchevobbe
: review+
Details
Bug 1448553 - Part 2: Decodeds Punycode-encoded international domain names and URI-encoded filenames in the Web Console so that they are displayed as human-readable Unicode text.
59 bytes, text/x-review-board-request
nchevobbe
: review+
Details
Bug 1448553 - Part 3: Decodeds Punycode-encoded international domain names in the Network Monitor so that they are displayed as human-readable Unicode text.
59 bytes, text/x-review-board-request
Honza
: review+
Details
Bug 1448553 - Part 4: Decodeds Punycode-encoded international domain names in the Storage Inspector so that they are displayed as human-readable Unicode text.
59 bytes, text/x-review-board-request
Details
Bug 1448553 - Part 5: Decodeds Punycode-encoded international domain names and URI-encoded filenames in the Developer Toolbox's frame menu so that they are displayed as human-readable Unicode text.
59 bytes, text/x-review-board-request
Details
Bug 1448553 - Part 6: Adds xpcshell-based unit tests for the common Unicode-URL related functions defined in Part 1
59 bytes, text/x-review-board-request
nchevobbe
: review+
Details
Screenshort before patching part 5.png
2.58 KB, image/png
Details
Screenshort after patching part 5.png
2.29 KB, image/png
Details
Bug 1448553 - Part 7: Adds mochitests to test the feature of filtering Unicode strings and Unicode filenames in the Web Console.
59 bytes, text/x-review-board-request
nchevobbe
: review+
Details
User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:58.0) Gecko/20100101 Firefox/58.0 Build ID: 20180123231643 Steps to reproduce: Step 1: Create an HTML file with the following content: <!DOCTYPE html ><meta charset="utf-8" ><title>Test to include Javascript named with non-ASCII characters</title ><script src="測.js"></script ><p>Test to include Javascript named with non-ASCII characters</p> Step 2: Create a Javascript file 測.js with the following content: console.log("a test"); Step 3: Open the HTML file with Firefox, and then see what the web console output. Actual results: It outputs text with a unreadable URI-encoded filename. Expected results: It should have output text with a readable URI-decoded filename; or at least it should be customizable through about:config for the user to choose which they would like to see, and default the value to "showing decoded".
Also, the full path of the file name will be displayed when the pointer device is hovering over the filename. The full path is shown in a URI-encoded way like the filename, so it needs to be decoded too. Besides, if the full path includes a non-ASCII domain name, it's shown in Punycode, which needs to be depunycoded.
Attached image Screenshot.png —
Here is a screenshot showing what I have described above
Summary: The web console should show a human-readable URI-decoded script name instead of an encoded one → The web console should show a human-readable URI-decoded script filename instead of an encoded one
Component: Untriaged → Developer Tools: Console
Assignee: nobody → zjz
Component: Developer Tools: Console → Developer Tools
Summary: The web console should show a human-readable URI-decoded script filename instead of an encoded one → Contents in developer tools should show human-readable Unicode domain names and Unicode filenames instead of showing Punycode domain names and URI-encoded filenames
I changed the summary to expand coverage for this bug. This kind of issues does not only exist on contents in console, but also on contents in debugger, network, etc.
I think this bug should block Bug 1450292
Flags: needinfo?(pbrosset)
Summary: Contents in developer tools should show human-readable Unicode domain names and Unicode filenames instead of showing Punycode domain names and URI-encoded filenames → Provide common implementations that make contents in developer tools show human-readable Unicode domain names and Unicode filenames instead of showing Punycode domain names and URI-encoded filenames
(I wondered if this is related to bug 987069... Looks like the latest news is that bug 1380617 made the Punycode domains appear, which is slightly better that before, but still sub-optimal. So, perhaps Punycode detection and conversion makes sense in DevTools UI side...?)
Blocks: 1450292
Flags: needinfo?(pbrosset)
(In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #5) > (I wondered if this is related to bug 987069... Looks like the latest news > is that bug 1380617 made the Punycode domains appear, which is slightly > better that before, but still sub-optimal. So, perhaps Punycode detection > and conversion makes sense in DevTools UI side...?) Yes, I agree with you. bug 987069 became actually defunct when bug 1380617 was resolved. The remaining work is in the DevTools side.
This is the screen after patching part 1. Now all the filename and hostname are readable as Unicode characters if they are non-English.
Comment on attachment 8964150 [details] Bug 1448553 - Part 1: Decodeds Punycode-encoded international domain names and URI-encoded filenames in the Web console(developer tool) so that they are displayed as human-readable Unicode text. https://reviewboard.mozilla.org/r/232914/#review238370 Code analysis found 2 defects in this patch: - 2 defects found by mozlint You can run this analysis locally with: - `./mach lint path/to/file` (JS/Python) If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx ::: devtools/client/shared/components/Frame.js:181 (Diff revision 1) > " " > ); > } > } > > - let displaySource = showFullSourceUrl ? long : short; > + let displaySource = showFullSourceUrl ? Error: Do not nest ternary expressions. [eslint: no-nested-ternary] ::: devtools/client/shared/source-utils.js:193 (Diff revision 1) > if (short === "/" && parsedUrl.pathname !== "/") { > short = parseURL(long.replace(/\/$/, "")).fileName; > } > + > + // Get the readable names. > + const idnService = Cc["@mozilla.org/network/idn-service;1"]. Error: Expected dot to be on same line as property. [eslint: dot-location]
Attachment #8964152 - Attachment description: Screenshot after paching part 1.png → Screenshot after patching part 1.png
After patching part 3, the tree pane in the Storage shows the Unicode domain name together with the raw domain name, but the table pane keeps the raw domain name unchanged. as from the perspective of client programmer, cookies and other storages are storaged according to the raw name instead of the decoded name, so I intentionally leave the raw domain name in the table unchanged, which is a special case particularly in part 3.
Comment on attachment 8964189 [details] Bug 1448553 - Part 3: Decodeds Punycode-encoded international domain names in the Storage(developer tool) so that they are displayed as human-readable Unicode text. https://reviewboard.mozilla.org/r/232934/#review238384 Code analysis found 2 defects in this patch: - 2 defects found by mozlint You can run this analysis locally with: - `./mach lint path/to/file` (JS/Python) If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx ::: devtools/client/storage/ui.js:437 (Diff revision 1) > * @param {object} See onEdit docs > */ > async handleAddedItems(added) { > + // idnService is used to convert host names into readable Unicode domain > + // names if they are in Punycode. > + const idnService = Cc["@mozilla.org/network/idn-service;1"].getService(Ci.nsIIDNService); Error: Line 437 exceeds the maximum line length of 90. [eslint: max-len] ::: devtools/client/storage/ui.js:642 (Diff revision 1) > * StorageFront.listStores call. > */ > populateStorageTree(storageTypes) { > + // idnService is used to convert host names into readable Unicode domain > + // names if they are in Punycode. > + const idnService = Cc["@mozilla.org/network/idn-service;1"].getService(Ci.nsIIDNService); Error: Line 642 exceeds the maximum line length of 90. [eslint: max-len]
Comment on attachment 8964189 [details] Bug 1448553 - Part 3: Decodeds Punycode-encoded international domain names in the Storage(developer tool) so that they are displayed as human-readable Unicode text. https://reviewboard.mozilla.org/r/232934/#review238386 Code analysis found 2 defects in this patch: - 2 defects found by mozlint You can run this analysis locally with: - `./mach lint path/to/file` (JS/Python) If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx ::: devtools/client/storage/ui.js:437 (Diff revision 2) > * @param {object} See onEdit docs > */ > async handleAddedItems(added) { > + // idnService is used to convert host names into readable Unicode domain > + // names if they are in Punycode. > + const idnService = Cc["@mozilla.org/network/idn-service;1"].getService(Ci.nsIIDNService); Error: Line 437 exceeds the maximum line length of 90. [eslint: max-len] ::: devtools/client/storage/ui.js:642 (Diff revision 2) > * StorageFront.listStores call. > */ > populateStorageTree(storageTypes) { > + // idnService is used to convert host names into readable Unicode domain > + // names if they are in Punycode. > + const idnService = Cc["@mozilla.org/network/idn-service;1"].getService(Ci.nsIIDNService); Error: Line 642 exceeds the maximum line length of 90. [eslint: max-len]
Thanks for working on this. From your screenshot, I notice the storage inspector is inconsistent with the other two tools you've patched. The storage inspector now displays both human-readable and punycode, while the other tools display just human-readable. Is that intentional ? Also, could you create a shared module in devtools/client/shared/ to avoid duplication ?
Flags: needinfo?(zjz)
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Attachment #8964150 - Flags: review?(jordan) → review?(nchevobbe)
(In reply to Tim Nguyen :ntim from comment #23) Thank you for the review. > Thanks for working on this. From your screenshot, I notice the storage > inspector is inconsistent with the other two tools you've patched. The > storage inspector now displays both human-readable and punycode, while the > other tools display just human-readable. Is that intentional ? > It is intentional. Because the table may show more than one domains, although they will all belong to one common parent domain, it may not be very appearant to match the level in the long Puyncode characters at a first glance，especially when the domain name consists more than two domain levels. So I labeled the Unicode domain name together with the Punycode domain name to help developers more easily match domains. > Also, could you create a shared module in devtools/client/shared/ to avoid > duplication ? I originally wanted to create a new shared module, but then I found there is a ready-made shared IDL interface nsIIDNService which converts a Punycode domain to a Unicode domain, and it's fairly easy to use, we just need to call its method convertToDisplayIDN and that's all. And also, converting URI-encoded filename is also fairly easy by just calling decodeURIComponent. So I think the code in the different tools are not actually duplicated, creating a shared module wouldn't help in the sense of avoiding duplication and reducing the number of the code lines, but it does help in the sense of naming a easy-to-remember method because the code of using nsIIDNService is a very long line and hard to remember. So, yeah, I will create a new shared module providing easy-to-remember functions for this.
Flags: needinfo?(zjz)
The patches are going through a refactoring, so please don't review it now, I'll upload a refactored one soon.
I was testing the Network panel using STRs from comment #0 and I am not sure if I see the actual issue. See the attached screenshot with highlighted areas where the file path/name is. No patch applied. What's wrong? Honza
> No patch applied. What's wrong? From the screenshots Zhang provided, only the domain name is broken for the netmonitor, not the filename.
(In reply to Jan Honza Odvarko [:Honza] from comment #26) > Created attachment 8964253 [details] > network-panel-punycode.png > > I was testing the Network panel using STRs from comment #0 and I am not sure > if I see the actual issue. See the attached screenshot with highlighted > areas where the file path/name is. No patch applied. What's wrong? > > Honza Both the Unicode filename and the Unicode domain name are broken in the web console. In the network monitor and the stroage inspector, only the Unicode domain name is broken.
(In reply to Jan Honza Odvarko [:Honza] from comment #26) > Created attachment 8964253 [details] > network-panel-punycode.png > > I was testing the Network panel using STRs from comment #0 and I am not sure > if I see the actual issue. See the attached screenshot with highlighted > areas where the file path/name is. No patch applied. What's wrong? > > Honza BTW, since you uploaded the screenshot, I would like to talk about the header content, I personally think it might be better to keep the request URL in the header panel to be the raw URL rather than the decoded one, because the header content is used to represent actually transferred data. So I prefer this one to be the raw. Please see bug 1450435 I filed.
Attachment #8964150 - Attachment is obsolete: true
Attachment #8964150 - Flags: review?(nchevobbe)
Attachment #8964153 - Attachment is obsolete: true
Attachment #8964153 - Flags: review?(odvarko)
Attachment #8964189 - Attachment is obsolete: true
Attachment #8964189 - Flags: review?(ntim.bugs)
Attachment #8964190 - Attachment description: Screenshot before patching part 3.png → Screenshot before patching part 4.png
Attachment #8964191 - Attachment description: Screenshot after patching part 3.png → Screenshot after patching part 4.png
Comment on attachment 8964343 [details] Bug 1448553 - Part 6: Adds xpcshell-based unit tests for the common Unicode-URL related functions defined in Part 1 https://reviewboard.mozilla.org/r/233070/#review238498 Code analysis found 30 defects in this patch: - 30 defects found by mozlint You can run this analysis locally with: - `./mach lint path/to/file` (JS/Python) If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx ::: devtools/client/shared/test/unit/test_unicode-url.js:19 (Diff revision 1) > +// List of URLs used to test Unicode URL conversion > +const TEST_URLS = [ > + // Type: Readable ASCII URLs > + // Expected: All of Unicode versions should equal to the raw. > + { > + raw: "https://example.org", Error: Extra space before value for key 'raw'. [eslint: key-spacing] ::: devtools/client/shared/test/unit/test_unicode-url.js:23 (Diff revision 1) > + { > + raw: "https://example.org", > + expectedUnicode: "https://example.org", > + }, > + { > + raw: "http://example.org", Error: Extra space before value for key 'raw'. [eslint: key-spacing] ::: devtools/client/shared/test/unit/test_unicode-url.js:27 (Diff revision 1) > + { > + raw: "http://example.org", > + expectedUnicode: "http://example.org", > + }, > + { > + raw: "ftp://example.org", Error: Extra space before value for key 'raw'. [eslint: key-spacing] ::: devtools/client/shared/test/unit/test_unicode-url.js:31 (Diff revision 1) > + { > + raw: "ftp://example.org", > + expectedUnicode: "ftp://example.org", > + }, > + { > + raw: "https://example.org.", Error: Extra space before value for key 'raw'. [eslint: key-spacing] ::: devtools/client/shared/test/unit/test_unicode-url.js:35 (Diff revision 1) > + { > + raw: "https://example.org.", > + expectedUnicode: "https://example.org.", > + }, > + { > + raw: "https://example.org/", Error: Extra space before value for key 'raw'. [eslint: key-spacing] ::: devtools/client/shared/test/unit/test_unicode-url.js:39 (Diff revision 1) > + { > + raw: "https://example.org/", > + expectedUnicode: "https://example.org/", > + }, > + { > + raw: "https://example.org/test", Error: Extra space before value for key 'raw'. [eslint: key-spacing] ::: devtools/client/shared/test/unit/test_unicode-url.js:43 (Diff revision 1) > + { > + raw: "https://example.org/test", > + expectedUnicode: "https://example.org/test", > + }, > + { > + raw: "https://example.org/test.html", Error: Extra space before value for key 'raw'. [eslint: key-spacing] ::: devtools/client/shared/test/unit/test_unicode-url.js:47 (Diff revision 1) > + { > + raw: "https://example.org/test.html", > + expectedUnicode: "https://example.org/test.html", > + }, > + { > + raw: "https://example.org/test.html?one=1&two=2", Error: Extra space before value for key 'raw'. [eslint: key-spacing] ::: devtools/client/shared/test/unit/test_unicode-url.js:51 (Diff revision 1) > + { > + raw: "https://example.org/test.html?one=1&two=2", > + expectedUnicode: "https://example.org/test.html?one=1&two=2", > + }, > + { > + raw: "https://example.org/test.html#here", Error: Extra space before value for key 'raw'. [eslint: key-spacing] ::: devtools/client/shared/test/unit/test_unicode-url.js:55 (Diff revision 1) > + { > + raw: "https://example.org/test.html#here", > + expectedUnicode: "https://example.org/test.html#here", > + }, > + { > + raw: "https://example.org/test.html?one=1&two=2#here", Error: Extra space before value for key 'raw'. [eslint: key-spacing] ::: devtools/client/shared/test/unit/test_unicode-url.js:63 (Diff revision 1) > + // Type: Unreadable URLs with either Punycode domain names or URI-encoded > + // paths > + // Expected: Unreadable domain names and URI-encoded paths should be converted > + // to readable Unicode. > + { > + raw: "https://xn--g6w.xn--8pv/test.html", Error: Extra space before value for key 'raw'. [eslint: key-spacing] ::: devtools/client/shared/test/unit/test_unicode-url.js:69 (Diff revision 1) > + // Do not type Unicode characters directly, because this test file isn't > + // specified with a known encoding. > + expectedUnicode: "https://\u6e2c.\u672c/test.html", > + }, > + { > + raw: "https://example.org/%E6%B8%AC%E8%A9%A6.html", Error: Extra space before value for key 'raw'. [eslint: key-spacing] ::: devtools/client/shared/test/unit/test_unicode-url.js:75 (Diff revision 1) > + // Do not type Unicode characters directly, because this test file isn't > + // specified with a known encoding. > + expectedUnicode: "https://example.org/\u6e2c\u8a66.html", > + }, > + { > + raw: "https://example.org/test.html?One=%E4%B8%80", Error: Extra space before value for key 'raw'. [eslint: key-spacing] ::: devtools/client/shared/test/unit/test_unicode-url.js:81 (Diff revision 1) > + // Do not type Unicode characters directly, because this test file isn't > + // specified with a known encoding. > + expectedUnicode: "https://example.org/test.html?One=\u4e00", > + }, > + { > + raw: "https://example.org/test.html?%E4%B8%80=1", Error: Extra space before value for key 'raw'. [eslint: key-spacing] ::: devtools/client/shared/test/unit/test_unicode-url.js:87 (Diff revision 1) > + // Do not type Unicode characters directly, because this test file isn't > + // specified with a known encoding. > + expectedUnicode: "https://example.org/test.html?\u4e00=1", > + }, > + { > + raw: "https://xn--g6w.xn--8pv/%E6%B8%AC%E8%A9%A6.html" + Error: Extra space before value for key 'raw'. [eslint: key-spacing] ::: devtools/client/shared/test/unit/test_unicode-url.js:99 (Diff revision 1) > + "#\u6b64", > + }, > + // Type: Malformed URLs > + // Expected: All should not be converted. > + { > + raw: "://example.org/test", Error: Extra space before value for key 'raw'. [eslint: key-spacing] ::: devtools/client/shared/test/unit/test_unicode-url.js:103 (Diff revision 1) > + { > + raw: "://example.org/test", > + expectedUnicode: "://example.org/test", > + }, > + { > + raw: "://xn--g6w.xn--8pv/%E6%B8%AC%E8%A9%A6.html" + Error: Extra space before value for key 'raw'. [eslint: key-spacing] ::: devtools/client/shared/test/unit/test_unicode-url.js:110 (Diff revision 1) > + expectedUnicode: "://xn--g6w.xn--8pv/%E6%B8%AC%E8%A9%A6.html" + > + "?%E4%B8%80=%E4%B8%80", > + }, > + { > + // %E8%A9 isn't an valid UTF-8 code, so this URL is malformed. > + raw: "https://xn--g6w.xn--8pv/%E6%B8%AC%E8%A9", Error: Extra space before value for key 'raw'. [eslint: key-spacing] ::: devtools/client/shared/test/unit/test_unicode-url.js:120 (Diff revision 1) > +// List of hostanmes used to test Unicode hostname conversion > +const TEST_HOSTNAMES = [ > + // Type: Readable ASCII hostnames > + // Expected: All of Unicode versions should equal to the raw. > + { > + raw: "example", Error: Extra space before value for key 'raw'. [eslint: key-spacing] ::: devtools/client/shared/test/unit/test_unicode-url.js:124 (Diff revision 1) > + { > + raw: "example", > + expectedUnicode: "example", > + }, > + { > + raw: "example.org", Error: Extra space before value for key 'raw'. [eslint: key-spacing] ::: devtools/client/shared/test/unit/test_unicode-url.js:130 (Diff revision 1) > + expectedUnicode: "example.org", > + }, > + // Type: Unreadable Punycode hostnames > + // Expected: Punycode should be converted to readable Unicode. > + { > + raw: "xn--g6w", Error: Extra space before value for key 'raw'. [eslint: key-spacing] ::: devtools/client/shared/test/unit/test_unicode-url.js:136 (Diff revision 1) > + // Do not type Unicode characters directly, because this test file isn't > + // specified with a known encoding. > + expectedUnicode: "\u6e2c", > + }, > + { > + raw: "xn--g6w.xn--8pv", Error: Extra space before value for key 'raw'. [eslint: key-spacing] ::: devtools/client/shared/test/unit/test_unicode-url.js:148 (Diff revision 1) > +// List of URL paths used to test Unicode URL path conversion > +const TEST_URL_PATHS = [ > + // Type: Readable ASCII URL paths > + // Expected: All of Unicode versions should equal to the raw. > + { > + raw: "test", Error: Extra space before value for key 'raw'. [eslint: key-spacing] ::: devtools/client/shared/test/unit/test_unicode-url.js:152 (Diff revision 1) > + { > + raw: "test", > + expectedUnicode: "test", > + }, > + { > + raw: "/", Error: Extra space before value for key 'raw'. [eslint: key-spacing] ::: devtools/client/shared/test/unit/test_unicode-url.js:156 (Diff revision 1) > + { > + raw: "/", > + expectedUnicode: "/", > + }, > + { > + raw: "/test", Error: Extra space before value for key 'raw'. [eslint: key-spacing] ::: devtools/client/shared/test/unit/test_unicode-url.js:160 (Diff revision 1) > + { > + raw: "/test", > + expectedUnicode: "/test", > + }, > + { > + raw: "/test.html?one=1&two=2#here", Error: Extra space before value for key 'raw'. [eslint: key-spacing] ::: devtools/client/shared/test/unit/test_unicode-url.js:166 (Diff revision 1) > + expectedUnicode: "/test.html?one=1&two=2#here", > + }, > + // Type: Unreadable URI-encoded URL paths > + // Expected: URL paths should be converted to readable Unicode. > + { > + raw: "/%E6%B8%AC%E8%A9%A6", Error: Extra space before value for key 'raw'. [eslint: key-spacing] ::: devtools/client/shared/test/unit/test_unicode-url.js:172 (Diff revision 1) > + // Do not type Unicode characters directly, because this test file isn't > + // specified with a known encoding. > + expectedUnicode: "/\u6e2c\u8a66", > + }, > + { > + raw: "/%E6%B8%AC%E8%A9%A6.html", Error: Extra space before value for key 'raw'. [eslint: key-spacing] ::: devtools/client/shared/test/unit/test_unicode-url.js:178 (Diff revision 1) > + // Do not type Unicode characters directly, because this test file isn't > + // specified with a known encoding. > + expectedUnicode: "/\u6e2c\u8a66.html", > + }, > + { > + raw: "/%E6%B8%AC%E8%A9%A6.html" + Error: Extra space before value for key 'raw'. [eslint: key-spacing] ::: devtools/client/shared/test/unit/test_unicode-url.js:191 (Diff revision 1) > + }, > + // Type: Malformed URL paths > + // Expected: All should not be converted. > + { > + // %E8%A9 isn't an valid UTF-8 code, so this URL is malformed. > + raw: "/%E6%B8%AC%E8%A9", Error: Extra space before value for key 'raw'. [eslint: key-spacing]
Attachment #8964154 - Attachment description: Screenshot1 before patching part 2.png → Screenshot1 before patching part 3.png
Attachment #8964155 - Attachment description: Screenshot2 before patching part 2.png → Screenshot2 before patching part 3.png
Attachment #8964156 - Attachment description: Screenshot1 after patching part 2.png → Screenshot1 after patching part 3.png
Attachment #8964157 - Attachment description: Screenshot2 after patching part 2.png → Screenshot2 after patching part 3.png
Attachment #8964152 - Attachment description: Screenshot after patching part 1.png → Screenshot after patching part 2.png
Seems Jordan Santell is inactive recently, so I hand off the review of web console to you two. And I also set the reviewer of all the newly added patches to you two, that means anyone of you is good, if one of you have time, then please help review. Thank you very much for your time of review!
Attachment #8964338 - Flags: review?(odvarko)
Attachment #8964338 - Flags: review?(ntim.bugs)
Attachment #8964338 - Flags: review?(nchevobbe)
Attachment #8964339 - Flags: review?(odvarko)
Attachment #8964339 - Flags: review?(ntim.bugs)
Attachment #8964339 - Flags: review?(nchevobbe)
Attachment #8964341 - Flags: review?(ntim.bugs) → review?(mratcliffe)
Attachment #8964342 - Flags: review?(odvarko)
Attachment #8964342 - Flags: review?(ntim.bugs)
Attachment #8964342 - Flags: review?(jdescottes)
Attachment #8964343 - Flags: review?(ntim.bugs)
Comment on attachment 8964338 [details] Bug 1448553 - Part 1: Defines common functions for getting a Unicode URL or a Unicode URL component https://reviewboard.mozilla.org/r/233060/#review238676 I only have a few nits. bit this looks good ::: devtools/client/shared/unicode-url.js:53 (Diff revision 1) > + } catch (_) { > + } I wonder if we should log a warning or something when decodeURIComponent throws. Do you have an idea when this could happen ? ::: devtools/client/shared/unicode-url.js:70 (Diff revision 1) > + * unreadable URI-encoded pathname, such as > + * http://xn--g6w.xn--8pv/%E8%A9%A6/%E6%B8%AC.js, then this function will return > + * the readable URL by docoding all its unreadable URL components to Unicode > + * characters. > + * > + * If the `url` is a malformed URL, then this function will the original `url`. nit: s/then this function will the original/then this function will return the original ::: devtools/client/shared/unicode-url.js:82 (Diff revision 1) > + * this function if the `url` itself is readable. > + */ > +function getUnicodeUrl(url) { > + try { > + const hostname = new URL(url).hostname; > + const readableHostname = idnService.convertToDisplayIDN(hostname, {}); nit: Maybe we could call getUnicodeHostname ? ::: devtools/client/shared/unicode-url.js:85 (Diff revision 1) > + } catch (_) { > + } maybe we should log a warning here ::: devtools/client/shared/unicode-url.js:90 (Diff revision 1) > +exports.getUnicodeHostname = getUnicodeHostname; > +exports.getUnicodeUrlPath = getUnicodeUrlPath; > +exports.getUnicodeUrl = getUnicodeUrl; nit: I find using module.exports a bit nicer ```js module.exports = { getUnicodeHostname, getUnicodeUrlPath, getUnicodeUrl, }; ```
Attachment #8964338 - Flags: review?(nchevobbe) → review+
Comment on attachment 8964339 [details] Bug 1448553 - Part 2: Decodeds Punycode-encoded international domain names and URI-encoded filenames in the Web Console so that they are displayed as human-readable Unicode text. https://reviewboard.mozilla.org/r/233062/#review238678 This looks good to me, thanks. Could we add mochitests for the panels using this component (webconsole, netmonitor) so we don't regress on this ?
Comment on attachment 8964338 [details] Bug 1448553 - Part 1: Defines common functions for getting a Unicode URL or a Unicode URL component https://reviewboard.mozilla.org/r/233060/#review238686 ::: devtools/client/shared/unicode-url.js:53 (Diff revision 1) > + } catch (_) { > + } This could happen if the user is going to visit a URL with an invalid UTF-8 code. For example, http://example.org/%E8%A9%A6.html is a valid URL, but http://example.org/%E8%A9.html is invalid, because %E8%A9 is not a valid UTF-8 code.
Comment on attachment 8964338 [details] Bug 1448553 - Part 1: Defines common functions for getting a Unicode URL or a Unicode URL component https://reviewboard.mozilla.org/r/233060/#review238676 > I wonder if we should log a warning or something when decodeURIComponent throws. Do you have an idea when this could happen ? This could happen if the user is going to visit a URL with an invalid UTF-8 code. For example, http://example.org/%E8%A9%A6.html is a valid URL, but http://example.org/%E8%A9.html is invalid, because %E8%A9 is not a valid UTF-8 code. > nit: s/then this function will the original/then this function will return the original Fixed. > nit: Maybe we could call getUnicodeHostname ? Good point. > maybe we should log a warning here Fixed > nit: I find using module.exports a bit nicer > > ```js > module.exports = { > getUnicodeHostname, > getUnicodeUrlPath, > getUnicodeUrl, > }; > ``` Okay, changed.
(In reply to Nicolas Chevobbe [:nchevobbe] from comment #43) > Comment on attachment 8964339 [details] > Bug 1448553 - Part 2: Decodeds Punycode-encoded international domain names > and URI-encoded filenames in the Web Console so that they are displayed as > human-readable Unicode text. > > https://reviewboard.mozilla.org/r/233062/#review238678 > > This looks good to me, thanks. > Could we add mochitests for the panels using this component (webconsole, > netmonitor) so we don't regress on this ? Okay, will do it.
Comment on attachment 8964338 [details] Bug 1448553 - Part 1: Defines common functions for getting a Unicode URL or a Unicode URL component https://reviewboard.mozilla.org/r/233060/#review238704 ::: devtools/client/shared/unicode-url.js:81 (Diff revision 1) > + * @return {string} The readable URL. It may be the same as the `url` passed to > + * this function if the `url` itself is readable. > + */ > +function getUnicodeUrl(url) { > + try { > + const hostname = new URL(url).hostname; nit: const { hostname } = new URL(url);
Comment on attachment 8964338 [details] Bug 1448553 - Part 1: Defines common functions for getting a Unicode URL or a Unicode URL component https://reviewboard.mozilla.org/r/233060/#review238700 ::: devtools/client/shared/unicode-url.js:67 (Diff revision 1) > + * then this function will simply return the original `url`. > + * > + * If the `url` includes either an unreadable Punycode domain name or an > + * unreadable URI-encoded pathname, such as > + * http://xn--g6w.xn--8pv/%E8%A9%A6/%E6%B8%AC.js, then this function will return > + * the readable URL by docoding all its unreadable URL components to Unicode nit: docoding -> decoding
Comment on attachment 8964342 [details] Bug 1448553 - Part 5: Decodeds Punycode-encoded international domain names and URI-encoded filenames in the Developer Toolbox's frame menu so that they are displayed as human-readable Unicode text. https://reviewboard.mozilla.org/r/233068/#review238710 ::: devtools/client/framework/toolbox.js:2219 (Diff revision 1) > - > if (this.target.isWebExtension) { > // Show a shorter url for extensions page. > label = this.target.getExtensionPathName(frame.url); > + } else { > + label = getUnicodeUrl(frame.url); Should this spot be updated as well: https://searchfox.org/mozilla-central/rev/f5fb323246bf22a3a3b4185882a1c5d8a2c02996/devtools/client/framework/toolbox.js#2129-2130 ?
Attachment #8964342 - Flags: review?(jdescottes) → review+
Comment on attachment 8964342 [details] Bug 1448553 - Part 5: Decodeds Punycode-encoded international domain names and URI-encoded filenames in the Developer Toolbox's frame menu so that they are displayed as human-readable Unicode text. https://reviewboard.mozilla.org/r/233068/#review238710 > Should this spot be updated as well: > https://searchfox.org/mozilla-central/rev/f5fb323246bf22a3a3b4185882a1c5d8a2c02996/devtools/client/framework/toolbox.js#2129-2130 > > ? Good catch. Fixed. Will upload a new patch in a while together with the other updated patches for this bug.
Comment on attachment 8964341 [details] Bug 1448553 - Part 4: Decodeds Punycode-encoded international domain names in the Storage Inspector so that they are displayed as human-readable Unicode text. https://reviewboard.mozilla.org/r/233066/#review238714
Attachment #8964341 - Flags: review?(mratcliffe) → review+
These updated patches fixed all the opened issues except mochitests. I'll upload mochitests later.
Zhang, it seems like you need to change the reviewers (as I changed them earlier) in your commit messages: Part 1 & 2: nchevobbe Part 3: Honza Part 4: miker Part 5: jdescottes Part 6: nchevobbe
Attachment #8964338 - Flags: review?(odvarko)
Attachment #8964338 - Flags: review?(ntim.bugs)
Attachment #8964339 - Flags: review?(odvarko)
Attachment #8964339 - Flags: review?(ntim.bugs)
Attachment #8964341 - Flags: review?(ntim.bugs)
Attachment #8964342 - Flags: review?(odvarko)
Attachment #8964342 - Flags: review?(ntim.bugs)
Attachment #8964343 - Flags: review?(odvarko)
Attachment #8964343 - Flags: review?(ntim.bugs)
Attachment #8964343 - Flags: review?(nchevobbe)
(In reply to Tim Nguyen :ntim from comment #59) > Zhang, it seems like you need to change the reviewers (as I changed them > earlier) in your commit messages: > > Part 1 & 2: nchevobbe > Part 3: Honza > Part 4: miker > Part 5: jdescottes > Part 6: nchevobbe Thank you. I didn't know that. I thought the r? fields in histedited patches wouldn't affect the current reviewers.
Comment on attachment 8964343 [details] Bug 1448553 - Part 6: Adds xpcshell-based unit tests for the common Unicode-URL related functions defined in Part 1 https://reviewboard.mozilla.org/r/233070/#review239080 A few nits but this looks good to me ::: devtools/client/shared/test/unit/test_unicode-url.js:54 (Diff revision 3) > + { > + raw: "https://example.org/test.html?one=1&two=2#here", > + expectedUnicode: "https://example.org/test.html?one=1&two=2#here", > + }, could we add tests for data: URIs ? ::: devtools/client/shared/test/unit/test_unicode-url.js:203 (Diff revision 3) > + for (let url of TEST_URLS) { > + let result = getUnicodeUrl(url.raw); > + equal(result, url.expectedUnicode, > + "Test getUnicodeUrl: " + url.raw + > + " should be unicodized to " + url.expectedUnicode + > + ", but the result is " + result); i don't think we need this last line. If the test fail, it will tell us what it was expecting and what it got ::: devtools/client/shared/test/unit/test_unicode-url.js:212 (Diff revision 3) > + for (let hostname of TEST_HOSTNAMES) { > + let result = getUnicodeHostname(hostname.raw); > + equal(result, hostname.expectedUnicode, > + "Test getUnicodeHostname: " + hostname.raw + > + " should be unicodized to " + hostname.expectedUnicode + > + ", but the result is " + result); ditto ::: devtools/client/shared/test/unit/test_unicode-url.js:221 (Diff revision 3) > + for (let urlPath of TEST_URL_PATHS) { > + let result = getUnicodeUrlPath(urlPath.raw); > + equal(result, urlPath.expectedUnicode, > + "Test getUnicodeUrlPath: " + urlPath.raw + > + " should be unicodized to " + urlPath.expectedUnicode + > + ", but the result is " + result); ditto
Attachment #8964343 - Flags: review?(nchevobbe) → review+
Comment on attachment 8964343 [details] Bug 1448553 - Part 6: Adds xpcshell-based unit tests for the common Unicode-URL related functions defined in Part 1 https://reviewboard.mozilla.org/r/233070/#review239080 > could we add tests for data: URIs ? getUnicodeUrl is not intended to be used for a data URI. Base64 URIs are fine because they don't contain %, but textual data URIs would be broken. So I don't think it's correct to call it for a data URIs. Maybe I should document part 1 to make it clear. > i don't think we need this last line. If the test fail, it will tell us what it was expecting and what it got Good point.
Comment on attachment 8964340 [details] Bug 1448553 - Part 3: Decodeds Punycode-encoded international domain names in the Network Monitor so that they are displayed as human-readable Unicode text. https://reviewboard.mozilla.org/r/233064/#review239116 Thanks for working on this! The patch looks good in genereal, I am just worried about the support for the DevTools Launchpad. See my inline comment Honza ::: devtools/client/netmonitor/src/utils/request-utils.js:10 (Diff revision 2) > /* eslint-disable mozilla/reject-some-requires */ > > "use strict"; > > +const { getUnicodeUrl, getUnicodeUrlPath, getUnicodeHostname } = > + require("devtools/client/shared/unicode-url"); This require loads a module that uses chrome API, specifically: const idnService = Cc["@mozilla.org/network/idn-service;1"].getService(Ci.nsIIDNService); The Network monitor panel related code (UI) is currently chrome API free - using only standard (web) content API, and so it can run within a tab (read more about the Launchpad [1]) I think that we should use the same technique as e.g. for the `open-request-in-tab.js` module in the Network panel. There are two versions of that module (two version, two implementations): devtools/client/netmonitor/src/utils/open-request-in-tab.js devtools/client/netmonitor/src/utils/firefox/open-request-in-tab.js The first chrome API free The second using chrome API [1] https://searchfox.org/mozilla-central/source/devtools/client/netmonitor/README.md So, I think there should be two version of the `unicode-url.js` module. This might be done as a follow up, but we should at least track this, since it breaks the Launchpad. Honza
@Nicolas, what's do you think about the Launchpad support? See my previous comment. I know the support for Launchpad is not the top priority atm, but I think that we should at least track issues. Honza
Flags: needinfo?(nchevobbe)
(In reply to Jan Honza Odvarko [:Honza] from comment #69) > Comment on attachment 8964340 [details] > Bug 1448553 - Part 3: Decodeds Punycode-encoded international domain names > in the Network Monitor so that they are displayed as human-readable Unicode > text. > > https://reviewboard.mozilla.org/r/233064/#review239116 > > Thanks for working on this! > > The patch looks good in genereal, I am just worried about the support for > the DevTools Launchpad. > See my inline comment > > Honza > > ::: devtools/client/netmonitor/src/utils/request-utils.js:10 > (Diff revision 2) > > /* eslint-disable mozilla/reject-some-requires */ > > > > "use strict"; > > > > +const { getUnicodeUrl, getUnicodeUrlPath, getUnicodeHostname } = > > + require("devtools/client/shared/unicode-url"); > > This require loads a module that uses chrome API, specifically: > > const idnService = > Cc["@mozilla.org/network/idn-service;1"].getService(Ci.nsIIDNService); > > The Network monitor panel related code (UI) is currently chrome API free - > using only standard (web) content API, and so it can run within a tab (read > more about the Launchpad [1]) > > I think that we should use the same technique as e.g. for the > `open-request-in-tab.js` module in the Network panel. > > There are two versions of that module (two version, two implementations): > devtools/client/netmonitor/src/utils/open-request-in-tab.js > devtools/client/netmonitor/src/utils/firefox/open-request-in-tab.js > > The first chrome API free > The second using chrome API > > [1] > https://searchfox.org/mozilla-central/source/devtools/client/netmonitor/ > README.md > > So, I think there should be two version of the `unicode-url.js` module. > > This might be done as a follow up, but we should at least track this, since > it breaks the Launchpad. > > Honza Thank you for the information. If we are going to implement a Chrome API free version of the `unicode-url.js`, I would suggest we use a ready-made Punycode decoder instead of writing one from scratch, as there are time-tested robust Javascript implementations. https://github.com/bestiejs/punycode.js This one seems practicable from the programming perspective, but it uses the MIT License, I have not enough knowledge on these licenses, so I am not sure if it's allowed to be included in a MPL project.
(In reply to Jan Honza Odvarko [:Honza] from comment #70) > @Nicolas, what's do you think about the Launchpad support? See my previous > comment. > I know the support for Launchpad is not the top priority atm, but I think > that we should at least track issues. > > Honza I think I would opt for a mock of this module, where each function returns what it was passed. The mock should then be declared in the webpack config. Given the status of the launchpad at the moment, I'd rather not look too much for a feature-parity replacement. We can always fix it later if we feel like the launchpad workflow is important for us.
Flags: needinfo?(nchevobbe)
(In reply to Zhang Junzhi from comment #71) > If we are going to implement a Chrome API free version of the > `unicode-url.js`, I would suggest we use a ready-made Punycode decoder > instead of writing one from scratch, as there are time-tested robust > Javascript implementations. I agree with Nicolas. Having empty mock of the module would be good enough for me too + file a follow up, so we can track the rest. Honza
(In reply to Jan Honza Odvarko [:Honza] from comment #73) > (In reply to Zhang Junzhi from comment #71) > > If we are going to implement a Chrome API free version of the > > `unicode-url.js`, I would suggest we use a ready-made Punycode decoder > > instead of writing one from scratch, as there are time-tested robust > > Javascript implementations. > > I agree with Nicolas. Having empty mock of the module would be good > enough for me too + file a follow up, so we can track the rest. > > Honza A mock module has been added.
(In reply to Nicolas Chevobbe [:nchevobbe] from comment #72) > (In reply to Jan Honza Odvarko [:Honza] from comment #70) > > @Nicolas, what's do you think about the Launchpad support? See my previous > > comment. > > I know the support for Launchpad is not the top priority atm, but I think > > that we should at least track issues. > > > > Honza > > I think I would opt for a mock of this module, where each function returns > what it was passed. The mock should then be declared in the webpack config. > Given the status of the launchpad at the moment, I'd rather not look too > much for a feature-parity replacement. We can always fix it later if we feel > like the launchpad workflow is important for us. I found a bug in part 1: The "Filter output" input couldn't correctly filter Unicode filenames, actually it still filtered filename by URI-encoded filenames even the filename was displayed as Unicode. Part 1 of this commit fixed this bug, now that the "Filter output" input works for Unicode filenames.
(In reply to Nicolas Chevobbe [:nchevobbe] from comment #72) > (In reply to Jan Honza Odvarko [:Honza] from comment #70) > > @Nicolas, what's do you think about the Launchpad support? See my previous > > comment. > > I know the support for Launchpad is not the top priority atm, but I think > > that we should at least track issues. > > > > Honza > > I think I would opt for a mock of this module, where each function returns > what it was passed. The mock should then be declared in the webpack config. > Given the status of the launchpad at the moment, I'd rather not look too > much for a feature-parity replacement. We can always fix it later if we feel > like the launchpad workflow is important for us. Sorry, typo. I meant part 2 when I said part 1.
Found one unnecessary newline I made in part 2, patches pushed again. Please be aware I pushed twice in several minutes.
Talking about mochitest, I have difficulty figuring out how to do mochitest for Unicode filenames. Since mach is buggy for handling Unicode filenames(maybe as is mochitest), so a file with a Unicode filename cannot be considered, so I am wondering how we can mochitest a URL request with a Unicode filename? Right now, I haven't found there is any ready-made mechanism which simulates a Unicode URL with a Unicode filename, I wonder if there is one, if not, do we need to implement such functionality in some underlying code?
(In reply to Zhang Junzhi from comment #88) > Talking about mochitest, I have difficulty figuring out how to do mochitest > for Unicode filenames. > > Since mach is buggy for handling Unicode filenames(maybe as is mochitest), > so a file with a Unicode filename cannot be considered, so I am wondering > how we can mochitest a URL request with a Unicode filename? Right now, I > haven't found there is any ready-made mechanism which simulates a Unicode > URL with a Unicode filename, I wonder if there is one, if not, do we need to > implement such functionality in some underlying code? build/pgo/server-locations.txt has some non-ascii host names you can use. If you need a non-ascii path name then you can probably do that using an sjs file, see https://developer.mozilla.org/en-US/docs/Mozilla/httpd.js/HTTP_server_for_unit_tests
(In reply to Tom Tromey :tromey from comment #89) > (In reply to Zhang Junzhi from comment #88) > > Talking about mochitest, I have difficulty figuring out how to do mochitest > > for Unicode filenames. > > > > Since mach is buggy for handling Unicode filenames(maybe as is mochitest), > > so a file with a Unicode filename cannot be considered, so I am wondering > > how we can mochitest a URL request with a Unicode filename? Right now, I > > haven't found there is any ready-made mechanism which simulates a Unicode > > URL with a Unicode filename, I wonder if there is one, if not, do we need to > > implement such functionality in some underlying code? > > build/pgo/server-locations.txt has some non-ascii host names you can use. > If you need a non-ascii path name then you can probably do that using an sjs > file, see > https://developer.mozilla.org/en-US/docs/Mozilla/httpd.js/ > HTTP_server_for_unit_tests Thank you very much for the information. Server-side script is really what I am looking for.
Mochitests have been added for the Web Console(Testing ). The changes in the Network Monitor and the Storage Inspector actually only changes text labels. No functionality invovled. We just need to make sure we're not breaking the exisiting mochitests in the two devtools, and I think it's enough.
BTW, the Network Monitor had supported the Unicode filename before this bug, but it was buggy for filtering the Unicode filename, that is, it already was broken before this bug, so I'll file a new bug for fixing that bug instead of fixing it in this one.
From some results of mochitests broken by this commit, I found that URI datas are possible to be passed in some cases, so I added support for the data: URIs in getUnicodeUrl, and also added xpcshell-based unit tests for data: URIs.
Comment on attachment 8964340 [details] Bug 1448553 - Part 3: Decodeds Punycode-encoded international domain names in the Network Monitor so that they are displayed as human-readable Unicode text. https://reviewboard.mozilla.org/r/233064/#review239970 Code analysis found 1 defect in this patch: - 1 defect found by mozlint You can run this analysis locally with: - `./mach lint path/to/file` (JS/Python) If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx ::: devtools/client/shared/unicode-url.js:97 (Diff revision 7) > * this function if the `url` itself is readable. > */ > function getUnicodeUrl(url) { > try { > const { protocol, hostname } = new URL(url); > if ("data:" === protocol) { Error: Expected literal to be on the right side of ===. [eslint: yoda]
Comment on attachment 8964338 [details] Bug 1448553 - Part 1: Defines common functions for getting a Unicode URL or a Unicode URL component https://reviewboard.mozilla.org/r/233060/#review240014 ::: devtools/client/shared/unicode-url.js:88 (Diff revision 5) > + * this function if the `url` itself is readable. > + */ > +function getUnicodeUrl(url) { > + try { > + const { protocol, hostname } = new URL(url); > + if (protocol === "data:") { nit: maybe we can move the check earlier (and outside of the try/catch block) ? Would the following work ? ```js if (url.startsWith("data:")) { return url; } ```
Attachment #8964338 - Flags: review+
Comment on attachment 8964339 [details] Bug 1448553 - Part 2: Decodeds Punycode-encoded international domain names and URI-encoded filenames in the Web Console so that they are displayed as human-readable Unicode text. https://reviewboard.mozilla.org/r/233062/#review240020 Seems good to me
Attachment #8964339 - Flags: review?(nchevobbe) → review+
Comment on attachment 8965384 [details] Bug 1448553 - Part 7: Adds mochitests to test the feature of filtering Unicode strings and Unicode filenames in the Web Console. https://reviewboard.mozilla.org/r/234124/#review240022 ::: devtools/client/webconsole/test/browser_webconsole_log_file_filter.js:6 (Diff revision 4) > -// Tests that the text filter box works to filter based on filenames > -// where the logs were generated. > +// Tests that the text filter box works to filter based on filenames where the > +// logs were generated. These tests are for bug 923281 and bug 1448553. So here we are in one of the old-frontend test, which will be removed in the coming month. You might want to modify devtools/client/webconsole/new-console-output/test/mochitest/browser_webconsole_filters.js instead, or create a new test in this folder.
Attachment #8965384 - Flags: review?(nchevobbe) → review-
(In reply to Nicolas Chevobbe [:nchevobbe] from comment #115) > Comment on attachment 8964338 [details] > Bug 1448553 - Part 1: Defines common functions for getting a Unicode URL or > a Unicode URL component > > https://reviewboard.mozilla.org/r/233060/#review240014 > > ::: devtools/client/shared/unicode-url.js:88 > (Diff revision 5) > > + * this function if the `url` itself is readable. > > + */ > > +function getUnicodeUrl(url) { > > + try { > > + const { protocol, hostname } = new URL(url); > > + if (protocol === "data:") { > > nit: maybe we can move the check earlier (and outside of the try/catch > block) ? > Would the following work ? > ```js > if (url.startsWith("data:")) { > return url; > } > ``` Do we need to consider the case where the data scheme is uppercased(E.g. DATA: or Data:)? If so, then we need to make a lowercased or uppercased conversion before calling startsWith("data:"). Since the function is used mostly for URLs, not for data: URIs, it would decrease the performance for URL conversion in this function. So I would like to suggest we keep the original code.
(In reply to Nicolas Chevobbe [:nchevobbe] from comment #117) > Comment on attachment 8965384 [details] > Bug 1448553 - Part 7: Adds mochitests to test the feature of filtering > Unicode strings and Unicode filenames the Web Console. > > https://reviewboard.mozilla.org/r/234124/#review240022 > > ::: devtools/client/webconsole/test/browser_webconsole_log_file_filter.js:6 > (Diff revision 4) > > -// Tests that the text filter box works to filter based on filenames > > -// where the logs were generated. > > +// Tests that the text filter box works to filter based on filenames where the > > +// logs were generated. These tests are for bug 923281 and bug 1448553. > > So here we are in one of the old-frontend test, which will be removed in the > coming month. > You might want to modify > devtools/client/webconsole/new-console-output/test/mochitest/ > browser_webconsole_filters.js instead, or create a new test in this folder. Okay, I will modify browser_webconsole_filters.js instead
Comment on attachment 8964340 [details] Bug 1448553 - Part 3: Decodeds Punycode-encoded international domain names in the Network Monitor so that they are displayed as human-readable Unicode text. https://reviewboard.mozilla.org/r/233064/#review240050 Thanks for the patch! Looks good to me. Just one thing, please move/rename the mockup module: from: devtools/client/netmonitor/src/utils/unicode-url.js to devtools/client/shared/webpack/shims/unicode-url-stub.js This way, it'll be ready to be used by other panels too (not only the Network panel can run on top of the Launchpad). R+ assuming the above is resolved and try is green. Thanks, Honza ::: devtools/client/netmonitor/src/utils/firefox/open-request-in-tab.js:13 (Diff revision 8) > +// take advantage of utilizing chrome APIs. But because of this, it isn't > +// intended to be used in Chrome-API-free applications, such as the Launchpad. > +// > +// Please keep in mind that if the feature in this file has changed, don't > +// forget to also change that accordingly in > +// devtools/client/netmonitor/src/utils/open-request-in-tab.js. Thanks for the comment!
Attachment #8964340 - Flags: review?(odvarko) → review+
(In reply to Jan Honza Odvarko [:Honza] from comment #120) > Comment on attachment 8964340 [details] > Bug 1448553 - Part 3: Decodeds Punycode-encoded international domain names > in the Network Monitor so that they are displayed as human-readable Unicode > text. > > https://reviewboard.mozilla.org/r/233064/#review240050 > > Thanks for the patch! > > Looks good to me. Just one thing, please move/rename the mockup module: > > from: > devtools/client/netmonitor/src/utils/unicode-url.js > > to > devtools/client/shared/webpack/shims/unicode-url-stub.js > > This way, it'll be ready to be used by other panels too (not only the > Network panel can run on top of the Launchpad). > > R+ assuming the above is resolved and try is green. > > Thanks, > Honza > > ::: devtools/client/netmonitor/src/utils/firefox/open-request-in-tab.js:13 > (Diff revision 8) > > +// take advantage of utilizing chrome APIs. But because of this, it isn't > > +// intended to be used in Chrome-API-free applications, such as the Launchpad. > > +// > > +// Please keep in mind that if the feature in this file has changed, don't > > +// forget to also change that accordingly in > > +// devtools/client/netmonitor/src/utils/open-request-in-tab.js. > > Thanks for the comment! Renamed, will be upload a while later.
(In reply to Nicolas Chevobbe [:nchevobbe] from comment #117) > Comment on attachment 8965384 [details] > Bug 1448553 - Part 7: Adds mochitests to test the feature of filtering > Unicode strings and Unicode filenames the Web Console. > > https://reviewboard.mozilla.org/r/234124/#review240022 > > ::: devtools/client/webconsole/test/browser_webconsole_log_file_filter.js:6 > (Diff revision 4) > > -// Tests that the text filter box works to filter based on filenames > > -// where the logs were generated. > > +// Tests that the text filter box works to filter based on filenames where the > > +// logs were generated. These tests are for bug 923281 and bug 1448553. > > So here we are in one of the old-frontend test, which will be removed in the > coming month. > You might want to modify > devtools/client/webconsole/new-console-output/test/mochitest/ > browser_webconsole_filters.js instead, or create a new test in this folder. Could I simply move devtools/client/webconsole/test/browser_webconsole_log_file_filter.js to devtools/client/webconsole/new-console-output/test/mochitest/browser_webconsole_filename_filters.js ? After looking into the code in browser_webconsole_file_filters.js, I believe it is just used to test filtering Javascript's outputs, not filenames. But the old browser_webconsole_log_file_filter.js is used to test filtering filenames(I.e. used to test bug 923281, which proposed adding a feature for searching filenames). So, the two files serve different purposes. So I suggest simply moving devtools/client/webconsole/test/browser_webconsole_log_file_filter.js to the new directory.
Of course, the old APIs in this file, if any, will be changed to new APIs.
> Could I simply move devtools/client/webconsole/test/browser_webconsole_log_file_filter.js to devtools/client/webconsole/new-console-output/test/mochitest/browser_webconsole_filename_filters.js ? Sure, as long as we keep a consistent style with the other tests from this directory.
Issues mentioned above have all been addressed.
Comment on attachment 8964340 [details] Bug 1448553 - Part 3: Decodeds Punycode-encoded international domain names in the Network Monitor so that they are displayed as human-readable Unicode text. https://reviewboard.mozilla.org/r/233064/#review240244 Code analysis found 1 defect in this patch: - 1 defect found by mozlint You can run this analysis locally with: - `./mach lint path/to/file` (JS/Python) If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx ::: devtools/client/netmonitor/webpack.config.js:92 (Diff revision 9) > > "devtools/shared/event-emitter": "devtools-modules/src/utils/event-emitter", > "devtools/shared/fronts/timeline": path.join(__dirname, "../../client/shared/webpack/shims/fronts-timeline-shim"), > "devtools/shared/platform/clipboard": path.join(__dirname, "../../client/shared/webpack/shims/platform-clipboard-stub"), > "devtools/client/netmonitor/src/utils/firefox/open-request-in-tab": path.join(__dirname, "src/utils/open-request-in-tab"), > + "devtools/client/shared/unicode-url": path.join(__dirname, "../../client/shared/webpack/shims/unicode-url-stub"), Error: Duplicate key 'devtools/client/shared/unicode-url'. [eslint: no-dupe-keys]
(In reply to Code Review Bot [:reviewbot] from comment #138) > Comment on attachment 8964340 [details] > Bug 1448553 - Part 3: Decodeds Punycode-encoded international domain names > in the Network Monitor so that they are displayed as human-readable Unicode > text. > > https://reviewboard.mozilla.org/r/233064/#review240244 > > > Code analysis found 1 defect in this patch: > - 1 defect found by mozlint > > You can run this analysis locally with: > - `./mach lint path/to/file` (JS/Python) > > > If you see a problem in this automated review, please report it here: > http://bit.ly/2y9N9Vx > > > ::: devtools/client/netmonitor/webpack.config.js:92 > (Diff revision 9) > > > > "devtools/shared/event-emitter": "devtools-modules/src/utils/event-emitter", > > "devtools/shared/fronts/timeline": path.join(__dirname, "../../client/shared/webpack/shims/fronts-timeline-shim"), > > "devtools/shared/platform/clipboard": path.join(__dirname, "../../client/shared/webpack/shims/platform-clipboard-stub"), > > "devtools/client/netmonitor/src/utils/firefox/open-request-in-tab": path.join(__dirname, "src/utils/open-request-in-tab"), > > + "devtools/client/shared/unicode-url": path.join(__dirname, "../../client/shared/webpack/shims/unicode-url-stub"), > > Error: Duplicate key 'devtools/client/shared/unicode-url'. [eslint: > no-dupe-keys] This one has been fixed, I pushed twice, and this one relates to the former push.
Jan, FYI, Please note that the unicode-url-stub.js is not removed(I originally added it in part 3, but now I added in part 2 instead of part 3, that's because webpack.config.js in the Web Console also needs it.), but due to the limitation of the review diff, it "mistakenly" shows as if the file is removed.
I just triggered dt and dt-e10s mochitests on all Linux64 Once if the try is all green, I'll continue to trigger mochitests on all platforms.
Try is all green except two timed-out failures, no functionality-related failure: https://treeherder.mozilla.org/#/jobs?repo=try&revision=8af14f5dcb155bd65c8b20b78aa77f7f790bdd81
Comment on attachment 8965384 [details] Bug 1448553 - Part 7: Adds mochitests to test the feature of filtering Unicode strings and Unicode filenames in the Web Console. https://reviewboard.mozilla.org/r/234124/#review240474 Thanks for adding the test. However, it seems that this is only testing the output, and not the filtering per-se. We should make sure that filtering work as expected. Also, let's not move the files but copy the ones we need. ::: devtools/client/webconsole/test/browser.ini (Diff revision 8) > - test-bug_923281_console_log_filter.html > - test-bug_923281_test1.js > - test-bug_923281_test2.js we shouldn't make change here. Could we copy the test file (with `hg cp`) instead of moving it ? ::: devtools/client/webconsole/new-console-output/test/mochitest/browser_webconsole_filename_filters.js:18 (Diff revision 8) > - hud = yield openConsole(); > - yield consoleOpened(); > > - testLiveFilteringOnSearchStrings(); > - > - hud = null; > +const TEST_ASCII_URI = "http://example.com/browser/devtools/client/" + > + "webconsole/new-console-output/test/mochitest/" + > + "test-bug_923281_console_filename_filter.html"; we try to not have the bug number in the filename anymore. Could you rename it when doing the copy ? ::: devtools/client/webconsole/new-console-output/test/mochitest/browser_webconsole_filename_filters.js:25 (Diff revision 8) > - setStringFilter("random"); > - is(countMessageNodes(), 1, "the log nodes not containing string " + > + is(findMessages(hud, "random").length, 1, > + "the log not containing string \"random\" are hidden"); > - "\"random\" are hidden"); > > - setStringFilter("test2.js"); > - is(countMessageNodes(), 2, "show only log nodes containing string " + > + is(findMessages(hud, "test2.js").length, 2, > + "show only logs containing string " + > - "\"test2.js\" or log nodes created from files with filename " + > + "\"test2.js\" or log nodes created from files with filename " + > - "containing \"test2.js\" as substring."); > + "containing \"test2.js\" as substring."); > > - setStringFilter("test1"); > - is(countMessageNodes(), 2, "show only log nodes containing string " + > + is(findMessages(hud, "test1").length, 2, > + "show only logs containing string " + > - "\"test1\" or log nodes created from files with filename " + > + "\"test1\" or log nodes created from files with filename " + > - "containing \"test1\" as substring."); > + "containing \"test1\" as substring."); I don't understand those assertions: we don't do any filtering between them, and I find it hard to follow what should be filtered or not from this function alone ::: devtools/client/webconsole/new-console-output/test/mochitest/browser_webconsole_filename_filters.js:42 (Diff revision 8) > - return displayedMessageNodes; > -} > +// There are three hanzi: > +// The `HANZI_1` is used both as a Unicode filename and a console log. > +// The `HANZI_2` is used only as a console log. > + > +// The value of the `uri` property is used as a URI-encoded form in a URL. > +// The value of the `js` is used as a Javascript string form. > +const HANZI_1 = { > + uriEncoded: "%E6%B8%AC", > + js: "\u6e2c", > + escapedJs: "\\u6e2c", > +}; > + > +// A hanzi in two forms. > +// The value of the `js` is used to in Javascript string. > +const HANZI_2 = { > + js: "\u8a66", > + escapedJs: "\\u8a66", > +}; > + > +const RESPONSE_DATA = `<!DOCTYPE html > +><meta charset="utf-8" > +><title>Test for filtering a Unicode filename</title > +><script> > +console.log("This log has no Unicode character."); > +console.log("This log has unicode: ${HANZI_1.escapedJs}"); > +console.log("This log has unicode: ${HANZI_2.escapedJs}"); > +</script>`; could this be moved to the top of the file please ? ::: devtools/client/webconsole/new-console-output/test/mochitest/browser_webconsole_filename_filters.js:106 (Diff revision 8) > +add_task(async function() { > + // Testcase 1: Test filtering logs with an ASCII filename. > + hud = await openNewTabAndConsole(TEST_ASCII_URI); > + testFilteringOutputsWithAsciiFilename(); > + > + // Testcase 2: Test filtering logs with a URI-encoded filename. > + hud = await openNewTabAndConsole(TEST_URI_ENCODED_URL); > + testFilteringOutputsWithUnicodeFilename(); > > -function setStringFilter(value) { > - hud.ui.filterBox.value = value; > + hud = null; > +}); Could we move this as the top of the file, since it the block that describes best what is happening in this test ? ::: devtools/client/webconsole/new-console-output/test/mochitest/browser_webconsole_filename_filters.js:108 (Diff revision 8) > + hud = await openNewTabAndConsole(TEST_ASCII_URI); > + testFilteringOutputsWithAsciiFilename(); > + > + // Testcase 2: Test filtering logs with a URI-encoded filename. > + hud = await openNewTabAndConsole(TEST_URI_ENCODED_URL); > + testFilteringOutputsWithUnicodeFilename(); Unless I'm missing something, the doesn't seem to test message filtering at all, but only that the message are displayed as expected. The test should be completed to make sure that filtering does work as intended.
Attachment #8965384 - Flags: review?(nchevobbe) → review-
(In reply to Nicolas Chevobbe [:nchevobbe] from comment #145) > Comment on attachment 8965384 [details] > Bug 1448553 - Part 7: Adds mochitests to test the feature of filtering > Unicode strings and Unicode filenames in the Web Console. > > https://reviewboard.mozilla.org/r/234124/#review240474 > > Thanks for adding the test. However, it seems that this is only testing the > output, and not the filtering per-se. We should make sure that filtering > work as expected. > Also, let's not move the files but copy the ones we need. Filtering is tested in serveral other test files, this file is added to test only filtering logs with filenames. > > ::: devtools/client/webconsole/test/browser.ini > (Diff revision 8) > > - test-bug_923281_console_log_filter.html > > - test-bug_923281_test1.js > > - test-bug_923281_test2.js > > we shouldn't make change here. > Could we copy the test file (with `hg cp`) instead of moving it ? Sure, I'll add a file instead of moving it. > > ::: > devtools/client/webconsole/new-console-output/test/mochitest/ > browser_webconsole_filename_filters.js:18 > (Diff revision 8) > > - hud = yield openConsole(); > > - yield consoleOpened(); > > > > - testLiveFilteringOnSearchStrings(); > > - > > - hud = null; > > +const TEST_ASCII_URI = "http://example.com/browser/devtools/client/" + > > + "webconsole/new-console-output/test/mochitest/" + > > + "test-bug_923281_console_filename_filter.html"; > > we try to not have the bug number in the filename anymore. Could you rename > it when doing the copy ? Yes, will do it. > > ::: > devtools/client/webconsole/new-console-output/test/mochitest/ > browser_webconsole_filename_filters.js:25 > (Diff revision 8) > > - setStringFilter("random"); > > - is(countMessageNodes(), 1, "the log nodes not containing string " + > > + is(findMessages(hud, "random").length, 1, > > + "the log not containing string \"random\" are hidden"); > > - "\"random\" are hidden"); > > > > - setStringFilter("test2.js"); > > - is(countMessageNodes(), 2, "show only log nodes containing string " + > > + is(findMessages(hud, "test2.js").length, 2, > > + "show only logs containing string " + > > - "\"test2.js\" or log nodes created from files with filename " + > > + "\"test2.js\" or log nodes created from files with filename " + > > - "containing \"test2.js\" as substring."); > > + "containing \"test2.js\" as substring."); > > > > - setStringFilter("test1"); > > - is(countMessageNodes(), 2, "show only log nodes containing string " + > > + is(findMessages(hud, "test1").length, 2, > > + "show only logs containing string " + > > - "\"test1\" or log nodes created from files with filename " + > > + "\"test1\" or log nodes created from files with filename " + > > - "containing \"test1\" as substring."); > > + "containing \"test1\" as substring."); > > I don't understand those assertions: we don't do any filtering between them, > and I find it hard to follow what should be filtered or not from this > function alone They are used to test filtering filenames, not the output contents themselves. > > ::: > devtools/client/webconsole/new-console-output/test/mochitest/ > browser_webconsole_filename_filters.js:42 > (Diff revision 8) > > - return displayedMessageNodes; > > -} > > +// There are three hanzi: > > +// The `HANZI_1` is used both as a Unicode filename and a console log. > > +// The `HANZI_2` is used only as a console log. > > + > > +// The value of the `uri` property is used as a URI-encoded form in a URL. > > +// The value of the `js` is used as a Javascript string form. > > +const HANZI_1 = { > > + uriEncoded: "%E6%B8%AC", > > + js: "\u6e2c", > > + escapedJs: "\\u6e2c", > > +}; > > + > > +// A hanzi in two forms. > > +// The value of the `js` is used to in Javascript string. > > +const HANZI_2 = { > > + js: "\u8a66", > > + escapedJs: "\\u8a66", > > +}; > > + > > +const RESPONSE_DATA = `<!DOCTYPE html > > +><meta charset="utf-8" > > +><title>Test for filtering a Unicode filename</title > > +><script> > > +console.log("This log has no Unicode character."); > > +console.log("This log has unicode: ${HANZI_1.escapedJs}"); > > +console.log("This log has unicode: ${HANZI_2.escapedJs}"); > > +</script>`; > > could this be moved to the top of the file please ? Will do. > > ::: > devtools/client/webconsole/new-console-output/test/mochitest/ > browser_webconsole_filename_filters.js:106 > (Diff revision 8) > > +add_task(async function() { > > + // Testcase 1: Test filtering logs with an ASCII filename. > > + hud = await openNewTabAndConsole(TEST_ASCII_URI); > > + testFilteringOutputsWithAsciiFilename(); > > + > > + // Testcase 2: Test filtering logs with a URI-encoded filename. > > + hud = await openNewTabAndConsole(TEST_URI_ENCODED_URL); > > + testFilteringOutputsWithUnicodeFilename(); > > > > -function setStringFilter(value) { > > - hud.ui.filterBox.value = value; > > + hud = null; > > +}); > > Could we move this as the top of the file, since it the block that describes > best what is happening in this test ? Will do. > > ::: > devtools/client/webconsole/new-console-output/test/mochitest/ > browser_webconsole_filename_filters.js:108 > (Diff revision 8) > > + hud = await openNewTabAndConsole(TEST_ASCII_URI); > > + testFilteringOutputsWithAsciiFilename(); > > + > > + // Testcase 2: Test filtering logs with a URI-encoded filename. > > + hud = await openNewTabAndConsole(TEST_URI_ENCODED_URL); > > + testFilteringOutputsWithUnicodeFilename(); > > Unless I'm missing something, the doesn't seem to test message filtering at > all, but only that the message are displayed as expected. > > The test should be completed to make sure that filtering does work as > intended. Both `testFilteringOutputsWithAsciiFilename` and `testFilteringOutputsWithAsciiFilename` test filtering messages together with filenames. For example, HANZI_1 is used both in an output message and a filename, while HANZI_2 is used only in a filename. `test2.js` is a file, and is also a string in test1.js. But I'll refine the test cases to make them more understandable.
Typo. s/They are used to test filtering filenames, not the output contents themselves/They are used to test filtering filenames, and the output contents themselves
Sorry to insist Zhang, but I triple checked the patch again and I can't see where we do set a text filter (i.e. fill the filter input with the filenames we want to check). Previously this was done in `setStringFilter` , which was called several times. Here the `findMessages` call only assert what is on the output, not any text filter the user might set.
(In reply to Nicolas Chevobbe [:nchevobbe] from comment #148) > Sorry to insist Zhang, but I triple checked the patch again and I can't see > where we do set a text filter (i.e. fill the filter input with the filenames > we want to check). > Previously this was done in `setStringFilter` , which was called several > times. > Here the `findMessages` call only assert what is on the output, not any text > filter the user might set. Ah, I see what you meant. Sorry, I misundersood `findMessages` without looking into the code of `findMessages`. I thought it's used to fill the filter input, and after replacing `findMessages` with `setStringFilter`, I got no assertion failure, which made me believe what I thought was right.
s/replacing `findMessages` with `setStringFilter`/replacing `setStringFilter` with `findMessages`
Attachment #8965384 - Attachment is obsolete: true
I rewrite the test file for testing the Webconsole's filter input from scratch. The tests were devided into different files, now the tests are integrated into a new file, and the new test file tests filtering for both the output contents and the filenames. The filter input test files in the old directory are untouched.
Comment on attachment 8966710 [details] Bug 1448553 - Part 7: Adds mochitests to test the feature of filtering Unicode strings and Unicode filenames in the Web Console. https://reviewboard.mozilla.org/r/235416/#review241100 Code analysis found 5 defects in this patch: - 5 defects found by mozlint You can run this analysis locally with: - `./mach lint path/to/file` (JS/Python) If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx ::: devtools/client/webconsole/new-console-output/test/mochitest/browser_webconsole_filter_by_input.js:63 (Diff revision 1) > +><script> > +console.log("Test filtering ${SEASON.english} names."); > +</script > +><script src="/${FILENAME_OF_ASCII_JS}"></script > +><script src="/${URI_ENCODED_FILENAME_OF_UNICODE_JS}"></script>`; > +let CONTENT_OF_ASCII_JS = ''; Error: Strings must use doublequote. [eslint: quotes] ::: devtools/client/webconsole/new-console-output/test/mochitest/browser_webconsole_filter_by_input.js:64 (Diff revision 1) > +console.log("Test filtering ${SEASON.english} names."); > +</script > +><script src="/${FILENAME_OF_ASCII_JS}"></script > +><script src="/${URI_ENCODED_FILENAME_OF_UNICODE_JS}"></script>`; > +let CONTENT_OF_ASCII_JS = ''; > +let CONTENT_OF_UNICODE_JS = ''; Error: Strings must use doublequote. [eslint: quotes] ::: devtools/client/webconsole/new-console-output/test/mochitest/browser_webconsole_filter_by_input.js:129 (Diff revision 1) > + for (let curSeason of SEASONS) { > + fillInFilterInputBox(curSeason.english); > + is(countVisibleLogs(), 1, > + `the number of all the logs containing ${curSeason.english}`); > + > + } Error: Block must not be padded by blank lines. [eslint: padded-blocks] ::: devtools/client/webconsole/new-console-output/test/mochitest/browser_webconsole_filter_by_input.js:142 (Diff revision 1) > + for (let curSeason of SEASONS) { > + fillInFilterInputBox(curSeason.chinese); > + is(countVisibleLogs(), 1, > + `the number of all the logs containing ${curSeason.chinese}`); > + > + } Error: Block must not be padded by blank lines. [eslint: padded-blocks] ::: devtools/client/webconsole/new-console-output/test/mochitest/browser_webconsole_filter_by_input.js:175 (Diff revision 1) > + EventUtils.sendString(value); > + } else { > + EventUtils.synthesizeKey("KEY_Delete"); > + } > + await new Promise(resolve => { > + setTimeout(() => { Error: Listen for events instead of settimeout() with arbitrary delay [eslint: mozilla/no-arbitrary-setTimeout]
Attachment #8966710 - Attachment is obsolete: true
Attachment #8966710 - Flags: review?(nchevobbe)
Comment on attachment 8966836 [details] Bug 1448553 - Part 7: Adds mochitests to test the feature of filtering Unicode strings and Unicode filenames in the Web Console. https://reviewboard.mozilla.org/r/235526/#review241440 Thanks a lot, this is taking shape nicely. I'm sorry I'll request another round of review to make sure the messages in the output are the one we expect (which we can't really tell now since we only assert message count) ::: devtools/client/webconsole/new-console-output/test/mochitest/browser_webconsole_filter_by_input.js:1 (Diff revision 1) > +/* -*- indent-tabs-mode: nil; js-indent-level: 2 -*- */ > +/* vim: set ft=javascript ts=2 et sw=2 tw=80: */ nit: I tend ot not include these anymore ::: devtools/client/webconsole/new-console-output/test/mochitest/browser_webconsole_filter_by_input.js:6 (Diff revision 1) > +// Tests that the text filter box works to filter based on filenames > +// where the logs were generated. nit: s/based on filenames where the logs were generated/based on location. I think this should be enough ::: devtools/client/webconsole/new-console-output/test/mochitest/browser_webconsole_filter_by_input.js:19 (Diff revision 1) > +// > +// We simulate an HTML file which executes two Javascript files, one with an > +// ASCII filename outputs some ASCII logs and the other one with a Unicode > +// filename outputs some Unicode logs. > + > +// seasons nit: let's remove this since it doesn't bring much values ::: devtools/client/webconsole/new-console-output/test/mochitest/browser_webconsole_filter_by_input.js:48 (Diff revision 1) > + escapedChinese: "\\u51ac", > + }, > +]; > + > +// filenames > +const FILENAME_OF_HTML = `test.html`; nit: HTML_FILENAME seems a bit better ::: devtools/client/webconsole/new-console-output/test/mochitest/browser_webconsole_filter_by_input.js:49 (Diff revision 1) > + }, > +]; > + > +// filenames > +const FILENAME_OF_HTML = `test.html`; > +const FILENAME_OF_ASCII_JS = `${SEASON.english}.js`; nit: JS_ASCII_FILENAME seems a bit better ::: devtools/client/webconsole/new-console-output/test/mochitest/browser_webconsole_filter_by_input.js:50 (Diff revision 1) > +]; > + > +// filenames > +const FILENAME_OF_HTML = `test.html`; > +const FILENAME_OF_ASCII_JS = `${SEASON.english}.js`; > +const FILENAME_OF_UNICODE_JS = `${SEASON.chinese}.js`; nit: JS_UNICODE_FILENAME seems a bit better ::: devtools/client/webconsole/new-console-output/test/mochitest/browser_webconsole_filter_by_input.js:51 (Diff revision 1) > + > +// filenames > +const FILENAME_OF_HTML = `test.html`; > +const FILENAME_OF_ASCII_JS = `${SEASON.english}.js`; > +const FILENAME_OF_UNICODE_JS = `${SEASON.chinese}.js`; > +const URI_ENCODED_FILENAME_OF_UNICODE_JS = this would be ENCODED_JS_UNICODE_FILENAME if we change the unicode js constant. ::: devtools/client/webconsole/new-console-output/test/mochitest/browser_webconsole_filter_by_input.js:56 (Diff revision 1) > +><meta charset="utf-8" > +><title>Test filtering logs by filling keywords in the filter input.</title > +><script> > +console.log("Test filtering ${SEASON.english} names."); > +</script > +><script src="/${FILENAME_OF_ASCII_JS}"></script > +><script src="/${URI_ENCODED_FILENAME_OF_UNICODE_JS}"></script>`; out of curiosity, why do you put closing `>` on a new line ? I find the code a bit hard to read that way ::: devtools/client/webconsole/new-console-output/test/mochitest/browser_webconsole_filter_by_input.js:63 (Diff revision 1) > +let CONTENT_OF_ASCII_JS = ""; > +let CONTENT_OF_UNICODE_JS = ""; > +for (let curSeason of SEASONS) { > + CONTENT_OF_ASCII_JS += `console.log("${curSeason.english}");`; > + CONTENT_OF_UNICODE_JS += `console.log("${curSeason.escapedChinese}");`; > +} maybe this could be moved inside createServerAndGetTestUrl since it doesn't seems to be used anywhere else ? ::: devtools/client/webconsole/new-console-output/test/mochitest/browser_webconsole_filter_by_input.js:70 (Diff revision 1) > +for (let curSeason of SEASONS) { > + CONTENT_OF_ASCII_JS += `console.log("${curSeason.english}");`; > + CONTENT_OF_UNICODE_JS += `console.log("${curSeason.escapedChinese}");`; > +} > + > +let hud; let's avoind having globals and instead pass the hud as function argument ::: devtools/client/webconsole/new-console-output/test/mochitest/browser_webconsole_filter_by_input.js:74 (Diff revision 1) > + hud = await openNewTabAndConsole(testUrl); > + testFilteringLogs(); so, before starting filtering things and making assertions, we might want to make sure all the messages are displayed. Since we use the console api, we can't really tell when we open the console that they have been displayed. On some slow machine this might take some time (and thus, create intermittents failure on our CI). What we know though is that the console api guarantee the order of logs. So here we could wait until there is a "winter" and a "\\u51ac" messages using waitFor and findMessage ```js // Let's wait for the last logged message of each // file to be displayed in the output. const lastSeason = SEASONS[SEASONS.length - 1]; await waitFor(() => findMessage(hud, lastSeason.english) && findMessage(hud, lastSeason.escapedChinese) ); ``` Or we can wait until we do have a certain count of messages. Do what you prefer :) ::: devtools/client/webconsole/new-console-output/test/mochitest/browser_webconsole_filter_by_input.js:75 (Diff revision 1) > +let hud; > + > +add_task(async function() { > + const testUrl = createServerAndGetTestUrl(); > + hud = await openNewTabAndConsole(testUrl); > + testFilteringLogs(); could we inline the function content in here ? Since we only have one function, and that it is used only once, I find it easier to have the whole test in the add_task block ::: devtools/client/webconsole/new-console-output/test/mochitest/browser_webconsole_filter_by_input.js:80 (Diff revision 1) > +function createServerAndGetTestUrl() { > + const httpServer = createTestHTTPServer(); > + > + httpServer.registerContentType("html", "text/html"); > + httpServer.registerContentType("js", "application/javascript"); > + > + httpServer.registerPathHandler("/" + FILENAME_OF_HTML, > + function(request, response) { > + response.setStatusLine(request.httpVersion, 200, "OK"); > + response.write(CONTENT_OF_HTML); > + } > + ); > + httpServer.registerPathHandler("/" + FILENAME_OF_ASCII_JS, > + function(request, response) { > + response.setStatusLine(request.httpVersion, 200, "OK"); > + response.write(CONTENT_OF_ASCII_JS); > + } > + ); > + httpServer.registerPathHandler("/" + URI_ENCODED_FILENAME_OF_UNICODE_JS, > + function(request, response) { > + response.setStatusLine(request.httpVersion, 200, "OK"); > + response.write(CONTENT_OF_UNICODE_JS); > + } > + ); > + const port = httpServer.identity.primaryPort; > + return `http://localhost:${port}/${FILENAME_OF_HTML}`; > +} that's nice :) ::: devtools/client/webconsole/new-console-output/test/mochitest/browser_webconsole_filter_by_input.js:120 (Diff revision 1) > + is(countVisibleLogs(), SEASONS.length, > + `the number of all the logs containing ${FILENAME_OF_ASCII_JS}`); > + > + // Every season name in English is outputted once. > + for (let curSeason of SEASONS) { > + fillInFilterInputBox(curSeason.english); > + is(countVisibleLogs(), 1, > + `the number of all the logs containing ${curSeason.english}`); > + } > + > + // All the logs outputted by the ASCII Javascript file are visible, the others > + // are hidden. > + fillInFilterInputBox(FILENAME_OF_UNICODE_JS); > + is(countVisibleLogs(), SEASONS.length, > + `the number of all the logs containing ${FILENAME_OF_UNICODE_JS}`); > + > + // Every season name in Chinese is outputted once. > + for (let curSeason of SEASONS) { > + fillInFilterInputBox(curSeason.chinese); > + is(countVisibleLogs(), 1, > + `the number of all the logs containing ${curSeason.chinese}`); > + } > + > + // The filename of the ASCII Javascript file contains the English word season, > + // so all the logs outputted by the file are visible, besides, the HTML > + // outputs one line containing the English word season, so it is also visible. > + // The other logs are hidden. So the number of all the visible logs is the > + // season number plus one. > + fillInFilterInputBox(SEASON.english); > + is(countVisibleLogs(), SEASONS.length + 1, > + `the number of all the logs containing ${SEASON.english}`); > + > + // The filename of the Unicode Javascript file contains the Chinese word > + // season, so all the logs outputted by the file are visible. The other logs > + // are hidden. So the number of all the visible logs is the season number. > + fillInFilterInputBox(SEASON.chinese); > + is(countVisibleLogs(), SEASONS.length, > + `the number of all the logs containing ${SEASON.chinese}`); > + > + // After clearing the text in the filter input box, all the logs are visible > + // again. > + fillInFilterInputBox(null); > + is(countVisibleLogs(), SEASONS.length * 2 + 1, > + "the total number of all the logs after clearing filtering"); instead of checking the number of messages, could we check that the expected message are indeed displayed (with findMessages for example) ? Because here, we might have the right number bit the wrong messages if something goes wrong. ::: devtools/client/webconsole/new-console-output/test/mochitest/browser_webconsole_filter_by_input.js:166 (Diff revision 1) > + fillInFilterInputBox(null); > + is(countVisibleLogs(), SEASONS.length * 2 + 1, > + "the total number of all the logs after clearing filtering"); > +} > + > +async function fillInFilterInputBox(value) { let's add the hud as the first parameter instead of relying on a global ::: devtools/client/webconsole/new-console-output/test/mochitest/browser_webconsole_filter_by_input.js:166 (Diff revision 1) > + fillInFilterInputBox(null); > + is(countVisibleLogs(), SEASONS.length * 2 + 1, > + "the total number of all the logs after clearing filtering"); > +} > + > +async function fillInFilterInputBox(value) { nit: Maybe we could rename this to `setTextFilterInput` ? ::: devtools/client/webconsole/new-console-output/test/mochitest/browser_webconsole_filter_by_input.js:169 (Diff revision 1) > + if (value) { > + EventUtils.sendString(value); > + } else { > + EventUtils.synthesizeKey("KEY_Delete"); > + } could we have a dedicated `clearFilterInput` function instead of passing null ? ::: devtools/client/webconsole/new-console-output/test/mochitest/browser_webconsole_filter_by_input.js:176 (Diff revision 1) > + } else { > + EventUtils.synthesizeKey("KEY_Delete"); > + } > +} > + > +function countVisibleLogs() { let's add the hud as the first parameter instead of relying on a global ::: devtools/client/webconsole/new-console-output/test/mochitest/browser_webconsole_filter_by_input.js:178 (Diff revision 1) > + let messageNodes = outputNode.querySelectorAll(".message"); > + let displayedMessageNodes = 0; > + let view = hud.iframeWindow; > + for (let i = 0; i < messageNodes.length; i++) { > + let computedStyle = view.getComputedStyle(messageNodes[i]); > + if (computedStyle.display !== "none") { > + displayedMessageNodes++; > + } > + } > + return displayedMessageNodes; we can directly return the result of the querySelectorAll since we don't rely on CSS to hide messages (they are not in the DOM anymore)
Attachment #8966836 - Flags: review?(nchevobbe) → review-
Forgot to tell you that the patch needs to be rebased against mozilla-central since there were changes in the webconsole folder architecture
(In reply to Nicolas Chevobbe [:nchevobbe] from comment #155) > devtools/client/webconsole/new-console-output/test/mochitest/ > browser_webconsole_filter_by_input.js:56 > (Diff revision 1) > > +><meta charset="utf-8" > > +><title>Test filtering logs by filling keywords in the filter input.</title > > +><script> > > +console.log("Test filtering ${SEASON.english} names."); > > +</script > > +><script src="/${FILENAME_OF_ASCII_JS}"></script > > +><script src="/${URI_ENCODED_FILENAME_OF_UNICODE_JS}"></script>`; > > out of curiosity, why do you put closing `>` on a new line ? > I find the code a bit hard to read that way > Ah, I am personally used to the style, I tend to write my own HTML documents without any non-semantical white spaces and line breaks, and for the purpose of ease CSS to prevent them from being unintentionally visible in some cases. Since it's definitely not the case, and some people may not be used to it, I changed it to the typical style.
The new patch addressed all the issues mentioned above and is rebased against mozilla-central
Comment on attachment 8966836 [details] Bug 1448553 - Part 7: Adds mochitests to test the feature of filtering Unicode strings and Unicode filenames in the Web Console. https://reviewboard.mozilla.org/r/235526/#review241762 Almost all good :) No need to ask for review again for the next patch, you can land it if you have a green TRY push Speaking of that, I ran the test with --verify, and everything is fine, except this error: ``` ERROR TEST-UNEXPECTED-FAIL | devtools/client/webconsole/test/mochitest/browser_webconsole_filter_by_input.js | leaked 2 window(s) until shutdown [url = http://localhost:60075/test.html] ``` ::: devtools/client/webconsole/test/mochitest/head.js:234 (Diff revision 2) > /** > + * Check if the content of a log message is what we expect. > + * > + * @param Object node > + * The node for the log message. > + * @param String expectedMessageBody > + * The string we expect to match the message body in the log message. > + * @param String expectedFilename > + * The string we expect to match the filename in the log message. > + */ > +function checkLogContent(node, expectedMessageBody, expectedFilename) { > + const messageBody = node.querySelector(".message-body").textContent; > + const location = node.querySelector(".message-location").textContent; > + // The location detail contains the line number and the column line, let's > + // strip them to get the filename. > + const filename = location.split(":")[0]; > + > + is(messageBody, expectedMessageBody, "the expected message body"); > + is(filename, expectedFilename, "the expected filename"); > +} nit: this is only used in one test, so I'd prefer if this was moved back into this test file. (We try to not clutter head.js too much). We can move it back here later if we feel the need to use it in multiple tests.
Attachment #8966836 - Flags: review?(nchevobbe) → review+
Thank you very much for the review, Nicolas. I did a mochitest with --verify, but couldn't reproduce this leaking error.
(In reply to Nicolas Chevobbe [:nchevobbe] from comment #170) > I pushed to TRY > https://treeherder.mozilla.org/#/ > jobs?repo=try&revision=3b9ca4d8a2956ed1c46fd27d078028e49747499a > Let's see if everything is okay Oh, sorry, I forgot to say I had pushed to TRY before: https://treeherder.mozilla.org/#/jobs?repo=try&revision=5248986fe9abc7f93fdb86d70116f1051d4e42fd
Keywords: checkin-needed
Pushed by ntim.bugs@gmail.com: https://hg.mozilla.org/integration/autoland/rev/348535562dc3 Part 1: Defines common functions for getting a Unicode URL or a Unicode URL component r=nchevobbe https://hg.mozilla.org/integration/autoland/rev/db537d7ce02c Part 2: Decodeds Punycode-encoded international domain names and URI-encoded filenames in the Web Console so that they are displayed as human-readable Unicode text. r=nchevobbe https://hg.mozilla.org/integration/autoland/rev/9f60e011a851 Part 3: Decodeds Punycode-encoded international domain names in the Network Monitor so that they are displayed as human-readable Unicode text. r=Honza https://hg.mozilla.org/integration/autoland/rev/7d8848e3708f Part 4: Decodeds Punycode-encoded international domain names in the Storage Inspector so that they are displayed as human-readable Unicode text. r=miker https://hg.mozilla.org/integration/autoland/rev/ff72f0d4f152 Part 5: Decodeds Punycode-encoded international domain names and URI-encoded filenames in the Developer Toolbox's frame menu so that they are displayed as human-readable Unicode text. r=jdescottes https://hg.mozilla.org/integration/autoland/rev/efcf3d2f9a52 Part 6: Adds xpcshell-based unit tests for the common Unicode-URL related functions defined in Part 1 r=nchevobbe https://hg.mozilla.org/integration/autoland/rev/24b9671c65e5 Part 7: Adds mochitests to test the feature of filtering Unicode strings and Unicode filenames in the Web Console. r=nchevobbe
Keywords: checkin-needed
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: