Give user ability to file a feedback report from the display error notification.

Give user ability to file a feedback report from the display error notification.

Make the display error notification clickable and starts the feedback app on click.
We log the available displays IDs and EDIDs.

BUG=570380
TEST=manually

Committed: https://crrev.com/965f9b3c3a245b52b8720cecbd598576853ba719
Cr-Commit-Position: refs/heads/master@{#373895}

[afakhry, 2016-01-26 23:15:06]

Description was changed from

==========
Give user ability to file a feedback report from the display error notification.

Make the display error notification clickable and starts the feedback app on
click.
We log the available displays IDs and EDIDs.

BUG=570380
TEST=
==========

to

==========
Give user ability to file a feedback report from the display error notification.

Make the display error notification clickable and starts the feedback app on
click.
We log the available displays IDs and EDIDs.

BUG=570380
TEST=manually
==========

[afakhry, 2016-01-27 03:06:23]

afakhry@chromium.org changed reviewers:
+ erg@chromium.org, kenrb@chromium.org, oshima@chromium.org, spang@chromium.org

[afakhry, 2016-01-27 03:06:24]

Hi. Please review this CL. Thanks!

oshima@chromium.org:
	ash/ash_chromeos_strings.grdp
	ash/display/display_error_observer_chromeos.cc
	ui/display/chromeos/display_snapshot_virtual.cc
	ui/display/chromeos/test/test_display_snapshot.cc
	ui/display/chromeos/x11/display_snapshot_x11.h
	ui/display/chromeos/x11/display_snapshot_x11.cc
	ui/display/chromeos/x11/native_display_delegate_x11.cc
	ui/display/chromeos/x11/native_display_event_dispatcher_x11_unittest.cc
	ui/display/types/display_snapshot.h
	ui/display/types/display_snapshot.cc
	ui/display/util/x11/edid_parser_x11.h
	ui/display/util/x11/edid_parser_x11.cc

spang@chromium.org:	
	ui/ozone/common/display_snapshot_proxy.cc
	ui/ozone/common/display_util.cc
	
erg@chromium.org:
	ui/views/widget/desktop_aura/desktop_screen_x11.cc
	
kenrb@chromium.org:
	ui/ozone/common/gpu/ozone_gpu_message_params.h
	ui/ozone/common/gpu/ozone_gpu_messages.h

[oshima, 2016-01-27 09:18:15]

https://codereview.chromium.org/1636153002/diff/40001/ash/display/display_err...
File ash/display/display_error_observer_chromeos.cc (right):

https://codereview.chromium.org/1636153002/diff/40001/ash/display/display_err...
ash/display/display_error_observer_chromeos.cc:35:
DisplayErrorNotificationDelegate() {}
= default. (this seems to be allowed now)

https://codereview.chromium.org/1636153002/diff/40001/ash/display/display_err...
ash/display/display_error_observer_chromeos.cc:46:
~DisplayErrorNotificationDelegate() override {}
ditto

https://codereview.chromium.org/1636153002/diff/40001/ash/display/display_err...
ash/display/display_error_observer_chromeos.cc:64:
base::StringAppendF(&error_log_message, "%02X", edid_byte);
base::HexEncode?

https://codereview.chromium.org/1636153002/diff/40001/ash/display/display_err...
ash/display/display_error_observer_chromeos.cc:69: return error_log_message;
can't you jus log it to the stream instead of creating a string?

https://codereview.chromium.org/1636153002/diff/40001/ui/display/chromeos/x11...
File ui/display/chromeos/x11/display_snapshot_x11.h (right):

https://codereview.chromium.org/1636153002/diff/40001/ui/display/chromeos/x11...
ui/display/chromeos/x11/display_snapshot_x11.h:21: class DISPLAY_EXPORT
DisplaySnapshotX11 : public DisplaySnapshot {
Aren't these x11 file dead code?

[Elliot Glaysher, 2016-01-27 18:33:01]

Trivial change in u/v/w/d/desktop_screen_x11.cc lgtm.

[afakhry, 2016-01-29 02:16:52]

PTAL. I also cleared out bytes 12-15 containing the serial number in EDID.

https://codereview.chromium.org/1636153002/diff/40001/ash/display/display_err...
File ash/display/display_error_observer_chromeos.cc (right):

https://codereview.chromium.org/1636153002/diff/40001/ash/display/display_err...
ash/display/display_error_observer_chromeos.cc:35:
DisplayErrorNotificationDelegate() {}
On 2016/01/27 09:18:14, oshima wrote:
> = default. (this seems to be allowed now)

Done.

https://codereview.chromium.org/1636153002/diff/40001/ash/display/display_err...
ash/display/display_error_observer_chromeos.cc:46:
~DisplayErrorNotificationDelegate() override {}
On 2016/01/27 09:18:14, oshima wrote:
> ditto

Done.

https://codereview.chromium.org/1636153002/diff/40001/ash/display/display_err...
ash/display/display_error_observer_chromeos.cc:64:
base::StringAppendF(&error_log_message, "%02X", edid_byte);
On 2016/01/27 09:18:14, oshima wrote:
> base::HexEncode?

Done.

https://codereview.chromium.org/1636153002/diff/40001/ash/display/display_err...
ash/display/display_error_observer_chromeos.cc:69: return error_log_message;
On 2016/01/27 09:18:14, oshima wrote:
> can't you jus log it to the stream instead of creating a string?

Done.

https://codereview.chromium.org/1636153002/diff/40001/ui/display/chromeos/x11...
File ui/display/chromeos/x11/display_snapshot_x11.h (right):

https://codereview.chromium.org/1636153002/diff/40001/ui/display/chromeos/x11...
ui/display/chromeos/x11/display_snapshot_x11.h:21: class DISPLAY_EXPORT
DisplaySnapshotX11 : public DisplaySnapshot {
On 2016/01/27 09:18:15, oshima wrote:
> Aren't these x11 file dead code?

It's probably dear. It gets compiled in linux though.

[dnicoara, 2016-01-29 14:29:31]

https://codereview.chromium.org/1636153002/diff/40001/ui/display/chromeos/x11...
File ui/display/chromeos/x11/display_snapshot_x11.h (right):

https://codereview.chromium.org/1636153002/diff/40001/ui/display/chromeos/x11...
ui/display/chromeos/x11/display_snapshot_x11.h:21: class DISPLAY_EXPORT
DisplaySnapshotX11 : public DisplaySnapshot {
On 2016/01/29 02:16:51, afakhry wrote:
> On 2016/01/27 09:18:15, oshima wrote:
> > Aren't these x11 file dead code?
> 
> It's probably dear. It gets compiled in linux though.

Unfortunately not dead yet :( ... nyan is still X11 and waiting on the graphics
driver to be in good shape: crbug.com/478339

[afakhry, 2016-02-02 19:04:44]

On 2016/01/29 14:29:31, dnicoara wrote:
>
https://codereview.chromium.org/1636153002/diff/40001/ui/display/chromeos/x11...
> File ui/display/chromeos/x11/display_snapshot_x11.h (right):
> 
>
https://codereview.chromium.org/1636153002/diff/40001/ui/display/chromeos/x11...
> ui/display/chromeos/x11/display_snapshot_x11.h:21: class DISPLAY_EXPORT
> DisplaySnapshotX11 : public DisplaySnapshot {
> On 2016/01/29 02:16:51, afakhry wrote:
> > On 2016/01/27 09:18:15, oshima wrote:
> > > Aren't these x11 file dead code?
> > 
> > It's probably dear. It gets compiled in linux though.
> 
> Unfortunately not dead yet :( ... nyan is still X11 and waiting on the
graphics
> driver to be in good shape: crbug.com/478339

Reviewers, friendly ping!

[oshima, 2016-02-02 20:27:17]

lgtm

https://codereview.chromium.org/1636153002/diff/60001/ui/display/types/displa...
File ui/display/types/display_snapshot.cc (right):

https://codereview.chromium.org/1636153002/diff/60001/ui/display/types/displa...
ui/display/types/display_snapshot.cc:37: for (int i = 12; i <= 15; ++i)
define const for the position and length, with reference to EDID spec

[dnicoara, 2016-02-02 20:43:50]

https://codereview.chromium.org/1636153002/diff/60001/ui/display/types/displa...
File ui/display/types/display_snapshot.cc (right):

https://codereview.chromium.org/1636153002/diff/60001/ui/display/types/displa...
ui/display/types/display_snapshot.cc:34: if (edid_.size() == 128U) {
You must handle the case when the size isn't 128.

You could do the for-loop like:
for (size_t i = 12; i <= std::min(15, edid_.size() - 1); ++i)
  edid_[i] = 0;

[kenrb, 2016-02-02 20:47:51]

ipc lgtm

[afakhry, 2016-02-05 16:47:53]

PTAL

https://codereview.chromium.org/1636153002/diff/60001/ui/display/types/displa...
File ui/display/types/display_snapshot.cc (right):

https://codereview.chromium.org/1636153002/diff/60001/ui/display/types/displa...
ui/display/types/display_snapshot.cc:34: if (edid_.size() == 128U) {
On 2016/02/02 20:43:50, dnicoara wrote:
> You must handle the case when the size isn't 128.
> 
> You could do the for-loop like:
> for (size_t i = 12; i <= std::min(15, edid_.size() - 1); ++i)
>   edid_[i] = 0;

Done.

https://codereview.chromium.org/1636153002/diff/60001/ui/display/types/displa...
ui/display/types/display_snapshot.cc:37: for (int i = 12; i <= 15; ++i)
On 2016/02/02 20:27:17, oshima wrote:
> define const for the position and length, with reference to EDID spec

Done.

[spang, 2016-02-05 19:33:36]

lgtm

[afakhry, 2016-02-05 19:34:35]

The CQ bit was checked by afakhry@chromium.org

[afakhry, 2016-02-05 19:34:35]

The patchset sent to the CQ was uploaded after l-g-t-m from erg@chromium.org,
kenrb@chromium.org, oshima@chromium.org
Link to the patchset: https://codereview.chromium.org/1636153002/#ps80001
(title: "more comments")

[commit-bot: I haz the power, 2016-02-05 19:37:34]

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

[commit-bot: I haz the power, 2016-02-05 20:24:57]

Description was changed from

==========
Give user ability to file a feedback report from the display error notification.

Make the display error notification clickable and starts the feedback app on
click.
We log the available displays IDs and EDIDs.

BUG=570380
TEST=manually
==========

to

==========
Give user ability to file a feedback report from the display error notification.

Make the display error notification clickable and starts the feedback app on
click.
We log the available displays IDs and EDIDs.

BUG=570380
TEST=manually
==========

[commit-bot: I haz the power, 2016-02-05 20:24:58]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2016-02-05 20:26:16]

Description was changed from

==========
Give user ability to file a feedback report from the display error notification.

Make the display error notification clickable and starts the feedback app on
click.
We log the available displays IDs and EDIDs.

BUG=570380
TEST=manually
==========

to

==========
Give user ability to file a feedback report from the display error notification.

Make the display error notification clickable and starts the feedback app on
click.
We log the available displays IDs and EDIDs.

BUG=570380
TEST=manually

Committed: https://crrev.com/965f9b3c3a245b52b8720cecbd598576853ba719
Cr-Commit-Position: refs/heads/master@{#373895}
==========

[commit-bot: I haz the power, 2016-02-05 20:26:17]

Patchset 5 (id:??) landed as
https://crrev.com/965f9b3c3a245b52b8720cecbd598576853ba719
Cr-Commit-Position: refs/heads/master@{#373895}

[please use gerrit instead, 2016-02-05 20:58:19]

A revert of this CL (patchset #5 id:80001) has been created in
https://codereview.chromium.org/1671223002/ by rouslan@chromium.org.

The reason for reverting is: Missing include causing compile failure in win64:

FAILED: ninja -t msvc -e environment.x64 -- E:\b\build\goma/gomacc.exe
"E:\b\depot_tools\win_toolchain\vs2013_files\4087e065abebdca6dbd0caca2910c6718d2ec67f\VC\bin\amd64/cl.exe"
/nologo /showIncludes /FC @obj/ui/display/types/types/display_snapshot.obj.rsp
/c ../../ui/display/types/display_snapshot.cc
/Foobj/ui/display/types/types/display_snapshot.obj
/Fdobj/ui/display/types/types_cc.pdb
e:\b\build\slave\win_x64_gn\build\src\ui\display\types\display_snapshot.cc(46) :
error C2039: 'min' : is not a member of 'std'
e:\b\build\slave\win_x64_gn\build\src\ui\display\types\display_snapshot.cc(46) :
error C3861: 'min': identifier not found
ninja: build stopped: subcommand failed..

[please use gerrit instead, 2016-02-05 20:59:46]

Please add #include <algorithm> to display_snapshot.cc and re-land.

[afakhry, 2016-02-05 21:16:16]

Description was changed from

==========
Give user ability to file a feedback report from the display error notification.

Make the display error notification clickable and starts the feedback app on
click.
We log the available displays IDs and EDIDs.

BUG=570380
TEST=manually

Committed: https://crrev.com/965f9b3c3a245b52b8720cecbd598576853ba719
Cr-Commit-Position: refs/heads/master@{#373895}
==========

to

==========
Give user ability to file a feedback report from the display error notification.

Make the display error notification clickable and starts the feedback app on
click.
We log the available displays IDs and EDIDs.

BUG=570380
TEST=manually

Committed: https://crrev.com/965f9b3c3a245b52b8720cecbd598576853ba719
Cr-Commit-Position: refs/heads/master@{#373895}
==========