Adding monotonic only API to Blink's animation delegate interface

First step to remove wallClockTime from animation interface.

We are currently removing trying to remove all references to base::Time (wall
clock time) from Chrome/Blink compositor/graphics stack.

This requires multiple two-sided patches. Following the process found at
https://groups.google.com/a/chromium.org/d/msg/chromium-dev/ukaRczAK6t8/0ludyZ1Nu4cJ

The steps are;
 * (This Patch) -- Adding monotonic only API to Blink's animation delegate interface
   https://codereview.chromium.org/185633002

 * Making the delegate adapter use monotonic only API
   https://codereview.chromium.org/185643002

 * Remove the wallClockTime API from Blink/WebKit.
   https://codereview.chromium.org/185393005

BUG=299945
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=168560

[ajuma, 2014-03-03 15:25:21]

Thanks for working on this!

Note that the Windows build breaks if this CL goes first.

https://codereview.chromium.org/185633002/diff/20001/public/platform/WebAnima...
File public/platform/WebAnimationDelegate.h (right):

https://codereview.chromium.org/185633002/diff/20001/public/platform/WebAnima...
public/platform/WebAnimationDelegate.h:36: // TODO: Removed wallClockTime after
the following file is updated;
Nit: Blink style is to use "FIXME" rather than "TODO" (see
http://www.chromium.org/blink/coding-style#TOC-Comments)

Also, s/Removed/Remove

https://codereview.chromium.org/185633002/diff/20001/public/platform/WebAnima...
public/platform/WebAnimationDelegate.h:46: virtual void
notifyAnimationStarted(double monotonicTime, WebAnimation::TargetProperty) = 0;
If this CL lands first (as in the steps you've described), this will initially
have no implementation anywhere. I'd suggest doing the chromium side first
(guarded by #ifdefs), then do all of the Blink side (and add the appropriate
#define), and then clean up the #ifdefs on the chromium side.

[ajuma, 2014-03-03 15:30:26]

https://codereview.chromium.org/185633002/diff/20001/public/platform/WebAnima...
File public/platform/WebAnimationDelegate.h (right):

https://codereview.chromium.org/185633002/diff/20001/public/platform/WebAnima...
public/platform/WebAnimationDelegate.h:46: virtual void
notifyAnimationStarted(double monotonicTime, WebAnimation::TargetProperty) = 0;
On 2014/03/03 15:25:21, ajuma wrote:
> If this CL lands first (as in the steps you've described), this will initially
> have no implementation anywhere. I'd suggest doing the chromium side first
> (guarded by #ifdefs), then do all of the Blink side (and add the appropriate
> #define), and then clean up the #ifdefs on the chromium side.

Actually, ignore the "this will have no implementation anywhere" part of this
comment, sorry. But I still suggest changing the order of the CLs to address the
Windows build error.

[mithro-old, 2014-03-04 01:39:18]

https://codereview.chromium.org/185633002/diff/20001/public/platform/WebAnima...
File public/platform/WebAnimationDelegate.h (right):

https://codereview.chromium.org/185633002/diff/20001/public/platform/WebAnima...
public/platform/WebAnimationDelegate.h:36: // TODO: Removed wallClockTime after
the following file is updated;
On 2014/03/03 15:25:21, ajuma wrote:
> Nit: Blink style is to use "FIXME" rather than "TODO" (see
> http://www.chromium.org/blink/coding-style#TOC-Comments)
> 
> Also, s/Removed/Remove

Done.

https://codereview.chromium.org/185633002/diff/20001/public/platform/WebAnima...
public/platform/WebAnimationDelegate.h:46: virtual void
notifyAnimationStarted(double monotonicTime, WebAnimation::TargetProperty) = 0;
On 2014/03/03 15:30:27, ajuma wrote:
> On 2014/03/03 15:25:21, ajuma wrote:
> > If this CL lands first (as in the steps you've described), this will
initially
> > have no implementation anywhere. I'd suggest doing the chromium side first
> > (guarded by #ifdefs), then do all of the Blink side (and add the appropriate
> > #define), and then clean up the #ifdefs on the chromium side.
> 
> Actually, ignore the "this will have no implementation anywhere" part of this
> comment, sorry. But I still suggest changing the order of the CLs to address
the
> Windows build error.

Adding BLINK_PLATFORM_EXPORT should fix windows. Testing now.

[mithro-old, 2014-03-04 05:26:02]

Looks like the windows build is fixed now. PTAL.

[ajuma, 2014-03-04 14:07:17]

lgtm

[mithro-old, 2014-03-04 14:11:21]

The CQ bit was checked by mithro@mithis.com

[commit-bot: I haz the power, 2014-03-04 14:11:29]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mithro@mithis.com/185633002/100001

[ajuma, 2014-03-04 14:13:31]

The CQ bit was unchecked by ajuma@chromium.org

[ajuma, 2014-03-04 14:13:59]

You still need an OWNERS review.

[mithro-old, 2014-03-04 14:15:15]

Yeah. Just wanted to run all the try bots.

Tim
On 5 Mar 2014 01:14, <ajuma@chromium.org> wrote:

> You still need an OWNERS review.
>
> https://codereview.chromium.org/185633002/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to blink-reviews+unsubscribe@chromium.org.

[danakj, 2014-03-04 15:33:23]

On 2014/03/04 14:15:15, mithro wrote:
> Yeah. Just wanted to run all the try bots.

There's the "Try more bots" link for that :)

[jamesr, 2014-03-04 21:38:37]

https://codereview.chromium.org/185633002/diff/100001/public/platform/WebAnim...
File public/platform/WebAnimationDelegate.h (right):

https://codereview.chromium.org/185633002/diff/100001/public/platform/WebAnim...
public/platform/WebAnimationDelegate.h:35: class BLINK_PLATFORM_EXPORT
WebAnimationDelegate {
why does this class need to be exported? i don't see any symbols on this class
that would require it - only pure virtual functions and inline functions.  If
this is supposed to be exported, where is the object file providing the
definition of the symbols you are exporting?

https://codereview.chromium.org/185633002/diff/100001/public/platform/WebAnim...
public/platform/WebAnimationDelegate.h:39: inline void
notifyAnimationStarted(double wallClockTime, double monotonicTime,
WebAnimation::TargetProperty prop)
i don't think the "inline" annotation provides any value

don't you need to mark these virtual so you can override them on the chromium
side?

the parameter name 'prop' provides no value here and should be omitted per Blink
style

[mithro-old, 2014-03-04 23:54:23]

Hi James,

Thank you for your review! Can you please take a look at look at the new version
and see if my comments make any sense?

Tim 'mithro' Ansell

https://codereview.chromium.org/185633002/diff/100001/public/platform/WebAnim...
File public/platform/WebAnimationDelegate.h (right):

https://codereview.chromium.org/185633002/diff/100001/public/platform/WebAnim...
public/platform/WebAnimationDelegate.h:35: class BLINK_PLATFORM_EXPORT
WebAnimationDelegate {
On 2014/03/04 21:38:37, jamesr wrote:
> why does this class need to be exported? i don't see any symbols on this class
> that would require it - only pure virtual functions and inline functions.  If
> this is supposed to be exported, where is the object file providing the
> definition of the symbols you are exporting?

The two methods I add below with inline definitions cause it to need the
BLINK_PLATFORM_EXPORT declaration otherwise Chrome on Windows fails to link
with;
---------------------
FAILED: ninja -t msvc -e environment.x86 -- E:\b\build\goma\gomacc.exe
"E:\b\depot_tools\win_toolchain\vs2013_files\VC\bin\amd64_x86\cl.exe" /nologo
/showIncludes /FC
@obj\third_party\WebKit\Source\platform\blink_platform.OverscrollTheme.obj.rsp
/c ..\..\third_party\WebKit\Source\platform\OverscrollTheme.cpp
/Foobj\third_party\WebKit\Source\platform\blink_platform.OverscrollTheme.obj
/Fdobj\third_party\WebKit\Source\platform\blink_platform.cc.pdb 
e:\b\build\slave\win\build\src\third_party\webkit\source\platform\graphics\graphicslayer.h(81)
: error C2220: warning treated as error - no 'object' file generated
e:\b\build\slave\win\build\src\third_party\webkit\source\platform\graphics\graphicslayer.h(81)
: warning C4275: non dll-interface class 'blink::WebAnimationDelegate' used as
base for dll-interface class 'WebCore::GraphicsLayer'
       
e:\b\build\slave\win\build\src\third_party\webkit\public\platform\webanimationdelegate.h(34)
: see declaration of 'blink::WebAnimationDelegate'
       
e:\b\build\slave\win\build\src\third_party\webkit\source\platform\graphics\graphicslayer.h(81)
: see declaration of 'WebCore::GraphicsLayer'
ninja: build stopped: subcommand failed.
---------------------

The suggested fix on #blink was adding a BLINK_PLATFORM_EXPORT.

https://codereview.chromium.org/185633002/diff/100001/public/platform/WebAnim...
public/platform/WebAnimationDelegate.h:39: inline void
notifyAnimationStarted(double wallClockTime, double monotonicTime,
WebAnimation::TargetProperty prop)
On 2014/03/04 21:38:37, jamesr wrote:
> i don't think the "inline" annotation provides any value

Fixed.

> don't you need to mark these virtual so you can override them on the chromium
> side?

This class is only inherited from in Blink, Chrome only calls the interface.

These non-virtual methods are only being added so I don't need a two-sided
patch. Making these methods non-virtual means that every inheritor inside Blink
is statically checked to be using the non-walltime API. These methods will be
removed in https://codereview.chromium.org/178103004/ which will be submitted
after https://codereview.chromium.org/185643002/ lands and deps are rolled.

FYI the only Chrome file which references this file/class is 
webkit/renderer/compositor_bindings/web_to_cc_animation_delegate_adapter.cc and
I've checked that chrome successfully links on Windows, Mac and Linux with this
CL patched.

> the parameter name 'prop' provides no value here and should be omitted per
Blink
> style

It needs a name so I can pass the value around?

[mithro-old, 2014-03-04 23:57:59]

On 2014/03/04 15:33:23, danakj wrote:
> On 2014/03/04 14:15:15, mithro wrote:
> > Yeah. Just wanted to run all the try bots.
> 
> There's the "Try more bots" link for that :)

That works well if you know which bots you want to try, but I can never remember
the exact list of bots that the CQ will run. The CQ won't submit a patch if it
is missing LGTMs, so checking that box seems pretty safe?

[jamesr, 2014-03-04 23:58:50]

On 2014/03/04 23:54:23, mithro wrote:
> Hi James,
> 
> Thank you for your review! Can you please take a look at look at the new
version
> and see if my comments make any sense?
> 
> Tim 'mithro' Ansell
> 
>
https://codereview.chromium.org/185633002/diff/100001/public/platform/WebAnim...
> File public/platform/WebAnimationDelegate.h (right):
> 
>
https://codereview.chromium.org/185633002/diff/100001/public/platform/WebAnim...
> public/platform/WebAnimationDelegate.h:35: class BLINK_PLATFORM_EXPORT
> WebAnimationDelegate {
> On 2014/03/04 21:38:37, jamesr wrote:
> > why does this class need to be exported? i don't see any symbols on this
class
> > that would require it - only pure virtual functions and inline functions. 
If
> > this is supposed to be exported, where is the object file providing the
> > definition of the symbols you are exporting?
> 
> The two methods I add below with inline definitions cause it to need the
> BLINK_PLATFORM_EXPORT declaration otherwise Chrome on Windows fails to link
> with;
> ---------------------
> FAILED: ninja -t msvc -e environment.x86 -- E:\b\build\goma\gomacc.exe
> "E:\b\depot_tools\win_toolchain\vs2013_files\VC\bin\amd64_x86\cl.exe" /nologo
> /showIncludes /FC
> @obj\third_party\WebKit\Source\platform\blink_platform.OverscrollTheme.obj.rsp
> /c ..\..\third_party\WebKit\Source\platform\OverscrollTheme.cpp
> /Foobj\third_party\WebKit\Source\platform\blink_platform.OverscrollTheme.obj
> /Fdobj\third_party\WebKit\Source\platform\blink_platform.cc.pdb 
>
e:\b\build\slave\win\build\src\third_party\webkit\source\platform\graphics\graphicslayer.h(81)
> : error C2220: warning treated as error - no 'object' file generated
>
e:\b\build\slave\win\build\src\third_party\webkit\source\platform\graphics\graphicslayer.h(81)
> : warning C4275: non dll-interface class 'blink::WebAnimationDelegate' used as
> base for dll-interface class 'WebCore::GraphicsLayer'
>        
>
e:\b\build\slave\win\build\src\third_party\webkit\public\platform\webanimationdelegate.h(34)
> : see declaration of 'blink::WebAnimationDelegate'
>        
>
e:\b\build\slave\win\build\src\third_party\webkit\source\platform\graphics\graphicslayer.h(81)
> : see declaration of 'WebCore::GraphicsLayer'
> ninja: build stopped: subcommand failed.
> ---------------------
> 
> The suggested fix on #blink was adding a BLINK_PLATFORM_EXPORT.
> 

Ah yes, warning 4275.  You don't really need this to be exported, visual studio
is simply being paranoid here in case you were doing funky things with static
data members on classes that wouldn't work cross-DLL, but you aren't so the
warning can be safely ignored.  #including PlatformExport.h will suppress this
warning via a pragma which is IMHO a better fix.  
>
https://codereview.chromium.org/185633002/diff/100001/public/platform/WebAnim...
> public/platform/WebAnimationDelegate.h:39: inline void
> notifyAnimationStarted(double wallClockTime, double monotonicTime,
> WebAnimation::TargetProperty prop)
> On 2014/03/04 21:38:37, jamesr wrote:
> > i don't think the "inline" annotation provides any value
> 
> Fixed.
> 
> > don't you need to mark these virtual so you can override them on the
chromium
> > side?
> 
> This class is only inherited from in Blink, Chrome only calls the interface.
> 
> These non-virtual methods are only being added so I don't need a two-sided
> patch. Making these methods non-virtual means that every inheritor inside
Blink
> is statically checked to be using the non-walltime API. These methods will be
> removed in https://codereview.chromium.org/178103004/ which will be submitted
> after https://codereview.chromium.org/185643002/ lands and deps are rolled.

OK then.

> 
> FYI the only Chrome file which references this file/class is 
> webkit/renderer/compositor_bindings/web_to_cc_animation_delegate_adapter.cc
and
> I've checked that chrome successfully links on Windows, Mac and Linux with
this
> CL patched.
> 
> > the parameter name 'prop' provides no value here and should be omitted per
> Blink
> > style
> 
> It needs a name so I can pass the value around?

Oh right, you're using the parameter.

[jamesr, 2014-03-05 00:00:34]

On 2014/03/04 23:57:59, mithro wrote:
> On 2014/03/04 15:33:23, danakj wrote:
> > On 2014/03/04 14:15:15, mithro wrote:
> > > Yeah. Just wanted to run all the try bots.
> > 
> > There's the "Try more bots" link for that :)
> 
> That works well if you know which bots you want to try, but I can never
remember
> the exact list of bots that the CQ will run. The CQ won't submit a patch if it
> is missing LGTMs, so checking that box seems pretty safe?

No - it's actually quite rude and dangerous as someone else who is reviewing the
patch unaware that you've clicked the CQ may issue an
'LGTM-with-required-comments' and be unaware that the patch is actually nearly
landed.  Don't click the CQ button until you are *sure* that the patch is ready
to go and that your reviewers agree.

If you want to run the patch on a reasonable set of trybots just type 'git cl
try' from your checkout.  There's a script that will pick out a good set of bots
depending on the contents of the patch.

[jamesr, 2014-03-05 00:04:23]

On 2014/03/05 00:00:34, jamesr wrote:
> No - it's actually quite rude and dangerous

Well, I suppose "quite rude" is highly subjective and debatable - some might
find it so, some might not.  I suppose I don't consider it very rude, but I do
think it can be dangerous for the reason I mentioned below.  As a reviewer I
don't really like surprises all that much.

[mithro-old, 2014-03-05 00:27:13]

> Ah yes, warning 4275.  You don't really need this to be exported, visual
studio
> is simply being paranoid here in case you were doing funky things with static
> data members on classes that wouldn't work cross-DLL, but you aren't so the
> warning can be safely ignored.  #including PlatformExport.h will suppress this
> warning via a pragma which is IMHO a better fix.  

It seems using platform/PlatformExport.h violates checkdeps rules;
--------------------------------
** Presubmit ERRORS **
You added one or more #includes that violate checkdeps rules.
  public/platform/WebAnimationDelegate.h
      Illegal include: "platform/PlatformExport.h"
    Because of no rule applying.
--------------------------------

I can't find any equivalent inside public/XXXXX?

What do you suggest? Should I copy across the pragma?
--------------------------------
#if defined(_MSC_VER)
#pragma warning(suppress:4275)
#endif
--------------------------------

[jamesr, 2014-03-05 00:33:13]

On 2014/03/05 00:27:13, mithro wrote:
> > Ah yes, warning 4275.  You don't really need this to be exported, visual
> studio
> > is simply being paranoid here in case you were doing funky things with
static
> > data members on classes that wouldn't work cross-DLL, but you aren't so the
> > warning can be safely ignored.  #including PlatformExport.h will suppress
this
> > warning via a pragma which is IMHO a better fix.  
> 
> It seems using platform/PlatformExport.h violates checkdeps rules;
> --------------------------------
> ** Presubmit ERRORS **
> You added one or more #includes that violate checkdeps rules.
>   public/platform/WebAnimationDelegate.h
>       Illegal include: "platform/PlatformExport.h"
>     Because of no rule applying.
> --------------------------------

The #include would go in the translation unit triggering the warning (i.e.
whatever file defines the class that inherits from your public API).  Looks like
that'd be GraphicsLayerSomething?

[mithro-old, 2014-03-05 01:04:25]

> No - it's actually quite rude and dangerous as someone else who is reviewing
the
> patch unaware that you've clicked the CQ may issue an
> 'LGTM-with-required-comments' and be unaware that the patch is actually nearly
> landed.  Don't click the CQ button until you are *sure* that the patch is
ready
> to go and that your reviewers agree.

Okay understood! I totally didn't think about the LGTM-with-required-comments
case.

> If you want to run the patch on a reasonable set of trybots just type 'git cl
> try' from your checkout.

I will give that option another go. I have found it really useful if you just
want to run a subset of the tests on bots.

>  There's a script that will pick out a good set of bots
> depending on the contents of the patch.

Do you mean that there is another script? Or are you talking about "git cl try"
here too?

I've also logged a feature request bug for a "CQ dry-run" at
https://code.google.com/p/chromium/issues/detail?id=349261 but I totally
understand that it is probably a low priority and has a high possibility of
never happening.

PS I logged https://code.google.com/p/chromium/issues/detail?id=349255 about the
false LGTM match too. My guess is that we don't really care and it will be a
closed WontFix issue, but couldn't find any existing discussion.

[jamesr, 2014-03-05 01:07:07]

On 2014/03/05 01:04:25, mithro wrote:
> > If you want to run the patch on a reasonable set of trybots just type 'git
cl
> > try' from your checkout.
> 
> I will give that option another go. I have found it really useful if you just
> want to run a subset of the tests on bots.

If you run 'git cl try' without any additional arguments it runs the
GetPreferredTrySlaves function in the relevant PRESUBMIT.py(s), if any, and
sends it to those bots.  That normally covers what you'd want to run by default
for a patch without needing a lot of thought.

> 
> >  There's a script that will pick out a good set of bots
> > depending on the contents of the patch.
> 
> Do you mean that there is another script? Or are you talking about "git cl
try"
> here too?

I'm talking about the functionality of 'git cl try'

[jamesr, 2014-03-05 01:07:52]

latest patch lgtm once you remove the EXPORT and add the #include to whatever
needs it inside Source/

[mithro-old, 2014-03-05 01:24:42]

Thanks for all your help so far!

On 2014/03/05 00:33:13, jamesr wrote:
> On 2014/03/05 00:27:13, mithro wrote:
> > > Ah yes, warning 4275.  You don't really need this to be exported, visual
> > studio
> > > is simply being paranoid here in case you were doing funky things with
> static
> > > data members on classes that wouldn't work cross-DLL, but you aren't so
the
> > > warning can be safely ignored.  #including PlatformExport.h will suppress
> this
> > > warning via a pragma which is IMHO a better fix.  
> > 
> > It seems using platform/PlatformExport.h violates checkdeps rules;
> > --------------------------------
> > ** Presubmit ERRORS **
> > You added one or more #includes that violate checkdeps rules.
> >   public/platform/WebAnimationDelegate.h
> >       Illegal include: "platform/PlatformExport.h"
> >     Because of no rule applying.
> > --------------------------------
> 
> The #include would go in the translation unit triggering the warning (i.e.
> whatever file defines the class that inherits from your public API).  Looks
like
> that'd be GraphicsLayerSomething?

The warning was;
------------------------
e:\b\build\slave\win\build\src\third_party\webkit\source\platform\graphics\graphicslayer.h(81)
: warning C4275: non dll-interface class 'blink::WebAnimationDelegate' used as
base for dll-interface class 'WebCore::GraphicsLayer'
------------------------

Source/platform/graphics/GraphicsLayer.h has;
------------------------
#ifndef GraphicsLayer_h
#define GraphicsLayer_h

#include "platform/PlatformExport.h"

<snip>

class PLATFORM_EXPORT GraphicsLayer : public GraphicsContextPainter, public
blink::WebAnimationDelegate, public blink::WebLayerScrollClient, public
blink::WebLayerClient {
------------------------

Any idea why the pragma inside PlatformExport.h isn't working?

Tim

[jamesr, 2014-03-05 01:27:12]

On 2014/03/05 01:24:42, mithro wrote:
> Thanks for all your help so far!
> 
> On 2014/03/05 00:33:13, jamesr wrote:
> > On 2014/03/05 00:27:13, mithro wrote:
> > > > Ah yes, warning 4275.  You don't really need this to be exported, visual
> > > studio
> > > > is simply being paranoid here in case you were doing funky things with
> > static
> > > > data members on classes that wouldn't work cross-DLL, but you aren't so
> the
> > > > warning can be safely ignored.  #including PlatformExport.h will
suppress
> > this
> > > > warning via a pragma which is IMHO a better fix.  
> > > 
> > > It seems using platform/PlatformExport.h violates checkdeps rules;
> > > --------------------------------
> > > ** Presubmit ERRORS **
> > > You added one or more #includes that violate checkdeps rules.
> > >   public/platform/WebAnimationDelegate.h
> > >       Illegal include: "platform/PlatformExport.h"
> > >     Because of no rule applying.
> > > --------------------------------
> > 
> > The #include would go in the translation unit triggering the warning (i.e.
> > whatever file defines the class that inherits from your public API).  Looks
> like
> > that'd be GraphicsLayerSomething?
> 
> The warning was;
> ------------------------
>
e:\b\build\slave\win\build\src\third_party\webkit\source\platform\graphics\graphicslayer.h(81)
> : warning C4275: non dll-interface class 'blink::WebAnimationDelegate' used as
> base for dll-interface class 'WebCore::GraphicsLayer'
> ------------------------
> 
> Source/platform/graphics/GraphicsLayer.h has;
> ------------------------
> #ifndef GraphicsLayer_h
> #define GraphicsLayer_h
> 
> #include "platform/PlatformExport.h"
> 
> <snip>
> 
> class PLATFORM_EXPORT GraphicsLayer : public GraphicsContextPainter, public
> blink::WebAnimationDelegate, public blink::WebLayerScrollClient, public
> blink::WebLayerClient {
> ------------------------
> 
> Any idea why the pragma inside PlatformExport.h isn't working?
> 
> Tim

Can you give me a link to the tryjob showing this error and the exact patch used
on that?  I'd like to investigate.

[mithro-old, 2014-03-05 01:30:59]

> Can you give me a link to the tryjob showing this error and the exact patch
used
> on that?  I'd like to investigate.

The patchset was https://codereview.chromium.org/185633002/#ps20001
The two trybot runs where;
 * http://build.chromium.org/p/tryserver.chromium/builders/win/builds/154680
 * http://build.chromium.org/p/tryserver.chromium/builders/win_blink/builds/1453

I've just uploaded the current patch without the BLINK_PLATFORM_EXPORT bit
(https://codereview.chromium.org/185633002/#ps140001) and am running it again on
the try bots.

[mithro-old, 2014-03-05 05:00:29]

On 2014/03/05 01:30:59, mithro wrote:
> > Can you give me a link to the tryjob showing this error and the exact patch
> used
> > on that?  I'd like to investigate.
> 
> The patchset was https://codereview.chromium.org/185633002/#ps20001
> The two trybot runs where;
>  * http://build.chromium.org/p/tryserver.chromium/builders/win/builds/154680
>  *
http://build.chromium.org/p/tryserver.chromium/builders/win_blink/builds/1453
> 
> I've just uploaded the current patch without the BLINK_PLATFORM_EXPORT bit
> (https://codereview.chromium.org/185633002/#ps140001) and am running it again
on
> the try bots.

Hi James,

The latest patch (https://codereview.chromium.org/185633002/#ps140001) just
failed on the windows try bot at
http://build.chromium.org/p/tryserver.chromium/builders/win/builds/155622/ste...
--------------------------------------------------------------------------
[4849/14479] CXX
obj\third_party\WebKit\Source\platform\graphics\blink_platform.GraphicsContext.obj
[4850/14479] CXX
obj\third_party\WebKit\Source\platform\graphics\blink_platform.GraphicsContextRecorder.obj
FAILED: ninja -t msvc -e environment.x86 -- E:\b\build\goma\gomacc.exe
"E:\b\depot_tools\win_toolchain\vs2013_files\VC\bin\amd64_x86\cl.exe" /nologo
/showIncludes /FC
@obj\third_party\WebKit\Source\platform\blink_platform.OverscrollTheme.obj.rsp
/c ..\..\third_party\WebKit\Source\platform\OverscrollTheme.cpp
/Foobj\third_party\WebKit\Source\platform\blink_platform.OverscrollTheme.obj
/Fdobj\third_party\WebKit\Source\platform\blink_platform.cc.pdb 
e:\b\build\slave\win\build\src\third_party\webkit\source\platform\graphics\graphicslayer.h(81)
: error C2220: warning treated as error - no 'object' file generated
e:\b\build\slave\win\build\src\third_party\webkit\source\platform\graphics\graphicslayer.h(81)
: warning C4275: non dll-interface class 'blink::WebAnimationDelegate' used as
base for dll-interface class 'WebCore::GraphicsLayer'
       
e:\b\build\slave\win\build\src\third_party\webkit\public\platform\webanimationdelegate.h(34)
: see declaration of 'blink::WebAnimationDelegate'
       
e:\b\build\slave\win\build\src\third_party\webkit\source\platform\graphics\graphicslayer.h(81)
: see declaration of 'WebCore::GraphicsLayer'
FAILED: ninja -t msvc -e environment.x86 -- E:\b\build\goma\gomacc.exe
"E:\b\depot_tools\win_toolchain\vs2013_files\VC\bin\amd64_x86\cl.exe" /nologo
/showIncludes /FC
@obj\third_party\WebKit\Source\platform\graphics\gpu\blink_platform.DrawingBuffer.obj.rsp
/c ..\..\third_party\WebKit\Source\platform\graphics\gpu\DrawingBuffer.cpp
/Foobj\third_party\WebKit\Source\platform\graphics\gpu\blink_platform.DrawingBuffer.obj
/Fdobj\third_party\WebKit\Source\platform\blink_platform.cc.pdb 
e:\b\build\slave\win\build\src\third_party\webkit\source\platform\graphics\graphicslayer.h(81)
: error C2220: warning treated as error - no 'object' file generated
e:\b\build\slave\win\build\src\third_party\webkit\source\platform\graphics\graphicslayer.h(81)
: warning C4275: non dll-interface class 'blink::WebAnimationDelegate' used as
base for dll-interface class 'WebCore::GraphicsLayer'
       
e:\b\build\slave\win\build\src\third_party\webkit\public\platform\webanimationdelegate.h(34)
: see declaration of 'blink::WebAnimationDelegate'
       
e:\b\build\slave\win\build\src\third_party\webkit\source\platform\graphics\graphicslayer.h(81)
: see declaration of 'WebCore::GraphicsLayer'
FAILED: ninja -t msvc -e environment.x86 -- E:\b\build\goma\gomacc.exe
"E:\b\depot_tools\win_toolchain\vs2013_files\VC\bin\amd64_x86\cl.exe" /nologo
/showIncludes /FC
@obj\third_party\WebKit\Source\platform\graphics\blink_platform.Canvas2DLayerBridge.obj.rsp
/c ..\..\third_party\WebKit\Source\platform\graphics\Canvas2DLayerBridge.cpp
/Foobj\third_party\WebKit\Source\platform\graphics\blink_platform.Canvas2DLayerBridge.obj
/Fdobj\third_party\WebKit\Source\platform\blink_platform.cc.pdb 
e:\b\build\slave\win\build\src\third_party\webkit\source\platform\graphics\graphicslayer.h(81)
: error C2220: warning treated as error - no 'object' file generated
e:\b\build\slave\win\build\src\third_party\webkit\source\platform\graphics\graphicslayer.h(81)
: warning C4275: non dll-interface class 'blink::WebAnimationDelegate' used as
base for dll-interface class 'WebCore::GraphicsLayer'
       
e:\b\build\slave\win\build\src\third_party\webkit\public\platform\webanimationdelegate.h(34)
: see declaration of 'blink::WebAnimationDelegate'
       
e:\b\build\slave\win\build\src\third_party\webkit\source\platform\graphics\graphicslayer.h(81)
: see declaration of 'WebCore::GraphicsLayer'
ninja: build stopped: subcommand failed.
--------------------------------------------------------------------------

Any ideas?

Tim

[jamesr, 2014-03-05 05:03:41]

I've no idea, the #pragma is supposed to prevent that from being an error.  My
windows box is currently out of sorts so I can't investigate locally.  Add the
export on the base class I guess, even though it's useless and will just bloat
the exports for no good reason :(

[mithro-old, 2014-03-05 05:34:59]

On 2014/03/05 05:03:41, jamesr wrote:
> I've no idea, the #pragma is supposed to prevent that from being an error.  My
> windows box is currently out of sorts so I can't investigate locally.  Add the
> export on the base class I guess, even though it's useless and will just bloat
> the exports for no good reason :(

I can see three options;
 * Live with an extra export for the day(s) it takes to land the complete 3
patch series. The EXPORT can be removed in
https://codereview.chromium.org/185393005/ when the class becomes a pure virtual
again.
 * Change to using ifdefs method of doing the two sided patch (commit to chrome
with an ifdef, remove code from blink, remove ifdefs from chrome)
 * Continue to try and figure out what is going on with the pragma and why it's
not working.

Which would you prefer? I've got a couple of other patches pending behind this
one, so I'd like to get these three into the queue ASAP.

Thanks for all your help.

Tim

[jamesr, 2014-03-05 06:03:40]

The pragma issue will not go away when the interface becomes pure virtual
again nor does it depends on the chromium side. Something is busted with
the warning levels. It's not worth spending more time on in this patch. I
would just land with the export annotation and not worry about it.

To unsubscribe from this group and stop receiving emails from it, send an email
to blink-reviews+unsubscribe@chromium.org.

[mithro-old, 2014-03-05 06:13:48]

The CQ bit was checked by mithro@mithis.com

[commit-bot: I haz the power, 2014-03-05 06:14:56]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mithro@mithis.com/185633002/200001

[mithro-old, 2014-03-05 06:21:39]

On 2014/03/05 06:03:40, jamesr wrote:
> The pragma issue will not go away when the interface becomes pure virtual
> again nor does it depends on the chromium side. Something is busted with
> the warning levels. It's not worth spending more time on in this patch. I
> would just land with the export annotation and not worry about it.

Okay, landing this patch.

From what I've seen, Windows *does* stop having the warning when
WebAnimationDelegate is a pure virtual class. It would also explain why the
warning never failed in this case before (as WebAnimationDelegate previously was
pure virtual too).

Can you also please take a look at https://codereview.chromium.org/185393005/ .
Once this CL lands, that CL will prove the above or not. (That CL can't land
until the Chrome one does however.)

[commit-bot: I haz the power, 2014-03-05 20:56:52]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mithro@mithis.com/185633002/200001

[commit-bot: I haz the power, 2014-03-06 03:50:14]

Change committed as 168560