Change single-display screens to use ScreenBase and DisplayList.

Change single-display screens to use ScreenBase and DisplayList.

Reduce similar code in single-display screens.

1. ScreenBase doesn't set itself as the screen instance in
display::Screen anymore, rather its subclass needs to decide if
it wants to be the screen instance.

2. Moved Displays::iterator FindDisplayById(int64_t id); to private
in DisplayList.

3. Fixed BlimpScreenTest since Display::SetScaleAndBounds
also updates the work area of a display.

4. Fixed RenderWidgetHostViewAuraTest.PhysicalBackingSizeWithScale
since added UpdateDisplay call in aura::TestScreen::SetDeviceScaleFactor.

BUG=none, related to https://codereview.chromium.org/2361283002
TEST=covered by blimp_unittests, app_shell_unittests,
     content_unittests, unit_tests, mash_unittests,
     extensions_browsertests

Committed: https://crrev.com/bcbb8c44d3e5d2bdc1da14867c3f05527e349d8b
Cr-Commit-Position: refs/heads/master@{#430312}

[riajiang, 2016-10-18 00:55:20]

Patchset #1 (id:1) has been deleted

[riajiang, 2016-10-18 01:09:07]

Patchset #1 (id:20001) has been deleted

[riajiang, 2016-10-18 01:10:46]

riajiang@chromium.org changed reviewers:
+ sadrul@chromium.org, sky@chromium.org

[riajiang, 2016-10-18 01:10:47]

Hi sadrul@ and sky@, PTAL. Thanks!

[riajiang, 2016-10-18 01:11:02]

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

[commit-bot: I haz the power, 2016-10-18 01:11:28]

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-18 01:35:21]

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

[commit-bot: I haz the power, 2016-10-18 01:35:22]

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

[sadrul, 2016-10-19 15:27:50]

Some of the try failures look related. I think the failure happens because
ScreenBase sets itself as the singleton screen instance, causing issues in
aura::test::TestScreen. Can you look into that?

[riajiang, 2016-10-25 00:04:53]

Patchset #2 (id:60001) has been deleted

[riajiang, 2016-10-26 22:35:28]

Description was changed from

==========
Change single-display screens to use ScreenBase and DisplayList.

BUG=none, related to https://codereview.chromium.org/2361283002
TEST=blimp_unittests app_shell_unittests
==========

to

==========
Change single-display screens to use ScreenBase and DisplayList.

Reduce similar code in single-display screens.

ScreenBase doesn't set itself as the screen instance in
display::Screen anymore, rather its subclass needs to decide if 
it wants to be the screen instance.

BUG=none, related to https://codereview.chromium.org/2361283002
TEST=covered by blimp_unittests, app_shell_unittests, 
     content_unittests, unit_tests, mash_unittests,
     extensions_browsertests
==========

[riajiang, 2016-10-26 23:40:48]

Patchset #2 (id:80001) has been deleted

[riajiang, 2016-10-26 23:41:27]

Please take another look. Thanks!

[sky, 2016-10-27 00:05:13]

https://codereview.chromium.org/2431473002/diff/100001/ui/display/display_list.h
File ui/display/display_list.h (right):

https://codereview.chromium.org/2431473002/diff/100001/ui/display/display_lis...
ui/display/display_list.h:56: Displays& displays() { return displays_; }
If you expose this, it means someone could modify the list outside of this
class. That is bad as it potentially defeats the observers added to this class.
Is there a reason you can't force calls to go through the mutation methods of
this class?

[riajiang, 2016-10-27 19:45:05]

Patchset #3 (id:120001) has been deleted

[riajiang, 2016-10-27 23:23:23]

https://codereview.chromium.org/2431473002/diff/100001/ui/display/display_list.h
File ui/display/display_list.h (right):

https://codereview.chromium.org/2431473002/diff/100001/ui/display/display_lis...
ui/display/display_list.h:56: Displays& displays() { return displays_; }
On 2016/10/27 00:05:13, sky wrote:
> If you expose this, it means someone could modify the list outside of this
> class. That is bad as it potentially defeats the observers added to this
class.
> Is there a reason you can't force calls to go through the mutation methods of
> this class?

I wanted to access one of the display in displays() and modify it (e.g. bounds).
UpdateDisplay() won't allow me to do that. But I found that I can use
FindDisplayById() for that. Is this approach okay?

[sky, 2016-10-27 23:38:33]

https://codereview.chromium.org/2431473002/diff/100001/ui/display/display_list.h
File ui/display/display_list.h (right):

https://codereview.chromium.org/2431473002/diff/100001/ui/display/display_lis...
ui/display/display_list.h:56: Displays& displays() { return displays_; }
On 2016/10/27 23:23:23, riajiang wrote:
> On 2016/10/27 00:05:13, sky wrote:
> > If you expose this, it means someone could modify the list outside of this
> > class. That is bad as it potentially defeats the observers added to this
> class.
> > Is there a reason you can't force calls to go through the mutation methods
of
> > this class?
> 
> I wanted to access one of the display in displays() and modify it (e.g.
bounds).
> UpdateDisplay() won't allow me to do that. But I found that I can use
> FindDisplayById() for that. Is this approach okay?

I think we shouldn't expose an iterator, only a const_iterator. Otherwise there
is the issue I have. As your use case, can't you get the display (using the
const_iterator), copy it, modify the bounds, then call UpdateDisplay? That way
observers are appropriately notified.

[riajiang, 2016-10-31 20:32:27]

Description was changed from

==========
Change single-display screens to use ScreenBase and DisplayList.

Reduce similar code in single-display screens.

ScreenBase doesn't set itself as the screen instance in
display::Screen anymore, rather its subclass needs to decide if 
it wants to be the screen instance.

BUG=none, related to https://codereview.chromium.org/2361283002
TEST=covered by blimp_unittests, app_shell_unittests, 
     content_unittests, unit_tests, mash_unittests,
     extensions_browsertests
==========

to

==========
Change single-display screens to use ScreenBase and DisplayList.

Reduce similar code in single-display screens.

ScreenBase doesn't set itself as the screen instance in
display::Screen anymore, rather its subclass needs to decide if 
it wants to be the screen instance.

Moved Displays::iterator FindDisplayById(int64_t id); to private
in DisplayList.

Fixed BlimpScreenTest since Display::SetScaleAndBounds 
also updates the work area of a display.

BUG=none, related to https://codereview.chromium.org/2361283002
TEST=covered by blimp_unittests, app_shell_unittests, 
     content_unittests, unit_tests, mash_unittests,
     extensions_browsertests
==========

[riajiang, 2016-10-31 21:49:16]

Patchset #5 (id:180001) has been deleted

[riajiang, 2016-10-31 22:29:00]

Patchset #5 (id:200001) has been deleted

[riajiang, 2016-11-01 18:10:53]

Description was changed from

==========
Change single-display screens to use ScreenBase and DisplayList.

Reduce similar code in single-display screens.

ScreenBase doesn't set itself as the screen instance in
display::Screen anymore, rather its subclass needs to decide if 
it wants to be the screen instance.

Moved Displays::iterator FindDisplayById(int64_t id); to private
in DisplayList.

Fixed BlimpScreenTest since Display::SetScaleAndBounds 
also updates the work area of a display.

BUG=none, related to https://codereview.chromium.org/2361283002
TEST=covered by blimp_unittests, app_shell_unittests, 
     content_unittests, unit_tests, mash_unittests,
     extensions_browsertests
==========

to

==========
Change single-display screens to use ScreenBase and DisplayList.

Reduce similar code in single-display screens.

1. ScreenBase doesn't set itself as the screen instance in
display::Screen anymore, rather its subclass needs to decide if 
it wants to be the screen instance.

2. Moved Displays::iterator FindDisplayById(int64_t id); to private
in DisplayList.

3. Fixed BlimpScreenTest since Display::SetScaleAndBounds 
also updates the work area of a display.

4. Fixed RenderWidgetHostViewAuraTest.PhysicalBackingSizeWithScale
since added UpdateDisplay call in aura::TestScreen::SetDeviceScaleFactor.

BUG=none, related to https://codereview.chromium.org/2361283002
TEST=covered by blimp_unittests, app_shell_unittests, 
     content_unittests, unit_tests, mash_unittests,
     extensions_browsertests
==========

[riajiang, 2016-11-01 20:35:44]

On 2016/10/27 23:38:33, sky wrote:
>
https://codereview.chromium.org/2431473002/diff/100001/ui/display/display_list.h
> File ui/display/display_list.h (right):
> 
>
https://codereview.chromium.org/2431473002/diff/100001/ui/display/display_lis...
> ui/display/display_list.h:56: Displays& displays() { return displays_; }
> On 2016/10/27 23:23:23, riajiang wrote:
> > On 2016/10/27 00:05:13, sky wrote:
> > > If you expose this, it means someone could modify the list outside of this
> > > class. That is bad as it potentially defeats the observers added to this
> > class.
> > > Is there a reason you can't force calls to go through the mutation methods
> of
> > > this class?
> > 
> > I wanted to access one of the display in displays() and modify it (e.g.
> bounds).
> > UpdateDisplay() won't allow me to do that. But I found that I can use
> > FindDisplayById() for that. Is this approach okay?
> 
> I think we shouldn't expose an iterator, only a const_iterator. Otherwise
there
> is the issue I have. As your use case, can't you get the display (using the
> const_iterator), copy it, modify the bounds, then call UpdateDisplay? That way
> observers are appropriately notified.

So I made the following changes:
1. Moved Displays::iterator FindDisplayById(int64_t id); to private
in DisplayList. Right now it's get the display (const_iterator) -> copy ->modify

-> call UpdateDisplay.
2. Fixed BlimpScreenTest since Display::SetScaleAndBounds 
also updates the work area of a display.
3. Fixed RenderWidgetHostViewAuraTest.PhysicalBackingSizeWithScale
since added UpdateDisplay call in aura::TestScreen::SetDeviceScaleFactor.

[sky, 2016-11-01 21:15:36]

https://codereview.chromium.org/2431473002/diff/260001/chrome/browser/manifes...
File chrome/browser/manifest/manifest_icon_selector_unittest.cc (right):

https://codereview.chromium.org/2431473002/diff/260001/chrome/browser/manifes...
chrome/browser/manifest/manifest_icon_selector_unittest.cc:27: if (display.id()
!= test_display.id()) {
Why the if/else case? Don't you know which case to deal with?

https://codereview.chromium.org/2431473002/diff/260001/content/browser/media/...
File content/browser/media/capture/web_contents_video_capture_device_unittest.cc
(right):

https://codereview.chromium.org/2431473002/diff/260001/content/browser/media/...
content/browser/media/capture/web_contents_video_capture_device_unittest.cc:611:
if (display.id() != test_display.id()) {
Similar coment here.

https://codereview.chromium.org/2431473002/diff/260001/ui/display/display_list.h
File ui/display/display_list.h (right):

https://codereview.chromium.org/2431473002/diff/260001/ui/display/display_lis...
ui/display/display_list.h:95: Displays::iterator FindDisplayByIdMut(int64_t id);
Mut->Internal?

https://codereview.chromium.org/2431473002/diff/260001/ui/display/screen_base.cc
File ui/display/screen_base.cc (left):

https://codereview.chromium.org/2431473002/diff/260001/ui/display/screen_base...
ui/display/screen_base.cc:16: display::Screen::SetScreenInstance(this);
How come you are removing the code that sets the instance from here and pushing
to subclasses?

https://codereview.chromium.org/2431473002/diff/260001/ui/display/screen_base...
ui/display/screen_base.cc:39: NOTIMPLEMENTED();
Why are you removing the NOTIMPLEMENTEDs?

[riajiang, 2016-11-02 01:18:39]

Patchset #8 (id:280001) has been deleted

[riajiang, 2016-11-02 01:23:13]

https://codereview.chromium.org/2431473002/diff/260001/chrome/browser/manifes...
File chrome/browser/manifest/manifest_icon_selector_unittest.cc (right):

https://codereview.chromium.org/2431473002/diff/260001/chrome/browser/manifes...
chrome/browser/manifest/manifest_icon_selector_unittest.cc:27: if (display.id()
!= test_display.id()) {
On 2016/11/01 21:15:36, sky wrote:
> Why the if/else case? Don't you know which case to deal with?

It is always the first case since the default display id in display::TestScreen
is not 0x1337 but I was thinking to make it more general here... I guess since
it specifically sets id here, it's somewhat clear that it's going to be
different id.

https://codereview.chromium.org/2431473002/diff/260001/content/browser/media/...
File content/browser/media/capture/web_contents_video_capture_device_unittest.cc
(right):

https://codereview.chromium.org/2431473002/diff/260001/content/browser/media/...
content/browser/media/capture/web_contents_video_capture_device_unittest.cc:611:
if (display.id() != test_display.id()) {
On 2016/11/01 21:15:36, sky wrote:
> Similar coment here.

Same as the reply above.

https://codereview.chromium.org/2431473002/diff/260001/ui/display/display_list.h
File ui/display/display_list.h (right):

https://codereview.chromium.org/2431473002/diff/260001/ui/display/display_lis...
ui/display/display_list.h:95: Displays::iterator FindDisplayByIdMut(int64_t id);
On 2016/11/01 21:15:36, sky wrote:
> Mut->Internal?

Done.

https://codereview.chromium.org/2431473002/diff/260001/ui/display/screen_base.cc
File ui/display/screen_base.cc (left):

https://codereview.chromium.org/2431473002/diff/260001/ui/display/screen_base...
ui/display/screen_base.cc:16: display::Screen::SetScreenInstance(this);
On 2016/11/01 21:15:36, sky wrote:
> How come you are removing the code that sets the instance from here and
pushing
> to subclasses?

As discussed: Right now BlimpScreen, CastScreen, TestScreen etc are all
subclasses of ScreenBase but they are not expected to be the screen instance. On
the other hand ScreenBase only had ScreenMus that is expected to be the screen
instance as its subclass before so I thought it's easier to make the change for
subclasses to decide whether it wanted to be the screen instance. A lot of tests
(for example this one
https://cs.chromium.org/chromium/src/chrome/test/base/in_process_browser_test...
which I wasn't sure how to make an easy change) set its screen instance. Maybe
change all these to not set itself as the screen instance in a future CL.

https://codereview.chromium.org/2431473002/diff/260001/ui/display/screen_base...
ui/display/screen_base.cc:39: NOTIMPLEMENTED();
On 2016/11/01 21:15:36, sky wrote:
> Why are you removing the NOTIMPLEMENTEDs?

As discussed, moved implementations to subclasses to ensure they implement the
right version. Instead of keeping these functions virtual in ScreenBase, I added
back this NOTIMPLEMENTEDs because WindowManager is creating an object of
ScreenBase. WDYT?

[sky, 2016-11-02 15:27:54]

https://codereview.chromium.org/2431473002/diff/300001/ui/aura/test/test_scre...
File ui/aura/test/test_screen.cc (right):

https://codereview.chromium.org/2431473002/diff/300001/ui/aura/test/test_scre...
ui/aura/test/test_screen.cc:49:
.CallWmNewDisplayAdded(*display_list().GetPrimaryDisplayIterator());
GetPrimaryDisplay()?

https://codereview.chromium.org/2431473002/diff/300001/ui/aura/test/test_scre...
ui/aura/test/test_screen.cc:52:
display_list().GetPrimaryDisplayIterator()->GetSizeInPixel()));
GetPrimaryDisplay()?

https://codereview.chromium.org/2431473002/diff/300001/ui/aura/test/test_scre...
ui/aura/test/test_screen.cc:63: display::Display
display(*display_list().GetPrimaryDisplayIterator());
GetPrimaryDisplay for all these?

https://codereview.chromium.org/2431473002/diff/300001/ui/display/test/test_s...
File ui/display/test/test_screen.cc (right):

https://codereview.chromium.org/2431473002/diff/300001/ui/display/test/test_s...
ui/display/test/test_screen.cc:32: return
*display_list().GetPrimaryDisplayIterator();
GetPrimaryDisplay().

https://codereview.chromium.org/2431473002/diff/300001/ui/display/test/test_s...
File ui/display/test/test_screen.h (right):

https://codereview.chromium.org/2431473002/diff/300001/ui/display/test/test_s...
ui/display/test/test_screen.h:29: return
*display_list().GetPrimaryDisplayIterator();
This is the same as GetPrimaryDisplay(). Do we really need both?

[riajiang, 2016-11-02 17:31:42]

On 2016/11/02 15:27:54, sky wrote:
>
https://codereview.chromium.org/2431473002/diff/300001/ui/aura/test/test_scre...
> File ui/aura/test/test_screen.cc (right):
> 
>
https://codereview.chromium.org/2431473002/diff/300001/ui/aura/test/test_scre...
> ui/aura/test/test_screen.cc:49:
> .CallWmNewDisplayAdded(*display_list().GetPrimaryDisplayIterator());
> GetPrimaryDisplay()?
> 
>
https://codereview.chromium.org/2431473002/diff/300001/ui/aura/test/test_scre...
> ui/aura/test/test_screen.cc:52:
> display_list().GetPrimaryDisplayIterator()->GetSizeInPixel()));
> GetPrimaryDisplay()?
> 
>
https://codereview.chromium.org/2431473002/diff/300001/ui/aura/test/test_scre...
> ui/aura/test/test_screen.cc:63: display::Display
> display(*display_list().GetPrimaryDisplayIterator());
> GetPrimaryDisplay for all these?
> 
>
https://codereview.chromium.org/2431473002/diff/300001/ui/display/test/test_s...
> File ui/display/test/test_screen.cc (right):
> 
>
https://codereview.chromium.org/2431473002/diff/300001/ui/display/test/test_s...
> ui/display/test/test_screen.cc:32: return
> *display_list().GetPrimaryDisplayIterator();
> GetPrimaryDisplay().
> 
>
https://codereview.chromium.org/2431473002/diff/300001/ui/display/test/test_s...
> File ui/display/test/test_screen.h (right):
> 
>
https://codereview.chromium.org/2431473002/diff/300001/ui/display/test/test_s...
> ui/display/test/test_screen.h:29: return
> *display_list().GetPrimaryDisplayIterator();
> This is the same as GetPrimaryDisplay(). Do we really need both?

But ScreenBase::GetPrimaryDisplay would make one more copy compared to using 
DisplayList::GetPrimaryDisplayIterator directly? GetPrimaryDisplayIterator is
still being used 
at some places to see if a display is the primary display or if a primary
display exists (e.g.
https://cs.chromium.org/chromium/src/ui/views/mus/screen_mus.cc?type=cs&sq=pa...

[sky, 2016-11-02 18:09:49]

On Wed, Nov 2, 2016 at 10:31 AM,  <riajiang@chromium.org> wrote:
> On 2016/11/02 15:27:54, sky wrote:
>>
>
https://codereview.chromium.org/2431473002/diff/300001/ui/aura/test/test_scre...
>> File ui/aura/test/test_screen.cc (right):
>>
>>
>
https://codereview.chromium.org/2431473002/diff/300001/ui/aura/test/test_scre...
>> ui/aura/test/test_screen.cc:49:
>> .CallWmNewDisplayAdded(*display_list().GetPrimaryDisplayIterator());
>> GetPrimaryDisplay()?
>>
>>
>
https://codereview.chromium.org/2431473002/diff/300001/ui/aura/test/test_scre...
>> ui/aura/test/test_screen.cc:52:
>> display_list().GetPrimaryDisplayIterator()->GetSizeInPixel()));
>> GetPrimaryDisplay()?
>>
>>
>
https://codereview.chromium.org/2431473002/diff/300001/ui/aura/test/test_scre...
>> ui/aura/test/test_screen.cc:63: display::Display
>> display(*display_list().GetPrimaryDisplayIterator());
>> GetPrimaryDisplay for all these?
>>
>>
>
https://codereview.chromium.org/2431473002/diff/300001/ui/display/test/test_s...
>> File ui/display/test/test_screen.cc (right):
>>
>>
>
https://codereview.chromium.org/2431473002/diff/300001/ui/display/test/test_s...
>> ui/display/test/test_screen.cc:32: return
>> *display_list().GetPrimaryDisplayIterator();
>> GetPrimaryDisplay().
>>
>>
>
https://codereview.chromium.org/2431473002/diff/300001/ui/display/test/test_s...
>> File ui/display/test/test_screen.h (right):
>>
>>
>
https://codereview.chromium.org/2431473002/diff/300001/ui/display/test/test_s...
>> ui/display/test/test_screen.h:29: return
>> *display_list().GetPrimaryDisplayIterator();
>> This is the same as GetPrimaryDisplay(). Do we really need both?
>
> But ScreenBase::GetPrimaryDisplay would make one more copy compared to using
> DisplayList::GetPrimaryDisplayIterator directly? GetPrimaryDisplayIterator
> is
> still being used
> at some places to see if a display is the primary display or if a primary
> display exists (e.g.
>
https://cs.chromium.org/chromium/src/ui/views/mus/screen_mus.cc?type=cs&sq=pa...
>
> https://codereview.chromium.org/2431473002/

This is certainly true, but this is test code, and it's better to have
a single function vs two that do the same thing.

  -Scott

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[kylechar, 2016-11-02 19:52:29]

On 2016/11/02 17:31:42, riajiang wrote:
> But ScreenBase::GetPrimaryDisplay would make one more copy compared to using 
> DisplayList::GetPrimaryDisplayIterator directly? GetPrimaryDisplayIterator is
> still being used 
> at some places to see if a display is the primary display or if a primary
> display exists (e.g.
>
https://cs.chromium.org/chromium/src/ui/views/mus/screen_mus.cc?type=cs&sq=pa...

The extra copy is most likely omitted due to copy elision.

http://en.cppreference.com/w/cpp/language/copy_elision

(I think this case would qualify for mandatory copy elision that is being
introduced in C++17).

[riajiang, 2016-11-02 23:12:52]

> This is certainly true, but this is test code, and it's better to have
> a single function vs two that do the same thing.
> 
>   -Scott

Oh sorry I misunderstood your comment - I thought you meant
DisplayList::GetPrimaryDisplayIterator() but you meant 
TestScreen::display(). Deleted display() and changed to call 
GetPrimaryDisplay() instead.

Also changed all display_list().GetPrimaryDisplayIterator() to be
GetPrimaryDisplay().


> The extra copy is most likely omitted due to copy elision.
> 
> http://en.cppreference.com/w/cpp/language/copy_elision
> 
> (I think this case would qualify for mandatory copy elision that is being
> introduced in C++17).

Thanks! Changed all display_list().GetPrimaryDisplayIterator() to be
GetPrimaryDisplay().

[sky, 2016-11-02 23:17:38]

LGTM - I'm not an owner of the all the directories you're changing. You'll need
to add reviewers for blimp, chromecast, content and headless.

[riajiang, 2016-11-03 01:04:18]

riajiang@chromium.org changed reviewers:
+ alokp@chromium.org, jamescook@chromium.org, miu@chromium.org,
nyquist@chromium.org, skyostil@chromium.org

[riajiang, 2016-11-03 01:04:19]

+skyostil@chromium.org: Please review changes in headless/

+alokp@chromium.org: Please review changes in chromecast/

+jamescook@chromium.org: Please review changes in extensions/shell/

+miu@chromium.org: Please review changes in content/browser/media/capture/

+nyquist@chromium.org: Please review changes in blimp/

Thanks!

[Sami, 2016-11-03 11:51:56]

headless/ lgtm, thanks! I've also kicked off a headless tryjob for you (it's not
yet on the cq).

[James Cook, 2016-11-03 16:47:32]

extensions/shell lgtm with nit

https://codereview.chromium.org/2431473002/diff/320001/extensions/shell/brows...
File extensions/shell/browser/shell_screen.cc (right):

https://codereview.chromium.org/2431473002/diff/320001/extensions/shell/brows...
extensions/shell/browser/shell_screen.cc:31: ProcessDisplayChanged(display, true
/*is_primary */);
nit: space after /*

[riajiang, 2016-11-03 22:43:03]

Patchset #10 (id:340001) has been deleted

[alokp, 2016-11-04 20:47:33]

chromecast/ lgtm

[miu, 2016-11-04 21:15:47]

web_contents_video_capture_device_unittest.cc lgtm

[nyquist, 2016-11-04 21:35:17]

blimp lgtm

[riajiang, 2016-11-04 22:13:09]

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

[commit-bot: I haz the power, 2016-11-04 22:14:18]

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-11-04 23:50:00]

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

[commit-bot: I haz the power, 2016-11-04 23:50:01]

Dry run: This issue passed the CQ dry run.

[sadrul, 2016-11-05 00:38:12]

lgtm

https://codereview.chromium.org/2431473002/diff/360001/content/browser/render...
File content/browser/renderer_host/render_widget_host_view_aura_unittest.cc
(right):

https://codereview.chromium.org/2431473002/diff/360001/content/browser/render...
content/browser/renderer_host/render_widget_host_view_aura_unittest.cc:1517:
EXPECT_EQ(1u, sink_->message_count());
Add a comment here that changing the DSF triggers the
RenderWidgetHostViewAura::OnDisplayMetricsChanged() observer callback, which
sends a ViewMsg_Resize::ID message to the renderer. Add a check here for that
(like above in lines 1498:1499)

https://codereview.chromium.org/2431473002/diff/360001/content/browser/render...
content/browser/renderer_host/render_widget_host_view_aura_unittest.cc:1528:
EXPECT_EQ(1u, sink_->message_count());
Test the message-type too like above.

[riajiang, 2016-11-07 16:23:09]

https://codereview.chromium.org/2431473002/diff/320001/extensions/shell/brows...
File extensions/shell/browser/shell_screen.cc (right):

https://codereview.chromium.org/2431473002/diff/320001/extensions/shell/brows...
extensions/shell/browser/shell_screen.cc:31: ProcessDisplayChanged(display, true
/*is_primary */);
On 2016/11/03 16:47:32, James Cook (OOO until Nov 9) wrote:
> nit: space after /*

Done.

https://codereview.chromium.org/2431473002/diff/360001/content/browser/render...
File content/browser/renderer_host/render_widget_host_view_aura_unittest.cc
(right):

https://codereview.chromium.org/2431473002/diff/360001/content/browser/render...
content/browser/renderer_host/render_widget_host_view_aura_unittest.cc:1517:
EXPECT_EQ(1u, sink_->message_count());
On 2016/11/05 00:38:12, sadrul wrote:
> Add a comment here that changing the DSF triggers the
> RenderWidgetHostViewAura::OnDisplayMetricsChanged() observer callback, which
> sends a ViewMsg_Resize::ID message to the renderer. Add a check here for that
> (like above in lines 1498:1499)

Done.

https://codereview.chromium.org/2431473002/diff/360001/content/browser/render...
content/browser/renderer_host/render_widget_host_view_aura_unittest.cc:1528:
EXPECT_EQ(1u, sink_->message_count());
On 2016/11/05 00:38:11, sadrul wrote:
> Test the message-type too like above.

Done.

[riajiang, 2016-11-07 16:24:11]

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

[commit-bot: I haz the power, 2016-11-07 16:24:41]

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-11-07 17:26:13]

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

[commit-bot: I haz the power, 2016-11-07 17:26:14]

Dry run: This issue passed the CQ dry run.

[riajiang, 2016-11-07 17:50:29]

The CQ bit was checked by riajiang@chromium.org

[riajiang, 2016-11-07 17:50:30]

The patchset sent to the CQ was uploaded after l-g-t-m from
jamescook@chromium.org, sky@chromium.org, skyostil@chromium.org,
sadrul@chromium.org, alokp@chromium.org, nyquist@chromium.org, miu@chromium.org
Link to the patchset: https://codereview.chromium.org/2431473002/#ps380001
(title: "renderer test")

[commit-bot: I haz the power, 2016-11-07 17:50:55]

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-11-07 17:58:11]

Description was changed from

==========
Change single-display screens to use ScreenBase and DisplayList.

Reduce similar code in single-display screens.

1. ScreenBase doesn't set itself as the screen instance in
display::Screen anymore, rather its subclass needs to decide if 
it wants to be the screen instance.

2. Moved Displays::iterator FindDisplayById(int64_t id); to private
in DisplayList.

3. Fixed BlimpScreenTest since Display::SetScaleAndBounds 
also updates the work area of a display.

4. Fixed RenderWidgetHostViewAuraTest.PhysicalBackingSizeWithScale
since added UpdateDisplay call in aura::TestScreen::SetDeviceScaleFactor.

BUG=none, related to https://codereview.chromium.org/2361283002
TEST=covered by blimp_unittests, app_shell_unittests, 
     content_unittests, unit_tests, mash_unittests,
     extensions_browsertests
==========

to

==========
Change single-display screens to use ScreenBase and DisplayList.

Reduce similar code in single-display screens.

1. ScreenBase doesn't set itself as the screen instance in
display::Screen anymore, rather its subclass needs to decide if 
it wants to be the screen instance.

2. Moved Displays::iterator FindDisplayById(int64_t id); to private
in DisplayList.

3. Fixed BlimpScreenTest since Display::SetScaleAndBounds 
also updates the work area of a display.

4. Fixed RenderWidgetHostViewAuraTest.PhysicalBackingSizeWithScale
since added UpdateDisplay call in aura::TestScreen::SetDeviceScaleFactor.

BUG=none, related to https://codereview.chromium.org/2361283002
TEST=covered by blimp_unittests, app_shell_unittests, 
     content_unittests, unit_tests, mash_unittests,
     extensions_browsertests
==========

[commit-bot: I haz the power, 2016-11-07 17:58:16]

Committed patchset #11 (id:380001)

[commit-bot: I haz the power, 2016-11-07 18:14:32]

Description was changed from

==========
Change single-display screens to use ScreenBase and DisplayList.

Reduce similar code in single-display screens.

1. ScreenBase doesn't set itself as the screen instance in
display::Screen anymore, rather its subclass needs to decide if 
it wants to be the screen instance.

2. Moved Displays::iterator FindDisplayById(int64_t id); to private
in DisplayList.

3. Fixed BlimpScreenTest since Display::SetScaleAndBounds 
also updates the work area of a display.

4. Fixed RenderWidgetHostViewAuraTest.PhysicalBackingSizeWithScale
since added UpdateDisplay call in aura::TestScreen::SetDeviceScaleFactor.

BUG=none, related to https://codereview.chromium.org/2361283002
TEST=covered by blimp_unittests, app_shell_unittests, 
     content_unittests, unit_tests, mash_unittests,
     extensions_browsertests
==========

to

==========
Change single-display screens to use ScreenBase and DisplayList.

Reduce similar code in single-display screens.

1. ScreenBase doesn't set itself as the screen instance in
display::Screen anymore, rather its subclass needs to decide if
it wants to be the screen instance.

2. Moved Displays::iterator FindDisplayById(int64_t id); to private
in DisplayList.

3. Fixed BlimpScreenTest since Display::SetScaleAndBounds
also updates the work area of a display.

4. Fixed RenderWidgetHostViewAuraTest.PhysicalBackingSizeWithScale
since added UpdateDisplay call in aura::TestScreen::SetDeviceScaleFactor.

BUG=none, related to https://codereview.chromium.org/2361283002
TEST=covered by blimp_unittests, app_shell_unittests,
     content_unittests, unit_tests, mash_unittests,
     extensions_browsertests

Committed: https://crrev.com/bcbb8c44d3e5d2bdc1da14867c3f05527e349d8b
Cr-Commit-Position: refs/heads/master@{#430312}
==========

[commit-bot: I haz the power, 2016-11-07 18:14:34]

Patchset 11 (id:??) landed as
https://crrev.com/bcbb8c44d3e5d2bdc1da14867c3f05527e349d8b
Cr-Commit-Position: refs/heads/master@{#430312}