Skip to content

GitLab

T
tangled
Switch branch/tag
  1. 07 Mar, 2020 1 commit
    • Daniel Cheng's avatar
      Simplify factory pattern used for test injection in device_sync/multidevice/secure_channel/tether · 4fd7e523
      Daniel Cheng authored 
      This CL simplifies and reduces the amount of indirection required for
injecting test fakes. This pattern has been inconsistently copied and
pasted across many different files, so things have gotten a bit messy.
Before this CL, there were two general strategies:

1. Hold a default Factory instance in a base::NoDestructor and delegate
   to it if no test factory override is set. This has the disadvantage
   of requiring an indirect call even when no injection is needed, as
   well as incurring overhead for storing, initializing, and accessing
   the base::NoDestructor.

2. Heap allocating a new default Factory instance if no test factory
   override is set. Like before, this has the disadvantage of always
   requiring an indirect call even when no injection is needed. It is
   also potentially leaky in tests.

Instead, these locations have been updated to follow the same pattern as
RenderFrameHostFactory and the naming conventions have been
standardized:

- Entry to the factory is via a static Factory::Create() method.
- Factory itself is now an interface class, with a pure virtual
  CreateInstance() method.
- The factory override is set via a static Factory::SetFactoryForTesting
  method().
- If a factory override is set, Factory::Create() delegates to the
  factory's CreateInstance() method.
- Otherwise, Factory::Create() simply performs uses the default
  construction path.

There is no real behavior change from before: previously, the static
factory method already had to have knowledge of the default factory, so
this simply lifts that logic into the static factory method itself and
eliminates an unneeded layer of indirection.

A few miscellaneous style fixes have been included as well:
- All default args have been removed from the virtual CreateInstance()
  methods. Default arguments on virtual methods are banned by the Google
  C++ style guide, due to their confusing semantics in that context.
- All the Factory interface classes now have a virtual destructor.

The net result is the removal of 60 unnecessary base::NoDestructor
globals, and the removal of another 18 bare new calls that could
potentially leak.

Bug: 960538
Tbr: avi@chromium.org
Tbr: khorimoto@chromium.org
Change-Id: I657af336ab1538bd762352062bdc97df451dabec
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2088358
Commit-Queue: Daniel Cheng <dcheng@chromium.org>
Reviewed-by: default avatarKyle Horimoto <khorimoto@chromium.org>
Reviewed-by: default avatarRyan Hansberry <hansberry@chromium.org>
Cr-Commit-Position: refs/heads/master@{#747937}
      4fd7e523
  3. 06 Mar, 2020 39 commits