Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2819393002:
Collect loading signal data from the background loader offliner (Closed)
|
Created:
3 years, 8 months ago by Pete Williamson Modified:
3 years, 8 months ago CC:
chromium-reviews, romax+watch_chromium.org, fgorski+watch_chromium.org, dewittj+watch_chromium.org, petewil+watch_chromium.org, chili+watch_chromium.org, dimich+watch_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionCollect loading signal data from the background loader offliner
We currently collect loading signal from the prerendering offliner, but
not the background loader offliner. This adds collection from the
background loader offliner too. The signals will be sent as JSON.
BUG=706628
Review-Url: https://codereview.chromium.org/2819393002
Cr-Commit-Position: refs/heads/master@{#466056}
Committed: https://chromium.googlesource.com/chromium/src/+/b00919686f1472b84a9f928aa22af4d47b0ffd5a
Patch Set 1 #Patch Set 2 : Add unit tests #
Total comments: 6
Patch Set 3 : Merge in the "extra headers" change #
Total comments: 2
Patch Set 4 : Add comment about why we use a double. #
Messages
Total messages: 26 (18 generated)
The CQ bit was checked by petewil@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: android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...) android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...) android_cronet on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_cron...) android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...) cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...) chromeos_amd64-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...) chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...) chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...) linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_chromeos_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 checked by petewil@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.
petewil@chromium.org changed reviewers: + chili@chromium.org
lgtm with small comment https://codereview.chromium.org/2819393002/diff/20001/chrome/browser/android/... File chrome/browser/android/offline_pages/background_loader_offliner.cc (right): https://codereview.chromium.org/2819393002/diff/20001/chrome/browser/android/... chrome/browser/android/offline_pages/background_loader_offliner.cc:356: "%s\r\n%s\r\n\r\n", kContentTransferEncodingBinary, kXHeaderForSignals); Just out of curiosity why \r\n rather than just \n? Does the \r provide any additional advantage/readability?
fgorski@chromium.org changed reviewers: + fgorski@chromium.org
https://codereview.chromium.org/2819393002/diff/20001/chrome/browser/android/... File chrome/browser/android/offline_pages/background_loader_offliner.cc (right): https://codereview.chromium.org/2819393002/diff/20001/chrome/browser/android/... chrome/browser/android/offline_pages/background_loader_offliner.cc:351: // Add loading signal into the MHTML that will be generated if the command This whole section is a copy-paste from prerendering offliner, therefore I don't believe it belongs either here or there. Please extract the code to its own class and call it from here and PO.
https://codereview.chromium.org/2819393002/diff/20001/chrome/browser/android/... File chrome/browser/android/offline_pages/background_loader_offliner.cc (right): https://codereview.chromium.org/2819393002/diff/20001/chrome/browser/android/... chrome/browser/android/offline_pages/background_loader_offliner.cc:351: // Add loading signal into the MHTML that will be generated if the command On 2017/04/19 15:45:51, fgorski wrote: > This whole section is a copy-paste from prerendering offliner, therefore I don't > believe it belongs either here or there. > > Please extract the code to its own class and call it from here and PO. We will be removing the prerendering offliner when we are happy with the background loading offliner. I think that the code is better where it is than refactored out. Do you still think this would be better pulled out, even once the prerendering offliner is gone? https://codereview.chromium.org/2819393002/diff/20001/chrome/browser/android/... chrome/browser/android/offline_pages/background_loader_offliner.cc:356: "%s\r\n%s\r\n\r\n", kContentTransferEncodingBinary, kXHeaderForSignals); On 2017/04/19 06:40:29, chili wrote: > Just out of curiosity why \r\n rather than just \n? Does the \r provide any > additional advantage/readability? It makes the headers in MHTML comply with the spec. As far as whether it is more readable, it depends on the system. On Windows, CRLF is the norm for text files, but not on linux. In either case, we expect this to be read by a machine, not a human, so keeping consistent with the other headers in the MHTML is the higher priority than readability.
The CQ bit was checked by petewil@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.
lgtm https://codereview.chromium.org/2819393002/diff/20001/chrome/browser/android/... File chrome/browser/android/offline_pages/background_loader_offliner.cc (right): https://codereview.chromium.org/2819393002/diff/20001/chrome/browser/android/... chrome/browser/android/offline_pages/background_loader_offliner.cc:351: // Add loading signal into the MHTML that will be generated if the command On 2017/04/19 17:45:49, Pete Williamson wrote: > On 2017/04/19 15:45:51, fgorski wrote: > > This whole section is a copy-paste from prerendering offliner, therefore I > don't > > believe it belongs either here or there. > > > > Please extract the code to its own class and call it from here and PO. > > We will be removing the prerendering offliner when we are happy with the > background loading offliner. I think that the code is better where it is than > refactored out. Do you still think this would be better pulled out, even once > the prerendering offliner is gone? As we discussed yesterday. Duplication still tells me that this code belongs someplace else. I think SignalCollector interface would do a great deal here for collection and the it should allow a serializer to attach to it for this printout. I am OK with not changing it now, but we should pay attention to things like that. Let's add a note in signal collector document so that we don't lose track of this. https://codereview.chromium.org/2819393002/diff/40001/chrome/browser/android/... File chrome/browser/android/offline_pages/background_loader_offliner.cc (right): https://codereview.chromium.org/2819393002/diff/40001/chrome/browser/android/... chrome/browser/android/offline_pages/background_loader_offliner.cc:471: double delay = delay_so_far.InMilliseconds(); Please add comment explaining what happens here: Why the types, implicit conversion, and so on.
https://codereview.chromium.org/2819393002/diff/20001/chrome/browser/android/... File chrome/browser/android/offline_pages/background_loader_offliner.cc (right): https://codereview.chromium.org/2819393002/diff/20001/chrome/browser/android/... chrome/browser/android/offline_pages/background_loader_offliner.cc:351: // Add loading signal into the MHTML that will be generated if the command On 2017/04/20 16:59:02, fgorski wrote: > On 2017/04/19 17:45:49, Pete Williamson wrote: > > On 2017/04/19 15:45:51, fgorski wrote: > > > This whole section is a copy-paste from prerendering offliner, therefore I > > don't > > > believe it belongs either here or there. > > > > > > Please extract the code to its own class and call it from here and PO. > > > > We will be removing the prerendering offliner when we are happy with the > > background loading offliner. I think that the code is better where it is than > > refactored out. Do you still think this would be better pulled out, even once > > the prerendering offliner is gone? > > As we discussed yesterday. Duplication still tells me that this code belongs > someplace else. I think SignalCollector interface would do a great deal here for > collection and the it should allow a serializer to attach to it for this > printout. I am OK with not changing it now, but we should pay attention to > things like that. > Let's add a note in signal collector document so that we don't lose track of > this. Done. https://codereview.chromium.org/2819393002/diff/40001/chrome/browser/android/... File chrome/browser/android/offline_pages/background_loader_offliner.cc (right): https://codereview.chromium.org/2819393002/diff/40001/chrome/browser/android/... chrome/browser/android/offline_pages/background_loader_offliner.cc:471: double delay = delay_so_far.InMilliseconds(); On 2017/04/20 16:59:02, fgorski wrote: > Please add comment explaining what happens here: > Why the types, implicit conversion, and so on. Done.
The CQ bit was checked by petewil@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from chili@chromium.org, fgorski@chromium.org Link to the patchset: https://codereview.chromium.org/2819393002/#ps60001 (title: "Add comment about why we use a double.")
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": 60001, "attempt_start_ts": 1492708006804410,
"parent_rev": "e3cbfeec943500d99832b93b015d62937ff15443", "commit_rev":
"b00919686f1472b84a9f928aa22af4d47b0ffd5a"}
Message was sent while issue was closed.
Description was changed from ========== Collect loading signal data from the background loader offliner We currently collect loading signal from the prerendering offliner, but not the background loader offliner. This adds collection from the background loader offliner too. The signals will be sent as JSON. BUG=706628 ========== to ========== Collect loading signal data from the background loader offliner We currently collect loading signal from the prerendering offliner, but not the background loader offliner. This adds collection from the background loader offliner too. The signals will be sent as JSON. BUG=706628 Review-Url: https://codereview.chromium.org/2819393002 Cr-Commit-Position: refs/heads/master@{#466056} Committed: https://chromium.googlesource.com/chromium/src/+/b00919686f1472b84a9f928aa22a... ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001) as https://chromium.googlesource.com/chromium/src/+/b00919686f1472b84a9f928aa22a... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
