Extract MonitorInfoProvider from WindowSizer.

Extract MonitorInfoProvider from WindowSizer.

And move the functionality into gfx::Screen.

BUG=101507
R=ben@chromium.org

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=113531

[Ben Goodger (Google), 2011-11-18 18:14:26]

No more code reviews on this random stuff until we finish the views ->
ui/views move!

I don't like leaving things in a mixed/inconsistent state for too long.

Thanks,

-Ben

On Fri, Nov 18, 2011 at 10:12 AM, <tfarina@chromium.org> wrote:

> Reviewers: Ben Goodger (Google),
>
> Description:
> Extract MonitorInfoProvider from WindowSizer.
>
> R=ben@chromium.org
>
>
> Please review this at
http://codereview.chromium.**org/8549008/<http://codereview.chromium.org/8549...
>
> SVN Base:
svn://svn.chromium.org/chrome/**trunk/src<http://svn.chromium.org/chrome/trunk/src>
>
> Affected files:
>  M chrome/browser/chromeos/**notifications/balloon_**collection_impl.cc
>  M chrome/browser/notifications/**balloon_collection_impl.cc
>  M chrome/browser/pdf_**browsertest.cc
>  M chrome/browser/ui/gtk/browser_**window_gtk.cc
>  A chrome/browser/ui/monitor_**info_provider.h
>  A chrome/browser/ui/monitor_**info_provider.cc
>  A chrome/browser/ui/monitor_**info_provider_aura.cc
>  A chrome/browser/ui/monitor_**info_provider_gtk.cc
>  A
chrome/browser/ui/monitor_**info_provider_mac.mm<http://monitor_info_provider_mac.mm>
>  A chrome/browser/ui/monitor_**info_provider_win.cc
>  M chrome/browser/ui/panels/**panel_manager.cc
>  M chrome/browser/ui/views/**bubble/border_contents.cc
>  M chrome/browser/ui/views/**chrome_views_delegate.cc
>  M chrome/browser/ui/views/**constrained_window_views.cc
>  M chrome/browser/ui/window_**sizer.h
>  M chrome/browser/ui/window_**sizer.cc
>  M chrome/browser/ui/window_**sizer_aura.cc
>  M chrome/browser/ui/window_**sizer_gtk.cc
>  M chrome/browser/ui/window_**sizer_mac.mm <http://window_sizer_mac.mm>
>  M chrome/browser/ui/window_**sizer_unittest.cc
>  M chrome/browser/ui/window_**sizer_win.cc
>  M chrome/chrome_browser.gypi
>
>
>

[tfarina, 2011-12-02 11:47:44]

On 2011/11/18 18:14:26, Ben Goodger (Google) wrote:
> No more code reviews on this random stuff until we finish the views ->
> ui/views move!
> 
> I don't like leaving things in a mixed/inconsistent state for too long.
> 
> Thanks,
> 
Done.

Please, could you review patch set 2?

Then I will start on crbug.com/105879

[Ben Goodger (Google), 2011-12-02 17:10:32]

http://codereview.chromium.org/8549008/diff/22001/chrome/browser/ui/monitor_i...
File chrome/browser/ui/monitor_info_provider.h (right):

http://codereview.chromium.org/8549008/diff/22001/chrome/browser/ui/monitor_i...
chrome/browser/ui/monitor_info_provider.h:18: class MonitorInfoProvider {
So, I actually think we can get rid of this class completely. It is mostly
redundant with gfx::Screen (ui/gfx/screen). Those areas where it's not, are
either unused or should just be merged onto gfx::Screen.

http://codereview.chromium.org/8549008/diff/22001/chrome/browser/ui/monitor_i...
chrome/browser/ui/monitor_info_provider.h:41: virtual void UpdateWorkAreas() =
0;
As far as I can tell, this function is never called.

http://codereview.chromium.org/8549008/diff/22001/chrome/browser/ui/monitor_i...
chrome/browser/ui/monitor_info_provider.h:44: size_t GetMonitorCount() const {
return work_areas_.size(); }
This function is only called from a function in WindowSizer that is unused.

http://codereview.chromium.org/8549008/diff/22001/chrome/browser/ui/monitor_i...
chrome/browser/ui/monitor_info_provider.h:48: gfx::Rect GetWorkAreaAt(size_t
index) const {
This function is only called from a function in WindowSizer that is unused.

http://codereview.chromium.org/8549008/diff/22001/chrome/browser/ui/window_si...
File chrome/browser/ui/window_sizer.cc (right):

http://codereview.chromium.org/8549008/diff/22001/chrome/browser/ui/window_si...
chrome/browser/ui/window_sizer.cc:200: bool WindowSizer::PositionIsOffscreen(int
position, Edge edge) const {
No one calls this function in Chrome. It can just be removed.

[tfarina, 2011-12-02 17:24:43]

http://codereview.chromium.org/8549008/diff/22001/chrome/browser/ui/monitor_i...
File chrome/browser/ui/monitor_info_provider.h (right):

http://codereview.chromium.org/8549008/diff/22001/chrome/browser/ui/monitor_i...
chrome/browser/ui/monitor_info_provider.h:18: class MonitorInfoProvider {
On 2011/12/02 17:10:32, Ben Goodger (Google) wrote:
> So, I actually think we can get rid of this class completely. It is mostly
> redundant with gfx::Screen (ui/gfx/screen). Those areas where it's not, are
> either unused or should just be merged onto gfx::Screen.

This is crbug.com/101507

http://codereview.chromium.org/8549008/diff/22001/chrome/browser/ui/monitor_i...
chrome/browser/ui/monitor_info_provider.h:41: virtual void UpdateWorkAreas() =
0;
On 2011/12/02 17:10:32, Ben Goodger (Google) wrote:
> As far as I can tell, this function is never called.
Removed.

http://codereview.chromium.org/8549008/diff/22001/chrome/browser/ui/monitor_i...
chrome/browser/ui/monitor_info_provider.h:44: size_t GetMonitorCount() const {
return work_areas_.size(); }
On 2011/12/02 17:10:32, Ben Goodger (Google) wrote:
> This function is only called from a function in WindowSizer that is unused.
Removed.

http://codereview.chromium.org/8549008/diff/22001/chrome/browser/ui/monitor_i...
chrome/browser/ui/monitor_info_provider.h:48: gfx::Rect GetWorkAreaAt(size_t
index) const {
On 2011/12/02 17:10:32, Ben Goodger (Google) wrote:
> This function is only called from a function in WindowSizer that is unused.
Removed.

http://codereview.chromium.org/8549008/diff/22001/chrome/browser/ui/window_si...
File chrome/browser/ui/window_sizer.cc (right):

http://codereview.chromium.org/8549008/diff/22001/chrome/browser/ui/window_si...
chrome/browser/ui/window_sizer.cc:200: bool WindowSizer::PositionIsOffscreen(int
position, Edge edge) const {
On 2011/12/02 17:10:32, Ben Goodger (Google) wrote:
> No one calls this function in Chrome. It can just be removed.

Removed.

[Ben Goodger (Google), 2011-12-02 17:29:13]

On Fri, Dec 2, 2011 at 9:24 AM, <tfarina@chromium.org> wrote:

> On 2011/12/02 17:10:32, Ben Goodger (Google) wrote:
>
>> So, I actually think we can get rid of this class completely. It is
>>
> mostly
>
>> redundant with gfx::Screen (ui/gfx/screen). Those areas where it's
>>
> not, are
>
>> either unused or should just be merged onto gfx::Screen.
>>
>
> This is crbug.com/101507


Yes, and I think that rather than refactoring all this and introducing more
files you could just fix that bug instead. (Along with removing the unused
WindowSizer functions mentioned here).

-Ben

[tfarina, 2011-12-02 18:26:45]

On 2011/12/02 17:29:13, Ben Goodger (Google) wrote:
> On Fri, Dec 2, 2011 at 9:24 AM, <mailto:tfarina@chromium.org> wrote:
> 
> > On 2011/12/02 17:10:32, Ben Goodger (Google) wrote:
> >
> >> So, I actually think we can get rid of this class completely. It is
> >>
> > mostly
> >
> >> redundant with gfx::Screen (ui/gfx/screen). Those areas where it's
> >>
> > not, are
> >
> >> either unused or should just be merged onto gfx::Screen.
> >>
> >
> > This is crbug.com/101507
> 
> 
> Yes, and I think that rather than refactoring all this and introducing more
> files you could just fix that bug instead. (Along with removing the unused
> WindowSizer functions mentioned here).
> 
I think I crank that. Please, take a look at patch set 13.

Thanks!

[Ben Goodger (Google), 2011-12-02 19:05:36]

LGTM once you exclude the DEPS file change.

http://codereview.chromium.org/8549008/diff/25047/DEPS
File DEPS (right):

http://codereview.chromium.org/8549008/diff/25047/DEPS#newcode41
DEPS:41: "v8_revision": "10136",
did you mean to change this file?

[tfarina, 2011-12-02 19:31:07]

On 2011/12/02 19:05:36, Ben Goodger (Google) wrote:
> LGTM once you exclude the DEPS file change.
> 
> http://codereview.chromium.org/8549008/diff/25047/DEPS
> File DEPS (right):
> 
> http://codereview.chromium.org/8549008/diff/25047/DEPS#newcode41
> DEPS:41: "v8_revision": "10136",
> did you mean to change this file?

D'oh not really, that was caused by a bad merge resolution. It should be fixed
now in patch set 4.

[Ben Goodger (Google), 2011-12-02 20:30:14]

lgtm

[tfarina, 2011-12-02 21:22:14]

On 2011/12/02 20:30:14, Ben Goodger (Google) wrote:
> lgtm

Ben, what I do about the window_sizer_unittest? They aren't passing because now
we don't fake the monitor layouts and sizes as we were doing (allowed by the
class inheritance).

Please, could you take a look at chrome/browser/ui/window_sizer_unittest.cc and
the tests using GetWindowBounds()?

[tfarina, 2011-12-06 01:01:34]

On 2011/12/02 21:22:14, tfarina wrote:
> On 2011/12/02 20:30:14, Ben Goodger (Google) wrote:
> > lgtm
> 
> Ben, what I do about the window_sizer_unittest? They aren't passing because
now
> we don't fake the monitor layouts and sizes as we were doing (allowed by the
> class inheritance).
> 
> Please, could you take a look at chrome/browser/ui/window_sizer_unittest.cc
and
> the tests using GetWindowBounds()?

ping??

[Ben Goodger (Google), 2011-12-06 16:38:10]

Maybe you still need something like MIP... but implemented in terms of
Screen for real Chrome, but implemented by a mock like the test does for
these tests.

I would leave MIP defined inside WindowSizer in this case, since it's
really only used for WindowSizer tests.

I would create the default impl inside window_sizer.cc that uses
gfx::Screen. When you construct WindowSizer without a MIP impl, you get
this default one. Then add a second WindowSizer ctor that takes a MIP impl
to use in place of the default one, and the tests can then specify that one.

-Ben

On Mon, Dec 5, 2011 at 5:01 PM, <tfarina@chromium.org> wrote:

> On 2011/12/02 21:22:14, tfarina wrote:
>
>> On 2011/12/02 20:30:14, Ben Goodger (Google) wrote:
>> > lgtm
>>
>
>  Ben, what I do about the window_sizer_unittest? They aren't passing
>> because
>>
> now
>
>> we don't fake the monitor layouts and sizes as we were doing (allowed by
>> the
>> class inheritance).
>>
>
>  Please, could you take a look at chrome/browser/ui/window_**
>> sizer_unittest.cc
>>
> and
>
>> the tests using GetWindowBounds()?
>>
>
> ping??
>
>
http://codereview.chromium.**org/8549008/<http://codereview.chromium.org/8549...
>

[tfarina, 2011-12-06 18:11:17]

On 2011/12/06 16:38:10, Ben Goodger (Google) wrote:
> Maybe you still need something like MIP... but implemented in terms of
> Screen for real Chrome, but implemented by a mock like the test does for
> these tests.
> 
> I would leave MIP defined inside WindowSizer in this case, since it's
> really only used for WindowSizer tests.
> 
> I would create the default impl inside window_sizer.cc that uses
> gfx::Screen. When you construct WindowSizer without a MIP impl, you get
> this default one. Then add a second WindowSizer ctor that takes a MIP impl
> to use in place of the default one, and the tests can then specify that one.
> 
Updated per your suggestion.

Please, review patch set 19.

Thanks!

[Ben Goodger (Google), 2011-12-07 17:17:34]

lgtm

http://codereview.chromium.org/8549008/diff/32002/chrome/browser/ui/window_si...
File chrome/browser/ui/window_sizer.h (right):

http://codereview.chromium.org/8549008/diff/32002/chrome/browser/ui/window_si...
chrome/browser/ui/window_sizer.h:48: // WindowSizer owns |state_provider| and
|monitor_info_provider|.
Also add this comment for the first function:

WindowSizer will create a default MonitorInfoProvider using the physical screen.

And for the second:

WindowSizer will use the supplied monitor info provider. Used for testing.