Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2412753002:
Add UMA for documents calling window.print(). (Closed)
|
Created:
4 years, 2 months ago by nasko Modified:
4 years, 2 months ago CC:
chromium-reviews, mlamouri+watch-content_chromium.org, creis+watch_chromium.org, nasko+codewatch_chromium.org, jam, darin-cc_chromium.org, asvitkine+watch_chromium.org, site-isolation-reviews_chormium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdd UMA for documents calling window.print().
BUG=631513
Committed: https://crrev.com/e440d8cb3d9ff035fa588fd0b13b73f3deaa9568
Cr-Commit-Position: refs/heads/master@{#424898}
Patch Set 1 #Patch Set 2 : Add UMA for out-of-process iframes printing. #
Total comments: 8
Patch Set 3 : Move histograms to PrintPreview category. #Patch Set 4 : Improve description. #Messages
Total messages: 28 (14 generated)
nasko@chromium.org changed reviewers: + isherman@chromium.org, thestig@chromium.org
Hey Lei and Ilya, Can you review this CL for me? It is a simple UMA for documents calling window.print() and indicating whether it is the main frame vs the subframe. Thanks in advance! Nasko
The CQ bit was checked by nasko@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 checked by nasko@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...
I've added an additional histogram to count the cases that we know are broken with out-of-process iframes. Please take another look.
https://codereview.chromium.org/2412753002/diff/20001/content/renderer/render... File content/renderer/render_view_impl.cc (right): https://codereview.chromium.org/2412753002/diff/20001/content/renderer/render... content/renderer/render_view_impl.cc:1641: frame->top()->isWebRemoteFrame()); frame->top() won't ever be a nullptr, right?
The CQ bit was checked by nasko@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2412753002/diff/20001/content/renderer/render... File content/renderer/render_view_impl.cc (right): https://codereview.chromium.org/2412753002/diff/20001/content/renderer/render... content/renderer/render_view_impl.cc:1641: frame->top()->isWebRemoteFrame()); On 2016/10/11 23:42:59, Lei Zhang wrote: > frame->top() won't ever be a nullptr, right? Yes, it should never be null, but can be either local or remote. We care about the cases that the top level frame is a remote one.
https://codereview.chromium.org/2412753002/diff/20001/content/renderer/render... File content/renderer/render_view_impl.cc (right): https://codereview.chromium.org/2412753002/diff/20001/content/renderer/render... content/renderer/render_view_impl.cc:1641: frame->top()->isWebRemoteFrame()); On 2016/10/11 23:44:17, nasko (slow) wrote: > On 2016/10/11 23:42:59, Lei Zhang wrote: > > frame->top() won't ever be a nullptr, right? > > Yes, it should never be null, but can be either local or remote. We care about > the cases that the top level frame is a remote one. Acknowledged.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
nasko@chromium.org changed reviewers: + asvitkine@chromium.org - isherman@chromium.org
Switching to asvitkine@ for OWNERS on historgrams.xml.
lgtm % comment https://codereview.chromium.org/2412753002/diff/20001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2412753002/diff/20001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:47258: +<histogram name="Printing.OutOfProcessSubframe" enum="Boolean"> Nit: Suggest adding a specific enum for this too.
isherman@chromium.org changed reviewers: + isherman@chromium.org
As mentioned on the earlier chat ping, not lgtm. (Not sure why you switched reviewers following that ping.) https://codereview.chromium.org/2412753002/diff/20001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2412753002/diff/20001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:47258: +<histogram name="Printing.OutOfProcessSubframe" enum="Boolean"> As I mentioned on chat, it's probably more appropriate to place these histograms under an existing top-level category, like "Blink.Printing.*" or "Renderer.Printing.*" ... or if there's a top-level group name that's typically used for the OOPIF work, that would be even better. https://codereview.chromium.org/2412753002/diff/20001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:47263: + the top level frame. Is this metric recorded only for subframes, or for all frames? Please clarify in the description.
Updated description and moved to PrintPreview category. https://codereview.chromium.org/2412753002/diff/20001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2412753002/diff/20001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:47258: +<histogram name="Printing.OutOfProcessSubframe" enum="Boolean"> On 2016/10/12 21:45:54, Ilya Sherman wrote: > As I mentioned on chat, it's probably more appropriate to place these histograms > under an existing top-level category, like "Blink.Printing.*" or > "Renderer.Printing.*" ... or if there's a top-level group name that's typically > used for the OOPIF work, that would be even better. I moved it to PrintPreview, as I think each invocation of window.print() will result in a print preview window to appear. Creating a new category might not be needed as I'm not sure if we will need this metric long term. https://codereview.chromium.org/2412753002/diff/20001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:47263: + the top level frame. On 2016/10/12 21:45:54, Ilya Sherman wrote: > Is this metric recorded only for subframes, or for all frames? Please clarify > in the description. Tried to reword a bit. Let me know if it is better or how I can clarify it further.
LGTM, thanks.
The CQ bit was checked by nasko@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from thestig@chromium.org, asvitkine@chromium.org Link to the patchset: https://codereview.chromium.org/2412753002/#ps60001 (title: "Improve description.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Description was changed from ========== Add UMA for documents calling window.print(). BUG=631513 ========== to ========== Add UMA for documents calling window.print(). BUG=631513 Committed: https://crrev.com/e440d8cb3d9ff035fa588fd0b13b73f3deaa9568 Cr-Commit-Position: refs/heads/master@{#424898} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/e440d8cb3d9ff035fa588fd0b13b73f3deaa9568 Cr-Commit-Position: refs/heads/master@{#424898} |
