Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2916253002:
Add UMA to determine how often we offline a page with previews. (Closed)
|
Created:
3 years, 6 months ago by Pete Williamson Modified:
3 years, 6 months ago CC:
chromium-reviews, jam, Randy Smith (Not in Mondays), darin-cc_chromium.org, asvitkine+watch_chromium.org, loading-reviews_chromium.org, mmenke Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdd UMA to determine how often we offline a page with previews.
We suspect that page previews may be causing problems with offline
pages. To understand the severity of the problem, we'll add a new
histogram that measures how often we turn on previews while offlining
a page.
BUG=703875
Review-Url: https://codereview.chromium.org/2916253002
Cr-Commit-Position: refs/heads/master@{#478483}
Committed: https://chromium.googlesource.com/chromium/src/+/61b63cbc244f348331c91d78402eb843bd185408
Patch Set 1 #
Total comments: 23
Patch Set 2 : CR feedback per RyanSturm and Chili #Patch Set 3 : Add a unit test, and a bit more detail to an existing test. #
Total comments: 2
Patch Set 4 : Added a check for a server offlined lite page, and both ways of disabling #Patch Set 5 : Fix copying of previews_state with the ChromeNavigationData object #
Total comments: 4
Patch Set 6 : CR fixes per RyanSturm #
Total comments: 2
Patch Set 7 : Histograms fix #
Messages
Total messages: 46 (19 generated)
petewil@chromium.org changed reviewers: + chili@chromium.org, ryansturm@chromium.org
https://codereview.chromium.org/2916253002/diff/1/chrome/browser/offline_page... File chrome/browser/offline_pages/background_loader_offliner.cc (right): https://codereview.chromium.org/2916253002/diff/1/chrome/browser/offline_page... chrome/browser/offline_pages/background_loader_offliner.cc:333: // Use some form of RTTI to know what I have before I cast it? Reviewers: What is your preference here? I'm thinking of adding the previews_state function to NavigationData, but I'm open to other options.
https://codereview.chromium.org/2916253002/diff/1/chrome/browser/offline_page... File chrome/browser/offline_pages/background_loader_offliner.cc (right): https://codereview.chromium.org/2916253002/diff/1/chrome/browser/offline_page... chrome/browser/offline_pages/background_loader_offliner.cc:80: int previews_enabling = 0; alternate names: preview_state_numeration? preview_state? is_previews_enabled? https://codereview.chromium.org/2916253002/diff/1/chrome/browser/offline_page... chrome/browser/offline_pages/background_loader_offliner.cc:333: // Use some form of RTTI to know what I have before I cast it? On 2017/06/02 00:47:37, Pete Williamson wrote: > Reviewers: What is your preference here? I'm thinking of adding the > previews_state function to NavigationData, but I'm open to other options. I would make a function called IsAnyPreviewsEnabled() in NavigationData (perhaps ryan can suggest better location)
https://codereview.chromium.org/2916253002/diff/1/chrome/browser/loader/chrom... File chrome/browser/loader/chrome_navigation_data.cc (right): https://codereview.chromium.org/2916253002/diff/1/chrome/browser/loader/chrom... chrome/browser/loader/chrome_navigation_data.cc:8: #include "components/data_reduction_proxy/core/browser/data_reduction_proxy_data.h" Don't need data_reduction_proxy_data.h included in the header and here. https://codereview.chromium.org/2916253002/diff/1/chrome/browser/loader/chrom... chrome/browser/loader/chrome_navigation_data.cc:35: if (data_reduction_proxy_data_) You need to also copy the previews state over. https://codereview.chromium.org/2916253002/diff/1/chrome/browser/loader/chrom... File chrome/browser/loader/chrome_navigation_data.h (right): https://codereview.chromium.org/2916253002/diff/1/chrome/browser/loader/chrom... chrome/browser/loader/chrome_navigation_data.h:12: #include "components/data_reduction_proxy/core/browser/data_reduction_proxy_data.h" Any reason for switching forward declare to header? Is it needed for the std::move so that the destructor is declared? https://codereview.chromium.org/2916253002/diff/1/chrome/browser/offline_page... File chrome/browser/offline_pages/background_loader_offliner.cc (right): https://codereview.chromium.org/2916253002/diff/1/chrome/browser/offline_page... chrome/browser/offline_pages/background_loader_offliner.cc:86: "OfflinePages.Background.OffliningPreviewStatus"), As a note, if you care about offline previews (which I don't think you do, since you don't need to re-offline the page), you would need to add extra handling. Even if you do think it's a case you might care about, a follow-up CL might be better for it. https://codereview.chromium.org/2916253002/diff/1/chrome/browser/offline_page... chrome/browser/offline_pages/background_loader_offliner.cc:333: // Use some form of RTTI to know what I have before I cast it? On 2017/06/02 01:38:07, chili wrote: > On 2017/06/02 00:47:37, Pete Williamson wrote: > > Reviewers: What is your preference here? I'm thinking of adding the > > previews_state function to NavigationData, but I'm open to other options. > > I would make a function called IsAnyPreviewsEnabled() in NavigationData (perhaps > ryan can suggest better location) So the basic idea is that Chrome will only ever implement one type of subclass, but other projects that use content/ may have their own implementation. Specifically, there are comments in navigation_handle.h that state that this NavgiationData will be the same as the one returned from GetNavigationData. It's an ugly layering contract, but you can add the same comment as from here (https://cs.chromium.org/chromium/src/chrome/browser/page_load_metrics/observe...) if you like. https://codereview.chromium.org/2916253002/diff/1/chrome/browser/offline_page... File chrome/browser/offline_pages/background_loader_offliner_unittest.cc (right): https://codereview.chromium.org/2916253002/diff/1/chrome/browser/offline_page... chrome/browser/offline_pages/background_loader_offliner_unittest.cc:591: 0, // PreviewsOff Seems like you should have a test for PreviewsOn as well. WebContentsTester exposes a method for setting the navigation data. https://cs.chromium.org/chromium/src/content/public/test/web_contents_tester.... The implementation is fairly straightforward, so I believe you could just call it with the web_contents_tester() and any navigationhandle. https://codereview.chromium.org/2916253002/diff/1/content/public/common/previ... File content/public/common/previews_state.h (right): https://codereview.chromium.org/2916253002/diff/1/content/public/common/previ... content/public/common/previews_state.h:5: #ifndef CONTENT_PUBLIC_COMMON_PREVIEWS_STATE_H_ nice catch! thanks! https://codereview.chromium.org/2916253002/diff/1/tools/metrics/histograms/en... File tools/metrics/histograms/enums.xml (right): https://codereview.chromium.org/2916253002/diff/1/tools/metrics/histograms/en... tools/metrics/histograms/enums.xml:29121: +<enum name="PreviewsEnabling" type="int"> nit: Sort of a weird name for this. Maybe Just use the existing BooleanEnabled enum (and boolean throughout the metrics part of the CL). OfflinePages.Background.OffliningPreviewEnabled might be a better name for the histogram in that case. Whatever is clearest to you is fine by me though.
Also, have you verified this works with chrome://histograms? https://codereview.chromium.org/2916253002/diff/1/chrome/browser/offline_page... File chrome/browser/offline_pages/background_loader_offliner.cc (right): https://codereview.chromium.org/2916253002/diff/1/chrome/browser/offline_page... chrome/browser/offline_pages/background_loader_offliner.cc:333: // Use some form of RTTI to know what I have before I cast it? On 2017/06/02 20:50:33, RyanSturm wrote: > On 2017/06/02 01:38:07, chili wrote: > > On 2017/06/02 00:47:37, Pete Williamson wrote: > > > Reviewers: What is your preference here? I'm thinking of adding the > > > previews_state function to NavigationData, but I'm open to other options. > > > > I would make a function called IsAnyPreviewsEnabled() in NavigationData > (perhaps > > ryan can suggest better location) > > So the basic idea is that Chrome will only ever implement one type of subclass, > but other projects that use content/ may have their own implementation. > > Specifically, there are comments in navigation_handle.h that state that this > NavgiationData will be the same as the one returned from GetNavigationData. > > It's an ugly layering contract, but you can add the same comment as from here > (https://cs.chromium.org/chromium/src/chrome/browser/page_load_metrics/observe...) > if you like. Also, if you like, I would not be opposed to adding a static method to ChromeNavigationData that takes a NavigationData. Something like "ChromeNavigationData* ChromeNavigationdata(NavigationData* navigation_data)" that moves the cast into the ChromeNavigationData code. Also, I think static_cast would be better than reinterpret_cast here.
https://codereview.chromium.org/2916253002/diff/1/chrome/browser/offline_page... File chrome/browser/offline_pages/background_loader_offliner.cc (right): https://codereview.chromium.org/2916253002/diff/1/chrome/browser/offline_page... chrome/browser/offline_pages/background_loader_offliner.cc:333: // Use some form of RTTI to know what I have before I cast it? On 2017/06/02 20:56:29, RyanSturm wrote: > On 2017/06/02 20:50:33, RyanSturm wrote: > > On 2017/06/02 01:38:07, chili wrote: > > > On 2017/06/02 00:47:37, Pete Williamson wrote: > > > > Reviewers: What is your preference here? I'm thinking of adding the > > > > previews_state function to NavigationData, but I'm open to other options. > > > > > > I would make a function called IsAnyPreviewsEnabled() in NavigationData > > (perhaps > > > ryan can suggest better location) > > > > So the basic idea is that Chrome will only ever implement one type of > subclass, > > but other projects that use content/ may have their own implementation. > > > > Specifically, there are comments in navigation_handle.h that state that this > > NavgiationData will be the same as the one returned from GetNavigationData. > > > > It's an ugly layering contract, but you can add the same comment as from here > > > (https://cs.chromium.org/chromium/src/chrome/browser/page_load_metrics/observe...) > > if you like. > > Also, if you like, I would not be opposed to adding a static method to > ChromeNavigationData that takes a NavigationData. Something like > "ChromeNavigationData* ChromeNavigationdata(NavigationData* navigation_data)" > that moves the cast into the ChromeNavigationData code. > > Also, I think static_cast would be better than reinterpret_cast here. Ooops: typo above: s/"ChromeNavigationData* ChromeNavigationdata(NavigationData* navigation_data)"/"ChromeNavigationData* ChromeNavigationdata::FromNavigationData(NavigationData* navigation_data)"
RyanSturm: Yes, I did test these at chrome:histograms, and I saw both the enabled and disabled counts go up at the appropriate times. https://codereview.chromium.org/2916253002/diff/1/chrome/browser/loader/chrom... File chrome/browser/loader/chrome_navigation_data.cc (right): https://codereview.chromium.org/2916253002/diff/1/chrome/browser/loader/chrom... chrome/browser/loader/chrome_navigation_data.cc:8: #include "components/data_reduction_proxy/core/browser/data_reduction_proxy_data.h" On 2017/06/02 20:50:33, RyanSturm wrote: > Don't need data_reduction_proxy_data.h included in the header and here. Done. https://codereview.chromium.org/2916253002/diff/1/chrome/browser/loader/chrom... chrome/browser/loader/chrome_navigation_data.cc:35: if (data_reduction_proxy_data_) On 2017/06/02 20:50:33, RyanSturm wrote: > You need to also copy the previews state over. Done. https://codereview.chromium.org/2916253002/diff/1/chrome/browser/loader/chrom... File chrome/browser/loader/chrome_navigation_data.h (right): https://codereview.chromium.org/2916253002/diff/1/chrome/browser/loader/chrom... chrome/browser/loader/chrome_navigation_data.h:12: #include "components/data_reduction_proxy/core/browser/data_reduction_proxy_data.h" On 2017/06/02 20:50:33, RyanSturm wrote: > Any reason for switching forward declare to header? Is it needed for the > std::move so that the destructor is declared? It was a different compile error: error: invalid application of 'sizeof' to an incomplete type 'data_reduction_proxy::DataReductionProxyData' https://codereview.chromium.org/2916253002/diff/1/chrome/browser/offline_page... File chrome/browser/offline_pages/background_loader_offliner.cc (right): https://codereview.chromium.org/2916253002/diff/1/chrome/browser/offline_page... chrome/browser/offline_pages/background_loader_offliner.cc:80: int previews_enabling = 0; On 2017/06/02 01:38:07, chili wrote: > alternate names: preview_state_numeration? preview_state? is_previews_enabled? picked "is_previews_enabled" https://codereview.chromium.org/2916253002/diff/1/chrome/browser/offline_page... chrome/browser/offline_pages/background_loader_offliner.cc:86: "OfflinePages.Background.OffliningPreviewStatus"), On 2017/06/02 20:50:33, RyanSturm wrote: > As a note, if you care about offline previews (which I don't think you do, since > you don't need to re-offline the page), you would need to add extra handling. > Even if you do think it's a case you might care about, a follow-up CL might be > better for it. Noted. I don't think we care about it. https://codereview.chromium.org/2916253002/diff/1/chrome/browser/offline_page... chrome/browser/offline_pages/background_loader_offliner.cc:333: // Use some form of RTTI to know what I have before I cast it? On 2017/06/02 20:56:29, RyanSturm wrote: > On 2017/06/02 20:50:33, RyanSturm wrote: > > On 2017/06/02 01:38:07, chili wrote: > > > On 2017/06/02 00:47:37, Pete Williamson wrote: > > > > Reviewers: What is your preference here? I'm thinking of adding the > > > > previews_state function to NavigationData, but I'm open to other options. > > > > > > I would make a function called IsAnyPreviewsEnabled() in NavigationData > > (perhaps > > > ryan can suggest better location) > > > > So the basic idea is that Chrome will only ever implement one type of > subclass, > > but other projects that use content/ may have their own implementation. > > > > Specifically, there are comments in navigation_handle.h that state that this > > NavgiationData will be the same as the one returned from GetNavigationData. > > > > It's an ugly layering contract, but you can add the same comment as from here > > > (https://cs.chromium.org/chromium/src/chrome/browser/page_load_metrics/observe...) > > if you like. > > Also, if you like, I would not be opposed to adding a static method to > ChromeNavigationData that takes a NavigationData. Something like > "ChromeNavigationData* ChromeNavigationdata(NavigationData* navigation_data)" > that moves the cast into the ChromeNavigationData code. > > Also, I think static_cast would be better than reinterpret_cast here. Changed to static cast. Decided not to move the code into ChromeNavigationData for now, but I can if you prefer it. https://codereview.chromium.org/2916253002/diff/1/chrome/browser/offline_page... File chrome/browser/offline_pages/background_loader_offliner_unittest.cc (right): https://codereview.chromium.org/2916253002/diff/1/chrome/browser/offline_page... chrome/browser/offline_pages/background_loader_offliner_unittest.cc:591: 0, // PreviewsOff On 2017/06/02 20:50:34, RyanSturm wrote: > Seems like you should have a test for PreviewsOn as well. > > WebContentsTester exposes a method for setting the navigation data. > > https://cs.chromium.org/chromium/src/content/public/test/web_contents_tester.... > > The implementation is fairly straightforward, so I believe you could just call > it with the web_contents_tester() and any navigationhandle. I did try that, but it turns out to be problematic, because the call to set_navigation_data() forced me to include a header file that is forbidden by deps of the unit test. Know any good workarounds? https://codereview.chromium.org/2916253002/diff/1/tools/metrics/histograms/en... File tools/metrics/histograms/enums.xml (right): https://codereview.chromium.org/2916253002/diff/1/tools/metrics/histograms/en... tools/metrics/histograms/enums.xml:29121: +<enum name="PreviewsEnabling" type="int"> On 2017/06/02 20:50:34, RyanSturm wrote: > nit: > > Sort of a weird name for this. Maybe Just use the existing BooleanEnabled enum > (and boolean throughout the metrics part of the CL). > OfflinePages.Background.OffliningPreviewEnabled might be a better name for the > histogram in that case. > > Whatever is clearest to you is fine by me though. Done
https://codereview.chromium.org/2916253002/diff/1/chrome/browser/offline_page... File chrome/browser/offline_pages/background_loader_offliner_unittest.cc (right): https://codereview.chromium.org/2916253002/diff/1/chrome/browser/offline_page... chrome/browser/offline_pages/background_loader_offliner_unittest.cc:591: 0, // PreviewsOff On 2017/06/02 21:40:32, Pete Williamson wrote: > On 2017/06/02 20:50:34, RyanSturm wrote: > > Seems like you should have a test for PreviewsOn as well. > > > > WebContentsTester exposes a method for setting the navigation data. > > > > > https://cs.chromium.org/chromium/src/content/public/test/web_contents_tester.... > > > > The implementation is fairly straightforward, so I believe you could just call > > it with the web_contents_tester() and any navigationhandle. > > I did try that, but it turns out to be problematic, because the call to > set_navigation_data() forced me to include a header file that is forbidden by > deps of the unit test. Know any good workarounds? I think that header is already added to this unittest on line 31. Maybe you tried to include content/browser/frame_host/navigation_handle_impl.h instead of web_contents_tester.h
https://codereview.chromium.org/2916253002/diff/1/chrome/browser/offline_page... File chrome/browser/offline_pages/background_loader_offliner_unittest.cc (right): https://codereview.chromium.org/2916253002/diff/1/chrome/browser/offline_page... chrome/browser/offline_pages/background_loader_offliner_unittest.cc:591: 0, // PreviewsOff On 2017/06/02 22:14:25, RyanSturm wrote: > On 2017/06/02 21:40:32, Pete Williamson wrote: > > On 2017/06/02 20:50:34, RyanSturm wrote: > > > Seems like you should have a test for PreviewsOn as well. > > > > > > WebContentsTester exposes a method for setting the navigation data. > > > > > > > > > https://cs.chromium.org/chromium/src/content/public/test/web_contents_tester.... > > > > > > The implementation is fairly straightforward, so I believe you could just > call > > > it with the web_contents_tester() and any navigationhandle. > > > > I did try that, but it turns out to be problematic, because the call to > > set_navigation_data() forced me to include a header file that is forbidden by > > deps of the unit test. Know any good workarounds? > > I think that header is already added to this unittest on line 31. Maybe you > tried to include content/browser/frame_host/navigation_handle_impl.h instead of > web_contents_tester.h Yep, that worked, test added. We now test both off and on.
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...)
https://codereview.chromium.org/2916253002/diff/40001/chrome/browser/offline_... File chrome/browser/offline_pages/background_loader_offliner.cc (right): https://codereview.chromium.org/2916253002/diff/40001/chrome/browser/offline_... chrome/browser/offline_pages/background_loader_offliner.cc:81: if (previews_state != content::PreviewsTypes::PREVIEWS_OFF) There is some subtlety with PREVIEWS_OFF vs PREVIEWS_NO_TRANSFORM. PREVIEWS_OFF can actually have a server previews (like LoFi or LitePages), but this can only happen when a user is opted into lite page previews via flags. It's an area we are still sorting out. However, ChromeNavigationData->data_reduction_proxy_data()->lite_page_received() will tell if it is a lite page. I don't think there is a way to currently get LoFi images without the bits set fortunately. How about the condition checks (ChromeNavigationData->data_reduction_proxy_data()->lite_page_received() || !(previews_state == content::PreviewsTypes::PREVIEWS_OFF || previews_state == content::PreviewsTypes::PREVIEWS_OFF)) Sorry about the confusion as this model is changing slightly throughout M61. Perhaps dougarnett@ could take a look to make sure this set of checks will be sufficient.
RyanSturm: When I look at this with logging at runtime, I typically see PREVIEWS_UNSPECIFIED. Does that mean that I'm checking too early in the process, before we know for sure whether we will be applying previews or not? It does seem to correlate with when I see a previewed page, but I thought I should double check. https://codereview.chromium.org/2916253002/diff/40001/chrome/browser/offline_... File chrome/browser/offline_pages/background_loader_offliner.cc (right): https://codereview.chromium.org/2916253002/diff/40001/chrome/browser/offline_... chrome/browser/offline_pages/background_loader_offliner.cc:81: if (previews_state != content::PreviewsTypes::PREVIEWS_OFF) On 2017/06/05 20:43:59, RyanSturm wrote: > There is some subtlety with PREVIEWS_OFF vs PREVIEWS_NO_TRANSFORM. PREVIEWS_OFF > can actually have a server previews (like LoFi or LitePages), but this can only > happen when a user is opted into lite page previews via flags. > > It's an area we are still sorting out. However, > ChromeNavigationData->data_reduction_proxy_data()->lite_page_received() will > tell if it is a lite page. I don't think there is a way to currently get LoFi > images without the bits set fortunately. > > How about the condition checks > (ChromeNavigationData->data_reduction_proxy_data()->lite_page_received() || > !(previews_state == content::PreviewsTypes::PREVIEWS_OFF || previews_state == > content::PreviewsTypes::PREVIEWS_OFF)) > > Sorry about the confusion as this model is changing slightly throughout M61. > Perhaps dougarnett@ could take a look to make sure this set of checks will be > sufficient. Added checks as you suggest (but worded slightly differently)
On 2017/06/05 21:40:47, Pete Williamson wrote: > RyanSturm: When I look at this with logging at runtime, I typically see > PREVIEWS_UNSPECIFIED. Does that mean that I'm checking too early in the process, > before we know for sure whether we will be applying previews or not? It does > seem to correlate with when I see a previewed page, but I thought I should > double check. > If that's true, there might be something preventing previews from being used at all on background offliner. When I do normal navigations, I usually see PREVIEWS_OFF (at least with the limited cases I tried) when not showign a preview. > https://codereview.chromium.org/2916253002/diff/40001/chrome/browser/offline_... > File chrome/browser/offline_pages/background_loader_offliner.cc (right): > > https://codereview.chromium.org/2916253002/diff/40001/chrome/browser/offline_... > chrome/browser/offline_pages/background_loader_offliner.cc:81: if > (previews_state != content::PreviewsTypes::PREVIEWS_OFF) > On 2017/06/05 20:43:59, RyanSturm wrote: > > There is some subtlety with PREVIEWS_OFF vs PREVIEWS_NO_TRANSFORM. > PREVIEWS_OFF > > can actually have a server previews (like LoFi or LitePages), but this can > only > > happen when a user is opted into lite page previews via flags. > > > > It's an area we are still sorting out. However, > > ChromeNavigationData->data_reduction_proxy_data()->lite_page_received() will > > tell if it is a lite page. I don't think there is a way to currently get LoFi > > images without the bits set fortunately. > > > > How about the condition checks > > (ChromeNavigationData->data_reduction_proxy_data()->lite_page_received() || > > !(previews_state == content::PreviewsTypes::PREVIEWS_OFF || previews_state == > > content::PreviewsTypes::PREVIEWS_OFF)) > > > > Sorry about the confusion as this model is changing slightly throughout M61. > > Perhaps dougarnett@ could take a look to make sure this set of checks will be > > sufficient. > > Added checks as you suggest (but worded slightly differently)
On 2017/06/05 21:54:27, RyanSturm wrote: > On 2017/06/05 21:40:47, Pete Williamson wrote: > > RyanSturm: When I look at this with logging at runtime, I typically see > > PREVIEWS_UNSPECIFIED. Does that mean that I'm checking too early in the > process, > > before we know for sure whether we will be applying previews or not? It does > > seem to correlate with when I see a previewed page, but I thought I should > > double check. > > > > If that's true, there might be something preventing previews from being used at > all on background offliner. When I do normal navigations, I usually see > PREVIEWS_OFF (at least with the limited cases I tried) when not showign a > preview. > > > > https://codereview.chromium.org/2916253002/diff/40001/chrome/browser/offline_... > > File chrome/browser/offline_pages/background_loader_offliner.cc (right): > > > > > https://codereview.chromium.org/2916253002/diff/40001/chrome/browser/offline_... > > chrome/browser/offline_pages/background_loader_offliner.cc:81: if > > (previews_state != content::PreviewsTypes::PREVIEWS_OFF) > > On 2017/06/05 20:43:59, RyanSturm wrote: > > > There is some subtlety with PREVIEWS_OFF vs PREVIEWS_NO_TRANSFORM. > > PREVIEWS_OFF > > > can actually have a server previews (like LoFi or LitePages), but this can > > only > > > happen when a user is opted into lite page previews via flags. > > > > > > It's an area we are still sorting out. However, > > > ChromeNavigationData->data_reduction_proxy_data()->lite_page_received() will > > > tell if it is a lite page. I don't think there is a way to currently get > LoFi > > > images without the bits set fortunately. > > > > > > How about the condition checks > > > (ChromeNavigationData->data_reduction_proxy_data()->lite_page_received() || > > > !(previews_state == content::PreviewsTypes::PREVIEWS_OFF || previews_state > == > > > content::PreviewsTypes::PREVIEWS_OFF)) > > > > > > Sorry about the confusion as this model is changing slightly throughout M61. > > > Perhaps dougarnett@ could take a look to make sure this set of checks will > be > > > sufficient. > > > > Added checks as you suggest (but worded slightly differently) Right, I also see PREVIEWS_OFF when not showing a preview, but I see PREVIEWS_UNSPECIFIED when showing a preview. This is a bit odd since I expected to see something like "CLIENT_LOFI_ON | CLIENT_LOFI_AUTO_RELOAD". Again it seems to correlate well with when we are showing a preview, but it still seems a bit strange to me.
On 2017/06/05 21:59:16, Pete Williamson wrote: > On 2017/06/05 21:54:27, RyanSturm wrote: > > On 2017/06/05 21:40:47, Pete Williamson wrote: > > > RyanSturm: When I look at this with logging at runtime, I typically see > > > PREVIEWS_UNSPECIFIED. Does that mean that I'm checking too early in the > > process, > > > before we know for sure whether we will be applying previews or not? It > does > > > seem to correlate with when I see a previewed page, but I thought I should > > > double check. > > > > > > > If that's true, there might be something preventing previews from being used > at > > all on background offliner. When I do normal navigations, I usually see > > PREVIEWS_OFF (at least with the limited cases I tried) when not showign a > > preview. > > > > > > > > https://codereview.chromium.org/2916253002/diff/40001/chrome/browser/offline_... > > > File chrome/browser/offline_pages/background_loader_offliner.cc (right): > > > > > > > > > https://codereview.chromium.org/2916253002/diff/40001/chrome/browser/offline_... > > > chrome/browser/offline_pages/background_loader_offliner.cc:81: if > > > (previews_state != content::PreviewsTypes::PREVIEWS_OFF) > > > On 2017/06/05 20:43:59, RyanSturm wrote: > > > > There is some subtlety with PREVIEWS_OFF vs PREVIEWS_NO_TRANSFORM. > > > PREVIEWS_OFF > > > > can actually have a server previews (like LoFi or LitePages), but this can > > > only > > > > happen when a user is opted into lite page previews via flags. > > > > > > > > It's an area we are still sorting out. However, > > > > ChromeNavigationData->data_reduction_proxy_data()->lite_page_received() > will > > > > tell if it is a lite page. I don't think there is a way to currently get > > LoFi > > > > images without the bits set fortunately. > > > > > > > > How about the condition checks > > > > (ChromeNavigationData->data_reduction_proxy_data()->lite_page_received() > || > > > > !(previews_state == content::PreviewsTypes::PREVIEWS_OFF || previews_state > > == > > > > content::PreviewsTypes::PREVIEWS_OFF)) > > > > > > > > Sorry about the confusion as this model is changing slightly throughout > M61. > > > > Perhaps dougarnett@ could take a look to make sure this set of checks will > > be > > > > sufficient. > > > > > > Added checks as you suggest (but worded slightly differently) > > Right, I also see PREVIEWS_OFF when not showing a preview, but I see > PREVIEWS_UNSPECIFIED when showing a preview. This is a bit odd since I expected > to see something like "CLIENT_LOFI_ON | CLIENT_LOFI_AUTO_RELOAD". Again it > seems to correlate well with when we are showing a preview, but it still seems a > bit strange to me. Strange. I saw LITE_PAGE (again not using background offliner). Chrome:// pages use PREVIEWS_UNSPECIFIED I believe as well. Is there a flag for launching the offliner?
Ah, the problem was that we were not properly copying the ChromeNavigationData, it failed to copy the previews_state whenever we were on a https link and the data_reduction_proxy_data was not set. To answer RyanSturm's question, the flag to turn on the background loader is "Use backgrouhnd loader instead of prerenderer to load pages"
lgtm % some nit comments. https://codereview.chromium.org/2916253002/diff/80001/chrome/browser/offline_... File chrome/browser/offline_pages/background_loader_offliner.cc (right): https://codereview.chromium.org/2916253002/diff/80001/chrome/browser/offline_... chrome/browser/offline_pages/background_loader_offliner.cc:81: content::PreviewsState previews_state) { nit: it's a little weird that you are pulling previews_state out of NavigationData outside this method but pulling lite_page_recieved out within this method. https://codereview.chromium.org/2916253002/diff/80001/chrome/browser/offline_... chrome/browser/offline_pages/background_loader_offliner.cc:96: UMA_HISTOGRAM_ENUMERATION( Any reason this isn't UMA_HISTOGRAM_BOOLEAN?
Fixes per RyanSturm https://codereview.chromium.org/2916253002/diff/80001/chrome/browser/offline_... File chrome/browser/offline_pages/background_loader_offliner.cc (right): https://codereview.chromium.org/2916253002/diff/80001/chrome/browser/offline_... chrome/browser/offline_pages/background_loader_offliner.cc:81: content::PreviewsState previews_state) { On 2017/06/06 20:49:52, RyanSturm wrote: > nit: it's a little weird that you are pulling previews_state out of > NavigationData outside this method but pulling lite_page_recieved out within > this method. Good point, moved the check for previews_state inside this function. https://codereview.chromium.org/2916253002/diff/80001/chrome/browser/offline_... chrome/browser/offline_pages/background_loader_offliner.cc:96: UMA_HISTOGRAM_ENUMERATION( On 2017/06/06 20:49:52, RyanSturm wrote: > Any reason this isn't UMA_HISTOGRAM_BOOLEAN? Done.
lgtm
petewil@chromium.org changed reviewers: + csharrison@chromium.org, holte@chromium.org
holte: Please look at Histograms CSHarrison: please look at chrome/browser/loader We changed chrome/browser/loader to allow the offline pages code to be able to see the previews state, which we need to see to collect UMA for when both offline pages and previews are enabled at the same time. More information about why we are doing this is available here: https://docs.google.com/document/d/1JLlGNL-zkGzVZqbHP40DbphDWMLGBLHB7IirPeWWMZw
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...
LGTM
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
petewil@chromium.org changed reviewers: + nasko@chromium.org
+Nasko - Please review previews_state.h (only). I discovered that the include header was misformatted, I corrected it (this is independent from the rest of the changes in the patch, just fixed it while I was working on related code).
lgtm
https://codereview.chromium.org/2916253002/diff/100001/chrome/browser/offline... File chrome/browser/offline_pages/background_loader_offliner.cc (right): https://codereview.chromium.org/2916253002/diff/100001/chrome/browser/offline... chrome/browser/offline_pages/background_loader_offliner.cc:99: UMA_HISTOGRAM_BOOLEAN( Actually, you probably need to change this to not use the UMA_HISTOGRAM_BOOLEAN macro, since it uses a static pointer will break with dynamic named histograms. Use the UmaHistogramBoolean function instead.
CR fixes for Holte https://codereview.chromium.org/2916253002/diff/100001/chrome/browser/offline... File chrome/browser/offline_pages/background_loader_offliner.cc (right): https://codereview.chromium.org/2916253002/diff/100001/chrome/browser/offline... chrome/browser/offline_pages/background_loader_offliner.cc:99: UMA_HISTOGRAM_BOOLEAN( On 2017/06/07 22:24:46, Steven Holte wrote: > Actually, you probably need to change this to not use the UMA_HISTOGRAM_BOOLEAN > macro, since it uses a static pointer will break with dynamic named histograms. > > Use the UmaHistogramBoolean function instead. Done. Thanks for catching this!
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...
lgtm
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
content/public/common/previews_state.h LGTM
The CQ bit was checked by petewil@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from ryansturm@chromium.org, chili@chromium.org, csharrison@chromium.org Link to the patchset: https://codereview.chromium.org/2916253002/#ps120001 (title: "Histograms fix")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 120001, "attempt_start_ts": 1497050826952600,
"parent_rev": "999a40e458bbcfbfad9bb4498c0e2799261a9ee3", "commit_rev":
"61b63cbc244f348331c91d78402eb843bd185408"}
Message was sent while issue was closed.
Description was changed from ========== Add UMA to determine how often we offline a page with previews. We suspect that page previews may be causing problems with offline pages. To understand the severity of the problem, we'll add a new histogram that measures how often we turn on previews while offlining a page. BUG=703875 ========== to ========== Add UMA to determine how often we offline a page with previews. We suspect that page previews may be causing problems with offline pages. To understand the severity of the problem, we'll add a new histogram that measures how often we turn on previews while offlining a page. BUG=703875 Review-Url: https://codereview.chromium.org/2916253002 Cr-Commit-Position: refs/heads/master@{#478483} Committed: https://chromium.googlesource.com/chromium/src/+/61b63cbc244f348331c91d78402e... ==========
Message was sent while issue was closed.
Committed patchset #7 (id:120001) as https://chromium.googlesource.com/chromium/src/+/61b63cbc244f348331c91d78402e... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
