Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2511313002:
transferSize implementation (Closed)
DescriptionTransferSize implementation.
BUG=663187
Committed: https://crrev.com/aa066fd6ef4f2546faafdaf3abcb328fc6cc709c
Cr-Commit-Position: refs/heads/master@{#437946}
Patch Set 1 #
Total comments: 4
Patch Set 2 : addressed the issue of m_mainResource's identifier being 0 in DocumentLoader.h #Patch Set 3 : add nullptr checking against m_mainResource #
Total comments: 2
Patch Set 4 : minor changes #
Total comments: 2
Patch Set 5 : added TODO #
Total comments: 8
Patch Set 6 : addressed comments #
Total comments: 4
Patch Set 7 : addressed comments #
Total comments: 15
Patch Set 8 : addressed comments #
Total comments: 2
Patch Set 9 : addressed comments #
Total comments: 2
Patch Set 10 : addressed comments #Patch Set 11 : added unit tests #Patch Set 12 : Got rid of DCHECKS cause they don't stand any more #
Total comments: 4
Patch Set 13 : addressed comments #
Total comments: 6
Patch Set 14 : fixed unit tests DCHECK failures #Patch Set 15 : report iframes' navigation timing info #
Total comments: 7
Patch Set 16 : addressed comments #
Total comments: 2
Patch Set 17 : addressed comments #
Total comments: 6
Patch Set 18 : addressed comments #
Dependent Patchsets: Messages
Total messages: 101 (49 generated)
Description was changed from ========== transferSize implementation BUG=663187 ========== to ========== TransferSize implementation. This version is not a final version of the implementation. Hoping to get some feedback and validation from experts before finalizing it. BUG=663187 ==========
sunjian@chromium.org changed reviewers: + panicker@google.com, yoav@yoav.ws
https://codereview.chromium.org/2511313002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/loader/DocumentLoader.cpp (right): https://codereview.chromium.org/2511313002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/loader/DocumentLoader.cpp:324: performance->addNavigationTiming(m_frame, getMainResourceTimingInfo()); I may be wrong but I don't think this is guaranteed to run after the window's load event ended. https://codereview.chromium.org/2511313002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/timing/PerformanceBase.cpp (left): https://codereview.chromium.org/2511313002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/timing/PerformanceBase.cpp:333: // TODO(sunjian) Implement transfer size. crbug/663187 You can remove the TODO
https://codereview.chromium.org/2511313002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/loader/DocumentLoader.cpp (right): https://codereview.chromium.org/2511313002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/loader/DocumentLoader.cpp:324: performance->addNavigationTiming(m_frame, getMainResourceTimingInfo()); On 2016/11/18 10:06:30, Yoav Weiss wrote: > I may be wrong but I don't think this is guaranteed to run after the window's > load event ended. You are right about this. I made a mistake recognizing this method finishing later cause the m_mainResource's identifier always being 0 when requested at nav timing instance construction. I found out that they reset m_mainResource when loading finished. Now i add a field to store the identifier to get around this. https://codereview.chromium.org/2511313002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/timing/PerformanceBase.cpp (left): https://codereview.chromium.org/2511313002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/timing/PerformanceBase.cpp:333: // TODO(sunjian) Implement transfer size. crbug/663187 On 2016/11/18 10:06:30, Yoav Weiss wrote: > You can remove the TODO Done.
panicker@chromium.org changed reviewers: + panicker@chromium.org - panicker@google.com
Looks reasonable to me. Yoav, WDYT? https://codereview.chromium.org/2511313002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/fetch/ResourceFetcher.h (right): https://codereview.chromium.org/2511313002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/fetch/ResourceFetcher.h:241: HashMap<unsigned long, std::unique_ptr<ResourceTimingInfo>>; HeapHashMap?
Patchset #4 (id:60001) has been deleted
Patchset #4 (id:80001) has been deleted
https://codereview.chromium.org/2511313002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/fetch/ResourceFetcher.h (right): https://codereview.chromium.org/2511313002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/fetch/ResourceFetcher.h:241: HashMap<unsigned long, std::unique_ptr<ResourceTimingInfo>>; On 2016/11/22 02:17:42, Shubhie wrote: > HeapHashMap? Talked offline. Agreed that no need to use a HeapHashMap in this case.
https://codereview.chromium.org/2511313002/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/fetch/ResourceFetcher.cpp (right): https://codereview.chromium.org/2511313002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/fetch/ResourceFetcher.cpp:1126: unsigned long identifier) { IIUC, this is only ever called with the mainResourceIdentifier(). Is that correct? Are there scenarios where the m_mainResourceTimingInfoMap will contain other resources?
https://codereview.chromium.org/2511313002/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/fetch/ResourceFetcher.cpp (right): https://codereview.chromium.org/2511313002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/fetch/ResourceFetcher.cpp:1126: unsigned long identifier) { On 2016/11/22 10:26:23, Yoav Weiss wrote: > IIUC, this is only ever called with the mainResourceIdentifier(). Is that > correct? > > Are there scenarios where the m_mainResourceTimingInfoMap will contain other > resources? Yes. For now it only gets called with mainResourceIdentifier(). As you can see from the changes in ResourceFetcher::storeResourceTimingInitiatorInformation(...), m_mainResourceTimingInfoMap definitely only contains main resources but not all of them. There are main resources that are captured by resource timing but they are not the ones that navigation timing cares about. It's also okay when ResourceFetcher::getMainResourceTimingInfo doesn't get called with mainResourceIdentifier() cause it will just return a nullptr if the identifier is not pointing to a main resource.
sunjian@chromium.org changed reviewers: + kinuko@chromium.org
panicker@chromium.org changed reviewers: + tyoshino@chromium.org
On 2016/11/22 22:25:44, sunjian wrote: > https://codereview.chromium.org/2511313002/diff/100001/third_party/WebKit/Sou... > File third_party/WebKit/Source/core/fetch/ResourceFetcher.cpp (right): > > https://codereview.chromium.org/2511313002/diff/100001/third_party/WebKit/Sou... > third_party/WebKit/Source/core/fetch/ResourceFetcher.cpp:1126: unsigned long > identifier) { > On 2016/11/22 10:26:23, Yoav Weiss wrote: > > IIUC, this is only ever called with the mainResourceIdentifier(). Is that > > correct? > > > > Are there scenarios where the m_mainResourceTimingInfoMap will contain other > > resources? > > Yes. For now it only gets called with mainResourceIdentifier(). > > As you can see from the changes in > ResourceFetcher::storeResourceTimingInitiatorInformation(...), > m_mainResourceTimingInfoMap definitely only contains main resources but not all > of them. There are main resources that are captured by resource timing but they > are not the ones that navigation timing cares about. > It's also okay when ResourceFetcher::getMainResourceTimingInfo doesn't get > called with mainResourceIdentifier() cause it will just return a nullptr if the > identifier is not pointing to a main resource. I guess I'm confused by why this needs to be a map rather than just a pointer. Can you clarify under which scenario this will contain more than one main resource? A comment to that effect might also be helpful.
On 2016/11/25 08:02:42, Yoav Weiss wrote: > On 2016/11/22 22:25:44, sunjian wrote: > > > https://codereview.chromium.org/2511313002/diff/100001/third_party/WebKit/Sou... > > File third_party/WebKit/Source/core/fetch/ResourceFetcher.cpp (right): > > > > > https://codereview.chromium.org/2511313002/diff/100001/third_party/WebKit/Sou... > > third_party/WebKit/Source/core/fetch/ResourceFetcher.cpp:1126: unsigned long > > identifier) { > > On 2016/11/22 10:26:23, Yoav Weiss wrote: > > > IIUC, this is only ever called with the mainResourceIdentifier(). Is that > > > correct? > > > > > > Are there scenarios where the m_mainResourceTimingInfoMap will contain other > > > resources? > > > > Yes. For now it only gets called with mainResourceIdentifier(). > > > > As you can see from the changes in > > ResourceFetcher::storeResourceTimingInitiatorInformation(...), > > m_mainResourceTimingInfoMap definitely only contains main resources but not > all > > of them. There are main resources that are captured by resource timing but > they > > are not the ones that navigation timing cares about. > > It's also okay when ResourceFetcher::getMainResourceTimingInfo doesn't get > > called with mainResourceIdentifier() cause it will just return a nullptr if > the > > identifier is not pointing to a main resource. > > I guess I'm confused by why this needs to be a map rather than just a pointer. > Can you clarify under which scenario this will contain more than one main > resource? > A comment to that effect might also be helpful. I see what you mean. I actually tried quite a few sites and had the program print out the size of the map and it seems that it is never greater than 1. But from the logic of the program per se, i can't be guaranteed that there will only be one instance that gets added to the map. Apparently, only main resources will be added to the map. But they also filter out part of them by having this context().updateTimingInfoForIFrameNavigation(info.get()) check. Map might be an overkill in here. But it's definite a safe way to doing it for now. WDYT?
Added a TODO and bug for the map issue Yoav mentioned.
https://codereview.chromium.org/2511313002/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/fetch/ResourceFetcher.cpp (right): https://codereview.chromium.org/2511313002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/fetch/ResourceFetcher.cpp:755: void ResourceFetcher::storeResourceTimingInitiatorInformation( Can you change this method's name, as it now stores more than just RT initiator data? https://codereview.chromium.org/2511313002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/fetch/ResourceFetcher.cpp:783: m_mainResourceTimingInfoMap.add(resource->identifier(), std::move(info)); kinuko@ - I'd love your opinion here as to whether we actually need a map? Are there legitimate scenarios where we can have multiple non-iframe main resources here? https://codereview.chromium.org/2511313002/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/loader/DocumentLoader.cpp (right): https://codereview.chromium.org/2511313002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/loader/DocumentLoader.cpp:149: return m_mainResourceIdentifier; Why do we need to store this rather than get the identifier from m_mainResource as before? Did you see issues related to that?
https://codereview.chromium.org/2511313002/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/fetch/ResourceFetcher.cpp (right): https://codereview.chromium.org/2511313002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/fetch/ResourceFetcher.cpp:755: void ResourceFetcher::storeResourceTimingInitiatorInformation( On 2016/11/29 10:39:32, Yoav Weiss wrote: > Can you change this method's name, as it now stores more than just RT initiator > data? Done. https://codereview.chromium.org/2511313002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/fetch/ResourceFetcher.cpp:783: m_mainResourceTimingInfoMap.add(resource->identifier(), std::move(info)); On 2016/11/29 10:39:32, Yoav Weiss wrote: > kinuko@ - I'd love your opinion here as to whether we actually need a map? Are > there legitimate scenarios where we can have multiple non-iframe main resources > here? Acknowledged. https://codereview.chromium.org/2511313002/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/loader/DocumentLoader.cpp (right): https://codereview.chromium.org/2511313002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/loader/DocumentLoader.cpp:149: return m_mainResourceIdentifier; On 2016/11/29 10:39:32, Yoav Weiss wrote: > Why do we need to store this rather than get the identifier from m_mainResource > as before? Did you see issues related to that? Yeah, m_mainResource gets reset to nullptr after in clearMainResourceHandle() after notifyFinished(Resource* resource) gets called. The mainResourceIdentifier() method was used for logging purpose when they decide not to continue loading (something went wrong) which will not cause m_mainResource gets cleared. But when loading finished (success or failure), they decided that they no longer need to hold on to this object any more.
panicker@chromium.org changed reviewers: + japhet@chromium.org
https://codereview.chromium.org/2511313002/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/fetch/ResourceFetcher.cpp (right): https://codereview.chromium.org/2511313002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/fetch/ResourceFetcher.cpp:783: m_mainResourceTimingInfoMap.add(resource->identifier(), std::move(info)); On 2016/11/29 19:33:30, sunjian wrote: > On 2016/11/29 10:39:32, Yoav Weiss wrote: > > kinuko@ - I'd love your opinion here as to whether we actually need a map? Are > > there legitimate scenarios where we can have multiple non-iframe main > resources > > here? > > Acknowledged. Also I added OWNERS of core/fetch to the review (tyoshino, Nate) - could you please chime in here?
https://codereview.chromium.org/2511313002/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/fetch/ResourceFetcher.cpp (right): https://codereview.chromium.org/2511313002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/fetch/ResourceFetcher.cpp:783: m_mainResourceTimingInfoMap.add(resource->identifier(), std::move(info)); On 2016/11/30 00:12:15, Shubhie wrote: > On 2016/11/29 19:33:30, sunjian wrote: > > On 2016/11/29 10:39:32, Yoav Weiss wrote: > > > kinuko@ - I'd love your opinion here as to whether we actually need a map? > Are > > > there legitimate scenarios where we can have multiple non-iframe main > > resources > > > here? > > > > Acknowledged. > > Also I added OWNERS of core/fetch to the review (tyoshino, Nate) - could you > please chime in here? Sorry for slow response. Hmm, I don't think we need this map... I might need to double-check but in my understanding ResourceFetcher is created 1:1 for a document and should be corresponding to one main resource.
https://codereview.chromium.org/2511313002/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/fetch/ResourceFetcher.cpp (right): https://codereview.chromium.org/2511313002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/core/fetch/ResourceFetcher.cpp:1156: // Store redirect responses that were packed inside the final response. We seem to be duplicating the almost same code here and below, could we clean it a bit? https://codereview.chromium.org/2511313002/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/fetch/ResourceFetcher.h (right): https://codereview.chromium.org/2511313002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/core/fetch/ResourceFetcher.h:243: MainResourceTimingInfoMap m_mainResourceTimingInfoMap; Again my gut feeling is we probably wouldn't need it... unless other experts say something different (if so I'll very happily stand corrected) could we drop this map and have some release CHECKs to see it isn't overwritten if we want to be extra sure about that?
The CQ bit was checked by sunjian@chromium.org to run a CQ dry run
https://codereview.chromium.org/2511313002/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/fetch/ResourceFetcher.cpp (right): https://codereview.chromium.org/2511313002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/core/fetch/ResourceFetcher.cpp:1156: // Store redirect responses that were packed inside the final response. On 2016/11/30 07:34:54, kinuko wrote: > We seem to be duplicating the almost same code here and below, could we clean it > a bit? Done. https://codereview.chromium.org/2511313002/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/fetch/ResourceFetcher.h (right): https://codereview.chromium.org/2511313002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/core/fetch/ResourceFetcher.h:243: MainResourceTimingInfoMap m_mainResourceTimingInfoMap; On 2016/11/30 07:34:54, kinuko wrote: > Again my gut feeling is we probably wouldn't need it... unless other experts say > something different (if so I'll very happily stand corrected) could we drop this > map and have some release CHECKs to see it isn't overwritten if we want to be > extra sure about that? Done.
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Thanks for your work on this :) Could you add some tests to make sure this is working properly? https://codereview.chromium.org/2511313002/diff/160001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/fetch/ResourceFetcher.cpp (right): https://codereview.chromium.org/2511313002/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/core/fetch/ResourceFetcher.cpp:106: void addRedirectsToResourceTimingInfo(Resource* resource, As this is used for both RT and NT2, do you think renaming this function would be appropriate? https://codereview.chromium.org/2511313002/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/core/fetch/ResourceFetcher.cpp:1144: if (m_mainResourceTimingInfo) { The condition here seems unnecessary other than for DCHECK purposes. Can you rewrite so that the condition is contained inside the DCHECK (to avoid slowing down shipping release builds) https://codereview.chromium.org/2511313002/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/core/fetch/ResourceFetcher.cpp:1436: m_mainResourceTimingInfo) { I think this is true for iframes as well. Would be great to see a test for that scenario (redirected iframe) https://codereview.chromium.org/2511313002/diff/160001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/fetch/ResourceFetcher.h (right): https://codereview.chromium.org/2511313002/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/core/fetch/ResourceFetcher.h:239: unsigned long m_mainResourceIdentifier; This should be initialized
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2511313002/diff/160001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/fetch/ResourceFetcher.cpp (right): https://codereview.chromium.org/2511313002/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/core/fetch/ResourceFetcher.cpp:106: void addRedirectsToResourceTimingInfo(Resource* resource, On 2016/12/01 21:53:55, Yoav Weiss wrote: > As this is used for both RT and NT2, do you think renaming this function would > be appropriate? I personally think it might be a better idea not to differentiate RT and NT2 here. Apparently this method is purely for updating ResourceTimingInfo object for all resources, including mainResource which is associated with NT2. And i made a decision in here to reuse ResourceTimingInfo for NT2 instead of creating a new counterpart class. https://codereview.chromium.org/2511313002/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/core/fetch/ResourceFetcher.cpp:1144: if (m_mainResourceTimingInfo) { On 2016/12/01 21:53:55, Yoav Weiss wrote: > The condition here seems unnecessary other than for DCHECK purposes. Can you > rewrite so that the condition is contained inside the DCHECK (to avoid slowing > down shipping release builds) Agree. But since this is a public method, how i can know for sure that other callers will invoke this method at the right time (after main resource is fetched) when they use it. https://codereview.chromium.org/2511313002/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/core/fetch/ResourceFetcher.cpp:1436: m_mainResourceTimingInfo) { On 2016/12/01 21:53:55, Yoav Weiss wrote: > I think this is true for iframes as well. Would be great to see a test for that > scenario (redirected iframe) Did you mean context().updateTimingInfoForIFrameNavigation(info.get())? So if it's iframe and also main resource, i ignored it, which means m_mainResourceTimingInfo will be nullptr, right? https://codereview.chromium.org/2511313002/diff/160001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/fetch/ResourceFetcher.h (right): https://codereview.chromium.org/2511313002/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/core/fetch/ResourceFetcher.h:239: unsigned long m_mainResourceIdentifier; On 2016/12/01 21:53:55, Yoav Weiss wrote: > This should be initialized Done.
Looking good to me. Are you thinking about proceeding with this CL further? The description of the CL says it's basically for getting initial feedback https://codereview.chromium.org/2511313002/diff/160001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/fetch/ResourceFetcher.cpp (right): https://codereview.chromium.org/2511313002/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/core/fetch/ResourceFetcher.cpp:1144: if (m_mainResourceTimingInfo) { On 2016/12/01 23:28:41, sunjian wrote: > On 2016/12/01 21:53:55, Yoav Weiss wrote: > > The condition here seems unnecessary other than for DCHECK purposes. Can you > > rewrite so that the condition is contained inside the DCHECK (to avoid slowing > > down shipping release builds) > > Agree. But since this is a public method, how i can know for sure that other > callers will invoke this method at the right time (after main resource is > fetched) when they use it. Just document / have a comment to say calling getMainResourceTimingInfo before main resource is fetched is invalid in .h, which should be enough. https://codereview.chromium.org/2511313002/diff/180001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/fetch/ResourceFetcher.cpp (right): https://codereview.chromium.org/2511313002/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/core/fetch/ResourceFetcher.cpp:1435: DCHECK(resource->identifier() == m_mainResourceIdentifier); DCHECK_EQ
Description was changed from ========== TransferSize implementation. This version is not a final version of the implementation. Hoping to get some feedback and validation from experts before finalizing it. BUG=663187 ========== to ========== TransferSize implementation. BUG=663187 ==========
On 2016/12/02 00:44:55, kinuko wrote: > Looking good to me. Are you thinking about proceeding with this CL further? > The description of the CL says it's basically for getting initial feedback > > https://codereview.chromium.org/2511313002/diff/160001/third_party/WebKit/Sou... > File third_party/WebKit/Source/core/fetch/ResourceFetcher.cpp (right): > > https://codereview.chromium.org/2511313002/diff/160001/third_party/WebKit/Sou... > third_party/WebKit/Source/core/fetch/ResourceFetcher.cpp:1144: if > (m_mainResourceTimingInfo) { > On 2016/12/01 23:28:41, sunjian wrote: > > On 2016/12/01 21:53:55, Yoav Weiss wrote: > > > The condition here seems unnecessary other than for DCHECK purposes. Can you > > > rewrite so that the condition is contained inside the DCHECK (to avoid > slowing > > > down shipping release builds) > > > > Agree. But since this is a public method, how i can know for sure that other > > callers will invoke this method at the right time (after main resource is > > fetched) when they use it. > > Just document / have a comment to say calling getMainResourceTimingInfo before > main resource is fetched is invalid in .h, which should be enough. > > https://codereview.chromium.org/2511313002/diff/180001/third_party/WebKit/Sou... > File third_party/WebKit/Source/core/fetch/ResourceFetcher.cpp (right): > > https://codereview.chromium.org/2511313002/diff/180001/third_party/WebKit/Sou... > third_party/WebKit/Source/core/fetch/ResourceFetcher.cpp:1435: > DCHECK(resource->identifier() == m_mainResourceIdentifier); > DCHECK_EQ Yes, i plan to go further with this cl.
https://codereview.chromium.org/2511313002/diff/180001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/fetch/ResourceFetcher.cpp (right): https://codereview.chromium.org/2511313002/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/core/fetch/ResourceFetcher.cpp:1435: DCHECK(resource->identifier() == m_mainResourceIdentifier); On 2016/12/02 00:44:55, kinuko wrote: > DCHECK_EQ Done.
https://codereview.chromium.org/2511313002/diff/160001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/fetch/ResourceFetcher.cpp (right): https://codereview.chromium.org/2511313002/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/core/fetch/ResourceFetcher.cpp:106: void addRedirectsToResourceTimingInfo(Resource* resource, On 2016/12/01 23:28:42, sunjian wrote: > On 2016/12/01 21:53:55, Yoav Weiss wrote: > > As this is used for both RT and NT2, do you think renaming this function would > > be appropriate? > > I personally think it might be a better idea not to differentiate RT and NT2 > here. Apparently this method is purely for updating ResourceTimingInfo object > for all resources, including mainResource which is associated with NT2. And i > made a decision in here to reuse ResourceTimingInfo for NT2 instead of creating > a new counterpart class. I'd have preferred something like "addRedirectsToTimingInfo()", but I'll leave that to you to decide. https://codereview.chromium.org/2511313002/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/core/fetch/ResourceFetcher.cpp:1144: if (m_mainResourceTimingInfo) { On 2016/12/01 23:28:41, sunjian wrote: > On 2016/12/01 21:53:55, Yoav Weiss wrote: > > The condition here seems unnecessary other than for DCHECK purposes. Can you > > rewrite so that the condition is contained inside the DCHECK (to avoid slowing > > down shipping release builds) > > Agree. But since this is a public method, how i can know for sure that other > callers will invoke this method at the right time (after main resource is > fetched) when they use it. if callers are invoking it before m_mainResourceTimingInfo was set, m_mainResourceTimingInfo.get() returns a nullptr, so the condition is not really meaningful. (plus as kinuko@ said, a comment + DCHECK will make sure this is not called when it's not supposed to) https://codereview.chromium.org/2511313002/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/core/fetch/ResourceFetcher.cpp:1436: m_mainResourceTimingInfo) { On 2016/12/01 23:28:41, sunjian wrote: > On 2016/12/01 21:53:55, Yoav Weiss wrote: > > I think this is true for iframes as well. Would be great to see a test for > that > > scenario (redirected iframe) > > Did you mean context().updateTimingInfoForIFrameNavigation(info.get())? So if > it's iframe and also main resource, i ignored it, which means > m_mainResourceTimingInfo will be nullptr, right? What happens when m_mainResourceTimingInfo is filled for the main resource, which is then added an iframe, which gets redirected? I think such a scenario will pass the condition here, but a test would show it either way for sure :)
https://codereview.chromium.org/2511313002/diff/200001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/fetch/ResourceFetcher.cpp (right): https://codereview.chromium.org/2511313002/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/core/fetch/ResourceFetcher.cpp:260: m_mainResourceIdentifier(0), This assumes a resource's identifier is never 0. Seems like current implementation does support that assumption, but it might be worth while to make that assumption explicit. (e.g. by DCHECKing input identifiers here)
Addressed comments and added unit tests. https://codereview.chromium.org/2511313002/diff/160001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/fetch/ResourceFetcher.cpp (right): https://codereview.chromium.org/2511313002/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/core/fetch/ResourceFetcher.cpp:106: void addRedirectsToResourceTimingInfo(Resource* resource, On 2016/12/02 07:10:05, Yoav Weiss wrote: > On 2016/12/01 23:28:42, sunjian wrote: > > On 2016/12/01 21:53:55, Yoav Weiss wrote: > > > As this is used for both RT and NT2, do you think renaming this function > would > > > be appropriate? > > > > I personally think it might be a better idea not to differentiate RT and NT2 > > here. Apparently this method is purely for updating ResourceTimingInfo object > > for all resources, including mainResource which is associated with NT2. And i > > made a decision in here to reuse ResourceTimingInfo for NT2 instead of > creating > > a new counterpart class. > > I'd have preferred something like "addRedirectsToTimingInfo()", but I'll leave > that to you to decide. Acknowledged. https://codereview.chromium.org/2511313002/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/core/fetch/ResourceFetcher.cpp:1144: if (m_mainResourceTimingInfo) { On 2016/12/02 07:10:05, Yoav Weiss wrote: > On 2016/12/01 23:28:41, sunjian wrote: > > On 2016/12/01 21:53:55, Yoav Weiss wrote: > > > The condition here seems unnecessary other than for DCHECK purposes. Can you > > > rewrite so that the condition is contained inside the DCHECK (to avoid > slowing > > > down shipping release builds) > > > > Agree. But since this is a public method, how i can know for sure that other > > callers will invoke this method at the right time (after main resource is > > fetched) when they use it. > > if callers are invoking it before m_mainResourceTimingInfo was set, > m_mainResourceTimingInfo.get() returns a nullptr, so the condition is not really > meaningful. (plus as kinuko@ said, a comment + DCHECK will make sure this is not > called when it's not supposed to) This commit is old. I uploaded a new commit and that one actually addressed that. https://codereview.chromium.org/2511313002/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/core/fetch/ResourceFetcher.cpp:1436: m_mainResourceTimingInfo) { On 2016/12/02 07:10:05, Yoav Weiss wrote: > On 2016/12/01 23:28:41, sunjian wrote: > > On 2016/12/01 21:53:55, Yoav Weiss wrote: > > > I think this is true for iframes as well. Would be great to see a test for > > that > > > scenario (redirected iframe) > > > > Did you mean context().updateTimingInfoForIFrameNavigation(info.get())? So if > > it's iframe and also main resource, i ignored it, which means > > m_mainResourceTimingInfo will be nullptr, right? > > What happens when m_mainResourceTimingInfo is filled for the main resource, > which is then added an iframe, which gets redirected? > I think such a scenario will pass the condition here, but a test would show it > either way for sure :) I see. So i modified the if condition a little bit. Instead of verifying whether m_mainResourceTimingInfo is empty or not, now i verify the whether the identifiers match and DCHECK m_mainResourceTimingInfo. I think this will eliminate the possibility you mentioned. Let me know if you think a test is still needed. https://codereview.chromium.org/2511313002/diff/200001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/fetch/ResourceFetcher.cpp (right): https://codereview.chromium.org/2511313002/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/core/fetch/ResourceFetcher.cpp:260: m_mainResourceIdentifier(0), On 2016/12/02 07:42:49, Yoav Weiss wrote: > This assumes a resource's identifier is never 0. Seems like current > implementation does support that assumption, but it might be worth while to make > that assumption explicit. (e.g. by DCHECKing input identifiers here) So getMainResourceTimingInfo() doesn't take any identifier any more. I am not sure whether that's where you want me to add the DCHECK or not?
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: cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
Some updates on DCHECKS in getMainResourceTimingInfo() in ResourceFetcher.
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_...)
On 2016/12/03 00:16:50, sunjian wrote: > Addressed comments and added unit tests. > > https://codereview.chromium.org/2511313002/diff/160001/third_party/WebKit/Sou... > File third_party/WebKit/Source/core/fetch/ResourceFetcher.cpp (right): > > https://codereview.chromium.org/2511313002/diff/160001/third_party/WebKit/Sou... > third_party/WebKit/Source/core/fetch/ResourceFetcher.cpp:106: void > addRedirectsToResourceTimingInfo(Resource* resource, > On 2016/12/02 07:10:05, Yoav Weiss wrote: > > On 2016/12/01 23:28:42, sunjian wrote: > > > On 2016/12/01 21:53:55, Yoav Weiss wrote: > > > > As this is used for both RT and NT2, do you think renaming this function > > would > > > > be appropriate? > > > > > > I personally think it might be a better idea not to differentiate RT and NT2 > > > here. Apparently this method is purely for updating ResourceTimingInfo > object > > > for all resources, including mainResource which is associated with NT2. And > i > > > made a decision in here to reuse ResourceTimingInfo for NT2 instead of > > creating > > > a new counterpart class. > > > > I'd have preferred something like "addRedirectsToTimingInfo()", but I'll leave > > that to you to decide. > > Acknowledged. > > https://codereview.chromium.org/2511313002/diff/160001/third_party/WebKit/Sou... > third_party/WebKit/Source/core/fetch/ResourceFetcher.cpp:1144: if > (m_mainResourceTimingInfo) { > On 2016/12/02 07:10:05, Yoav Weiss wrote: > > On 2016/12/01 23:28:41, sunjian wrote: > > > On 2016/12/01 21:53:55, Yoav Weiss wrote: > > > > The condition here seems unnecessary other than for DCHECK purposes. Can > you > > > > rewrite so that the condition is contained inside the DCHECK (to avoid > > slowing > > > > down shipping release builds) > > > > > > Agree. But since this is a public method, how i can know for sure that other > > > callers will invoke this method at the right time (after main resource is > > > fetched) when they use it. > > > > if callers are invoking it before m_mainResourceTimingInfo was set, > > m_mainResourceTimingInfo.get() returns a nullptr, so the condition is not > really > > meaningful. (plus as kinuko@ said, a comment + DCHECK will make sure this is > not > > called when it's not supposed to) > > This commit is old. I uploaded a new commit and that one actually addressed > that. > > https://codereview.chromium.org/2511313002/diff/160001/third_party/WebKit/Sou... > third_party/WebKit/Source/core/fetch/ResourceFetcher.cpp:1436: > m_mainResourceTimingInfo) { > On 2016/12/02 07:10:05, Yoav Weiss wrote: > > On 2016/12/01 23:28:41, sunjian wrote: > > > On 2016/12/01 21:53:55, Yoav Weiss wrote: > > > > I think this is true for iframes as well. Would be great to see a test for > > > that > > > > scenario (redirected iframe) > > > > > > Did you mean context().updateTimingInfoForIFrameNavigation(info.get())? So > if > > > it's iframe and also main resource, i ignored it, which means > > > m_mainResourceTimingInfo will be nullptr, right? > > > > What happens when m_mainResourceTimingInfo is filled for the main resource, > > which is then added an iframe, which gets redirected? > > I think such a scenario will pass the condition here, but a test would show it > > either way for sure :) > > I see. So i modified the if condition a little bit. Instead of verifying whether > m_mainResourceTimingInfo is empty or not, now i verify the whether the > identifiers match and DCHECK m_mainResourceTimingInfo. I think this will > eliminate the possibility you mentioned. Let me know if you think a test is > still needed. > Great to see this is fixed. A test that exercises that scenario would be a good way to see this doesn't regress. Please add one. > https://codereview.chromium.org/2511313002/diff/200001/third_party/WebKit/Sou... > File third_party/WebKit/Source/core/fetch/ResourceFetcher.cpp (right): > > https://codereview.chromium.org/2511313002/diff/200001/third_party/WebKit/Sou... > third_party/WebKit/Source/core/fetch/ResourceFetcher.cpp:260: > m_mainResourceIdentifier(0), > On 2016/12/02 07:42:49, Yoav Weiss wrote: > > This assumes a resource's identifier is never 0. Seems like current > > implementation does support that assumption, but it might be worth while to > make > > that assumption explicit. (e.g. by DCHECKing input identifiers here) > > So getMainResourceTimingInfo() doesn't take any identifier any more. I am not > sure whether that's where you want me to add the DCHECK or not? In functions that take identifier as input, a `DCHECK_NE(identifier, 0)` would make sure this assumption is explicit, and remains true in the future.
Test failures seem relevant https://codereview.chromium.org/2511313002/diff/260001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/fetch/ResourceFetcher.h (right): https://codereview.chromium.org/2511313002/diff/260001/third_party/WebKit/Sou... third_party/WebKit/Source/core/fetch/ResourceFetcher.h:162: // Calling this method before main resource is fetched is invalid. Nit: spurious space
On 2016/12/03 19:45:03, Yoav Weiss wrote: > On 2016/12/03 00:16:50, sunjian wrote: > > Addressed comments and added unit tests. > > > > > https://codereview.chromium.org/2511313002/diff/160001/third_party/WebKit/Sou... > > File third_party/WebKit/Source/core/fetch/ResourceFetcher.cpp (right): > > > > > https://codereview.chromium.org/2511313002/diff/160001/third_party/WebKit/Sou... > > third_party/WebKit/Source/core/fetch/ResourceFetcher.cpp:106: void > > addRedirectsToResourceTimingInfo(Resource* resource, > > On 2016/12/02 07:10:05, Yoav Weiss wrote: > > > On 2016/12/01 23:28:42, sunjian wrote: > > > > On 2016/12/01 21:53:55, Yoav Weiss wrote: > > > > > As this is used for both RT and NT2, do you think renaming this function > > > would > > > > > be appropriate? > > > > > > > > I personally think it might be a better idea not to differentiate RT and > NT2 > > > > here. Apparently this method is purely for updating ResourceTimingInfo > > object > > > > for all resources, including mainResource which is associated with NT2. > And > > i > > > > made a decision in here to reuse ResourceTimingInfo for NT2 instead of > > > creating > > > > a new counterpart class. > > > > > > I'd have preferred something like "addRedirectsToTimingInfo()", but I'll > leave > > > that to you to decide. > > > > Acknowledged. > > > > > https://codereview.chromium.org/2511313002/diff/160001/third_party/WebKit/Sou... > > third_party/WebKit/Source/core/fetch/ResourceFetcher.cpp:1144: if > > (m_mainResourceTimingInfo) { > > On 2016/12/02 07:10:05, Yoav Weiss wrote: > > > On 2016/12/01 23:28:41, sunjian wrote: > > > > On 2016/12/01 21:53:55, Yoav Weiss wrote: > > > > > The condition here seems unnecessary other than for DCHECK purposes. Can > > you > > > > > rewrite so that the condition is contained inside the DCHECK (to avoid > > > slowing > > > > > down shipping release builds) > > > > > > > > Agree. But since this is a public method, how i can know for sure that > other > > > > callers will invoke this method at the right time (after main resource is > > > > fetched) when they use it. > > > > > > if callers are invoking it before m_mainResourceTimingInfo was set, > > > m_mainResourceTimingInfo.get() returns a nullptr, so the condition is not > > really > > > meaningful. (plus as kinuko@ said, a comment + DCHECK will make sure this is > > not > > > called when it's not supposed to) > > > > This commit is old. I uploaded a new commit and that one actually addressed > > that. > > > > > https://codereview.chromium.org/2511313002/diff/160001/third_party/WebKit/Sou... > > third_party/WebKit/Source/core/fetch/ResourceFetcher.cpp:1436: > > m_mainResourceTimingInfo) { > > On 2016/12/02 07:10:05, Yoav Weiss wrote: > > > On 2016/12/01 23:28:41, sunjian wrote: > > > > On 2016/12/01 21:53:55, Yoav Weiss wrote: > > > > > I think this is true for iframes as well. Would be great to see a test > for > > > > that > > > > > scenario (redirected iframe) > > > > > > > > Did you mean context().updateTimingInfoForIFrameNavigation(info.get())? So > > if > > > > it's iframe and also main resource, i ignored it, which means > > > > m_mainResourceTimingInfo will be nullptr, right? > > > > > > What happens when m_mainResourceTimingInfo is filled for the main resource, > > > which is then added an iframe, which gets redirected? > > > I think such a scenario will pass the condition here, but a test would show > it > > > either way for sure :) > > > > I see. So i modified the if condition a little bit. Instead of verifying > whether > > m_mainResourceTimingInfo is empty or not, now i verify the whether the > > identifiers match and DCHECK m_mainResourceTimingInfo. I think this will > > eliminate the possibility you mentioned. Let me know if you think a test is > > still needed. > > > > Great to see this is fixed. A test that exercises that scenario would be a good > way to see this doesn't regress. > Please add one. > > > > https://codereview.chromium.org/2511313002/diff/200001/third_party/WebKit/Sou... > > File third_party/WebKit/Source/core/fetch/ResourceFetcher.cpp (right): > > > > > https://codereview.chromium.org/2511313002/diff/200001/third_party/WebKit/Sou... > > third_party/WebKit/Source/core/fetch/ResourceFetcher.cpp:260: > > m_mainResourceIdentifier(0), > > On 2016/12/02 07:42:49, Yoav Weiss wrote: > > > This assumes a resource's identifier is never 0. Seems like current > > > implementation does support that assumption, but it might be worth while to > > make > > > that assumption explicit. (e.g. by DCHECKing input identifiers here) > > > > So getMainResourceTimingInfo() doesn't take any identifier any more. I am not > > sure whether that's where you want me to add the DCHECK or not? > > In functions that take identifier as input, a `DCHECK_NE(identifier, 0)` would > make sure this assumption is explicit, and remains true in the future. Right. Since we are not adding any functions that take an identifier any more, i guess there nothing more for me to do regarding this comment, right?
Almost. Added a comment where such a DCHECK would be useful https://codereview.chromium.org/2511313002/diff/260001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/fetch/ResourceFetcher.cpp (right): https://codereview.chromium.org/2511313002/diff/260001/third_party/WebKit/Sou... third_party/WebKit/Source/core/fetch/ResourceFetcher.cpp:800: m_mainResourceIdentifier = resource->identifier(); Can you add a DCHECK here to make sure the resource identifier is never 0?
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 #13 (id:280001) 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: 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_...)
Just a couple of questions, sorry if they were covered already and I missed them in the scrollback. https://codereview.chromium.org/2511313002/diff/300001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/fetch/ResourceFetcher.cpp (right): https://codereview.chromium.org/2511313002/diff/300001/third_party/WebKit/Sou... third_party/WebKit/Source/core/fetch/ResourceFetcher.cpp:1169: resource->identifier() == m_mainResourceIdentifier) { I didn't follow your explanation earlier: why isn't "if (resource->getType() == Resource::MainResource)" sufficient here? There should only be one Resource::MainResource load per ResourceFetcher, right? https://codereview.chromium.org/2511313002/diff/300001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/timing/PerformanceBase.cpp (right): https://codereview.chromium.org/2511313002/diff/300001/third_party/WebKit/Sou... third_party/WebKit/Source/core/timing/PerformanceBase.cpp:347: documentLoader->getMainResourceTimingInfo(); Should the main resource timing info always be the first entry in the m_resourceTimingBuffer? Could we get away without this accessor?
https://codereview.chromium.org/2511313002/diff/300001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/fetch/ResourceFetcher.cpp (right): https://codereview.chromium.org/2511313002/diff/300001/third_party/WebKit/Sou... third_party/WebKit/Source/core/fetch/ResourceFetcher.cpp:1169: resource->identifier() == m_mainResourceIdentifier) { On 2016/12/06 20:07:50, Nate Chapin (slow until Jan 3) wrote: > I didn't follow your explanation earlier: why isn't "if (resource->getType() == > Resource::MainResource)" sufficient here? There should only be one > Resource::MainResource load per ResourceFetcher, right? We weren't certain of that, therefore the identifier was added as a precautionary measure. However if you are certain of the fact that there is always one main resource, then the identifier can be removed. (This question was asked earlier in the thread of the OWNERS, thanks for chiming in here!).
https://codereview.chromium.org/2511313002/diff/300001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/fetch/ResourceFetcher.cpp (right): https://codereview.chromium.org/2511313002/diff/300001/third_party/WebKit/Sou... third_party/WebKit/Source/core/fetch/ResourceFetcher.cpp:1169: resource->identifier() == m_mainResourceIdentifier) { On 2016/12/06 20:07:50, Nate Chapin (slow until Jan 3) wrote: > I didn't follow your explanation earlier: why isn't "if (resource->getType() == > Resource::MainResource)" sufficient here? There should only be one > Resource::MainResource load per ResourceFetcher, right? An iframe resource loading here would have a MainResource type, but a different identifier than that of the main resource.
On 2016/12/07 01:26:37, Yoav Weiss wrote: > https://codereview.chromium.org/2511313002/diff/300001/third_party/WebKit/Sou... > File third_party/WebKit/Source/core/fetch/ResourceFetcher.cpp (right): > > https://codereview.chromium.org/2511313002/diff/300001/third_party/WebKit/Sou... > third_party/WebKit/Source/core/fetch/ResourceFetcher.cpp:1169: > resource->identifier() == m_mainResourceIdentifier) { > On 2016/12/06 20:07:50, Nate Chapin (slow until Jan 3) wrote: > > I didn't follow your explanation earlier: why isn't "if (resource->getType() > == > > Resource::MainResource)" sufficient here? There should only be one > > Resource::MainResource load per ResourceFetcher, right? > > An iframe resource loading here would have a MainResource type, but a different > identifier than that of the main resource. True, but an iframe will have its own ResourceFetcher, so it should still be one MainResource load per ResourceFetcher.
On 2016/12/07 18:11:43, Nate Chapin (slow until Jan 3) wrote: > On 2016/12/07 01:26:37, Yoav Weiss wrote: > > > https://codereview.chromium.org/2511313002/diff/300001/third_party/WebKit/Sou... > > File third_party/WebKit/Source/core/fetch/ResourceFetcher.cpp (right): > > > > > https://codereview.chromium.org/2511313002/diff/300001/third_party/WebKit/Sou... > > third_party/WebKit/Source/core/fetch/ResourceFetcher.cpp:1169: > > resource->identifier() == m_mainResourceIdentifier) { > > On 2016/12/06 20:07:50, Nate Chapin (slow until Jan 3) wrote: > > > I didn't follow your explanation earlier: why isn't "if (resource->getType() > > == > > > Resource::MainResource)" sufficient here? There should only be one > > > Resource::MainResource load per ResourceFetcher, right? > > > > An iframe resource loading here would have a MainResource type, but a > different > > identifier than that of the main resource. > > True, but an iframe will have its own ResourceFetcher, so it should still be one > MainResource load per ResourceFetcher. OK, my bad. In that case, I agree this extra check is not necessary.
https://codereview.chromium.org/2511313002/diff/260001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/fetch/ResourceFetcher.cpp (right): https://codereview.chromium.org/2511313002/diff/260001/third_party/WebKit/Sou... third_party/WebKit/Source/core/fetch/ResourceFetcher.cpp:800: m_mainResourceIdentifier = resource->identifier(); On 2016/12/05 19:21:04, Yoav Weiss wrote: > Can you add a DCHECK here to make sure the resource identifier is never 0? Done. https://codereview.chromium.org/2511313002/diff/260001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/fetch/ResourceFetcher.h (right): https://codereview.chromium.org/2511313002/diff/260001/third_party/WebKit/Sou... third_party/WebKit/Source/core/fetch/ResourceFetcher.h:162: // Calling this method before main resource is fetched is invalid. On 2016/12/03 19:55:35, Yoav Weiss wrote: > Nit: spurious space Done. https://codereview.chromium.org/2511313002/diff/300001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/fetch/ResourceFetcher.cpp (right): https://codereview.chromium.org/2511313002/diff/300001/third_party/WebKit/Sou... third_party/WebKit/Source/core/fetch/ResourceFetcher.cpp:1169: resource->identifier() == m_mainResourceIdentifier) { On 2016/12/06 20:07:50, Nate Chapin (slow until Jan 3) wrote: > I didn't follow your explanation earlier: why isn't "if (resource->getType() == > Resource::MainResource)" sufficient here? There should only be one > Resource::MainResource load per ResourceFetcher, right? I think you are correct about the fact that each ResourceFetcher corresponds to only one main resource. We thought it could be mapped to multiple, including some iframes. https://codereview.chromium.org/2511313002/diff/300001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/timing/PerformanceBase.cpp (right): https://codereview.chromium.org/2511313002/diff/300001/third_party/WebKit/Sou... third_party/WebKit/Source/core/timing/PerformanceBase.cpp:347: documentLoader->getMainResourceTimingInfo(); On 2016/12/06 20:07:50, Nate Chapin (slow until Jan 3) wrote: > Should the main resource timing info always be the first entry in the > m_resourceTimingBuffer? Could we get away without this accessor? We need this accessor to access non-iframe main resource, which wasn't captured before because this is specifically for navigation timing 2. The first entry in m_resourceTimingBuffer you referred is iframe. I'll change the name of the method and buffer just to be more clear.
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: Try jobs failed on following builders: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
Minor nits to remove obsolete comments. https://codereview.chromium.org/2511313002/diff/340001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/fetch/ResourceFetcher.cpp (right): https://codereview.chromium.org/2511313002/diff/340001/third_party/WebKit/Sou... third_party/WebKit/Source/core/fetch/ResourceFetcher.cpp:1141: // m_navigationTimingInfo can be null if it is an iframe navigation restored remove this comment? https://codereview.chromium.org/2511313002/diff/340001/third_party/WebKit/Sou... third_party/WebKit/Source/core/fetch/ResourceFetcher.cpp:1165: // When it is non-iframe main resource. remove this comment https://codereview.chromium.org/2511313002/diff/340001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/fetch/ResourceLoader.cpp (right): https://codereview.chromium.org/2511313002/diff/340001/third_party/WebKit/Sou... third_party/WebKit/Source/core/fetch/ResourceLoader.cpp:82: } revert this file?
Patchset #16 (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...
LGTM % a test for the iframe case. Thanks for your continuous work on this issue :)
https://codereview.chromium.org/2511313002/diff/340001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/fetch/ResourceFetcher.cpp (right): https://codereview.chromium.org/2511313002/diff/340001/third_party/WebKit/Sou... third_party/WebKit/Source/core/fetch/ResourceFetcher.cpp:796: if (isMainResource) { Could you add a test for the iframe navTiming case? https://codereview.chromium.org/2511313002/diff/380001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/fetch/ResourceFetcher.cpp (right): https://codereview.chromium.org/2511313002/diff/380001/third_party/WebKit/Sou... third_party/WebKit/Source/core/fetch/ResourceFetcher.cpp:108: const Vector<ResourceResponse>& responses = Nit: const auto&
On 2016/12/09 00:11:13, Yoav Weiss wrote: > https://codereview.chromium.org/2511313002/diff/340001/third_party/WebKit/Sou... > File third_party/WebKit/Source/core/fetch/ResourceFetcher.cpp (right): > > https://codereview.chromium.org/2511313002/diff/340001/third_party/WebKit/Sou... > third_party/WebKit/Source/core/fetch/ResourceFetcher.cpp:796: if > (isMainResource) { > Could you add a test for the iframe navTiming case? > > https://codereview.chromium.org/2511313002/diff/380001/third_party/WebKit/Sou... > File third_party/WebKit/Source/core/fetch/ResourceFetcher.cpp (right): > > https://codereview.chromium.org/2511313002/diff/380001/third_party/WebKit/Sou... > third_party/WebKit/Source/core/fetch/ResourceFetcher.cpp:108: const > Vector<ResourceResponse>& responses = > Nit: const auto& So i provided unit tests to make sure the m_navigationTimingInfo gets created and populated correctly. From my understanding, loading an iframe is the same as loading a regular document, which includes loading one main resource and a bunch of non-main resources. In terms of testing each iframe has a nav timing instance, we plan to cover do it in the webplatform tests. Does that make sense to you?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2016/12/09 00:27:03, sunjian wrote: > On 2016/12/09 00:11:13, Yoav Weiss wrote: > > > https://codereview.chromium.org/2511313002/diff/340001/third_party/WebKit/Sou... > > File third_party/WebKit/Source/core/fetch/ResourceFetcher.cpp (right): > > > > > https://codereview.chromium.org/2511313002/diff/340001/third_party/WebKit/Sou... > > third_party/WebKit/Source/core/fetch/ResourceFetcher.cpp:796: if > > (isMainResource) { > > Could you add a test for the iframe navTiming case? > > > > > https://codereview.chromium.org/2511313002/diff/380001/third_party/WebKit/Sou... > > File third_party/WebKit/Source/core/fetch/ResourceFetcher.cpp (right): > > > > > https://codereview.chromium.org/2511313002/diff/380001/third_party/WebKit/Sou... > > third_party/WebKit/Source/core/fetch/ResourceFetcher.cpp:108: const > > Vector<ResourceResponse>& responses = > > Nit: const auto& > > So i provided unit tests to make sure the m_navigationTimingInfo gets created > and populated correctly. From my understanding, loading an iframe is the same as > loading a regular document, which includes loading one main resource and a bunch > of non-main resources. In terms of testing each iframe has a nav timing > instance, we plan to cover do it in the webplatform tests. Does that make sense > to you? Coverage in WPT makes sense. Can you add a TODO to that effect?
On 2016/12/09 00:29:48, Yoav Weiss wrote: > On 2016/12/09 00:27:03, sunjian wrote: > > On 2016/12/09 00:11:13, Yoav Weiss wrote: > > > > > > https://codereview.chromium.org/2511313002/diff/340001/third_party/WebKit/Sou... > > > File third_party/WebKit/Source/core/fetch/ResourceFetcher.cpp (right): > > > > > > > > > https://codereview.chromium.org/2511313002/diff/340001/third_party/WebKit/Sou... > > > third_party/WebKit/Source/core/fetch/ResourceFetcher.cpp:796: if > > > (isMainResource) { > > > Could you add a test for the iframe navTiming case? > > > > > > > > > https://codereview.chromium.org/2511313002/diff/380001/third_party/WebKit/Sou... > > > File third_party/WebKit/Source/core/fetch/ResourceFetcher.cpp (right): > > > > > > > > > https://codereview.chromium.org/2511313002/diff/380001/third_party/WebKit/Sou... > > > third_party/WebKit/Source/core/fetch/ResourceFetcher.cpp:108: const > > > Vector<ResourceResponse>& responses = > > > Nit: const auto& > > > > So i provided unit tests to make sure the m_navigationTimingInfo gets created > > and populated correctly. From my understanding, loading an iframe is the same > as > > loading a regular document, which includes loading one main resource and a > bunch > > of non-main resources. In terms of testing each iframe has a nav timing > > instance, we plan to cover do it in the webplatform tests. Does that make > sense > > to you? > > Coverage in WPT makes sense. Can you add a TODO to that effect? So the test actually is already written. But it is on Github. Webplatform tests will go parallel with our new features like transferSize and redirect opt in, add a TODO in here will cause me to come back just to get rid of the TODOs cause there will not reflection in the code that the TODOs already got resolved.
https://codereview.chromium.org/2511313002/diff/340001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/fetch/ResourceFetcher.cpp (right): https://codereview.chromium.org/2511313002/diff/340001/third_party/WebKit/Sou... third_party/WebKit/Source/core/fetch/ResourceFetcher.cpp:1141: // m_navigationTimingInfo can be null if it is an iframe navigation restored On 2016/12/08 20:47:06, panicker wrote: > remove this comment? Done. https://codereview.chromium.org/2511313002/diff/340001/third_party/WebKit/Sou... third_party/WebKit/Source/core/fetch/ResourceFetcher.cpp:1165: // When it is non-iframe main resource. On 2016/12/08 20:47:06, panicker wrote: > remove this comment Done. https://codereview.chromium.org/2511313002/diff/340001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/fetch/ResourceLoader.cpp (right): https://codereview.chromium.org/2511313002/diff/340001/third_party/WebKit/Sou... third_party/WebKit/Source/core/fetch/ResourceLoader.cpp:82: } On 2016/12/08 20:47:06, panicker wrote: > revert this file? Done. https://codereview.chromium.org/2511313002/diff/380001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/fetch/ResourceFetcher.cpp (right): https://codereview.chromium.org/2511313002/diff/380001/third_party/WebKit/Sou... third_party/WebKit/Source/core/fetch/ResourceFetcher.cpp:108: const Vector<ResourceResponse>& responses = On 2016/12/09 00:11:13, Yoav Weiss wrote: > Nit: const auto& Done.
On 2016/12/09 00:48:58, sunjian wrote: > On 2016/12/09 00:29:48, Yoav Weiss wrote: > > On 2016/12/09 00:27:03, sunjian wrote: > > > On 2016/12/09 00:11:13, Yoav Weiss wrote: > > > > > > > > > > https://codereview.chromium.org/2511313002/diff/340001/third_party/WebKit/Sou... > > > > File third_party/WebKit/Source/core/fetch/ResourceFetcher.cpp (right): > > > > > > > > > > > > > > https://codereview.chromium.org/2511313002/diff/340001/third_party/WebKit/Sou... > > > > third_party/WebKit/Source/core/fetch/ResourceFetcher.cpp:796: if > > > > (isMainResource) { > > > > Could you add a test for the iframe navTiming case? > > > > > > > > > > > > > > https://codereview.chromium.org/2511313002/diff/380001/third_party/WebKit/Sou... > > > > File third_party/WebKit/Source/core/fetch/ResourceFetcher.cpp (right): > > > > > > > > > > > > > > https://codereview.chromium.org/2511313002/diff/380001/third_party/WebKit/Sou... > > > > third_party/WebKit/Source/core/fetch/ResourceFetcher.cpp:108: const > > > > Vector<ResourceResponse>& responses = > > > > Nit: const auto& > > > > > > So i provided unit tests to make sure the m_navigationTimingInfo gets > created > > > and populated correctly. From my understanding, loading an iframe is the > same > > as > > > loading a regular document, which includes loading one main resource and a > > bunch > > > of non-main resources. In terms of testing each iframe has a nav timing > > > instance, we plan to cover do it in the webplatform tests. Does that make > > sense > > > to you? > > > > Coverage in WPT makes sense. Can you add a TODO to that effect? > > So the test actually is already written. But it is on Github. Webplatform tests > will go parallel with our new features like transferSize and redirect opt in, > add a TODO in here will cause me to come back just to get rid of the TODOs cause > there will not reflection in the code that the TODOs already got resolved. fair enough. If the tests are landing soon, that works for me
https://codereview.chromium.org/2511313002/diff/400001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/fetch/ResourceFetcher.cpp (right): https://codereview.chromium.org/2511313002/diff/400001/third_party/WebKit/Sou... third_party/WebKit/Source/core/fetch/ResourceFetcher.cpp:117: } It looks PerformanceResourceTiming is constructed with the result of passesTimingAllowCheck() to reflect Timing-Allow-Origin, and it affects the result of transferSize() (https://w3c.github.io/resource-timing/#dom-performanceresourcetiming-transfer...). PerformanceNavigationTiming is passing true for the allowTimingDetails argument. I see your TODO comment there. Don't we need to implement it now? I'm not yet so familiar with the resource timing and navigation timing. Tell me if I'm misunderstanding. https://codereview.chromium.org/2511313002/diff/400001/third_party/WebKit/Sou... third_party/WebKit/Source/core/fetch/ResourceFetcher.cpp:786: // and Redirect timing opt in information. Redirect -> redirect opt in -> opt-in https://codereview.chromium.org/2511313002/diff/400001/third_party/WebKit/Sou... third_party/WebKit/Source/core/fetch/ResourceFetcher.cpp:1176: } don't we need to have the same protocol/status check as L1182? Here're the commits explaining why the check has been introduced: https://chromium.googlesource.com/chromium/src/+/895ef0862cc0da80e9f6c2fb9b18... https://chromium.googlesource.com/chromium/src/+/558203127f4c7ca76b2e02965eaa... https://chromium.googlesource.com/chromium/src/+/9f72becdf7e9c050ec341d8356ec... https://chromium.googlesource.com/chromium/src/+/2f9a97aa70c6386dce993735d7db...
https://codereview.chromium.org/2511313002/diff/400001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/fetch/ResourceFetcher.cpp (right): https://codereview.chromium.org/2511313002/diff/400001/third_party/WebKit/Sou... third_party/WebKit/Source/core/fetch/ResourceFetcher.cpp:117: } On 2016/12/09 07:35:58, tyoshino wrote: > It looks PerformanceResourceTiming is constructed with the result of > passesTimingAllowCheck() to reflect Timing-Allow-Origin, and it affects the > result of transferSize() > (https://w3c.github.io/resource-timing/#dom-performanceresourcetiming-transfer...). > > PerformanceNavigationTiming is passing true for the allowTimingDetails argument. > I see your TODO comment there. Don't we need to implement it now? > > I'm not yet so familiar with the resource timing and navigation timing. Tell me > if I'm misunderstanding. Jian has a separate CL for addressing this: https://codereview.chromium.org/2550883003/ Feel free to comment there. The implementation is still behind a runtime flag (test-only), so submitting these out of order is not a huge deal. https://codereview.chromium.org/2511313002/diff/400001/third_party/WebKit/Sou... third_party/WebKit/Source/core/fetch/ResourceFetcher.cpp:1176: } On 2016/12/09 07:35:58, tyoshino wrote: > don't we need to have the same protocol/status check as L1182? > > Here're the commits explaining why the check has been introduced: > https://chromium.googlesource.com/chromium/src/+/895ef0862cc0da80e9f6c2fb9b18... > https://chromium.googlesource.com/chromium/src/+/558203127f4c7ca76b2e02965eaa... > https://chromium.googlesource.com/chromium/src/+/9f72becdf7e9c050ec341d8356ec... > https://chromium.googlesource.com/chromium/src/+/2f9a97aa70c6386dce993735d7db... Adding isHttp makes sense. As for the status-code, the spec does not seem to indicate this (either way) , I'll defer to Yoav here.
https://codereview.chromium.org/2511313002/diff/400001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/fetch/ResourceFetcher.cpp (right): https://codereview.chromium.org/2511313002/diff/400001/third_party/WebKit/Sou... third_party/WebKit/Source/core/fetch/ResourceFetcher.cpp:1176: } On 2016/12/09 17:47:15, panicker wrote: > On 2016/12/09 07:35:58, tyoshino wrote: > > don't we need to have the same protocol/status check as L1182? > > > > Here're the commits explaining why the check has been introduced: > > > https://chromium.googlesource.com/chromium/src/+/895ef0862cc0da80e9f6c2fb9b18... > > > https://chromium.googlesource.com/chromium/src/+/558203127f4c7ca76b2e02965eaa... > > > https://chromium.googlesource.com/chromium/src/+/9f72becdf7e9c050ec341d8356ec... > > > https://chromium.googlesource.com/chromium/src/+/2f9a97aa70c6386dce993735d7db... > > Adding isHttp makes sense. As for the status-code, the spec does not seem to > indicate this (either way) , I'll defer to Yoav here. I agree isHTTP() here makes sense (even if it's significantly less of an issue in navigation requests than it is for resources, e.g. data URIs). Regarding status checks, I suggest to keep the current state (where transferSize reporting does not depend on status code) as lack of spec indication means we don't need to take it into account. We can potentially file a spec issue and ask for clarification on that front. Barring security implications (which I struggle to see here), I don't see why we would prevent reporting of size for navigation requests based on their status code.
Patchset #18 (id:420001) 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 yoav@yoav.ws Link to the patchset: https://codereview.chromium.org/2511313002/#ps440001 (title: "addressed comments")
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": 440001, "attempt_start_ts": 1481573122875550,
"parent_rev": "78006ea9583f9673726c0bbb0c1d1d92dfe812d6", "commit_rev":
"2dc9ed29e8994006fc6612ead3d88636482103fb"}
Message was sent while issue was closed.
Description was changed from ========== TransferSize implementation. BUG=663187 ========== to ========== TransferSize implementation. BUG=663187 Review-Url: https://codereview.chromium.org/2511313002 ==========
Message was sent while issue was closed.
Committed patchset #18 (id:440001)
Message was sent while issue was closed.
Description was changed from ========== TransferSize implementation. BUG=663187 Review-Url: https://codereview.chromium.org/2511313002 ========== to ========== TransferSize implementation. BUG=663187 Committed: https://crrev.com/aa066fd6ef4f2546faafdaf3abcb328fc6cc709c Cr-Commit-Position: refs/heads/master@{#437946} ==========
Message was sent while issue was closed.
Patchset 18 (id:??) landed as https://crrev.com/aa066fd6ef4f2546faafdaf3abcb328fc6cc709c Cr-Commit-Position: refs/heads/master@{#437946} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
