Several minor changes to ScreenCapturerWinMagnifier

Several minor changes to ScreenCapturerWinMagnifier
1. Remove legacy screen-saver-blocking logic
2. tls_index_ is not a good choice, we can use thread-static
3. ScreenCapturerHelper is not designed for this scenario
4. Disable this capturer on 2+ monitors system

BUG=638802
Committed: https://crrev.com/05bba2be48f241e865675fdffb17fff8f9efb62d
Cr-Commit-Position: refs/heads/master@{#14342}

[Hzj_jie, 2016-09-08 01:07:54]

Patchset #1 (id:1) has been deleted

[Hzj_jie, 2016-09-08 01:25:25]

Patchset #1 (id:20001) has been deleted

[Hzj_jie, 2016-09-08 01:33:02]

Patchset #1 (id:40001) has been deleted

[Hzj_jie, 2016-09-08 02:18:03]

Patchset #1 (id:60001) has been deleted

[Hzj_jie, 2016-09-08 02:26:15]

Description was changed from

==========
[WebRTC] Several minor changes to ScreenCapturerWinMagnifier
1. Remove legacy screen-saver-blocking logic
2. tls_index_ is not a good choice, we can use lambda expression
3. ScreenCapturerHelper is not designed for this scenario
4. Disable this capturer on 2+ monitors system

BUG=
==========

to

==========
[WebRTC] Several minor changes to ScreenCapturerWinMagnifier
1. Remove legacy screen-saver-blocking logic
2. tls_index_ is not a good choice, we can use thread-static
3. ScreenCapturerHelper is not designed for this scenario
4. Disable this capturer on 2+ monitors system

BUG=
==========

[Hzj_jie, 2016-09-08 02:30:35]

Patchset #1 (id:80001) has been deleted

[Hzj_jie, 2016-09-08 18:45:12]

Patchset #2 (id:120001) has been deleted

[Hzj_jie, 2016-09-08 18:50:25]

Patchset #1 (id:100001) has been deleted

[Hzj_jie, 2016-09-14 17:49:23]

Patchset #1 (id:100002) has been deleted

[Hzj_jie, 2016-09-14 18:22:09]

Patchset #1 (id:150001) has been deleted

[Hzj_jie, 2016-09-14 19:06:07]

Description was changed from

==========
[WebRTC] Several minor changes to ScreenCapturerWinMagnifier
1. Remove legacy screen-saver-blocking logic
2. tls_index_ is not a good choice, we can use thread-static
3. ScreenCapturerHelper is not designed for this scenario
4. Disable this capturer on 2+ monitors system

BUG=
==========

to

==========
Several minor changes to ScreenCapturerWinMagnifier
1. Remove legacy screen-saver-blocking logic
2. tls_index_ is not a good choice, we can use thread-static
3. ScreenCapturerHelper is not designed for this scenario
4. Disable this capturer on 2+ monitors system

BUG=
==========

[Hzj_jie, 2016-09-14 22:18:04]

zijiehe@chromium.org changed reviewers:
+ sergeyu@chromium.org

[qiangchen, 2016-09-14 23:06:24]

On 2016/09/14 22:18:05, Hzj_jie wrote:

I am not quite familiar with tls_index_. Can you let jiayl@ take a look? In case
they are used to handle some special use cases.

[Hzj_jie, 2016-09-15 18:33:34]

On 2016/09/14 23:06:24, qiangchenC wrote:
> On 2016/09/14 22:18:05, Hzj_jie wrote:
> 
> I am not quite familiar with tls_index_. Can you let jiayl@ take a look? In
case
> they are used to handle some special use cases.

I believe SergeyU@ will take care of this change.

[Hzj_jie, 2016-09-15 18:34:45]

Patchset #1 (id:170001) has been deleted

[Sergey Ulanov, 2016-09-19 20:53:18]

https://codereview.chromium.org/2319383002/diff/190001/webrtc/modules/desktop...
File webrtc/modules/desktop_capture/screen_capturer_unittest.cc (right):

https://codereview.chromium.org/2319383002/diff/190001/webrtc/modules/desktop...
webrtc/modules/desktop_capture/screen_capturer_unittest.cc:383: // Disabled due
to being flaky due to the fact that it useds rendering / UI,
typo: useds

https://codereview.chromium.org/2319383002/diff/190001/webrtc/modules/desktop...
File webrtc/modules/desktop_capture/win/screen_capturer_win_magnifier.cc
(right):

https://codereview.chromium.org/2319383002/diff/190001/webrtc/modules/desktop...
webrtc/modules/desktop_capture/win/screen_capturer_win_magnifier.cc:36:
thread_local static ScreenCapturerWinMagnifier* kCurrent;
- thread_local is not listed on chromium-cpp.appspot.com, so I don't think it's
approved. 
- This is clearly not a const, so it shouldn't use const name.
- You probably need "= nullptr;" to make sure it gets initialized properly.

https://codereview.chromium.org/2319383002/diff/190001/webrtc/modules/desktop...
webrtc/modules/desktop_capture/win/screen_capturer_win_magnifier.cc:128:
differ_->CalcDirtyRegion(
Do we need to keep the differ here?

https://codereview.chromium.org/2319383002/diff/190001/webrtc/modules/desktop...
webrtc/modules/desktop_capture/win/screen_capturer_win_magnifier.cc:212:
RTC_DCHECK(kCurrent != nullptr);
Please replace all asserts with RTC_DCHECK

https://codereview.chromium.org/2319383002/diff/190001/webrtc/modules/desktop...
webrtc/modules/desktop_capture/win/screen_capturer_win_magnifier.cc:219: if
(GetSystemMetrics(SM_CMONITORS) != 1) {
I think it's better to keep this check in ScreenCapturerWinMagnifier::Capture().
Number of monitors can change dynamically while the capturer is active.

[Hzj_jie, 2016-09-19 22:05:23]

https://codereview.chromium.org/2319383002/diff/190001/webrtc/modules/desktop...
File webrtc/modules/desktop_capture/screen_capturer_unittest.cc (right):

https://codereview.chromium.org/2319383002/diff/190001/webrtc/modules/desktop...
webrtc/modules/desktop_capture/screen_capturer_unittest.cc:383: // Disabled due
to being flaky due to the fact that it useds rendering / UI,
On 2016/09/19 20:53:18, Sergey Ulanov wrote:
> typo: useds

Ah, yes, all updated.

https://codereview.chromium.org/2319383002/diff/190001/webrtc/modules/desktop...
File webrtc/modules/desktop_capture/win/screen_capturer_win_magnifier.cc
(right):

https://codereview.chromium.org/2319383002/diff/190001/webrtc/modules/desktop...
webrtc/modules/desktop_capture/win/screen_capturer_win_magnifier.cc:36:
thread_local static ScreenCapturerWinMagnifier* kCurrent;
On 2016/09/19 20:53:18, Sergey Ulanov wrote:
> - thread_local is not listed on http://chromium-cpp.appspot.com, so I don't
think it's
> approved. 
> - This is clearly not a const, so it shouldn't use const name.
> - You probably need "= nullptr;" to make sure it gets initialized properly.

Emmm, a Google internal mail thread shows thread_local will eventually be the
norm. But yes, since it's not actively allowed. I will use __declspec(thread)
instead, which is MSVC specific implementation, and widely used in Chromium.

Others are updated.

https://codereview.chromium.org/2319383002/diff/190001/webrtc/modules/desktop...
webrtc/modules/desktop_capture/win/screen_capturer_win_magnifier.cc:128:
differ_->CalcDirtyRegion(
On 2016/09/19 20:53:18, Sergey Ulanov wrote:
> Do we need to keep the differ here?

No, but all the differs will be removed in
https://codereview.chromium.org/2348803003/.

https://codereview.chromium.org/2319383002/diff/190001/webrtc/modules/desktop...
webrtc/modules/desktop_capture/win/screen_capturer_win_magnifier.cc:212:
RTC_DCHECK(kCurrent != nullptr);
On 2016/09/19 20:53:18, Sergey Ulanov wrote:
> Please replace all asserts with RTC_DCHECK

Done.

https://codereview.chromium.org/2319383002/diff/190001/webrtc/modules/desktop...
webrtc/modules/desktop_capture/win/screen_capturer_win_magnifier.cc:219: if
(GetSystemMetrics(SM_CMONITORS) != 1) {
On 2016/09/19 20:53:18, Sergey Ulanov wrote:
> I think it's better to keep this check in
ScreenCapturerWinMagnifier::Capture().
> Number of monitors can change dynamically while the capturer is active.

Good point. Updated.

[Sergey Ulanov, 2016-09-19 22:21:25]

https://codereview.chromium.org/2319383002/diff/190001/webrtc/modules/desktop...
File webrtc/modules/desktop_capture/win/screen_capturer_win_magnifier.cc
(right):

https://codereview.chromium.org/2319383002/diff/190001/webrtc/modules/desktop...
webrtc/modules/desktop_capture/win/screen_capturer_win_magnifier.cc:36:
thread_local static ScreenCapturerWinMagnifier* kCurrent;
On 2016/09/19 22:05:23, Hzj_jie wrote:
> On 2016/09/19 20:53:18, Sergey Ulanov wrote:
> > - thread_local is not listed on http://chromium-cpp.appspot.com, so I don't
> think it's
> > approved. 
> > - This is clearly not a const, so it shouldn't use const name.
> > - You probably need "= nullptr;" to make sure it gets initialized properly.
> 
> Emmm, a Google internal mail thread shows thread_local will eventually be the
> norm. But yes, since it's not actively allowed. I will use __declspec(thread)
> instead, which is MSVC specific implementation, and widely used in Chromium.

__declspec(thread) seems used in some third-party projects, but not in chromium
itself. It's also not used in WebRTC, so I'd prefer avoiding it as well. What's
the reason we cannot keep tls_index?

> 
> Others are updated.

[Hzj_jie, 2016-09-19 22:29:42]

On 2016/09/19 22:21:25, Sergey Ulanov wrote:
>
https://codereview.chromium.org/2319383002/diff/190001/webrtc/modules/desktop...
> File webrtc/modules/desktop_capture/win/screen_capturer_win_magnifier.cc
> (right):
> 
>
https://codereview.chromium.org/2319383002/diff/190001/webrtc/modules/desktop...
> webrtc/modules/desktop_capture/win/screen_capturer_win_magnifier.cc:36:
> thread_local static ScreenCapturerWinMagnifier* kCurrent;
> On 2016/09/19 22:05:23, Hzj_jie wrote:
> > On 2016/09/19 20:53:18, Sergey Ulanov wrote:
> > > - thread_local is not listed on http://chromium-cpp.appspot.com, so I
don't
> > think it's
> > > approved. 
> > > - This is clearly not a const, so it shouldn't use const name.
> > > - You probably need "= nullptr;" to make sure it gets initialized
properly.
> > 
> > Emmm, a Google internal mail thread shows thread_local will eventually be
the
> > norm. But yes, since it's not actively allowed. I will use
__declspec(thread)
> > instead, which is MSVC specific implementation, and widely used in Chromium.
> 
> __declspec(thread) seems used in some third-party projects, but not in
chromium
> itself. It's also not used in WebRTC, so I'd prefer avoiding it as well.
What's
> the reason we cannot keep tls_index?
tls_index needs to be actively initialized, which is more complex than compiler
annotation or declarator.
And __declspec(thread) is used by PPAPI
(https://cs.chromium.org/chromium/src/ppapi/lib/gl/gles2/gl2ext_ppapi.c?q=decl...).
But yes, I have not found it in WebRTC.
> 
> > 
> > Others are updated.

[Sergey Ulanov, 2016-09-20 23:33:22]

I'd still prefer to avoid __declspec(thread) as I'm not sure it's safe. E.g.
does it add new static initialization code?
Also I don't see a strong reason to change what we have now. __declspec(thread)
doesn't really make the code easier to understand.
ppapi/lib/gl/gles2/gl2ext_ppapi.c does use __declspec(thread), but as far as I
can tell it's used only for tests and not compiled in chrome.

[Hzj_jie, 2016-09-21 00:01:46]

Patchset #3 (id:230001) has been deleted

[Hzj_jie, 2016-09-21 00:05:37]

Patchset #3 (id:250001) has been deleted

[Hzj_jie, 2016-09-21 00:07:59]

Patchset #3 (id:270001) has been deleted

[Sergey Ulanov, 2016-09-21 20:34:29]

lgtm

[Hzj_jie, 2016-09-21 21:18:46]

I have just realized these Mag* functions are available on Windows Vista or
upper, since we won't support Windows XP anymore, we can use regular include and
function calls to replace the dynamical loaded library. I have added a TODO in
the cc file.

[Hzj_jie, 2016-09-21 21:37:48]

The CQ bit was checked by zijiehe@chromium.org

[Hzj_jie, 2016-09-21 21:37:49]

The patchset sent to the CQ was uploaded after l-g-t-m from sergeyu@chromium.org
Link to the patchset: https://codereview.chromium.org/2319383002/#ps310001
(title: "Add TODO to use include & regular function calls to replace dynamical
loaded library")

[commit-bot: I haz the power, 2016-09-21 21:37:54]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...

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

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

[commit-bot: I haz the power, 2016-09-21 21:48:04]

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

[Hzj_jie, 2016-09-21 21:49:17]

Description was changed from

==========
Several minor changes to ScreenCapturerWinMagnifier
1. Remove legacy screen-saver-blocking logic
2. tls_index_ is not a good choice, we can use thread-static
3. ScreenCapturerHelper is not designed for this scenario
4. Disable this capturer on 2+ monitors system

BUG=
==========

to

==========
Several minor changes to ScreenCapturerWinMagnifier
1. Remove legacy screen-saver-blocking logic
2. tls_index_ is not a good choice, we can use thread-static
3. ScreenCapturerHelper is not designed for this scenario
4. Disable this capturer on 2+ monitors system

BUG=638802
==========

[Hzj_jie, 2016-09-21 21:49:25]

The CQ bit was checked by zijiehe@chromium.org

[commit-bot: I haz the power, 2016-09-21 21:49:35]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...

[commit-bot: I haz the power, 2016-09-21 23:17:51]

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

[commit-bot: I haz the power, 2016-09-21 23:17:52]

Try jobs failed on following builders:
  android_arm64_rel on master.tryserver.webrtc (JOB_TIMED_OUT, no build URL)

[Hzj_jie, 2016-09-22 00:10:18]

The CQ bit was checked by zijiehe@chromium.org

[commit-bot: I haz the power, 2016-09-22 00:10:25]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...

[commit-bot: I haz the power, 2016-09-22 00:25:43]

Description was changed from

==========
Several minor changes to ScreenCapturerWinMagnifier
1. Remove legacy screen-saver-blocking logic
2. tls_index_ is not a good choice, we can use thread-static
3. ScreenCapturerHelper is not designed for this scenario
4. Disable this capturer on 2+ monitors system

BUG=638802
==========

to

==========
Several minor changes to ScreenCapturerWinMagnifier
1. Remove legacy screen-saver-blocking logic
2. tls_index_ is not a good choice, we can use thread-static
3. ScreenCapturerHelper is not designed for this scenario
4. Disable this capturer on 2+ monitors system

BUG=638802
==========

[commit-bot: I haz the power, 2016-09-22 00:25:44]

Committed patchset #4 (id:310001)

[commit-bot: I haz the power, 2016-09-22 00:25:54]

Description was changed from

==========
Several minor changes to ScreenCapturerWinMagnifier
1. Remove legacy screen-saver-blocking logic
2. tls_index_ is not a good choice, we can use thread-static
3. ScreenCapturerHelper is not designed for this scenario
4. Disable this capturer on 2+ monitors system

BUG=638802
==========

to

==========
Several minor changes to ScreenCapturerWinMagnifier
1. Remove legacy screen-saver-blocking logic
2. tls_index_ is not a good choice, we can use thread-static
3. ScreenCapturerHelper is not designed for this scenario
4. Disable this capturer on 2+ monitors system

BUG=638802

Committed: https://crrev.com/05bba2be48f241e865675fdffb17fff8f9efb62d
Cr-Commit-Position: refs/heads/master@{#14342}
==========

[commit-bot: I haz the power, 2016-09-22 00:25:55]

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