Refactor BrowserProcessPlatformPart.

Refactor BrowserProcessPlatformPart.

The previous design was to have a class, BrowserProcessPlatformPart, which has various implementations for specific platforms and a common implementation for all other platforms. These implementations (e.g., browser_process_platform_part_(chromeos|aurawin).h) both implement the common methods defined in the common implementation (browser_process_platform_part.h) and some methods specific to their platform.

The main problem with this design is that each custom implementation needs to re-implement the default behavior even for the methods for which the base implementation would have been fine (since it's a compile-time choice and doesn't use inheritance).

This will become worst as more common methods get added to the base BrowserProcessPlatformPart, forcing every custom implementation to provide a copy of the default implementation for that method...

The previous design also forces ugly #if ... #elif ... #elif ... #else ...#endif style of includes just to use the common methods since all headers, but one, are discarded by gyp.

This will become worst as more platforms need custom BrowserProcessPlatformPart.


Both scenarios making this worst are happening as part of addressing https://codereview.chromium.org/14576015/diff/43017/chrome/browser/lifetime/application_lifetime.cc#newcode116 where adding a common call to BrowserProcessPlatformPart and using it to get rid of the ifdefs there implies one more common call and 2 more platforms.


With the new design:
There is a common base class which platform-specific implementation override as desired.

The #ifdef include-mess is dealt with by having a meta-include in browser_process_platform_part.h which knows which impl to include at compile-time.

This is a precursor CL to https://codereview.chromium.org/14576015

BUG=235648, 232842, 179830
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=201028

[gab, 2013-05-16 22:53:40]

Scott, PTAL, I feel this refactoring is required to address your comment
@https://codereview.chromium.org/14576015/diff/43017/chrome/browser/lifetime/application_lifetime.cc#newcode116
without creating a mess.

Also adding Robert with whom I came up with this concept and Erik who thought we
might be able to do something better, but will be able to assess further when
faced with the full explanation.

Thanks!
Gab

[sky, 2013-05-16 23:31:55]

I agree the current approach is a bit painful. I'm hoping we can avoid
yet another layer of indirection though. What about renaming
BrowserProcessPlatformPart to BrowserProcessPlatformPartBase (or some
similar name) and then having the platform implementations actually
subclass that and call them BrowserProcessPlatformPart. For the
platforms that don't care we could typedef
BrowserProcessPlatformPartBase as BrowserProcessPlatformPart.

  -Scott

On Thu, May 16, 2013 at 3:53 PM,  <gab@chromium.org> wrote:
> Reviewers: sky, robertshield, erikwright,
>
> Message:
> Scott, PTAL, I feel this refactoring is required to address your comment
>
@https://codereview.chromium.org/14576015/diff/43017/chrome/browser/lifetime/application_lifetime.cc#newcode116
> without creating a mess.
>
> Also adding Robert with whom I came up with this concept and Erik who
> thought we
> might be able to do something better, but will be able to assess further
> when
> faced with the full explanation.
>
> Thanks!
> Gab
>
> Description:
> Refactor BrowserProcessPlatformPart.
>
> The previous design was to have a class, BrowserProcessPlatformPart, which
> has
> various implementations for specific platforms and a common implementation
> for
> all other platforms. These implementations (e.g.,
> browser_process_platform_part_(chromeos|aurawin).h) both implement the
> common
> methods defined in the common implementation
> (browser_process_platform_part.h)
> and some methods specific to their platform.
>
> The main problem with this design is that each custom implementation needs
> to
> re-implement the default behavior even for the methods for which the base
> implementation would have been fine (since it's a compile-time choice and
> doesn't use inheritance).
>
> This will become worst as more common methods get added to the base
> BrowserProcessPlatformPart, forcing every custom implementation to provide a
> copy of the default implementation for that method...
>
> The previous design also forces ugly #if ... #elif ... #elif ... #else
> ...#endif
> style of includes just to use the common methods since all headers, but one,
> are
> discarded by gyp.
>
> This will become worst as more platforms need custom
> BrowserProcessPlatformPart.
>
>
> Both scenarios making this worst are happening as part of addressing
>
https://codereview.chromium.org/14576015/diff/43017/chrome/browser/lifetime/a...
> where adding a common call to BrowserProcessPlatformPart and using it to get
> rid
> of the ifdefs there implies one more common call and 2 more platforms.
>
>
> With the new design:
> There is a common pure virtual BrowserProcessPlatformPart class which
> implements
> the default functionality.
> There are platform-specific BrowserProcessPlatformPartImpl classes that
> subclass
> BrowserProcessPlatformPart and override only the methods for which they need
> a
> custom behavior.
>
> To access methods specific to an implementation of
> BrowserProcessPlatformPartImpl, platform-specific code can request
> platform_part->AsBrowserProcessPlatformPartImpl()->PlatformSpecificMethod().
>
> Adding a new common method with a default behavior and a custom behavior for
> a
> single-platform now only involves touching BrowserProcessPlatformPart and
> that
> platform's BrowserProcessPlatformPartImpl.
>
> Adding a new platform to override some BrowserProcessPlatformPart methods
> now
> only involves adding a BrowserProcessPlatformPartImpl for that platform and
> changing chrome_browser.gypi to exclude
> browser_process_platform_part_stub.cc
> for that platform.
>
> No more #if ... #elif ... #elif ... #else ...#endif include mess (except in
> browser_process_platform_part.cc for the factory Create() method...).
>
> This is a precursor CL to https://codereview.chromium.org/14576015
>
> BUG=235648, 232842, 179830
>
> Please review this at https://codereview.chromium.org/14951007/
>
> SVN Base: svn://svn.chromium.org/chrome/trunk/src
>
> Affected files:
>   M chrome/browser/browser_process_impl.cc
>   M chrome/browser/browser_process_platform_part.h
>   M chrome/browser/browser_process_platform_part.cc
>   D chrome/browser/browser_process_platform_part_aurawin.h
>   D chrome/browser/browser_process_platform_part_aurawin.cc
>   D chrome/browser/browser_process_platform_part_chromeos.h
>   D chrome/browser/browser_process_platform_part_chromeos.cc
>   A chrome/browser/browser_process_platform_part_impl_aurawin.h
>   A + chrome/browser/browser_process_platform_part_impl_aurawin.cc
>   A + chrome/browser/browser_process_platform_part_impl_chromeos.h
>   A + chrome/browser/browser_process_platform_part_impl_chromeos.cc
>   A chrome/browser/browser_process_platform_part_impl_stub.h
>   A chrome/browser/browser_process_platform_part_impl_stub.cc
>   M chrome/browser/chrome_browser_main.cc
>   M chrome/browser/chromeos/chrome_browser_main_chromeos.cc
>   M chrome/browser/chromeos/extensions/networking_private_api.cc
>   M chrome/browser/chromeos/login/login_display_host_impl.cc
>   M chrome/browser/chromeos/memory/oom_priority_manager.cc
>   M chrome/browser/chromeos/memory/oom_priority_manager_browsertest.cc
>   M chrome/browser/metro_viewer/metro_viewer_process_host_win.cc
>   M chrome/browser/process_singleton_win.cc
>   M chrome/browser/profiles/profile_manager.cc
>   M chrome/browser/ui/webui/about_ui.cc
>   M chrome/browser/ui/webui/chromeos/login/signin_screen_handler.cc
>   M chrome/chrome_browser.gypi
>   M chrome/test/base/testing_browser_process_platform_part_chromeos.h
>
>

[gab, 2013-05-17 02:47:42]

I like that, the only problem is it keeps the #if ... #elif ... #elif ... #else
... #endif include mess around in callers' files including only
browser_process_platform_part_base.h is insufficient to actually use
platform_part (even for methods defined in the base... I was surprised by this
at first, but it makes sense as a caller that doesn't know about
BrowserProcessPlatformPart doesn't know it's a subclass of
BrowserProcessPlatformPartBase either... and thus can't use the common methods
although it technically has everything it needs...)...

We could do as in base_paths.h and have the #ifdef include mess in the header
which would implicitly include the right impl for anyone who includes
browser_process_platform_part_base.h (given the impl is an implementation
detail, I think it's ok to relieve the caller from that burden and drop the
ifdefs in browser_process_platform_part_base.h even if that kind of breaks
include-what-you-use...).

WDYT?


On 2013/05/16 23:31:55, sky wrote:
> I agree the current approach is a bit painful. I'm hoping we can avoid
> yet another layer of indirection though. What about renaming
> BrowserProcessPlatformPart to BrowserProcessPlatformPartBase (or some
> similar name) and then having the platform implementations actually
> subclass that and call them BrowserProcessPlatformPart. For the
> platforms that don't care we could typedef
> BrowserProcessPlatformPartBase as BrowserProcessPlatformPart.
> 
>   -Scott
> 
> On Thu, May 16, 2013 at 3:53 PM, 
<https://mail.google.com/mail/?view=cm&fs=1&tf=1&to=gab@chromium.org> wrote:
> > Reviewers: sky, robertshield, erikwright,
> >
> > Message:
> > Scott, PTAL, I feel this refactoring is required to address your comment
> >
>
@https://codereview.chromium.org/14576015/diff/43017/chrome/browser/lifetime/application_lifetime.cc#newcode116
> > without creating a mess.
> >
> > Also adding Robert with whom I came up with this concept and Erik who
> > thought we
> > might be able to do something better, but will be able to assess further
> > when
> > faced with the full explanation.
> >
> > Thanks!
> > Gab
> >
> > Description:
> > Refactor BrowserProcessPlatformPart.
> >
> > The previous design was to have a class, BrowserProcessPlatformPart, which
> > has
> > various implementations for specific platforms and a common implementation
> > for
> > all other platforms. These implementations (e.g.,
> > browser_process_platform_part_(chromeos|aurawin).h) both implement the
> > common
> > methods defined in the common implementation
> > (browser_process_platform_part.h)
> > and some methods specific to their platform.
> >
> > The main problem with this design is that each custom implementation needs
> > to
> > re-implement the default behavior even for the methods for which the base
> > implementation would have been fine (since it's a compile-time choice and
> > doesn't use inheritance).
> >
> > This will become worst as more common methods get added to the base
> > BrowserProcessPlatformPart, forcing every custom implementation to provide a
> > copy of the default implementation for that method...
> >
> > The previous design also forces ugly #if ... #elif ... #elif ... #else
> > ...#endif
> > style of includes just to use the common methods since all headers, but one,
> > are
> > discarded by gyp.
> >
> > This will become worst as more platforms need custom
> > BrowserProcessPlatformPart.
> >
> >
> > Both scenarios making this worst are happening as part of addressing
> >
>
https://codereview.chromium.org/14576015/diff/43017/chrome/browser/lifetime/a...
> > where adding a common call to BrowserProcessPlatformPart and using it to get
> > rid
> > of the ifdefs there implies one more common call and 2 more platforms.
> >
> >
> > With the new design:
> > There is a common pure virtual BrowserProcessPlatformPart class which
> > implements
> > the default functionality.
> > There are platform-specific BrowserProcessPlatformPartImpl classes that
> > subclass
> > BrowserProcessPlatformPart and override only the methods for which they need
> > a
> > custom behavior.
> >
> > To access methods specific to an implementation of
> > BrowserProcessPlatformPartImpl, platform-specific code can request
> > platform_part->AsBrowserProcessPlatformPartImpl()->PlatformSpecificMethod().
> >
> > Adding a new common method with a default behavior and a custom behavior for
> > a
> > single-platform now only involves touching BrowserProcessPlatformPart and
> > that
> > platform's BrowserProcessPlatformPartImpl.
> >
> > Adding a new platform to override some BrowserProcessPlatformPart methods
> > now
> > only involves adding a BrowserProcessPlatformPartImpl for that platform and
> > changing chrome_browser.gypi to exclude
> > browser_process_platform_part_stub.cc
> > for that platform.
> >
> > No more #if ... #elif ... #elif ... #else ...#endif include mess (except in
> > browser_process_platform_part.cc for the factory Create() method...).
> >
> > This is a precursor CL to https://codereview.chromium.org/14576015
> >
> > BUG=235648, 232842, 179830
> >
> > Please review this at https://codereview.chromium.org/14951007/
> >
> > SVN Base: svn://svn.chromium.org/chrome/trunk/src
> >
> > Affected files:
> >   M chrome/browser/browser_process_impl.cc
> >   M chrome/browser/browser_process_platform_part.h
> >   M chrome/browser/browser_process_platform_part.cc
> >   D chrome/browser/browser_process_platform_part_aurawin.h
> >   D chrome/browser/browser_process_platform_part_aurawin.cc
> >   D chrome/browser/browser_process_platform_part_chromeos.h
> >   D chrome/browser/browser_process_platform_part_chromeos.cc
> >   A chrome/browser/browser_process_platform_part_impl_aurawin.h
> >   A + chrome/browser/browser_process_platform_part_impl_aurawin.cc
> >   A + chrome/browser/browser_process_platform_part_impl_chromeos.h
> >   A + chrome/browser/browser_process_platform_part_impl_chromeos.cc
> >   A chrome/browser/browser_process_platform_part_impl_stub.h
> >   A chrome/browser/browser_process_platform_part_impl_stub.cc
> >   M chrome/browser/chrome_browser_main.cc
> >   M chrome/browser/chromeos/chrome_browser_main_chromeos.cc
> >   M chrome/browser/chromeos/extensions/networking_private_api.cc
> >   M chrome/browser/chromeos/login/login_display_host_impl.cc
> >   M chrome/browser/chromeos/memory/oom_priority_manager.cc
> >   M chrome/browser/chromeos/memory/oom_priority_manager_browsertest.cc
> >   M chrome/browser/metro_viewer/metro_viewer_process_host_win.cc
> >   M chrome/browser/process_singleton_win.cc
> >   M chrome/browser/profiles/profile_manager.cc
> >   M chrome/browser/ui/webui/about_ui.cc
> >   M chrome/browser/ui/webui/chromeos/login/signin_screen_handler.cc
> >   M chrome/chrome_browser.gypi
> >   M chrome/test/base/testing_browser_process_platform_part_chromeos.h
> >
> >

[gab, 2013-05-17 03:46:20]

PTAL, now using BrowserProcessPlatformPartBase as a base for all impls (and
typedefed the base to the impl on platforms that don't care).

Used browser_process_platform_part.h to be a meta-include that knows which impl
to include without having include messes in every consumer of the common calls.

Cheers!
Gab

[sky, 2013-05-17 15:42:33]

I like what you have here. Much cleaner.

https://codereview.chromium.org/14951007/diff/14001/chrome/browser/browser_pr...
File chrome/browser/browser_process_platform_part.h (right):

https://codereview.chromium.org/14951007/diff/14001/chrome/browser/browser_pr...
chrome/browser/browser_process_platform_part.h:16: typedef
BrowserProcessPlatformPartBase BrowserProcessPlatformPart
Aren't you missing a ';' here?

https://codereview.chromium.org/14951007/diff/14001/chrome/browser/browser_pr...
File chrome/browser/browser_process_platform_part_base.h (right):

https://codereview.chromium.org/14951007/diff/14001/chrome/browser/browser_pr...
chrome/browser/browser_process_platform_part_base.h:13: // implementations. This
class itself should never be used verbatim.
This last sentence is wrong since this is used on platforms that don't need a
platform specific part, right?

[gab, 2013-05-17 19:47:00]

PTAL, also fixed TestingBrowserProcessPlatformPart (see issue the previous
paradigm created on https://codereview.chromium.org/14698027/). I thought of
giving it the same design we are doing in this CL for
BrowserProcessPlatformPart. but it felt overkill since the only purpose of
TestingBrowserProcessPlatformPart seems to be to not create an
OomPriorityManager in tests on ChromeOS so I simply brought all of the logic in
TestingBrowserProcessPlatformPart itself and added an OS_CHROMEOS specific
section to that file (feels cleaner than an extra level of platform-specific
complexity just to achieve this single feature).

Cheers,
Gab

https://codereview.chromium.org/14951007/diff/14001/chrome/browser/browser_pr...
File chrome/browser/browser_process_platform_part.h (right):

https://codereview.chromium.org/14951007/diff/14001/chrome/browser/browser_pr...
chrome/browser/browser_process_platform_part.h:16: typedef
BrowserProcessPlatformPartBase BrowserProcessPlatformPart
On 2013/05/17 15:42:33, sky wrote:
> Aren't you missing a ';' here?

Indeed :), done.

https://codereview.chromium.org/14951007/diff/14001/chrome/browser/browser_pr...
File chrome/browser/browser_process_platform_part_base.h (right):

https://codereview.chromium.org/14951007/diff/14001/chrome/browser/browser_pr...
chrome/browser/browser_process_platform_part_base.h:13: // implementations. This
class itself should never be used verbatim.
On 2013/05/17 15:42:33, sky wrote:
> This last sentence is wrong since this is used on platforms that don't need a
> platform specific part, right?

What I meant is it can't be used "verbatim"; i.e. when you use it through the
typedef you use it as BrowserProcessPlatformPart, not verbatim as
BrowserProcessPlatformPartBase so that's fine.

I was thinking we could also add a presubmit check to make sure no one ever does
"new BrowserProcessPlatformPartBase()", but that might be overkill..?

[sky, 2013-05-17 21:19:05]

LGTM

[commit-bot: I haz the power, 2013-05-17 23:37:31]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/gab@chromium.org/14951007/59001

[commit-bot: I haz the power, 2013-05-17 23:50:47]

Sorry for I got bad news for ya.
Compile failed with a clobber build on ios_dbg_simulator.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=ios_dbg_si...
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2013-05-19 00:02:53]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/gab@chromium.org/14951007/70001

[commit-bot: I haz the power, 2013-05-19 00:18:18]

Sorry for I got bad news for ya.
Compile failed with a clobber build on ios_dbg_simulator.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=ios_dbg_si...
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2013-05-19 01:22:51]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/gab@chromium.org/14951007/83003

[commit-bot: I haz the power, 2013-05-19 01:37:41]

Sorry for I got bad news for ya.
Compile failed with a clobber build on ios_dbg_simulator.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=ios_dbg_si...
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2013-05-19 04:16:14]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/gab@chromium.org/14951007/81004

[commit-bot: I haz the power, 2013-05-20 05:30:18]

Change committed as 201028