Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2857063002:
Add a way to send the resource percentage signal to the RC. (Closed)
|
Created:
3 years, 7 months ago by Pete Williamson Modified:
3 years, 5 months ago CC:
blundell+watchlist_chromium.org, cbentzel+watch_chromium.org, chili+watch_chromium.org, chromium-reviews, csharrison+watch_chromium.org, dewittj+watch_chromium.org, dimich+watch_chromium.org, droger+watchlist_chromium.org, fgorski+watch_chromium.org, gavinp+prer_chromium.org, loading-reviews+metrics_chromium.org, petewil+watch_chromium.org, romax+watch_chromium.org, sdefresne+watchlist_chromium.org, speed-metrics-reviews_chromium.org, tburkard+watch_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdd a way to send the resource percentage signal to the RC.
This lets us send the resource completeness signals to the
RequestCoordinator. Unit tests are not yet added.
BUG=699313
Patch Set 1 #Patch Set 2 : merged #Patch Set 3 : Remove dependency on content, since components cannot use content. #Patch Set 4 : Add header for new enum. #Patch Set 5 : Add header file declaration #Patch Set 6 : qualify resource type #Patch Set 7 : Build fix on some bots #Patch Set 8 : Fix warning on some bots #Patch Set 9 : Unit test working #Patch Set 10 : cleanup #
Total comments: 5
Patch Set 11 : CR feedback, fix unit test. #
Total comments: 18
Patch Set 12 : Remove logging #Patch Set 13 : More RyanSturm feedback #
Total comments: 7
Patch Set 14 : comment fix #Patch Set 15 : Workaround for unit tests needing to use something other than the main frame type. #Patch Set 16 : Fix memory leak in unit test, which failed on the ASAN bot. #Patch Set 17 : Make sure we don't send notifications unless we are offlining #
Total comments: 3
Patch Set 18 : Move OfflinerUserData to its new home in chrome/browser/offline_pages #
Total comments: 14
Patch Set 19 : Turn off other metrics which might require tab helpers when background loading #
Total comments: 33
Patch Set 20 : CR Feedback per Dimich, BMcQuade, and CSHarrison #
Total comments: 6
Messages
Total messages: 116 (82 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: 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...) mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
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_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_android on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...) 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_...) linux_chromium_tsan_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 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_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...) android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...) cast_shell_android on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...) linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...) 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-...) linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_compile_dbg_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 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: linux_chromium_tsan_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 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: chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...) linux_chromium_tsan_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 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: 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-...) linux_chromium_tsan_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 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: ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...) 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 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: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
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_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
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_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...)
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: 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 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: cast_shell_android on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...)
ryansturm@chromium.org changed reviewers: + ryansturm@chromium.org
Still looking at this, but here are a couple of comments. https://codereview.chromium.org/2857063002/diff/180001/chrome/browser/page_lo... File chrome/browser/page_load_metrics/observers/resource_tracking_page_load_metrics_observer.h (right): https://codereview.chromium.org/2857063002/diff/180001/chrome/browser/page_lo... chrome/browser/page_load_metrics/observers/resource_tracking_page_load_metrics_observer.h:38: void InformObservers(const content::ResourceType type, private method? https://codereview.chromium.org/2857063002/diff/180001/chrome/browser/page_lo... File chrome/browser/page_load_metrics/page_load_metrics_initialize.cc (right): https://codereview.chromium.org/2857063002/diff/180001/chrome/browser/page_lo... chrome/browser/page_load_metrics/page_load_metrics_initialize.cc:146: offline_pages::RequestCoordinator* request_coordinator = Can you move the code to get the RequestCoordinator into OnStart of ResourceTrackingPageLoadMetricsObserver (it's called on the same stack as the constructor).
The CQ bit was checked by petewil@chromium.org to run a CQ dry run
https://codereview.chromium.org/2857063002/diff/180001/chrome/browser/page_lo... File chrome/browser/page_load_metrics/observers/resource_tracking_page_load_metrics_observer.h (right): https://codereview.chromium.org/2857063002/diff/180001/chrome/browser/page_lo... chrome/browser/page_load_metrics/observers/resource_tracking_page_load_metrics_observer.h:38: void InformObservers(const content::ResourceType type, On 2017/05/10 15:55:53, RyanSturm wrote: > private method? Done. https://codereview.chromium.org/2857063002/diff/180001/chrome/browser/page_lo... File chrome/browser/page_load_metrics/page_load_metrics_initialize.cc (right): https://codereview.chromium.org/2857063002/diff/180001/chrome/browser/page_lo... chrome/browser/page_load_metrics/page_load_metrics_initialize.cc:146: offline_pages::RequestCoordinator* request_coordinator = On 2017/05/10 15:55:53, RyanSturm wrote: > Can you move the code to get the RequestCoordinator into OnStart of > ResourceTrackingPageLoadMetricsObserver (it's called on the same stack as the > constructor). I originally did this for dependency injection to make the code more testable. I can change it as you suggest, but then I'm not sure how to unit test the ResourceTrackingPageLoadMetricsObserver - do you know any techniques to mock out getting a keyed service in a unit test? I would think it required enough of chrome to be a full browser test at that point.
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_...)
I only looked at p_l_m/ https://codereview.chromium.org/2857063002/diff/180001/chrome/browser/page_lo... File chrome/browser/page_load_metrics/page_load_metrics_initialize.cc (right): https://codereview.chromium.org/2857063002/diff/180001/chrome/browser/page_lo... chrome/browser/page_load_metrics/page_load_metrics_initialize.cc:146: offline_pages::RequestCoordinator* request_coordinator = On 2017/05/10 18:14:53, Pete Williamson wrote: > On 2017/05/10 15:55:53, RyanSturm wrote: > > Can you move the code to get the RequestCoordinator into OnStart of > > ResourceTrackingPageLoadMetricsObserver (it's called on the same stack as the > > constructor). > > I originally did this for dependency injection to make the code more testable. > I can change it as you suggest, but then I'm not sure how to unit test the > ResourceTrackingPageLoadMetricsObserver - do you know any techniques to mock out > getting a keyed service in a unit test? I would think it required enough of > chrome to be a full browser test at that point. That's a valid point. I've done some other hacky approaches here using virtualization: https://cs.chromium.org/chromium/src/chrome/browser/page_load_metrics/observe... If you are more comfortable with that approach, it keeps the boundaries a little more clear. Generally, I agree dependency injection is better though, so I won't push back. https://codereview.chromium.org/2857063002/diff/200001/chrome/browser/page_lo... File chrome/browser/page_load_metrics/observers/resource_tracking_page_load_metrics_observer.cc (right): https://codereview.chromium.org/2857063002/diff/200001/chrome/browser/page_lo... chrome/browser/page_load_metrics/observers/resource_tracking_page_load_metrics_observer.cc:9: #include "content/public/common/resource_type.h" resource_type.h is included in the header already. https://codereview.chromium.org/2857063002/diff/200001/chrome/browser/page_lo... chrome/browser/page_load_metrics/observers/resource_tracking_page_load_metrics_observer.cc:12: offline_pages::ResourceDataType ConvertResourceTypeToResourceDataType( nit: A short method comment here would be good. https://codereview.chromium.org/2857063002/diff/200001/chrome/browser/page_lo... chrome/browser/page_load_metrics/observers/resource_tracking_page_load_metrics_observer.cc:26: return offline_pages::ResourceDataType::OTHER; Can you group these by return value? e.g.: case (content::RESOURCE_TYPE_MAIN_FRAME): case (content::RESOURCE_TYPE_SUB_FRAME): return offline_pages::ResourceDataType::TEXT_HTML; case (content::RESOURCE_TYPE_STYLESHEET): return offline_pages::ResourceDataType::TEXT_CSS; case (content::RESOURCE_TYPE_FONT_RESOURCE): case (content::RESOURCE_TYPE_OBJECT): return offline_pages::ResourceDataType::OTHER; https://codereview.chromium.org/2857063002/diff/200001/chrome/browser/page_lo... chrome/browser/page_load_metrics/observers/resource_tracking_page_load_metrics_observer.cc:70: DVLOG(0) << "@@@@@@ type " << extra_request_start_info.resource_type << " " Is this log statement needed here? https://codereview.chromium.org/2857063002/diff/200001/chrome/browser/page_lo... chrome/browser/page_load_metrics/observers/resource_tracking_page_load_metrics_observer.cc:86: DVLOG(0) << "@@@@@@ type " << extra_request_complete_info.resource_type << " " is this log needed? same below https://codereview.chromium.org/2857063002/diff/200001/chrome/browser/page_lo... File chrome/browser/page_load_metrics/observers/resource_tracking_page_load_metrics_observer.h (right): https://codereview.chromium.org/2857063002/diff/200001/chrome/browser/page_lo... chrome/browser/page_load_metrics/observers/resource_tracking_page_load_metrics_observer.h:42: void GetCountsForTypeForTesting(const content::ResourceType type, How are you planning on using this? I am a fan of the new InformObservers approach. Can this go away or is it still needed? https://codereview.chromium.org/2857063002/diff/200001/chrome/browser/page_lo... chrome/browser/page_load_metrics/observers/resource_tracking_page_load_metrics_observer.h:56: // Unowned pointer to the request_coordinator_, it will be valid for the nit: s/the request_coordinator_/the current profile's ResourceTrackerObserver/ https://codereview.chromium.org/2857063002/diff/200001/chrome/browser/page_lo... File chrome/browser/page_load_metrics/observers/resource_tracking_page_load_metrics_observer_unittest.cc (right): https://codereview.chromium.org/2857063002/diff/200001/chrome/browser/page_lo... chrome/browser/page_load_metrics/observers/resource_tracking_page_load_metrics_observer_unittest.cc:24: // FUTURE: If we ever override SetUp, ensure we call the base class SetUp. nit: I don't think this comment is necessary. https://codereview.chromium.org/2857063002/diff/200001/chrome/browser/page_lo... File chrome/browser/page_load_metrics/page_load_metrics_initialize.cc (right): https://codereview.chromium.org/2857063002/diff/200001/chrome/browser/page_lo... chrome/browser/page_load_metrics/page_load_metrics_initialize.cc:7: #include "base/logging.h" Is this needed?
That's fine, I'm happy to have you looking at only p_l_m. https://codereview.chromium.org/2857063002/diff/200001/chrome/browser/page_lo... File chrome/browser/page_load_metrics/observers/resource_tracking_page_load_metrics_observer.cc (right): https://codereview.chromium.org/2857063002/diff/200001/chrome/browser/page_lo... chrome/browser/page_load_metrics/observers/resource_tracking_page_load_metrics_observer.cc:9: #include "content/public/common/resource_type.h" On 2017/05/10 18:30:13, RyanSturm wrote: > resource_type.h is included in the header already. OK removed (My original thinking was that I shouldn't be relying on other headers for things that I need). https://codereview.chromium.org/2857063002/diff/200001/chrome/browser/page_lo... chrome/browser/page_load_metrics/observers/resource_tracking_page_load_metrics_observer.cc:12: offline_pages::ResourceDataType ConvertResourceTypeToResourceDataType( On 2017/05/10 18:30:13, RyanSturm wrote: > nit: A short method comment here would be good. Done. https://codereview.chromium.org/2857063002/diff/200001/chrome/browser/page_lo... chrome/browser/page_load_metrics/observers/resource_tracking_page_load_metrics_observer.cc:26: return offline_pages::ResourceDataType::OTHER; On 2017/05/10 18:30:13, RyanSturm wrote: > Can you group these by return value? e.g.: > > case (content::RESOURCE_TYPE_MAIN_FRAME): > case (content::RESOURCE_TYPE_SUB_FRAME): > return offline_pages::ResourceDataType::TEXT_HTML; > case (content::RESOURCE_TYPE_STYLESHEET): > return offline_pages::ResourceDataType::TEXT_CSS; > case (content::RESOURCE_TYPE_FONT_RESOURCE): > case (content::RESOURCE_TYPE_OBJECT): > return offline_pages::ResourceDataType::OTHER; Done. https://codereview.chromium.org/2857063002/diff/200001/chrome/browser/page_lo... chrome/browser/page_load_metrics/observers/resource_tracking_page_load_metrics_observer.cc:70: DVLOG(0) << "@@@@@@ type " << extra_request_start_info.resource_type << " " On 2017/05/10 18:30:13, RyanSturm wrote: > Is this log statement needed here? Oops, I forgot to remove these. Gone now. https://codereview.chromium.org/2857063002/diff/200001/chrome/browser/page_lo... chrome/browser/page_load_metrics/observers/resource_tracking_page_load_metrics_observer.cc:86: DVLOG(0) << "@@@@@@ type " << extra_request_complete_info.resource_type << " " On 2017/05/10 18:30:13, RyanSturm wrote: > is this log needed? same below Done. https://codereview.chromium.org/2857063002/diff/200001/chrome/browser/page_lo... File chrome/browser/page_load_metrics/observers/resource_tracking_page_load_metrics_observer.h (right): https://codereview.chromium.org/2857063002/diff/200001/chrome/browser/page_lo... chrome/browser/page_load_metrics/observers/resource_tracking_page_load_metrics_observer.h:42: void GetCountsForTypeForTesting(const content::ResourceType type, On 2017/05/10 18:30:13, RyanSturm wrote: > How are you planning on using this? I am a fan of the new InformObservers > approach. Can this go away or is it still needed? This is only used from the unit test. I removed it (and that part of the test) and we now rely on the observer checking instead. https://codereview.chromium.org/2857063002/diff/200001/chrome/browser/page_lo... chrome/browser/page_load_metrics/observers/resource_tracking_page_load_metrics_observer.h:56: // Unowned pointer to the request_coordinator_, it will be valid for the On 2017/05/10 18:30:13, RyanSturm wrote: > nit: s/the request_coordinator_/the current profile's ResourceTrackerObserver/ Done. https://codereview.chromium.org/2857063002/diff/200001/chrome/browser/page_lo... File chrome/browser/page_load_metrics/observers/resource_tracking_page_load_metrics_observer_unittest.cc (right): https://codereview.chromium.org/2857063002/diff/200001/chrome/browser/page_lo... chrome/browser/page_load_metrics/observers/resource_tracking_page_load_metrics_observer_unittest.cc:24: // FUTURE: If we ever override SetUp, ensure we call the base class SetUp. On 2017/05/10 18:30:13, RyanSturm wrote: > nit: I don't think this comment is necessary. Removed (I added it because it cost me a few hours to discover originally). https://codereview.chromium.org/2857063002/diff/200001/chrome/browser/page_lo... File chrome/browser/page_load_metrics/page_load_metrics_initialize.cc (right): https://codereview.chromium.org/2857063002/diff/200001/chrome/browser/page_lo... chrome/browser/page_load_metrics/page_load_metrics_initialize.cc:7: #include "base/logging.h" On 2017/05/10 18:30:14, RyanSturm wrote: > Is this needed? Gone (leftover from logging)
page_load_metrics lgtm % a few nits https://codereview.chromium.org/2857063002/diff/240001/chrome/browser/page_lo... File chrome/browser/page_load_metrics/observers/resource_tracking_page_load_metrics_observer.cc (right): https://codereview.chromium.org/2857063002/diff/240001/chrome/browser/page_lo... chrome/browser/page_load_metrics/observers/resource_tracking_page_load_metrics_observer.cc:50: // TODO(petewiL): Store this by type. Until we do, only look at images. nit: s/petewiL/petewil/ (capitalized L) unless this is an intentional stylization https://codereview.chromium.org/2857063002/diff/240001/chrome/browser/page_lo... chrome/browser/page_load_metrics/observers/resource_tracking_page_load_metrics_observer.cc:51: if (extra_request_start_info.resource_type == content::RESOURCE_TYPE_IMAGE) { Just a thought, you might want to convert the type here and in OnLoadedResource before the image comparison (x == offline_pages::ResourceDataType::IMAGE). I suggest this so that if you change ConvertResourceTypeToResourceDataType the behavior will be correct here, but I'd be fine with changing it later as this class changes. https://codereview.chromium.org/2857063002/diff/240001/chrome/browser/page_lo... File chrome/browser/page_load_metrics/page_load_metrics_initialize.cc (right): https://codereview.chromium.org/2857063002/diff/240001/chrome/browser/page_lo... chrome/browser/page_load_metrics/page_load_metrics_initialize.cc:143: // Whether we are using the pre-renderer or not, set up a Not sure if you saw the previous comment from patch 10: "That's a valid point. I've done some other hacky approaches here using virtualization: https://cs.chromium.org/chromium/src/chrome/browser/page_load_metrics/observe... If you are more comfortable with that approach, it keeps the boundaries a little more clear. Generally, I agree dependency injection is better though, so I won't push back."
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...
petewil@chromium.org changed reviewers: + dimich@chromium.org
+Dimich for offline_pages code. https://codereview.chromium.org/2857063002/diff/240001/chrome/browser/page_lo... File chrome/browser/page_load_metrics/observers/resource_tracking_page_load_metrics_observer.cc (right): https://codereview.chromium.org/2857063002/diff/240001/chrome/browser/page_lo... chrome/browser/page_load_metrics/observers/resource_tracking_page_load_metrics_observer.cc:50: // TODO(petewiL): Store this by type. Until we do, only look at images. On 2017/05/10 19:45:35, RyanSturm wrote: > nit: s/petewiL/petewil/ (capitalized L) unless this is an intentional > stylization Done. https://codereview.chromium.org/2857063002/diff/240001/chrome/browser/page_lo... chrome/browser/page_load_metrics/observers/resource_tracking_page_load_metrics_observer.cc:51: if (extra_request_start_info.resource_type == content::RESOURCE_TYPE_IMAGE) { On 2017/05/10 19:45:35, RyanSturm wrote: > Just a thought, you might want to convert the type here and in OnLoadedResource > before the image comparison (x == offline_pages::ResourceDataType::IMAGE). I > suggest this so that if you change ConvertResourceTypeToResourceDataType the > behavior will be correct here, but I'd be fine with changing it later as this > class changes. I'd like to change it later with the next changelist. The next changelist will add storage for all types of resources, and I haven't yet decided whether to store them as content::ResourceType or offline_pages::ResourceDataType. https://codereview.chromium.org/2857063002/diff/240001/chrome/browser/page_lo... File chrome/browser/page_load_metrics/page_load_metrics_initialize.cc (right): https://codereview.chromium.org/2857063002/diff/240001/chrome/browser/page_lo... chrome/browser/page_load_metrics/page_load_metrics_initialize.cc:143: // Whether we are using the pre-renderer or not, set up a On 2017/05/10 19:45:35, RyanSturm wrote: > Not sure if you saw the previous comment from patch 10: > > "That's a valid point. I've done some other hacky approaches here using > virtualization: > https://cs.chromium.org/chromium/src/chrome/browser/page_load_metrics/observe... > > If you are more comfortable with that approach, it keeps the boundaries a little > more clear. Generally, I agree dependency injection is better though, so I won't > push back." Oh, sorry, I should have replied. Looking at the approach, I prefer dependency injection, so unless your reasons are strong, I'd like to keep the code as is. If you feel strongly about it, I can definitely change to the virtualization approach.
https://codereview.chromium.org/2857063002/diff/240001/chrome/browser/page_lo... File chrome/browser/page_load_metrics/observers/page_load_metrics_observer_test_harness.cc (right): https://codereview.chromium.org/2857063002/diff/240001/chrome/browser/page_lo... chrome/browser/page_load_metrics/observers/page_load_metrics_observer_test_harness.cc:160: info.resource_type, info.was_cached, It looks like this change breaks a lot of tests which are assuming all resources are of type script. Fix to the other tests coming soon.
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: linux_chromium_chromeos_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 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.
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: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
petewil@chromium.org changed reviewers: + chili@chromium.org
RyanSturm: thanks for the LGTM, please see the new changes in PageLoadMetrics to make sure they are OK too. Dimich: OK to look at now, design issue is fixed (now we only send signals when offlining, not on all pages) Chili: Please look at the OfflinerUserData change. https://codereview.chromium.org/2857063002/diff/320001/chrome/browser/page_lo... File chrome/browser/page_load_metrics/observers/offliner_user_data.h (right): https://codereview.chromium.org/2857063002/diff/320001/chrome/browser/page_lo... chrome/browser/page_load_metrics/observers/offliner_user_data.h:13: class OfflinerUserData : public content::WebContentsUserData<OfflinerUserData> { Reviewers: This is a kind of odd place for the UserData, but everything compiles here for now. As soon as Dimich lands the chrome/browser/offline_pages directory change (before I check this in), I plan to move this class to that directory. The file is here now to avoid putting it under an "android" directory, and because I couldn't get it to be a proper dependency in components.
https://codereview.chromium.org/2857063002/diff/320001/chrome/browser/page_lo... File chrome/browser/page_load_metrics/observers/offliner_user_data.h (right): https://codereview.chromium.org/2857063002/diff/320001/chrome/browser/page_lo... chrome/browser/page_load_metrics/observers/offliner_user_data.h:13: class OfflinerUserData : public content::WebContentsUserData<OfflinerUserData> { On 2017/05/16 17:51:19, Pete Williamson wrote: > Reviewers: This is a kind of odd place for the UserData, but everything > compiles here for now. As soon as Dimich lands the chrome/browser/offline_pages > directory change (before I check this in), I plan to move this class to that > directory. > > The file is here now to avoid putting it under an "android" directory, and > because I couldn't get it to be a proper dependency in components. If this is just because it wasn't building resource_tracking_page_load_metrics_observer.cc on desktop. Is it possible to leave it where it is for now (android/ code) and have an #if defined(OS_ANDROID) around the code in OnCommit() and return STOP_OBSERVING if its desktop?
On 2017/05/16 17:56:31, RyanSturm wrote: > https://codereview.chromium.org/2857063002/diff/320001/chrome/browser/page_lo... > File chrome/browser/page_load_metrics/observers/offliner_user_data.h (right): > > https://codereview.chromium.org/2857063002/diff/320001/chrome/browser/page_lo... > chrome/browser/page_load_metrics/observers/offliner_user_data.h:13: class > OfflinerUserData : public content::WebContentsUserData<OfflinerUserData> { > On 2017/05/16 17:51:19, Pete Williamson wrote: > > Reviewers: This is a kind of odd place for the UserData, but everything > > compiles here for now. As soon as Dimich lands the > chrome/browser/offline_pages > > directory change (before I check this in), I plan to move this class to that > > directory. > > > > The file is here now to avoid putting it under an "android" directory, and > > because I couldn't get it to be a proper dependency in components. > > > If this is just because it wasn't building > resource_tracking_page_load_metrics_observer.cc on desktop. Is it possible to > leave it where it is for now (android/ code) and have an #if defined(OS_ANDROID) > around the code in OnCommit() and return STOP_OBSERVING if its desktop? I think a better home for the class is chrome/browser/offline_pages, so I'll move it there as soon as the appropriate change lands, which I expect to be before this changelist lands. It was more a problem of not finding the android directory on platforms like windows. While I already have a #define set up to avoid instantiating the resource_tracking_page_load_metrics_observer code on other platforms, I haven't really set up to prevent building it on other platforms. And indeed, someday we expect to move the code out of android and have it run on other platforms.
On 2017/05/16 20:38:45, Pete Williamson wrote: > On 2017/05/16 17:56:31, RyanSturm wrote: > > > https://codereview.chromium.org/2857063002/diff/320001/chrome/browser/page_lo... > > File chrome/browser/page_load_metrics/observers/offliner_user_data.h (right): > > > > > https://codereview.chromium.org/2857063002/diff/320001/chrome/browser/page_lo... > > chrome/browser/page_load_metrics/observers/offliner_user_data.h:13: class > > OfflinerUserData : public content::WebContentsUserData<OfflinerUserData> { > > On 2017/05/16 17:51:19, Pete Williamson wrote: > > > Reviewers: This is a kind of odd place for the UserData, but everything > > > compiles here for now. As soon as Dimich lands the > > chrome/browser/offline_pages > > > directory change (before I check this in), I plan to move this class to that > > > directory. > > > > > > The file is here now to avoid putting it under an "android" directory, and > > > because I couldn't get it to be a proper dependency in components. > > > > > > If this is just because it wasn't building > > resource_tracking_page_load_metrics_observer.cc on desktop. Is it possible to > > leave it where it is for now (android/ code) and have an #if > defined(OS_ANDROID) > > around the code in OnCommit() and return STOP_OBSERVING if its desktop? > > I think a better home for the class is chrome/browser/offline_pages, so I'll > move it there as soon as the appropriate change lands, which I expect to be > before this changelist lands. > > It was more a problem of not finding the android directory on platforms like > windows. While I already have a #define set up to avoid instantiating the > resource_tracking_page_load_metrics_observer code on other platforms, I haven't > really set up to prevent building it on other platforms. And indeed, someday we > expect to move the code out of android and have it run on other platforms. lgtm % moving it to that directory. I was thinking that you could define out the include as well assuming that was the problem (like https://cs.chromium.org/chromium/src/chrome/browser/previews/previews_infobar...)
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: 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/2857063002/diff/320001/chrome/browser/page_lo... File chrome/browser/page_load_metrics/observers/offliner_user_data.h (right): https://codereview.chromium.org/2857063002/diff/320001/chrome/browser/page_lo... chrome/browser/page_load_metrics/observers/offliner_user_data.h:13: class OfflinerUserData : public content::WebContentsUserData<OfflinerUserData> { On 2017/05/16 17:56:30, RyanSturm wrote: > On 2017/05/16 17:51:19, Pete Williamson wrote: > > Reviewers: This is a kind of odd place for the UserData, but everything > > compiles here for now. As soon as Dimich lands the > chrome/browser/offline_pages > > directory change (before I check this in), I plan to move this class to that > > directory. > > > > The file is here now to avoid putting it under an "android" directory, and > > because I couldn't get it to be a proper dependency in components. > > > If this is just because it wasn't building > resource_tracking_page_load_metrics_observer.cc on desktop. Is it possible to > leave it where it is for now (android/ code) and have an #if defined(OS_ANDROID) > around the code in OnCommit() and return STOP_OBSERVING if its desktop? And the file is now moved to its new home, chrome/browser/offline_pages.
lgtm
lgtm with some questions https://codereview.chromium.org/2857063002/diff/340001/chrome/browser/offline... File chrome/browser/offline_pages/background_loader_offliner.cc (right): https://codereview.chromium.org/2857063002/diff/340001/chrome/browser/offline... chrome/browser/offline_pages/background_loader_offliner.cc:93: return static_cast<BackgroundLoaderOffliner*>(offliner); just out of curiosity, what would happen if you do static_cast<>(nullptr)? https://codereview.chromium.org/2857063002/diff/340001/chrome/browser/offline... chrome/browser/offline_pages/background_loader_offliner.cc:464: chrome::InitializePageLoadMetricsForWebContents(contents); Do we want ALL the metrics observers? Assuming there's not going to be much overhead
csharrison@chromium.org changed reviewers: + csharrison@chromium.org
Quick drive-by https://codereview.chromium.org/2857063002/diff/340001/chrome/browser/offline... File chrome/browser/offline_pages/background_loader_offliner.cc (right): https://codereview.chromium.org/2857063002/diff/340001/chrome/browser/offline... chrome/browser/offline_pages/background_loader_offliner.cc:464: chrome::InitializePageLoadMetricsForWebContents(contents); On 2017/05/17 18:00:07, chili wrote: > Do we want ALL the metrics observers? Assuming there's not going to be much > overhead +1 can you explain the consequences of this change? Will all of our clients see volume changes in metrics?
https://codereview.chromium.org/2857063002/diff/340001/chrome/browser/offline... File chrome/browser/offline_pages/background_loader_offliner.cc (right): https://codereview.chromium.org/2857063002/diff/340001/chrome/browser/offline... chrome/browser/offline_pages/background_loader_offliner.cc:464: chrome::InitializePageLoadMetricsForWebContents(contents); On 2017/05/17 18:30:31, Charlie Harrison wrote: > On 2017/05/17 18:00:07, chili wrote: > > Do we want ALL the metrics observers? Assuming there's not going to be much > > overhead > > +1 can you explain the consequences of this change? Will all of our clients see > volume changes in metrics? I believe the navigation should be entirely in the background, but any foreground/background agnostic metrics would change. petewil, can you verify that all events in these cases will be marked as background and OnStart started_in_foreground is false. It's a good point that we may want to add an extra param to chrome::InitializePageLoadMetricsForWebContents for this if we think it will drastically (or incorrectly) change the population.
https://codereview.chromium.org/2857063002/diff/340001/chrome/browser/offline... File chrome/browser/offline_pages/background_loader_offliner.cc (right): https://codereview.chromium.org/2857063002/diff/340001/chrome/browser/offline... chrome/browser/offline_pages/background_loader_offliner.cc:464: chrome::InitializePageLoadMetricsForWebContents(contents); On 2017/05/17 18:36:58, RyanSturm wrote: > On 2017/05/17 18:30:31, Charlie Harrison wrote: > > On 2017/05/17 18:00:07, chili wrote: > > > Do we want ALL the metrics observers? Assuming there's not going to be much > > > overhead > > > > +1 can you explain the consequences of this change? Will all of our clients > see > > volume changes in metrics? > > I believe the navigation should be entirely in the background, but any > foreground/background agnostic metrics would change. > > petewil, can you verify that all events in these cases will be marked as > background and OnStart started_in_foreground is false. > > It's a good point that we may want to add an extra param to > chrome::InitializePageLoadMetricsForWebContents for this if we think it will > drastically (or incorrectly) change the population. 1 more thing. Is the background offliner a replacement for the pre-render code, and if so is the pre-render code path already causing the PLM observers to be added?
Answers for Chili, CHarrison, and RyanSturm https://codereview.chromium.org/2857063002/diff/340001/chrome/browser/offline... File chrome/browser/offline_pages/background_loader_offliner.cc (right): https://codereview.chromium.org/2857063002/diff/340001/chrome/browser/offline... chrome/browser/offline_pages/background_loader_offliner.cc:464: chrome::InitializePageLoadMetricsForWebContents(contents); On 2017/05/17 18:36:58, RyanSturm wrote: > On 2017/05/17 18:30:31, Charlie Harrison wrote: > > On 2017/05/17 18:00:07, chili wrote: > > > Do we want ALL the metrics observers? Assuming there's not going to be much > > > overhead > > > > +1 can you explain the consequences of this change? Will all of our clients > see > > volume changes in metrics? > > I believe the navigation should be entirely in the background, but any > foreground/background agnostic metrics would change. > > petewil, can you verify that all events in these cases will be marked as > background and OnStart started_in_foreground is false. > > It's a good point that we may want to add an extra param to > chrome::InitializePageLoadMetricsForWebContents for this if we think it will > drastically (or incorrectly) change the population. Chili: We do want to use several metrics observers, not just the resource tracking page load metrics observer. We eventually want to use the First Meaningful Paint and Time To Interactive metric. Foreground web tabs always call AttachTabHelpers and get all of the observers every time (though our ResourceTrackingPageLoadMetrics will be turned off for them). Charlie: This will turn on observers only for background offlining pages. Normally, AttachTabHelpers should turn these on for all foreground pages already, this change just adds the observers for background loaded offline pages. Ryan: 1) all events will be in the background. We do this by creating a web contents with no associated tab. We don't explicitly set started_in_foreground anywhere. Does that answer your question? 2) Background offliner is indeed a replacement for the prerenderer. I'm not hooking up the ResourceTrackingPageLoadMetrics to the prerenderer since we hope to replace it soon.
https://codereview.chromium.org/2857063002/diff/340001/chrome/browser/offline... File chrome/browser/offline_pages/background_loader_offliner.cc (right): https://codereview.chromium.org/2857063002/diff/340001/chrome/browser/offline... chrome/browser/offline_pages/background_loader_offliner.cc:464: chrome::InitializePageLoadMetricsForWebContents(contents); On 2017/05/17 19:50:07, Pete Williamson wrote: > On 2017/05/17 18:36:58, RyanSturm wrote: > > On 2017/05/17 18:30:31, Charlie Harrison wrote: > > > On 2017/05/17 18:00:07, chili wrote: > > > > Do we want ALL the metrics observers? Assuming there's not going to be > much > > > > overhead > > > > > > +1 can you explain the consequences of this change? Will all of our clients > > see > > > volume changes in metrics? > > > > I believe the navigation should be entirely in the background, but any > > foreground/background agnostic metrics would change. > > > > petewil, can you verify that all events in these cases will be marked as > > background and OnStart started_in_foreground is false. > > > > It's a good point that we may want to add an extra param to > > chrome::InitializePageLoadMetricsForWebContents for this if we think it will > > drastically (or incorrectly) change the population. > > Chili: We do want to use several metrics observers, not just the resource > tracking page load metrics observer. We eventually want to use the First > Meaningful Paint and Time To Interactive metric. Foreground web tabs always > call AttachTabHelpers and get all of the observers every time (though our > ResourceTrackingPageLoadMetrics will be turned off for them). > > Charlie: This will turn on observers only for background offlining pages. > Normally, AttachTabHelpers should turn these on for all foreground pages > already, this change just adds the observers for background loaded offline > pages. > > Ryan: > 1) all events will be in the background. We do this by creating a web contents > with no associated tab. We don't explicitly set started_in_foreground anywhere. > Does that answer your question? > > 2) Background offliner is indeed a replacement for the prerenderer. I'm not > hooking up the ResourceTrackingPageLoadMetrics to the prerenderer since we hope > to replace it soon. WRT to point 2. My question was a little more subtle. I had thought that pre-renderer navigations were already tracked in PLM. If those offline pre-renderer navigations are already included by the current offlining logic, then nothing changes by including the navigations in the new background offlining logic.
csharrison@chromium.org changed reviewers: + bmcquade@chromium.org
https://codereview.chromium.org/2857063002/diff/340001/chrome/browser/offline... File chrome/browser/offline_pages/background_loader_offliner.cc (right): https://codereview.chromium.org/2857063002/diff/340001/chrome/browser/offline... chrome/browser/offline_pages/background_loader_offliner.cc:464: chrome::InitializePageLoadMetricsForWebContents(contents); On 2017/05/17 19:54:32, RyanSturm wrote: > On 2017/05/17 19:50:07, Pete Williamson wrote: > > On 2017/05/17 18:36:58, RyanSturm wrote: > > > On 2017/05/17 18:30:31, Charlie Harrison wrote: > > > > On 2017/05/17 18:00:07, chili wrote: > > > > > Do we want ALL the metrics observers? Assuming there's not going to be > > much > > > > > overhead > > > > > > > > +1 can you explain the consequences of this change? Will all of our > clients > > > see > > > > volume changes in metrics? > > > > > > I believe the navigation should be entirely in the background, but any > > > foreground/background agnostic metrics would change. > > > > > > petewil, can you verify that all events in these cases will be marked as > > > background and OnStart started_in_foreground is false. > > > > > > It's a good point that we may want to add an extra param to > > > chrome::InitializePageLoadMetricsForWebContents for this if we think it will > > > drastically (or incorrectly) change the population. > > > > Chili: We do want to use several metrics observers, not just the resource > > tracking page load metrics observer. We eventually want to use the First > > Meaningful Paint and Time To Interactive metric. Foreground web tabs always > > call AttachTabHelpers and get all of the observers every time (though our > > ResourceTrackingPageLoadMetrics will be turned off for them). > > > > Charlie: This will turn on observers only for background offlining pages. > > Normally, AttachTabHelpers should turn these on for all foreground pages > > already, this change just adds the observers for background loaded offline > > pages. > > > > Ryan: > > 1) all events will be in the background. We do this by creating a web > contents > > with no associated tab. We don't explicitly set started_in_foreground > anywhere. > > Does that answer your question? > > > > 2) Background offliner is indeed a replacement for the prerenderer. I'm not > > hooking up the ResourceTrackingPageLoadMetrics to the prerenderer since we > hope > > to replace it soon. > > WRT to point 2. > > My question was a little more subtle. I had thought that pre-renderer > navigations were already tracked in PLM. If those offline pre-renderer > navigations are already included by the current offlining logic, then nothing > changes by including the navigations in the new background offlining logic. Most observers don't want to be notified of prerenders: https://cs.chromium.org/chromium/src/chrome/browser/page_load_metrics/page_lo... I'm a little bit nervous about initializing all observers on WebContents that aren't associated with tabs and don't have the standard tab helpers (some observers have dependencies on other tab helpers). If possible, it might make sense to add more logic in PageLoadMetrics initialization to do what we do today for prerender. +bmcquade for thoughts as well.
Answers for RyanSturm https://codereview.chromium.org/2857063002/diff/340001/chrome/browser/offline... File chrome/browser/offline_pages/background_loader_offliner.cc (right): https://codereview.chromium.org/2857063002/diff/340001/chrome/browser/offline... chrome/browser/offline_pages/background_loader_offliner.cc:464: chrome::InitializePageLoadMetricsForWebContents(contents); On 2017/05/17 19:54:32, RyanSturm wrote: > On 2017/05/17 19:50:07, Pete Williamson wrote: > > On 2017/05/17 18:36:58, RyanSturm wrote: > > > On 2017/05/17 18:30:31, Charlie Harrison wrote: > > > > On 2017/05/17 18:00:07, chili wrote: > > > > > Do we want ALL the metrics observers? Assuming there's not going to be > > much > > > > > overhead > > > > > > > > +1 can you explain the consequences of this change? Will all of our > clients > > > see > > > > volume changes in metrics? > > > > > > I believe the navigation should be entirely in the background, but any > > > foreground/background agnostic metrics would change. > > > > > > petewil, can you verify that all events in these cases will be marked as > > > background and OnStart started_in_foreground is false. > > > > > > It's a good point that we may want to add an extra param to > > > chrome::InitializePageLoadMetricsForWebContents for this if we think it will > > > drastically (or incorrectly) change the population. > > > > Chili: We do want to use several metrics observers, not just the resource > > tracking page load metrics observer. We eventually want to use the First > > Meaningful Paint and Time To Interactive metric. Foreground web tabs always > > call AttachTabHelpers and get all of the observers every time (though our > > ResourceTrackingPageLoadMetrics will be turned off for them). > > > > Charlie: This will turn on observers only for background offlining pages. > > Normally, AttachTabHelpers should turn these on for all foreground pages > > already, this change just adds the observers for background loaded offline > > pages. > > > > Ryan: > > 1) all events will be in the background. We do this by creating a web > contents > > with no associated tab. We don't explicitly set started_in_foreground > anywhere. > > Does that answer your question? > > > > 2) Background offliner is indeed a replacement for the prerenderer. I'm not > > hooking up the ResourceTrackingPageLoadMetrics to the prerenderer since we > hope > > to replace it soon. > > WRT to point 2. > > My question was a little more subtle. I had thought that pre-renderer > navigations were already tracked in PLM. If those offline pre-renderer > navigations are already included by the current offlining logic, then nothing > changes by including the navigations in the new background offlining logic. OK, let me clarify a bit and see if that helps: Today we have two different offliners. We use the prerenderer, and if a certain flag is set, we use the BackgroundLoaderOffliner instead. It seems that the Prerender is already calling AttachTabHelpers, which turns on all the PageLoadMetrics observers. This change turns on the observers, but only for the BackgroundLoaderOffliner.
Answers for CSHarrison https://codereview.chromium.org/2857063002/diff/340001/chrome/browser/offline... File chrome/browser/offline_pages/background_loader_offliner.cc (right): https://codereview.chromium.org/2857063002/diff/340001/chrome/browser/offline... chrome/browser/offline_pages/background_loader_offliner.cc:464: chrome::InitializePageLoadMetricsForWebContents(contents); On 2017/05/17 19:59:07, Charlie Harrison wrote: > On 2017/05/17 19:54:32, RyanSturm wrote: > > On 2017/05/17 19:50:07, Pete Williamson wrote: > > > On 2017/05/17 18:36:58, RyanSturm wrote: > > > > On 2017/05/17 18:30:31, Charlie Harrison wrote: > > > > > On 2017/05/17 18:00:07, chili wrote: > > > > > > Do we want ALL the metrics observers? Assuming there's not going to be > > > much > > > > > > overhead > > > > > > > > > > +1 can you explain the consequences of this change? Will all of our > > clients > > > > see > > > > > volume changes in metrics? > > > > > > > > I believe the navigation should be entirely in the background, but any > > > > foreground/background agnostic metrics would change. > > > > > > > > petewil, can you verify that all events in these cases will be marked as > > > > background and OnStart started_in_foreground is false. > > > > > > > > It's a good point that we may want to add an extra param to > > > > chrome::InitializePageLoadMetricsForWebContents for this if we think it > will > > > > drastically (or incorrectly) change the population. > > > > > > Chili: We do want to use several metrics observers, not just the resource > > > tracking page load metrics observer. We eventually want to use the First > > > Meaningful Paint and Time To Interactive metric. Foreground web tabs always > > > call AttachTabHelpers and get all of the observers every time (though our > > > ResourceTrackingPageLoadMetrics will be turned off for them). > > > > > > Charlie: This will turn on observers only for background offlining pages. > > > Normally, AttachTabHelpers should turn these on for all foreground pages > > > already, this change just adds the observers for background loaded offline > > > pages. > > > > > > Ryan: > > > 1) all events will be in the background. We do this by creating a web > > contents > > > with no associated tab. We don't explicitly set started_in_foreground > > anywhere. > > > Does that answer your question? > > > > > > 2) Background offliner is indeed a replacement for the prerenderer. I'm not > > > hooking up the ResourceTrackingPageLoadMetrics to the prerenderer since we > > hope > > > to replace it soon. > > > > WRT to point 2. > > > > My question was a little more subtle. I had thought that pre-renderer > > navigations were already tracked in PLM. If those offline pre-renderer > > navigations are already included by the current offlining logic, then nothing > > changes by including the navigations in the new background offlining logic. > > Most observers don't want to be notified of prerenders: > https://cs.chromium.org/chromium/src/chrome/browser/page_load_metrics/page_lo... > > I'm a little bit nervous about initializing all observers on WebContents that > aren't associated with tabs and don't have the standard tab helpers (some > observers have dependencies on other tab helpers). > > If possible, it might make sense to add more logic in PageLoadMetrics > initialization to do what we do today for prerender. > > +bmcquade for thoughts as well. CHarrison: Ah, I didn't realize that some observers needed the tab helpers. In that case, we probably want to limit which observers are used when we use the BackgroundLoaderOffliner. Unfortunately, unlike the pre-renderer, the system is generally not aware of when the BackgroundLoaderOffliner is in use, we'd have to add some detection in PageLoadMetricsInitialize. Does adding an additional argument to InitializePageLoadMetricsForWebContents seem reasonable to you?
https://codereview.chromium.org/2857063002/diff/340001/chrome/browser/offline... File chrome/browser/offline_pages/background_loader_offliner.cc (right): https://codereview.chromium.org/2857063002/diff/340001/chrome/browser/offline... chrome/browser/offline_pages/background_loader_offliner.cc:464: chrome::InitializePageLoadMetricsForWebContents(contents); On 2017/05/17 20:09:08, Pete Williamson wrote: > On 2017/05/17 19:59:07, Charlie Harrison wrote: > > On 2017/05/17 19:54:32, RyanSturm wrote: > > > On 2017/05/17 19:50:07, Pete Williamson wrote: > > > > On 2017/05/17 18:36:58, RyanSturm wrote: > > > > > On 2017/05/17 18:30:31, Charlie Harrison wrote: > > > > > > On 2017/05/17 18:00:07, chili wrote: > > > > > > > Do we want ALL the metrics observers? Assuming there's not going to > be > > > > much > > > > > > > overhead > > > > > > > > > > > > +1 can you explain the consequences of this change? Will all of our > > > clients > > > > > see > > > > > > volume changes in metrics? > > > > > > > > > > I believe the navigation should be entirely in the background, but any > > > > > foreground/background agnostic metrics would change. > > > > > > > > > > petewil, can you verify that all events in these cases will be marked as > > > > > background and OnStart started_in_foreground is false. > > > > > > > > > > It's a good point that we may want to add an extra param to > > > > > chrome::InitializePageLoadMetricsForWebContents for this if we think it > > will > > > > > drastically (or incorrectly) change the population. > > > > > > > > Chili: We do want to use several metrics observers, not just the resource > > > > tracking page load metrics observer. We eventually want to use the First > > > > Meaningful Paint and Time To Interactive metric. Foreground web tabs > always > > > > call AttachTabHelpers and get all of the observers every time (though our > > > > ResourceTrackingPageLoadMetrics will be turned off for them). > > > > > > > > Charlie: This will turn on observers only for background offlining pages. > > > > Normally, AttachTabHelpers should turn these on for all foreground pages > > > > already, this change just adds the observers for background loaded offline > > > > pages. > > > > > > > > Ryan: > > > > 1) all events will be in the background. We do this by creating a web > > > contents > > > > with no associated tab. We don't explicitly set started_in_foreground > > > anywhere. > > > > Does that answer your question? > > > > > > > > 2) Background offliner is indeed a replacement for the prerenderer. I'm > not > > > > hooking up the ResourceTrackingPageLoadMetrics to the prerenderer since we > > > hope > > > > to replace it soon. > > > > > > WRT to point 2. > > > > > > My question was a little more subtle. I had thought that pre-renderer > > > navigations were already tracked in PLM. If those offline pre-renderer > > > navigations are already included by the current offlining logic, then > nothing > > > changes by including the navigations in the new background offlining logic. > > > > Most observers don't want to be notified of prerenders: > > > https://cs.chromium.org/chromium/src/chrome/browser/page_load_metrics/page_lo... > > > > I'm a little bit nervous about initializing all observers on WebContents that > > aren't associated with tabs and don't have the standard tab helpers (some > > observers have dependencies on other tab helpers). > > > > If possible, it might make sense to add more logic in PageLoadMetrics > > initialization to do what we do today for prerender. > > > > +bmcquade for thoughts as well. > > CHarrison: Ah, I didn't realize that some observers needed the tab helpers. In > that case, we probably want to limit which observers are used when we use the > BackgroundLoaderOffliner. Unfortunately, unlike the pre-renderer, the system is > generally not aware of when the BackgroundLoaderOffliner is in use, we'd have to > add some detection in PageLoadMetricsInitialize. Does adding an additional > argument to InitializePageLoadMetricsForWebContents seem reasonable to you? Yeah I think it makes sense to align as closely with prerender as possible in this case. I think most of our observers just care about normal page loads.
https://codereview.chromium.org/2857063002/diff/340001/chrome/browser/offline... File chrome/browser/offline_pages/background_loader_offliner.cc (right): https://codereview.chromium.org/2857063002/diff/340001/chrome/browser/offline... chrome/browser/offline_pages/background_loader_offliner.cc:464: chrome::InitializePageLoadMetricsForWebContents(contents); On 2017/05/17 20:14:30, Charlie Harrison wrote: > On 2017/05/17 20:09:08, Pete Williamson wrote: > > On 2017/05/17 19:59:07, Charlie Harrison wrote: > > > On 2017/05/17 19:54:32, RyanSturm wrote: > > > > On 2017/05/17 19:50:07, Pete Williamson wrote: > > > > > On 2017/05/17 18:36:58, RyanSturm wrote: > > > > > > On 2017/05/17 18:30:31, Charlie Harrison wrote: > > > > > > > On 2017/05/17 18:00:07, chili wrote: > > > > > > > > Do we want ALL the metrics observers? Assuming there's not going > to > > be > > > > > much > > > > > > > > overhead > > > > > > > > > > > > > > +1 can you explain the consequences of this change? Will all of our > > > > clients > > > > > > see > > > > > > > volume changes in metrics? > > > > > > > > > > > > I believe the navigation should be entirely in the background, but any > > > > > > foreground/background agnostic metrics would change. > > > > > > > > > > > > petewil, can you verify that all events in these cases will be marked > as > > > > > > background and OnStart started_in_foreground is false. > > > > > > > > > > > > It's a good point that we may want to add an extra param to > > > > > > chrome::InitializePageLoadMetricsForWebContents for this if we think > it > > > will > > > > > > drastically (or incorrectly) change the population. > > > > > > > > > > Chili: We do want to use several metrics observers, not just the > resource > > > > > tracking page load metrics observer. We eventually want to use the > First > > > > > Meaningful Paint and Time To Interactive metric. Foreground web tabs > > always > > > > > call AttachTabHelpers and get all of the observers every time (though > our > > > > > ResourceTrackingPageLoadMetrics will be turned off for them). > > > > > > > > > > Charlie: This will turn on observers only for background offlining > pages. > > > > > Normally, AttachTabHelpers should turn these on for all foreground pages > > > > > already, this change just adds the observers for background loaded > offline > > > > > pages. > > > > > > > > > > Ryan: > > > > > 1) all events will be in the background. We do this by creating a web > > > > contents > > > > > with no associated tab. We don't explicitly set started_in_foreground > > > > anywhere. > > > > > Does that answer your question? > > > > > > > > > > 2) Background offliner is indeed a replacement for the prerenderer. I'm > > not > > > > > hooking up the ResourceTrackingPageLoadMetrics to the prerenderer since > we > > > > hope > > > > > to replace it soon. > > > > > > > > WRT to point 2. > > > > > > > > My question was a little more subtle. I had thought that pre-renderer > > > > navigations were already tracked in PLM. If those offline pre-renderer > > > > navigations are already included by the current offlining logic, then > > nothing > > > > changes by including the navigations in the new background offlining > logic. > > > > > > Most observers don't want to be notified of prerenders: > > > > > > https://cs.chromium.org/chromium/src/chrome/browser/page_load_metrics/page_lo... > > > > > > I'm a little bit nervous about initializing all observers on WebContents > that > > > aren't associated with tabs and don't have the standard tab helpers (some > > > observers have dependencies on other tab helpers). > > > > > > If possible, it might make sense to add more logic in PageLoadMetrics > > > initialization to do what we do today for prerender. > > > > > > +bmcquade for thoughts as well. > > > > CHarrison: Ah, I didn't realize that some observers needed the tab helpers. > In > > that case, we probably want to limit which observers are used when we use the > > BackgroundLoaderOffliner. Unfortunately, unlike the pre-renderer, the system > is > > generally not aware of when the BackgroundLoaderOffliner is in use, we'd have > to > > add some detection in PageLoadMetricsInitialize. Does adding an additional > > argument to InitializePageLoadMetricsForWebContents seem reasonable to you? > > Yeah I think it makes sense to align as closely with prerender as possible in > this case. I think most of our observers just care about normal page loads. CSHarrison: Done.
Yeah that's much better for me. I'll defer to Ryan for the full review. https://codereview.chromium.org/2857063002/diff/360001/chrome/browser/page_lo... File chrome/browser/page_load_metrics/page_load_metrics_initialize.h (right): https://codereview.chromium.org/2857063002/diff/360001/chrome/browser/page_lo... chrome/browser/page_load_metrics/page_load_metrics_initialize.h:16: void InitializePageLoadMetricsForWebContents(content::WebContents* web_contents, Can you document this bool here?
https://codereview.chromium.org/2857063002/diff/360001/chrome/browser/BUILD.gn File chrome/browser/BUILD.gn (right): https://codereview.chromium.org/2857063002/diff/360001/chrome/browser/BUILD.g... chrome/browser/BUILD.gn:804: "offline_pages/offliner_user_data.cc", These should be below under "if (enable_offline_pages)" section. https://codereview.chromium.org/2857063002/diff/360001/chrome/browser/android... File chrome/browser/android/offline_pages/prerendering_loader.cc (right): https://codereview.chromium.org/2857063002/diff/360001/chrome/browser/android... chrome/browser/android/offline_pages/prerendering_loader.cc:91: void PrerenderingLoader::AddResourceSignal(const ResourceDataType type, It is not clear why these are called "Add*" when they constantly re-set the values, since they are called up to 2 times per each loaded resource... Would it be better to call them SetResourceSignal? https://codereview.chromium.org/2857063002/diff/360001/chrome/browser/android... chrome/browser/android/offline_pages/prerendering_loader.cc:217: if (type == ResourceDataType::IMAGE) This conditional can go to AddResourceSignal where the rest of the logic is, then this method can be just removed. https://codereview.chromium.org/2857063002/diff/360001/chrome/browser/android... File chrome/browser/android/offline_pages/prerendering_offliner.cc (right): https://codereview.chromium.org/2857063002/diff/360001/chrome/browser/android... chrome/browser/android/offline_pages/prerendering_offliner.cc:301: loader_->ObserveResourceTracking(type, started_count, completed_count); Why not just call the loader_->AddResourceSignal here? That would eliminate one method. https://codereview.chromium.org/2857063002/diff/360001/chrome/browser/offline... File chrome/browser/offline_pages/background_loader_offliner.cc (right): https://codereview.chromium.org/2857063002/diff/360001/chrome/browser/offline... chrome/browser/offline_pages/background_loader_offliner.cc:496: double percentage = 100.0 * static_cast<double>(completed_count) / Not clear why do we need the percentage here in addition to started/completed. It is always computable if needed later. https://codereview.chromium.org/2857063002/diff/360001/chrome/browser/offline... File chrome/browser/offline_pages/offliner_user_data.cc (right): https://codereview.chromium.org/2857063002/diff/360001/chrome/browser/offline... chrome/browser/offline_pages/offliner_user_data.cc:6: #include "base/logging.h" // TODO: remove before checkin. TODO https://codereview.chromium.org/2857063002/diff/360001/chrome/browser/page_lo... File chrome/browser/page_load_metrics/observers/page_load_metrics_observer_test_harness.cc (right): https://codereview.chromium.org/2857063002/diff/360001/chrome/browser/page_lo... chrome/browser/page_load_metrics/observers/page_load_metrics_observer_test_harness.cc:158: content::ResourceType resource_type = content::RESOURCE_TYPE_SCRIPT; Is this unrelated change? https://codereview.chromium.org/2857063002/diff/360001/chrome/browser/page_lo... File chrome/browser/page_load_metrics/observers/resource_tracking_page_load_metrics_observer.cc (right): https://codereview.chromium.org/2857063002/diff/360001/chrome/browser/page_lo... chrome/browser/page_load_metrics/observers/resource_tracking_page_load_metrics_observer.cc:68: InformObservers(content::ResourceType::RESOURCE_TYPE_IMAGE, started_count_, The started_count_ and completed_count_ may not be passed here since the InformObservers has access to the same class data fields. https://codereview.chromium.org/2857063002/diff/360001/chrome/browser/page_lo... chrome/browser/page_load_metrics/observers/resource_tracking_page_load_metrics_observer.cc:79: offline_pages::Offliner* offliner = Instead of passing a RequestCoordinator to constructor, it's easy to cache the pointer to offliner_ here and directly call ObserveResourceTracking on it. This allos to remove this method from RequestCoordinator as well. https://codereview.chromium.org/2857063002/diff/360001/chrome/browser/page_lo... File chrome/browser/page_load_metrics/observers/resource_tracking_page_load_metrics_observer.h (right): https://codereview.chromium.org/2857063002/diff/360001/chrome/browser/page_lo... chrome/browser/page_load_metrics/observers/resource_tracking_page_load_metrics_observer.h:43: void InformObservers(const content::ResourceType type, Since there is only one 'observer', perhaps it's more clear to call it 'client', and also rename: offline_pages::ResourceTrackerObserver -> offline_pages::ResourceTrackerClient? This way, there is no clash with 'Observers" pattern which assumes multiple observers. https://codereview.chromium.org/2857063002/diff/360001/components/offline_pag... File components/offline_pages/core/background/resource_data_type.h (right): https://codereview.chromium.org/2857063002/diff/360001/components/offline_pag... components/offline_pages/core/background/resource_data_type.h:15: enum ResourceDataType { It's simpler to move this enum into a proper class as a inner enum class. Enums don't usually deserve a separate files. Also, it looks like this enum will always be used in the context of ResourceTrackingPageLoadMetricsObserver::Client (see comment in resource_tracker_observer.h)
thanks! i'll need to circle back but here are some initial comments. https://codereview.chromium.org/2857063002/diff/360001/chrome/browser/offline... File chrome/browser/offline_pages/background_loader_offliner.cc (right): https://codereview.chromium.org/2857063002/diff/360001/chrome/browser/offline... chrome/browser/offline_pages/background_loader_offliner.cc:464: chrome::InitializePageLoadMetricsForWebContents( do we want to bring up the full page load metrics infrastructure, including all observers, for this background loader offliner mode? My sense is we probably don't want to do this - for example I don't think we want to be recording core page load metrics for this context. In general we only want to bring up the page load metrics infrastructure for tabs that are actually being displayed to users. On first glance, I think this is probably out of scope for page load metrics. I'll need to look at this change in a bit more detail to better understand the goals & constraints - will try to follow up soon. https://codereview.chromium.org/2857063002/diff/360001/chrome/browser/page_lo... File chrome/browser/page_load_metrics/observers/page_load_metrics_observer_test_harness.cc (right): https://codereview.chromium.org/2857063002/diff/360001/chrome/browser/page_lo... chrome/browser/page_load_metrics/observers/page_load_metrics_observer_test_harness.cc:158: content::ResourceType resource_type = content::RESOURCE_TYPE_SCRIPT; On 2017/05/18 at 02:28:10, Dmitry Titov wrote: > Is this unrelated change? jkarlin was mentioning some challenges in this area IIRC - I'd like to get his opinion on this change & whether there's something else we could/should do here. https://codereview.chromium.org/2857063002/diff/360001/chrome/browser/page_lo... File chrome/browser/page_load_metrics/observers/resource_tracking_page_load_metrics_observer.cc (right): https://codereview.chromium.org/2857063002/diff/360001/chrome/browser/page_lo... chrome/browser/page_load_metrics/observers/resource_tracking_page_load_metrics_observer.cc:76: if (navigation_handle == nullptr) nav handle should never be null here - have you encountered this? if so that's a serious bug https://codereview.chromium.org/2857063002/diff/360001/chrome/browser/page_lo... File chrome/browser/page_load_metrics/observers/resource_tracking_page_load_metrics_observer.h (right): https://codereview.chromium.org/2857063002/diff/360001/chrome/browser/page_lo... chrome/browser/page_load_metrics/observers/resource_tracking_page_load_metrics_observer.h:43: void InformObservers(const content::ResourceType type, On 2017/05/18 at 02:28:10, Dmitry Titov wrote: > Since there is only one 'observer', perhaps it's more clear to call it 'client', and also rename: > offline_pages::ResourceTrackerObserver -> offline_pages::ResourceTrackerClient? > > This way, there is no clash with 'Observers" pattern which assumes multiple observers. Agree - given that the containing class is also itself a PageLoadMetricsObserver, I think using Client here will help to distinguish. https://codereview.chromium.org/2857063002/diff/360001/chrome/browser/page_lo... File chrome/browser/page_load_metrics/page_load_metrics_initialize.h (right): https://codereview.chromium.org/2857063002/diff/360001/chrome/browser/page_lo... chrome/browser/page_load_metrics/page_load_metrics_initialize.h:17: bool background_rendering); this name 'background_rendering' feels overly specific to be a parameter to this function. i'll need to look at this a bit more, but my initial reaction is we may want to change the way this is structured a bit.
bmcquade@chromium.org changed reviewers: + jkarlin@chromium.org
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...
Answers and CR Fixes for BMcQuade Dimich CSHarrison Chili Basic changes for this patch set are to stop using the RequestCoordinator and pass the results straight to the Offliner, since we have a pointer to it available anyway, and to move the ResourceDataType enum into offliner.h https://codereview.chromium.org/2857063002/diff/340001/chrome/browser/offline... File chrome/browser/offline_pages/background_loader_offliner.cc (right): https://codereview.chromium.org/2857063002/diff/340001/chrome/browser/offline... chrome/browser/offline_pages/background_loader_offliner.cc:93: return static_cast<BackgroundLoaderOffliner*>(offliner); On 2017/05/17 18:00:07, chili wrote: > just out of curiosity, what would happen if you do static_cast<>(nullptr)? You get a nullptr with a C++ type "BackgroundLoaderOffliner" https://codereview.chromium.org/2857063002/diff/340001/chrome/browser/offline... chrome/browser/offline_pages/background_loader_offliner.cc:464: chrome::InitializePageLoadMetricsForWebContents(contents); On 2017/05/17 18:00:07, chili wrote: > Do we want ALL the metrics observers? Assuming there's not going to be much > overhead We only want the ones we use, and InitializePLMFWC() only starts the ones we use, not all of them. https://codereview.chromium.org/2857063002/diff/360001/chrome/browser/BUILD.gn File chrome/browser/BUILD.gn (right): https://codereview.chromium.org/2857063002/diff/360001/chrome/browser/BUILD.g... chrome/browser/BUILD.gn:804: "offline_pages/offliner_user_data.cc", On 2017/05/18 02:28:09, Dmitry Titov wrote: > These should be below under "if (enable_offline_pages)" section. I moved these, but ended up putting them back. When I moved them, the tests in PageLoadMetrics failed, because they need access to these files. I tried putting them in "test_support", but then the browser unit tests end up with two copies of these files. One thing that I didn't try (which I can if you think it is the right thing to do) is to make a new gn target "external_test_support" with just these two files, and using that target from the PageLoadMetrics unit tests. https://codereview.chromium.org/2857063002/diff/360001/chrome/browser/android... File chrome/browser/android/offline_pages/prerendering_loader.cc (right): https://codereview.chromium.org/2857063002/diff/360001/chrome/browser/android... chrome/browser/android/offline_pages/prerendering_loader.cc:91: void PrerenderingLoader::AddResourceSignal(const ResourceDataType type, On 2017/05/18 02:28:10, Dmitry Titov wrote: > It is not clear why these are called "Add*" when they constantly re-set the > values, since they are called up to 2 times per each loaded resource... Would it > be better to call them SetResourceSignal? Moved the content into ObserverResourceTracking everywhere. https://codereview.chromium.org/2857063002/diff/360001/chrome/browser/android... chrome/browser/android/offline_pages/prerendering_loader.cc:217: if (type == ResourceDataType::IMAGE) On 2017/05/18 02:28:10, Dmitry Titov wrote: > This conditional can go to AddResourceSignal where the rest of the logic is, > then this method can be just removed. Kept this method, since it is the exposed interface method, but moved AddResourceSignal into here. https://codereview.chromium.org/2857063002/diff/360001/chrome/browser/android... File chrome/browser/android/offline_pages/prerendering_offliner.cc (right): https://codereview.chromium.org/2857063002/diff/360001/chrome/browser/android... chrome/browser/android/offline_pages/prerendering_offliner.cc:301: loader_->ObserveResourceTracking(type, started_count, completed_count); On 2017/05/18 02:28:10, Dmitry Titov wrote: > Why not just call the loader_->AddResourceSignal here? That would eliminate one > method. Since this is the interface method, I decided to keep it, and merged the contents of AddResourceSignal into the loader's ObserveResourceTracking. https://codereview.chromium.org/2857063002/diff/360001/chrome/browser/offline... File chrome/browser/offline_pages/background_loader_offliner.cc (right): https://codereview.chromium.org/2857063002/diff/360001/chrome/browser/offline... chrome/browser/offline_pages/background_loader_offliner.cc:464: chrome::InitializePageLoadMetricsForWebContents( On 2017/05/23 05:00:01, Bryan McQuade wrote: > do we want to bring up the full page load metrics infrastructure, including all > observers, for this background loader offliner mode? My sense is we probably > don't want to do this - for example I don't think we want to be recording core > page load metrics for this context. In general we only want to bring up the page > load metrics infrastructure for tabs that are actually being displayed to users. > On first glance, I think this is probably out of scope for page load metrics. > > I'll need to look at this change in a bit more detail to better understand the > goals & constraints - will try to follow up soon. As the code is written now, InitializePageLoadMetricsForWebContents will only initialize the observers that we use while doing background offlining, instead of all of them. My intent moving forwards is to leverage other page load metrics for right moment detection. We've talked before about using the FirstMeaningfulPaint and TimeToInteractive(once it is available) metrics. https://codereview.chromium.org/2857063002/diff/360001/chrome/browser/offline... chrome/browser/offline_pages/background_loader_offliner.cc:496: double percentage = 100.0 * static_cast<double>(completed_count) / On 2017/05/18 02:28:10, Dmitry Titov wrote: > Not clear why do we need the percentage here in addition to started/completed. > It is always computable if needed later. OK, removed percentage. https://codereview.chromium.org/2857063002/diff/360001/chrome/browser/offline... File chrome/browser/offline_pages/offliner_user_data.cc (right): https://codereview.chromium.org/2857063002/diff/360001/chrome/browser/offline... chrome/browser/offline_pages/offliner_user_data.cc:6: #include "base/logging.h" // TODO: remove before checkin. On 2017/05/18 02:28:10, Dmitry Titov wrote: > TODO Done. https://codereview.chromium.org/2857063002/diff/360001/chrome/browser/page_lo... File chrome/browser/page_load_metrics/observers/page_load_metrics_observer_test_harness.cc (right): https://codereview.chromium.org/2857063002/diff/360001/chrome/browser/page_lo... chrome/browser/page_load_metrics/observers/page_load_metrics_observer_test_harness.cc:158: content::ResourceType resource_type = content::RESOURCE_TYPE_SCRIPT; On 2017/05/18 02:28:10, Dmitry Titov wrote: > Is this unrelated change? This is a bug that we need to fix to enable the new tests to pass. It could easily be split into a different changelist, LMK if you prefer that, and I'll split it out. https://codereview.chromium.org/2857063002/diff/360001/chrome/browser/page_lo... File chrome/browser/page_load_metrics/observers/resource_tracking_page_load_metrics_observer.cc (right): https://codereview.chromium.org/2857063002/diff/360001/chrome/browser/page_lo... chrome/browser/page_load_metrics/observers/resource_tracking_page_load_metrics_observer.cc:68: InformObservers(content::ResourceType::RESOURCE_TYPE_IMAGE, started_count_, On 2017/05/18 02:28:10, Dmitry Titov wrote: > The started_count_ and completed_count_ may not be passed here since the > InformObservers has access to the same class data fields. Done. https://codereview.chromium.org/2857063002/diff/360001/chrome/browser/page_lo... chrome/browser/page_load_metrics/observers/resource_tracking_page_load_metrics_observer.cc:76: if (navigation_handle == nullptr) On 2017/05/23 05:00:01, Bryan McQuade wrote: > nav handle should never be null here - have you encountered this? if so that's a > serious bug Never encountered it, just programming defensively. Changed to a DCHECK. https://codereview.chromium.org/2857063002/diff/360001/chrome/browser/page_lo... chrome/browser/page_load_metrics/observers/resource_tracking_page_load_metrics_observer.cc:79: offline_pages::Offliner* offliner = On 2017/05/18 02:28:10, Dmitry Titov wrote: > Instead of passing a RequestCoordinator to constructor, it's easy to cache the > pointer to offliner_ here and directly call ObserveResourceTracking on it. This > allos to remove this method from RequestCoordinator as well. Done. https://codereview.chromium.org/2857063002/diff/360001/chrome/browser/page_lo... File chrome/browser/page_load_metrics/observers/resource_tracking_page_load_metrics_observer.h (right): https://codereview.chromium.org/2857063002/diff/360001/chrome/browser/page_lo... chrome/browser/page_load_metrics/observers/resource_tracking_page_load_metrics_observer.h:43: void InformObservers(const content::ResourceType type, On 2017/05/18 02:28:10, Dmitry Titov wrote: > Since there is only one 'observer', perhaps it's more clear to call it 'client', > and also rename: > offline_pages::ResourceTrackerObserver -> offline_pages::ResourceTrackerClient? > > This way, there is no clash with 'Observers" pattern which assumes multiple > observers. Renamed this function to InformClient. We can safely remove the ResourceTrackerObserver class, since Dimich pointed out that we can use the interface directly on the offliner, so I removed that class and all references to it instead of renaming it. https://codereview.chromium.org/2857063002/diff/360001/chrome/browser/page_lo... File chrome/browser/page_load_metrics/page_load_metrics_initialize.h (right): https://codereview.chromium.org/2857063002/diff/360001/chrome/browser/page_lo... chrome/browser/page_load_metrics/page_load_metrics_initialize.h:16: void InitializePageLoadMetricsForWebContents(content::WebContents* web_contents, On 2017/05/17 23:56:24, Charlie Harrison wrote: > Can you document this bool here? Done. https://codereview.chromium.org/2857063002/diff/360001/chrome/browser/page_lo... chrome/browser/page_load_metrics/page_load_metrics_initialize.h:17: bool background_rendering); On 2017/05/23 05:00:01, Bryan McQuade wrote: > this name 'background_rendering' feels overly specific to be a parameter to this > function. > > i'll need to look at this a bit more, but my initial reaction is we may want to > change the way this is structured a bit. What do you think of a separate init function for just initializing the metrics we want in background mode? https://codereview.chromium.org/2857063002/diff/360001/components/offline_pag... File components/offline_pages/core/background/resource_data_type.h (right): https://codereview.chromium.org/2857063002/diff/360001/components/offline_pag... components/offline_pages/core/background/resource_data_type.h:15: enum ResourceDataType { On 2017/05/18 02:28:10, Dmitry Titov wrote: > It's simpler to move this enum into a proper class as a inner enum class. Enums > don't > usually deserve a separate files. Also, it looks like this enum will always be > used in the context of ResourceTrackingPageLoadMetricsObserver::Client (see > comment in resource_tracker_observer.h) Since it is referenced in offliner.h, i moved it into offliner.h This enum is used by offliner.h, the PageLoadMetrics code (ResourceTrackingPageLoadMetricsObserver) and the OfflinePages code (BackgroundLoadingOffliner).
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
I'm not sure why, but the comments I tried to post to this change earlier seemed to get lost. Trying again. If this was already posted, apologies: Charles and I got to talk about this a bit, and came to the conclusion that this use case is not a good match for page load metrics. The core page load metrics classes, such as MWCO and PageLoadTracker, have a bunch of internal UMA metrics that they track to understand behavior for pages that are loaded in tabs that users see. These metrics are important since they let us reason about what kinds of things we see for real web pages in the field as well as how frequently we see those things. By bringing page load metrics up in other contexts, such as the background offliner, it's no longer possible to use these metrics to reason about the behavior of page load metrics loaded in tabs that users see, because we're recording metrics for both tab helpers and background offliners. We could try to factor the code such that metrics are only recorded in some contexts, but I feel that this complexity is probably not warranted. Fortunately, Charles and I think it may actually be straightforward to break your dependency on page load metrics here - the resulting code might end up being simpler than the PLM integration we have in this change. Assuming you only load one page per background offliner web contents, you should be able to just add callbacks from ChromeResourceDispatcherHostDelegate for resource start / complete, that call directly into your OfflinerUserData to record metrics scoped to a background offline load. Hopefully this is a workable solution for you. I'm sorry to not be able to support this case in page load metrics. Charles and I would be happy to discuss the alternate implementation in more detail over VC if that would help. Thanks, Bryan
https://codereview.chromium.org/2857063002/diff/360001/chrome/browser/BUILD.gn File chrome/browser/BUILD.gn (right): https://codereview.chromium.org/2857063002/diff/360001/chrome/browser/BUILD.g... chrome/browser/BUILD.gn:804: "offline_pages/offliner_user_data.cc", This does not sound right. This is 'browser' target, it has nothing to do with building unittests. I may be wrong here, but none of other offline_pages files are included unconditionally and there are other unittests depending on them. Could you double-check please? https://codereview.chromium.org/2857063002/diff/380001/chrome/browser/offline... File chrome/browser/offline_pages/offliner_user_data.cc (right): https://codereview.chromium.org/2857063002/diff/380001/chrome/browser/offline... chrome/browser/offline_pages/offliner_user_data.cc:13: new OfflinerUserData(offliner))); formatting... git cl format? https://codereview.chromium.org/2857063002/diff/380001/chrome/browser/page_lo... File chrome/browser/page_load_metrics/observers/resource_tracking_page_load_metrics_observer.cc (right): https://codereview.chromium.org/2857063002/diff/380001/chrome/browser/page_lo... chrome/browser/page_load_metrics/observers/resource_tracking_page_load_metrics_observer.cc:47: // TODO(petewil): Store this by type. Until we do, only look at images. Why limit to images here? This looks like a general-purpose resource loading observer, the concern of a specific endpoint not being able to report about other types should probably be solved there. Otherwise it's unclear why to even have ConvertResourceTypeToResourceDataType(). For example, you could just call InformClient(offline_pages::Offliner::ResourceDataType::IMAGE) and avoid having that conversion method altogether. https://codereview.chromium.org/2857063002/diff/380001/chrome/browser/page_lo... chrome/browser/page_load_metrics/observers/resource_tracking_page_load_metrics_observer.cc:48: if (extra_request_start_info.resource_type == content::RESOURCE_TYPE_IMAGE) { Between next 3 lines, content::RESOURCE_TYPE_IMAGE is used as also content::ResourceType::RESOURCE_TYPE_IMAGE. There should be one way to use it. https://codereview.chromium.org/2857063002/diff/380001/chrome/browser/page_lo... chrome/browser/page_load_metrics/observers/resource_tracking_page_load_metrics_observer.cc:61: content::RESOURCE_TYPE_IMAGE) { formatting. git cl format this CL please. https://codereview.chromium.org/2857063002/diff/380001/chrome/browser/page_lo... File chrome/browser/page_load_metrics/observers/resource_tracking_page_load_metrics_observer.h (right): https://codereview.chromium.org/2857063002/diff/380001/chrome/browser/page_lo... chrome/browser/page_load_metrics/observers/resource_tracking_page_load_metrics_observer.h:51: // kept valid by the caller for the lifetime of the PageLoadMetricsObserver. It is hard to say if "must be kept valid" requirement is already enforced somehow or not. Would a weak ref work here? Our offliners have WeakRefFactories...
Putting this changelist on hold for now since BMcQuade has suggested an alternate approach.
https://codereview.chromium.org/2857063002/diff/380001/chrome/browser/page_lo... File chrome/browser/page_load_metrics/observers/page_load_metrics_observer_test_harness.cc (right): https://codereview.chromium.org/2857063002/diff/380001/chrome/browser/page_lo... chrome/browser/page_load_metrics/observers/page_load_metrics_observer_test_harness.cc:162: if (info.resource_type != content::RESOURCE_TYPE_MAIN_FRAME) Changing the resource_type like this isn't okay here. It means that AdsPageLoadMetricsObserverTest.NoHistogramWithoutCommit isn't testing what it thinks it's testing. The test may still pass, but it also won't fail anymore if the code regresses.
Per comments, are you planning to close this issue and open another or modify this one later?
On 2017/06/06 04:43:44, Dmitry Titov wrote: > Per comments, are you planning to close this issue and open another or modify > this one later? The plan is to close this issue. I am removing reviewers, and I will keep the issue for a bit longer in case we have another direction change.
Description was changed from ========== Add a way to send the resource percentage signal to the RC. This lets us send the resource completeness signals to the RequestCoordinator. Unit tests are not yet added. BUG=699313 ========== to ========== Add a way to send the resource percentage signal to the RC. This lets us send the resource completeness signals to the RequestCoordinator. Unit tests are not yet added. BUG=699313 ==========
|
