Bug 61152

Created attachment 94141 [details] Patch

8 months since this was posted. No comment. Do we really wan this? or can this bug be closed.

I think we can move forward again. We had the W3C security and privacy teams review it. I think Tony was going to report the results to the old thread on webkit-dev. Maybe he already did. I can post a link in the metabug too. FYI, I'm planning to finish up the implementation this quarter.

Created attachment 139121 [details] Patch

Comment on attachment 139121 [details] Patch Attachment 139121 [details] did not pass qt-wk2-ews (qt): Output: http://queues.webkit.org/results/12560070

Comment on attachment 139121 [details] Patch Attachment 139121 [details] did not pass gtk-ews (gtk): Output: http://queues.webkit.org/results/12549076

Created attachment 139126 [details] Patch

Comment on attachment 139126 [details] Patch Attachment 139126 [details] did not pass qt-wk2-ews (qt): Output: http://queues.webkit.org/results/12554084

Created attachment 139128 [details] Patch

Comment on attachment 139128 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=139128&action=review Setting r- for now until we figure out the comment about initiator type/time. Everything else is nit-picky. > Source/WebCore/ChangeLog:6 > + This patch implements the Resource Timing interface. It doesn't do anything I recommend a link to the spec in the ChangeLog. > Source/WebCore/page/Performance.cpp:63 > + return frame()->document()->scriptExecutionContext(); Are we guaranteed a Frame here? Also, Document is a ScriptExecutionContext, so is the scriptExecutionContext() call doing anything useful? > Source/WebCore/page/Performance.cpp:94 > + for (Vector<RefPtr<PerformanceEntry> >::const_iterator resource = m_resourceTimingBuffer.begin(); resource != m_resourceTimingBuffer.end(); ++resource) Vector does have an append() that takes a Vector and appends all. Would it be cleaner to avoid the iterator here and modify PerformanceEntryList to take advantage of that? We seem to append all more than once. > Source/WebCore/page/Performance.cpp:138 > +{ Add FIXME > Source/WebCore/page/Performance.h:43 > +#include "ResourceResponse.h" Can these both be forward declared? > Source/WebCore/page/PerformanceResourceTiming.cpp:50 > + case ResourceRequest::InitiatorIsCSS: There was a recent thread about these. Are they being reworked in the spec? > Source/WebCore/page/PerformanceResourceTiming.cpp:68 > + case ResourceRequest::InitiatorIsUnknown: Are there known unknowns? If so, this shouldn't hit the ASSERT_NOT_REACHED(). > Source/WebCore/page/PerformanceResourceTiming.h:36 > +#include "Performance.h" Unused? > Source/WebCore/page/PerformanceResourceTiming.h:38 > +#include "ResourceLoadTiming.h" I always forget whether it is okay to forward declare something used as a RefPtr. You have it one way for Document and another way for ResourceLoadTiming. So either this can be forward declared or else we are pulling in Document.h transitively and it should be explicit. > Source/WebCore/page/PerformanceResourceTiming.h:39 > +#include "ResourceRequest.h" Forward declare like ResourceResponse? > Source/WebCore/page/PerformanceResourceTiming.h:41 > +#include <wtf/RefCounted.h> Should be wtf/RefPtr.h, right? And also add wtf/text/WTFString.h. > Source/WebCore/platform/network/ResourceRequestBase.h:153 > + double initiatedTime() const { return m_initiatedTime; } The devtools timeline seems to already know initiatedTime and initiatorType. DevTools and ResourceTiming should probably share the same data store for this information. Have you looked into how that works and whether any of these ResourceRequest modifications are absolutely necessary? > Source/WebCore/platform/network/ResourceRequestBase.h:214 > + double m_initiatedTime; Worth enforcing const here?

Created attachment 146451 [details] Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=139128&action=review >> Source/WebCore/ChangeLog:6 >> + This patch implements the Resource Timing interface. It doesn't do anything > > I recommend a link to the spec in the ChangeLog. Done. >> Source/WebCore/page/Performance.cpp:63 >> + return frame()->document()->scriptExecutionContext(); > > Are we guaranteed a Frame here? > > Also, Document is a ScriptExecutionContext, so is the scriptExecutionContext() call doing anything useful? Fixed both. >> Source/WebCore/page/Performance.cpp:94 >> + for (Vector<RefPtr<PerformanceEntry> >::const_iterator resource = m_resourceTimingBuffer.begin(); resource != m_resourceTimingBuffer.end(); ++resource) > > Vector does have an append() that takes a Vector and appends all. Would it be cleaner to avoid the iterator here and modify PerformanceEntryList to take advantage of that? We seem to append all more than once. Good idea. Done. >> Source/WebCore/page/Performance.cpp:138 >> +{ > > Add FIXME Done. >> Source/WebCore/page/Performance.h:43 >> +#include "ResourceResponse.h" > > Can these both be forward declared? Yep. >> Source/WebCore/page/PerformanceResourceTiming.cpp:50 >> + case ResourceRequest::InitiatorIsCSS: > > There was a recent thread about these. Are they being reworked in the spec? They are. Should we match the current spec or what it's going to be? I could file a bug for the latter. >> Source/WebCore/page/PerformanceResourceTiming.cpp:68 >> + case ResourceRequest::InitiatorIsUnknown: > > Are there known unknowns? If so, this shouldn't hit the ASSERT_NOT_REACHED(). No, AFAICT it shouldn't be reached. >> Source/WebCore/page/PerformanceResourceTiming.h:36 >> +#include "Performance.h" > > Unused? Yep. >> Source/WebCore/page/PerformanceResourceTiming.h:38 >> +#include "ResourceLoadTiming.h" > > I always forget whether it is okay to forward declare something used as a RefPtr. You have it one way for Document and another way for ResourceLoadTiming. So either this can be forward declared or else we are pulling in Document.h transitively and it should be explicit. Forward declaration seems fine so I made both that. I put a destructor in the .cpp where both are fully defined to be sure it'll work. >> Source/WebCore/page/PerformanceResourceTiming.h:39 >> +#include "ResourceRequest.h" > > Forward declare like ResourceResponse? Done. >> Source/WebCore/page/PerformanceResourceTiming.h:41 >> +#include <wtf/RefCounted.h> > > Should be wtf/RefPtr.h, right? And also add wtf/text/WTFString.h. Done. >> Source/WebCore/platform/network/ResourceRequestBase.h:153 >> + double initiatedTime() const { return m_initiatedTime; } > > The devtools timeline seems to already know initiatedTime and initiatorType. DevTools and ResourceTiming should probably share the same data store for this information. > > Have you looked into how that works and whether any of these ResourceRequest modifications are absolutely necessary? I removed initiatedTime. Inspector has two relevant fields to initiatorType: initiator and type. Initiator comes from inspecting state (is the parser running, is there a JS callstack). Type comes from the CachedResourceType. That's insufficient for our needs, because it can't identify the source element. So, I think we still need to add initiatorType. Not sure if Inspector type should switch to initiatorType. It'll probably become harder once that's a node name though. >> Source/WebCore/platform/network/ResourceRequestBase.h:214 >> + double m_initiatedTime; > > Worth enforcing const here? No longer there.

Comment on attachment 146451 [details] Patch Attachment 146451 [details] did not pass qt-wk2-ews (qt): Output: http://queues.webkit.org/results/12908555

Comment on attachment 146451 [details] Patch Attachment 146451 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/12908559

Created attachment 146466 [details] Patch

Comment on attachment 146466 [details] Patch Attachment 146466 [details] did not pass qt-wk2-ews (qt): Output: http://queues.webkit.org/results/12921405

Comment on attachment 146466 [details] Patch Attachment 146466 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/12910537

Comment on attachment 146466 [details] Patch Attachment 146466 [details] did not pass efl-ews (efl): Output: http://queues.webkit.org/results/12908610

Created attachment 147155 [details] Patch

Created attachment 147721 [details] Patch

Comment on attachment 147721 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=147721&action=review > Source/WebCore/ChangeLog:12611 > +2012-04-25 James Simonsen <simonjam@chromium.org> Merge problem > Source/WebCore/page/PerformanceResourceTiming.cpp:107 > + connectStart = m_timing->dnsEnd; A lot of these conditions remind me of similar ones in Navigation Timing that should probably stay in sync in the future. I don't have a specific suggestion, but Is there any reasonable way to factor out any commonality between the two?

Created attachment 148852 [details] Patch for landing

Comment on attachment 148852 [details] Patch for landing Clearing flags on attachment: 148852 Committed r120962: <http://trac.webkit.org/changeset/120962>

All reviewed patches have been landed. Closing bug.