Clean up singleton implementation

Clean up singleton implementation

Move the singleton instances into their namespaces, and use
get()/getInstance() for uniform accesses.

Committed: https://pdfium.googlesource.com/pdfium/+/47228aceb86744f858ab8bfa98f3f8b62054dfae

[Wei Li, 2016-07-15 19:50:16]

weili@chromium.org changed reviewers:
+ thestig@chromium.org

[Wei Li, 2016-07-15 19:50:17]

pls review, thanks

[Lei Zhang, 2016-07-16 01:23:03]

https://codereview.chromium.org/2154843002/diff/1/core/fpdfapi/include/cpdf_m...
File core/fpdfapi/include/cpdf_modulemgr.h (right):

https://codereview.chromium.org/2154843002/diff/1/core/fpdfapi/include/cpdf_m...
core/fpdfapi/include/cpdf_modulemgr.h:59: static CPDF_ModuleMgr* m_pDefaultMgr;
Leave it as a global, for the same reason as CPDFXFA_App.

https://codereview.chromium.org/2154843002/diff/1/core/fxge/include/fx_ge.h
File core/fxge/include/fx_ge.h (right):

https://codereview.chromium.org/2154843002/diff/1/core/fxge/include/fx_ge.h#n...
core/fxge/include/fx_ge.h:47: static CFX_GEModule* m_pGEModule;
Leave as a global, for the same reason as CPDFXFA_App.

https://codereview.chromium.org/2154843002/diff/1/fpdfsdk/fpdfxfa/include/fpd...
File fpdfsdk/fpdfxfa/include/fpdfxfa_app.h (right):

https://codereview.chromium.org/2154843002/diff/1/fpdfsdk/fpdfxfa/include/fpd...
fpdfsdk/fpdfxfa/include/fpdfxfa_app.h:81: static CPDFXFA_App* m_pApp;
Only used in fpdfsdk/fpdfxfa/fpdfxfa_app.cpp, and
fpdfsdk/fpdfxfa/fpdfxfa_app.cpp does not contain any other classes - put it in
there instead?

https://codereview.chromium.org/2154843002/diff/1/fpdfsdk/javascript/JS_Globa...
File fpdfsdk/javascript/JS_GlobalData.h (right):

https://codereview.chromium.org/2154843002/diff/1/fpdfsdk/javascript/JS_Globa...
fpdfsdk/javascript/JS_GlobalData.h:87: static CJS_GlobalData* m_pInstance;
s_ for static?

[Wei Li, 2016-07-18 19:07:07]

ptal, thanks

https://codereview.chromium.org/2154843002/diff/1/core/fpdfapi/include/cpdf_m...
File core/fpdfapi/include/cpdf_modulemgr.h (right):

https://codereview.chromium.org/2154843002/diff/1/core/fpdfapi/include/cpdf_m...
core/fpdfapi/include/cpdf_modulemgr.h:59: static CPDF_ModuleMgr* m_pDefaultMgr;
On 2016/07/16 01:23:03, Lei Zhang wrote:
> Leave it as a global, for the same reason as CPDFXFA_App.

Not sure which style to follow as both g_ and s_ were used. Let's go with s_ to
be different from global variables.

https://codereview.chromium.org/2154843002/diff/1/core/fxge/include/fx_ge.h
File core/fxge/include/fx_ge.h (right):

https://codereview.chromium.org/2154843002/diff/1/core/fxge/include/fx_ge.h#n...
core/fxge/include/fx_ge.h:47: static CFX_GEModule* m_pGEModule;
On 2016/07/16 01:23:03, Lei Zhang wrote:
> Leave as a global, for the same reason as CPDFXFA_App.

Same here.

https://codereview.chromium.org/2154843002/diff/1/fpdfsdk/fpdfxfa/include/fpd...
File fpdfsdk/fpdfxfa/include/fpdfxfa_app.h (right):

https://codereview.chromium.org/2154843002/diff/1/fpdfsdk/fpdfxfa/include/fpd...
fpdfsdk/fpdfxfa/include/fpdfxfa_app.h:81: static CPDFXFA_App* m_pApp;
On 2016/07/16 01:23:03, Lei Zhang wrote:
> Only used in fpdfsdk/fpdfxfa/fpdfxfa_app.cpp, and
> fpdfsdk/fpdfxfa/fpdfxfa_app.cpp does not contain any other classes - put it in
> there instead?

Not much difference, let's keep the implementation uniform?

https://codereview.chromium.org/2154843002/diff/1/fpdfsdk/javascript/JS_Globa...
File fpdfsdk/javascript/JS_GlobalData.h (right):

https://codereview.chromium.org/2154843002/diff/1/fpdfsdk/javascript/JS_Globa...
fpdfsdk/javascript/JS_GlobalData.h:87: static CJS_GlobalData* m_pInstance;
On 2016/07/16 01:23:03, Lei Zhang wrote:
> s_ for static?

Done.

[Lei Zhang, 2016-07-18 19:35:00]

https://codereview.chromium.org/2154843002/diff/1/core/fpdfapi/include/cpdf_m...
File core/fpdfapi/include/cpdf_modulemgr.h (right):

https://codereview.chromium.org/2154843002/diff/1/core/fpdfapi/include/cpdf_m...
core/fpdfapi/include/cpdf_modulemgr.h:59: static CPDF_ModuleMgr* m_pDefaultMgr;
On 2016/07/18 19:07:07, Wei Li wrote:
> On 2016/07/16 01:23:03, Lei Zhang wrote:
> > Leave it as a global, for the same reason as CPDFXFA_App.
> 
> Not sure which style to follow as both g_ and s_ were used. Let's go with s_
to
> be different from global variables.

Sorry if I was unclear. What I was trying to say is this pointer can be declared
in the .cpp file as a global in an anonymous namespace. Doesn't need to be in
the header.

[Wei Li, 2016-07-18 20:02:45]

On 2016/07/18 19:35:00, Lei Zhang wrote:
>
https://codereview.chromium.org/2154843002/diff/1/core/fpdfapi/include/cpdf_m...
> File core/fpdfapi/include/cpdf_modulemgr.h (right):
> 
>
https://codereview.chromium.org/2154843002/diff/1/core/fpdfapi/include/cpdf_m...
> core/fpdfapi/include/cpdf_modulemgr.h:59: static CPDF_ModuleMgr*
m_pDefaultMgr;
> On 2016/07/18 19:07:07, Wei Li wrote:
> > On 2016/07/16 01:23:03, Lei Zhang wrote:
> > > Leave it as a global, for the same reason as CPDFXFA_App.
> > 
> > Not sure which style to follow as both g_ and s_ were used. Let's go with s_
> to
> > be different from global variables.
> 
> Sorry if I was unclear. What I was trying to say is this pointer can be
declared
> in the .cpp file as a global in an anonymous namespace. Doesn't need to be in
> the header.

Any advantage of using anonymous global? It should be ok in either way. I find a
class member variable
is a bit clear as it is only accessed through this class.

[Lei Zhang, 2016-07-18 20:25:19]

On 2016/07/18 20:02:45, Wei Li wrote:
> Any advantage of using anonymous global? It should be ok in either way. I find
a
> class member variable
> is a bit clear as it is only accessed through this class.

Just more encapsulation. For instance, if there's a global PrivateImplClass*,
you don't even have to expose the existence of that through the header.

We generally do file-level globals in Chromium. The main reason we haven't been
doing more of it PDFium is because there are files that have impls for more than
1 class. In those cases, we risk the wrong class getting access to the global.
As we clean up the codebase, we are making it 1 impl per file, so it is getting
safer to do file-level globals.

[Wei Li, 2016-07-18 21:20:33]

On 2016/07/18 20:25:19, Lei Zhang wrote:
> On 2016/07/18 20:02:45, Wei Li wrote:
> > Any advantage of using anonymous global? It should be ok in either way. I
find
> a
> > class member variable
> > is a bit clear as it is only accessed through this class.
> 
> Just more encapsulation. For instance, if there's a global PrivateImplClass*,
> you don't even have to expose the existence of that through the header.
> 
> We generally do file-level globals in Chromium. The main reason we haven't
been
> doing more of it PDFium is because there are files that have impls for more
than
> 1 class. In those cases, we risk the wrong class getting access to the global.
> As we clean up the codebase, we are making it 1 impl per file, so it is
getting
> safer to do file-level globals.

Aren't private member variables already be encapsulated within that class?
Declaring in the header doesn't mean it is exposed, right?

Chromium used a singleton class (in base/singleton.h) for this purpose, which
has
a similar implementation.

[Lei Zhang, 2016-07-18 23:05:24]

On 2016/07/18 21:20:33, Wei Li wrote:
> Aren't private member variables already be encapsulated within that class?
> Declaring in the header doesn't mean it is exposed, right?

Yes, but declaring them in the header still exposes their existence to someone
reading the header.

For instance:

----
foo.h

class Foo {
 public:
  Foo()
  ...

 private:
  class PrivateImplClass;
  static PrivateImplClass* s_instance_;
};

exposes the fact that PrivateImplClass exists to someone reading the header.
Whereas:

foo.cc

namespace {

class PrivateImplClass {
  ...
};

PrivateImplClass* g_instance;

}  // namespace

does not expose the existence of PrivateImplClass to the outside world at all.

Sorry to be adament here, but I really would like us to be consistent, in the
way that matches Chromium style.

Looking in Chromium's base/, globals in .cc files - git grep '^[A-Za-z0-9*]* g_'
base|grep cc:

outnumber static privates by a big margin: git grep '^ *static [^(]*;$'
base|grep h:|grep -v const

> Chromium used a singleton class (in base/singleton.h) for this purpose, which
> has
> a similar implementation.

base/memory/singleton.h is actually a much more complicated beast than what we
have here. The header also starts off with a warning to prefer not using
base::Singleton.

[Wei Li, 2016-07-19 21:18:37]

On 2016/07/18 23:05:24, Lei Zhang wrote:
> On 2016/07/18 21:20:33, Wei Li wrote:
> > Aren't private member variables already be encapsulated within that class?
> > Declaring in the header doesn't mean it is exposed, right?
> 
> Yes, but declaring them in the header still exposes their existence to someone
> reading the header.
> 
> For instance:
> 
> ----
> foo.h
> 
> class Foo {
>  public:
>   Foo()
>   ...
> 
>  private:
>   class PrivateImplClass;
>   static PrivateImplClass* s_instance_;
> };
> 
> exposes the fact that PrivateImplClass exists to someone reading the header.
> Whereas:
> 
> foo.cc
> 
> namespace {
> 
> class PrivateImplClass {
>   ...
> };
> 
> PrivateImplClass* g_instance;
> 
> }  // namespace
> 
> does not expose the existence of PrivateImplClass to the outside world at all.
> 
> Sorry to be adament here, but I really would like us to be consistent, in the
> way that matches Chromium style.
> 
> Looking in Chromium's base/, globals in .cc files - git grep '^[A-Za-z0-9*]*
g_'
> base|grep cc:
> 
> outnumber static privates by a big margin: git grep '^ *static [^(]*;$'
> base|grep h:|grep -v const
> 
> > Chromium used a singleton class (in base/singleton.h) for this purpose,
which
> > has
> > a similar implementation.
> 
> base/memory/singleton.h is actually a much more complicated beast than what we
> have here. The header also starts off with a warning to prefer not using
> base::Singleton.

ok, let's use globals instead. 

ptal, thanks.

[Lei Zhang, 2016-07-20 00:34:46]

lgtm

https://codereview.chromium.org/2154843002/diff/100001/xfa/fwl/theme/cfwl_arr...
File xfa/fwl/theme/cfwl_arrowdata.h (right):

https://codereview.chromium.org/2154843002/diff/100001/xfa/fwl/theme/cfwl_arr...
xfa/fwl/theme/cfwl_arrowdata.h:25: static FX_BOOL IsInstance();
HasInstance

[Wei Li, 2016-07-20 17:33:15]

thanks

https://codereview.chromium.org/2154843002/diff/100001/xfa/fwl/theme/cfwl_arr...
File xfa/fwl/theme/cfwl_arrowdata.h (right):

https://codereview.chromium.org/2154843002/diff/100001/xfa/fwl/theme/cfwl_arr...
xfa/fwl/theme/cfwl_arrowdata.h:25: static FX_BOOL IsInstance();
On 2016/07/20 00:34:46, Lei Zhang wrote:
> HasInstance

Done.

[Wei Li, 2016-07-20 17:33:53]

The CQ bit was checked by weili@chromium.org

[Wei Li, 2016-07-20 17:33:53]

The patchset sent to the CQ was uploaded after l-g-t-m from thestig@chromium.org
Link to the patchset: https://codereview.chromium.org/2154843002/#ps140001
(title: "rebase")

[commit-bot: I haz the power, 2016-07-20 17:33:56]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-07-20 17:35:34]

Description was changed from

==========
Clean up singleton implementation

Move the singleton instances into their namespaces, and use
get()/getInstance() for uniform accesses.
==========

to

==========
Clean up singleton implementation

Move the singleton instances into their namespaces, and use
get()/getInstance() for uniform accesses.

Committed:
https://pdfium.googlesource.com/pdfium/+/47228aceb86744f858ab8bfa98f3f8b62054...
==========

[commit-bot: I haz the power, 2016-07-20 17:35:36]

Committed patchset #8 (id:140001) as
https://pdfium.googlesource.com/pdfium/+/47228aceb86744f858ab8bfa98f3f8b62054...

[commit-bot: I haz the power, 2016-07-20 17:35:38]

CQ bit was unchecked.