Add more options to --screen-config flag.

Add more options to --screen-config flag.

Allow the user to specify more options for each DisplaySnapshot created
by FakeDisplayDelegate. Alternate resolutions, different refresh rates
and various option toggles are added.

Add tests for FakeDisplaySnapshot that verify the display specification
string parsing.

BUG=611475
Committed: https://crrev.com/3cabb1432a49769f618d868b218ad119709560d7
Cr-Commit-Position: refs/heads/master@{#425084}

[kylechar, 2016-10-05 20:30:41]

kylechar@chromium.org changed reviewers:
+ derat@chromium.org

[Daniel Erat, 2016-10-05 21:09:46]

cool, i mostly just have nits

https://codereview.chromium.org/2395873002/diff/20001/ui/display/fake_display...
File ui/display/fake_display_delegate.h (right):

https://codereview.chromium.org/2395873002/diff/20001/ui/display/fake_display...
ui/display/fake_display_delegate.h:30: //  
HxW[%R][#HxW[%R][:HxW[%R]]][^D][/[a][c][i][o]][,...]
can you document what the #- and :-prefixed dimensions do? i take it from the
example below that '#' is used to list a second mode.

https://codereview.chromium.org/2395873002/diff/20001/ui/display/fake_display...
File ui/display/fake_display_snapshot.cc (right):

https://codereview.chromium.org/2395873002/diff/20001/ui/display/fake_display...
ui/display/fake_display_snapshot.cc:19: using base::StringPiece;
nit: move this up above "namespace display"?

https://codereview.chromium.org/2395873002/diff/20001/ui/display/fake_display...
ui/display/fake_display_snapshot.cc:72: StringPiece SplitFromString(StringPiece*
str, StringPiece delimiter) {
maybe a name like "ExtractSuffix" would be clearer?

https://codereview.chromium.org/2395873002/diff/20001/ui/display/fake_display...
ui/display/fake_display_snapshot.cc:84: // Parses a display mode from a string
in the format HxW[%R], returns null if
nit: s/returns/returning/

https://codereview.chromium.org/2395873002/diff/20001/ui/display/fake_display...
ui/display/fake_display_snapshot.cc:85: // string is not valid format.
nit: s/not valid format/invalid/

https://codereview.chromium.org/2395873002/diff/20001/ui/display/fake_display...
ui/display/fake_display_snapshot.cc:91: // Refresh rate is optional, will be be
60 if not specified.
nit: s/, will/ and will/

https://codereview.chromium.org/2395873002/diff/20001/ui/display/fake_display...
ui/display/fake_display_snapshot.cc:101: // Parses list of alternate display
modes, adding each new display mode to the
nit: s/list/a list/

https://codereview.chromium.org/2395873002/diff/20001/ui/display/fake_display...
ui/display/fake_display_snapshot.cc:105: if (resolutions.empty())
nit: no need for this early return, right? i'd assume that SplitString just
returns an empty vector if the input is empty

https://codereview.chromium.org/2395873002/diff/20001/ui/display/fake_display...
ui/display/fake_display_snapshot.cc:117: // Parses device DPI and set on display
snapshot.
nit: s/set on display snapshot/updates |builder|/ ?

https://codereview.chromium.org/2395873002/diff/20001/ui/display/fake_display...
ui/display/fake_display_snapshot.cc:119: if (dpi.empty())
this doesn't seem necessary either

https://codereview.chromium.org/2395873002/diff/20001/ui/display/fake_display...
ui/display/fake_display_snapshot.cc:130: if (options.empty())
nor this; the for loop should just be a no-op if the string is zero-length

https://codereview.chromium.org/2395873002/diff/20001/ui/display/fake_display...
ui/display/fake_display_snapshot.cc:344: HandleModes(&builder, resolutions);
it's fine if you leave it like this, but i wouldn't object to just inlining
these Handle* methods inside of this method since they're all tiny and only
called from this one place.

https://codereview.chromium.org/2395873002/diff/20001/ui/display/fake_display...
File ui/display/fake_display_snapshot.h (right):

https://codereview.chromium.org/2395873002/diff/20001/ui/display/fake_display...
ui/display/fake_display_snapshot.h:110: // Creates a display snapshot from the
provided |spec| string. Returns null if
mention the header where the spec format is described

https://codereview.chromium.org/2395873002/diff/20001/ui/display/fake_display...
File ui/display/fake_display_snapshot_unittests.cc (right):

https://codereview.chromium.org/2395873002/diff/20001/ui/display/fake_display...
ui/display/fake_display_snapshot_unittests.cc:113: 
maybe also test that you return null if things are passed in the wrong order,
since that seems like something someone might do accidentally?

[kylechar, 2016-10-07 16:14:44]

https://codereview.chromium.org/2395873002/diff/20001/ui/display/fake_display...
File ui/display/fake_display_delegate.h (right):

https://codereview.chromium.org/2395873002/diff/20001/ui/display/fake_display...
ui/display/fake_display_delegate.h:30: //  
HxW[%R][#HxW[%R][:HxW[%R]]][^D][/[a][c][i][o]][,...]
On 2016/10/05 21:09:45, Daniel Erat wrote:
> can you document what the #- and :-prefixed dimensions do? i take it from the
> example below that '#' is used to list a second mode.

Improved this comment a bunch I hope.

https://codereview.chromium.org/2395873002/diff/20001/ui/display/fake_display...
File ui/display/fake_display_snapshot.cc (right):

https://codereview.chromium.org/2395873002/diff/20001/ui/display/fake_display...
ui/display/fake_display_snapshot.cc:19: using base::StringPiece;
On 2016/10/05 21:09:45, Daniel Erat wrote:
> nit: move this up above "namespace display"?

Done.

https://codereview.chromium.org/2395873002/diff/20001/ui/display/fake_display...
ui/display/fake_display_snapshot.cc:72: StringPiece SplitFromString(StringPiece*
str, StringPiece delimiter) {
On 2016/10/05 21:09:45, Daniel Erat wrote:
> maybe a name like "ExtractSuffix" would be clearer?

Done.

https://codereview.chromium.org/2395873002/diff/20001/ui/display/fake_display...
ui/display/fake_display_snapshot.cc:84: // Parses a display mode from a string
in the format HxW[%R], returns null if
On 2016/10/05 21:09:45, Daniel Erat wrote:
> nit: s/returns/returning/

Done.

https://codereview.chromium.org/2395873002/diff/20001/ui/display/fake_display...
ui/display/fake_display_snapshot.cc:85: // string is not valid format.
On 2016/10/05 21:09:46, Daniel Erat wrote:
> nit: s/not valid format/invalid/

Done.

https://codereview.chromium.org/2395873002/diff/20001/ui/display/fake_display...
ui/display/fake_display_snapshot.cc:91: // Refresh rate is optional, will be be
60 if not specified.
On 2016/10/05 21:09:46, Daniel Erat wrote:
> nit: s/, will/ and will/

Done.

https://codereview.chromium.org/2395873002/diff/20001/ui/display/fake_display...
ui/display/fake_display_snapshot.cc:101: // Parses list of alternate display
modes, adding each new display mode to the
On 2016/10/05 21:09:46, Daniel Erat wrote:
> nit: s/list/a list/

Done.

https://codereview.chromium.org/2395873002/diff/20001/ui/display/fake_display...
ui/display/fake_display_snapshot.cc:105: if (resolutions.empty())
On 2016/10/05 21:09:46, Daniel Erat wrote:
> nit: no need for this early return, right? i'd assume that SplitString just
> returns an empty vector if the input is empty

Done.

https://codereview.chromium.org/2395873002/diff/20001/ui/display/fake_display...
ui/display/fake_display_snapshot.cc:117: // Parses device DPI and set on display
snapshot.
On 2016/10/05 21:09:46, Daniel Erat wrote:
> nit: s/set on display snapshot/updates |builder|/ ?

Done.

https://codereview.chromium.org/2395873002/diff/20001/ui/display/fake_display...
ui/display/fake_display_snapshot.cc:119: if (dpi.empty())
On 2016/10/05 21:09:46, Daniel Erat wrote:
> this doesn't seem necessary either

This has changed a bit, PTAL.

https://codereview.chromium.org/2395873002/diff/20001/ui/display/fake_display...
ui/display/fake_display_snapshot.cc:130: if (options.empty())
On 2016/10/05 21:09:46, Daniel Erat wrote:
> nor this; the for loop should just be a no-op if the string is zero-length

Done.

https://codereview.chromium.org/2395873002/diff/20001/ui/display/fake_display...
ui/display/fake_display_snapshot.cc:344: HandleModes(&builder, resolutions);
On 2016/10/05 21:09:46, Daniel Erat wrote:
> it's fine if you leave it like this, but i wouldn't object to just inlining
> these Handle* methods inside of this method since they're all tiny and only
> called from this one place.

I've modified all of this a bit and added return values.

https://codereview.chromium.org/2395873002/diff/20001/ui/display/fake_display...
File ui/display/fake_display_snapshot.h (right):

https://codereview.chromium.org/2395873002/diff/20001/ui/display/fake_display...
ui/display/fake_display_snapshot.h:110: // Creates a display snapshot from the
provided |spec| string. Returns null if
On 2016/10/05 21:09:46, Daniel Erat wrote:
> mention the header where the spec format is described

Done.

https://codereview.chromium.org/2395873002/diff/20001/ui/display/fake_display...
File ui/display/fake_display_snapshot_unittests.cc (right):

https://codereview.chromium.org/2395873002/diff/20001/ui/display/fake_display...
ui/display/fake_display_snapshot_unittests.cc:113: 
On 2016/10/05 21:09:46, Daniel Erat wrote:
> maybe also test that you return null if things are passed in the wrong order,
> since that seems like something someone might do accidentally?

Hmm, that's actually brings up a good point and the current implementation will
not notice a lot of errors (because sscanf happily ignores the end of the
string). I'll make the implementation stricter and add some more tests. I've
made it a bit stricter (PTAL) and added some tests to ensure.

[Daniel Erat, 2016-10-07 17:17:22]

thanks! i just have a few more comments.

https://codereview.chromium.org/2395873002/diff/60001/ui/display/fake_display...
File ui/display/fake_display_delegate.cc (right):

https://codereview.chromium.org/2395873002/diff/60001/ui/display/fake_display...
ui/display/fake_display_delegate.cc:7: #include <inttypes.h>
i was going to suggest including base/format_macros.h (which will also pull in
inttypes.h) instead, since i think it's a bit more cross-platform-friendly
(perhaps not that it matters here), but it looks like it and inttypes.h are
neck-and-neck in terms of usage, so fine to leave like this :-)

https://codereview.chromium.org/2395873002/diff/60001/ui/display/fake_display...
File ui/display/fake_display_snapshot.cc (right):

https://codereview.chromium.org/2395873002/diff/60001/ui/display/fake_display...
ui/display/fake_display_snapshot.cc:70: // Extracts text after specified
delimiter. If the delimiter doesn't exist then
nit: "If the delimiter doesn't appear exactly once ..."?

https://codereview.chromium.org/2395873002/diff/60001/ui/display/fake_display...
ui/display/fake_display_snapshot.cc:95: if (!RE2::FullMatch(str,
"\\d+x\\d+(?:%\\d+(?:\\.\\d*)?)?")) {
since you're already using a regexp, why not extract sub-patterns into variables
here instead of using sscanf() for that? if i'm reading re2.h correctly (search
for "CAVEAT"), the optional refresh rate needs to be extracted to a string
(which will be empty if it's not present).

https://codereview.chromium.org/2395873002/diff/60001/ui/display/fake_display...
File ui/display/fake_display_snapshot_unittests.cc (right):

https://codereview.chromium.org/2395873002/diff/60001/ui/display/fake_display...
ui/display/fake_display_snapshot_unittests.cc:43: EXPECT_EQ(120.0f,
display->native_mode()->refresh_rate());
mind adding a ModeToString() function that returns e.g. "1024x768%120" to the
anonymous namespace in this file? then you can simplify these checks to a single
statement:

  EXPECT_EQ("1024x768%120", ModeToString(display->native_mode()));

[kylechar, 2016-10-11 13:56:49]

https://codereview.chromium.org/2395873002/diff/60001/ui/display/fake_display...
File ui/display/fake_display_snapshot.cc (right):

https://codereview.chromium.org/2395873002/diff/60001/ui/display/fake_display...
ui/display/fake_display_snapshot.cc:70: // Extracts text after specified
delimiter. If the delimiter doesn't exist then
On 2016/10/07 17:17:22, Daniel Erat wrote:
> nit: "If the delimiter doesn't appear exactly once ..."?

Done.

https://codereview.chromium.org/2395873002/diff/60001/ui/display/fake_display...
ui/display/fake_display_snapshot.cc:95: if (!RE2::FullMatch(str,
"\\d+x\\d+(?:%\\d+(?:\\.\\d*)?)?")) {
On 2016/10/07 17:17:22, Daniel Erat wrote:
> since you're already using a regexp, why not extract sub-patterns into
variables
> here instead of using sscanf() for that? if i'm reading re2.h correctly
(search
> for "CAVEAT"), the optional refresh rate needs to be extracted to a string
> (which will be empty if it's not present).

For the caveat I don't think it applies to the regex in this patch. The (:?) is
a non-capture group so it's just used for matching. That caveat seems like it
would make it impossible to use re2 to extract an optional float value? However,
I just tried it out and it works fine for me. So I've removed the sscanf bit.

https://codereview.chromium.org/2395873002/diff/60001/ui/display/fake_display...
File ui/display/fake_display_snapshot_unittests.cc (right):

https://codereview.chromium.org/2395873002/diff/60001/ui/display/fake_display...
ui/display/fake_display_snapshot_unittests.cc:43: EXPECT_EQ(120.0f,
display->native_mode()->refresh_rate());
On 2016/10/07 17:17:22, Daniel Erat wrote:
> mind adding a ModeToString() function that returns e.g. "1024x768%120" to the
> anonymous namespace in this file? then you can simplify these checks to a
single
> statement:
> 
>   EXPECT_EQ("1024x768%120", ModeToString(display->native_mode()));

Hmmm, not sure I'm a fan of comparing strings. How about a function that does
the three EXPECT_EQ() calls instead? It would accomplish the same
simplification.

It actually seems like a proper matcher would be ideal here. I actually wrote up
a full gmock matcher based on testing::MatcherInterface<> but it's a ton of
boilerplate. So that doesn't seem worth it, at least in this case.

[kylechar, 2016-10-11 14:01:53]

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

[kylechar, 2016-10-11 14:01:54]

+thakis for DEPS on re2

[Daniel Erat, 2016-10-11 15:50:06]

https://codereview.chromium.org/2395873002/diff/80001/ui/display/fake_display...
File ui/display/fake_display_snapshot_unittests.cc (right):

https://codereview.chromium.org/2395873002/diff/80001/ui/display/fake_display...
ui/display/fake_display_snapshot_unittests.cc:22: void ExpectDisplayMode(const
ui::DisplayMode* mode,
the main reason i don't like helper functions like this is that if any of these
expectations fail, i believe that you'll just get a useless error message
pointing toward line 27, 28, or 29. i guess you're just doing one comparison in
most of the tests, which makes it a bit easier to pin down where the problem is,
but if you instead do string comparisons and have a function to format a mode as
a string, then the expectation failure will be in the test itself.

[kylechar, 2016-10-11 19:09:30]

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

[commit-bot: I haz the power, 2016-10-11 19:09:58]

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, 2016-10-11 20:33:28]

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

[commit-bot: I haz the power, 2016-10-11 20:33:28]

Dry run: This issue passed the CQ dry run.

[kylechar, 2016-10-11 20:39:04]

If you're okay with the matcher approach then it should probably move to
//ui/display/test but I want to double check before doing more work on it.

https://codereview.chromium.org/2395873002/diff/80001/ui/display/fake_display...
File ui/display/fake_display_snapshot_unittests.cc (right):

https://codereview.chromium.org/2395873002/diff/80001/ui/display/fake_display...
ui/display/fake_display_snapshot_unittests.cc:22: void ExpectDisplayMode(const
ui::DisplayMode* mode,
On 2016/10/11 15:50:06, Daniel Erat wrote:
> the main reason i don't like helper functions like this is that if any of
these
> expectations fail, i believe that you'll just get a useless error message
> pointing toward line 27, 28, or 29. i guess you're just doing one comparison
in
> most of the tests, which makes it a bit easier to pin down where the problem
is,
> but if you instead do string comparisons and have a function to format a mode
as
> a string, then the expectation failure will be in the test itself. 

Hmm. That's a good point. Although I'm weary about printing float values and
comparing them.

I've upload the patch with the matcher in it, since it's already done, which
fixes both of the problems. WDYT?

[Daniel Erat, 2016-10-11 21:00:31]

thanks, the matcher-based approach looks great. lgtm with a few comments

https://codereview.chromium.org/2395873002/diff/100001/ui/display/fake_displa...
File ui/display/fake_display_snapshot_unittests.cc (right):

https://codereview.chromium.org/2395873002/diff/100001/ui/display/fake_displa...
ui/display/fake_display_snapshot_unittests.cc:30: virtual bool
MatchAndExplain(const ui::DisplayMode& mode,
if these are coming from MatcherInterface, you should use 'override' rather than
'virtual'

https://codereview.chromium.org/2395873002/diff/100001/ui/display/fake_displa...
ui/display/fake_display_snapshot_unittests.cc:32: return mode.size() == size_ &&
mode.refresh_rate() == refresh_rate_;
nit: maybe compare the rate difference against some epsilon, since you said you
were worried about floating-point errors before?

[kylechar, 2016-10-12 13:24:50]

https://codereview.chromium.org/2395873002/diff/100001/ui/display/fake_displa...
File ui/display/fake_display_snapshot_unittests.cc (right):

https://codereview.chromium.org/2395873002/diff/100001/ui/display/fake_displa...
ui/display/fake_display_snapshot_unittests.cc:30: virtual bool
MatchAndExplain(const ui::DisplayMode& mode,
On 2016/10/11 21:00:31, Daniel Erat wrote:
> if these are coming from MatcherInterface, you should use 'override' rather
than
> 'virtual'

Done.

https://codereview.chromium.org/2395873002/diff/100001/ui/display/fake_displa...
ui/display/fake_display_snapshot_unittests.cc:32: return mode.size() == size_ &&
mode.refresh_rate() == refresh_rate_;
On 2016/10/11 21:00:31, Daniel Erat wrote:
> nit: maybe compare the rate difference against some epsilon, since you said
you
> were worried about floating-point errors before?

Done.

[kylechar, 2016-10-12 13:41:08]

Patchset #7 (id:120001) has been deleted

[kylechar, 2016-10-12 13:47:51]

kylechar@chromium.org changed reviewers:
+ tfarina@chromium.org
- thakis@chromium.org

[kylechar, 2016-10-12 13:47:52]

+tfarina for DEPS on third_party/re2 instead.

[Daniel Erat, 2016-10-12 14:50:50]

lgtm

[tfarina, 2016-10-13 13:15:42]

RE2 dependency LGTM.

https://codereview.chromium.org/2395873002/diff/140001/ui/display/fake_displa...
File ui/display/fake_display_snapshot_unittests.cc (right):

https://codereview.chromium.org/2395873002/diff/140001/ui/display/fake_displa...
ui/display/fake_display_snapshot_unittests.cc:10: #include
"ui/display/fake_display_snapshot.h"
We usually put this as the first include like we do with the corresponding
source file, not sure if that is still recommended or not though.

[kylechar, 2016-10-13 13:32:29]

Thanks!

https://codereview.chromium.org/2395873002/diff/140001/ui/display/fake_displa...
File ui/display/fake_display_snapshot_unittests.cc (right):

https://codereview.chromium.org/2395873002/diff/140001/ui/display/fake_displa...
ui/display/fake_display_snapshot_unittests.cc:10: #include
"ui/display/fake_display_snapshot.h"
On 2016/10/13 13:15:42, tfarina wrote:
> We usually put this as the first include like we do with the corresponding
> source file, not sure if that is still recommended or not though.

It doesn't seem to be clearly specified if that rule applies to tests or not. It
doesn't seem like a bad convention (and lots of tests do things that way
already) so sure. Done.

[kylechar, 2016-10-13 13:33:54]

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

[commit-bot: I haz the power, 2016-10-13 13:34:06]

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, 2016-10-13 13:55:55]

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

[commit-bot: I haz the power, 2016-10-13 13:55:56]

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

[kylechar, 2016-10-13 14:07:43]

The CQ bit was checked by kylechar@chromium.org

[kylechar, 2016-10-13 14:07:43]

The patchset sent to the CQ was uploaded after l-g-t-m from
tfarina@chromium.org, derat@chromium.org
Link to the patchset: https://codereview.chromium.org/2395873002/#ps180001
(title: "Fix that one test I changed to check error messages.")

[commit-bot: I haz the power, 2016-10-13 14:07:51]

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, 2016-10-13 14:45:49]

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

[commit-bot: I haz the power, 2016-10-13 14:45:51]

Try jobs failed on following builders:
  win_clang on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_clang/builds/...)

[kylechar, 2016-10-13 16:00:17]

The CQ bit was checked by kylechar@chromium.org

[kylechar, 2016-10-13 16:00:18]

The patchset sent to the CQ was uploaded after l-g-t-m from
tfarina@chromium.org, derat@chromium.org
Link to the patchset: https://codereview.chromium.org/2395873002/#ps200001
(title: "Fix windows compile")

[commit-bot: I haz the power, 2016-10-13 16:00:42]

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, 2016-10-13 18:03:58]

Committed patchset #10 (id:200001)

[commit-bot: I haz the power, 2016-10-13 18:06:54]

Description was changed from

==========
Add more options to --screen-config flag.

Allow the user to specify more options for each DisplaySnapshot created
by FakeDisplayDelegate. Alternate resolutions, different refresh rates
and various option toggles are added.

Add tests for FakeDisplaySnapshot that verify the display specification
string parsing.

BUG=611475
==========

to

==========
Add more options to --screen-config flag.

Allow the user to specify more options for each DisplaySnapshot created
by FakeDisplayDelegate. Alternate resolutions, different refresh rates
and various option toggles are added.

Add tests for FakeDisplaySnapshot that verify the display specification
string parsing.

BUG=611475

Committed: https://crrev.com/3cabb1432a49769f618d868b218ad119709560d7
Cr-Commit-Position: refs/heads/master@{#425084}
==========

[commit-bot: I haz the power, 2016-10-13 18:06:55]

Patchset 10 (id:??) landed as
https://crrev.com/3cabb1432a49769f618d868b218ad119709560d7
Cr-Commit-Position: refs/heads/master@{#425084}