Modified RenderFrameObserver API

Modified RenderFrameObserver API

We add a method to RenderFrameObserver in order to propogate
notifications out of Blink to chrome/. This is detailed in the design document https://docs.google.com/document/d/1bBO5JjQhOYjiLx_XpajIsV3vQt-h-0faKpQYM5dXdig

BUG=382542
Committed: https://crrev.com/a865aa3758fc96f66b86ec0e6ed1fc7b51ae5d59
Cr-Commit-Position: refs/heads/master@{#348224}

[Bryan McQuade, 2015-08-14 12:01:39]

bmcquade@chromium.org changed reviewers:
+ bmcquade@chromium.org

[Bryan McQuade, 2015-08-14 12:01:41]

https://codereview.chromium.org/1289053003/diff/1/content/public/renderer/ren...
File content/public/renderer/render_frame_observer.h (right):

https://codereview.chromium.org/1289053003/diff/1/content/public/renderer/ren...
content/public/renderer/render_frame_observer.h:105: // Notifications when
Performance data becomes available
for the comment, I'd reference the class name explicitly:
// Notifications when |PerformanceTiming| data becomes available.

https://codereview.chromium.org/1289053003/diff/1/content/public/renderer/ren...
content/public/renderer/render_frame_observer.h:106: virtual void
OnPerformanceTimingChanged() {}
In addition to this change, I'd also include the part of the CL where
RenderFrameImpl implements the new callback and invokes this new method on all
of its observers.

https://codereview.chromium.org/1289053003/diff/1/content/public/renderer/ren...
content/public/renderer/render_frame_observer.h:106: virtual void
OnPerformanceTimingChanged() {}
in keeping with the other naming, maybe call this DidChangePerformanceTiming?

[Charlie Harrison, 2015-08-18 21:02:32]

csharrison@chromium.org changed reviewers:
+ kinuko@chromium.org
- bmcquade@chromium.org

[Charlie Harrison, 2015-08-18 21:06:04]

This will glue my blink changes https://codereview.chromium.org/1288973002/ 

to Bryan's CL

[kinuko, 2015-08-19 04:18:15]

It's not really about this change but one question: this doesn't really tell
what performance timing value is changed, which means that consumer of this API
needs to go through available fields and track what's sent for the document or
not, which feels a little less scalable.  Any reason you prefer this style? 
Have we considered using enum etc to notify that which value is updated?

Also: is it ok that we're eventually going to replace DocumentState's timing
info?

[Charlie Harrison, 2015-08-19 13:28:39]

On 2015/08/19 04:18:15, kinuko wrote:
> It's not really about this change but one question: this doesn't really tell
> what performance timing value is changed, which means that consumer of this
API
> needs to go through available fields and track what's sent for the document or
> not, which feels a little less scalable.  Any reason you prefer this style? 
> Have we considered using enum etc to notify that which value is updated?
> 
> Also: is it ok that we're eventually going to replace DocumentState's timing
> info?

Yeah Bryan and I discussed sending data through the observers, but settled on
this
for simplicity. The goal was to make the renderer changes as simple and general 
as possible, and make our consumer of the API responsible for all the logic. 
This style also matches the rest of RenderFrameObserver's API. 

What about DocumentState's timing are you changing? The data will be coming from
DocumentTiming and DocumentLoadTiming.

[kinuko, 2015-08-20 02:01:24]

On 2015/08/19 13:28:39, csharrison wrote:
> On 2015/08/19 04:18:15, kinuko wrote:
> > It's not really about this change but one question: this doesn't really tell
> > what performance timing value is changed, which means that consumer of this
> API
> > needs to go through available fields and track what's sent for the document
or
> > not, which feels a little less scalable.  Any reason you prefer this style? 
> > Have we considered using enum etc to notify that which value is updated?
> > 
> > Also: is it ok that we're eventually going to replace DocumentState's timing
> > info?
> 
> Yeah Bryan and I discussed sending data through the observers, but settled on
> this
> for simplicity. The goal was to make the renderer changes as simple and
general 
> as possible, and make our consumer of the API responsible for all the logic. 
> This style also matches the rest of RenderFrameObserver's API. 

I see, I started to understand that several decisions have made before we have
this API. I think I have some comments in the current design (and looks like
some other people may have too), I'd like to wait until the discussion on
bryan's other CL settles.

> What about DocumentState's timing are you changing? The data will be coming
from
> DocumentTiming and DocumentLoadTiming.

Not really, I'm just hoping that we'll have single well-designed page metrics
logging system so that we can deprecate old PLT logging code eventually.

[Charlie Harrison, 2015-08-20 13:27:07]

On 2015/08/20 02:01:24, kinuko wrote:
> On 2015/08/19 13:28:39, csharrison wrote:
> > On 2015/08/19 04:18:15, kinuko wrote:
> > > It's not really about this change but one question: this doesn't really
tell
> > > what performance timing value is changed, which means that consumer of
this
> > API
> > > needs to go through available fields and track what's sent for the
document
> or
> > > not, which feels a little less scalable.  Any reason you prefer this
style? 
> > > Have we considered using enum etc to notify that which value is updated?
> > > 
> > > Also: is it ok that we're eventually going to replace DocumentState's
timing
> > > info?
> > 
> > Yeah Bryan and I discussed sending data through the observers, but settled
on
> > this
> > for simplicity. The goal was to make the renderer changes as simple and
> general 
> > as possible, and make our consumer of the API responsible for all the logic.

> > This style also matches the rest of RenderFrameObserver's API. 
> 
> I see, I started to understand that several decisions have made before we have
> this API. I think I have some comments in the current design (and looks like
> some other people may have too), I'd like to wait until the discussion on
> bryan's other CL settles.
> 
> > What about DocumentState's timing are you changing? The data will be coming
> from
> > DocumentTiming and DocumentLoadTiming.
> 
> Not really, I'm just hoping that we'll have single well-designed page metrics
> logging system so that we can deprecate old PLT logging code eventually.

Sounds good to me. Let's move discussion to Bryan's CL.

[Charlie Harrison, 2015-09-03 21:39:43]

On 2015/08/20 13:27:07, csharrison wrote:
> On 2015/08/20 02:01:24, kinuko wrote:
> > On 2015/08/19 13:28:39, csharrison wrote:
> > > On 2015/08/19 04:18:15, kinuko wrote:
> > > > It's not really about this change but one question: this doesn't really
> tell
> > > > what performance timing value is changed, which means that consumer of
> this
> > > API
> > > > needs to go through available fields and track what's sent for the
> document
> > or
> > > > not, which feels a little less scalable.  Any reason you prefer this
> style? 
> > > > Have we considered using enum etc to notify that which value is updated?
> > > > 
> > > > Also: is it ok that we're eventually going to replace DocumentState's
> timing
> > > > info?
> > > 
> > > Yeah Bryan and I discussed sending data through the observers, but settled
> on
> > > this
> > > for simplicity. The goal was to make the renderer changes as simple and
> > general 
> > > as possible, and make our consumer of the API responsible for all the
logic.
> 
> > > This style also matches the rest of RenderFrameObserver's API. 
> > 
> > I see, I started to understand that several decisions have made before we
have
> > this API. I think I have some comments in the current design (and looks like
> > some other people may have too), I'd like to wait until the discussion on
> > bryan's other CL settles.
> > 
> > > What about DocumentState's timing are you changing? The data will be
coming
> > from
> > > DocumentTiming and DocumentLoadTiming.
> > 
> > Not really, I'm just hoping that we'll have single well-designed page
metrics
> > logging system so that we can deprecate old PLT logging code eventually.
> 
> Sounds good to me. Let's move discussion to Bryan's CL.

Hi Kinuko, I have migrated and updated Bryan's CL here
https://codereview.chromium.org/1312213010/ but we need this change to move
forward on it. Our new design has us sending the entire timing struct over IPC,
rather than just the updated value, so this API makes more sense and won't
produce a lot of boilerplate code. The updated design doc has more information:
https://docs.google.com/document/d/1HJsJ5W2H_3qRdqPAUgAEo10AF8gXPTXZLUET4X4_s....

[kinuko (google), 2015-09-09 21:46:16]

On 2015/09/03 21:39:43, csharrison wrote:
> On 2015/08/20 13:27:07, csharrison wrote:
> > On 2015/08/20 02:01:24, kinuko wrote:
> > > On 2015/08/19 13:28:39, csharrison wrote:
> > > > On 2015/08/19 04:18:15, kinuko wrote:
> > > > > It's not really about this change but one question: this doesn't
really
> > tell
> > > > > what performance timing value is changed, which means that consumer of
> > this
> > > > API
> > > > > needs to go through available fields and track what's sent for the
> > document
> > > or
> > > > > not, which feels a little less scalable.  Any reason you prefer this
> > style? 
> > > > > Have we considered using enum etc to notify that which value is
updated?
> > > > > 
> > > > > Also: is it ok that we're eventually going to replace DocumentState's
> > timing
> > > > > info?
> > > > 
> > > > Yeah Bryan and I discussed sending data through the observers, but
settled
> > on
> > > > this
> > > > for simplicity. The goal was to make the renderer changes as simple and
> > > general 
> > > > as possible, and make our consumer of the API responsible for all the
> logic.
> > 
> > > > This style also matches the rest of RenderFrameObserver's API. 
> > > 
> > > I see, I started to understand that several decisions have made before we
> have
> > > this API. I think I have some comments in the current design (and looks
like
> > > some other people may have too), I'd like to wait until the discussion on
> > > bryan's other CL settles.
> > > 
> > > > What about DocumentState's timing are you changing? The data will be
> coming
> > > from
> > > > DocumentTiming and DocumentLoadTiming.
> > > 
> > > Not really, I'm just hoping that we'll have single well-designed page
> metrics
> > > logging system so that we can deprecate old PLT logging code eventually.
> > 
> > Sounds good to me. Let's move discussion to Bryan's CL.
> 
> Hi Kinuko, I have migrated and updated Bryan's CL here
> https://codereview.chromium.org/1312213010/ but we need this change to move
> forward on it. Our new design has us sending the entire timing struct over
IPC,
> rather than just the updated value, so this API makes more sense and won't
> produce a lot of boilerplate code. The updated design doc has more
information:
>
https://docs.google.com/document/d/1HJsJ5W2H_3qRdqPAUgAEo10AF8gXPTXZLUET4X4_s....

Apologies for my slow review!  Yes this makes more sense now.

[kinuko (google), 2015-09-09 21:46:27]

On 2015/09/09 21:46:16, kinuko (google) wrote:
> On 2015/09/03 21:39:43, csharrison wrote:
> > On 2015/08/20 13:27:07, csharrison wrote:
> > > On 2015/08/20 02:01:24, kinuko wrote:
> > > > On 2015/08/19 13:28:39, csharrison wrote:
> > > > > On 2015/08/19 04:18:15, kinuko wrote:
> > > > > > It's not really about this change but one question: this doesn't
> really
> > > tell
> > > > > > what performance timing value is changed, which means that consumer
of
> > > this
> > > > > API
> > > > > > needs to go through available fields and track what's sent for the
> > > document
> > > > or
> > > > > > not, which feels a little less scalable.  Any reason you prefer this
> > > style? 
> > > > > > Have we considered using enum etc to notify that which value is
> updated?
> > > > > > 
> > > > > > Also: is it ok that we're eventually going to replace
DocumentState's
> > > timing
> > > > > > info?
> > > > > 
> > > > > Yeah Bryan and I discussed sending data through the observers, but
> settled
> > > on
> > > > > this
> > > > > for simplicity. The goal was to make the renderer changes as simple
and
> > > > general 
> > > > > as possible, and make our consumer of the API responsible for all the
> > logic.
> > > 
> > > > > This style also matches the rest of RenderFrameObserver's API. 
> > > > 
> > > > I see, I started to understand that several decisions have made before
we
> > have
> > > > this API. I think I have some comments in the current design (and looks
> like
> > > > some other people may have too), I'd like to wait until the discussion
on
> > > > bryan's other CL settles.
> > > > 
> > > > > What about DocumentState's timing are you changing? The data will be
> > coming
> > > > from
> > > > > DocumentTiming and DocumentLoadTiming.
> > > > 
> > > > Not really, I'm just hoping that we'll have single well-designed page
> > metrics
> > > > logging system so that we can deprecate old PLT logging code eventually.
> > > 
> > > Sounds good to me. Let's move discussion to Bryan's CL.
> > 
> > Hi Kinuko, I have migrated and updated Bryan's CL here
> > https://codereview.chromium.org/1312213010/ but we need this change to move
> > forward on it. Our new design has us sending the entire timing struct over
> IPC,
> > rather than just the updated value, so this API makes more sense and won't
> > produce a lot of boilerplate code. The updated design doc has more
> information:
> >
>
https://docs.google.com/document/d/1HJsJ5W2H_3qRdqPAUgAEo10AF8gXPTXZLUET4X4_s....
> 
> Apologies for my slow review!  Yes this makes more sense now.

LGTM

[Charlie Harrison, 2015-09-09 22:23:36]

The CQ bit was checked by csharrison@chromium.org

[commit-bot: I haz the power, 2015-09-09 22:23:43]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1289053003/20001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1289053003/20001

[commit-bot: I haz the power, 2015-09-09 22:23:45]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-09-09 22:23:45]

No L-G-T-M from a valid reviewer yet. Only full committers are accepted.
Even if an L-G-T-M may have been provided, it was from a non-committer,
_not_ a full super star committer.
See http://www.chromium.org/getting-involved/become-a-committer
Note that this has nothing to do with OWNERS files.

[Charlie Harrison, 2015-09-09 22:28:12]

On 2015/09/09 22:23:45, commit-bot: I haz the power wrote:
> No L-G-T-M from a valid reviewer yet. Only full committers are accepted.
> Even if an L-G-T-M may have been provided, it was from a non-committer,
> _not_ a full super star committer.
> See http://www.chromium.org/getting-involved/become-a-committer
> Note that this has nothing to do with OWNERS files.

Hey kinuko, can you lgtm from your chromium account?

[kinuko, 2015-09-10 02:03:42]

lgtm.

[kinuko, 2015-09-10 02:05:23]

The CQ bit was checked by kinuko@chromium.org

[commit-bot: I haz the power, 2015-09-10 02:05:37]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1289053003/20001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1289053003/20001

[Charlie Harrison, 2015-09-10 02:48:56]

On 2015/09/10 02:05:37, commit-bot: I haz the power wrote:
> CQ is trying da patch. Follow status at
>  https://chromium-cq-status.appspot.com/patch-status/1289053003/20001
> View timeline at
>  https://chromium-cq-status.appspot.com/patch-timeline/1289053003/20001

thanks :)

[commit-bot: I haz the power, 2015-09-10 03:22:48]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-09-10 03:22:49]

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_...)

[Charlie Harrison, 2015-09-10 20:26:44]

The CQ bit was checked by csharrison@chromium.org

[commit-bot: I haz the power, 2015-09-10 20:28:01]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1289053003/20001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1289053003/20001

[commit-bot: I haz the power, 2015-09-10 20:34:20]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2015-09-10 20:35:31]

Patchset 2 (id:??) landed as
https://crrev.com/a865aa3758fc96f66b86ec0e6ed1fc7b51ae5d59
Cr-Commit-Position: refs/heads/master@{#348224}

[commit-bot: I haz the power, 2015-09-23 12:13:42]

Patchset 2 (id:??) landed as
https://crrev.com/a865aa3758fc96f66b86ec0e6ed1fc7b51ae5d59
Cr-Commit-Position: refs/heads/master@{#348224}