Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2212183004:
Log to UMA when x-origin frames (1) are created and (2) become visible (Closed)
|
Created:
4 years, 4 months ago by dgrogan Modified:
4 years, 3 months ago CC:
asvitkine+watch_chromium.org, blink-reviews, chromium-reviews, mlamouri+watch-blink_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionLog UMA when x-origin documents (1) are created and (2) become visible
This will give us an upper bound on the % of iframes for which we can
defer loading.
BUG=635105
Committed: https://crrev.com/2e418c5d9e4ba04885654f1b53ea80f60b1316fd
Cr-Commit-Position: refs/heads/master@{#417064}
Patch Set 1 #
Total comments: 12
Patch Set 2 : rebase #Patch Set 3 : log creation after installNewDocument #
Total comments: 4
Patch Set 4 : use frameloader->stateMachine. kill racy DCHECK. rename histogram #
Total comments: 2
Patch Set 5 : move to Document #
Total comments: 7
Patch Set 6 : change some null checks to DCHECK #Patch Set 7 : DCHECK frame().document() #Patch Set 8 : ToT #Patch Set 9 : rename histogram: s/Frame/Document/ #
Total comments: 7
Patch Set 10 : put histogram under Navigation category #Patch Set 11 : fix histogram description #
Messages
Total messages: 50 (17 generated)
dgrogan@chromium.org changed reviewers: + ojan@chromium.org, skyostil@chromium.org
Sami, do you know of a better place to log when a cross-origin frame is created? And could you look at the TODO questions in FrameView.cpp? Ojan, I've started work on detecting TopOrLeft but want to get this in first. I don't think the question about which cross-origin check is dire, I assume they'll be approximately equal. But I'm curious why throttling went with the approach it did. Sami, could you shed some light on that?
Would this work as an intersection observer? I'm planning to make FrameView throttling be drive by an intersection observer soon so a lot of the visibility computation here will go away. https://codereview.chromium.org/2212183004/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/frame/FrameView.cpp (right): https://codereview.chromium.org/2212183004/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/frame/FrameView.cpp:4185: // TODO(dgrogan): Document why we need this check. For cycles in the frame tree? This is so that we don't traverse the frame tree unnecessarily. https://codereview.chromium.org/2212183004/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/frame/FrameView.cpp:4201: // TODO(dgrogan): Change this to DCHECK(m_viewportIntersectionValid)? Right, we should be able to require that.
It will work probably work fine as an intersection observer. Ojan pointed out a small difference that we have to watch out for though: intersection observer has a delay of up to 100ms before it notifies its observers, so the chance of users seeing stale content increases. I was also planning on switching this to an intersection observer sometime in the next few weeks but want to get this rough data first. https://codereview.chromium.org/2212183004/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/frame/FrameView.cpp (right): https://codereview.chromium.org/2212183004/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/frame/FrameView.cpp:4185: // TODO(dgrogan): Document why we need this check. For cycles in the frame tree? On 2016/08/08 11:09:47, Sami wrote: > This is so that we don't traverse the frame tree unnecessarily. It what cases will it be triggered though?
Description was changed from ========== Log to UMA when x-origin frames (1) are created and (2) become visible BUG=635105 ========== to ========== Log to UMA when x-origin frames (1) are created and (2) become visible This will give us an upper bound on the % of iframes for which we can defer loading. BUG=635105 ==========
https://codereview.chromium.org/2212183004/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/frame/FrameView.cpp (right): https://codereview.chromium.org/2212183004/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/frame/FrameView.cpp:4185: // TODO(dgrogan): Document why we need this check. For cycles in the frame tree? On 2016/08/08 at 17:45:48, dgrogan (OOO Aug 9 - 17) wrote: > On 2016/08/08 11:09:47, Sami wrote: > > This is so that we don't traverse the frame tree unnecessarily. > > It what cases will it be triggered though? https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/frame/Fra... It's setting dirty bits on the ancestor chain so it knows to walk down the subtree to a specific frame. THis way we only traverse the subtrees of the frame tree that have a dirty frame. In either case, I'd leave out this TODO. I don't think committing a TODO to leave a comment is very worthwhile. Feel free to add a comment by the member variable definitions in the header, but I wouldn't bother adding a TODO. https://codereview.chromium.org/2212183004/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/frame/FrameView.cpp:4201: // TODO(dgrogan): Change this to DCHECK(m_viewportIntersectionValid)? On 2016/08/08 at 11:09:47, Sami wrote: > Right, we should be able to require that. Same as above, either add the DCHECK or leave the whole thing out. Adding comments like this clutters the code. https://codereview.chromium.org/2212183004/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/frame/LocalFrame.cpp (right): https://codereview.chromium.org/2212183004/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/frame/LocalFrame.cpp:509: // more than once. Nit: I think this would get called every time a frame is navigated? Or are you seeing it multiple times with a single frame's page load? s/seems to happen/happens/ More importantly, I think this comment is not in the most useful place. The DCHECK you added to isCrossOrigin seems to cover this? https://codereview.chromium.org/2212183004/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/frame/LocalFrame.cpp:883: if (!isCrossOrigin()) I think this is probably the right one. I think the comment is good for now, but should probably be phrased in terms of a TODO to figure out which one to use long-term. https://codereview.chromium.org/2212183004/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/frame/LocalFrame.cpp:887: // Can't do it in the ctor because isCrossOrigin relies on document(), which isn't set yet. How about we log this in setDOMWindow? That actually seems better in retrospect because it will reset everytime a frame is navigated (which is the behavior we want I think). Also in setDOMWindow, you can reset m_visibilityWasLogged = false so that we get a fresh count for each time a frame is navigated.
https://codereview.chromium.org/2212183004/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/frame/FrameView.cpp (right): https://codereview.chromium.org/2212183004/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/frame/FrameView.cpp:4201: // TODO(dgrogan): Change this to DCHECK(m_viewportIntersectionValid)? On 2016/08/08 22:01:54, ojan wrote: > On 2016/08/08 at 11:09:47, Sami wrote: > > Right, we should be able to require that. > > Same as above, either add the DCHECK or leave the whole thing out. Adding > comments like this clutters the code. Sorry, it wasn't clear at all, but these comments weren't meant to survive through to the final patchset. They're more placeholders for us to discuss stuff. I spun this one out to its own tiny patch already. https://codereview.chromium.org/2212183004/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/frame/LocalFrame.cpp (right): https://codereview.chromium.org/2212183004/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/frame/LocalFrame.cpp:509: // more than once. On 2016/08/08 22:01:54, ojan wrote: > Nit: > I think this would get called every time a frame is navigated? Or are you seeing > it multiple times with a single frame's page load? In my testing it happens exactly twice per frame load, but I didn't dig in to find out why. About the comment, agreed it has no place in the repository. https://codereview.chromium.org/2212183004/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/frame/LocalFrame.cpp:887: // Can't do it in the ctor because isCrossOrigin relies on document(), which isn't set yet. On 2016/08/08 22:01:54, ojan wrote: > How about we log this in setDOMWindow? We have the same isCrossOrigin problem there: the domWindow object has a null m_document, so isCrossOrigin fails the new DCHECK. > That actually seems better in retrospect > because it will reset everytime a frame is navigated (which is the behavior we > want I think). > > Also in setDOMWindow, you can reset m_visibilityWasLogged = false so that we get > a fresh count for each time a frame is navigated. Even if we could log from setDOMWindow, I'm not sure we'd want to reset m_visibilityWasLogged. Resetting could lead to more visible pages than created pages... unless I'm misunderstanding what you mean.
dcheng@chromium.org changed reviewers: + dcheng@chromium.org
https://codereview.chromium.org/2212183004/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/frame/LocalFrame.cpp (right): https://codereview.chromium.org/2212183004/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/frame/LocalFrame.cpp:887: // Can't do it in the ctor because isCrossOrigin relies on document(), which isn't set yet. On 2016/08/09 00:30:16, dgrogan (OOO Aug 9 - 17) wrote: > On 2016/08/08 22:01:54, ojan wrote: > > How about we log this in setDOMWindow? > > We have the same isCrossOrigin problem there: the domWindow object has a null > m_document, so isCrossOrigin fails the new DCHECK. > > > That actually seems better in retrospect > > because it will reset everytime a frame is navigated (which is the behavior we > > want I think). > > > > Also in setDOMWindow, you can reset m_visibilityWasLogged = false so that we > get > > a fresh count for each time a frame is navigated. > > Even if we could log from setDOMWindow, I'm not sure we'd want to reset > m_visibilityWasLogged. Resetting could lead to more visible pages than created > pages... unless I'm misunderstanding what you mean. If this needs to depend on isCrossOrigin(), it should probably be done in installNewDocument() or later: until that's complete, there's no meaningful security origin.
On 2016/08/08 17:45:48, dgrogan (OOO Aug 9 - 17) wrote: > It will work probably work fine as an intersection observer. Ojan pointed out a > small difference that we have to watch out for though: intersection observer has > a delay of up to 100ms before it notifies its observers, so the chance of users > seeing stale content increases. That 100ms is also a problem for FrameView throttling, so I was thinking of adding some kind of low latency flag to the observer so it will get notified ASAP.
dcheng, could you take a look? https://codereview.chromium.org/2212183004/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/frame/LocalFrame.cpp (right): https://codereview.chromium.org/2212183004/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/frame/LocalFrame.cpp:860: if (!m_creationWasLogged && isCrossOriginSubframe()) { installNewDocument is called twice when an iframe is first created (at least when it is created with a srcdoc) so I had to keep LocalFrame::m_creationWasLogged, but the current solution is still better than having to check m_creationWasLogged every time the visibility might have changed.
https://codereview.chromium.org/2212183004/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/frame/LocalFrame.cpp (right): https://codereview.chromium.org/2212183004/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/frame/LocalFrame.cpp:860: if (!m_creationWasLogged && isCrossOriginSubframe()) { On 2016/08/19 23:56:23, dgrogan wrote: > installNewDocument is called twice when an iframe is first created (at least > when it is created with a srcdoc) so I had to keep > LocalFrame::m_creationWasLogged, but the current solution is still better than > having to check m_creationWasLogged every time the visibility might have > changed. When a frame is first created, it always starts on the initial empty document (https://html.spec.whatwg.org/multipage/browsers.html#initialise-the-document-... kind of references this). So what happens is the initial empty document is created and installed first, then the Document for the srcdoc is created and installed. Can we use FrameLoaderStateMachine::creatingInitialEmptyDocument() or isDisplayingInitialEmptyDocument() to distinguish between these cases? I'm guessing we don't want to record the UMA for the initial empty document.
Patchset #4 (id:60001) has been deleted
Patchset #4 (id:80001) has been deleted
Thanks for your help. I now need to figure out if I want to remove m_creationWasLogged. Removing it would mean UMA is counting the number of documents that we would have skipped loading, which seems marginally better than number of frames. I'll ponder it more tomorrow but thoughts are welcome. https://codereview.chromium.org/2212183004/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/frame/LocalFrame.cpp (right): https://codereview.chromium.org/2212183004/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/frame/LocalFrame.cpp:860: if (!m_creationWasLogged && isCrossOriginSubframe()) { > Can we use FrameLoaderStateMachine::creatingInitialEmptyDocument() or > isDisplayingInitialEmptyDocument() to distinguish between these cases? I'm > guessing we don't want to record the UMA for the initial empty document. You're right. This function is now only called when committedFirstRealDocumentLoad() is true. https://codereview.chromium.org/2212183004/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/frame/LocalFrame.cpp:869: DCHECK(m_creationWasLogged); This was racy.
Patchset #4 (id:100001) has been deleted
Ready for another look, ojan and/or dcheng. I decided to keep one counter per frame, not per document, to keep explanations simple. They should be roughly equal anyway.
https://codereview.chromium.org/2212183004/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/loader/DocumentLoader.cpp (right): https://codereview.chromium.org/2212183004/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/loader/DocumentLoader.cpp:699: frame->didInstallNewRealDocument(); I think it would be nicer if we could do this without adding a new callback. There are several approaches: the easiest is to probably just move the creation logging trigger into DOcument's constructor, after we call initSecurityContext. However, this raises an interesting question: what do we want to time? If we time from the creation of the cross-origin document to when it becomes visible, this is not actually the same as the timing of the creation of the initial frame to when the cross-origin document loads. The problem, of course, is that we don't /know/ that a LocalFrame is going to be used for a cross-origin document until the actual navigation commits.
https://codereview.chromium.org/2212183004/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/loader/DocumentLoader.cpp (right): https://codereview.chromium.org/2212183004/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/loader/DocumentLoader.cpp:699: frame->didInstallNewRealDocument(); On 2016/08/26 18:28:29, dcheng wrote: > I think it would be nicer if we could do this without adding a new callback. This allowed the enum and logging code to live solely in LocalFrame.cpp, and not be exposed to anything else, which is a nice property. > There are several approaches: the easiest is to probably just move the creation > logging trigger into DOcument's constructor, after we call initSecurityContext. > > However, this raises an interesting question: what do we want to time? If we > time from the creation of the cross-origin document to when it becomes visible, > this is not actually the same as the timing of the creation of the initial frame > to when the cross-origin document loads. I might be nitpicking here, but we're not trying to time anything, just count. Maybe you are using those terms interchangeably? But to get at the crux of your question, I think it doesn't matter exactly what we count (yet). For now we're trying to get a first rough estimate of the effect of a potential future intervention. As long as we're not off by a factor of 2-3, we're fine. Instead of being frame-centric, as this CL has been so far, we could be document-centric and measure (x-origin documents that have been loaded in a visible frame) / (x-origin documents that have been created). Do you think that would lead to something cleaner? > The problem, of course, is that we don't /know/ that a LocalFrame is going to be > used for a cross-origin document until the actual navigation commits.
On 2016/08/26 18:54:06, dgrogan wrote: > https://codereview.chromium.org/2212183004/diff/120001/third_party/WebKit/Sou... > File third_party/WebKit/Source/core/loader/DocumentLoader.cpp (right): > > https://codereview.chromium.org/2212183004/diff/120001/third_party/WebKit/Sou... > third_party/WebKit/Source/core/loader/DocumentLoader.cpp:699: > frame->didInstallNewRealDocument(); > On 2016/08/26 18:28:29, dcheng wrote: > > I think it would be nicer if we could do this without adding a new callback. > > This allowed the enum and logging code to live solely in LocalFrame.cpp, and not > be exposed to anything else, which is a nice property. > > > There are several approaches: the easiest is to probably just move the > creation > > logging trigger into DOcument's constructor, after we call > initSecurityContext. > > > > However, this raises an interesting question: what do we want to time? If we > > time from the creation of the cross-origin document to when it becomes > visible, > > this is not actually the same as the timing of the creation of the initial > frame > > to when the cross-origin document loads. > > I might be nitpicking here, but we're not trying to time anything, just count. > Maybe you are using those terms interchangeably? Hmm. I guess I got confused! > > But to get at the crux of your question, I think it doesn't matter exactly what > we count (yet). For now we're trying to get a first rough estimate of the effect > of a potential future intervention. As long as we're not off by a factor of 2-3, > we're fine. > > Instead of being frame-centric, as this CL has been so far, we could be > document-centric and measure (x-origin documents that have been loaded in a > visible frame) / (x-origin documents that have been created). Do you think that > would lead to something cleaner? Document is probably better, since being cross origin is really a property of Document, not Frame. > > > The problem, of course, is that we don't /know/ that a LocalFrame is going to > be > > used for a cross-origin document until the actual navigation commits.
Patchset #5 (id:140001) has been deleted
Patchset #5 (id:160001) has been deleted
We now count Documents instead of LocalFrames. Ready for another look.
https://codereview.chromium.org/2212183004/diff/180001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/dom/Document.cpp (right): https://codereview.chromium.org/2212183004/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/core/dom/Document.cpp:503: if (frame() && getSecurityOrigin() && frame()->tree().top()->securityContext() && !getSecurityOrigin()->canAccess(frame()->tree().top()->securityContext()->getSecurityOrigin())) I don't think securityOrigin can be null here, unless something is really broken. https://codereview.chromium.org/2212183004/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/core/dom/Document.cpp:6031: if (visible && !m_visibilityWasLogged && frame() && frame()->isCrossOriginSubframe()) { Since frame()->document() points to this document(), frame() should not be null. Let's just DCHECK()? https://codereview.chromium.org/2212183004/diff/180001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/frame/FrameView.cpp (right): https://codereview.chromium.org/2212183004/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/core/frame/FrameView.cpp:4263: if (frame().document()) Do we need to do this check because Widget isn't synchronously detached (see UpdateSuspendScope)?
https://codereview.chromium.org/2212183004/diff/180001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/dom/Document.cpp (right): https://codereview.chromium.org/2212183004/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/core/dom/Document.cpp:503: if (frame() && getSecurityOrigin() && frame()->tree().top()->securityContext() && !getSecurityOrigin()->canAccess(frame()->tree().top()->securityContext()->getSecurityOrigin())) On 2016/08/29 20:37:19, dcheng wrote: > I don't think securityOrigin can be null here, unless something is really > broken. Good to know, changed to DCHECK. https://codereview.chromium.org/2212183004/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/core/dom/Document.cpp:6031: if (visible && !m_visibilityWasLogged && frame() && frame()->isCrossOriginSubframe()) { On 2016/08/29 20:37:19, dcheng wrote: > Since frame()->document() points to this document(), frame() should not be null. > Let's just DCHECK()? Done. https://codereview.chromium.org/2212183004/diff/180001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/frame/FrameView.cpp (right): https://codereview.chromium.org/2212183004/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/core/frame/FrameView.cpp:4263: if (frame().document()) On 2016/08/29 20:37:19, dcheng wrote: > Do we need to do this check because Widget isn't synchronously detached (see > UpdateSuspendScope)? Not sure... I was cargo-culting from the first clause of line 4258's DCHECK that implies frame().document() can be null.
https://codereview.chromium.org/2212183004/diff/180001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/frame/FrameView.cpp (right): https://codereview.chromium.org/2212183004/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/core/frame/FrameView.cpp:4263: if (frame().document()) On 2016/08/31 19:52:41, dgrogan wrote: > On 2016/08/29 20:37:19, dcheng wrote: > > Do we need to do this check because Widget isn't synchronously detached (see > > UpdateSuspendScope)? > > Not sure... I was cargo-culting from the first clause of line 4258's DCHECK that > implies frame().document() can be null. Looking at the code, I'm actually not sure how we'd end up in this situation. I believe this task is only ever posted when we are attached (e.g. frame() is not null) and the task itself is cancelled when the FrameView is disposed. Can you try just asserting that frame().document() is non-null and see what happens?
The CQ bit was checked by dgrogan@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...
Description was changed from ========== Log to UMA when x-origin frames (1) are created and (2) become visible This will give us an upper bound on the % of iframes for which we can defer loading. BUG=635105 ========== to ========== Log UMA when x-origin documents (1) are created and (2) become visible This will give us an upper bound on the % of iframes for which we can defer loading. BUG=635105 ==========
> Looking at the code, I'm actually not sure how we'd end up in this situation. I > believe this task is only ever posted when we are attached (e.g. frame() is not > null) and the task itself is cancelled when the FrameView is disposed. > > Can you try just asserting that frame().document() is non-null and see what > happens? The trybots report that such a DCHECK is ok. I'm going to rename the histogram to reflect that we're now recording documents instead of frames.
Ok, ready for another look.
The CQ bit was checked by ojan@chromium.org
lgtm
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
dgrogan@chromium.org changed reviewers: + isherman@chromium.org
isherman, could you review the new histogram?
https://codereview.chromium.org/2212183004/diff/260001/tools/metrics/histogra... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2212183004/diff/260001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:8726: +<histogram name="DeferredDocumentLoading.StatesV1" It looks like you're adding a new top-level category, "DeferredDocumentLoading". We generally discourage now top-level categories, because there's only one global namespace, and most of the key ideas are already covered in it. Hence, I'd encourage you to explore to see whether there might already exist a top-level category appropriate for this histogram. If there isn't, then I'd encourage you to explore possible generalizations of this name, e.g. to capture user agent interventions / mitigations more broadly. I have definitely seen other histograms landed for this effort, though I can't at the moment recall which ones they were... https://codereview.chromium.org/2212183004/diff/260001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:74647: + <int value="1" label="WouldLoadBecauseVisible"/> Should there also be an entry related to cross-origin communication, and possibly others?
blink LGTM, thanks for doing the investigations on what can be null
https://codereview.chromium.org/2212183004/diff/260001/tools/metrics/histogra... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2212183004/diff/260001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:8726: +<histogram name="DeferredDocumentLoading.StatesV1" On 2016/09/06 21:29:17, Ilya Sherman wrote: > It looks like you're adding a new top-level category, "DeferredDocumentLoading". > We generally discourage now top-level categories, because there's only one > global namespace, and most of the key ideas are already covered in it. Hence, > I'd encourage you to explore to see whether there might already exist a > top-level category appropriate for this histogram. Sounds good, I put it under Navigation. The other candidate was PageLoad but that seems to be pretty self-contained and I don't want to intrude. > If there isn't, then I'd encourage you to explore possible generalizations of > this name, e.g. to capture user agent interventions / mitigations more broadly. > I have definitely seen other histograms landed for this effort, though I can't > at the moment recall which ones they were... Perhaps you were thinking of WebFont.InterventionResult* ? https://codereview.chromium.org/2212183004/diff/260001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:74647: + <int value="1" label="WouldLoadBecauseVisible"/> On 2016/09/06 21:29:17, Ilya Sherman wrote: > Should there also be an entry related to cross-origin communication, and > possibly others? Depends on the accepted best practices, with which I am not familiar. I assumed that I should only add entries here once the code actually logs them. Is that wrong? Anything I add here would be merely speculative at this point...
https://codereview.chromium.org/2212183004/diff/260001/tools/metrics/histogra... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2212183004/diff/260001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:8726: +<histogram name="DeferredDocumentLoading.StatesV1" On 2016/09/07 17:54:50, dgrogan wrote: > On 2016/09/06 21:29:17, Ilya Sherman wrote: > > It looks like you're adding a new top-level category, > "DeferredDocumentLoading". > > We generally discourage now top-level categories, because there's only one > > global namespace, and most of the key ideas are already covered in it. Hence, > > I'd encourage you to explore to see whether there might already exist a > > top-level category appropriate for this histogram. > > Sounds good, I put it under Navigation. The other candidate was PageLoad but > that seems to be pretty self-contained and I don't want to intrude. > > > If there isn't, then I'd encourage you to explore possible generalizations of > > this name, e.g. to capture user agent interventions / mitigations more > broadly. > > I have definitely seen other histograms landed for this effort, though I can't > > at the moment recall which ones they were... > > Perhaps you were thinking of WebFont.InterventionResult* ? Yes, probably =) https://codereview.chromium.org/2212183004/diff/260001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:74647: + <int value="1" label="WouldLoadBecauseVisible"/> On 2016/09/07 17:54:50, dgrogan wrote: > On 2016/09/06 21:29:17, Ilya Sherman wrote: > > Should there also be an entry related to cross-origin communication, and > > possibly others? > > Depends on the accepted best practices, with which I am not familiar. I assumed > that I should only add entries here once the code actually logs them. Is that > wrong? Anything I add here would be merely speculative at this point... Indeed, the best practice is to only list values that match the implementation. It's just surprising that the histogram description lists more things than are actually logged.
Oh, and: metrics lgtm
Thanks for the reviews, all https://codereview.chromium.org/2212183004/diff/260001/tools/metrics/histogra... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2212183004/diff/260001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:74647: + <int value="1" label="WouldLoadBecauseVisible"/> On 2016/09/07 18:00:38, Ilya Sherman wrote: > On 2016/09/07 17:54:50, dgrogan wrote: > > On 2016/09/06 21:29:17, Ilya Sherman wrote: > > > Should there also be an entry related to cross-origin communication, and > > > possibly others? > > > > Depends on the accepted best practices, with which I am not familiar. I > assumed > > that I should only add entries here once the code actually logs them. Is that > > wrong? Anything I add here would be merely speculative at this point... > > Indeed, the best practice is to only list values that match the implementation. > It's just surprising that the histogram description lists more things than are > actually logged. Ah, I wrote the description aspirationally but later cut the cross-origin communication part. Description now fixed.
The CQ bit was checked by dgrogan@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from dcheng@chromium.org, ojan@chromium.org, isherman@chromium.org Link to the patchset: https://codereview.chromium.org/2212183004/#ps300001 (title: "fix histogram 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 #11 (id:300001)
Message was sent while issue was closed.
Description was changed from ========== Log UMA when x-origin documents (1) are created and (2) become visible This will give us an upper bound on the % of iframes for which we can defer loading. BUG=635105 ========== to ========== Log UMA when x-origin documents (1) are created and (2) become visible This will give us an upper bound on the % of iframes for which we can defer loading. BUG=635105 Committed: https://crrev.com/2e418c5d9e4ba04885654f1b53ea80f60b1316fd Cr-Commit-Position: refs/heads/master@{#417064} ==========
Message was sent while issue was closed.
Patchset 11 (id:??) landed as https://crrev.com/2e418c5d9e4ba04885654f1b53ea80f60b1316fd Cr-Commit-Position: refs/heads/master@{#417064} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
