Make DisplayInfoProvider an interface

Make DisplayInfoProvider an interface

Now is up to the component embedding the SystemInfo API to implement
the DisplayInfoProvider. The motivation for this change is we want to
move the SystemInfo API to extensions/ but DisplayInfoProvider depends
on ash/, which will link with ui/views/, and that is way too big.

The ultimate goal is to have SystemInfo on the app_shell without increasing its
footprint considerably.

BUG=392842
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=291250

[tmpsantos, 2014-08-15 15:46:19]

This patch relates to https://codereview.chromium.org/389633002

I'm isolating the DisplayInfoProvider implementation to not pull the ash/
dependency to extensions/ when moving the SystemInfo API to there.

Please review.

[James Cook, 2014-08-15 19:58:42]

Thanks for taking on this refactoring!

This per-platform subclassing pattern is unusual in the code base and I'd like
to see it changed.  (It was odd before, and I'd like to avoid introducing the
DisplayInfoProviderImpl layer.)

I suggest doing something similar to what we do with BrowserNonClientFrameView*
in c/b/ui/views/frame:

DisplayInfoProvider gains pure-virtual methods like you've done here
DislayInfoProvider maintains Get/SetForTesting/etc.
The per-platform implementations are named subclasses:
DisplayInfoProviderChromeos
DisplayInfoProviderWin
DisplayInfoProviderMac
and these match the filenames.

There is a single per-platform factory function (either an
extensions::CreateDisplayInfoProvider() or DisplayInfoProvider::Create()) that
has an implementation in each of the per-platform .cc files.

That makes the filenames match the class names, requires only a few lines of
platform-specific factory, and is more consistent with the rest of chrome.

https://codereview.chromium.org/476103002/diff/20001/chrome/browser/extension...
File chrome/browser/extensions/api/system_display/display_info_provider.cc
(right):

https://codereview.chromium.org/476103002/diff/20001/chrome/browser/extension...
chrome/browser/extensions/api/system_display/display_info_provider.cc:4: 
Shouldn't this include display_info_provider.h?

https://codereview.chromium.org/476103002/diff/20001/chrome/browser/extension...
File chrome/browser/extensions/api/system_display/display_info_provider.h
(right):

https://codereview.chromium.org/476103002/diff/20001/chrome/browser/extension...
chrome/browser/extensions/api/system_display/display_info_provider.h:10: 
nit: #include "base/macros.h" for DISALLOW_COPY_AND_ASSIGN

https://codereview.chromium.org/476103002/diff/20001/chrome/browser/extension...
chrome/browser/extensions/api/system_display/display_info_provider.h:11:
#include "chrome/common/extensions/api/system_display.h"
nit: Can you forward declare api::system_display::DisplayProperties and
DisplayUnitInfo to avoid this include?

https://codereview.chromium.org/476103002/diff/20001/chrome/browser/extension...
chrome/browser/extensions/api/system_display/display_info_provider.h:34: virtual
bool SetInfo(const std::string& display_id,
Please document this method.

https://codereview.chromium.org/476103002/diff/20001/chrome/browser/extension...
File chrome/browser/extensions/display_info_provider_impl.h (right):

https://codereview.chromium.org/476103002/diff/20001/chrome/browser/extension...
chrome/browser/extensions/display_info_provider_impl.h:13: public :
no space after "public"

https://codereview.chromium.org/476103002/diff/20001/chrome/browser/extension...
chrome/browser/extensions/display_info_provider_impl.h:26: };
DISALLOW_COPY_AND_ASSIGN

https://codereview.chromium.org/476103002/diff/20001/chrome/browser/extension...
File chrome/browser/extensions/display_info_provider_impl_chromeos.cc (right):

https://codereview.chromium.org/476103002/diff/20001/chrome/browser/extension...
chrome/browser/extensions/display_info_provider_impl_chromeos.cc:357: const
DisplayProperties& info,
please run "git cl format" on patches -- it'll fix indentation like this

[tmpsantos, 2014-08-19 21:33:09]

On 2014/08/15 19:58:42, James Cook wrote:
> Thanks for taking on this refactoring!
> 
> This per-platform subclassing pattern is unusual in the code base and I'd like
> to see it changed.  (It was odd before, and I'd like to avoid introducing the
> DisplayInfoProviderImpl layer.)
> 
> I suggest doing something similar to what we do with
BrowserNonClientFrameView*
> in c/b/ui/views/frame:
> 
> DisplayInfoProvider gains pure-virtual methods like you've done here
> DislayInfoProvider maintains Get/SetForTesting/etc.
> The per-platform implementations are named subclasses:
> DisplayInfoProviderChromeos
> DisplayInfoProviderWin
> DisplayInfoProviderMac
> and these match the filenames.
> 
> There is a single per-platform factory function (either an
> extensions::CreateDisplayInfoProvider() or DisplayInfoProvider::Create()) that
> has an implementation in each of the per-platform .cc files.
> 
> That makes the filenames match the class names, requires only a few lines of
> platform-specific factory, and is more consistent with the rest of chrome.
> 
>
https://codereview.chromium.org/476103002/diff/20001/chrome/browser/extension...
> File chrome/browser/extensions/api/system_display/display_info_provider.cc
> (right):
> 
>
https://codereview.chromium.org/476103002/diff/20001/chrome/browser/extension...
> chrome/browser/extensions/api/system_display/display_info_provider.cc:4: 
> Shouldn't this include display_info_provider.h?
> 
>
https://codereview.chromium.org/476103002/diff/20001/chrome/browser/extension...
> File chrome/browser/extensions/api/system_display/display_info_provider.h
> (right):
> 
>
https://codereview.chromium.org/476103002/diff/20001/chrome/browser/extension...
> chrome/browser/extensions/api/system_display/display_info_provider.h:10: 
> nit: #include "base/macros.h" for DISALLOW_COPY_AND_ASSIGN
> 
>
https://codereview.chromium.org/476103002/diff/20001/chrome/browser/extension...
> chrome/browser/extensions/api/system_display/display_info_provider.h:11:
> #include "chrome/common/extensions/api/system_display.h"
> nit: Can you forward declare api::system_display::DisplayProperties and
> DisplayUnitInfo to avoid this include?
> 
>
https://codereview.chromium.org/476103002/diff/20001/chrome/browser/extension...
> chrome/browser/extensions/api/system_display/display_info_provider.h:34:
virtual
> bool SetInfo(const std::string& display_id,
> Please document this method.
> 
>
https://codereview.chromium.org/476103002/diff/20001/chrome/browser/extension...
> File chrome/browser/extensions/display_info_provider_impl.h (right):
> 
>
https://codereview.chromium.org/476103002/diff/20001/chrome/browser/extension...
> chrome/browser/extensions/display_info_provider_impl.h:13: public :
> no space after "public"
> 
>
https://codereview.chromium.org/476103002/diff/20001/chrome/browser/extension...
> chrome/browser/extensions/display_info_provider_impl.h:26: };
> DISALLOW_COPY_AND_ASSIGN
> 
>
https://codereview.chromium.org/476103002/diff/20001/chrome/browser/extension...
> File chrome/browser/extensions/display_info_provider_impl_chromeos.cc (right):
> 
>
https://codereview.chromium.org/476103002/diff/20001/chrome/browser/extension...
> chrome/browser/extensions/display_info_provider_impl_chromeos.cc:357: const
> DisplayProperties& info,
> please run "git cl format" on patches -- it'll fix indentation like this

Thanks for reviewing! All the issues were addressed on this new patch.

[James Cook, 2014-08-19 21:48:52]

Looking good. A few nits.

https://codereview.chromium.org/476103002/diff/80001/chrome/browser/extension...
File chrome/browser/extensions/api/system_display/display_info_provider.cc
(right):

https://codereview.chromium.org/476103002/diff/80001/chrome/browser/extension...
chrome/browser/extensions/api/system_display/display_info_provider.cc:16:
DisplayInfoProvider* g_display_info_provider = NULL;
Thanks for moving this.

https://codereview.chromium.org/476103002/diff/80001/chrome/browser/extension...
File chrome/browser/extensions/api/system_display/display_info_provider.h
(right):

https://codereview.chromium.org/476103002/diff/80001/chrome/browser/extension...
chrome/browser/extensions/api/system_display/display_info_provider.h:35: // or
InitializeForTesting() or not called yet.
This isn't true -- it creates a new one if it doesn't exist already. Either you
should require other parts of Chrome to call Create() first (I would be OK with
that) or fix the comment.

https://codereview.chromium.org/476103002/diff/80001/chrome/browser/extension...
File chrome/browser/extensions/display_info_provider_aura.h (right):

https://codereview.chromium.org/476103002/diff/80001/chrome/browser/extension...
chrome/browser/extensions/display_info_provider_aura.h:19: virtual bool
SetInfo(const std::string& display_id,
nit: put a comment like "// DisplayInfoProvider overrides:" here

https://codereview.chromium.org/476103002/diff/80001/chrome/browser/extension...
chrome/browser/extensions/display_info_provider_aura.h:22: 
nit: no blank line

https://codereview.chromium.org/476103002/diff/80001/chrome/browser/extension...
File chrome/browser/extensions/display_info_provider_chromeos.h (right):

https://codereview.chromium.org/476103002/diff/80001/chrome/browser/extension...
chrome/browser/extensions/display_info_provider_chromeos.h:8: #include <string>
nit: you don't need this, generally for types that are included as part of an
interface we don't re-include the header

https://codereview.chromium.org/476103002/diff/80001/chrome/browser/extension...
File chrome/browser/extensions/display_info_provider_mac.cc (right):

https://codereview.chromium.org/476103002/diff/80001/chrome/browser/extension...
chrome/browser/extensions/display_info_provider_mac.cc:1: // Copyright 2013 The
Chromium Authors. All rights reserved.
Can you try uploading with --similarity=50 (or 30 or lower) to see if this can
be recognized as a rename instead of a file add? That way it will preserve the
history (and your name will be on fewer lines in git blame :-).

https://codereview.chromium.org/476103002/diff/80001/chrome/browser/extension...
File chrome/browser/extensions/display_info_provider_mac.h (right):

https://codereview.chromium.org/476103002/diff/80001/chrome/browser/extension...
chrome/browser/extensions/display_info_provider_mac.h:8: #include <string>
ditto

https://codereview.chromium.org/476103002/diff/80001/chrome/browser/extension...
chrome/browser/extensions/display_info_provider_mac.h:19: virtual bool
SetInfo(const std::string& display_id,
// DisplayInfoProvider overrides:

https://codereview.chromium.org/476103002/diff/80001/chrome/browser/extension...
chrome/browser/extensions/display_info_provider_mac.h:22: 
nit: no blank line

https://codereview.chromium.org/476103002/diff/80001/chrome/browser/extension...
File chrome/browser/extensions/display_info_provider_win.h (right):

https://codereview.chromium.org/476103002/diff/80001/chrome/browser/extension...
chrome/browser/extensions/display_info_provider_win.h:8: #include <string>
ditto, and ditto below

[tmpsantos, 2014-08-20 18:58:32]

https://codereview.chromium.org/476103002/diff/80001/chrome/browser/extension...
File chrome/browser/extensions/display_info_provider_aura.h (right):

https://codereview.chromium.org/476103002/diff/80001/chrome/browser/extension...
chrome/browser/extensions/display_info_provider_aura.h:19: virtual bool
SetInfo(const std::string& display_id,
On 2014/08/19 21:48:51, James Cook wrote:
> nit: put a comment like "// DisplayInfoProvider overrides:" here

Done on all platform specific headers.

https://codereview.chromium.org/476103002/diff/80001/chrome/browser/extension...
chrome/browser/extensions/display_info_provider_aura.h:22: 
On 2014/08/19 21:48:51, James Cook wrote:
> nit: no blank line

Done on all platform specific headers.

https://codereview.chromium.org/476103002/diff/80001/chrome/browser/extension...
File chrome/browser/extensions/display_info_provider_chromeos.h (right):

https://codereview.chromium.org/476103002/diff/80001/chrome/browser/extension...
chrome/browser/extensions/display_info_provider_chromeos.h:8: #include <string>
On 2014/08/19 21:48:52, James Cook wrote:
> nit: you don't need this, generally for types that are included as part of an
> interface we don't re-include the header

I originally wrote without it, but "git cl lint" was complaining.

chrome/browser/extensions/display_info_provider_chromeos.h:20:  Add #include
<string> for string  [build/include_what_you_use] [4]

I'm assuming as a false positive and removing this from all the platform
specific hearders.

https://codereview.chromium.org/476103002/diff/80001/chrome/browser/extension...
File chrome/browser/extensions/display_info_provider_mac.cc (right):

https://codereview.chromium.org/476103002/diff/80001/chrome/browser/extension...
chrome/browser/extensions/display_info_provider_mac.cc:1: // Copyright 2013 The
Chromium Authors. All rights reserved.
On 2014/08/19 21:48:52, James Cook wrote:
> Can you try uploading with --similarity=50 (or 30 or lower) to see if this can
> be recognized as a rename instead of a file add? That way it will preserve the
> history (and your name will be on fewer lines in git blame :-).

Did it with 30 and apparently it worked (50 is the default). I don't wanna take
the blame for anything. :)

[Lei Zhang, 2014-08-20 19:51:20]

lgtm assuming James approves.

https://codereview.chromium.org/476103002/diff/120001/chrome/browser/extensio...
File chrome/browser/extensions/display_info_provider_chromeos_unittest.cc
(right):

https://codereview.chromium.org/476103002/diff/120001/chrome/browser/extensio...
chrome/browser/extensions/display_info_provider_chromeos_unittest.cc:56: return
base::StringPrintf(
Just use Insets::ToString() here?

[James Cook, 2014-08-20 19:57:59]

LGTM. Thanks again for refactoring this!

https://codereview.chromium.org/476103002/diff/120001/chrome/browser/extensio...
File chrome/browser/extensions/api/system_display/display_info_provider.cc
(right):

https://codereview.chromium.org/476103002/diff/120001/chrome/browser/extensio...
chrome/browser/extensions/api/system_display/display_info_provider.cc:16:
DisplayInfoProvider* g_display_info_provider = NULL;
nit: I presume this is leaked intentionally.  If so, you might comment that
here.

https://codereview.chromium.org/476103002/diff/120001/chrome/browser/extensio...
File chrome/browser/extensions/display_info_provider_aura.cc (right):

https://codereview.chromium.org/476103002/diff/120001/chrome/browser/extensio...
chrome/browser/extensions/display_info_provider_aura.cc:19: *error = "Not
implemented";
nice attention to detail

[tmpsantos, 2014-08-21 13:31:56]

Thanks all for reviewing. Patch rebased and I will cq+ as soon as the bots are
happy about it.

https://codereview.chromium.org/476103002/diff/120001/chrome/browser/extensio...
File chrome/browser/extensions/api/system_display/display_info_provider.cc
(right):

https://codereview.chromium.org/476103002/diff/120001/chrome/browser/extensio...
chrome/browser/extensions/api/system_display/display_info_provider.cc:16:
DisplayInfoProvider* g_display_info_provider = NULL;
On 2014/08/20 19:57:59, James Cook wrote:
> nit: I presume this is leaked intentionally.  If so, you might comment that
> here.

Done.

https://codereview.chromium.org/476103002/diff/120001/chrome/browser/extensio...
File chrome/browser/extensions/display_info_provider_chromeos_unittest.cc
(right):

https://codereview.chromium.org/476103002/diff/120001/chrome/browser/extensio...
chrome/browser/extensions/display_info_provider_chromeos_unittest.cc:56: return
base::StringPrintf(
On 2014/08/20 19:51:20, Lei Zhang wrote:
> Just use Insets::ToString() here?

The "insets" variable here is not a "gfx::Insets", but a
"api::system_display::Insets", which is a struct generated by the idl/schema
compiler. I think the test converts it to the same output you would get from
"gfx::Insets::ToString()" to make it more readable for people familiar with
"gfx::Insets". Like in:

EXPECT_EQ("20,30,50,60", SystemInfoDisplayInsetsToString(result[1]->overscan));

[tmpsantos, 2014-08-21 18:34:36]

The CQ bit was checked by thiago.santos@intel.com

[commit-bot: I haz the power, 2014-08-21 18:38:26]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/thiago.santos@intel.com/476103002/140001

[commit-bot: I haz the power, 2014-08-21 23:49:03]

Committed patchset #5 (140001) as 291250