Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2586063002:
DevTools: Show redirects from navigator.sendBeacon() (Closed)
|
Created:
4 years ago by ahmetemirercin Modified:
3 years, 11 months ago CC:
blink-reviews, chromium-reviews, gavinp+loader_chromium.org, Nate Chapin, loading-reviews_chromium.org, tyoshino+watch_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionDevTools: Show redirects from navigator.sendBeacon()
BUG=672258
Review-Url: https://codereview.chromium.org/2586063002
Cr-Commit-Position: refs/heads/master@{#442880}
Committed: https://chromium.googlesource.com/chromium/src/+/ce6ec84a84d649f1792d4ef52b611050bba468d4
Patch Set 1 #
Total comments: 2
Patch Set 2 : Test added #
Total comments: 8
Patch Set 3 : Protocol test added #
Total comments: 7
Patch Set 4 : used sendCommandOrDie #Patch Set 5 : rebased #Patch Set 6 : corrected m_frame->frame() #
Messages
Total messages: 37 (15 generated)
Description was changed from ========== DevTools: Show redirects from navigator.sendBeacon() BUG= ========== to ========== DevTools: Show redirects from navigator.sendBeacon() BUG=672258 ==========
ahmetemiremir@gmail.com changed reviewers: + allada@chromium.org, paulirish@chromium.org
Please take a look at this.
allada@chromium.org changed reviewers: + tyoshino@chromium.org
+tyoshino for further check. to me it's lgtm. I love the patch, but I think we need a test, I'll be willing to write a test for it if you are uncomfortable with it (this would be a devtools-protocol test). https://codereview.chromium.org/2586063002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/loader/PingLoader.cpp (right): https://codereview.chromium.org/2586063002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/loader/PingLoader.cpp:327: if (m_frame && m_frame->document()) { nit: I am not sure about code style for this area, but it is not consistent with the section on line 312. I will defer to tyoshino@ for guidance.
On 2016/12/20 02:35:54, allada wrote: > +tyoshino for further check. > > to me it's lgtm. > > I love the patch, but I think we need a test, I'll be willing to write a test > for it if you are uncomfortable with it (this would be a devtools-protocol > test). > > https://codereview.chromium.org/2586063002/diff/1/third_party/WebKit/Source/c... > File third_party/WebKit/Source/core/loader/PingLoader.cpp (right): > > https://codereview.chromium.org/2586063002/diff/1/third_party/WebKit/Source/c... > third_party/WebKit/Source/core/loader/PingLoader.cpp:327: if (m_frame && > m_frame->document()) { > nit: I am not sure about code style for this area, but it is not consistent with > the section on line 312. I will defer to tyoshino@ for guidance. Thanks for the review.I will write a test for this and wait tyoshino for further check.
https://codereview.chromium.org/2586063002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/loader/PingLoader.cpp (right): https://codereview.chromium.org/2586063002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/loader/PingLoader.cpp:327: if (m_frame && m_frame->document()) { On 2016/12/20 02:35:54, allada wrote: > nit: I am not sure about code style for this area, but it is not consistent with > the section on line 312. I will defer to tyoshino@ for guidance. It seems line 312 is result of this refactoring but could be shortened. https://chromium.googlesource.com/chromium/src/+/89aaa753cac97aa76612a34dcff6...
Just added a test.
Thanks! The test looks great! We are trying to be more careful on when we add front-end tests because they take apx 1/2 second minimum to run per test, where as protocol tests are about 30x faster. https://codereview.chromium.org/2586063002/diff/20001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/http/tests/inspector/network/ping-redirect.html (right): https://codereview.chromium.org/2586063002/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/http/tests/inspector/network/ping-redirect.html:1: <html> Can we do a protocol test instead? They are significantly faster than a front-end tests. examples: https://cs.chromium.org/chromium/src/third_party/WebKit/LayoutTests/inspector... and https://cs.chromium.org/chromium/src/third_party/WebKit/LayoutTests/http/test... You'll need to put it in: third_party/WebKit/LayoutTests/http/tests/inspector-protocol/ and move the php page to: third_party/WebKit/LayoutTests/http/tests/inspector-protocol/resources https://codereview.chromium.org/2586063002/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/http/tests/inspector/network/ping-redirect.html:18: InspectorTest.addResult("Redirect Source: " + request.redirectSource.url); Although this will work, I'd prefer to see something more like: var url = ''; if (request.redirectSource) url = request.redirectSource.url.substr(request.redirectSource.url.indexOf("/")); InspectorTest.addResult("Redirect Source: " + url); Because if the test fails it won't timeout and instead it'll have a text diff which makes debugging so much better. It also removes any url dependent data. Just in case we have to use ipv6 for tests this test will already be ready for it. Extra credit points if you add a helper function in the inspector-test.js file to strip domain info from urls :-) https://codereview.chromium.org/2586063002/diff/20001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/http/tests/inspector/network/resources/ping-redirect.php (right): https://codereview.chromium.org/2586063002/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/http/tests/inspector/network/resources/ping-redirect.php:5: header('HTTP/1.1 307 Temporary Redirect'); nit: Lets make the string usage consistent, either ' or " are fine... in PHP single quotes are actually slightly faster than double, but since PHP is not a language that we use at Google or on chrome we do not have a style guide, so you get to choose, but just be consistent :-) https://codereview.chromium.org/2586063002/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/http/tests/inspector/network/resources/ping-redirect.php:5: header('HTTP/1.1 307 Temporary Redirect'); nit: Lets also move this to the top line to make it easier to see apx what order the headers will be sent in.
Patchset #3 (id:40001) has been deleted
https://codereview.chromium.org/2586063002/diff/20001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/http/tests/inspector/network/ping-redirect.html (right): https://codereview.chromium.org/2586063002/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/http/tests/inspector/network/ping-redirect.html:1: <html> On 2016/12/21 22:53:14, allada wrote: > Can we do a protocol test instead? They are significantly faster than a > front-end tests. > > examples: > > https://cs.chromium.org/chromium/src/third_party/WebKit/LayoutTests/inspector... > and > https://cs.chromium.org/chromium/src/third_party/WebKit/LayoutTests/http/test... > > You'll need to put it in: > > third_party/WebKit/LayoutTests/http/tests/inspector-protocol/ > > and move the php page to: > third_party/WebKit/LayoutTests/http/tests/inspector-protocol/resources Done. https://codereview.chromium.org/2586063002/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/http/tests/inspector/network/ping-redirect.html:18: InspectorTest.addResult("Redirect Source: " + request.redirectSource.url); On 2016/12/21 22:53:14, allada wrote: > Although this will work, I'd prefer to see something more like: > > var url = ''; > if (request.redirectSource) > url = > request.redirectSource.url.substr(request.redirectSource.url.indexOf("/")); > InspectorTest.addResult("Redirect Source: " + url); > > Because if the test fails it won't timeout and instead it'll have a text diff > which makes debugging so much better. > It also removes any url dependent data. Just in case we have to use ipv6 for > tests this test will already be ready for it. > > Extra credit points if you add a helper function in the inspector-test.js file > to strip domain info from urls :-) Done. https://codereview.chromium.org/2586063002/diff/20001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/http/tests/inspector/network/resources/ping-redirect.php (right): https://codereview.chromium.org/2586063002/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/http/tests/inspector/network/resources/ping-redirect.php:5: header('HTTP/1.1 307 Temporary Redirect'); On 2016/12/21 22:53:14, allada wrote: > nit: Lets also move this to the top line to make it easier to see apx what order > the headers will be sent in. Done. https://codereview.chromium.org/2586063002/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/http/tests/inspector/network/resources/ping-redirect.php:5: header('HTTP/1.1 307 Temporary Redirect'); On 2016/12/21 22:53:14, allada wrote: > nit: Lets make the string usage consistent, either ' or " are fine... in PHP > single quotes are actually slightly faster than double, but since PHP is not a > language that we use at Google or on chrome we do not have a style guide, so you > get to choose, but just be consistent :-) Done.
Patchset #3 (id:60001) has been deleted
I forgot to mention, you need to add yourself to the AUTHORS file. see: https://www.chromium.org/developers/contributing-code#TOC-Legal-stuff
On 2016/12/29 19:46:29, allada wrote: > I forgot to mention, you need to add yourself to the AUTHORS file. > > see: https://www.chromium.org/developers/contributing-code#TOC-Legal-stuff Thanks but I'm already in that file.
tyoshino, could you take a look?
Sorry for delay. I'm back from vacation. I'll review this next week.
lgtm https://codereview.chromium.org/2586063002/diff/80001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/http/tests/inspector-protocol/inspector-protocol-test.js (right): https://codereview.chromium.org/2586063002/diff/80001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/http/tests/inspector-protocol/inspector-protocol-test.js:46: } can't we use the URL API for this? not available in inspector tests? https://codereview.chromium.org/2586063002/diff/80001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/http/tests/inspector-protocol/ping-redirect.html (right): https://codereview.chromium.org/2586063002/diff/80001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/http/tests/inspector-protocol/ping-redirect.html:21: function didEnableNetwork() some inspector-protocol tests are checking .error of the 1st argument, e.g. network-data-length.html. Don't you need it?
https://codereview.chromium.org/2586063002/diff/80001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/http/tests/inspector-protocol/inspector-protocol-test.js (right): https://codereview.chromium.org/2586063002/diff/80001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/http/tests/inspector-protocol/inspector-protocol-test.js:46: } On 2017/01/10 05:21:39, tyoshino wrote: > can't we use the URL API for this? not available in inspector tests? Sadly no, to keep protocol tests fast we load very minimal amount of library/utils as possible. If we moved it to a normal layout test we could, but in this case we should not. I believe this code was taken from: https://cs.chromium.org/chromium/src/third_party/WebKit/Source/devtools/front... Which is not available in protocol tests. https://codereview.chromium.org/2586063002/diff/80001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/http/tests/inspector-protocol/ping-redirect.html (right): https://codereview.chromium.org/2586063002/diff/80001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/http/tests/inspector-protocol/ping-redirect.html:21: function didEnableNetwork() On 2017/01/10 05:21:39, tyoshino wrote: > some inspector-protocol tests are checking .error of the 1st argument, e.g. > network-data-length.html. Don't you need it? Good point I forgot about this. We prefer to now use: InspectorTest.sendCommandOrDie("Network.enable", {}, didEnableNetwork); As this is much easier to read and won't time out the test.
https://codereview.chromium.org/2586063002/diff/80001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/http/tests/inspector-protocol/inspector-protocol-test.js (right): https://codereview.chromium.org/2586063002/diff/80001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/http/tests/inspector-protocol/inspector-protocol-test.js:46: } On 2017/01/10 07:14:16, allada wrote: > On 2017/01/10 05:21:39, tyoshino wrote: > > can't we use the URL API for this? not available in inspector tests? > > Sadly no, to keep protocol tests fast we load very minimal amount of > library/utils as possible. If we moved it to a normal layout test we could, but > in this case we should not. > > I believe this code was taken from: > https://cs.chromium.org/chromium/src/third_party/WebKit/Source/devtools/front... > > Which is not available in protocol tests. Acknowledged. https://codereview.chromium.org/2586063002/diff/80001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/http/tests/inspector-protocol/ping-redirect.html (right): https://codereview.chromium.org/2586063002/diff/80001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/http/tests/inspector-protocol/ping-redirect.html:21: function didEnableNetwork() On 2017/01/10 07:14:16, allada wrote: > On 2017/01/10 05:21:39, tyoshino wrote: > > some inspector-protocol tests are checking .error of the 1st argument, e.g. > > network-data-length.html. Don't you need it? > > Good point I forgot about this. We prefer to now use: > > InspectorTest.sendCommandOrDie("Network.enable", {}, didEnableNetwork); > > As this is much easier to read and won't time out the test. Sounds good!
https://codereview.chromium.org/2586063002/diff/80001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/http/tests/inspector-protocol/ping-redirect.html (right): https://codereview.chromium.org/2586063002/diff/80001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/http/tests/inspector-protocol/ping-redirect.html:21: function didEnableNetwork() On 2017/01/10 07:14:16, allada wrote: > On 2017/01/10 05:21:39, tyoshino wrote: > > some inspector-protocol tests are checking .error of the 1st argument, e.g. > > network-data-length.html. Don't you need it? > > Good point I forgot about this. We prefer to now use: > > InspectorTest.sendCommandOrDie("Network.enable", {}, didEnableNetwork); > > As this is much easier to read and won't time out the test. Done.
lgtm
The CQ bit was checked by ahmetemiremir@gmail.com
The patchset sent to the CQ was uploaded after l-g-t-m from allada@chromium.org Link to the patchset: https://codereview.chromium.org/2586063002/#ps100001 (title: "used sendCommandOrDie")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...) ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...) ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)
The CQ bit was checked by ahmetemiremir@gmail.com
The patchset sent to the CQ was uploaded after l-g-t-m from tyoshino@chromium.org, allada@chromium.org Link to the patchset: https://codereview.chromium.org/2586063002/#ps120001 (title: "rebased")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...) linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by ahmetemiremir@gmail.com
The patchset sent to the CQ was uploaded after l-g-t-m from tyoshino@chromium.org, allada@chromium.org Link to the patchset: https://codereview.chromium.org/2586063002/#ps140001 (title: "corrected m_frame->frame()")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 140001, "attempt_start_ts": 1484132578996570,
"parent_rev": "f812e777f77f24979571a28472c064efade8ede8", "commit_rev":
"ce6ec84a84d649f1792d4ef52b611050bba468d4"}
Message was sent while issue was closed.
Description was changed from ========== DevTools: Show redirects from navigator.sendBeacon() BUG=672258 ========== to ========== DevTools: Show redirects from navigator.sendBeacon() BUG=672258 Review-Url: https://codereview.chromium.org/2586063002 Cr-Commit-Position: refs/heads/master@{#442880} Committed: https://chromium.googlesource.com/chromium/src/+/ce6ec84a84d649f1792d4ef52b61... ==========
Message was sent while issue was closed.
Committed patchset #6 (id:140001) as https://chromium.googlesource.com/chromium/src/+/ce6ec84a84d649f1792d4ef52b61... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
