Don't bother asking the platform for the delegates if they are already set

Don't bother asking the platform for the delegates if they are already set

For Ozone, there is no default platform. Since the unittests use their own
delegates this simplifies things by not requiring an Ozone platform when
executing the unittests.

BUG=none

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

[dnicoara, 2014-07-09 17:20:03]

PTAL

[Daniel Erat, 2014-07-09 18:11:24]

https://codereview.chromium.org/383463002/diff/1/ui/display/chromeos/display_...
File ui/display/chromeos/display_configurator.cc (right):

https://codereview.chromium.org/383463002/diff/1/ui/display/chromeos/display_...
ui/display/chromeos/display_configurator.cc:185: PlatformInitialize();
i'm concerned that someone further down the line will make PlatformInitialize()
do additional non-delegate initialization that'll then get skipped when the
delegates have been set earlier. could you refactor things to make this less
likely to happen by e.g. calling PlatformInitialize() unconditionally but
passing a |create_delegates| argument to it?

[dnicoara, 2014-07-09 19:24:37]

https://codereview.chromium.org/383463002/diff/1/ui/display/chromeos/display_...
File ui/display/chromeos/display_configurator.cc (right):

https://codereview.chromium.org/383463002/diff/1/ui/display/chromeos/display_...
ui/display/chromeos/display_configurator.cc:185: PlatformInitialize();
On 2014/07/09 18:11:24, Daniel Erat wrote:
> i'm concerned that someone further down the line will make
PlatformInitialize()
> do additional non-delegate initialization that'll then get skipped when the
> delegates have been set earlier. could you refactor things to make this less
> likely to happen by e.g. calling PlatformInitialize() unconditionally but
> passing a |create_delegates| argument to it?

Hmm, I don't really want to add too much complexity around PlatformInitialize()
since it's really only used to create the platform delegates. What's your
opinion on dropping the PlatformInitialize() function and adding
CreatePlatformNativeDisplayDelegate() and CreatePlatformTouchscreenDelegate()?

[Daniel Erat, 2014-07-09 19:45:52]

On 2014/07/09 19:24:37, dnicoara wrote:
>
https://codereview.chromium.org/383463002/diff/1/ui/display/chromeos/display_...
> File ui/display/chromeos/display_configurator.cc (right):
> 
>
https://codereview.chromium.org/383463002/diff/1/ui/display/chromeos/display_...
> ui/display/chromeos/display_configurator.cc:185: PlatformInitialize();
> On 2014/07/09 18:11:24, Daniel Erat wrote:
> > i'm concerned that someone further down the line will make
> PlatformInitialize()
> > do additional non-delegate initialization that'll then get skipped when the
> > delegates have been set earlier. could you refactor things to make this less
> > likely to happen by e.g. calling PlatformInitialize() unconditionally but
> > passing a |create_delegates| argument to it?
> 
> Hmm, I don't really want to add too much complexity around
PlatformInitialize()
> since it's really only used to create the platform delegates. What's your
> opinion on dropping the PlatformInitialize() function and adding
> CreatePlatformNativeDisplayDelegate() and CreatePlatformTouchscreenDelegate()?

sgtm (or just a single method called CreatePlatformDelegates() if you'll always
be setting either both or neither of them)

[dnicoara, 2014-07-09 21:05:27]

On 2014/07/09 19:45:52, Daniel Erat wrote:
> On 2014/07/09 19:24:37, dnicoara wrote:
> >
>
https://codereview.chromium.org/383463002/diff/1/ui/display/chromeos/display_...
> > File ui/display/chromeos/display_configurator.cc (right):
> > 
> >
>
https://codereview.chromium.org/383463002/diff/1/ui/display/chromeos/display_...
> > ui/display/chromeos/display_configurator.cc:185: PlatformInitialize();
> > On 2014/07/09 18:11:24, Daniel Erat wrote:
> > > i'm concerned that someone further down the line will make
> > PlatformInitialize()
> > > do additional non-delegate initialization that'll then get skipped when
the
> > > delegates have been set earlier. could you refactor things to make this
less
> > > likely to happen by e.g. calling PlatformInitialize() unconditionally but
> > > passing a |create_delegates| argument to it?
> > 
> > Hmm, I don't really want to add too much complexity around
> PlatformInitialize()
> > since it's really only used to create the platform delegates. What's your
> > opinion on dropping the PlatformInitialize() function and adding
> > CreatePlatformNativeDisplayDelegate() and
CreatePlatformTouchscreenDelegate()?
> 
> sgtm (or just a single method called CreatePlatformDelegates() if you'll
always
> be setting either both or neither of them)

Done. I've left them separate since I'm planning on removing the
TouchscreenDelegate so there will be only one delegate then.

[Daniel Erat, 2014-07-09 21:17:23]

lgtm

https://codereview.chromium.org/383463002/diff/20001/ui/display/chromeos/disp...
File ui/display/chromeos/display_configurator.cc (right):

https://codereview.chromium.org/383463002/diff/20001/ui/display/chromeos/disp...
ui/display/chromeos/display_configurator.cc:186: native_display_delegate_ =
CreatePlatformNativeDisplayDelegate();
nit: probably safer to do this now:

  if (!native...)
    native... = ...;
  if (!touchscreen...)
    touchscreen... = ....;

[dnicoara, 2014-07-09 21:21:50]

Thanks!

https://codereview.chromium.org/383463002/diff/20001/ui/display/chromeos/disp...
File ui/display/chromeos/display_configurator.cc (right):

https://codereview.chromium.org/383463002/diff/20001/ui/display/chromeos/disp...
ui/display/chromeos/display_configurator.cc:186: native_display_delegate_ =
CreatePlatformNativeDisplayDelegate();
On 2014/07/09 21:17:23, Daniel Erat wrote:
> nit: probably safer to do this now:
> 
>   if (!native...)
>     native... = ...;
>   if (!touchscreen...)
>     touchscreen... = ....;

Done.

[dnicoara, 2014-07-09 21:21:59]

The CQ bit was checked by dnicoara@chromium.org

[commit-bot: I haz the power, 2014-07-09 21:24:06]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/dnicoara@chromium.org/383463002/40001

[commit-bot: I haz the power, 2014-07-10 00:24:09]

Change committed as 282188