Make surface GetFormat pure virtual, add missing overrides.

Make surface GetFormat pure virtual, add missing overrides.

The base fallback with NOTIMPLEMENTED() warning was unhelpful,
change it to explicitly require overrides in subclasses so
there's no ambiguity if the default is ok or not.

BUG=694857
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel

Review-Url: https://codereview.chromium.org/2702403009
Cr-Commit-Position: refs/heads/master@{#452695}
Committed: https://chromium.googlesource.com/chromium/src/+/10aaaf5a41d02653d144a4e78c43c29974860e39

[klausw, 2017-02-23 20:22:43]

klausw@chromium.org changed reviewers:
+ jbauman@chromium.org

[klausw, 2017-02-23 20:22:53]

The CQ bit was checked by klausw@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-23 20:23:23]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[jbauman, 2017-02-23 20:32:33]

lgtm

[commit-bot: I haz the power, 2017-02-23 20:44:20]

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

[commit-bot: I haz the power, 2017-02-23 20:44:21]

Dry run: Try jobs failed on following builders:
  android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)

[klausw, 2017-02-23 20:51:46]

klausw@chromium.org changed reviewers:
+ boliu@chromium.org

[klausw, 2017-02-23 20:51:47]

boliu@chromium.org: Please review changes in android_webview

[klausw, 2017-02-23 20:51:56]

The CQ bit was checked by klausw@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-23 20:52:30]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[klausw, 2017-02-23 20:59:45]

The CQ bit was unchecked by klausw@chromium.org

[klausw, 2017-02-23 20:59:54]

Description was changed from

==========
Make surface GetFormat pure virtual, add missing overrides.

The base fallback with NOTIMPLEMENTED() warning was unhelpful,
change it to explicitly require overrides in subclasses so
there's no ambiguity if the default is ok or not.

BUG=694857
==========

to

==========
Make surface GetFormat pure virtual, add missing overrides.

The base fallback with NOTIMPLEMENTED() warning was unhelpful,
change it to explicitly require overrides in subclasses so
there's no ambiguity if the default is ok or not.

BUG=694857
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel
==========

[klausw, 2017-02-23 21:13:15]

The CQ bit was checked by klausw@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-23 21:13:49]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[boliu, 2017-02-23 21:33:32]

https://codereview.chromium.org/2702403009/diff/40001/android_webview/browser...
File android_webview/browser/aw_gl_surface.cc (right):

https://codereview.chromium.org/2702403009/diff/40001/android_webview/browser...
android_webview/browser/aw_gl_surface.cc:43: return GLSurfaceFormat();
add NOTIMPLEMENTED here?

[commit-bot: I haz the power, 2017-02-23 21:35:53]

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

[commit-bot: I haz the power, 2017-02-23 21:35:54]

Dry run: Try jobs failed on following builders:
  android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)

[boliu, 2017-02-23 21:40:09]

On 2017/02/23 21:33:32, boliu wrote:
>
https://codereview.chromium.org/2702403009/diff/40001/android_webview/browser...
> File android_webview/browser/aw_gl_surface.cc (right):
> 
>
https://codereview.chromium.org/2702403009/diff/40001/android_webview/browser...
> android_webview/browser/aw_gl_surface.cc:43: return GLSurfaceFormat();
> add NOTIMPLEMENTED here?

lgtm after that and fixing compile

[klausw, 2017-02-23 22:08:47]

https://codereview.chromium.org/2702403009/diff/40001/android_webview/browser...
File android_webview/browser/aw_gl_surface.cc (right):

https://codereview.chromium.org/2702403009/diff/40001/android_webview/browser...
android_webview/browser/aw_gl_surface.cc:43: return GLSurfaceFormat();
On 2017/02/23 21:33:31, boliu wrote:
> add NOTIMPLEMENTED here?

No, this is the complete implementation already. Requiring the overrides makes
it explicit that there's no need to distinguish implementation-dependent format
properties.

I've added comments to gl_surface_format.h to make the intended semantics a bit
clearer.

[klausw, 2017-02-23 22:09:04]

The CQ bit was checked by klausw@chromium.org

[klausw, 2017-02-23 22:09:04]

The patchset sent to the CQ was uploaded after l-g-t-m from
jbauman@chromium.org, boliu@chromium.org
Link to the patchset: https://codereview.chromium.org/2702403009/#ps80001
(title: "Fix type namespaces, add Windows overides, add comments.")

[commit-bot: I haz the power, 2017-02-23 22:10:02]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[klausw, 2017-02-23 22:25:21]

The CQ bit was unchecked by klausw@chromium.org

[klausw, 2017-02-23 22:25:46]

The CQ bit was checked by klausw@chromium.org

[klausw, 2017-02-23 22:25:47]

The patchset sent to the CQ was uploaded after l-g-t-m from boliu@chromium.org,
jbauman@chromium.org
Link to the patchset: https://codereview.chromium.org/2702403009/#ps100001
(title: "Fix another missing prefix")

[commit-bot: I haz the power, 2017-02-23 22:26:41]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-23 23:04:05]

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

[commit-bot: I haz the power, 2017-02-23 23:04:07]

Try jobs failed on following builders:
  win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)
  win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[Ken Russell (switch to Gerrit), 2017-02-23 23:07:31]

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

[Ken Russell (switch to Gerrit), 2017-02-23 23:08:17]

Let's please fix up the compile failures and get this in ASAP. It's spamming the
bots' logs badly. Thanks.

[klausw, 2017-02-23 23:13:17]

The CQ bit was checked by klausw@chromium.org

[klausw, 2017-02-23 23:13:17]

The patchset sent to the CQ was uploaded after l-g-t-m from boliu@chromium.org,
jbauman@chromium.org
Link to the patchset: https://codereview.chromium.org/2702403009/#ps120001
(title: "Fix class name for Windows")

[commit-bot: I haz the power, 2017-02-23 23:14:09]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[klausw, 2017-02-23 23:24:27]

On 2017/02/23 23:08:17, Ken Russell wrote:
> Let's please fix up the compile failures and get this in ASAP. It's spamming
the
> bots' logs badly. Thanks.

Sorry about the noise, I just kicked off a retry. Is there a better way to try
compilation on platforms I don't have available locally? (Other than the obvious
"just get it right in the first place"...)

Looks like this is an unfortunate side effect of r452313 "Fix virtualized
contexts on low-spec Android devices" which adds a GetFormat call for the
default offscreen surface. The offending NOTIMPLEMENTED() itself has been
present since at least 2011, but apparently no other code had been looking at
the format on these platforms previously.

[Ken Russell (switch to Gerrit), 2017-02-24 00:37:30]

Description was changed from

==========
Make surface GetFormat pure virtual, add missing overrides.

The base fallback with NOTIMPLEMENTED() warning was unhelpful,
change it to explicitly require overrides in subclasses so
there's no ambiguity if the default is ok or not.

BUG=694857
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel
==========

to

==========
Make surface GetFormat pure virtual, add missing overrides.

The base fallback with NOTIMPLEMENTED() warning was unhelpful,
change it to explicitly require overrides in subclasses so
there's no ambiguity if the default is ok or not.

BUG=694857
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel
==========

[Ken Russell (switch to Gerrit), 2017-02-24 00:38:05]

On 2017/02/23 23:24:27, klausw wrote:
> On 2017/02/23 23:08:17, Ken Russell wrote:
> > Let's please fix up the compile failures and get this in ASAP. It's spamming
> the
> > bots' logs badly. Thanks.
> 
> Sorry about the noise, I just kicked off a retry. Is there a better way to try
> compilation on platforms I don't have available locally? (Other than the
obvious
> "just get it right in the first place"...)
> 
> Looks like this is an unfortunate side effect of r452313 "Fix virtualized
> contexts on low-spec Android devices" which adds a GetFormat call for the
> default offscreen surface. The offending NOTIMPLEMENTED() itself has been
> present since at least 2011, but apparently no other code had been looking at
> the format on these platforms previously.

Understood. Still, the large volume of logs is causing problems. I wouldn't be
surprised if some of the excess load we've seen on Chromium's buildbot masters
over the past couple of days is due to the increase in log volume.

[commit-bot: I haz the power, 2017-02-24 00:50:57]

CQ is committing da patch.

Bot data: {"patchset_id": 120001, "attempt_start_ts": 1487891597289530,
"parent_rev": "1e4f91b70e9ad4d92b64c85fd8094a31fa0b5c3e", "commit_rev":
"10aaaf5a41d02653d144a4e78c43c29974860e39"}

[commit-bot: I haz the power, 2017-02-24 00:51:38]

Description was changed from

==========
Make surface GetFormat pure virtual, add missing overrides.

The base fallback with NOTIMPLEMENTED() warning was unhelpful,
change it to explicitly require overrides in subclasses so
there's no ambiguity if the default is ok or not.

BUG=694857
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel
==========

to

==========
Make surface GetFormat pure virtual, add missing overrides.

The base fallback with NOTIMPLEMENTED() warning was unhelpful,
change it to explicitly require overrides in subclasses so
there's no ambiguity if the default is ok or not.

BUG=694857
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel

Review-Url: https://codereview.chromium.org/2702403009
Cr-Commit-Position: refs/heads/master@{#452695}
Committed:
https://chromium.googlesource.com/chromium/src/+/10aaaf5a41d02653d144a4e78c43...
==========

[commit-bot: I haz the power, 2017-02-24 00:51:39]

Committed patchset #7 (id:120001) as
https://chromium.googlesource.com/chromium/src/+/10aaaf5a41d02653d144a4e78c43...

[klausw, 2017-02-24 01:05:20]

On 2017/02/24 00:38:05, Ken Russell wrote:
> Understood. Still, the large volume of logs is causing problems. I wouldn't be
> surprised if some of the excess load we've seen on Chromium's buildbot masters
> over the past couple of days is due to the increase in log volume.

Please let me know ASAP in case this isn't solved by the just-submitted patch or
if you run into other related issues.

I saw earlier that NOTIMPLEMENTED messages are completely suppressed in the
build flavor I was using for testing the patch that introduced this, which
explains why I hadn't noticed the message :-/ 

#if defined(OS_ANDROID) && defined(OFFICIAL_BUILD)
#define NOTIMPLEMENTED_POLICY 0