Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2647643004:
Report nav timing 2 instance as soon as it's requested. (Closed)
|
Created:
3 years, 11 months ago by sunjian Modified:
3 years, 9 months ago CC:
chromium-reviews, tyoshino+watch_chromium.org, Yoav Weiss, loading-reviews_chromium.org, gavinp+loader_chromium.org, blink-reviews, loading-reviews+fetch_chromium.org, Nate Chapin Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionReport nav timing 2 instance as soon as it's requested.
Currently NT2 instance is not available until loadEventEnd happens. However,
this new feature contradicts with the way current clients use NT1 which is available
as soon as it gets requested. In order to align with NT1's usage behavior, we
need to also make NT2 available as soon as clients request for it.
Doc: https://docs.google.com/a/chromium.org/document/d/1QX2zVcYpuFTsq0MEZH9J8ZUT4CCBezXKE--eX2ctOhw/edit?usp=sharing
BUG=687398
Review-Url: https://codereview.chromium.org/2647643004
Cr-Commit-Position: refs/heads/master@{#457871}
Committed: https://chromium.googlesource.com/chromium/src/+/36d0286404018e441692f66d1289f33ff33a05d1
Patch Set 1 : working version #Patch Set 2 : got rid of test cases that don't apply anymore #
Total comments: 5
Patch Set 3 : make a copy of navigationTimingInfo for ResourceFetcher #
Total comments: 16
Patch Set 4 : report name as initial URL #Patch Set 5 : make ResourceTimingInfo ref-counted #
Total comments: 40
Patch Set 6 : addressed comments #Patch Set 7 : sync #
Total comments: 5
Patch Set 8 : addressed comments #
Total comments: 6
Patch Set 9 : addressed comments #
Total comments: 2
Patch Set 10 : addressed comments #
Total comments: 4
Patch Set 11 : moved layout test to external/wpt directory #
Total comments: 2
Patch Set 12 : merged with crbug/2747933002 #Patch Set 13 : refactor getNavigationType(...) out #
Total comments: 1
Patch Set 14 : WeakMemeber<LocalFrame> to Member<LocalFrame> #Patch Set 15 : Memebr<LocalFrame>-->WeakMember<LocalFrame> #Patch Set 16 : add TODO #Messages
Total messages: 147 (91 generated)
Patchset #1 (id:1) has been deleted
Patchset #2 (id:40001) has been deleted
Patchset #2 (id:60001) has been deleted
Description was changed from ========== not for review not for review BUG= ========== to ========== Report nav timing 2 instance as soon as it's available. This patch is not ready to be reviewed yet. Just for prototyping and getting people on board. BUG= ==========
Patchset #3 (id:100001) has been deleted
Patchset #3 (id:120001) has been deleted
Patchset #1 (id:20001) has been deleted
Patchset #2 (id:140001) has been deleted
Patchset #2 (id:160001) has been deleted
Patchset #2 (id:180001) has been deleted
Description was changed from ========== Report nav timing 2 instance as soon as it's available. This patch is not ready to be reviewed yet. Just for prototyping and getting people on board. BUG= ========== to ========== Report nav timing 2 instance as soon as it's requested. This patch is not ready to be reviewed yet. Just for prototyping and getting people on board. BUG=687398 ==========
Description was changed from ========== Report nav timing 2 instance as soon as it's requested. This patch is not ready to be reviewed yet. Just for prototyping and getting people on board. BUG=687398 ========== to ========== Report nav timing 2 instance as soon as it's requested. This patch is not ready to be reviewed yet. Just for prototyping and getting people on board. Doc: https://docs.google.com/a/chromium.org/document/d/1QX2zVcYpuFTsq0MEZH9J8ZUT4C... BUG=687398 ==========
Description was changed from ========== Report nav timing 2 instance as soon as it's requested. This patch is not ready to be reviewed yet. Just for prototyping and getting people on board. Doc: https://docs.google.com/a/chromium.org/document/d/1QX2zVcYpuFTsq0MEZH9J8ZUT4C... BUG=687398 ========== to ========== Report nav timing 2 instance as soon as it's requested. Doc: https://docs.google.com/a/chromium.org/document/d/1QX2zVcYpuFTsq0MEZH9J8ZUT4C... BUG=687398 ==========
Patchset #2 (id:200001) has been deleted
Patchset #2 (id:220001) has been deleted
The CQ bit was checked by sunjian@chromium.org to run a CQ dry run
The CQ bit was unchecked by sunjian@chromium.org
sunjian@chromium.org changed reviewers: + panicker@chromium.org, yoav@yoav.ws
Changes: 1.Create nav timing instance when accessed through getEntries/getEntriesByType/getEntriesByName or when loadEventEnd happens. 2. Notify the observers of nav timing instance when loadEventEnd happens. 3. Nav timing 2 instance will also available in non-http sites like data:uri as long as load events exist.
Patchset #1 (id:80001) has been deleted
ksakamoto@chromium.org changed reviewers: + ksakamoto@chromium.org
https://codereview.chromium.org/2647643004/diff/260001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/loader/fetch/ResourceFetcher.cpp (right): https://codereview.chromium.org/2647643004/diff/260001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/loader/fetch/ResourceFetcher.cpp:657: return std::move(m_navigationTimingInfo); Scripts can be executed from partially loaded main document, so if we release m_navigationTimingInfo while the main resource is still loading, ResourceFetcher::handleLoaderFinish() may crash.
Addressed comments. Please take another look! https://codereview.chromium.org/2647643004/diff/260001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/loader/fetch/ResourceFetcher.cpp (right): https://codereview.chromium.org/2647643004/diff/260001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/loader/fetch/ResourceFetcher.cpp:657: return std::move(m_navigationTimingInfo); On 2017/02/03 01:28:07, ksakamoto (slow) wrote: > Scripts can be executed from partially loaded main document, so if we release > m_navigationTimingInfo while the main resource is still loading, > ResourceFetcher::handleLoaderFinish() may crash. Added an extra field which is raw pointer of m_navigationTimingInfo. I think it will fix this problem.
https://codereview.chromium.org/2647643004/diff/260001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/loader/fetch/ResourceFetcher.cpp (right): https://codereview.chromium.org/2647643004/diff/260001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/loader/fetch/ResourceFetcher.cpp:657: return std::move(m_navigationTimingInfo); On 2017/02/03 20:15:21, sunjian wrote: > On 2017/02/03 01:28:07, ksakamoto (slow) wrote: > > Scripts can be executed from partially loaded main document, so if we release > > m_navigationTimingInfo while the main resource is still loading, > > ResourceFetcher::handleLoaderFinish() may crash. > > Added an extra field which is raw pointer of m_navigationTimingInfo. I think it > will fix this problem. This is strange. I think your intention is shared_ptr i.e. fetcher and NavigationTiming both share ownership of this ResourceTimingInfo. Probably makes sense to use scoped_refptr. Note that shared_ptr is not allowed in chromium: https://chromium-cpp.appspot.com/
https://codereview.chromium.org/2647643004/diff/280001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/timing/Performance.cpp (right): https://codereview.chromium.org/2647643004/diff/280001/third_party/WebKit/Sou... third_party/WebKit/Source/core/timing/Performance.cpp:152: PerformanceNavigationTiming* Performance::getNavigationTimingInstance() { createNavigationTimingInstance? https://codereview.chromium.org/2647643004/diff/280001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/timing/PerformanceBase.cpp (left): https://codereview.chromium.org/2647643004/diff/280001/third_party/WebKit/Sou... third_party/WebKit/Source/core/timing/PerformanceBase.cpp:84: NavigationType type, could you move this to a separate CL? https://codereview.chromium.org/2647643004/diff/280001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/timing/PerformanceBase.cpp (right): https://codereview.chromium.org/2647643004/diff/280001/third_party/WebKit/Sou... third_party/WebKit/Source/core/timing/PerformanceBase.cpp:330: void PerformanceBase::notifyNavigationTimingToObserver() { s/ToObserver/ToObservers/ https://codereview.chromium.org/2647643004/diff/280001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/timing/PerformanceBase.h (right): https://codereview.chromium.org/2647643004/diff/280001/third_party/WebKit/Sou... third_party/WebKit/Source/core/timing/PerformanceBase.h:141: static SecurityOrigin* getSecurityOrigin(ExecutionContext*); Is this for calling from NT? If so, not worth making a public member, just add a helper there. https://codereview.chromium.org/2647643004/diff/280001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/timing/PerformanceEntry.h (right): https://codereview.chromium.org/2647643004/diff/280001/third_party/WebKit/Sou... third_party/WebKit/Source/core/timing/PerformanceEntry.h:76: // finish time available at construction time. Add: // Other classes must NOT override this. https://codereview.chromium.org/2647643004/diff/280001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/timing/PerformanceNavigationTiming.h (right): https://codereview.chromium.org/2647643004/diff/280001/third_party/WebKit/Sou... third_party/WebKit/Source/core/timing/PerformanceNavigationTiming.h:36: // Attributes inherrited from PerformanceResourceTiming. Add link to relevant section in design doc, and also link to future plan where this won't be necessary (when RT behavior is made consistent with NT) https://codereview.chromium.org/2647643004/diff/280001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/timing/PerformanceResourceTiming.cpp (right): https://codereview.chromium.org/2647643004/diff/280001/third_party/WebKit/Sou... third_party/WebKit/Source/core/timing/PerformanceResourceTiming.cpp:69: // This constructor is for PerformanceNavigationTiming. Add TODO with link to design doc and indicate this is temporary https://codereview.chromium.org/2647643004/diff/280001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/timing/PerformanceResourceTiming.h (right): https://codereview.chromium.org/2647643004/diff/280001/third_party/WebKit/Sou... third_party/WebKit/Source/core/timing/PerformanceResourceTiming.h:68: Add link to design doc ...
https://codereview.chromium.org/2647643004/diff/260001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/loader/fetch/ResourceFetcher.cpp (right): https://codereview.chromium.org/2647643004/diff/260001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/loader/fetch/ResourceFetcher.cpp:657: return std::move(m_navigationTimingInfo); On 2017/02/03 23:27:43, panicker wrote: > On 2017/02/03 20:15:21, sunjian wrote: > > On 2017/02/03 01:28:07, ksakamoto (slow) wrote: > > > Scripts can be executed from partially loaded main document, so if we > release > > > m_navigationTimingInfo while the main resource is still loading, > > > ResourceFetcher::handleLoaderFinish() may crash. > > > > Added an extra field which is raw pointer of m_navigationTimingInfo. I think > it > > will fix this problem. > > This is strange. I think your intention is shared_ptr i.e. fetcher and > NavigationTiming both share ownership of this ResourceTimingInfo. Probably makes > sense to use scoped_refptr. > Note that shared_ptr is not allowed in chromium: > https://chromium-cpp.appspot.com/ A shared smart pointer probably makes things easier. But i don't think we can convert a std::unique_ptr into a scoped_refptr (correct me if otherwise). And ResourceTimingInfo only allows single ownership (its create method returns a std::unique_ptr object). https://www.chromium.org/developers/smart-pointer-guidelines. This doc mentions that we should try to avoid using scoped_refptr if possible. I don't like having a raw pointer either because it may end up getting orphaned and pointing to something that doesn't exist anymore. However, this worry kind of goes away when knowing this situation is unlikely to happen cause PerformanceNavigationTiming instance, if gets created, will always outlive ResourceFetcher instance, which means that the raw pointer reference will never get orphaned. Correct me if i'm missing something.
panicker@chromium.org changed reviewers: + haraken@chromium.org
+Kentaro to advise on pointer usage (and to review the patch as well :)) https://codereview.chromium.org/2647643004/diff/260001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/loader/fetch/ResourceFetcher.cpp (right): https://codereview.chromium.org/2647643004/diff/260001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/loader/fetch/ResourceFetcher.cpp:657: return std::move(m_navigationTimingInfo); On 2017/02/04 00:08:36, sunjian wrote: > On 2017/02/03 23:27:43, panicker wrote: > > On 2017/02/03 20:15:21, sunjian wrote: > > > On 2017/02/03 01:28:07, ksakamoto (slow) wrote: > > > > Scripts can be executed from partially loaded main document, so if we > > release > > > > m_navigationTimingInfo while the main resource is still loading, > > > > ResourceFetcher::handleLoaderFinish() may crash. > > > > > > Added an extra field which is raw pointer of m_navigationTimingInfo. I think > > it > > > will fix this problem. > > > > This is strange. I think your intention is shared_ptr i.e. fetcher and > > NavigationTiming both share ownership of this ResourceTimingInfo. Probably > makes > > sense to use scoped_refptr. > > Note that shared_ptr is not allowed in chromium: > > https://chromium-cpp.appspot.com/ > > A shared smart pointer probably makes things easier. But i don't think we can > convert a std::unique_ptr into a scoped_refptr (correct me if otherwise). And > ResourceTimingInfo only allows single ownership (its create method returns a > std::unique_ptr object). > https://www.chromium.org/developers/smart-pointer-guidelines. This doc mentions > that we should try to avoid using scoped_refptr if possible. I don't like having > a raw pointer either because it may end up getting orphaned and pointing to > something that doesn't exist anymore. However, this worry kind of goes away when > knowing this situation is unlikely to happen cause PerformanceNavigationTiming > instance, if gets created, will always outlive ResourceFetcher instance, which > means that the raw pointer reference will never get orphaned. Correct me if i'm > missing something. I think shared_ptr is the right thing here. We want to clean this up when ref count goes to zero, vs. making a manual raw pointer in ResourceFetcher and making assumptions about its lifetime compared that of a PerformanceEntry. +haraken could you chime in here? before this patch: ResourceTimingInfo instance was completely owned by platform/loader/fetch/ResourceFetcher.h and kept in ResourceFetcher as a unique_ptr With this patch, we want to share the ownership of this instance with PerformanceNavigationTiming. This seems like a reasonable use of shared_ptr? Also I noticed that core/ uses std::shared_ptr and rest of Source/ uses scoped_refptr. What's the official recommendation for core/?
On 2017/02/07 00:45:05, panicker wrote: > +Kentaro to advise on pointer usage > (and to review the patch as well :)) > > https://codereview.chromium.org/2647643004/diff/260001/third_party/WebKit/Sou... > File third_party/WebKit/Source/platform/loader/fetch/ResourceFetcher.cpp > (right): > > https://codereview.chromium.org/2647643004/diff/260001/third_party/WebKit/Sou... > third_party/WebKit/Source/platform/loader/fetch/ResourceFetcher.cpp:657: return > std::move(m_navigationTimingInfo); > On 2017/02/04 00:08:36, sunjian wrote: > > On 2017/02/03 23:27:43, panicker wrote: > > > On 2017/02/03 20:15:21, sunjian wrote: > > > > On 2017/02/03 01:28:07, ksakamoto (slow) wrote: > > > > > Scripts can be executed from partially loaded main document, so if we > > > release > > > > > m_navigationTimingInfo while the main resource is still loading, > > > > > ResourceFetcher::handleLoaderFinish() may crash. > > > > > > > > Added an extra field which is raw pointer of m_navigationTimingInfo. I > think > > > it > > > > will fix this problem. > > > > > > This is strange. I think your intention is shared_ptr i.e. fetcher and > > > NavigationTiming both share ownership of this ResourceTimingInfo. Probably > > makes > > > sense to use scoped_refptr. > > > Note that shared_ptr is not allowed in chromium: > > > https://chromium-cpp.appspot.com/ > > > > A shared smart pointer probably makes things easier. But i don't think we can > > convert a std::unique_ptr into a scoped_refptr (correct me if otherwise). And > > ResourceTimingInfo only allows single ownership (its create method returns a > > std::unique_ptr object). > > https://www.chromium.org/developers/smart-pointer-guidelines. This doc > mentions > > that we should try to avoid using scoped_refptr if possible. I don't like > having > > a raw pointer either because it may end up getting orphaned and pointing to > > something that doesn't exist anymore. However, this worry kind of goes away > when > > knowing this situation is unlikely to happen cause PerformanceNavigationTiming > > instance, if gets created, will always outlive ResourceFetcher instance, which > > means that the raw pointer reference will never get orphaned. Correct me if > i'm > > missing something. > > I think shared_ptr is the right thing here. We want to clean this up when ref > count goes to zero, > vs. making a manual raw pointer in ResourceFetcher and making assumptions about > its lifetime compared that of a PerformanceEntry. > > +haraken could you chime in here? > before this patch: ResourceTimingInfo instance was completely owned by > platform/loader/fetch/ResourceFetcher.h and kept in ResourceFetcher as a > unique_ptr > With this patch, we want to share the ownership of this instance with > PerformanceNavigationTiming. This seems like a reasonable use of shared_ptr? > > Also I noticed that core/ uses std::shared_ptr and rest of Source/ uses > scoped_refptr. What's the official recommendation for core/? Blink should not use shared_ptr or scoped_refptr. Where are they used? Blink uses Oilpan or RefPtr. If the lifetime relationship is simple (or you have some reason you cannot use Oilpan), use RefPtr. Otherwise, use Oilpan.
On 2017/02/07 01:09:58, haraken wrote: > On 2017/02/07 00:45:05, panicker wrote: > > +Kentaro to advise on pointer usage > > (and to review the patch as well :)) > > > > > https://codereview.chromium.org/2647643004/diff/260001/third_party/WebKit/Sou... > > File third_party/WebKit/Source/platform/loader/fetch/ResourceFetcher.cpp > > (right): > > > > > https://codereview.chromium.org/2647643004/diff/260001/third_party/WebKit/Sou... > > third_party/WebKit/Source/platform/loader/fetch/ResourceFetcher.cpp:657: > return > > std::move(m_navigationTimingInfo); > > On 2017/02/04 00:08:36, sunjian wrote: > > > On 2017/02/03 23:27:43, panicker wrote: > > > > On 2017/02/03 20:15:21, sunjian wrote: > > > > > On 2017/02/03 01:28:07, ksakamoto (slow) wrote: > > > > > > Scripts can be executed from partially loaded main document, so if we > > > > release > > > > > > m_navigationTimingInfo while the main resource is still loading, > > > > > > ResourceFetcher::handleLoaderFinish() may crash. > > > > > > > > > > Added an extra field which is raw pointer of m_navigationTimingInfo. I > > think > > > > it > > > > > will fix this problem. > > > > > > > > This is strange. I think your intention is shared_ptr i.e. fetcher and > > > > NavigationTiming both share ownership of this ResourceTimingInfo. Probably > > > makes > > > > sense to use scoped_refptr. > > > > Note that shared_ptr is not allowed in chromium: > > > > https://chromium-cpp.appspot.com/ > > > > > > A shared smart pointer probably makes things easier. But i don't think we > can > > > convert a std::unique_ptr into a scoped_refptr (correct me if otherwise). > And > > > ResourceTimingInfo only allows single ownership (its create method returns a > > > std::unique_ptr object). > > > https://www.chromium.org/developers/smart-pointer-guidelines. This doc > > mentions > > > that we should try to avoid using scoped_refptr if possible. I don't like > > having > > > a raw pointer either because it may end up getting orphaned and pointing to > > > something that doesn't exist anymore. However, this worry kind of goes away > > when > > > knowing this situation is unlikely to happen cause > PerformanceNavigationTiming > > > instance, if gets created, will always outlive ResourceFetcher instance, > which > > > means that the raw pointer reference will never get orphaned. Correct me if > > i'm > > > missing something. > > > > I think shared_ptr is the right thing here. We want to clean this up when ref > > count goes to zero, > > vs. making a manual raw pointer in ResourceFetcher and making assumptions > about > > its lifetime compared that of a PerformanceEntry. > > > > +haraken could you chime in here? > > before this patch: ResourceTimingInfo instance was completely owned by > > platform/loader/fetch/ResourceFetcher.h and kept in ResourceFetcher as a > > unique_ptr > > With this patch, we want to share the ownership of this instance with > > PerformanceNavigationTiming. This seems like a reasonable use of shared_ptr? > > > > Also I noticed that core/ uses std::shared_ptr and rest of Source/ uses > > scoped_refptr. What's the official recommendation for core/? > > Blink should not use shared_ptr or scoped_refptr. Where are they used? > > Blink uses Oilpan or RefPtr. If the lifetime relationship is simple (or you have > some reason you cannot use Oilpan), use RefPtr. Otherwise, use Oilpan. So right now we are dealing with ResourceTimingInfo (https://cs.chromium.org/chromium/src/third_party/WebKit/Source/platform/netwo...). Its create method returns a std::unique_ptr. The question is can we convert a std::unique_ptr into a RefPtr?
On 2017/02/07 01:15:39, sunjian wrote: > On 2017/02/07 01:09:58, haraken wrote: > > On 2017/02/07 00:45:05, panicker wrote: > > > +Kentaro to advise on pointer usage > > > (and to review the patch as well :)) > > > > > > > > > https://codereview.chromium.org/2647643004/diff/260001/third_party/WebKit/Sou... > > > File third_party/WebKit/Source/platform/loader/fetch/ResourceFetcher.cpp > > > (right): > > > > > > > > > https://codereview.chromium.org/2647643004/diff/260001/third_party/WebKit/Sou... > > > third_party/WebKit/Source/platform/loader/fetch/ResourceFetcher.cpp:657: > > return > > > std::move(m_navigationTimingInfo); > > > On 2017/02/04 00:08:36, sunjian wrote: > > > > On 2017/02/03 23:27:43, panicker wrote: > > > > > On 2017/02/03 20:15:21, sunjian wrote: > > > > > > On 2017/02/03 01:28:07, ksakamoto (slow) wrote: > > > > > > > Scripts can be executed from partially loaded main document, so if > we > > > > > release > > > > > > > m_navigationTimingInfo while the main resource is still loading, > > > > > > > ResourceFetcher::handleLoaderFinish() may crash. > > > > > > > > > > > > Added an extra field which is raw pointer of m_navigationTimingInfo. I > > > think > > > > > it > > > > > > will fix this problem. > > > > > > > > > > This is strange. I think your intention is shared_ptr i.e. fetcher and > > > > > NavigationTiming both share ownership of this ResourceTimingInfo. > Probably > > > > makes > > > > > sense to use scoped_refptr. > > > > > Note that shared_ptr is not allowed in chromium: > > > > > https://chromium-cpp.appspot.com/ > > > > > > > > A shared smart pointer probably makes things easier. But i don't think we > > can > > > > convert a std::unique_ptr into a scoped_refptr (correct me if otherwise). > > And > > > > ResourceTimingInfo only allows single ownership (its create method returns > a > > > > std::unique_ptr object). > > > > https://www.chromium.org/developers/smart-pointer-guidelines. This doc > > > mentions > > > > that we should try to avoid using scoped_refptr if possible. I don't like > > > having > > > > a raw pointer either because it may end up getting orphaned and pointing > to > > > > something that doesn't exist anymore. However, this worry kind of goes > away > > > when > > > > knowing this situation is unlikely to happen cause > > PerformanceNavigationTiming > > > > instance, if gets created, will always outlive ResourceFetcher instance, > > which > > > > means that the raw pointer reference will never get orphaned. Correct me > if > > > i'm > > > > missing something. > > > > > > I think shared_ptr is the right thing here. We want to clean this up when > ref > > > count goes to zero, > > > vs. making a manual raw pointer in ResourceFetcher and making assumptions > > about > > > its lifetime compared that of a PerformanceEntry. > > > > > > +haraken could you chime in here? > > > before this patch: ResourceTimingInfo instance was completely owned by > > > platform/loader/fetch/ResourceFetcher.h and kept in ResourceFetcher as a > > > unique_ptr > > > With this patch, we want to share the ownership of this instance with > > > PerformanceNavigationTiming. This seems like a reasonable use of shared_ptr? > > > > > > Also I noticed that core/ uses std::shared_ptr and rest of Source/ uses > > > scoped_refptr. What's the official recommendation for core/? > > > > Blink should not use shared_ptr or scoped_refptr. Where are they used? > > > > Blink uses Oilpan or RefPtr. If the lifetime relationship is simple (or you > have > > some reason you cannot use Oilpan), use RefPtr. Otherwise, use Oilpan. > > So right now we are dealing with ResourceTimingInfo > (https://cs.chromium.org/chromium/src/third_party/WebKit/Source/platform/netwo...). > Its create method returns a std::unique_ptr. The question is can we convert a > std::unique_ptr into a RefPtr? No. You need to make ResourceTimingInfo inherit from RefCounted<ResourceTimingInfo> to use RefPtr or inherit from GarbageCollectedFinalized<ResourceTimingInfo> to use Oilpan.
On 2017/02/07 01:18:25, haraken wrote: > On 2017/02/07 01:15:39, sunjian wrote: > > On 2017/02/07 01:09:58, haraken wrote: > > > On 2017/02/07 00:45:05, panicker wrote: > > > > +Kentaro to advise on pointer usage > > > > (and to review the patch as well :)) > > > > > > > > > > > > > > https://codereview.chromium.org/2647643004/diff/260001/third_party/WebKit/Sou... > > > > File third_party/WebKit/Source/platform/loader/fetch/ResourceFetcher.cpp > > > > (right): > > > > > > > > > > > > > > https://codereview.chromium.org/2647643004/diff/260001/third_party/WebKit/Sou... > > > > third_party/WebKit/Source/platform/loader/fetch/ResourceFetcher.cpp:657: > > > return > > > > std::move(m_navigationTimingInfo); > > > > On 2017/02/04 00:08:36, sunjian wrote: > > > > > On 2017/02/03 23:27:43, panicker wrote: > > > > > > On 2017/02/03 20:15:21, sunjian wrote: > > > > > > > On 2017/02/03 01:28:07, ksakamoto (slow) wrote: > > > > > > > > Scripts can be executed from partially loaded main document, so if > > we > > > > > > release > > > > > > > > m_navigationTimingInfo while the main resource is still loading, > > > > > > > > ResourceFetcher::handleLoaderFinish() may crash. > > > > > > > > > > > > > > Added an extra field which is raw pointer of m_navigationTimingInfo. > I > > > > think > > > > > > it > > > > > > > will fix this problem. > > > > > > > > > > > > This is strange. I think your intention is shared_ptr i.e. fetcher and > > > > > > NavigationTiming both share ownership of this ResourceTimingInfo. > > Probably > > > > > makes > > > > > > sense to use scoped_refptr. > > > > > > Note that shared_ptr is not allowed in chromium: > > > > > > https://chromium-cpp.appspot.com/ > > > > > > > > > > A shared smart pointer probably makes things easier. But i don't think > we > > > can > > > > > convert a std::unique_ptr into a scoped_refptr (correct me if > otherwise). > > > And > > > > > ResourceTimingInfo only allows single ownership (its create method > returns > > a > > > > > std::unique_ptr object). > > > > > https://www.chromium.org/developers/smart-pointer-guidelines. This doc > > > > mentions > > > > > that we should try to avoid using scoped_refptr if possible. I don't > like > > > > having > > > > > a raw pointer either because it may end up getting orphaned and pointing > > to > > > > > something that doesn't exist anymore. However, this worry kind of goes > > away > > > > when > > > > > knowing this situation is unlikely to happen cause > > > PerformanceNavigationTiming > > > > > instance, if gets created, will always outlive ResourceFetcher instance, > > > which > > > > > means that the raw pointer reference will never get orphaned. Correct me > > if > > > > i'm > > > > > missing something. > > > > > > > > I think shared_ptr is the right thing here. We want to clean this up when > > ref > > > > count goes to zero, > > > > vs. making a manual raw pointer in ResourceFetcher and making assumptions > > > about > > > > its lifetime compared that of a PerformanceEntry. > > > > > > > > +haraken could you chime in here? > > > > before this patch: ResourceTimingInfo instance was completely owned by > > > > platform/loader/fetch/ResourceFetcher.h and kept in ResourceFetcher as a > > > > unique_ptr > > > > With this patch, we want to share the ownership of this instance with > > > > PerformanceNavigationTiming. This seems like a reasonable use of > shared_ptr? > > > > > > > > Also I noticed that core/ uses std::shared_ptr and rest of Source/ uses > > > > scoped_refptr. What's the official recommendation for core/? > > > > > > Blink should not use shared_ptr or scoped_refptr. Where are they used? > > > > > > Blink uses Oilpan or RefPtr. If the lifetime relationship is simple (or you > > have > > > some reason you cannot use Oilpan), use RefPtr. Otherwise, use Oilpan. > > > > So right now we are dealing with ResourceTimingInfo > > > (https://cs.chromium.org/chromium/src/third_party/WebKit/Source/platform/netwo...). > > Its create method returns a std::unique_ptr. The question is can we convert a > > std::unique_ptr into a RefPtr? > > No. You need to make ResourceTimingInfo inherit from > RefCounted<ResourceTimingInfo> to use RefPtr or inherit from > GarbageCollectedFinalized<ResourceTimingInfo> to use Oilpan. Right. I wasn't confident about doing that because it seems the author of ResourceTimingInfo intends to make it a singly-owned object. Is there any other alternatives we can use?
On 2017/02/07 01:28:49, sunjian wrote: > On 2017/02/07 01:18:25, haraken wrote: > > On 2017/02/07 01:15:39, sunjian wrote: > > > On 2017/02/07 01:09:58, haraken wrote: > > > > On 2017/02/07 00:45:05, panicker wrote: > > > > > +Kentaro to advise on pointer usage > > > > > (and to review the patch as well :)) > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/2647643004/diff/260001/third_party/WebKit/Sou... > > > > > File third_party/WebKit/Source/platform/loader/fetch/ResourceFetcher.cpp > > > > > (right): > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/2647643004/diff/260001/third_party/WebKit/Sou... > > > > > third_party/WebKit/Source/platform/loader/fetch/ResourceFetcher.cpp:657: > > > > return > > > > > std::move(m_navigationTimingInfo); > > > > > On 2017/02/04 00:08:36, sunjian wrote: > > > > > > On 2017/02/03 23:27:43, panicker wrote: > > > > > > > On 2017/02/03 20:15:21, sunjian wrote: > > > > > > > > On 2017/02/03 01:28:07, ksakamoto (slow) wrote: > > > > > > > > > Scripts can be executed from partially loaded main document, so > if > > > we > > > > > > > release > > > > > > > > > m_navigationTimingInfo while the main resource is still loading, > > > > > > > > > ResourceFetcher::handleLoaderFinish() may crash. > > > > > > > > > > > > > > > > Added an extra field which is raw pointer of > m_navigationTimingInfo. > > I > > > > > think > > > > > > > it > > > > > > > > will fix this problem. > > > > > > > > > > > > > > This is strange. I think your intention is shared_ptr i.e. fetcher > and > > > > > > > NavigationTiming both share ownership of this ResourceTimingInfo. > > > Probably > > > > > > makes > > > > > > > sense to use scoped_refptr. > > > > > > > Note that shared_ptr is not allowed in chromium: > > > > > > > https://chromium-cpp.appspot.com/ > > > > > > > > > > > > A shared smart pointer probably makes things easier. But i don't think > > we > > > > can > > > > > > convert a std::unique_ptr into a scoped_refptr (correct me if > > otherwise). > > > > And > > > > > > ResourceTimingInfo only allows single ownership (its create method > > returns > > > a > > > > > > std::unique_ptr object). > > > > > > https://www.chromium.org/developers/smart-pointer-guidelines. This doc > > > > > mentions > > > > > > that we should try to avoid using scoped_refptr if possible. I don't > > like > > > > > having > > > > > > a raw pointer either because it may end up getting orphaned and > pointing > > > to > > > > > > something that doesn't exist anymore. However, this worry kind of goes > > > away > > > > > when > > > > > > knowing this situation is unlikely to happen cause > > > > PerformanceNavigationTiming > > > > > > instance, if gets created, will always outlive ResourceFetcher > instance, > > > > which > > > > > > means that the raw pointer reference will never get orphaned. Correct > me > > > if > > > > > i'm > > > > > > missing something. > > > > > > > > > > I think shared_ptr is the right thing here. We want to clean this up > when > > > ref > > > > > count goes to zero, > > > > > vs. making a manual raw pointer in ResourceFetcher and making > assumptions > > > > about > > > > > its lifetime compared that of a PerformanceEntry. > > > > > > > > > > +haraken could you chime in here? > > > > > before this patch: ResourceTimingInfo instance was completely owned by > > > > > platform/loader/fetch/ResourceFetcher.h and kept in ResourceFetcher as a > > > > > unique_ptr > > > > > With this patch, we want to share the ownership of this instance with > > > > > PerformanceNavigationTiming. This seems like a reasonable use of > > shared_ptr? > > > > > > > > > > Also I noticed that core/ uses std::shared_ptr and rest of Source/ uses > > > > > scoped_refptr. What's the official recommendation for core/? > > > > > > > > Blink should not use shared_ptr or scoped_refptr. Where are they used? > > > > > > > > Blink uses Oilpan or RefPtr. If the lifetime relationship is simple (or > you > > > have > > > > some reason you cannot use Oilpan), use RefPtr. Otherwise, use Oilpan. > > > > > > So right now we are dealing with ResourceTimingInfo > > > > > > (https://cs.chromium.org/chromium/src/third_party/WebKit/Source/platform/netwo...). > > > Its create method returns a std::unique_ptr. The question is can we convert > a > > > std::unique_ptr into a RefPtr? > > > > No. You need to make ResourceTimingInfo inherit from > > RefCounted<ResourceTimingInfo> to use RefPtr or inherit from > > GarbageCollectedFinalized<ResourceTimingInfo> to use Oilpan. > > Right. I wasn't confident about doing that because it seems the author of > ResourceTimingInfo intends to make it a singly-owned object. Is there any other > alternatives we can use? I'd just use RefPtr (or Oilpan) since it might be conceptually singly-owned but there are actually multiple pointers pointing to that object.
On 2017/02/07 01:34:43, haraken wrote: > On 2017/02/07 01:28:49, sunjian wrote: > > On 2017/02/07 01:18:25, haraken wrote: > > > On 2017/02/07 01:15:39, sunjian wrote: > > > > On 2017/02/07 01:09:58, haraken wrote: > > > > > On 2017/02/07 00:45:05, panicker wrote: > > > > > > +Kentaro to advise on pointer usage > > > > > > (and to review the patch as well :)) > > > > > > > > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/2647643004/diff/260001/third_party/WebKit/Sou... > > > > > > File > third_party/WebKit/Source/platform/loader/fetch/ResourceFetcher.cpp > > > > > > (right): > > > > > > > > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/2647643004/diff/260001/third_party/WebKit/Sou... > > > > > > > third_party/WebKit/Source/platform/loader/fetch/ResourceFetcher.cpp:657: > > > > > return > > > > > > std::move(m_navigationTimingInfo); > > > > > > On 2017/02/04 00:08:36, sunjian wrote: > > > > > > > On 2017/02/03 23:27:43, panicker wrote: > > > > > > > > On 2017/02/03 20:15:21, sunjian wrote: > > > > > > > > > On 2017/02/03 01:28:07, ksakamoto (slow) wrote: > > > > > > > > > > Scripts can be executed from partially loaded main document, > so > > if > > > > we > > > > > > > > release > > > > > > > > > > m_navigationTimingInfo while the main resource is still > loading, > > > > > > > > > > ResourceFetcher::handleLoaderFinish() may crash. > > > > > > > > > > > > > > > > > > Added an extra field which is raw pointer of > > m_navigationTimingInfo. > > > I > > > > > > think > > > > > > > > it > > > > > > > > > will fix this problem. > > > > > > > > > > > > > > > > This is strange. I think your intention is shared_ptr i.e. fetcher > > and > > > > > > > > NavigationTiming both share ownership of this ResourceTimingInfo. > > > > Probably > > > > > > > makes > > > > > > > > sense to use scoped_refptr. > > > > > > > > Note that shared_ptr is not allowed in chromium: > > > > > > > > https://chromium-cpp.appspot.com/ > > > > > > > > > > > > > > A shared smart pointer probably makes things easier. But i don't > think > > > we > > > > > can > > > > > > > convert a std::unique_ptr into a scoped_refptr (correct me if > > > otherwise). > > > > > And > > > > > > > ResourceTimingInfo only allows single ownership (its create method > > > returns > > > > a > > > > > > > std::unique_ptr object). > > > > > > > https://www.chromium.org/developers/smart-pointer-guidelines. This > doc > > > > > > mentions > > > > > > > that we should try to avoid using scoped_refptr if possible. I don't > > > like > > > > > > having > > > > > > > a raw pointer either because it may end up getting orphaned and > > pointing > > > > to > > > > > > > something that doesn't exist anymore. However, this worry kind of > goes > > > > away > > > > > > when > > > > > > > knowing this situation is unlikely to happen cause > > > > > PerformanceNavigationTiming > > > > > > > instance, if gets created, will always outlive ResourceFetcher > > instance, > > > > > which > > > > > > > means that the raw pointer reference will never get orphaned. > Correct > > me > > > > if > > > > > > i'm > > > > > > > missing something. > > > > > > > > > > > > I think shared_ptr is the right thing here. We want to clean this up > > when > > > > ref > > > > > > count goes to zero, > > > > > > vs. making a manual raw pointer in ResourceFetcher and making > > assumptions > > > > > about > > > > > > its lifetime compared that of a PerformanceEntry. > > > > > > > > > > > > +haraken could you chime in here? > > > > > > before this patch: ResourceTimingInfo instance was completely owned by > > > > > > platform/loader/fetch/ResourceFetcher.h and kept in ResourceFetcher as > a > > > > > > unique_ptr > > > > > > With this patch, we want to share the ownership of this instance with > > > > > > PerformanceNavigationTiming. This seems like a reasonable use of > > > shared_ptr? > > > > > > > > > > > > Also I noticed that core/ uses std::shared_ptr and rest of Source/ > uses > > > > > > scoped_refptr. What's the official recommendation for core/? > > > > > > > > > > Blink should not use shared_ptr or scoped_refptr. Where are they used? > > > > > > > > > > Blink uses Oilpan or RefPtr. If the lifetime relationship is simple (or > > you > > > > have > > > > > some reason you cannot use Oilpan), use RefPtr. Otherwise, use Oilpan. > > > > > > > > So right now we are dealing with ResourceTimingInfo > > > > > > > > > > (https://cs.chromium.org/chromium/src/third_party/WebKit/Source/platform/netwo...). > > > > Its create method returns a std::unique_ptr. The question is can we > convert > > a > > > > std::unique_ptr into a RefPtr? > > > > > > No. You need to make ResourceTimingInfo inherit from > > > RefCounted<ResourceTimingInfo> to use RefPtr or inherit from > > > GarbageCollectedFinalized<ResourceTimingInfo> to use Oilpan. > > > > Right. I wasn't confident about doing that because it seems the author of > > ResourceTimingInfo intends to make it a singly-owned object. Is there any > other > > alternatives we can use? > > I'd just use RefPtr (or Oilpan) since it might be conceptually singly-owned but > there are actually multiple pointers pointing to that object. Sounds good. Thanks!
Patchset #5 (id:320001) has been deleted
Patchset #5 (id:340001) has been deleted
The CQ bit was checked by sunjian@chromium.org to run a CQ dry run
The CQ bit was unchecked by sunjian@chromium.org
The CQ bit was checked by sunjian@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_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
Patchset #5 (id:360001) has been deleted
The CQ bit was checked by sunjian@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.
Made ResourceTimingInfo ref-counted, addressed comments. Passed the dry run. https://codereview.chromium.org/2647643004/diff/280001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/timing/Performance.cpp (right): https://codereview.chromium.org/2647643004/diff/280001/third_party/WebKit/Sou... third_party/WebKit/Source/core/timing/Performance.cpp:152: PerformanceNavigationTiming* Performance::getNavigationTimingInstance() { On 2017/02/03 23:53:52, panicker wrote: > createNavigationTimingInstance? Done. https://codereview.chromium.org/2647643004/diff/280001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/timing/PerformanceBase.cpp (left): https://codereview.chromium.org/2647643004/diff/280001/third_party/WebKit/Sou... third_party/WebKit/Source/core/timing/PerformanceBase.cpp:84: NavigationType type, On 2017/02/03 23:53:52, panicker wrote: > could you move this to a separate CL? It's actually easier to keep this change. getNavigationType is not even being used as you can see. It would be pretty awkward to keep it. https://codereview.chromium.org/2647643004/diff/280001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/timing/PerformanceBase.cpp (right): https://codereview.chromium.org/2647643004/diff/280001/third_party/WebKit/Sou... third_party/WebKit/Source/core/timing/PerformanceBase.cpp:330: void PerformanceBase::notifyNavigationTimingToObserver() { On 2017/02/03 23:53:52, panicker wrote: > s/ToObserver/ToObservers/ Done. https://codereview.chromium.org/2647643004/diff/280001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/timing/PerformanceBase.h (right): https://codereview.chromium.org/2647643004/diff/280001/third_party/WebKit/Sou... third_party/WebKit/Source/core/timing/PerformanceBase.h:141: static SecurityOrigin* getSecurityOrigin(ExecutionContext*); On 2017/02/03 23:53:52, panicker wrote: > Is this for calling from NT? If so, not worth making a public member, just add a > helper there. Made it inline in PerformanceNavigationTiming and kept it as how it was (a private-scoped method) in PerformanceBase. Don't want to introduce any new change that's not directly related to this patch. https://codereview.chromium.org/2647643004/diff/280001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/timing/PerformanceEntry.h (right): https://codereview.chromium.org/2647643004/diff/280001/third_party/WebKit/Sou... third_party/WebKit/Source/core/timing/PerformanceEntry.h:76: // finish time available at construction time. On 2017/02/03 23:53:52, panicker wrote: > Add: > // Other classes must NOT override this. Done. https://codereview.chromium.org/2647643004/diff/280001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/timing/PerformanceNavigationTiming.h (right): https://codereview.chromium.org/2647643004/diff/280001/third_party/WebKit/Sou... third_party/WebKit/Source/core/timing/PerformanceNavigationTiming.h:36: // Attributes inherrited from PerformanceResourceTiming. On 2017/02/03 23:53:52, panicker wrote: > Add link to relevant section in design doc, and also link to future plan where > this won't be necessary (when RT behavior is made consistent with NT) Done. https://codereview.chromium.org/2647643004/diff/280001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/timing/PerformanceResourceTiming.cpp (right): https://codereview.chromium.org/2647643004/diff/280001/third_party/WebKit/Sou... third_party/WebKit/Source/core/timing/PerformanceResourceTiming.cpp:69: // This constructor is for PerformanceNavigationTiming. On 2017/02/03 23:53:52, panicker wrote: > Add TODO with link to design doc and indicate this is temporary I'm not sure what TODO to add. We can't get rid of this constructor until we decide to implement PerformanceResourceTiming the same way PerformanceNavigationTiming is implemented. I don't know if it is a separate bug to fix. https://codereview.chromium.org/2647643004/diff/280001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/timing/PerformanceResourceTiming.h (right): https://codereview.chromium.org/2647643004/diff/280001/third_party/WebKit/Sou... third_party/WebKit/Source/core/timing/PerformanceResourceTiming.h:68: On 2017/02/03 23:53:52, panicker wrote: > Add link to design doc ... Done.
https://codereview.chromium.org/2647643004/diff/380001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/loader/DocumentLoader.cpp (right): https://codereview.chromium.org/2647643004/diff/380001/third_party/WebKit/Sou... third_party/WebKit/Source/core/loader/DocumentLoader.cpp:69: #include "platform/network/ResourceTimingInfo.h" Still needed? https://codereview.chromium.org/2647643004/diff/380001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/timing/Performance.cpp (right): https://codereview.chromium.org/2647643004/diff/380001/third_party/WebKit/Sou... third_party/WebKit/Source/core/timing/Performance.cpp:144: ResourceTimingInfo* Performance::getNavigationTimingInfo() const { It's probably better to just inline this function since it's only used once. https://codereview.chromium.org/2647643004/diff/380001/third_party/WebKit/Sou... third_party/WebKit/Source/core/timing/Performance.cpp:154: toTimeOrigin(frame())); You can just use timeOrigin(). https://codereview.chromium.org/2647643004/diff/380001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/timing/PerformanceBase.cpp (left): https://codereview.chromium.org/2647643004/diff/380001/third_party/WebKit/Sou... third_party/WebKit/Source/core/timing/PerformanceBase.cpp:348: if (!RuntimeEnabledFeatures::performanceNavigationTiming2Enabled()) Does this runtime flag still work after your change? https://codereview.chromium.org/2647643004/diff/380001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/timing/PerformanceNavigationTiming.cpp (right): https://codereview.chromium.org/2647643004/diff/380001/third_party/WebKit/Sou... third_party/WebKit/Source/core/timing/PerformanceNavigationTiming.cpp:20: ResourceTimingInfo* info, Since this constructor takes ownership of the ResourceTimingInfo, the argument should be a PassRefPtr<ResourceTimingInfo>. https://codereview.chromium.org/2647643004/diff/380001/third_party/WebKit/Sou... third_party/WebKit/Source/core/timing/PerformanceNavigationTiming.cpp:73: bool PerformanceNavigationTiming::getAllowRedirectDetails() const { Can you precompute this in the constructor and store it in a member field? https://codereview.chromium.org/2647643004/diff/380001/third_party/WebKit/Sou... third_party/WebKit/Source/core/timing/PerformanceNavigationTiming.cpp:185: return ""; // Not available yet. Can this happen? I think navigation type should be available by the time scripts start running. https://codereview.chromium.org/2647643004/diff/380001/third_party/WebKit/Sou... third_party/WebKit/Source/core/timing/PerformanceNavigationTiming.cpp:203: DOMHighResTimeStamp PerformanceNavigationTiming::workerStart() const { Can we reduce code duplication between PerformanceNavigationTiming and PerformanceResourceTiming? For example, workerStart could be simplified like this: DOMHighResTimeStamp PerformanceResourceTiming::workerStart(ResourceLoadTiming* timing) const { // common underlying implementation using |timing| } // virtual function DOMHighResTimeStamp PerformanceResourceTiming::workerStart() const { return workerStart(m_timing); } DOMHighResTimeStamp PerformanceNavigationTiming::workerStart() const { return workerStart(resourceLoadTiming()); } https://codereview.chromium.org/2647643004/diff/380001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/timing/PerformanceNavigationTiming.h (right): https://codereview.chromium.org/2647643004/diff/380001/third_party/WebKit/Sou... third_party/WebKit/Source/core/timing/PerformanceNavigationTiming.h:38: // https://goo.gl/uNecAj. Can you make this doc publicly visible? https://codereview.chromium.org/2647643004/diff/380001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/loader/fetch/ResourceFetcher.cpp (right): https://codereview.chromium.org/2647643004/diff/380001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/loader/fetch/ResourceFetcher.cpp:673: ResourceTimingInfo* ResourceFetcher::getNavigationTimingInfo() { Why is this moved from line 1134? https://codereview.chromium.org/2647643004/diff/380001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/loader/fetch/ResourceFetcher.h (right): https://codereview.chromium.org/2647643004/diff/380001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/loader/fetch/ResourceFetcher.h:145: // This method should only be called once. Now this can be called multiple times safely.
https://codereview.chromium.org/2647643004/diff/380001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/timing/PerformanceNavigationTiming.cpp (right): https://codereview.chromium.org/2647643004/diff/380001/third_party/WebKit/Sou... third_party/WebKit/Source/core/timing/PerformanceNavigationTiming.cpp:203: DOMHighResTimeStamp PerformanceNavigationTiming::workerStart() const { On 2017/02/13 08:22:29, Kunihiko Sakamoto wrote: > Can we reduce code duplication between PerformanceNavigationTiming and > PerformanceResourceTiming? > For example, workerStart could be simplified like this: > > > DOMHighResTimeStamp PerformanceResourceTiming::workerStart(ResourceLoadTiming* > timing) const { > // common underlying implementation using |timing| > } > > // virtual function > DOMHighResTimeStamp PerformanceResourceTiming::workerStart() const { > return workerStart(m_timing); > } > > DOMHighResTimeStamp PerformanceNavigationTiming::workerStart() const { > return workerStart(resourceLoadTiming()); > } Or even simpler: // Overridden by PerformanceResourceTiming virtual ResourceLoadTiming* PerformanceResourceTiming::resourceLoadTiming() { return m_timing; } // non-virtual DOMHighResTimeStamp PerformanceResourceTiming::workerStart() const { ResourceLoadTiming* timing = resourceLoadTiming(); // ... }
The CQ bit was checked by sunjian@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-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)
The CQ bit was checked by sunjian@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-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...)
The CQ bit was checked by sunjian@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-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...) 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 sunjian@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.
Addressed comments. Please take another look! https://codereview.chromium.org/2647643004/diff/380001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/loader/DocumentLoader.cpp (right): https://codereview.chromium.org/2647643004/diff/380001/third_party/WebKit/Sou... third_party/WebKit/Source/core/loader/DocumentLoader.cpp:69: #include "platform/network/ResourceTimingInfo.h" On 2017/02/13 08:22:29, Kunihiko Sakamoto wrote: > Still needed? It compiles fine without it. But it is actually a direct dependency. Maybe some other header files included it also. But I think it's better to have it? https://codereview.chromium.org/2647643004/diff/380001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/timing/Performance.cpp (right): https://codereview.chromium.org/2647643004/diff/380001/third_party/WebKit/Sou... third_party/WebKit/Source/core/timing/Performance.cpp:144: ResourceTimingInfo* Performance::getNavigationTimingInfo() const { On 2017/02/13 08:22:29, Kunihiko Sakamoto wrote: > It's probably better to just inline this function since it's only used > once. Done. https://codereview.chromium.org/2647643004/diff/380001/third_party/WebKit/Sou... third_party/WebKit/Source/core/timing/Performance.cpp:154: toTimeOrigin(frame())); On 2017/02/13 08:22:29, Kunihiko Sakamoto wrote: > You can just use timeOrigin(). Done. https://codereview.chromium.org/2647643004/diff/380001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/timing/PerformanceBase.cpp (left): https://codereview.chromium.org/2647643004/diff/380001/third_party/WebKit/Sou... third_party/WebKit/Source/core/timing/PerformanceBase.cpp:348: if (!RuntimeEnabledFeatures::performanceNavigationTiming2Enabled()) On 2017/02/13 08:22:29, Kunihiko Sakamoto wrote: > Does this runtime flag still work after your change? I don't think we need the flag any more since this feature is already flipped to stable. https://codereview.chromium.org/2647643004/diff/380001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/timing/PerformanceNavigationTiming.cpp (right): https://codereview.chromium.org/2647643004/diff/380001/third_party/WebKit/Sou... third_party/WebKit/Source/core/timing/PerformanceNavigationTiming.cpp:20: ResourceTimingInfo* info, On 2017/02/13 08:22:29, Kunihiko Sakamoto wrote: > Since this constructor takes ownership of the ResourceTimingInfo, the argument > should be a PassRefPtr<ResourceTimingInfo>. I don't want PerformanceNavigationTiming to own ResourceTimingInfo alone. I still want ResourceFetcher owning it at the same time. https://codereview.chromium.org/2647643004/diff/380001/third_party/WebKit/Sou... third_party/WebKit/Source/core/timing/PerformanceNavigationTiming.cpp:73: bool PerformanceNavigationTiming::getAllowRedirectDetails() const { On 2017/02/13 08:22:29, Kunihiko Sakamoto wrote: > Can you precompute this in the constructor and store it in a member field? That would be ideal. But we don't know exactly when this is going to be triggered. We provide PerformanceNavigationTiming instance as early as it's requested which can be as soon as Performance instance is available. Specifically, we rely on a final ResourceResponse instance in order to compute this flag. And we don't know when this happens. https://codereview.chromium.org/2647643004/diff/380001/third_party/WebKit/Sou... third_party/WebKit/Source/core/timing/PerformanceNavigationTiming.cpp:185: return ""; // Not available yet. On 2017/02/13 08:22:29, Kunihiko Sakamoto wrote: > Can this happen? I think navigation type should be available by the time scripts > start running. You are probably right. I'll go ahead and replace the checking with DCHECKS then. https://codereview.chromium.org/2647643004/diff/380001/third_party/WebKit/Sou... third_party/WebKit/Source/core/timing/PerformanceNavigationTiming.cpp:203: DOMHighResTimeStamp PerformanceNavigationTiming::workerStart() const { On 2017/02/13 08:22:29, Kunihiko Sakamoto wrote: > Can we reduce code duplication between PerformanceNavigationTiming and > PerformanceResourceTiming? > For example, workerStart could be simplified like this: > > > DOMHighResTimeStamp PerformanceResourceTiming::workerStart(ResourceLoadTiming* > timing) const { > // common underlying implementation using |timing| > } > > // virtual function > DOMHighResTimeStamp PerformanceResourceTiming::workerStart() const { > return workerStart(m_timing); > } > > DOMHighResTimeStamp PerformanceNavigationTiming::workerStart() const { > return workerStart(resourceLoadTiming()); > } Acknowledged. https://codereview.chromium.org/2647643004/diff/380001/third_party/WebKit/Sou... third_party/WebKit/Source/core/timing/PerformanceNavigationTiming.cpp:203: DOMHighResTimeStamp PerformanceNavigationTiming::workerStart() const { On 2017/02/13 08:22:29, Kunihiko Sakamoto wrote: > Can we reduce code duplication between PerformanceNavigationTiming and > PerformanceResourceTiming? > For example, workerStart could be simplified like this: > > > DOMHighResTimeStamp PerformanceResourceTiming::workerStart(ResourceLoadTiming* > timing) const { > // common underlying implementation using |timing| > } > > // virtual function > DOMHighResTimeStamp PerformanceResourceTiming::workerStart() const { > return workerStart(m_timing); > } > > DOMHighResTimeStamp PerformanceNavigationTiming::workerStart() const { > return workerStart(resourceLoadTiming()); > } Done. https://codereview.chromium.org/2647643004/diff/380001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/timing/PerformanceNavigationTiming.h (right): https://codereview.chromium.org/2647643004/diff/380001/third_party/WebKit/Sou... third_party/WebKit/Source/core/timing/PerformanceNavigationTiming.h:38: // https://goo.gl/uNecAj. On 2017/02/13 08:22:29, Kunihiko Sakamoto wrote: > Can you make this doc publicly visible? Done. https://codereview.chromium.org/2647643004/diff/380001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/loader/fetch/ResourceFetcher.cpp (right): https://codereview.chromium.org/2647643004/diff/380001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/loader/fetch/ResourceFetcher.cpp:673: ResourceTimingInfo* ResourceFetcher::getNavigationTimingInfo() { On 2017/02/13 08:22:29, Kunihiko Sakamoto wrote: > Why is this moved from line 1134? I think i deleted it because i thought i didn't need it any more. Later i brought it back to a different place. I was trying different approaches before. https://codereview.chromium.org/2647643004/diff/380001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/loader/fetch/ResourceFetcher.h (right): https://codereview.chromium.org/2647643004/diff/380001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/loader/fetch/ResourceFetcher.h:145: // This method should only be called once. On 2017/02/13 08:22:29, Kunihiko Sakamoto wrote: > Now this can be called multiple times safely. Done.
The CQ bit was checked by sunjian@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 sunjian@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.
https://codereview.chromium.org/2647643004/diff/380001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/loader/DocumentLoader.cpp (right): https://codereview.chromium.org/2647643004/diff/380001/third_party/WebKit/Sou... third_party/WebKit/Source/core/loader/DocumentLoader.cpp:69: #include "platform/network/ResourceTimingInfo.h" On 2017/02/14 21:29:02, sunjian wrote: > On 2017/02/13 08:22:29, Kunihiko Sakamoto wrote: > > Still needed? > > It compiles fine without it. But it is actually a direct dependency. Maybe some > other header files included it also. But I think it's better to have it? Oh I see. Let's keep it. https://codereview.chromium.org/2647643004/diff/380001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/timing/PerformanceBase.cpp (left): https://codereview.chromium.org/2647643004/diff/380001/third_party/WebKit/Sou... third_party/WebKit/Source/core/timing/PerformanceBase.cpp:348: if (!RuntimeEnabledFeatures::performanceNavigationTiming2Enabled()) On 2017/02/14 21:29:03, sunjian wrote: > On 2017/02/13 08:22:29, Kunihiko Sakamoto wrote: > > Does this runtime flag still work after your change? > > I don't think we need the flag any more since this feature is already flipped to > stable. The flag makes it easier to unship the feature if a critical security / privacy issue is found. I recommend keeping it functional for 1-2 milestones after launch, unless it's hard to maintain. Maybe we can just make createNavigationTimingInstance() return nullptr if the flag is off? https://codereview.chromium.org/2647643004/diff/380001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/timing/PerformanceNavigationTiming.cpp (right): https://codereview.chromium.org/2647643004/diff/380001/third_party/WebKit/Sou... third_party/WebKit/Source/core/timing/PerformanceNavigationTiming.cpp:20: ResourceTimingInfo* info, On 2017/02/14 21:29:03, sunjian wrote: > On 2017/02/13 08:22:29, Kunihiko Sakamoto wrote: > > Since this constructor takes ownership of the ResourceTimingInfo, the argument > > should be a PassRefPtr<ResourceTimingInfo>. > I don't want PerformanceNavigationTiming to own ResourceTimingInfo alone. I > still want ResourceFetcher owning it at the same time. But once ResourceFetcher has gone, PerformanceNavigationTiming actually becomes the owner, right? I'm not sure whether we have clear style guideline for such shared-ownership case. Perhaps haraken@ might know? https://codereview.chromium.org/2647643004/diff/380001/third_party/WebKit/Sou... third_party/WebKit/Source/core/timing/PerformanceNavigationTiming.cpp:73: bool PerformanceNavigationTiming::getAllowRedirectDetails() const { On 2017/02/14 21:29:03, sunjian wrote: > On 2017/02/13 08:22:29, Kunihiko Sakamoto wrote: > > Can you precompute this in the constructor and store it in a member field? > > That would be ideal. But we don't know exactly when this is going to be > triggered. We provide PerformanceNavigationTiming instance as early as it's > requested which can be as soon as Performance instance is available. > Specifically, we rely on a final ResourceResponse instance in order to compute > this flag. And we don't know when this happens. Does that mean this function could return different values depending on timings? That's really weird. Looking at the code, ResourceTimingInfo::setFinalResponse() is called from ResourceFetcher::handleLoaderFinish(), which is called when main document has finished loading. So, if this function is called while document is still loading, m_resourceTimingInfo->finalResponse() below would return an uninitialized (default-constructed) value. This is clearly not correct. PerformanceBase::allowsTimingRedirect() only needs the url and headers of final response. Could we get these information at PerformanceNavigationTiming construction time? https://codereview.chromium.org/2647643004/diff/380001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/loader/fetch/ResourceFetcher.cpp (right): https://codereview.chromium.org/2647643004/diff/380001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/loader/fetch/ResourceFetcher.cpp:673: ResourceTimingInfo* ResourceFetcher::getNavigationTimingInfo() { On 2017/02/14 21:29:03, sunjian wrote: > On 2017/02/13 08:22:29, Kunihiko Sakamoto wrote: > > Why is this moved from line 1134? > > I think i deleted it because i thought i didn't need it any more. Later i > brought it back to a different place. I was trying different approaches before. I see. Can you move it back to original position, for cleaner git-blame? https://codereview.chromium.org/2647643004/diff/420001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/timing/Performance.cpp (right): https://codereview.chromium.org/2647643004/diff/420001/third_party/WebKit/Sou... third_party/WebKit/Source/core/timing/Performance.cpp:146: if (frame()) { If frame() is null, can we return nullptr here? Currently we're returning PerformanceNavigationTiming(nullptr, nullptr, timeOrigin()), which isn't useful at all. Then we can get rid of many null checks in PerformanceNavigationTiming.
Please take another look! https://codereview.chromium.org/2647643004/diff/380001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/loader/DocumentLoader.cpp (right): https://codereview.chromium.org/2647643004/diff/380001/third_party/WebKit/Sou... third_party/WebKit/Source/core/loader/DocumentLoader.cpp:69: #include "platform/network/ResourceTimingInfo.h" On 2017/02/15 06:35:30, Kunihiko Sakamoto wrote: > On 2017/02/14 21:29:02, sunjian wrote: > > On 2017/02/13 08:22:29, Kunihiko Sakamoto wrote: > > > Still needed? > > > > It compiles fine without it. But it is actually a direct dependency. Maybe > some > > other header files included it also. But I think it's better to have it? > > Oh I see. Let's keep it. Acknowledged. https://codereview.chromium.org/2647643004/diff/380001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/timing/PerformanceBase.cpp (left): https://codereview.chromium.org/2647643004/diff/380001/third_party/WebKit/Sou... third_party/WebKit/Source/core/timing/PerformanceBase.cpp:348: if (!RuntimeEnabledFeatures::performanceNavigationTiming2Enabled()) On 2017/02/15 06:35:30, Kunihiko Sakamoto wrote: > On 2017/02/14 21:29:03, sunjian wrote: > > On 2017/02/13 08:22:29, Kunihiko Sakamoto wrote: > > > Does this runtime flag still work after your change? > > > > I don't think we need the flag any more since this feature is already flipped > to > > stable. > > The flag makes it easier to unship the feature if a critical security / privacy > issue is found. I recommend keeping it functional for 1-2 milestones after > launch, unless it's hard to maintain. > > Maybe we can just make createNavigationTimingInstance() return nullptr if the > flag is off? Done. https://codereview.chromium.org/2647643004/diff/380001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/timing/PerformanceNavigationTiming.cpp (right): https://codereview.chromium.org/2647643004/diff/380001/third_party/WebKit/Sou... third_party/WebKit/Source/core/timing/PerformanceNavigationTiming.cpp:20: ResourceTimingInfo* info, On 2017/02/15 06:35:30, Kunihiko Sakamoto wrote: > On 2017/02/14 21:29:03, sunjian wrote: > > On 2017/02/13 08:22:29, Kunihiko Sakamoto wrote: > > > Since this constructor takes ownership of the ResourceTimingInfo, the > argument > > > should be a PassRefPtr<ResourceTimingInfo>. > > I don't want PerformanceNavigationTiming to own ResourceTimingInfo alone. I > > still want ResourceFetcher owning it at the same time. > > But once ResourceFetcher has gone, PerformanceNavigationTiming actually becomes > the owner, right? > > I'm not sure whether we have clear style guideline for such shared-ownership > case. Perhaps haraken@ might know? Doesn't PerformanceNavigationTiming become the owner automatically? RecourceTimingInfo will get ref()ed again when PerformanceNavigationTiming gets constructed. If ResourceFetcher dies, ResourceTimingInfo gets deref()ed, but ResourceTimingInfo should still be alive after that point right? https://codereview.chromium.org/2647643004/diff/380001/third_party/WebKit/Sou... third_party/WebKit/Source/core/timing/PerformanceNavigationTiming.cpp:73: bool PerformanceNavigationTiming::getAllowRedirectDetails() const { On 2017/02/15 06:35:30, Kunihiko Sakamoto wrote: > On 2017/02/14 21:29:03, sunjian wrote: > > On 2017/02/13 08:22:29, Kunihiko Sakamoto wrote: > > > Can you precompute this in the constructor and store it in a member field? > > > > That would be ideal. But we don't know exactly when this is going to be > > triggered. We provide PerformanceNavigationTiming instance as early as it's > > requested which can be as soon as Performance instance is available. > > Specifically, we rely on a final ResourceResponse instance in order to compute > > this flag. And we don't know when this happens. > > Does that mean this function could return different values depending on timings? > That's really weird. > > Looking at the code, ResourceTimingInfo::setFinalResponse() is called from > ResourceFetcher::handleLoaderFinish(), which is called when main document has > finished loading. So, if this function is called while document is still > loading, m_resourceTimingInfo->finalResponse() below would return an > uninitialized (default-constructed) value. This is clearly not correct. > > PerformanceBase::allowsTimingRedirect() only needs the url and headers of final > response. Could we get these information at PerformanceNavigationTiming > construction time? If we can get the final response at construction time, that would be ideal. But the nature of this change is to create PerformanceNavigationTiming instance as soon as it is requested which can be as early as Performance instance becomes available. And we have no control over when exactly it will be constructed. But good news is that when final response is an empty object, PerformanceBase::allowsTimingRedirect allows returns false. So the return value is actually predictable. It's false before final response arrives, and becomes whatever it should be and stay the same after it arrives. https://codereview.chromium.org/2647643004/diff/380001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/loader/fetch/ResourceFetcher.cpp (right): https://codereview.chromium.org/2647643004/diff/380001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/loader/fetch/ResourceFetcher.cpp:673: ResourceTimingInfo* ResourceFetcher::getNavigationTimingInfo() { On 2017/02/15 06:35:30, Kunihiko Sakamoto wrote: > On 2017/02/14 21:29:03, sunjian wrote: > > On 2017/02/13 08:22:29, Kunihiko Sakamoto wrote: > > > Why is this moved from line 1134? > > > > I think i deleted it because i thought i didn't need it any more. Later i > > brought it back to a different place. I was trying different approaches > before. > > I see. Can you move it back to original position, for cleaner git-blame? Done. https://codereview.chromium.org/2647643004/diff/420001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/timing/Performance.cpp (right): https://codereview.chromium.org/2647643004/diff/420001/third_party/WebKit/Sou... third_party/WebKit/Source/core/timing/Performance.cpp:146: if (frame()) { On 2017/02/15 06:35:30, Kunihiko Sakamoto wrote: > If frame() is null, can we return nullptr here? Currently we're returning > PerformanceNavigationTiming(nullptr, nullptr, timeOrigin()), which isn't useful > at all. > > Then we can get rid of many null checks in PerformanceNavigationTiming. Agree with returning nullptr directly when frame() is nullptr. Not sur e we can't get rid of null checks in PerformanceNavigationTiming. Because even though PerformanceNavigationTiming can be constructed with a non-null frame, it might become nullptr halfway right? LocalFrame is only a WeakMember of PerformanceNavigationTiming.
The CQ bit was checked by sunjian@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...
Patchset #8 (id:440001) has been deleted
The CQ bit was checked by sunjian@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.
https://codereview.chromium.org/2647643004/diff/380001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/timing/PerformanceNavigationTiming.cpp (right): https://codereview.chromium.org/2647643004/diff/380001/third_party/WebKit/Sou... third_party/WebKit/Source/core/timing/PerformanceNavigationTiming.cpp:73: bool PerformanceNavigationTiming::getAllowRedirectDetails() const { On 2017/02/15 20:30:28, sunjian wrote: > On 2017/02/15 06:35:30, Kunihiko Sakamoto wrote: > > On 2017/02/14 21:29:03, sunjian wrote: > > > On 2017/02/13 08:22:29, Kunihiko Sakamoto wrote: > > > > Can you precompute this in the constructor and store it in a member field? > > > > > > That would be ideal. But we don't know exactly when this is going to be > > > triggered. We provide PerformanceNavigationTiming instance as early as it's > > > requested which can be as soon as Performance instance is available. > > > Specifically, we rely on a final ResourceResponse instance in order to > compute > > > this flag. And we don't know when this happens. > > > > Does that mean this function could return different values depending on > timings? > > That's really weird. > > > > Looking at the code, ResourceTimingInfo::setFinalResponse() is called from > > ResourceFetcher::handleLoaderFinish(), which is called when main document has > > finished loading. So, if this function is called while document is still > > loading, m_resourceTimingInfo->finalResponse() below would return an > > uninitialized (default-constructed) value. This is clearly not correct. > > > > PerformanceBase::allowsTimingRedirect() only needs the url and headers of > final > > response. Could we get these information at PerformanceNavigationTiming > > construction time? > If we can get the final response at construction time, that would be ideal. But > the nature of this change is to create PerformanceNavigationTiming instance as > soon as it is requested which can be as early as Performance instance becomes > available. And we have no control over when exactly it will be constructed. But > good news is that when final response is an empty object, > PerformanceBase::allowsTimingRedirect allows returns false. So the return value > is actually predictable. It's false before final response arrives, and becomes > whatever it should be and stay the same after it arrives. Can we add an explicit check here with a comment: if the document has not finished loading return false explicitly, instead of depending on lower layers to do something reasonable. https://codereview.chromium.org/2647643004/diff/420001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/timing/Performance.cpp (right): https://codereview.chromium.org/2647643004/diff/420001/third_party/WebKit/Sou... third_party/WebKit/Source/core/timing/Performance.cpp:146: if (frame()) { On 2017/02/15 20:30:28, sunjian wrote: > On 2017/02/15 06:35:30, Kunihiko Sakamoto wrote: > > If frame() is null, can we return nullptr here? Currently we're returning > > PerformanceNavigationTiming(nullptr, nullptr, timeOrigin()), which isn't > useful > > at all. > > > > Then we can get rid of many null checks in PerformanceNavigationTiming. > > Agree with returning nullptr directly when frame() is nullptr. Not sur e we > can't get rid of null checks in PerformanceNavigationTiming. Because even though > PerformanceNavigationTiming can be constructed with a non-null frame, it might > become nullptr halfway right? LocalFrame is only a WeakMember of > PerformanceNavigationTiming. Which dchecks are being referred to here specifically?
On 2017/02/16 00:37:23, panicker wrote: > https://codereview.chromium.org/2647643004/diff/380001/third_party/WebKit/Sou... > File third_party/WebKit/Source/core/timing/PerformanceNavigationTiming.cpp > (right): > > https://codereview.chromium.org/2647643004/diff/380001/third_party/WebKit/Sou... > third_party/WebKit/Source/core/timing/PerformanceNavigationTiming.cpp:73: bool > PerformanceNavigationTiming::getAllowRedirectDetails() const { > On 2017/02/15 20:30:28, sunjian wrote: > > On 2017/02/15 06:35:30, Kunihiko Sakamoto wrote: > > > On 2017/02/14 21:29:03, sunjian wrote: > > > > On 2017/02/13 08:22:29, Kunihiko Sakamoto wrote: > > > > > Can you precompute this in the constructor and store it in a member > field? > > > > > > > > That would be ideal. But we don't know exactly when this is going to be > > > > triggered. We provide PerformanceNavigationTiming instance as early as > it's > > > > requested which can be as soon as Performance instance is available. > > > > Specifically, we rely on a final ResourceResponse instance in order to > > compute > > > > this flag. And we don't know when this happens. > > > > > > Does that mean this function could return different values depending on > > timings? > > > That's really weird. > > > > > > Looking at the code, ResourceTimingInfo::setFinalResponse() is called from > > > ResourceFetcher::handleLoaderFinish(), which is called when main document > has > > > finished loading. So, if this function is called while document is still > > > loading, m_resourceTimingInfo->finalResponse() below would return an > > > uninitialized (default-constructed) value. This is clearly not correct. > > > > > > PerformanceBase::allowsTimingRedirect() only needs the url and headers of > > final > > > response. Could we get these information at PerformanceNavigationTiming > > > construction time? > > If we can get the final response at construction time, that would be ideal. > But > > the nature of this change is to create PerformanceNavigationTiming instance as > > soon as it is requested which can be as early as Performance instance becomes > > available. And we have no control over when exactly it will be constructed. > But > > good news is that when final response is an empty object, > > PerformanceBase::allowsTimingRedirect allows returns false. So the return > value > > is actually predictable. It's false before final response arrives, and becomes > > whatever it should be and stay the same after it arrives. > > Can we add an explicit check here with a comment: if the document has not > finished loading return false explicitly, instead of depending on lower layers > to do something reasonable. > > https://codereview.chromium.org/2647643004/diff/420001/third_party/WebKit/Sou... > File third_party/WebKit/Source/core/timing/Performance.cpp (right): > > https://codereview.chromium.org/2647643004/diff/420001/third_party/WebKit/Sou... > third_party/WebKit/Source/core/timing/Performance.cpp:146: if (frame()) { > On 2017/02/15 20:30:28, sunjian wrote: > > On 2017/02/15 06:35:30, Kunihiko Sakamoto wrote: > > > If frame() is null, can we return nullptr here? Currently we're returning > > > PerformanceNavigationTiming(nullptr, nullptr, timeOrigin()), which isn't > > useful > > > at all. > > > > > > Then we can get rid of many null checks in PerformanceNavigationTiming. > > > > Agree with returning nullptr directly when frame() is nullptr. Not sur e we > > can't get rid of null checks in PerformanceNavigationTiming. Because even > though > > PerformanceNavigationTiming can be constructed with a non-null frame, it might > > become nullptr halfway right? LocalFrame is only a WeakMember of > > PerformanceNavigationTiming. > > Which dchecks are being referred to here specifically? I think he was talking about getting rid of null checks not DCHECKs.
Can we have a layout test for the new behavior? https://codereview.chromium.org/2647643004/diff/380001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/timing/PerformanceNavigationTiming.cpp (right): https://codereview.chromium.org/2647643004/diff/380001/third_party/WebKit/Sou... third_party/WebKit/Source/core/timing/PerformanceNavigationTiming.cpp:20: ResourceTimingInfo* info, On 2017/02/15 20:30:28, sunjian wrote: > On 2017/02/15 06:35:30, Kunihiko Sakamoto wrote: > > On 2017/02/14 21:29:03, sunjian wrote: > > > On 2017/02/13 08:22:29, Kunihiko Sakamoto wrote: > > > > Since this constructor takes ownership of the ResourceTimingInfo, the > > argument > > > > should be a PassRefPtr<ResourceTimingInfo>. > > > I don't want PerformanceNavigationTiming to own ResourceTimingInfo alone. I > > > still want ResourceFetcher owning it at the same time. > > > > But once ResourceFetcher has gone, PerformanceNavigationTiming actually > becomes > > the owner, right? > > > > I'm not sure whether we have clear style guideline for such shared-ownership > > case. Perhaps haraken@ might know? > > Doesn't PerformanceNavigationTiming become the owner automatically? > RecourceTimingInfo will get ref()ed again when PerformanceNavigationTiming gets > constructed. If ResourceFetcher dies, ResourceTimingInfo gets deref()ed, but > ResourceTimingInfo should still be alive after that point right? Yeah ResourceTimingInfo* parameter will work, this is purely a style matter. I'll defer this to OWNERS reviewer. https://codereview.chromium.org/2647643004/diff/380001/third_party/WebKit/Sou... third_party/WebKit/Source/core/timing/PerformanceNavigationTiming.cpp:73: bool PerformanceNavigationTiming::getAllowRedirectDetails() const { On 2017/02/16 00:37:23, panicker wrote: > On 2017/02/15 20:30:28, sunjian wrote: > > On 2017/02/15 06:35:30, Kunihiko Sakamoto wrote: > > > On 2017/02/14 21:29:03, sunjian wrote: > > > > On 2017/02/13 08:22:29, Kunihiko Sakamoto wrote: > > > > > Can you precompute this in the constructor and store it in a member > field? > > > > > > > > That would be ideal. But we don't know exactly when this is going to be > > > > triggered. We provide PerformanceNavigationTiming instance as early as > it's > > > > requested which can be as soon as Performance instance is available. > > > > Specifically, we rely on a final ResourceResponse instance in order to > > compute > > > > this flag. And we don't know when this happens. > > > > > > Does that mean this function could return different values depending on > > timings? > > > That's really weird. > > > > > > Looking at the code, ResourceTimingInfo::setFinalResponse() is called from > > > ResourceFetcher::handleLoaderFinish(), which is called when main document > has > > > finished loading. So, if this function is called while document is still > > > loading, m_resourceTimingInfo->finalResponse() below would return an > > > uninitialized (default-constructed) value. This is clearly not correct. > > > > > > PerformanceBase::allowsTimingRedirect() only needs the url and headers of > > final > > > response. Could we get these information at PerformanceNavigationTiming > > > construction time? > > If we can get the final response at construction time, that would be ideal. > But > > the nature of this change is to create PerformanceNavigationTiming instance as > > soon as it is requested which can be as early as Performance instance becomes > > available. And we have no control over when exactly it will be constructed. > But > > good news is that when final response is an empty object, > > PerformanceBase::allowsTimingRedirect allows returns false. So the return > value > > is actually predictable. It's false before final response arrives, and becomes > > whatever it should be and stay the same after it arrives. > > Can we add an explicit check here with a comment: if the document has not > finished loading return false explicitly, instead of depending on lower layers > to do something reasonable. So unloadEventStart, unloadEventEnd, redirectCount, redirectStart, and redirectEnd will return zero until the main resource finishes loading... That sounds like a bug to me, because all the information needed to do the check should be available by the time JS can start running (and requesting PerformanceNavigationTiming instance). We can address that later. Could you file a bug and put a TODO comment here with the crbug link? For now, +1 for adding an explicit check. https://codereview.chromium.org/2647643004/diff/420001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/timing/Performance.cpp (right): https://codereview.chromium.org/2647643004/diff/420001/third_party/WebKit/Sou... third_party/WebKit/Source/core/timing/Performance.cpp:146: if (frame()) { On 2017/02/15 20:30:28, sunjian wrote: > On 2017/02/15 06:35:30, Kunihiko Sakamoto wrote: > > If frame() is null, can we return nullptr here? Currently we're returning > > PerformanceNavigationTiming(nullptr, nullptr, timeOrigin()), which isn't > useful > > at all. > > > > Then we can get rid of many null checks in PerformanceNavigationTiming. > > Agree with returning nullptr directly when frame() is nullptr. Not sur e we > can't get rid of null checks in PerformanceNavigationTiming. Because even though > PerformanceNavigationTiming can be constructed with a non-null frame, it might > become nullptr halfway right? LocalFrame is only a WeakMember of > PerformanceNavigationTiming. Ah I overlooked that m_frame is a weak pointer. Similarly, can we return a nullptr if documentLoader->getNavigationTimingInfo() is null? https://codereview.chromium.org/2647643004/diff/460001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/external/wpt/navigation-timing/nav2_test_open_blank_page.html (left): https://codereview.chromium.org/2647643004/diff/460001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/external/wpt/navigation-timing/nav2_test_open_blank_page.html:16: assert_equals(iframe_performance.getEntriesByType("navigation").length, 0, "Expected there is no navigation timing instance"); Blank pages and data:url now have navigation timing? Is this compatible with other browsers? https://codereview.chromium.org/2647643004/diff/460001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/timing/PerformanceNavigationTiming.cpp (right): https://codereview.chromium.org/2647643004/diff/460001/third_party/WebKit/Sou... third_party/WebKit/Source/core/timing/PerformanceNavigationTiming.cpp:99: ExecutionContext* PerformanceNavigationTiming::getExecutionContext() const { This function is used only once. Inline? https://codereview.chromium.org/2647643004/diff/460001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/timing/PerformanceResourceTiming.cpp (right): https://codereview.chromium.org/2647643004/diff/460001/third_party/WebKit/Sou... third_party/WebKit/Source/core/timing/PerformanceResourceTiming.cpp:106: AtomicString PerformanceResourceTiming::initiatorType() const { Let's make this a virtual function directly.
Patchset #9 (id:480001) has been deleted
Please take another look! https://codereview.chromium.org/2647643004/diff/380001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/timing/PerformanceNavigationTiming.cpp (right): https://codereview.chromium.org/2647643004/diff/380001/third_party/WebKit/Sou... third_party/WebKit/Source/core/timing/PerformanceNavigationTiming.cpp:20: ResourceTimingInfo* info, On 2017/02/16 08:42:29, Kunihiko Sakamoto wrote: > On 2017/02/15 20:30:28, sunjian wrote: > > On 2017/02/15 06:35:30, Kunihiko Sakamoto wrote: > > > On 2017/02/14 21:29:03, sunjian wrote: > > > > On 2017/02/13 08:22:29, Kunihiko Sakamoto wrote: > > > > > Since this constructor takes ownership of the ResourceTimingInfo, the > > > argument > > > > > should be a PassRefPtr<ResourceTimingInfo>. > > > > I don't want PerformanceNavigationTiming to own ResourceTimingInfo alone. > I > > > > still want ResourceFetcher owning it at the same time. > > > > > > But once ResourceFetcher has gone, PerformanceNavigationTiming actually > > becomes > > > the owner, right? > > > > > > I'm not sure whether we have clear style guideline for such shared-ownership > > > case. Perhaps haraken@ might know? > > > > Doesn't PerformanceNavigationTiming become the owner automatically? > > RecourceTimingInfo will get ref()ed again when PerformanceNavigationTiming > gets > > constructed. If ResourceFetcher dies, ResourceTimingInfo gets deref()ed, but > > ResourceTimingInfo should still be alive after that point right? > > Yeah ResourceTimingInfo* parameter will work, this is purely a style matter. > I'll defer this to OWNERS reviewer. Acknowledged. https://codereview.chromium.org/2647643004/diff/380001/third_party/WebKit/Sou... third_party/WebKit/Source/core/timing/PerformanceNavigationTiming.cpp:73: bool PerformanceNavigationTiming::getAllowRedirectDetails() const { On 2017/02/16 00:37:23, panicker wrote: > On 2017/02/15 20:30:28, sunjian wrote: > > On 2017/02/15 06:35:30, Kunihiko Sakamoto wrote: > > > On 2017/02/14 21:29:03, sunjian wrote: > > > > On 2017/02/13 08:22:29, Kunihiko Sakamoto wrote: > > > > > Can you precompute this in the constructor and store it in a member > field? > > > > > > > > That would be ideal. But we don't know exactly when this is going to be > > > > triggered. We provide PerformanceNavigationTiming instance as early as > it's > > > > requested which can be as soon as Performance instance is available. > > > > Specifically, we rely on a final ResourceResponse instance in order to > > compute > > > > this flag. And we don't know when this happens. > > > > > > Does that mean this function could return different values depending on > > timings? > > > That's really weird. > > > > > > Looking at the code, ResourceTimingInfo::setFinalResponse() is called from > > > ResourceFetcher::handleLoaderFinish(), which is called when main document > has > > > finished loading. So, if this function is called while document is still > > > loading, m_resourceTimingInfo->finalResponse() below would return an > > > uninitialized (default-constructed) value. This is clearly not correct. > > > > > > PerformanceBase::allowsTimingRedirect() only needs the url and headers of > > final > > > response. Could we get these information at PerformanceNavigationTiming > > > construction time? > > If we can get the final response at construction time, that would be ideal. > But > > the nature of this change is to create PerformanceNavigationTiming instance as > > soon as it is requested which can be as early as Performance instance becomes > > available. And we have no control over when exactly it will be constructed. > But > > good news is that when final response is an empty object, > > PerformanceBase::allowsTimingRedirect allows returns false. So the return > value > > is actually predictable. It's false before final response arrives, and becomes > > whatever it should be and stay the same after it arrives. > > Can we add an explicit check here with a comment: if the document has not > finished loading return false explicitly, instead of depending on lower layers > to do something reasonable. I don't think there is a reasonable way to tell if the load has finished or not. It doesn't return a pointer. It returns a default ResourceResponse object. It seems to me checking whether what gets returned is a default or finalized object is kind of hacky. https://codereview.chromium.org/2647643004/diff/380001/third_party/WebKit/Sou... third_party/WebKit/Source/core/timing/PerformanceNavigationTiming.cpp:73: bool PerformanceNavigationTiming::getAllowRedirectDetails() const { On 2017/02/16 08:42:29, Kunihiko Sakamoto wrote: > On 2017/02/16 00:37:23, panicker wrote: > > On 2017/02/15 20:30:28, sunjian wrote: > > > On 2017/02/15 06:35:30, Kunihiko Sakamoto wrote: > > > > On 2017/02/14 21:29:03, sunjian wrote: > > > > > On 2017/02/13 08:22:29, Kunihiko Sakamoto wrote: > > > > > > Can you precompute this in the constructor and store it in a member > > field? > > > > > > > > > > That would be ideal. But we don't know exactly when this is going to be > > > > > triggered. We provide PerformanceNavigationTiming instance as early as > > it's > > > > > requested which can be as soon as Performance instance is available. > > > > > Specifically, we rely on a final ResourceResponse instance in order to > > > compute > > > > > this flag. And we don't know when this happens. > > > > > > > > Does that mean this function could return different values depending on > > > timings? > > > > That's really weird. > > > > > > > > Looking at the code, ResourceTimingInfo::setFinalResponse() is called from > > > > ResourceFetcher::handleLoaderFinish(), which is called when main document > > has > > > > finished loading. So, if this function is called while document is still > > > > loading, m_resourceTimingInfo->finalResponse() below would return an > > > > uninitialized (default-constructed) value. This is clearly not correct. > > > > > > > > PerformanceBase::allowsTimingRedirect() only needs the url and headers of > > > final > > > > response. Could we get these information at PerformanceNavigationTiming > > > > construction time? > > > If we can get the final response at construction time, that would be ideal. > > But > > > the nature of this change is to create PerformanceNavigationTiming instance > as > > > soon as it is requested which can be as early as Performance instance > becomes > > > available. And we have no control over when exactly it will be constructed. > > But > > > good news is that when final response is an empty object, > > > PerformanceBase::allowsTimingRedirect allows returns false. So the return > > value > > > is actually predictable. It's false before final response arrives, and > becomes > > > whatever it should be and stay the same after it arrives. > > > > Can we add an explicit check here with a comment: if the document has not > > finished loading return false explicitly, instead of depending on lower layers > > to do something reasonable. > > So unloadEventStart, unloadEventEnd, redirectCount, redirectStart, and > redirectEnd will return zero until the main resource finishes loading... That > sounds like a bug to me, because all the information needed to do the check > should be available by the time JS can start running (and requesting > PerformanceNavigationTiming instance). > > We can address that later. Could you file a bug and put a TODO comment here with > the crbug link? > > For now, +1 for adding an explicit check. As far as i know, NT1 kind of has the same problem. The timing allow flag also keeps changing as time goes on. It seems more like a dilemma than a bug to me. If we want to expose NavigationTiming instance as early as possible, which means loading might not even finish yet, we have to rely on the current state of the loading. For instance, there is no way for you to know whether this is one redirect that is cross-origin or not until loading finishes. I can leave a TODO here for sure. But i think it should be a good idea to bring the people who speced this into discussion. And for explicit checking, do you have any suggestions? I can't check whether it's a nullptr or not. It returns a default-constructed object. https://codereview.chromium.org/2647643004/diff/420001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/timing/Performance.cpp (right): https://codereview.chromium.org/2647643004/diff/420001/third_party/WebKit/Sou... third_party/WebKit/Source/core/timing/Performance.cpp:146: if (frame()) { On 2017/02/16 08:42:29, Kunihiko Sakamoto wrote: > On 2017/02/15 20:30:28, sunjian wrote: > > On 2017/02/15 06:35:30, Kunihiko Sakamoto wrote: > > > If frame() is null, can we return nullptr here? Currently we're returning > > > PerformanceNavigationTiming(nullptr, nullptr, timeOrigin()), which isn't > > useful > > > at all. > > > > > > Then we can get rid of many null checks in PerformanceNavigationTiming. > > > > Agree with returning nullptr directly when frame() is nullptr. Not sur e we > > can't get rid of null checks in PerformanceNavigationTiming. Because even > though > > PerformanceNavigationTiming can be constructed with a non-null frame, it might > > become nullptr halfway right? LocalFrame is only a WeakMember of > > PerformanceNavigationTiming. > > Ah I overlooked that m_frame is a weak pointer. > > Similarly, can we return a nullptr if documentLoader->getNavigationTimingInfo() > is null? Acknowledged. https://codereview.chromium.org/2647643004/diff/460001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/external/wpt/navigation-timing/nav2_test_open_blank_page.html (left): https://codereview.chromium.org/2647643004/diff/460001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/external/wpt/navigation-timing/nav2_test_open_blank_page.html:16: assert_equals(iframe_performance.getEntriesByType("navigation").length, 0, "Expected there is no navigation timing instance"); On 2017/02/16 08:42:29, Kunihiko Sakamoto wrote: > Blank pages and data:url now have navigation timing? Is this compatible with > other browsers? I only know that NT1 does expose navigation timing in blank pages and supposedly also in data:urls. I added these two tests in the wpt repository. They are not from other browsers. The reason we decided not to expose navigation timing before is because there was a checking for ResourceLoadTiming instance before constructing a navigation timing instance. If ResourceLoadTiming doesn't exist, we don't expose it. I think somebody suggested not to expose nav timing instance when ResourceLoadingTiming doesn't exist, and i just followed the comment. But now it is a different story, we can not determine if we are visiting a blank page or a data:url basing on ResourceLoadTiming being nullptr because it can be nullptr at an early stage even when visiting regular http sites, right? https://codereview.chromium.org/2647643004/diff/460001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/timing/PerformanceNavigationTiming.cpp (right): https://codereview.chromium.org/2647643004/diff/460001/third_party/WebKit/Sou... third_party/WebKit/Source/core/timing/PerformanceNavigationTiming.cpp:99: ExecutionContext* PerformanceNavigationTiming::getExecutionContext() const { On 2017/02/16 08:42:29, Kunihiko Sakamoto wrote: > This function is used only once. Inline? Done. https://codereview.chromium.org/2647643004/diff/460001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/timing/PerformanceResourceTiming.cpp (right): https://codereview.chromium.org/2647643004/diff/460001/third_party/WebKit/Sou... third_party/WebKit/Source/core/timing/PerformanceResourceTiming.cpp:106: AtomicString PerformanceResourceTiming::initiatorType() const { On 2017/02/16 08:42:30, Kunihiko Sakamoto wrote: > Let's make this a virtual function directly. Done.
I realized this is much trickier than I initially thought. Let's think about the domainLookupStart attribute. PerformanceNavigationTiming::resourceLoadTiming() returns nullptr until the final response arrives, so PerformanceResourceTiming::domainLookupStart() falls back to fetchStart(), which can return non-zero value even if main document is loading. Once the final response is set, domainLookupStart suddenly changes its value, returning the "correct" timing. Similar things will happen to other attributes that use ResourceLoadTiming. What if we allow creating PerformanceNavigationTiming only when ResourceTimingInfo has final response? This would resolve the getAllowRedirectDetails() non-determinism and the blank / data-uri issue as well. From the web-developers perspective, this (roughly) means that navigation timing is not available before the DOMContentLoaded event. I think this is better than returning NT with unreliable values. What do you think?
On 2017/02/17 03:59:02, Kunihiko Sakamoto wrote: > I realized this is much trickier than I initially thought. > > Let's think about the domainLookupStart attribute. > PerformanceNavigationTiming::resourceLoadTiming() returns nullptr until the > final response arrives, so PerformanceResourceTiming::domainLookupStart() falls > back to fetchStart(), which can return non-zero value even if main document is > loading. Once the final response is set, domainLookupStart suddenly changes its > value, returning the "correct" timing. Similar things will happen to other > attributes that use ResourceLoadTiming. > > What if we allow creating PerformanceNavigationTiming only when > ResourceTimingInfo has final response? This would resolve the > getAllowRedirectDetails() non-determinism and the blank / data-uri issue as > well. > > From the web-developers perspective, this (roughly) means that navigation timing > is not available before the DOMContentLoaded event. I think this is better than > returning NT with unreliable values. > > What do you think? I think not returning navigation entry until DOMContentLoaded will not address the original spec (data bias) issue that the CL is attempting to fix: https://github.com/w3c/navigation-timing/issues/50 I asked JIan to look into adding an explicit setter / getter to deal with returning reasonable results before final response is available. Would that address your concern? In general the expectation is that the Navigation timing entry will not have certain fields (return default values) until the notifyObservers is called. If you think certain fields are special and must not change - let us know and let's discuss on the spec bug.
On 2017/02/17 17:08:34, panicker wrote: > On 2017/02/17 03:59:02, Kunihiko Sakamoto wrote: > > I realized this is much trickier than I initially thought. > > > > Let's think about the domainLookupStart attribute. > > PerformanceNavigationTiming::resourceLoadTiming() returns nullptr until the > > final response arrives, so PerformanceResourceTiming::domainLookupStart() > falls > > back to fetchStart(), which can return non-zero value even if main document is > > loading. Once the final response is set, domainLookupStart suddenly changes > its > > value, returning the "correct" timing. Similar things will happen to other > > attributes that use ResourceLoadTiming. > > > > What if we allow creating PerformanceNavigationTiming only when > > ResourceTimingInfo has final response? This would resolve the > > getAllowRedirectDetails() non-determinism and the blank / data-uri issue as > > well. > > > > From the web-developers perspective, this (roughly) means that navigation > timing > > is not available before the DOMContentLoaded event. I think this is better > than > > returning NT with unreliable values. > > > > What do you think? > > I think not returning navigation entry until DOMContentLoaded will not address > the original spec (data bias) issue that the CL is attempting to fix: > https://github.com/w3c/navigation-timing/issues/50 At least Bryan may be happy with my suggestion, as his two examples both use DCL :) > I asked JIan to look into adding an explicit setter / getter to deal with > returning reasonable results before final response is available. > Would that address your concern? My concern is that after this change: (a) Zero value of Nav Timing attributes can mean "not applicable" (e.g. redirectStart when there's no redirect), *or* the value is not available yet. (b) Some values aren't populated timely, e.g. domainLookupStart will not be populated until final response is available, although domain lookup must be finished long before that. I'm afraid these would make it hard to use NT API correctly. If developers misuse the API, reported data would be no better than biased data... Note that fixing (b) completely would require considerable amount of plumbing from net to blink, and might cause performance regression by increased IPC traffic. > In general the expectation is that the Navigation timing entry will not have > certain fields (return default values) until the notifyObservers is called. Would it be a bad idea to return null for these not-available-yet fields, instead of zero? > If you think certain fields are special and must not change - let us know and > let's discuss on the spec bug. Yeah probably we should move the discussion to the spec bug.
On 2017/02/21 03:48:18, Kunihiko Sakamoto wrote: > On 2017/02/17 17:08:34, panicker wrote: > > On 2017/02/17 03:59:02, Kunihiko Sakamoto wrote: > > > I realized this is much trickier than I initially thought. > > > > > > Let's think about the domainLookupStart attribute. > > > PerformanceNavigationTiming::resourceLoadTiming() returns nullptr until the > > > final response arrives, so PerformanceResourceTiming::domainLookupStart() > > falls > > > back to fetchStart(), which can return non-zero value even if main document > is > > > loading. Once the final response is set, domainLookupStart suddenly changes > > its > > > value, returning the "correct" timing. Similar things will happen to other > > > attributes that use ResourceLoadTiming. > > > > > > What if we allow creating PerformanceNavigationTiming only when > > > ResourceTimingInfo has final response? This would resolve the > > > getAllowRedirectDetails() non-determinism and the blank / data-uri issue as > > > well. > > > > > > From the web-developers perspective, this (roughly) means that navigation > > timing > > > is not available before the DOMContentLoaded event. I think this is better > > than > > > returning NT with unreliable values. > > > > > > What do you think? > > > > I think not returning navigation entry until DOMContentLoaded will not address > > the original spec (data bias) issue that the CL is attempting to fix: > > https://github.com/w3c/navigation-timing/issues/50 > > At least Bryan may be happy with my suggestion, as his two examples both use DCL > :) > > > I asked JIan to look into adding an explicit setter / getter to deal with > > returning reasonable results before final response is available. > > Would that address your concern? > > My concern is that after this change: > > (a) Zero value of Nav Timing attributes can mean "not applicable" (e.g. > redirectStart when there's no redirect), *or* the value is not available yet. > (b) Some values aren't populated timely, e.g. domainLookupStart will not be > populated until final response is available, although domain lookup must be > finished long before that. > > I'm afraid these would make it hard to use NT API correctly. If developers > misuse the API, reported data would be no better than biased data... > > Note that fixing (b) completely would require considerable amount of plumbing > from net to blink, and might cause performance regression by increased IPC > traffic. > > > In general the expectation is that the Navigation timing entry will not have > > certain fields (return default values) until the notifyObservers is called. > > Would it be a bad idea to return null for these not-available-yet fields, > instead of zero? > > > If you think certain fields are special and must not change - let us know and > > let's discuss on the spec bug. > > Yeah probably we should move the discussion to the spec bug. Kinuhiko, Please see last two comments in spec bug and chime in: https://github.com/w3c/navigation-timing/issues/50#event-976859794 Thanks!
OK, let's proceed with current approach. LGTM with a final comment addressed. Thanks for your patience! https://codereview.chromium.org/2647643004/diff/500001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/http/tests/performance-timing/nav2-test-instance-accessible-from-the-start.html (right): https://codereview.chromium.org/2647643004/diff/500001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/http/tests/performance-timing/nav2-test-instance-accessible-from-the-start.html:6: assert_true(navigationTiming2 !== undefined); Please write assertions inside test(). https://github.com/w3c/testharness.js/blob/master/docs/api.md Also, could we test that some fields have reasonable values?
Comments addressed! Please take another look! https://codereview.chromium.org/2647643004/diff/500001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/http/tests/performance-timing/nav2-test-instance-accessible-from-the-start.html (right): https://codereview.chromium.org/2647643004/diff/500001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/http/tests/performance-timing/nav2-test-instance-accessible-from-the-start.html:6: assert_true(navigationTiming2 !== undefined); On 2017/03/10 08:19:00, Kunihiko Sakamoto wrote: > Please write assertions inside test(). > https://github.com/w3c/testharness.js/blob/master/docs/api.md > > Also, could we test that some fields have reasonable values? Done.
LGTM. Kentaro, could you please review for owners? Thanks. https://codereview.chromium.org/2647643004/diff/520001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/http/tests/performance-timing/nav2-test-instance-accessible-from-the-start.html (right): https://codereview.chromium.org/2647643004/diff/520001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/http/tests/performance-timing/nav2-test-instance-accessible-from-the-start.html:4: <script> move this under web-platform-tests in external/wpt/ ?
The change looks good overall but it looks like that this CL is doing multiple things in one go. Would it be possible to split the CLs into a couple of pieces?
On 2017/03/13 14:34:15, haraken wrote: > The change looks good overall but it looks like that this CL is doing multiple > things in one go. Would it be possible to split the CLs into a couple of pieces? I think it makes sense to split the changes in ResourceTimingInfo.* into a separate cl. The changes in ResourceTimingInfo.* is about making ResourceTimingInfo ref-counted. But if i split it, then the larger change has to wait till the small one gets landed. +panicker. What do you think?
https://codereview.chromium.org/2647643004/diff/520001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/http/tests/performance-timing/nav2-test-instance-accessible-from-the-start.html (right): https://codereview.chromium.org/2647643004/diff/520001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/http/tests/performance-timing/nav2-test-instance-accessible-from-the-start.html:4: <script> On 2017/03/10 21:46:36, panicker wrote: > move this under web-platform-tests in external/wpt/ ? I thought of doing this. But i am not sure whether this feature will apply to other browsers. Let me know if you think it's okay to move it to external/wpt/.
On 2017/03/13 19:08:05, sunjian wrote: > On 2017/03/13 14:34:15, haraken wrote: > > The change looks good overall but it looks like that this CL is doing multiple > > things in one go. Would it be possible to split the CLs into a couple of > pieces? > > I think it makes sense to split the changes in ResourceTimingInfo.* into a > separate cl. The changes in ResourceTimingInfo.* is about making > ResourceTimingInfo ref-counted. > But if i split it, then the larger change has to wait till the small one gets > landed. +panicker. What do you think? Yes I agree. Let's split into 3 CLs as discussed.
https://codereview.chromium.org/2647643004/diff/520001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/http/tests/performance-timing/nav2-test-instance-accessible-from-the-start.html (right): https://codereview.chromium.org/2647643004/diff/520001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/http/tests/performance-timing/nav2-test-instance-accessible-from-the-start.html:4: <script> On 2017/03/13 19:08:18, sunjian wrote: > On 2017/03/10 21:46:36, panicker wrote: > > move this under web-platform-tests in external/wpt/ ? > > I thought of doing this. But i am not sure whether this feature will apply to > other browsers. > Let me know if you think it's okay to move it to external/wpt/. Yes it's fine to move to external/wpt/ as this is part of the spec for NT2.
https://codereview.chromium.org/2647643004/diff/520001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/http/tests/performance-timing/nav2-test-instance-accessible-from-the-start.html (right): https://codereview.chromium.org/2647643004/diff/520001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/http/tests/performance-timing/nav2-test-instance-accessible-from-the-start.html:4: <script> On 2017/03/13 20:42:40, panicker wrote: > On 2017/03/13 19:08:18, sunjian wrote: > > On 2017/03/10 21:46:36, panicker wrote: > > > move this under web-platform-tests in external/wpt/ ? > > > > I thought of doing this. But i am not sure whether this feature will apply to > > other browsers. > > Let me know if you think it's okay to move it to external/wpt/. > > Yes it's fine to move to external/wpt/ as this is part of the spec for NT2. Done.
You should rebase it with ToT and remove the RefPtr change from the CL. https://codereview.chromium.org/2647643004/diff/540001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/timing/PerformanceNavigationTiming.h (right): https://codereview.chromium.org/2647643004/diff/540001/third_party/WebKit/Sou... third_party/WebKit/Source/core/timing/PerformanceNavigationTiming.h:81: WeakMember<LocalFrame> m_frame; I guess this should just be a Member. Is there any reason you need to make it weak? i.e., does PerformanceNavigationTiming outlive LocalFrame?
Addressed comments and got rid of the refactoring change for getNavigationType in PerformanceBase to make this cl even smaller. Will have a follow-up cl to add the refactoring again. +haranken, can you take another pass at this please? https://codereview.chromium.org/2647643004/diff/540001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/timing/PerformanceNavigationTiming.h (right): https://codereview.chromium.org/2647643004/diff/540001/third_party/WebKit/Sou... third_party/WebKit/Source/core/timing/PerformanceNavigationTiming.h:81: WeakMember<LocalFrame> m_frame; On 2017/03/14 20:10:40, haraken wrote: > > I guess this should just be a Member. > > Is there any reason you need to make it weak? i.e., does > PerformanceNavigationTiming outlive LocalFrame? I guess i wasn't taking who outlives whom into consideration. PerformanceNavigationTiming only references LocalFrame for information but not meant to be an owner of a LocalFrame which entails getting involved in the life-cycle of LocalFrame. Correct me if i'm wrong about this please!
LGTM Please add a more detailed explanation to the CL description. https://codereview.chromium.org/2647643004/diff/580001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/timing/PerformanceNavigationTiming.h (right): https://codereview.chromium.org/2647643004/diff/580001/third_party/WebKit/Sou... third_party/WebKit/Source/core/timing/PerformanceNavigationTiming.h:82: WeakMember<LocalFrame> m_frame; GC has a ability to correct a reference cycle, so you don't need to worry about adding a strong reference to the object. We should use WeakMember only when the pointing object outlives the pointed object.
Description was changed from ========== Report nav timing 2 instance as soon as it's requested. Doc: https://docs.google.com/a/chromium.org/document/d/1QX2zVcYpuFTsq0MEZH9J8ZUT4C... BUG=687398 ========== to ========== Report nav timing 2 instance as soon as it's requested. Currently NT2 instance is not available until loadEventEnd happens. However, this new feature contradicts with the way current clients use NT1 which is available as soon as it gets requested. In order to align with NT1's usage behavior, we need to also make NT2 available as soon as clients request for it. Doc: https://docs.google.com/a/chromium.org/document/d/1QX2zVcYpuFTsq0MEZH9J8ZUT4C... BUG=687398 ==========
On 2017/03/15 08:47:22, haraken wrote: > LGTM > > Please add a more detailed explanation to the CL description. > > https://codereview.chromium.org/2647643004/diff/580001/third_party/WebKit/Sou... > File third_party/WebKit/Source/core/timing/PerformanceNavigationTiming.h > (right): > > https://codereview.chromium.org/2647643004/diff/580001/third_party/WebKit/Sou... > third_party/WebKit/Source/core/timing/PerformanceNavigationTiming.h:82: > WeakMember<LocalFrame> m_frame; > > GC has a ability to correct a reference cycle, so you don't need to worry about > adding a strong reference to the object. > > We should use WeakMember only when the pointing object outlives the pointed > object. SGTM!
The CQ bit was checked by sunjian@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from ksakamoto@chromium.org, panicker@chromium.org, haraken@chromium.org Link to the patchset: https://codereview.chromium.org/2647643004/#ps600001 (title: "WeakMemeber<LocalFrame> to Member<LocalFrame>")
The CQ bit was unchecked by sunjian@chromium.org
The CQ bit was checked by sunjian@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from haraken@chromium.org, ksakamoto@chromium.org, panicker@chromium.org Link to the patchset: https://codereview.chromium.org/2647643004/#ps620001 (title: "sync")
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: android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...) mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
Patchset #15 (id:620001) has been deleted
Patchset #14 (id:600001) has been deleted
The CQ bit was checked by sunjian@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from haraken@chromium.org, ksakamoto@chromium.org, panicker@chromium.org Link to the patchset: https://codereview.chromium.org/2647643004/#ps640001 (title: "WeakMemeber<LocalFrame> to Member<LocalFrame>")
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: linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
Patchset #14 (id:640001) has been deleted
The CQ bit was checked by sunjian@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from haraken@chromium.org, ksakamoto@chromium.org, panicker@chromium.org Link to the patchset: https://codereview.chromium.org/2647643004/#ps660001 (title: "WeakMemeber<LocalFrame> to Member<LocalFrame>")
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: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
On 2017/03/16 01:37:15, commit-bot: I haz the power wrote: > 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_...) +haraken. Hi haraken, i think this failure is about GC not being able to collect iframe? Should we change it back to WeakMember?
On 2017/03/16 17:23:00, sunjian wrote: > On 2017/03/16 01:37:15, commit-bot: I haz the power wrote: > > 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_...) > > +haraken. Hi haraken, i think this failure is about GC not being able to collect > iframe? Should we change it back to WeakMember? Yeah, it seems related. I'm a bit afraid that PerformanceNavigationTiming is leaking (for some reason). Would you try to change it to WeakMember and manually confirm that PerformanceNavigationTiming gets collected in those two tests (by inserting printf to constructor and destructor)? If PerformanceNavigationTiming is leaking, we need to fix it.
On 2017/03/16 18:10:26, haraken wrote: > On 2017/03/16 17:23:00, sunjian wrote: > > On 2017/03/16 01:37:15, commit-bot: I haz the power wrote: > > > 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_...) > > > > +haraken. Hi haraken, i think this failure is about GC not being able to > collect > > iframe? Should we change it back to WeakMember? > > Yeah, it seems related. > > I'm a bit afraid that PerformanceNavigationTiming is leaking (for some reason). > Would you try to change it to WeakMember and manually confirm that > PerformanceNavigationTiming gets collected in those two tests (by inserting > printf to constructor and destructor)? If PerformanceNavigationTiming is > leaking, we need to fix it. Just confirmed, with WeakMember it works. And PNT's destructor did get called, so no leak. PNT may outlive LocalFrame or both of them live equally long.
On 2017/03/16 18:27:50, sunjian wrote: > On 2017/03/16 18:10:26, haraken wrote: > > On 2017/03/16 17:23:00, sunjian wrote: > > > On 2017/03/16 01:37:15, commit-bot: I haz the power wrote: > > > > 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_...) > > > > > > +haraken. Hi haraken, i think this failure is about GC not being able to > > collect > > > iframe? Should we change it back to WeakMember? > > > > Yeah, it seems related. > > > > I'm a bit afraid that PerformanceNavigationTiming is leaking (for some > reason). > > Would you try to change it to WeakMember and manually confirm that > > PerformanceNavigationTiming gets collected in those two tests (by inserting > > printf to constructor and destructor)? If PerformanceNavigationTiming is > > leaking, we need to fix it. > > Just confirmed, with WeakMember it works. And PNT's destructor did get called, > so no leak. PNT may outlive LocalFrame or both of them live equally long. Shall i go ahead and land it with WeakMember?
haraken@chromium.org changed reviewers: + sigbjornf@opera.com
Hmm, I looked at the code and don't understand why it needs to be weak, but yeah, it would not be a good idea to block your CL on it. Would you add a TODO to investigate it? LGTM. Also I don't understand why message-port-gc-closed.html creates the PerformanceNavigationTiming object in the first place though. +sigbjorn FYI
On 2017/03/17 12:07:20, haraken wrote: > Hmm, I looked at the code and don't understand why it needs to be weak, but > yeah, it would not be a good idea to block your CL on it. Would you add a TODO > to investigate it? LGTM. > > Also I don't understand why message-port-gc-closed.html creates the > PerformanceNavigationTiming object in the first place though. > > +sigbjorn FYI I wonder if it is because we don't clear a LocalDOMWindow's supplements upon frameDestroyed() that there's an extra lag here.. but a guess. It'd be worth understanding.
On 2017/03/17 12:07:20, haraken wrote: > Hmm, I looked at the code and don't understand why it needs to be weak, but > yeah, it would not be a good idea to block your CL on it. Would you add a TODO > to investigate it? LGTM. > > Also I don't understand why message-port-gc-closed.html creates the > PerformanceNavigationTiming object in the first place though. > > +sigbjorn FYI PNT will always get created as long as loadEvent* happens.
Patchset #15 (id:680001) has been deleted
The CQ bit was checked by sunjian@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from ksakamoto@chromium.org, panicker@chromium.org, haraken@chromium.org Link to the patchset: https://codereview.chromium.org/2647643004/#ps700001 (title: "Memebr<LocalFrame>-->WeakMember<LocalFrame>")
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 sunjian@chromium.org
The CQ bit was checked by sunjian@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from haraken@chromium.org, ksakamoto@chromium.org, panicker@chromium.org Link to the patchset: https://codereview.chromium.org/2647643004/#ps720001 (title: "add TODO")
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": 720001, "attempt_start_ts": 1489776007080900,
"parent_rev": "f00e8b48a63cdac2e5d21fe63581bb488c3a355f", "commit_rev":
"36d0286404018e441692f66d1289f33ff33a05d1"}
Message was sent while issue was closed.
Description was changed from ========== Report nav timing 2 instance as soon as it's requested. Currently NT2 instance is not available until loadEventEnd happens. However, this new feature contradicts with the way current clients use NT1 which is available as soon as it gets requested. In order to align with NT1's usage behavior, we need to also make NT2 available as soon as clients request for it. Doc: https://docs.google.com/a/chromium.org/document/d/1QX2zVcYpuFTsq0MEZH9J8ZUT4C... BUG=687398 ========== to ========== Report nav timing 2 instance as soon as it's requested. Currently NT2 instance is not available until loadEventEnd happens. However, this new feature contradicts with the way current clients use NT1 which is available as soon as it gets requested. In order to align with NT1's usage behavior, we need to also make NT2 available as soon as clients request for it. Doc: https://docs.google.com/a/chromium.org/document/d/1QX2zVcYpuFTsq0MEZH9J8ZUT4C... BUG=687398 Review-Url: https://codereview.chromium.org/2647643004 Cr-Commit-Position: refs/heads/master@{#457871} Committed: https://chromium.googlesource.com/chromium/src/+/36d0286404018e441692f66d1289... ==========
Message was sent while issue was closed.
Committed patchset #16 (id:720001) as https://chromium.googlesource.com/chromium/src/+/36d0286404018e441692f66d1289... |
