Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(45)

Issues Search
    My Issues | Starred     Open | Closed | All

Issue 1288973002: Observing DocumentTiming and sending data to RenderFrameImpl (Closed)

Created:
5 years, 4 months ago by Charlie Harrison
Modified:
5 years, 4 months ago
CC:
blink-reviews, blink-reviews-dom_chromium.org, dglazkov+blink, eae+blinkwatch, gavinp+loader_chromium.org, Nate Chapin, kinuko+watch, rwlbuis, tyoshino+watch_chromium.org
Base URL:
https://chromium.googlesource.com/chromium/blink.git@master
Target Ref:
refs/heads/master
Project:
blink
Visibility:
Public.

Description

Observing DocumentTiming and sending data to RenderFrameImpl Data from DocumentTiming must be pushed out of blink as it comes in, to avoid losing data from fast shut downs. We propogate these notifications to the WebFrameClient, for easy access outside of blink. BUG=382542 Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=200730

Patch Set 1 #

Patch Set 2 : remove unneeded webFrameClient() exposure #

Total comments: 9

Patch Set 3 : minor edits from review #

Total comments: 6

Patch Set 4 : more comments and one misplaced notifyDocumentTimingChanged() #

Patch Set 5 : Simplified Document/DocumentTiming abstraction #

Patch Set 6 : #

Patch Set 7 : minor fixes #

Total comments: 5

Patch Set 8 : DocumentLoadTime instrumentation + fixes #

Patch Set 9 : Header shuffling #

Total comments: 1

Patch Set 10 : Document logic moved to DocumentTiming #

Total comments: 2
Unified diffs Side-by-side diffs Delta from patch set Stats (+100 lines, -8 lines) Patch
M Source/core/dom/Document.h View 1 2 3 4 5 6 7 8 9 1 chunk +1 line, -0 lines 0 comments Download
M Source/core/dom/Document.cpp View 1 2 3 4 5 6 7 8 9 2 chunks +10 lines, -4 lines 0 comments Download
M Source/core/dom/DocumentTiming.h View 1 2 3 4 5 6 7 2 chunks +9 lines, -1 line 1 comment Download
M Source/core/dom/DocumentTiming.cpp View 1 2 3 4 5 6 7 8 9 1 chunk +16 lines, -1 line 0 comments Download
M Source/core/loader/DocumentLoadTiming.h View 1 2 3 4 5 6 7 8 3 chunks +7 lines, -1 line 0 comments Download
M Source/core/loader/DocumentLoadTiming.cpp View 1 2 3 4 5 6 7 8 5 chunks +19 lines, -1 line 0 comments Download
M Source/core/loader/DocumentLoader.h View 1 2 3 4 5 6 7 8 3 chunks +6 lines, -0 lines 0 comments Download
M Source/core/loader/DocumentLoader.cpp View 1 2 3 4 5 6 7 8 9 3 chunks +19 lines, -0 lines 1 comment Download
M Source/core/loader/FrameLoaderClient.h View 1 2 3 1 chunk +3 lines, -0 lines 0 comments Download
M Source/web/FrameLoaderClientImpl.h View 1 2 3 4 5 6 7 1 chunk +1 line, -0 lines 0 comments Download
M Source/web/FrameLoaderClientImpl.cpp View 1 2 3 4 5 6 7 8 9 1 chunk +6 lines, -0 lines 0 comments Download
M public/web/WebFrameClient.h View 1 2 3 1 chunk +3 lines, -0 lines 0 comments Download

Messages

Total messages: 32 (9 generated)
Bryan McQuade
This looks really good. Here are a few small change suggestions. https://codereview.chromium.org/1288973002/diff/20001/Source/core/dom/Document.cpp File Source/core/dom/Document.cpp (right): ...
5 years, 4 months ago (2015-08-14 11:59:09 UTC) #2
Charlie Harrison
https://codereview.chromium.org/1288973002/diff/20001/Source/core/loader/FrameLoaderClient.h File Source/core/loader/FrameLoaderClient.h (right): https://codereview.chromium.org/1288973002/diff/20001/Source/core/loader/FrameLoaderClient.h#newcode136 Source/core/loader/FrameLoaderClient.h:136: // Transmits the change in the set of watched ...
5 years, 4 months ago (2015-08-14 13:56:35 UTC) #3
Bryan McQuade
Looks good to me - let's address these and then you can send this to ...
5 years, 4 months ago (2015-08-14 15:14:16 UTC) #4
Bryan McQuade
https://codereview.chromium.org/1288973002/diff/40001/Source/core/dom/DocumentTiming.h File Source/core/dom/DocumentTiming.h (right): https://codereview.chromium.org/1288973002/diff/40001/Source/core/dom/DocumentTiming.h#newcode37 Source/core/dom/DocumentTiming.h:37: virtual void onDocumentTimingChanged() const = 0; On 2015/08/14 15:14:16, ...
5 years, 4 months ago (2015-08-14 15:23:24 UTC) #5
Nate Chapin
This strategy seems good. https://codereview.chromium.org/1288973002/diff/120001/Source/core/dom/Document.cpp File Source/core/dom/Document.cpp (right): https://codereview.chromium.org/1288973002/diff/120001/Source/core/dom/Document.cpp#newcode4497 Source/core/dom/Document.cpp:4497: WeakPtrWillBeRawPtr<Document> Document::weakPtrWillBeRawPtr() Should this have ...
5 years, 4 months ago (2015-08-14 20:54:56 UTC) #7
Charlie Harrison
Finishing touches to the Blink change. Mirrored functionality from DocumentTiming in DocumentLoadTiming.
5 years, 4 months ago (2015-08-17 13:56:44 UTC) #8
Nate Chapin
LGTM with a code health question https://codereview.chromium.org/1288973002/diff/160001/Source/core/dom/Document.cpp File Source/core/dom/Document.cpp (right): https://codereview.chromium.org/1288973002/diff/160001/Source/core/dom/Document.cpp#newcode5203 Source/core/dom/Document.cpp:5203: frame()->loader().client()->didChangePerformanceTiming(); It seems ...
5 years, 4 months ago (2015-08-17 18:17:21 UTC) #9
Charlie Harrison
On 2015/08/17 18:17:21, Nate Chapin wrote: > LGTM with a code health question > > ...
5 years, 4 months ago (2015-08-17 18:22:37 UTC) #10
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1288973002/180001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1288973002/180001
5 years, 4 months ago (2015-08-17 20:39:52 UTC) #13
commit-bot: I haz the power
Try jobs failed on following builders: win_chromium_rel_ng on tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_ng/builds/94248)
5 years, 4 months ago (2015-08-17 21:47:05 UTC) #15
commit-bot: I haz the power
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1288973002/180001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1288973002/180001
5 years, 4 months ago (2015-08-17 22:29:41 UTC) #17
commit-bot: I haz the power
Dry run: Try jobs failed on following builders: mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_ng/builds/100010)
5 years, 4 months ago (2015-08-17 23:53:12 UTC) #19
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1288973002/180001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1288973002/180001
5 years, 4 months ago (2015-08-18 15:21:04 UTC) #21
commit-bot: I haz the power
Committed patchset #10 (id:180001) as https://src.chromium.org/viewvc/blink?view=rev&revision=200730
5 years, 4 months ago (2015-08-18 15:25:55 UTC) #22
dcheng
I don't think this patch should have exposed WeakPtrs for Document or DocumentLoader. Can we ...
5 years, 4 months ago (2015-08-18 17:24:48 UTC) #24
Charlie Harrison
On 2015/08/18 17:24:48, dcheng wrote: > I don't think this patch should have exposed WeakPtrs ...
5 years, 4 months ago (2015-08-18 17:38:21 UTC) #25
dcheng
On 2015/08/18 at 17:38:21, csharrison wrote: > On 2015/08/18 17:24:48, dcheng wrote: > > I ...
5 years, 4 months ago (2015-08-18 17:39:21 UTC) #26
Charlie Harrison
On 2015/08/18 17:39:21, dcheng wrote: > On 2015/08/18 at 17:38:21, csharrison wrote: > > On ...
5 years, 4 months ago (2015-08-18 17:43:07 UTC) #27
sof
On 2015/08/18 17:39:21, dcheng wrote: > On 2015/08/18 at 17:38:21, csharrison wrote: > > On ...
5 years, 4 months ago (2015-08-18 17:43:22 UTC) #28
sof
On 2015/08/18 17:43:07, csharrison wrote: > On 2015/08/18 17:39:21, dcheng wrote: > > On 2015/08/18 ...
5 years, 4 months ago (2015-08-18 17:44:13 UTC) #29
Charlie Harrison
On 2015/08/18 17:44:13, sof wrote: > On 2015/08/18 17:43:07, csharrison wrote: > > On 2015/08/18 ...
5 years, 4 months ago (2015-08-18 17:57:49 UTC) #30
dcheng
On 2015/08/18 at 17:57:49, csharrison wrote: > On 2015/08/18 17:44:13, sof wrote: > > On ...
5 years, 4 months ago (2015-08-18 18:12:54 UTC) #31
dcheng
5 years, 4 months ago (2015-08-18 18:13:27 UTC) #32
Message was sent while issue was closed. 
On 2015/08/18 at 18:12:54, dcheng wrote:
> On 2015/08/18 at 17:57:49, csharrison wrote:
> > On 2015/08/18 17:44:13, sof wrote:
> > > On 2015/08/18 17:43:07, csharrison wrote:
> > > > On 2015/08/18 17:39:21, dcheng wrote:
> > > > > On 2015/08/18 at 17:38:21, csharrison wrote:
> > > > > > On 2015/08/18 17:24:48, dcheng wrote:
> > > > > > > I don't think this patch should have exposed WeakPtrs for Document
or
> > > > > > > DocumentLoader. Can we undo those changes?
> > > > > > > 
> > > > > > >
> > > > >
> > > >
> > >
https://codereview.chromium.org/1288973002/diff/180001/Source/core/dom/Docume...
> > > > > > > File Source/core/dom/DocumentTiming.h (right):
> > > > > > > 
> > > > > > >
> > > > >
> > > >
> > >
https://codereview.chromium.org/1288973002/diff/180001/Source/core/dom/Docume...
> > > > > > > Source/core/dom/DocumentTiming.h:37:
> > > > > > > DocumentTiming(WeakPtrWillBeRawPtr<Document>);
> > > > > > > Why does this need to be a WeakPtr? Isn't this owned by Document?
> > > > > > > 
> > > > > > >
> > > > >
> > > >
> > >
https://codereview.chromium.org/1288973002/diff/180001/Source/core/loader/Doc...
> > > > > > > File Source/core/loader/DocumentLoader.cpp (right):
> > > > > > > 
> > > > > > >
> > > > >
> > > >
> > >
https://codereview.chromium.org/1288973002/diff/180001/Source/core/loader/Doc...
> > > > > > > Source/core/loader/DocumentLoader.cpp:97: ,
> > > > > > > m_documentLoadTiming(this->weakReference())
> > > > > > > Ditto.
> > > > > > 
> > > > > > Nate suggested these be weak. Could the back-references to parent
objects
> > > > > delay Oilpan GC?
> > > > > 
> > > > > That won't delay GC: Oilpan knows how to collect cycles (actually,
this was
> > > > one
> > > > > of the motivations for Oilpan)
> > > > 
> > > > Ok great. Will upload another CL changing this and set you as reviewer.
> > > 
> > > Please Cc: oilpan-reviews also.
> > 
> > Just to clarify: looking at https://www.chromium.org/blink/blink-gc, the
pointers to the parent (GC'ed) objects should be e.g. Member<Document>
m_document? Or can it just be a raw pointer?
> 
> DocumentLoader is not on the Oilpan heap, so a raw pointer should be fine.
> 
> Document is on the Oilpan heap. A member pointer to an Oilpan object should be
Member<OilpanObject> m_pointer. But since we are still in the transition state
for Document, it would be a RawPtrWillBeMember<Document> m_document.

I lied: codesearch linked me to the wrong DocumentLoader. DocumentLoader is GCed
in Oilpan, so the same comments apply.

Powered by Google App Engine
This is Rietveld 408576698