Closed
Bug 1337721
Opened 7 years ago
Closed 7 years ago
Redirected HTTP channel doesn't inherit the channel priority
Categories
(Core :: Networking: HTTP, defect)
Core
Networking: HTTP
Tracking
()
RESOLVED
FIXED
mozilla54
Tracking | Status | |
---|---|---|
firefox54 | --- | fixed |
People
(Reporter: schien, Assigned: schien)
References
Details
(Whiteboard: [necko-active])
Attachments
(2 files)
The redirected HTTP channel should inherit the channel priority to preserve the loading order. Maybe we can set priority in here: https://searchfox.org/mozilla-central/rev/848c29538ab007fb95dc6cff194f0e3d3809613d/netwerk/protocol/http/HttpBaseChannel.cpp#3012
Comment 1•7 years ago
|
||
Good catch! Yes, that is the place.
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Updated•7 years ago
|
Assignee: nobody → schien
Whiteboard: [necko-active]
Comment 4•7 years ago
|
||
mozreview-review |
Comment on attachment 8835932 [details] Bug 1337721 - Part 1, preserve the channel priority after redirect. https://reviewboard.mozilla.org/r/111486/#review112834 thanks!
Attachment #8835932 -
Flags: review?(honzab.moz) → review+
Comment 5•7 years ago
|
||
mozreview-review |
Comment on attachment 8835933 [details] Bug 1337721 - Part 2, add xpcshell test for channel priority. https://reviewboard.mozilla.org/r/111488/#review112842 ::: netwerk/test/unit/test_channel_priority.js:52 (Diff revision 1) > + requestChannel.asyncOpen2(new ChannelListener(checkResponse, requestChannel)); > +} > + > +function checkResponse(request, buffer, requestChannel) { > + requestChannel.QueryInterface(Ci.nsISupportsPriority); > + Assert.equal(requestChannel.priority, Ci.nsISupportsPriority.PRIORITY_HIGHEST); what is the requestChannel? ::: netwerk/test/unit/test_channel_priority.js:73 (Diff revision 1) > + > +function run_test() { // jshint ignore:line > + if (!runningInParent) { > + // report test finished to parent process > + add_test(function () { > + do_send_remote_message('finished'); are you sure this is the place to send 'finish'? shouldn't it happen after all the requests are made?
Assignee | ||
Comment 6•7 years ago
|
||
mozreview-review-reply |
Comment on attachment 8835933 [details] Bug 1337721 - Part 2, add xpcshell test for channel priority. https://reviewboard.mozilla.org/r/111488/#review112842 > what is the requestChannel? This is the original channel object we created in line 44. We store it as the closure context for the `checkResponse` callback. See https://searchfox.org/mozilla-central/rev/d3307f19d5dac31d7d36fc206b00b686de82eee4/netwerk/test/unit/head_channels.js#51 and https://searchfox.org/mozilla-central/rev/d3307f19d5dac31d7d36fc206b00b686de82eee4/netwerk/test/unit/head_channels.js#170 > are you sure this is the place to send 'finish'? shouldn't it happen after all the requests are made? This code will only send 'finish' after all the requests are made because it is added as the last test step. I should change the comment to: ``` // add a task to report test finished to parent process at the end of test queue, // since do_register_cleanup is not available in child xpcshell test script. ```
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 9•7 years ago
|
||
mozreview-review |
Comment on attachment 8835933 [details] Bug 1337721 - Part 2, add xpcshell test for channel priority. https://reviewboard.mozilla.org/r/111488/#review115232
Comment 10•7 years ago
|
||
Comment on attachment 8835933 [details] Bug 1337721 - Part 2, add xpcshell test for channel priority. stupid reviewberd
Attachment #8835933 -
Flags: review?(honzab.moz) → review+
Assignee | ||
Comment 11•7 years ago
|
||
hmm...the r+ flag didn't reflect to reviewboard. Let me update the r+ on reviewboard manually.
Assignee | ||
Comment 12•7 years ago
|
||
mozreview-review |
Comment on attachment 8835933 [details] Bug 1337721 - Part 2, add xpcshell test for channel priority. https://reviewboard.mozilla.org/r/111488/#review115504 help reflect the r+ flag from bugzilla.
Attachment #8835933 -
Flags: review+
Assignee | ||
Comment 13•7 years ago
|
||
(In reply to Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz) from comment #12) > Comment on attachment 8835933 [details] > Bug 1337721 - Part 2, add xpcshell test for channel priority. > > https://reviewboard.mozilla.org/r/111488/#review115504 > > help reflect the r+ flag from bugzilla. Hmm...the commit message of part-2 will be rewrite to "r=schien" because I click r+ on reviewboard. Still need @mayhemer's r+ on reviewboard to avoid the confusion in commit message.
Flags: needinfo?(honzab.moz)
Comment 14•7 years ago
|
||
(In reply to Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz) from comment #13) > (In reply to Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz) from comment > #12) > > Comment on attachment 8835933 [details] > > Bug 1337721 - Part 2, add xpcshell test for channel priority. > > > > https://reviewboard.mozilla.org/r/111488/#review115504 > > > > help reflect the r+ flag from bugzilla. > > Hmm...the commit message of part-2 will be rewrite to "r=schien" because I > click r+ on reviewboard. Still need @mayhemer's r+ on reviewboard to avoid > the confusion in commit message. There is one. Easy solution: don't use crappy reviewboard :)
Flags: needinfo?(honzab.moz)
Comment 15•7 years ago
|
||
mozreview-review |
Comment on attachment 8835933 [details] Bug 1337721 - Part 2, add xpcshell test for channel priority. https://reviewboard.mozilla.org/r/111488/#review115538
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 18•7 years ago
|
||
Pushed by schien@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/b2ba4b8eb7f3 Part 1, preserve the channel priority after redirect. r=mayhemer https://hg.mozilla.org/integration/autoland/rev/f7fd37690c35 Part 2, add xpcshell test for channel priority. r=mayhemer
Comment 19•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/b2ba4b8eb7f3 https://hg.mozilla.org/mozilla-central/rev/f7fd37690c35
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox54:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
You need to log in
before you can comment on or make changes to this bug.
Description
•