WebRtc Win Desktop capture: ignore Win8+ Modern Apps' windows.

WebRtc Win Desktop capture: ignore Win8+ Modern Apps' windows.

Microsoft introduced modern app from win8. Modern apps can be used cross Microsoft's platforms.

It was confirmed from Microsoft that there is no support for modern app's window capture.

BUG=526883
Committed: https://crrev.com/371dc7e5601f05015d4816ed2624cc8f5f01d19a
Cr-Commit-Position: refs/heads/master@{#10154}

[GeorgeZ, 2015-09-30 21:48:13]

gyzhou@chromium.org changed reviewers:
+ mcasas@chromium.org

[GeorgeZ, 2015-09-30 21:48:14]

Miguel

Could you help me do a code review.

Thanks,

George

[mcasas, 2015-10-01 16:54:46]

On 2015/09/30 21:48:14, GeorgeZ wrote:
> Miguel
> 
> Could you help me do a code review.
> 
> Thanks,
> 
> George

gyzhou@, this CL is in the wrong code review site. It should go 
in https://webrtc-codereview.appspot.com, and then you can
send it for review to e.g. mcasas@webrtc.org. Make sure to
patch against a WebRtc tree, i.e. [1,2] (Note on [2], if you 
upload a patch as @chromium.org, you're an external 
contributor with all agreements in place). Thanks!

[1] http://www.webrtc.org/native-code/development#TOC-Getting-the-code
[2] http://www.webrtc.org/contributing

[mcasas, 2015-10-01 16:59:27]

On 2015/10/01 16:54:46, mcasas wrote:
> On 2015/09/30 21:48:14, GeorgeZ wrote:
> > Miguel
> > 
> > Could you help me do a code review.
> > 
> > Thanks,
> > 
> > George
> 
> gyzhou@, this CL is in the wrong code review site. It should go 
> in https://webrtc-codereview.appspot.com, and then you can
> send it for review to e.g. mailto:mcasas@webrtc.org. Make sure to
> patch against a WebRtc tree, i.e. [1,2] (Note on [2], if you 
> upload a patch as @chromium.org, you're an external 
> contributor with all agreements in place). Thanks!
> 
> [1] http://www.webrtc.org/native-code/development#TOC-Getting-the-code
> [2] http://www.webrtc.org/contributing

Heh sorry, niklase@ corrected me: I was opening the review
with my @chromium.org account, when I should use 
@webrtc.org or at least codereview.webrtc.org.

[mcasas, 2015-10-01 17:19:04]

https://codereview.webrtc.org/1371383003/diff/1/webrtc/modules/desktop_captur...
File webrtc/modules/desktop_capture/window_capturer_win.cc (right):

https://codereview.webrtc.org/1371383003/diff/1/webrtc/modules/desktop_captur...
webrtc/modules/desktop_capture/window_capturer_win.cc:70: // Webrtc is not able
to capture modern apps' windows on 09/30/2015.
This is a bit too verbose :) Please rephrase more succintly
and with a view on the purpose, i.e. "Windows X cannot
capture windows of type Y; those are identified by Z and W".

What's "modern" referring to? If there is a bug associated
to this peculiar situation, mention it like http://crbug.com/bla.

https://codereview.webrtc.org/1371383003/diff/1/webrtc/modules/desktop_captur...
webrtc/modules/desktop_capture/window_capturer_win.cc:72: char clsName[1024];
This needs to be WCHAR right? Like in l.53.

https://codereview.webrtc.org/1371383003/diff/1/webrtc/modules/desktop_captur...
webrtc/modules/desktop_capture/window_capturer_win.cc:73: GetClassNameA(hwnd,
clsName, 1024);
What about the return value? and what is 1024? 
Sugggestion: 

const size_t kMaxNameLength = 1024u;
std::wstring clsName(kMaxNameLength);
const int class_name_length = GetClassNAmeA(hwnd, clsName.data(),
kMaxNameLength);
RTC_DCHECK(class_name_length) << "Error retrieving the application's class
name";
if (clsName == "ApplicationFrameWindow" ||
    clsName == "Windows.UI.CoreWindow)) {
  return TRUE;
}

(Mind the  {}, needed if either condition or body are longer than
a line).

(Mind TRUE versus |true|).

https://codereview.webrtc.org/1371383003/diff/1/webrtc/modules/desktop_captur...
webrtc/modules/desktop_capture/window_capturer_win.cc:75: strcmp(clsName,
"Windows.UI.Core.CoreWindow") == 0)
Nit: I wouldn't use camlcase names ever, but if that's 
the style of the file/folder, then so be it. Rename my
variables accordingly above if this is the case.

[gyzhou, 2015-10-01 19:32:44]

On 2015/10/01 17:19:04, mcasas wrote:
>
https://codereview.webrtc.org/1371383003/diff/1/webrtc/modules/desktop_captur...
> File webrtc/modules/desktop_capture/window_capturer_win.cc (right):
> 
>
https://codereview.webrtc.org/1371383003/diff/1/webrtc/modules/desktop_captur...
> webrtc/modules/desktop_capture/window_capturer_win.cc:70: // Webrtc is not
able
> to capture modern apps' windows on 09/30/2015.
> This is a bit too verbose :) Please rephrase more succintly
> and with a view on the purpose, i.e. "Windows X cannot
> capture windows of type Y; those are identified by Z and W".
> 
> What's "modern" referring to? If there is a bug associated
> to this peculiar situation, mention it like http://crbug.com/bla.
> 
>
https://codereview.webrtc.org/1371383003/diff/1/webrtc/modules/desktop_captur...
> webrtc/modules/desktop_capture/window_capturer_win.cc:72: char clsName[1024];
> This needs to be WCHAR right? Like in l.53.
> 
>
https://codereview.webrtc.org/1371383003/diff/1/webrtc/modules/desktop_captur...
> webrtc/modules/desktop_capture/window_capturer_win.cc:73: GetClassNameA(hwnd,
> clsName, 1024);
> What about the return value? and what is 1024? 
> Sugggestion: 
> 
> const size_t kMaxNameLength = 1024u;
> std::wstring clsName(kMaxNameLength);
> const int class_name_length = GetClassNAmeA(hwnd, clsName.data(),
> kMaxNameLength);
> RTC_DCHECK(class_name_length) << "Error retrieving the application's class
> name";
> if (clsName == "ApplicationFrameWindow" ||
>     clsName == "Windows.UI.CoreWindow)) {
>   return TRUE;
> }
> 
> (Mind the  {}, needed if either condition or body are longer than
> a line).
> 
> (Mind TRUE versus |true|).
> 
>
https://codereview.webrtc.org/1371383003/diff/1/webrtc/modules/desktop_captur...
> webrtc/modules/desktop_capture/window_capturer_win.cc:75: strcmp(clsName,
> "Windows.UI.Core.CoreWindow") == 0)
> Nit: I wouldn't use camlcase names ever, but if that's 
> the style of the file/folder, then so be it. Rename my
> variables accordingly above if this is the case.

I updated the code based on your review comments.

Thanks,

George

[mcasas, 2015-10-01 21:59:17]

It's customary to click on reviewers' comments
and click on "Done" or "Reply" with comments.

https://chromiumcodereview.appspot.com/1371383003/diff/20001/webrtc/modules/d...
File webrtc/modules/desktop_capture/window_capturer_win.cc (right):

https://chromiumcodereview.appspot.com/1371383003/diff/20001/webrtc/modules/d...
webrtc/modules/desktop_capture/window_capturer_win.cc:71: // and/or
windows.UI.Core.coreWindow.
Suggestion of rephrase:
  // Windows 8 introduced a "Modern App" identified by their class name being
  // either ApplicationFrameWindow or windows.UI.Core.coreWindow. The associated
  // windows cannot be captured, so we skip them, http://crbug.com/526883.

https://chromiumcodereview.appspot.com/1371383003/diff/20001/webrtc/modules/d...
webrtc/modules/desktop_capture/window_capturer_win.cc:73: char
class_name[kMaxNameLength];
Why not reuse |kClassLength|? (l.43)

https://chromiumcodereview.appspot.com/1371383003/diff/20001/webrtc/modules/d...
webrtc/modules/desktop_capture/window_capturer_win.cc:74: int class_name_length
= GetClassNameA(hwnd, class_name, kMaxNameLength);
const

https://chromiumcodereview.appspot.com/1371383003/diff/20001/webrtc/modules/d...
webrtc/modules/desktop_capture/window_capturer_win.cc:74: int class_name_length
= GetClassNameA(hwnd, class_name, kMaxNameLength);
|class_name_length| or |classNameLength| ?

[gyzhou, 2015-10-01 23:09:23]

On 2015/10/01 21:59:17, mcasas wrote:
> It's customary to click on reviewers' comments
> and click on "Done" or "Reply" with comments.
> 
>
https://chromiumcodereview.appspot.com/1371383003/diff/20001/webrtc/modules/d...
> File webrtc/modules/desktop_capture/window_capturer_win.cc (right):
> 
>
https://chromiumcodereview.appspot.com/1371383003/diff/20001/webrtc/modules/d...
> webrtc/modules/desktop_capture/window_capturer_win.cc:71: // and/or
> windows.UI.Core.coreWindow.
> Suggestion of rephrase:
>   // Windows 8 introduced a "Modern App" identified by their class name being
>   // either ApplicationFrameWindow or windows.UI.Core.coreWindow. The
associated
>   // windows cannot be captured, so we skip them, http://crbug.com/526883.
> 
>
https://chromiumcodereview.appspot.com/1371383003/diff/20001/webrtc/modules/d...
> webrtc/modules/desktop_capture/window_capturer_win.cc:73: char
> class_name[kMaxNameLength];
> Why not reuse |kClassLength|? (l.43)
> 
>
https://chromiumcodereview.appspot.com/1371383003/diff/20001/webrtc/modules/d...
> webrtc/modules/desktop_capture/window_capturer_win.cc:74: int
class_name_length
> = GetClassNameA(hwnd, class_name, kMaxNameLength);
> const
> 
>
https://chromiumcodereview.appspot.com/1371383003/diff/20001/webrtc/modules/d...
> webrtc/modules/desktop_capture/window_capturer_win.cc:74: int
class_name_length
> = GetClassNameA(hwnd, class_name, kMaxNameLength);
> |class_name_length| or |classNameLength| ?

Updated based on comments and move the position of the code.

Thanks,

George

[mcasas, 2015-10-02 00:34:05]

https://chromiumcodereview.appspot.com/1371383003/diff/40001/webrtc/modules/d...
File webrtc/modules/desktop_capture/window_capturer_win.cc (right):

https://chromiumcodereview.appspot.com/1371383003/diff/40001/webrtc/modules/d...
webrtc/modules/desktop_capture/window_capturer_win.cc:59: wcscmp(class_name,
L"Windows.UI.Core.CoreWindow") == 0)
Still needs {} around l.60.

https://chromiumcodereview.appspot.com/1371383003/diff/40001/webrtc/modules/d...
webrtc/modules/desktop_capture/window_capturer_win.cc:59: wcscmp(class_name,
L"Windows.UI.Core.CoreWindow") == 0)
What about 
  if (rtc::IsWindows8OrLater() &&
      (wcscmp(class_name, L"ApplicationFrameWindow") == 0 ||
       wcscmp(class_name, L"Windows.UI.Core.CoreWindow") == 0)) {
    bla;
  }

?

[gyzhou, 2015-10-02 18:30:34]

gyzhou@google.com changed reviewers:
+ gyzhou@google.com

[gyzhou, 2015-10-02 18:30:35]

Updated based on your comments.

https://chromiumcodereview.appspot.com/1371383003/diff/1/webrtc/modules/deskt...
File webrtc/modules/desktop_capture/window_capturer_win.cc (right):

https://chromiumcodereview.appspot.com/1371383003/diff/1/webrtc/modules/deskt...
webrtc/modules/desktop_capture/window_capturer_win.cc:70: // Webrtc is not able
to capture modern apps' windows on 09/30/2015.
On 2015/10/01 17:19:04, mcasas wrote:
> This is a bit too verbose :) Please rephrase more succintly
> and with a view on the purpose, i.e. "Windows X cannot
> capture windows of type Y; those are identified by Z and W".
> 
> What's "modern" referring to? If there is a bug associated
> to this peculiar situation, mention it like http://crbug.com/bla.
> 

Done.

https://chromiumcodereview.appspot.com/1371383003/diff/1/webrtc/modules/deskt...
webrtc/modules/desktop_capture/window_capturer_win.cc:72: char clsName[1024];
On 2015/10/01 17:19:04, mcasas wrote:
> This needs to be WCHAR right? Like in l.53.

GetClassName using WCHAR as input
GetClassNameA can use char as input

https://chromiumcodereview.appspot.com/1371383003/diff/1/webrtc/modules/deskt...
webrtc/modules/desktop_capture/window_capturer_win.cc:73: GetClassNameA(hwnd,
clsName, 1024);
On 2015/10/01 17:19:04, mcasas wrote:
> What about the return value? and what is 1024? 
> Sugggestion: 
> 
> const size_t kMaxNameLength = 1024u;
> std::wstring clsName(kMaxNameLength);
> const int class_name_length = GetClassNAmeA(hwnd, clsName.data(),
> kMaxNameLength);
> RTC_DCHECK(class_name_length) << "Error retrieving the application's class
> name";
> if (clsName == "ApplicationFrameWindow" ||
>     clsName == "Windows.UI.CoreWindow)) {
>   return TRUE;
> }
> 
> (Mind the  {}, needed if either condition or body are longer than
> a line).
> 
> (Mind TRUE versus |true|).

Done.

https://chromiumcodereview.appspot.com/1371383003/diff/1/webrtc/modules/deskt...
webrtc/modules/desktop_capture/window_capturer_win.cc:75: strcmp(clsName,
"Windows.UI.Core.CoreWindow") == 0)
On 2015/10/01 17:19:04, mcasas wrote:
> Nit: I wouldn't use camlcase names ever, but if that's 
> the style of the file/folder, then so be it. Rename my
> variables accordingly above if this is the case.

Done.

https://chromiumcodereview.appspot.com/1371383003/diff/20001/webrtc/modules/d...
File webrtc/modules/desktop_capture/window_capturer_win.cc (right):

https://chromiumcodereview.appspot.com/1371383003/diff/20001/webrtc/modules/d...
webrtc/modules/desktop_capture/window_capturer_win.cc:71: // and/or
windows.UI.Core.coreWindow.
On 2015/10/01 21:59:17, mcasas wrote:
> Suggestion of rephrase:
>   // Windows 8 introduced a "Modern App" identified by their class name being
>   // either ApplicationFrameWindow or windows.UI.Core.coreWindow. The
associated
>   // windows cannot be captured, so we skip them, http://crbug.com/526883.

Done.

https://chromiumcodereview.appspot.com/1371383003/diff/20001/webrtc/modules/d...
webrtc/modules/desktop_capture/window_capturer_win.cc:73: char
class_name[kMaxNameLength];
On 2015/10/01 21:59:17, mcasas wrote:
> Why not reuse |kClassLength|? (l.43)

Done.

Since GetClassName() called in lin45. It makes sensor to move this part of code
up to remove some redundancy.

https://chromiumcodereview.appspot.com/1371383003/diff/20001/webrtc/modules/d...
webrtc/modules/desktop_capture/window_capturer_win.cc:74: int class_name_length
= GetClassNameA(hwnd, class_name, kMaxNameLength);
On 2015/10/01 21:59:17, mcasas wrote:
> const

Done.

https://chromiumcodereview.appspot.com/1371383003/diff/20001/webrtc/modules/d...
webrtc/modules/desktop_capture/window_capturer_win.cc:74: int class_name_length
= GetClassNameA(hwnd, class_name, kMaxNameLength);
On 2015/10/01 21:59:17, mcasas wrote:
> |class_name_length| or |classNameLength| ?

class_name_length is consistent with other part of the code.

https://chromiumcodereview.appspot.com/1371383003/diff/20001/webrtc/modules/d...
webrtc/modules/desktop_capture/window_capturer_win.cc:74: int class_name_length
= GetClassNameA(hwnd, class_name, kMaxNameLength);
On 2015/10/01 21:59:17, mcasas wrote:
> const

Done.

https://chromiumcodereview.appspot.com/1371383003/diff/40001/webrtc/modules/d...
File webrtc/modules/desktop_capture/window_capturer_win.cc (right):

https://chromiumcodereview.appspot.com/1371383003/diff/40001/webrtc/modules/d...
webrtc/modules/desktop_capture/window_capturer_win.cc:59: wcscmp(class_name,
L"Windows.UI.Core.CoreWindow") == 0)
On 2015/10/02 00:34:05, mcasas wrote:
> What about 
>   if (rtc::IsWindows8OrLater() &&
>       (wcscmp(class_name, L"ApplicationFrameWindow") == 0 ||
>        wcscmp(class_name, L"Windows.UI.Core.CoreWindow") == 0)) {
>     bla;
>   }
> 
> ?

Done.

https://chromiumcodereview.appspot.com/1371383003/diff/40001/webrtc/modules/d...
webrtc/modules/desktop_capture/window_capturer_win.cc:59: wcscmp(class_name,
L"Windows.UI.Core.CoreWindow") == 0)
On 2015/10/02 00:34:05, mcasas wrote:
> Still needs {} around l.60.

Done.

[mcasas, 2015-10-02 18:59:25]

mcasas@chromium.org changed reviewers:
+ sergeyu@chromium.org
- gyzhou@google.com

[mcasas, 2015-10-02 18:59:26]

LGTM % my comment below.

You'll still need an OWNER approval, +sergeyu@ for that.

Also, please update de description to be more descriptive,
e.g.

WebRtc Win Desktop capture: ignore Win8+ Modern Apps' windows.

etc.

https://chromiumcodereview.appspot.com/1371383003/diff/60001/webrtc/modules/d...
File webrtc/modules/desktop_capture/window_capturer_win.cc (right):

https://chromiumcodereview.appspot.com/1371383003/diff/60001/webrtc/modules/d...
webrtc/modules/desktop_capture/window_capturer_win.cc:59: wcscmp(class_name,
L"Windows.UI.Core.CoreWindow") == 0)){
Space between ) and { 
:)

Pro-tip: run `git cl format` to properly indent and
justify your patch before `git cl upload`, and your
style-guide problems will (mostly) disappear.

[GeorgeZ, 2015-10-02 20:30:43]

On 2015/10/02 18:59:26, mcasas wrote:
> LGTM % my comment below.
> 
> You'll still need an OWNER approval, +sergeyu@ for that.
> 
> Also, please update de description to be more descriptive,
> e.g.
> 
> WebRtc Win Desktop capture: ignore Win8+ Modern Apps' windows.
> 
> etc.
> 
>
https://chromiumcodereview.appspot.com/1371383003/diff/60001/webrtc/modules/d...
> File webrtc/modules/desktop_capture/window_capturer_win.cc (right):
> 
>
https://chromiumcodereview.appspot.com/1371383003/diff/60001/webrtc/modules/d...
> webrtc/modules/desktop_capture/window_capturer_win.cc:59: wcscmp(class_name,
> L"Windows.UI.Core.CoreWindow") == 0)){
> Space between ) and { 
> :)
> 
> Pro-tip: run `git cl format` to properly indent and
> justify your patch before `git cl upload`, and your
> style-guide problems will (mostly) disappear.

Appreciate all your help.

George

[GeorgeZ, 2015-10-02 20:32:41]

Sergey,

Please have a review for me.

Thanks,

George

https://chromiumcodereview.appspot.com/1371383003/diff/60001/webrtc/modules/d...
File webrtc/modules/desktop_capture/window_capturer_win.cc (right):

https://chromiumcodereview.appspot.com/1371383003/diff/60001/webrtc/modules/d...
webrtc/modules/desktop_capture/window_capturer_win.cc:59: wcscmp(class_name,
L"Windows.UI.Core.CoreWindow") == 0)){
On 2015/10/02 18:59:26, mcasas wrote:
> Space between ) and { 
> :)
> 
> Pro-tip: run `git cl format` to properly indent and
> justify your patch before `git cl upload`, and your
> style-guide problems will (mostly) disappear.

Done.

[Sergey Ulanov, 2015-10-02 21:48:17]

lgtm

[gyzhou, 2015-10-02 21:48:38]

The CQ bit was checked by gyzhou@google.com

[gyzhou, 2015-10-02 21:48:38]

The patchset sent to the CQ was uploaded after l-g-t-m from mcasas@chromium.org
Link to the patchset: https://codereview.chromium.org/1371383003/#ps80001
(title: "review4")

[commit-bot: I haz the power, 2015-10-02 21:48:50]

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

[commit-bot: I haz the power, 2015-10-02 22:36:35]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2015-10-02 22:36:42]

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