SystemDisplayApi: Introdice OverscanTracker

SystemDisplayApi: Introdice OverscanTracker

The system.display.overscanCalibrationStart method displays an overlay
for adjusting overscan when called.

If the web context initiating the call is deleted before
overscanCalibrationComplete is called, the overaly will remain with
no way to remove it (without running Settings or an app that shows
and removes the overaly correctly).

This change creates a tracker so that if the render frame is deleted
before overscanCalibrationComplete is called, the calibration will
be reset and completed to remove the overlay.

BUG=666590
Committed: https://crrev.com/8f2b71add2ce71f468fb8274c20d7ea41d36886b
Cr-Commit-Position: refs/heads/master@{#437448}

[stevenjb, 2016-12-07 19:27:58]

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

[stevenjb, 2016-12-07 19:28:14]

stevenjb@chromium.org changed reviewers:
+ asargent@chromium.org, xiyuan@chromium.org

[commit-bot: I haz the power, 2016-12-07 19:28:29]

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-12-07 20:01:15]

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

[commit-bot: I haz the power, 2016-12-07 20:01:15]

Dry run: 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/...)

[xiyuan, 2016-12-07 21:32:40]

https://codereview.chromium.org/2558453004/diff/1/extensions/browser/api/syst...
File extensions/browser/api/system_display/system_display_api.cc (right):

https://codereview.chromium.org/2558453004/diff/1/extensions/browser/api/syst...
extensions/browser/api/system_display/system_display_api.cc:121: if (!tracker)
Should we use |s_overscan_tracker| directly since we might not want to create
the tracker in this method?

https://codereview.chromium.org/2558453004/diff/1/extensions/browser/api/syst...
File extensions/browser/api/system_display/system_display_apitest.cc (right):

https://codereview.chromium.org/2558453004/diff/1/extensions/browser/api/syst...
extensions/browser/api/system_display/system_display_apitest.cc:136: bool
calibration_started(const std::string& id) {
nit: const method?

https://codereview.chromium.org/2558453004/diff/1/extensions/browser/api/syst...
extensions/browser/api/system_display/system_display_apitest.cc:140: bool
calibration_changed(const std::string& id) {
nit: const method?

https://codereview.chromium.org/2558453004/diff/1/extensions/browser/api/syst...
extensions/browser/api/system_display/system_display_apitest.cc:203: const char
test_manifest[] =
nit: const -> constexpr, test_manifest -> kTestManifest

https://codereview.chromium.org/2558453004/diff/1/extensions/browser/api/syst...
extensions/browser/api/system_display/system_display_apitest.cc:214: const char
test_manifest_kiosk[] =
nit: const -> constexpr, test_manifest_kiosk -> kTestManifestKiosk

[stevenjb, 2016-12-07 21:54:42]

https://codereview.chromium.org/2558453004/diff/1/extensions/browser/api/syst...
File extensions/browser/api/system_display/system_display_api.cc (right):

https://codereview.chromium.org/2558453004/diff/1/extensions/browser/api/syst...
extensions/browser/api/system_display/system_display_api.cc:121: if (!tracker)
On 2016/12/07 21:32:40, xiyuan wrote:
> Should we use |s_overscan_tracker| directly since we might not want to create
> the tracker in this method?

Oops, yes, thanks for catching! I don't actually need Get() so removing
entirely. Fixed.

https://codereview.chromium.org/2558453004/diff/1/extensions/browser/api/syst...
File extensions/browser/api/system_display/system_display_apitest.cc (right):

https://codereview.chromium.org/2558453004/diff/1/extensions/browser/api/syst...
extensions/browser/api/system_display/system_display_apitest.cc:136: bool
calibration_started(const std::string& id) {
On 2016/12/07 21:32:40, xiyuan wrote:
> nit: const method?

Done.

https://codereview.chromium.org/2558453004/diff/1/extensions/browser/api/syst...
extensions/browser/api/system_display/system_display_apitest.cc:140: bool
calibration_changed(const std::string& id) {
On 2016/12/07 21:32:40, xiyuan wrote:
> nit: const method?

Done.

https://codereview.chromium.org/2558453004/diff/1/extensions/browser/api/syst...
extensions/browser/api/system_display/system_display_apitest.cc:203: const char
test_manifest[] =
On 2016/12/07 21:32:40, xiyuan wrote:
> nit: const -> constexpr, test_manifest -> kTestManifest

Done.

https://codereview.chromium.org/2558453004/diff/1/extensions/browser/api/syst...
extensions/browser/api/system_display/system_display_apitest.cc:214: const char
test_manifest_kiosk[] =
On 2016/12/07 21:32:40, xiyuan wrote:
> nit: const -> constexpr, test_manifest_kiosk -> kTestManifestKiosk

Done.

[xiyuan, 2016-12-07 21:59:16]

lgtm

[stevenjb, 2016-12-07 23:01:48]

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

[commit-bot: I haz the power, 2016-12-07 23:02:33]

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-12-08 00:34:11]

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

[commit-bot: I haz the power, 2016-12-08 00:34:12]

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

[asargent_no_longer_on_chrome, 2016-12-08 22:03:25]

lgtm

https://codereview.chromium.org/2558453004/diff/40001/extensions/browser/api/...
File extensions/browser/api/system_display/system_display_api.cc (right):

https://codereview.chromium.org/2558453004/diff/40001/extensions/browser/api/...
extensions/browser/api/system_display/system_display_api.cc:101: static
OverscanTracker* s_overscan_tracker = nullptr;
FYI, in many other areas of extensions code, we've used a "g_" prefix (denoting
"global") instead of "s_" - you might consider doing likewise for consistency.

[stevenjb, 2016-12-08 22:05:52]

https://codereview.chromium.org/2558453004/diff/40001/extensions/browser/api/...
File extensions/browser/api/system_display/system_display_api.cc (right):

https://codereview.chromium.org/2558453004/diff/40001/extensions/browser/api/...
extensions/browser/api/system_display/system_display_api.cc:101: static
OverscanTracker* s_overscan_tracker = nullptr;
On 2016/12/08 22:03:25, Antony Sargent wrote:
> FYI, in many other areas of extensions code, we've used a "g_" prefix
(denoting
> "global") instead of "s_" - you might consider doing likewise for consistency.

> 

I tend to use g_ for statics that might be referenced globally, and s_ for file
local statics, but I'm happy to change this.

Cheers.

[stevenjb, 2016-12-09 01:17:02]

The CQ bit was checked by stevenjb@chromium.org

[stevenjb, 2016-12-09 01:17:02]

The patchset sent to the CQ was uploaded after l-g-t-m from xiyuan@chromium.org,
asargent@chromium.org
Link to the patchset: https://codereview.chromium.org/2558453004/#ps60001
(title: "Rebase + s_overscan_tracker -> g_overscan_tracker")

[commit-bot: I haz the power, 2016-12-09 01:17:45]

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-12-09 03:21:15]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1481246222051250,
"parent_rev": "c8da1a34adc33585c3de4504dfc542e1258d9aef", "commit_rev":
"1d933efa51f039b92d8d64e4779350aee6c6f314"}

[commit-bot: I haz the power, 2016-12-09 03:21:51]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-12-09 03:24:42]

Description was changed from

==========
SystemDisplayApi: Introdice OverscanTracker

The system.display.overscanCalibrationStart method displays an overlay
for adjusting overscan when called.

If the web context initiating the call is deleted before
overscanCalibrationComplete is called, the overaly will remain with
no way to remove it (without running Settings or an app that shows
and removes the overaly correctly).

This change creates a tracker so that if the render frame is deleted
before overscanCalibrationComplete is called, the calibration will
be reset and completed to remove the overlay.

BUG=666590
==========

to

==========
SystemDisplayApi: Introdice OverscanTracker

The system.display.overscanCalibrationStart method displays an overlay
for adjusting overscan when called.

If the web context initiating the call is deleted before
overscanCalibrationComplete is called, the overaly will remain with
no way to remove it (without running Settings or an app that shows
and removes the overaly correctly).

This change creates a tracker so that if the render frame is deleted
before overscanCalibrationComplete is called, the calibration will
be reset and completed to remove the overlay.

BUG=666590

Committed: https://crrev.com/8f2b71add2ce71f468fb8274c20d7ea41d36886b
Cr-Commit-Position: refs/heads/master@{#437448}
==========

[commit-bot: I haz the power, 2016-12-09 03:24:43]

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