Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
321373003:
Changing animate to beginFrame and use struct rather than naked double to allow extension. (Closed)
|
Created:
6 years, 6 months ago by mithro-old Modified:
6 years, 5 months ago CC:
darktears, blink-reviews, blink-reviews-animation_chromium.org, dglazkov+blink, dstockwell, Eric Willigers, jamesr, Mike Lawther (Google), rjwright, Steve Block, Timothy Loh Base URL:
https://chromium.googlesource.com/chromium/blink.git@master Visibility:
Public. |
DescriptionChanging animate to beginFrame and use struct rather than naked double to allow extension.
This change makes it significantly easier to extend the interface for the needs for the Blink scheduler, web animations engine and future similar changes. It also brings the concept in Blink closer to the values in Chrome.
BUG=346230
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=178493
Patch Set 1 #
Total comments: 9
Patch Set 2 : Fixing Adam+Shane's feedback. #
Total comments: 4
Patch Set 3 : Adding missing <limit> include. #Patch Set 4 : Rebasing onto master. #
Total comments: 1
Patch Set 5 : Rebase onto origin/master. #Patch Set 6 : Refactoring and reducing this patch based on nduca's suggestions. #
Total comments: 6
Patch Set 7 : Fixing Sami's suggestions. #
Total comments: 2
Patch Set 8 : Using const reference. #Patch Set 9 : Reducing the surface area of this patch even further. #
Total comments: 11
Patch Set 10 : Fixing review comments. #Patch Set 11 : Adding another FIXME comment. #
Messages
Total messages: 37 (0 generated)
Hi Adam, This patch isn't quite ready, but wanted to get it in front of someone to make sure it wasn't totally heading down the wrong path. Tim
Seems reasonable. https://codereview.chromium.org/321373003/diff/1/public/platform/WebFrameTime.h File public/platform/WebFrameTime.h (right): https://codereview.chromium.org/321373003/diff/1/public/platform/WebFrameTime... public/platform/WebFrameTime.h:39: WebFrameTime(double flt, uint64_t d, uint64_t fdt) Please use complete words in variable names. https://codereview.chromium.org/321373003/diff/1/public/platform/WebFrameTime... public/platform/WebFrameTime.h:46: // FIXME(mithro): Upgrade frame_time, deadline and frame_dispaly_time to TimeTick class. frame_time -> frameTime frame_dispaly_time -> frameDisplayTime https://codereview.chromium.org/321373003/diff/1/public/platform/WebFrameTime... public/platform/WebFrameTime.h:49: double frameLastTime; lastFrameTime? https://codereview.chromium.org/321373003/diff/1/public/platform/WebFrameTime... public/platform/WebFrameTime.h:60: inline bool frameDisplayTimeValid() The keyword |inline| doesn't do anything there. https://codereview.chromium.org/321373003/diff/1/public/platform/WebFrameTime... public/platform/WebFrameTime.h:65: inline double frameTime() ditto
When ready, who would be the correct person to review? Tim 'mithro' Ansell On 12 Jun 2014 05:33, <abarth@chromium.org> wrote: > Seems reasonable. > > > https://codereview.chromium.org/321373003/diff/1/public/ > platform/WebFrameTime.h > File public/platform/WebFrameTime.h (right): > > https://codereview.chromium.org/321373003/diff/1/public/ > platform/WebFrameTime.h#newcode39 > public/platform/WebFrameTime.h:39: WebFrameTime(double flt, uint64_t d, > uint64_t fdt) > Please use complete words in variable names. > > https://codereview.chromium.org/321373003/diff/1/public/ > platform/WebFrameTime.h#newcode46 > public/platform/WebFrameTime.h:46: // FIXME(mithro): Upgrade frame_time, > deadline and frame_dispaly_time to TimeTick class. > frame_time -> frameTime > frame_dispaly_time -> frameDisplayTime > > https://codereview.chromium.org/321373003/diff/1/public/ > platform/WebFrameTime.h#newcode49 > public/platform/WebFrameTime.h:49: double frameLastTime; > lastFrameTime? > > https://codereview.chromium.org/321373003/diff/1/public/ > platform/WebFrameTime.h#newcode60 > public/platform/WebFrameTime.h:60: inline bool frameDisplayTimeValid() > The keyword |inline| doesn't do anything there. > > https://codereview.chromium.org/321373003/diff/1/public/ > platform/WebFrameTime.h#newcode65 > public/platform/WebFrameTime.h:65: inline double frameTime() > ditto > > https://codereview.chromium.org/321373003/ > To unsubscribe from this group and stop receiving emails from it, send an email to blink-reviews+unsubscribe@chromium.org.
On 2014/06/11 at 21:50:39, mithro wrote: > When ready, who would be the correct person to review? Probably jamesr if he's willing.
On 2014/06/11 21:52:39, abarth wrote: > On 2014/06/11 at 21:50:39, mithro wrote: > > When ready, who would be the correct person to review? > > Probably jamesr if he's willing. This is looking good - having regular frame times throughout the system is going to be really nice for animations. Something else that would help is having a predicted next frame time.
Hello James, I have fixed the comments from Adam and Shane had, can you please review this CL for landing? The aim here is to upgrade the animation interfaces between Chrome and Blink to provide the extra information needed to Blink about rendering (such as the deadline, time to render for and predicted next time too). This is all part of the goal to make sure everything is using the same time for rendering and also work needed for the Blink scheduler idea of Eric's. I'm working on the Chrome side version of this too and will send that out when ready but don't want to keep Blink waiting on those changes. Tim 'mithro' Ansell https://codereview.chromium.org/321373003/diff/1/public/platform/WebFrameTime.h File public/platform/WebFrameTime.h (right): https://codereview.chromium.org/321373003/diff/1/public/platform/WebFrameTime... public/platform/WebFrameTime.h:39: WebFrameTime(double flt, uint64_t d, uint64_t fdt) On 2014/06/11 19:33:01, abarth wrote: > Please use complete words in variable names. Done. https://codereview.chromium.org/321373003/diff/1/public/platform/WebFrameTime... public/platform/WebFrameTime.h:49: double frameLastTime; On 2014/06/11 19:33:01, abarth wrote: > lastFrameTime? Done. https://codereview.chromium.org/321373003/diff/1/public/platform/WebFrameTime... public/platform/WebFrameTime.h:60: inline bool frameDisplayTimeValid() On 2014/06/11 19:33:01, abarth wrote: > The keyword |inline| doesn't do anything there. I removed these functions and just made sure displayTime was always populated (with lastFrameTime if we don't know otherwise). https://codereview.chromium.org/321373003/diff/1/public/platform/WebFrameTime... public/platform/WebFrameTime.h:65: inline double frameTime() On 2014/06/11 19:33:01, abarth wrote: > ditto Done.
Where are these numbers going to come from on the chromium side?
On 2014/06/18 05:49:38, jamesr wrote: > Where are these numbers going to come from on the chromium side? These numbers come from the compositor scheduler. The BeginFrameArgs structure has both the lastFrameTime and renderDeadline values. I have a patch which makes the compositor pass the BeginFrameArgs down to the Chrome/Blink boundary but am currently trying to solve a couple of other yaks before I want to land that. displayFrameTime will be equivalent to lastFrameTime until I finish making the whole compositor stack reliably use the same/right frame time. Then we will start looking at using a different time to pass in. See my talk at BlinkOn about the possibilities of what times this could end up being. predicatedNextFrameTime will be created from the scheduler based on the BeginFrameArgs (and will probably end up in the BeginFrameArgs structure as well). I again have code which does filtering and produces a nice stable prediction but until everything is using the right time it is kind of a moot point. Does that all make sense? Tim 'mithro' Ansell
On 2014/06/18 06:03:26, mithro wrote: > On 2014/06/18 05:49:38, jamesr wrote: > > Where are these numbers going to come from on the chromium side? > > These numbers come from the compositor scheduler. > > The BeginFrameArgs structure has both the lastFrameTime and renderDeadline > values. I have a patch which makes the compositor pass the BeginFrameArgs down > to the Chrome/Blink boundary but am currently trying to solve a couple of other > yaks before I want to land that. > > displayFrameTime will be equivalent to lastFrameTime until I finish making the > whole compositor stack reliably use the same/right frame time. Then we will > start looking at using a different time to pass in. See my talk at BlinkOn about > the possibilities of what times this could end up being. > > predicatedNextFrameTime will be created from the scheduler based on the > BeginFrameArgs (and will probably end up in the BeginFrameArgs structure as > well). I again have code which does filtering and produces a nice stable > prediction but until everything is using the right time it is kind of a moot > point. > > Does that all make sense? > > Tim 'mithro' Ansell Friendly ping. Tim
Hi, Stumbled across this CL while browsing. I'm new to Chrome so my intuitions may be off base! Simon https://codereview.chromium.org/321373003/diff/40001/Source/core/page/Autoscr... File Source/core/page/AutoscrollController.cpp (right): https://codereview.chromium.org/321373003/diff/40001/Source/core/page/Autoscr... Source/core/page/AutoscrollController.cpp:228: if (frameTime.displayFrameTime - m_dragAndDropAutoscrollStartTime > autoscrollDelay) Not sure on the rules as to why WebFrametime is a struct as opposed to a class, but it might be nice to add a timeElapsedSince() function to WebFrameTime to avoid exposing implementation details and improve readability, so this line would become: if (frameTime.timeElapsedSince(m_dragAndDropAutoscrollStartTime) > autoscrollDelay) https://codereview.chromium.org/321373003/diff/40001/Source/platform/exported... File Source/platform/exported/WebActiveGestureAnimation.cpp (right): https://codereview.chromium.org/321373003/diff/40001/Source/platform/exported... Source/platform/exported/WebActiveGestureAnimation.cpp:65: return m_curve->apply(time - m_startTime, m_target); As per previous comment this would become: return m_curve->apply( frameTime.timeElapsedSince(m_StartTime) ... ). As a knock on we'd no longer need the 'double time' local, instead reading it directly into m_startTime.
Hi Simon, This code is a little different because the public/ directory of Blink contains the API between Blink and Chrome and thus is suppose to avoid actual implementation. Most of these interfaces are either structures or abstract classes with virtual methods. I'm no expert on this area however. Tim https://codereview.chromium.org/321373003/diff/40001/Source/core/page/Autoscr... File Source/core/page/AutoscrollController.cpp (right): https://codereview.chromium.org/321373003/diff/40001/Source/core/page/Autoscr... Source/core/page/AutoscrollController.cpp:228: if (frameTime.displayFrameTime - m_dragAndDropAutoscrollStartTime > autoscrollDelay) On 2014/06/24 10:10:26, picksi wrote: > Not sure on the rules as to why WebFrametime is a struct as opposed to a class, > but it might be nice to add a timeElapsedSince() function to WebFrameTime to > avoid exposing implementation details and improve readability, so this line > would become: > if (frameTime.timeElapsedSince(m_dragAndDropAutoscrollStartTime) > > autoscrollDelay) See my previous comments. https://codereview.chromium.org/321373003/diff/40001/Source/platform/exported... File Source/platform/exported/WebActiveGestureAnimation.cpp (right): https://codereview.chromium.org/321373003/diff/40001/Source/platform/exported... Source/platform/exported/WebActiveGestureAnimation.cpp:65: return m_curve->apply(time - m_startTime, m_target); On 2014/06/24 10:10:26, picksi wrote: > As per previous comment this would become: > return m_curve->apply( frameTime.timeElapsedSince(m_StartTime) ... ). > > As a knock on we'd no longer need the 'double time' local, instead reading it > directly into m_startTime. See previous comment.
Thanks Tim, this looks super useful! https://codereview.chromium.org/321373003/diff/80001/public/platform/WebFrame... File public/platform/WebFrameTime.h (right): https://codereview.chromium.org/321373003/diff/80001/public/platform/WebFrame... public/platform/WebFrameTime.h:38: struct WebFrameTime { Any reason not just to call this WebBeginFrameArgs and pass in the same data as we get through BeginFrame? Right now this is almost exactly the same except we have just one predicted frame time instead of an interval. To be clear I'm not opposed to calling this WebFrameTime, but let's make it compatible with BeginFrameArgs.
Hi Sami, Sorry about the brevity, writing this on my phone so you get this before another 24 hour cycle. I did initially start with BeginFrameArgs but with the extra members, different member types and members I wanted to remove it quickly diverged so I decided to use a different name so they weren't confused. One part that I don't want to inherit from BeginFrameArgs is the interval member. The actual value of that member is very low, while the misuse potential is *very* high. For example, the actual render interval is also non-fixed (for example when background ticking). It easy to see how some one could start using (lastTime + interval * 2) in their code. The deadline and render and next render times give you the actual values you want without the likelihood of you getting it wrong. Happy to explain further. Tim 'mithro' Ansell Thanks Tim, this looks super useful! https://codereview.chromium.org/321373003/diff/80001/ public/platform/WebFrameTime.h File public/platform/WebFrameTime.h (right): https://codereview.chromium.org/321373003/diff/80001/ public/platform/WebFrameTime.h#newcode38 public/platform/WebFrameTime.h:38: struct WebFrameTime { Any reason not just to call this WebBeginFrameArgs and pass in the same data as we get through BeginFrame? Right now this is almost exactly the same except we have just one predicted frame time instead of an interval. To be clear I'm not opposed to calling this WebFrameTime, but let's make it compatible with BeginFrameArgs. https://codereview.chromium.org/321373003/ To unsubscribe from this group and stop receiving emails from it, send an email to blink-reviews+unsubscribe@chromium.org.
Thanks for the quick reply. Maybe I'm coming at this from a different angle, but I think being able to extrapolate future frame times beyond one vsync is something a blink scheduler could make good use of. For instance, imagine getting a bunch of tasks to run after a quiescent period. If none of those are urgent tasks, we could consider delaying them until the next BeginFrame just to see if anything more critical comes up. One example is the v8 'idle' gc that currently uses an arbitrary 1 second timeout to run. I get your concern with people misusing this information. How about keeping the actual values inside WebFrameTime and adding accessors that compute the more useful values? (e.g. predictedNextFrameTimeBasedOnCurrentTime() but with a better name :) - Sami On 2 July 2014 15:53, Tim Ansell <mithro@mithis.com> wrote: > Hi Sami, > > Sorry about the brevity, writing this on my phone so you get this before > another 24 hour cycle. > > I did initially start with BeginFrameArgs but with the extra members, > different member types and members I wanted to remove it quickly diverged > so I decided to use a different name so they weren't confused. > > One part that I don't want to inherit from BeginFrameArgs is the interval > member. The actual value of that member is very low, while the misuse > potential is *very* high. For example, the actual render interval is also > non-fixed (for example when background ticking). It easy to see how some > one could start using (lastTime + interval * 2) in their code. > > The deadline and render and next render times give you the actual values > you want without the likelihood of you getting it wrong. > > Happy to explain further. > > Tim 'mithro' Ansell > Thanks Tim, this looks super useful! > > > https://codereview.chromium.org/321373003/diff/80001/ > public/platform/WebFrameTime.h > File public/platform/WebFrameTime.h (right): > > https://codereview.chromium.org/321373003/diff/80001/ > public/platform/WebFrameTime.h#newcode38 > public/platform/WebFrameTime.h:38: struct WebFrameTime { > Any reason not just to call this WebBeginFrameArgs and pass in the same > data as we get through BeginFrame? Right now this is almost exactly the > same except we have just one predicted frame time instead of an > interval. > > To be clear I'm not opposed to calling this WebFrameTime, but let's make > it compatible with BeginFrameArgs. > > https://codereview.chromium.org/321373003/ > To unsubscribe from this group and stop receiving emails from it, send an email to blink-reviews+unsubscribe@chromium.org.
WebFrameTime is a bit odd of a name, since it includes more that just the time, but I like the overall direction of the change. It doesn't surprise me that we would wnat more context about the frame we're being asked to produce and this seems like a fine way to do this. Alternatively we could be given a FrameID and request this information from the platform (which might allow it to be updated after the frame work starts). But this seems fine too.
I think this is OK and we can iterate from here. I also agree with Sami that if Chromium's class fits our needs (BeginFrameArgs) we should just mirror that here. But I'm not sure this CL needs another few weeks of discussion. If you have a plan to use this information to improve the code immediately, then l-g-t-m. We can always change the names later (although changing API-level names is more painful than ones just in Blink) If this is still speculative then we should wait until we actually have concrete uses of this information. lgtm. My understanding is that Sami and his team in london are actively working on the various schedulers across Chrome, including Blink's. Unfortunately his timezone is about as incompatibile with yours as mine. :(
Right, I don't want to block progress here since I think this is definitely moving things in the right direction. My requirements above only exist as whiteboard diagrams at the moment so they don't have real weight yet.
This feels too speculative to put in as public API. We don't have any code that generates or consumes these values yet, so these may or may not end up being the values we want. I think it'd be much easier to add things as they are used.
On 2014/07/02 18:03:29, jamesr wrote: > This feels too speculative to put in as public API. We don't have any code that > generates or consumes these values yet, so these may or may not end up being the > values we want. I think it'd be much easier to add things as they are used. Hi James, There are a bunch of patches which follow this patch and make use of these new features. One example is patch at https://codereview.chromium.org/369793003/#ps40001 which makes the web-animation engine use them. As mentioned I also have patches to get the values from the compositor scheduler into blink side. The only field which doesn't have a direct usage very soon is the deadline field, but I believe Sami's team in london will rapidly work on using that. Tim
TLDR, for this patch, just do this:
class WebBeginFrameArgs {
double frame_begin_time;
};
WebWidget::BeginFrame(const WebBeginFrameArgs& args) = 0; // deprecate and
remove ::Animate()
Detailed opinion:
- WebWidget::animate should be renamed to WebWidget::BeginFrame. This is what we
call it in chrome, and with the blink scheduler, ::Animate will take on more
responsibilities than just animation
- Instead of WebFrameTime, call it WebBeginFrameArgs. Thats what we have in cc,
and we know there are other parameters that are highly likely to be added in the
future, e.g. timestamps from the fps api, or skewport info, etc.
- No need platform object. Its better for the WebBeginFrameArgs to be unpacked
by WebViewImpl into core primitives. Skewport for instance needs to go to a
different part of core than, say, the frame timestamps which would go to the
scheduler.
- WebBeginFrameArgs should have *only* frame begin time for this patch. Add new
fields only when they're needed, as additions. Switching to a struct makes this
much easier vis a vis 3-sided-patch logistics.
- New fields should be exactly same as cc BeginFrame, not changed around. E.g.
if cc BeginFrameArgs has frameBeginTime and frameduration, then web should have
the same, not
- Hookup from WebViewImpl to core/ should be a completely separate patch
sequence. Possibly all hookup to core/ objects like the animation systems should
be thorugh scheduler.
- Hence, a good patch before hooking BeginFrame up to core/ would be to
implement skeleton scheduler classes in core/scheduler per sami's design doc.
Sami has this WIP already, I gather.
- Please dont land any code for output time for now. I would like to see common
code landed and working across all platforms for correct routing of
frame_beign_time before we begin on output time logistics.
And for the love of god, go do a patch for the universe that renames frame_time
to frame_begin_time so we're more precise about naming.
Hope this helps.
Hi everyone,
Could you all please take another look (specially jamesr). I've incorporated
Nat's feedback which has significantly reduced this patch size.
On 2014/07/15 19:08:30, nduca wrote:
> TLDR, for this patch, just do this:
> class WebBeginFrameArgs {
> double frame_begin_time;
> };
> WebWidget::BeginFrame(const WebBeginFrameArgs& args) = 0; // deprecate and
> remove ::Animate()
This is basically what this patch is now.
> Detailed opinion:
> - WebWidget::animate should be renamed to WebWidget::BeginFrame. This is what
we
> call it in chrome, and with the blink scheduler, ::Animate will take on more
> responsibilities than just animation
Done (although matching Blink naming conventions).
> - Instead of WebFrameTime, call it WebBeginFrameArgs. Thats what we have in
cc,
> and we know there are other parameters that are highly likely to be added in
the
> future, e.g. timestamps from the fps api, or skewport info, etc.
Done.
> - No need platform object. Its better for the WebBeginFrameArgs to be unpacked
> by WebViewImpl into core primitives. Skewport for instance needs to go to a
> different part of core than, say, the frame timestamps which would go to the
> scheduler.
Done.
> - WebBeginFrameArgs should have *only* frame begin time for this patch. Add
new
> fields only when they're needed, as additions. Switching to a struct makes
this
> much easier vis a vis 3-sided-patch logistics.
Done.
> - New fields should be exactly same as cc BeginFrame, not changed around. E.g.
> if cc BeginFrameArgs has frameBeginTime and frameduration, then web should
have
> the same, not
Doing the previous change makes this comment no longer relavent to this specific
patch, but will keep it in mind for the follow up patches.
> - Hookup from WebViewImpl to core/ should be a completely separate patch
> sequence. Possibly all hookup to core/ objects like the animation systems
should
> be thorugh scheduler.
Done. Will send out the patch which does the hook up through core shortly.
> - Hence, a good patch before hooking BeginFrame up to core/ would be to
> implement skeleton scheduler classes in core/scheduler per sami's design doc.
> Sami has this WIP already, I gather.
Will send the above patch to Sami and make sure it's heading in the right
direction (and if we want to make it dependent on his code/scheduler work).
> - Please dont land any code for output time for now. I would like to see
common
> code landed and working across all platforms for correct routing of
> frame_beign_time before we begin on output time logistics.
Okay.
> And for the love of god, go do a patch for the universe that renames
frame_time
> to frame_begin_time so we're more precise about naming.
frame_time is actually "lastVsyncTimeBeforeThisFrame" but that is kind of
unwieldy. I went with the following but very open to a better name.
+ // Time in CLOCK_MONOTONIC that is the most recent vsync time.
+ double lastFrameTime;
I'll get a patch which changes BeginFrameArgs in Chrome to use the same name if
we decided that this is the right name.
Anything I've missed?
Tim 'mithro' Ansell
Thanks Tim, I think this version gives us a good foundation. https://codereview.chromium.org/321373003/diff/140001/Source/web/WebViewImpl.cpp File Source/web/WebViewImpl.cpp (right): https://codereview.chromium.org/321373003/diff/140001/Source/web/WebViewImpl.... Source/web/WebViewImpl.cpp:1706: TRACE_EVENT0("blink", "WebViewImpl::animate"); Please update this trace event too. https://codereview.chromium.org/321373003/diff/140001/Source/web/WebViewImpl.h File Source/web/WebViewImpl.h (right): https://codereview.chromium.org/321373003/diff/140001/Source/web/WebViewImpl.... Source/web/WebViewImpl.h:44: #include "public/web/WebBeginFrameArgs.h" I don't think you need this include here since the base class already knows about this type. https://codereview.chromium.org/321373003/diff/140001/public/web/WebWidget.h File public/web/WebWidget.h (right): https://codereview.chromium.org/321373003/diff/140001/public/web/WebWidget.h#... public/web/WebWidget.h:97: virtual void beginFrame(WebBeginFrameArgs frameTime) { } Seems like this should be a const WebBeginFrameArgs& (here and elsewhere).
https://codereview.chromium.org/321373003/diff/140001/Source/web/WebViewImpl.cpp File Source/web/WebViewImpl.cpp (right): https://codereview.chromium.org/321373003/diff/140001/Source/web/WebViewImpl.... Source/web/WebViewImpl.cpp:1706: TRACE_EVENT0("blink", "WebViewImpl::animate"); On 2014/07/16 10:41:15, Sami wrote: > Please update this trace event too. Done. https://codereview.chromium.org/321373003/diff/140001/Source/web/WebViewImpl.h File Source/web/WebViewImpl.h (right): https://codereview.chromium.org/321373003/diff/140001/Source/web/WebViewImpl.... Source/web/WebViewImpl.h:44: #include "public/web/WebBeginFrameArgs.h" On 2014/07/16 10:41:15, Sami wrote: > I don't think you need this include here since the base class already knows > about this type. Done. https://codereview.chromium.org/321373003/diff/140001/public/web/WebWidget.h File public/web/WebWidget.h (right): https://codereview.chromium.org/321373003/diff/140001/public/web/WebWidget.h#... public/web/WebWidget.h:97: virtual void beginFrame(WebBeginFrameArgs frameTime) { } On 2014/07/16 10:41:15, Sami wrote: > Seems like this should be a const WebBeginFrameArgs& (here and elsewhere). Done.
https://codereview.chromium.org/321373003/diff/160001/public/web/WebWidget.h File public/web/WebWidget.h (right): https://codereview.chromium.org/321373003/diff/160001/public/web/WebWidget.h#... public/web/WebWidget.h:97: virtual void beginFrame(const WebBeginFrameArgs frameTime) { } Should be a const reference instead of just const: "const WebBeginFrameArgs&"
https://codereview.chromium.org/321373003/diff/160001/public/web/WebWidget.h File public/web/WebWidget.h (right): https://codereview.chromium.org/321373003/diff/160001/public/web/WebWidget.h#... public/web/WebWidget.h:97: virtual void beginFrame(const WebBeginFrameArgs frameTime) { } On 2014/07/16 14:38:55, Sami wrote: > Should be a const reference instead of just const: "const WebBeginFrameArgs&" Done.
Thanks, lgtm! https://codereview.chromium.org/321373003/diff/200001/public/web/WebBeginFram... File public/web/WebBeginFrameArgs.h (right): https://codereview.chromium.org/321373003/diff/200001/public/web/WebBeginFram... public/web/WebBeginFrameArgs.h:1: /* There's a shorter license block we should use for new files -- see the end of http://www.chromium.org/blink/coding-style
https://codereview.chromium.org/321373003/diff/200001/public/web/WebBeginFram... File public/web/WebBeginFrameArgs.h (right): https://codereview.chromium.org/321373003/diff/200001/public/web/WebBeginFram... public/web/WebBeginFrameArgs.h:45: double lastFrameTime; lastFrameTimeMonotonic
lgtm https://codereview.chromium.org/321373003/diff/200001/Source/web/WebPagePopup... File Source/web/WebPagePopupImpl.cpp (right): https://codereview.chromium.org/321373003/diff/200001/Source/web/WebPagePopup... Source/web/WebPagePopupImpl.cpp:285: PageWidgetDelegate::animate(m_page.get(), monotonicallyIncreasingTime()); do you have a particular reason to not use the parameter for these types of widgets? https://codereview.chromium.org/321373003/diff/200001/public/web/WebBeginFram... File public/web/WebBeginFrameArgs.h (right): https://codereview.chromium.org/321373003/diff/200001/public/web/WebBeginFram... public/web/WebBeginFrameArgs.h:41: // FIXME(mithro): Upgrade the time in CLOCK_MONOTONIC values to use a blink style is just FIXME, no name. If you want a more persistent reference here put a link to a bug number https://codereview.chromium.org/321373003/diff/200001/public/web/WebWidget.h File public/web/WebWidget.h (right): https://codereview.chromium.org/321373003/diff/200001/public/web/WebWidget.h#... public/web/WebWidget.h:92: // FIXME(mithro): Remove this function once Chrome side patch lands. ditto
i think your cl description needs some updating
https://codereview.chromium.org/321373003/diff/200001/Source/web/WebPagePopup... File Source/web/WebPagePopupImpl.cpp (right): https://codereview.chromium.org/321373003/diff/200001/Source/web/WebPagePopup... Source/web/WebPagePopupImpl.cpp:285: PageWidgetDelegate::animate(m_page.get(), monotonicallyIncreasingTime()); On 2014/07/17 19:58:16, jamesr wrote: > do you have a particular reason to not use the parameter for these types of > widgets? This method *should* use the passed in parameter. Making it do so would make this patch have actually functional changes and sadly it causes a bunch of tests to fail. (You can see it in previous patch sets.) I've move that change into a separate patch which also fixes the tests. https://codereview.chromium.org/321373003/diff/200001/public/web/WebBeginFram... File public/web/WebBeginFrameArgs.h (right): https://codereview.chromium.org/321373003/diff/200001/public/web/WebBeginFram... public/web/WebBeginFrameArgs.h:1: /* On 2014/07/17 10:37:46, Sami wrote: > There's a shorter license block we should use for new files -- see the end of > http://www.chromium.org/blink/coding-style Done. https://codereview.chromium.org/321373003/diff/200001/public/web/WebBeginFram... public/web/WebBeginFrameArgs.h:41: // FIXME(mithro): Upgrade the time in CLOCK_MONOTONIC values to use a On 2014/07/17 19:58:16, jamesr wrote: > blink style is just FIXME, no name. If you want a more persistent reference here > put a link to a bug number Done. https://codereview.chromium.org/321373003/diff/200001/public/web/WebBeginFram... public/web/WebBeginFrameArgs.h:45: double lastFrameTime; On 2014/07/17 18:26:42, nduca wrote: > lastFrameTimeMonotonic Done. https://codereview.chromium.org/321373003/diff/200001/public/web/WebWidget.h File public/web/WebWidget.h (right): https://codereview.chromium.org/321373003/diff/200001/public/web/WebWidget.h#... public/web/WebWidget.h:92: // FIXME(mithro): Remove this function once Chrome side patch lands. On 2014/07/17 19:58:16, jamesr wrote: > ditto Done.
https://codereview.chromium.org/321373003/diff/200001/Source/web/WebPagePopup... File Source/web/WebPagePopupImpl.cpp (right): https://codereview.chromium.org/321373003/diff/200001/Source/web/WebPagePopup... Source/web/WebPagePopupImpl.cpp:285: PageWidgetDelegate::animate(m_page.get(), monotonicallyIncreasingTime()); On 2014/07/18 00:21:04, mithro(OO till 11 Aug) wrote: > On 2014/07/17 19:58:16, jamesr wrote: > > do you have a particular reason to not use the parameter for these types of > > widgets? > > This method *should* use the passed in parameter. > > Making it do so would make this patch have actually functional changes and sadly > it causes a bunch of tests to fail. (You can see it in previous patch sets.) > > I've move that change into a separate patch which also fixes the tests. Then leave a FIXME, ideally referencing the bug you filed that described the test failures so the next patch will have proper context
On 2014/07/18 00:30:16, jamesr wrote: > https://codereview.chromium.org/321373003/diff/200001/Source/web/WebPagePopup... > File Source/web/WebPagePopupImpl.cpp (right): > > https://codereview.chromium.org/321373003/diff/200001/Source/web/WebPagePopup... > Source/web/WebPagePopupImpl.cpp:285: PageWidgetDelegate::animate(m_page.get(), > monotonicallyIncreasingTime()); > On 2014/07/18 00:21:04, mithro(OO till 11 Aug) wrote: > > On 2014/07/17 19:58:16, jamesr wrote: > > > do you have a particular reason to not use the parameter for these types of > > > widgets? > > > > This method *should* use the passed in parameter. > > > > Making it do so would make this patch have actually functional changes and > sadly > > it causes a bunch of tests to fail. (You can see it in previous patch sets.) > > > > I've move that change into a separate patch which also fixes the tests. > > Then leave a FIXME, ideally referencing the bug you filed that described the > test failures so the next patch will have proper context Done!
Anyone got any last comments?
go for it, lgtm
The CQ bit was checked by mithro@mithis.com
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mithro@mithis.com/321373003/240001
Message was sent while issue was closed.
Change committed as 178493 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
