Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2519893002:
Stop dispatching encoded-data-length on each data chunk arrival (Closed)
|
Created:
4 years, 1 month ago by yhirano Modified:
4 years ago CC:
chromium-reviews, tyoshino+watch_chromium.org, Yoav Weiss, lushnikov+blink_chromium.org, pfeldman+blink_chromium.org, caseq+blink_chromium.org, apavlov+blink_chromium.org, gavinp+loader_chromium.org, devtools-reviews_chromium.org, blink-reviews, loading-reviews+fetch_chromium.org, Nate Chapin, loading-reviews_chromium.org, kozyatinskiy+blink_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionStop dispatching encoded-data-length on each data chunk arrival
With this CL, blink stops dispatching encoded-data-length to the inspector in
FrameFetchContext::dispatchDidReceiveData. Instead, it dispatches
encoded-data-length separately. This is needed to use mojo data pipe for transferring
the response body.
https://docs.google.com/document/d/1Q-LpTPcV8QApeXSL24MVZHjP-uQK7RAK300t_qZaGFQ/edit#
BUG=666217
Committed: https://crrev.com/8d02be17b54b90ab3e139b7a958edb34bc706106
Cr-Commit-Position: refs/heads/master@{#437147}
Patch Set 1 : fix #
Total comments: 3
Patch Set 2 : rebase #Patch Set 3 : fix #Patch Set 4 : fix #Patch Set 5 : fix #Patch Set 6 : fix #Patch Set 7 : PS5 again #
Total comments: 8
Patch Set 8 : fix #Patch Set 9 : fix #
Total comments: 3
Patch Set 10 : fix #Patch Set 11 : fix #Dependent Patchsets: Messages
Total messages: 89 (72 generated)
The CQ bit was checked by yhirano@chromium.org to run a CQ dry run
Dry run: 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
Dry run: Try jobs failed on following builders: linux_chromium_asan_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 yhirano@chromium.org to run a CQ dry run
Dry run: 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
Dry run: Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
The CQ bit was checked by yhirano@chromium.org to run a CQ dry run
Dry run: 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
Dry run: This issue passed the CQ dry run.
Patchset #2 (id:20001) has been deleted
Patchset #1 (id:1) has been deleted
Description was changed from ========== Stop dispatching encoded-data-length on each data chunk arrival BUG=666227 ========== to ========== Stop dispatching encoded-data-length on each data chunk arrival With this CL, blink stops dispatching encoded-data-length to the inspector in FrameFetchContext::dispatchDidReceiveData. Instead, it dispatches encoded-data-length when the response completes. This is needed to use mojo data pipe for transferring the response body. https://docs.google.com/document/d/1Q-LpTPcV8QApeXSL24MVZHjP-uQK7RAK300t_qZaG... BUG=666227 ==========
yhirano@chromium.org changed reviewers: + allada@chromium.org, dgozman@chromium.org, hiroshige@chromium.org
Hi, can you take a look? I changed one test due to the field removal in InspectorTraceEvents.cpp. We can avoid it by filling the field with 0. Which do you think is better? Thanks,
https://codereview.chromium.org/2519893002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/fetch/FetchContext.h (right): https://codereview.chromium.org/2519893002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/fetch/FetchContext.h:113: int64_t encodedDataLength, I like this one! https://codereview.chromium.org/2519893002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/inspector/InspectorNetworkAgent.cpp (right): https://codereview.chromium.org/2519893002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/inspector/InspectorNetworkAgent.cpp:753: 0); This is a regression - when large request is processing, you don't see correct live updates in DevTools. It's also a public field reported outside, which means there will be other broken clients in the wild. Can we explore alternative solution of dispatching this separately (as outlined in the doc)?
The CQ bit was checked by yhirano@chromium.org to run a CQ dry run
Dry run: 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
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by yhirano@chromium.org to run a CQ dry run
Dry run: 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
Dry run: Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
The CQ bit was checked by yhirano@chromium.org to run a CQ dry run
Dry run: 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
Dry run: Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
https://codereview.chromium.org/2519893002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/inspector/InspectorNetworkAgent.cpp (right): https://codereview.chromium.org/2519893002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/inspector/InspectorNetworkAgent.cpp:753: 0); On 2016/11/22 17:54:13, dgozman wrote: > This is a regression - when large request is processing, you don't see correct > live updates in DevTools. > It's also a public field reported outside, which means there will be other > broken clients in the wild. > > Can we explore alternative solution of dispatching this separately (as outlined > in the doc)? Yes, this CL changes the behavior. Is the notification really important? PS4 does not change the behavior so much (I will update the failing test expectation). Do you like it? We'll have a dedicated IPC message only for devtools then.
yhirano@chromium.org changed reviewers: + kouhei@chromium.org
yhirano@chromium.org changed reviewers: + kouhei@chromium.org
+kouhei@ fyi
+kouhei@ fyi
The CQ bit was checked by yhirano@chromium.org to run a CQ dry run
Dry run: 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
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by yhirano@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by yhirano@chromium.org to run a CQ dry run
Dry run: 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
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2519893002/diff/160001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/fetch/ResourceFetcher.cpp (right): https://codereview.chromium.org/2519893002/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/core/fetch/ResourceFetcher.cpp:1281: context().dispatchDidReceiveData(identifier, data, dataLength); dispatchDidReceiveEncodedData here as well? https://codereview.chromium.org/2519893002/diff/160001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/inspector/InspectorInstrumentation.idl (right): https://codereview.chromium.org/2519893002/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/core/inspector/InspectorInstrumentation.idl:171: void didReceiveEncodedData([Keep] LocalFrame*, unsigned long identifier, int encodedDataLength); didReceiveEncodedDataLength https://codereview.chromium.org/2519893002/diff/160001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/inspector/InspectorNetworkAgent.cpp (right): https://codereview.chromium.org/2519893002/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/core/inspector/InspectorNetworkAgent.cpp:760: frontend()->dataReceived(requestId, monotonicallyIncreasingTime(), 0, This looks bad: we first send non-zero data with zero encoded length, then we send zero data with non-zero encoded length. This is very confusing for anyone receiving the notification. We should instead send them together in the same notification. https://codereview.chromium.org/2519893002/diff/160001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/loader/FrameFetchContext.cpp (right): https://codereview.chromium.org/2519893002/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/core/loader/FrameFetchContext.cpp:460: int64_t encodedDataLength, Why pass this if it's unused?
The CQ bit was checked by yhirano@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Patchset #8 (id:180001) has been deleted
Dry run: 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_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by yhirano@chromium.org to run a CQ dry run
Dry run: 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
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2519893002/diff/160001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/fetch/ResourceFetcher.cpp (right): https://codereview.chromium.org/2519893002/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/core/fetch/ResourceFetcher.cpp:1281: context().dispatchDidReceiveData(identifier, data, dataLength); On 2016/11/30 18:02:17, dgozman wrote: > dispatchDidReceiveEncodedData here as well? This is not used and will be removed shortly. https://codereview.chromium.org/2519893002/diff/160001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/inspector/InspectorInstrumentation.idl (right): https://codereview.chromium.org/2519893002/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/core/inspector/InspectorInstrumentation.idl:171: void didReceiveEncodedData([Keep] LocalFrame*, unsigned long identifier, int encodedDataLength); On 2016/11/30 18:02:17, dgozman wrote: > didReceiveEncodedDataLength Done. https://codereview.chromium.org/2519893002/diff/160001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/inspector/InspectorNetworkAgent.cpp (right): https://codereview.chromium.org/2519893002/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/core/inspector/InspectorNetworkAgent.cpp:760: frontend()->dataReceived(requestId, monotonicallyIncreasingTime(), 0, On 2016/11/30 18:02:17, dgozman wrote: > This looks bad: we first send non-zero data with zero encoded length, then we > send zero data with non-zero encoded length. This is very confusing for anyone > receiving the notification. We should instead send them together in the same > notification. |length| and |encoded_data_length| are different concepts and there is no strong reason to dispatch them altogether. The values will be dispatched separately from the browser process with mojo, and there will be no one-to-one relationship between the messages. https://codereview.chromium.org/2519893002/diff/160001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/loader/FrameFetchContext.cpp (right): https://codereview.chromium.org/2519893002/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/core/loader/FrameFetchContext.cpp:460: int64_t encodedDataLength, On 2016/11/30 18:02:17, dgozman wrote: > Why pass this if it's unused? For consistency.
The CQ bit was checked by yhirano@chromium.org to run a CQ dry run
Dry run: 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
Dry run: This issue passed the CQ dry run.
Description was changed from ========== Stop dispatching encoded-data-length on each data chunk arrival With this CL, blink stops dispatching encoded-data-length to the inspector in FrameFetchContext::dispatchDidReceiveData. Instead, it dispatches encoded-data-length when the response completes. This is needed to use mojo data pipe for transferring the response body. https://docs.google.com/document/d/1Q-LpTPcV8QApeXSL24MVZHjP-uQK7RAK300t_qZaG... BUG=666227 ========== to ========== Stop dispatching encoded-data-length on each data chunk arrival With this CL, blink stops dispatching encoded-data-length to the inspector in FrameFetchContext::dispatchDidReceiveData. Instead, it dispatches encoded-data-length separately. This is needed to use mojo data pipe for transferring the response body. https://docs.google.com/document/d/1Q-LpTPcV8QApeXSL24MVZHjP-uQK7RAK300t_qZaG... BUG=666227 ==========
Description was changed from ========== Stop dispatching encoded-data-length on each data chunk arrival With this CL, blink stops dispatching encoded-data-length to the inspector in FrameFetchContext::dispatchDidReceiveData. Instead, it dispatches encoded-data-length separately. This is needed to use mojo data pipe for transferring the response body. https://docs.google.com/document/d/1Q-LpTPcV8QApeXSL24MVZHjP-uQK7RAK300t_qZaG... BUG=666227 ========== to ========== Stop dispatching encoded-data-length on each data chunk arrival With this CL, blink stops dispatching encoded-data-length to the inspector in FrameFetchContext::dispatchDidReceiveData. Instead, it dispatches encoded-data-length separately. This is needed to use mojo data pipe for transferring the response body. https://docs.google.com/document/d/1Q-LpTPcV8QApeXSL24MVZHjP-uQK7RAK300t_qZaG... BUG=666227 ==========
I updated core/inspector/ implementation based on the offline discussion. PTAL.
lgtm. Thanks! https://codereview.chromium.org/2519893002/diff/220001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/inspector/InspectorNetworkAgent.cpp (right): https://codereview.chromium.org/2519893002/diff/220001/third_party/WebKit/Sou... third_party/WebKit/Source/core/inspector/InspectorNetworkAgent.cpp:775: frontend()->dataReceived(requestId, monotonicallyIncreasingTime(), 0, This is unfortunate, but there is no way around it. https://codereview.chromium.org/2519893002/diff/220001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/loader/FrameFetchContext.cpp (right): https://codereview.chromium.org/2519893002/diff/220001/third_party/WebKit/Sou... third_party/WebKit/Source/core/loader/FrameFetchContext.cpp:417: TRACE_EVENT1("devtools.timeline", "ResourceReceivedData", "data", Let's not report this one - it doesn't bring any information at all.
On 2016/12/06 18:29:31, dgozman wrote: > lgtm. Thanks! > > https://codereview.chromium.org/2519893002/diff/220001/third_party/WebKit/Sou... > File third_party/WebKit/Source/core/inspector/InspectorNetworkAgent.cpp (right): > > https://codereview.chromium.org/2519893002/diff/220001/third_party/WebKit/Sou... > third_party/WebKit/Source/core/inspector/InspectorNetworkAgent.cpp:775: > frontend()->dataReceived(requestId, monotonicallyIncreasingTime(), 0, > This is unfortunate, but there is no way around it. > > https://codereview.chromium.org/2519893002/diff/220001/third_party/WebKit/Sou... > File third_party/WebKit/Source/core/loader/FrameFetchContext.cpp (right): > > https://codereview.chromium.org/2519893002/diff/220001/third_party/WebKit/Sou... > third_party/WebKit/Source/core/loader/FrameFetchContext.cpp:417: > TRACE_EVENT1("devtools.timeline", "ResourceReceivedData", "data", > Let's not report this one - it doesn't bring any information at all. lgtm (if dgozman's comments are addressed)
The CQ bit was checked by yhirano@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2519893002/diff/220001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/loader/FrameFetchContext.cpp (right): https://codereview.chromium.org/2519893002/diff/220001/third_party/WebKit/Sou... third_party/WebKit/Source/core/loader/FrameFetchContext.cpp:417: TRACE_EVENT1("devtools.timeline", "ResourceReceivedData", "data", On 2016/12/06 18:29:31, dgozman wrote: > Let's not report this one - it doesn't bring any information at all. Done.
The CQ bit was unchecked by yhirano@chromium.org
The CQ bit was checked by yhirano@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from hiroshige@chromium.org, dgozman@chromium.org Link to the patchset: https://codereview.chromium.org/2519893002/#ps240001 (title: "fix")
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: linux_chromium_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 yhirano@chromium.org to run a CQ dry run
Dry run: 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
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by yhirano@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from hiroshige@chromium.org, dgozman@chromium.org Link to the patchset: https://codereview.chromium.org/2519893002/#ps260001 (title: "fix")
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": 260001, "attempt_start_ts": 1481161871833140,
"parent_rev": "33a24129ea6e3f6d879e48226d829f0d2825530c", "commit_rev":
"f695d9461b91770bada7bed8605f6e88b127ba99"}
Message was sent while issue was closed.
Description was changed from ========== Stop dispatching encoded-data-length on each data chunk arrival With this CL, blink stops dispatching encoded-data-length to the inspector in FrameFetchContext::dispatchDidReceiveData. Instead, it dispatches encoded-data-length separately. This is needed to use mojo data pipe for transferring the response body. https://docs.google.com/document/d/1Q-LpTPcV8QApeXSL24MVZHjP-uQK7RAK300t_qZaG... BUG=666227 ========== to ========== Stop dispatching encoded-data-length on each data chunk arrival With this CL, blink stops dispatching encoded-data-length to the inspector in FrameFetchContext::dispatchDidReceiveData. Instead, it dispatches encoded-data-length separately. This is needed to use mojo data pipe for transferring the response body. https://docs.google.com/document/d/1Q-LpTPcV8QApeXSL24MVZHjP-uQK7RAK300t_qZaG... BUG=666227 ==========
Message was sent while issue was closed.
Committed patchset #11 (id:260001)
Message was sent while issue was closed.
Description was changed from ========== Stop dispatching encoded-data-length on each data chunk arrival With this CL, blink stops dispatching encoded-data-length to the inspector in FrameFetchContext::dispatchDidReceiveData. Instead, it dispatches encoded-data-length separately. This is needed to use mojo data pipe for transferring the response body. https://docs.google.com/document/d/1Q-LpTPcV8QApeXSL24MVZHjP-uQK7RAK300t_qZaG... BUG=666227 ========== to ========== Stop dispatching encoded-data-length on each data chunk arrival With this CL, blink stops dispatching encoded-data-length to the inspector in FrameFetchContext::dispatchDidReceiveData. Instead, it dispatches encoded-data-length separately. This is needed to use mojo data pipe for transferring the response body. https://docs.google.com/document/d/1Q-LpTPcV8QApeXSL24MVZHjP-uQK7RAK300t_qZaG... BUG=666227 Committed: https://crrev.com/8d02be17b54b90ab3e139b7a958edb34bc706106 Cr-Commit-Position: refs/heads/master@{#437147} ==========
Message was sent while issue was closed.
Patchset 11 (id:??) landed as https://crrev.com/8d02be17b54b90ab3e139b7a958edb34bc706106 Cr-Commit-Position: refs/heads/master@{#437147}
Message was sent while issue was closed.
Description was changed from ========== Stop dispatching encoded-data-length on each data chunk arrival With this CL, blink stops dispatching encoded-data-length to the inspector in FrameFetchContext::dispatchDidReceiveData. Instead, it dispatches encoded-data-length separately. This is needed to use mojo data pipe for transferring the response body. https://docs.google.com/document/d/1Q-LpTPcV8QApeXSL24MVZHjP-uQK7RAK300t_qZaG... BUG=666227 Committed: https://crrev.com/8d02be17b54b90ab3e139b7a958edb34bc706106 Cr-Commit-Position: refs/heads/master@{#437147} ========== to ========== Stop dispatching encoded-data-length on each data chunk arrival With this CL, blink stops dispatching encoded-data-length to the inspector in FrameFetchContext::dispatchDidReceiveData. Instead, it dispatches encoded-data-length separately. This is needed to use mojo data pipe for transferring the response body. https://docs.google.com/document/d/1Q-LpTPcV8QApeXSL24MVZHjP-uQK7RAK300t_qZaG... BUG=666217 Committed: https://crrev.com/8d02be17b54b90ab3e139b7a958edb34bc706106 Cr-Commit-Position: refs/heads/master@{#437147} ==========
Message was sent while issue was closed.
(I specified a wrong bug number, so fixed it here after the CL is landed.) |
