Refactors std::vector<Display> out of ScreenMus

Refactors std::vector<Display> out of ScreenMus

This way it's easier to have a test based version that uses similar
code, which I've added here too.

BUG=603369
TEST=none
R=sadrul@chromium.org

Committed: https://crrev.com/7afbf7c1f9178c11282c278b7e93f8cb3922b712
Cr-Commit-Position: refs/heads/master@{#394259}

[sadrul, 2016-05-17 22:49:22]

I was thinking DisplayList could live in ui/display, and display::Screen could
use it. But for now, this lgtm

[sky, 2016-05-17 22:56:39]

The CQ bit was checked by sky@chromium.org

[commit-bot: I haz the power, 2016-05-17 22:57:47]

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

[commit-bot: I haz the power, 2016-05-17 23:07:51]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-05-17 23:09:29]

Description was changed from

==========
Refactors std::vector<Display> out of ScreenMus

This way it's easier to have a test based version that uses similar
code, which I've added here too.

BUG=603369
TEST=none
R=sadrul@chromium.org
==========

to

==========
Refactors std::vector<Display> out of ScreenMus

This way it's easier to have a test based version that uses similar
code, which I've added here too.

BUG=603369
TEST=none
R=sadrul@chromium.org

Committed: https://crrev.com/7afbf7c1f9178c11282c278b7e93f8cb3922b712
Cr-Commit-Position: refs/heads/master@{#394259}
==========

[commit-bot: I haz the power, 2016-05-17 23:09:33]

Patchset 4 (id:??) landed as
https://crrev.com/7afbf7c1f9178c11282c278b7e93f8cb3922b712
Cr-Commit-Position: refs/heads/master@{#394259}

[Nico, 2016-05-27 13:38:06]

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

[Nico, 2016-05-27 13:38:07]

https://codereview.chromium.org/1984393002/diff/60001/mash/wm/test/wm_test_sc...
File mash/wm/test/wm_test_screen.cc (right):

https://codereview.chromium.org/1984393002/diff/60001/mash/wm/test/wm_test_sc...
mash/wm/test/wm_test_screen.cc:11: #define NOTIMPLEMENTED() DVLOG(1) <<
"notimplemented"
…what? If NOTIMPLEMENTED doesn't do what you want, why not add your own macro
instead of redefining a well-known macro?

[sky, 2016-05-27 15:23:46]

The meaning of NOTIMPLEMENTED is well understood. The reason I
redefined it is while we're in the process of getting this code
working there is way too much log spam. I suppose I could have went
with something like IMPLEMENT_ME instead of NOTIMPLEMENTED, but that
doesn't feel satisfying... Others have taken to commenting out the
NOTIMPLEMENTED which is equally unsatisfying.

  -Scott

On Fri, May 27, 2016 at 6:38 AM,  <thakis@chromium.org> wrote:
>
>
https://codereview.chromium.org/1984393002/diff/60001/mash/wm/test/wm_test_sc...
> File mash/wm/test/wm_test_screen.cc (right):
>
>
https://codereview.chromium.org/1984393002/diff/60001/mash/wm/test/wm_test_sc...
> mash/wm/test/wm_test_screen.cc:11: #define NOTIMPLEMENTED() DVLOG(1) <<
> "notimplemented"
> …what? If NOTIMPLEMENTED doesn't do what you want, why not add your own
> macro instead of redefining a well-known macro?
>
> https://codereview.chromium.org/1984393002/

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[sadrul, 2016-05-27 15:29:02]

On 2016/05/27 15:23:46, sky wrote:
> The meaning of NOTIMPLEMENTED is well understood. The reason I
> redefined it is while we're in the process of getting this code
> working there is way too much log spam. I suppose I could have went
> with something like IMPLEMENT_ME instead of NOTIMPLEMENTED, but that
> doesn't feel satisfying... Others have taken to commenting out the
> NOTIMPLEMENTED which is equally unsatisfying.

I am in favour of defining NOTIMPLEMENTED_POLICY to 5 instead
(https://code.google.com/p/chromium/codesearch#chromium/src/base/logging.h&sq=...),
although it doesn't look like we ever use it anywhere.

[Nico, 2016-05-27 15:35:40]

On 2016/05/27 15:29:02, sadrul wrote:
> On 2016/05/27 15:23:46, sky wrote:
> > The meaning of NOTIMPLEMENTED is well understood. The reason I
> > redefined it is while we're in the process of getting this code
> > working there is way too much log spam. I suppose I could have went
> > with something like IMPLEMENT_ME instead of NOTIMPLEMENTED, but that
> > doesn't feel satisfying... Others have taken to commenting out the
> > NOTIMPLEMENTED which is equally unsatisfying.
> 
> I am in favour of defining NOTIMPLEMENTED_POLICY to 5 instead
>
(https://code.google.com/p/chromium/codesearch#chromium/src/base/logging.h&sq=...),
> although it doesn't look like we ever use it anywhere.

I was looking at that today (which is how I found this CL). I think we should
get rid of some (all?) of these policies, and we should probably default to not
printing anything for NOTIMPLEMENTED in at least official release builds (like
we already do for android).

It looks like base/notimplemented.h was added to help with porting to mac and
linux long ago, and nowadays NOTIMPLEMENTED has somewhat unclear semantics and
hundreds and hundreds of uses. Some of them look like a TODO comment that stays
around for years, except that it also adds bloat to our binary.

[sadrul, 2016-05-27 16:03:40]

On 2016/05/27 15:35:40, Nico (away until Tue May 31) wrote:
> On 2016/05/27 15:29:02, sadrul wrote:
> > On 2016/05/27 15:23:46, sky wrote:
> > > The meaning of NOTIMPLEMENTED is well understood. The reason I
> > > redefined it is while we're in the process of getting this code
> > > working there is way too much log spam. I suppose I could have went
> > > with something like IMPLEMENT_ME instead of NOTIMPLEMENTED, but that
> > > doesn't feel satisfying... Others have taken to commenting out the
> > > NOTIMPLEMENTED which is equally unsatisfying.
> > 
> > I am in favour of defining NOTIMPLEMENTED_POLICY to 5 instead
> >
>
(https://code.google.com/p/chromium/codesearch#chromium/src/base/logging.h&sq=...),
> > although it doesn't look like we ever use it anywhere.
> 
> I was looking at that today (which is how I found this CL). I think we should
> get rid of some (all?) of these policies, and we should probably default to
not
> printing anything for NOTIMPLEMENTED in at least official release builds (like
> we already do for android).
> 
> It looks like base/notimplemented.h was added to help with porting to mac and
> linux long ago, and nowadays NOTIMPLEMENTED has somewhat unclear semantics and
> hundreds and hundreds of uses. Some of them look like a TODO comment that
stays
> around for years, except that it also adds bloat to our binary.

I feel like there will often be a need for NOTIMPLEMENTED()s, but perhaps we
could both require a crbug referenced from the macro, and default to 'print just
once'?

[sky, 2016-05-27 16:37:38]

I believe Ben commented the other day setting the policy to 5 didn't
work, by which he means it still spammed the console.

I like Sadrul's suggestion of making NOTIMPLEMENTED only log once for
a particular location.

  -Scott

On Fri, May 27, 2016 at 8:29 AM,  <sadrul@chromium.org> wrote:
> On 2016/05/27 15:23:46, sky wrote:
>> The meaning of NOTIMPLEMENTED is well understood. The reason I
>> redefined it is while we're in the process of getting this code
>> working there is way too much log spam. I suppose I could have went
>> with something like IMPLEMENT_ME instead of NOTIMPLEMENTED, but that
>> doesn't feel satisfying... Others have taken to commenting out the
>> NOTIMPLEMENTED which is equally unsatisfying.
>
> I am in favour of defining NOTIMPLEMENTED_POLICY to 5 instead
>
(https://code.google.com/p/chromium/codesearch#chromium/src/base/logging.h&sq=...),
> although it doesn't look like we ever use it anywhere.
>
> https://codereview.chromium.org/1984393002/

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.