gl: Remove redundant methods/variables from GLSurfaceFormat

gl: Remove redundant methods/variables from GLSurfaceFormat

This commit removes IsDefault(), SetIsSurfaceless() and
IsSurfaceless() from GLSurfaceFormat.
GLSurface::IsSurfaceless() can be directly used instead
of GLSurfaceFormat::IsSurfaceless().

BUG=704581
Review-Url: https://codereview.chromium.org/2764853002
Cr-Commit-Position: refs/heads/master@{#459252}
Committed: https://chromium.googlesource.com/chromium/src/+/bedc60e74ddc887d5599b468c83191f992311458

[Chandan, 2017-03-21 11:00:58]

Description was changed from

==========
gl: Remove few redundant methods/variables from GLSurfaceFormat

This commit removes IsDefault(), SetIsSurfaceless() and
IsSurfaceless() from GLSurfaceFormat.
GLSurface::IsSurfaceless() can be directly used instead
of GLSurfaceFormat::IsSurfaceless().

BUG=None
==========

to

==========
gl: Remove few redundant methods/variables from GLSurfaceFormat

This commit removes IsDefault(), SetIsSurfaceless() and
IsSurfaceless() from GLSurfaceFormat.
GLSurface::IsSurfaceless() can be directly used instead
of GLSurfaceFormat::IsSurfaceless().

BUG=None
==========

[Chandan, 2017-03-21 11:00:58]

c.padhi@samsung.com changed reviewers:
+ a.suchit@chromium.org, klausw@chromium.org

[Chandan, 2017-03-21 11:01:13]

On 2017/03/21 11:00:58, Chandan wrote:
> mailto:c.padhi@samsung.com changed reviewers:
> + mailto:a.suchit@chromium.org, mailto:klausw@chromium.org

PTAL!

[Chandan, 2017-03-22 07:27:21]

c.padhi@samsung.com changed reviewers:
+ jbauman@chromium.org, kbr@chromium.org, piman@chromium.org

[Chandan, 2017-03-22 07:27:43]

c.padhi@samsung.com changed reviewers:
+ uzair.jaleel@samsung.com

[Chandan, 2017-03-22 07:28:05]

On 2017/03/22 07:27:21, Chandan wrote:
> mailto:c.padhi@samsung.com changed reviewers:
> + mailto:jbauman@chromium.org, mailto:kbr@chromium.org,
mailto:piman@chromium.org

@reviewers, PTAL. Thank you.

[Chandan, 2017-03-22 11:56:51]

The CQ bit was checked by c.padhi@samsung.com to run a CQ dry run

[commit-bot: I haz the power, 2017-03-22 11:57:04]

Dry run: 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-03-22 11:57:05]

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

[commit-bot: I haz the power, 2017-03-22 11:57:06]

Dry run: No L-G-T-M from a valid reviewer yet.
CQ run can only be started once the patch has received an L-G-T-M from a full
committer.
Even if an L-G-T-M may have been provided, it was from a non-committer,_not_ a
full super star committer.
Committers are members of the group "project-chromium-committers".
Note that this has nothing to do with OWNERS files.

[piman, 2017-03-22 21:31:36]

https://codereview.chromium.org/2764853002/diff/1/ui/gl/init/gl_factory_ozone.cc
File ui/gl/init/gl_factory_ozone.cc (left):

https://codereview.chromium.org/2764853002/diff/1/ui/gl/init/gl_factory_ozone...
ui/gl/init/gl_factory_ozone.cc:91: }
Is it ok to remove this check? Why?

[Ken Russell (switch to Gerrit), 2017-03-22 21:36:47]

Please file a bug for changes like this and refer to any earlier bugs.

klausw@ touched this code most recently in
https://chromium.googlesource.com/chromium/src/+/ec8edae43017bf5ea65d3b65edbc...
, and I'm not sure whether this proposed CL takes things backward from where he
was thinking.

[klausw, 2017-03-22 21:49:21]

> klausw@ touched this code most recently in
>
https://chromium.googlesource.com/chromium/src/+/ec8edae43017bf5ea65d3b65edbc...
> , and I'm not sure whether this proposed CL takes things backward from where
he
> was thinking.

I'm in favor of this cleanup apart from the removed check.

My change in http://crrev.com/2616723002 was mainly aimed at replacing the
format enum with a class that behaves equivalently. I intentionally tried to
avoid any refactoring that didn't seem trivial, so I implemented a replacement
for the SURFACE_SURFACELESS enum value.

Assuming that this change is self consistent and works as expected, removing
this attribute would be an improvement.

https://codereview.chromium.org/2764853002/diff/1/ui/gl/init/gl_factory_ozone.cc
File ui/gl/init/gl_factory_ozone.cc (left):

https://codereview.chromium.org/2764853002/diff/1/ui/gl/init/gl_factory_ozone...
ui/gl/init/gl_factory_ozone.cc:91: }
On 2017/03/22 21:31:36, piman wrote:
> Is it ok to remove this check? Why?

Removing the check is *not* ok, we have a currently active bug where having this
check helped identify what's going on.

You could replace it with format.IsCompatible(GLSurfaceFormat()) or equivalent.
It's a bit less efficient than explicitly tracking changes away from default,
but I doubt it matters - surface creation is expensive, and a few extra
comparisons wouldn't be a big deal.

To avoid gratuitous construction just for comparison purposes, should we add a
GLSurfaceFormat::DEFAULT static const member or a static getter for something
equivalent?

[piman, 2017-03-22 21:59:35]

https://codereview.chromium.org/2764853002/diff/1/ui/gl/init/gl_factory_ozone.cc
File ui/gl/init/gl_factory_ozone.cc (left):

https://codereview.chromium.org/2764853002/diff/1/ui/gl/init/gl_factory_ozone...
ui/gl/init/gl_factory_ozone.cc:91: }
On 2017/03/22 21:49:21, klausw wrote:
> On 2017/03/22 21:31:36, piman wrote:
> > Is it ok to remove this check? Why?
> 
> Removing the check is *not* ok, we have a currently active bug where having
this
> check helped identify what's going on.
> 
> You could replace it with format.IsCompatible(GLSurfaceFormat()) or
equivalent.
> It's a bit less efficient than explicitly tracking changes away from default,
> but I doubt it matters - surface creation is expensive, and a few extra
> comparisons wouldn't be a big deal.
> 
> To avoid gratuitous construction just for comparison purposes, should we add a
> GLSurfaceFormat::DEFAULT static const member or a static getter for something
> equivalent?

Meh. Default-constructing a GLSurfaceFormat on the stack is trivial compared to
even just the logging code, no need for fanciness here.

[Chandan, 2017-03-23 12:30:08]

Description was changed from

==========
gl: Remove few redundant methods/variables from GLSurfaceFormat

This commit removes IsDefault(), SetIsSurfaceless() and
IsSurfaceless() from GLSurfaceFormat.
GLSurface::IsSurfaceless() can be directly used instead
of GLSurfaceFormat::IsSurfaceless().

BUG=None
==========

to

==========
gl: Remove redundant methods/variables from GLSurfaceFormat

This commit removes IsDefault(), SetIsSurfaceless() and
IsSurfaceless() from GLSurfaceFormat.
GLSurface::IsSurfaceless() can be directly used instead
of GLSurfaceFormat::IsSurfaceless().

BUG=655722
==========

[Chandan, 2017-03-23 12:34:05]

On 2017/03/22 21:36:47, Ken Russell wrote:
> Please file a bug for changes like this and refer to any earlier bugs.
> 
> klausw@ touched this code most recently in
>
https://chromium.googlesource.com/chromium/src/+/ec8edae43017bf5ea65d3b65edbc...
> , and I'm not sure whether this proposed CL takes things backward from where
he
> was thinking.

Sure. I have added the same bug that was mentioned in
ec8edae43017bf5ea65d3b65edbcfd740af453c7.

[Chandan, 2017-03-23 12:34:27]

https://codereview.chromium.org/2764853002/diff/1/ui/gl/init/gl_factory_ozone.cc
File ui/gl/init/gl_factory_ozone.cc (left):

https://codereview.chromium.org/2764853002/diff/1/ui/gl/init/gl_factory_ozone...
ui/gl/init/gl_factory_ozone.cc:91: }
On 2017/03/22 21:49:21, klausw wrote:
> On 2017/03/22 21:31:36, piman wrote:
> > Is it ok to remove this check? Why?
> 
> Removing the check is *not* ok, we have a currently active bug where having
this
> check helped identify what's going on.
> 
> You could replace it with format.IsCompatible(GLSurfaceFormat()) or
equivalent.
> It's a bit less efficient than explicitly tracking changes away from default,
> but I doubt it matters - surface creation is expensive, and a few extra
> comparisons wouldn't be a big deal.
> 
> To avoid gratuitous construction just for comparison purposes, should we add a
> GLSurfaceFormat::DEFAULT static const member or a static getter for something
> equivalent?

Uploaded patch set 2 with format.IsCompatible(GLSurfaceFormat()).
PTAL.

[klausw, 2017-03-23 16:35:30]

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

[commit-bot: I haz the power, 2017-03-23 16:35:57]

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

[klausw, 2017-03-23 16:43:19]

On 2017/03/23 12:34:05, Chandan wrote:
> On 2017/03/22 21:36:47, Ken Russell wrote:
> > Please file a bug for changes like this and refer to any earlier bugs.
> > 
> > klausw@ touched this code most recently in
> >
>
https://chromium.googlesource.com/chromium/src/+/ec8edae43017bf5ea65d3b65edbc...
> > , and I'm not sure whether this proposed CL takes things backward from where
> he
> > was thinking.
> 
> Sure. I have added the same bug that was mentioned in
> ec8edae43017bf5ea65d3b65edbcfd740af453c7.

LGTM but see below:

I filed a new bug http://crbug.com/704581 , please use that one instead. The bug
you linked is closed and not really appropriate for this refactor.

I've kicked off a commit queue dry run to check for build or test issues. Due to
the wide variety of systems involved, it's a good sanity check to make sure this
didn't miss any existing usage.

[commit-bot: I haz the power, 2017-03-23 17:33:18]

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

[commit-bot: I haz the power, 2017-03-23 17:33:19]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Chandan, 2017-03-23 18:46:45]

Description was changed from

==========
gl: Remove redundant methods/variables from GLSurfaceFormat

This commit removes IsDefault(), SetIsSurfaceless() and
IsSurfaceless() from GLSurfaceFormat.
GLSurface::IsSurfaceless() can be directly used instead
of GLSurfaceFormat::IsSurfaceless().

BUG=655722
==========

to

==========
gl: Remove redundant methods/variables from GLSurfaceFormat

This commit removes IsDefault(), SetIsSurfaceless() and
IsSurfaceless() from GLSurfaceFormat.
GLSurface::IsSurfaceless() can be directly used instead
of GLSurfaceFormat::IsSurfaceless().

BUG=704581
==========

[Chandan, 2017-03-23 18:48:20]

On 2017/03/23 16:43:19, klausw wrote:
> On 2017/03/23 12:34:05, Chandan wrote:
> > On 2017/03/22 21:36:47, Ken Russell wrote:
> > > Please file a bug for changes like this and refer to any earlier bugs.
> > > 
> > > klausw@ touched this code most recently in
> > >
> >
>
https://chromium.googlesource.com/chromium/src/+/ec8edae43017bf5ea65d3b65edbc...
> > > , and I'm not sure whether this proposed CL takes things backward from
where
> > he
> > > was thinking.
> > 
> > Sure. I have added the same bug that was mentioned in
> > ec8edae43017bf5ea65d3b65edbcfd740af453c7.
> 
> LGTM but see below:
> 
> I filed a new bug http://crbug.com/704581 , please use that one instead. The
bug
> you linked is closed and not really appropriate for this refactor.
> 
> I've kicked off a commit queue dry run to check for build or test issues. Due
to
> the wide variety of systems involved, it's a good sanity check to make sure
this
> didn't miss any existing usage.

Thank you. Updated bug id.

[Chandan, 2017-03-23 18:48:38]

The CQ bit was checked by c.padhi@samsung.com

[commit-bot: I haz the power, 2017-03-23 18:49:34]

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-03-23 18:49:35]

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

[commit-bot: I haz the power, 2017-03-23 18:49:36]

No L-G-T-M from a valid reviewer yet.
CQ run can only be started once the patch has received an L-G-T-M from a full
committer.
Even if an L-G-T-M may have been provided, it was from a non-committer,_not_ a
full super star committer.
Committers are members of the group "project-chromium-committers".
Note that this has nothing to do with OWNERS files.

[piman, 2017-03-23 20:34:01]

lgtm

[Chandan, 2017-03-23 20:35:19]

The CQ bit was checked by c.padhi@samsung.com

[commit-bot: I haz the power, 2017-03-23 20:36:04]

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-03-23 21:31:54]

CQ is committing da patch.

Bot data: {"patchset_id": 20001, "attempt_start_ts": 1490301319006590,
"parent_rev": "123744c5850b84cf7f3ef9228b6d64a281732443", "commit_rev":
"8e88e20905a944556a4da43094249de0a1cad16d"}

[commit-bot: I haz the power, 2017-03-23 21:50:43]

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

[commit-bot: I haz the power, 2017-03-23 21:50:46]

Prior attempt to commit was detected, but we were not able to check whether the
issue was successfully committed. Please check Git history manually and re-check
CQ or close this issue as needed.

[klausw, 2017-03-23 22:09:47]

The CQ bit was checked by klausw@chromium.org

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

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-03-23 22:20:18]

CQ is committing da patch.

Bot data: {"patchset_id": 20001, "attempt_start_ts": 1490306987786630,
"parent_rev": "d982fdfedc982d24055caff18bb07852116d5fda", "commit_rev":
"bedc60e74ddc887d5599b468c83191f992311458"}

[commit-bot: I haz the power, 2017-03-23 22:21:06]

Description was changed from

==========
gl: Remove redundant methods/variables from GLSurfaceFormat

This commit removes IsDefault(), SetIsSurfaceless() and
IsSurfaceless() from GLSurfaceFormat.
GLSurface::IsSurfaceless() can be directly used instead
of GLSurfaceFormat::IsSurfaceless().

BUG=704581
==========

to

==========
gl: Remove redundant methods/variables from GLSurfaceFormat

This commit removes IsDefault(), SetIsSurfaceless() and
IsSurfaceless() from GLSurfaceFormat.
GLSurface::IsSurfaceless() can be directly used instead
of GLSurfaceFormat::IsSurfaceless().

BUG=704581

Review-Url: https://codereview.chromium.org/2764853002
Cr-Commit-Position: refs/heads/master@{#459252}
Committed:
https://chromium.googlesource.com/chromium/src/+/bedc60e74ddc887d5599b468c831...
==========

[commit-bot: I haz the power, 2017-03-23 22:21:08]

Committed patchset #2 (id:20001) as
https://chromium.googlesource.com/chromium/src/+/bedc60e74ddc887d5599b468c831...

[klausw, 2017-03-23 23:05:34]

On 2017/03/23 22:21:08, commit-bot: I haz the power wrote:
> Committed patchset #2 (id:20001) as
>
https://chromium.googlesource.com/chromium/src/+/bedc60e74ddc887d5599b468c831...

I re-did the commit since it hadn't gone through earlier, it worked fine this
time. I closed the bug. Thank you for doing this!