Reland r167487: Get full WebPluginInfo for the PDF plug-in before enabling it for print preview.

Reland r167487: Get full WebPluginInfo for the PDF plug-in before enabling it for print preview.

Previous review: http://codereview.chromium.org/11364202/

TBR=thestig@chromium.org
BUG=159902, 113008
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=169938

[Bernhard Bauer, 2012-11-15 16:27:12]

Please review...

Peter: This does what you suggested in http://crbug.com/113008 (Don't unregister
window classes) by making the ClassRegistrar singleton leaky. See comment 6 (or
http://crbug.com/159902) for why; TL;DR is unit_tests broke when tearing down
other singletons via ShadowingAtExitManager.

Ben: Invoked for ui/ OWNERS and general Windows wisdom.

Lei: TBR for the rest, no changes compared to the last version there.

Thanks!

[Ben Goodger (Google), 2012-11-15 17:08:22]

I'll admit I've never really understood the complexity we have around WNDCLASS
registration in WindowImpl. Maybe cpu understands?

[Peter Kasting, 2012-11-15 18:53:50]

LGTM

https://codereview.chromium.org/11417003/diff/1/chrome/browser/printing/print...
File chrome/browser/printing/print_preview_tab_controller.cc (right):

https://codereview.chromium.org/11417003/diff/1/chrome/browser/printing/print...
chrome/browser/printing/print_preview_tab_controller.cc:66: pdf_plugin_path,
&pdf_plugin)) {
Nit: Indent 4, not 8.

{} is optional.

https://codereview.chromium.org/11417003/diff/1/chrome/browser/printing/print...
File chrome/browser/printing/print_preview_tab_controller_unittest.cc (right):

https://codereview.chromium.org/11417003/diff/1/chrome/browser/printing/print...
chrome/browser/printing/print_preview_tab_controller_unittest.cc:37:
PrintPreviewTabControllerUnitTest() {}
Nit: While here, can you de-inline these member functions?  This particular
class is tiny so it's not too hard to read as-is, but we try to move people away
from having class members inlined just because they're in a .cc file (especially
for things like virtual functions).

https://codereview.chromium.org/11417003/diff/1/chrome/browser/printing/print...
chrome/browser/printing/print_preview_tab_controller_unittest.cc:53:
webkit::npapi::MockPluginList plugin_list_;
Nit: DISALLOW_COPY_AND_ASSIGN too

https://codereview.chromium.org/11417003/diff/1/chrome/browser/ui/webui/print...
File chrome/browser/ui/webui/print_preview/print_preview_ui_unittest.cc (right):

https://codereview.chromium.org/11417003/diff/1/chrome/browser/ui/webui/print...
chrome/browser/ui/webui/print_preview/print_preview_ui_unittest.cc:47:
PrintPreviewUIUnitTest() {}
Hmm, any place we could put a combined version of this class and the one in the
previous file?  Since they're nearly identical...

https://codereview.chromium.org/11417003/diff/1/ui/base/win/window_impl.cc
File ui/base/win/window_impl.cc (right):

https://codereview.chromium.org/11417003/diff/1/ui/base/win/window_impl.cc#ne...
ui/base/win/window_impl.cc:47: static ClassRegistrar* GetInstance() {
Nit: Would be good to de-inline this as well.

Also, Google style guide says static member functions go after
constructor/destructor.

[cpu_(ooo_6.6-7.5), 2012-11-15 19:08:36]

http://codereview.chromium.org/11417003/diff/1/ui/base/win/window_impl.cc
File ui/base/win/window_impl.cc (left):

http://codereview.chromium.org/11417003/diff/1/ui/base/win/window_impl.cc#old...
ui/base/win/window_impl.cc:56: << ". Error = " << GetLastError();
So we are leaking the classes, I would be ok with this if you can assert we
create a bounded number of them. For example, tooltips and other transient
windows we make.

In regular windows programs you would only have a small set of window classes,
say 3 or 4, but our code is generic and complicated.

http://codereview.chromium.org/11417003/diff/1/ui/base/win/window_impl.cc#old...
ui/base/win/window_impl.cc:105: // The ATOM returned from registering the window
class.
I would rather keep the atom and ditch the name.

[Bernhard Bauer, 2012-11-15 23:26:29]

Thanks for the reviews! I'll do the fixes tomorrow.

http://codereview.chromium.org/11417003/diff/1/chrome/browser/printing/print_...
File chrome/browser/printing/print_preview_tab_controller_unittest.cc (right):

http://codereview.chromium.org/11417003/diff/1/chrome/browser/printing/print_...
chrome/browser/printing/print_preview_tab_controller_unittest.cc:53:
webkit::npapi::MockPluginList plugin_list_;
On 2012/11/15 18:53:50, Peter Kasting wrote:
> Nit: DISALLOW_COPY_AND_ASSIGN too

Gahh, I always forget these. Note to self: write a clang plug-in that checks
this.

http://codereview.chromium.org/11417003/diff/1/ui/base/win/window_impl.cc
File ui/base/win/window_impl.cc (left):

http://codereview.chromium.org/11417003/diff/1/ui/base/win/window_impl.cc#old...
ui/base/win/window_impl.cc:56: << ". Error = " << GetLastError();
On 2012/11/15 19:08:36, cpu wrote:
> So we are leaking the classes, I would be ok with this if you can assert we
> create a bounded number of them. For example, tooltips and other transient
> windows we make.

Hm, if I read http://crbug.com/113008 correct, we already fail to unregister
them either way, as there is still a window open when we try to.

In any case, I can run this with some logging tomorrow to get a feeling for the
number of classes.

> In regular windows programs you would only have a small set of window classes,
> say 3 or 4, but our code is generic and complicated.

http://codereview.chromium.org/11417003/diff/1/ui/base/win/window_impl.cc#old...
ui/base/win/window_impl.cc:105: // The ATOM returned from registering the window
class.
On 2012/11/15 19:08:36, cpu wrote:
> I would rather keep the atom and ditch the name.

But we need the name to find classes by name, or am I missing something?

[cpu_(ooo_6.6-7.5), 2012-11-16 21:35:32]

On 2012/11/15 23:26:29, Bernhard Bauer wrote:
> Thanks for the reviews! I'll do the fixes tomorrow.
> 
>
http://codereview.chromium.org/11417003/diff/1/chrome/browser/printing/print_...
> File chrome/browser/printing/print_preview_tab_controller_unittest.cc (right):
> 
>
http://codereview.chromium.org/11417003/diff/1/chrome/browser/printing/print_...
> chrome/browser/printing/print_preview_tab_controller_unittest.cc:53:
> webkit::npapi::MockPluginList plugin_list_;
> On 2012/11/15 18:53:50, Peter Kasting wrote:
> > Nit: DISALLOW_COPY_AND_ASSIGN too
> 
> Gahh, I always forget these. Note to self: write a clang plug-in that checks
> this.
> 
> http://codereview.chromium.org/11417003/diff/1/ui/base/win/window_impl.cc
> File ui/base/win/window_impl.cc (left):
> 
>
http://codereview.chromium.org/11417003/diff/1/ui/base/win/window_impl.cc#old...
> ui/base/win/window_impl.cc:56: << ". Error = " << GetLastError();
> On 2012/11/15 19:08:36, cpu wrote:
> > So we are leaking the classes, I would be ok with this if you can assert we
> > create a bounded number of them. For example, tooltips and other transient
> > windows we make.
> 
> Hm, if I read http://crbug.com/113008 correct, we already fail to unregister
> them either way, as there is still a window open when we try to.
> 
> In any case, I can run this with some logging tomorrow to get a feeling for
the
> number of classes.
> 
> > In regular windows programs you would only have a small set of window
classes,
> > say 3 or 4, but our code is generic and complicated.
> 
>
http://codereview.chromium.org/11417003/diff/1/ui/base/win/window_impl.cc#old...
> ui/base/win/window_impl.cc:105: // The ATOM returned from registering the
window
> class.
> On 2012/11/15 19:08:36, cpu wrote:
> > I would rather keep the atom and ditch the name.
> 
> But we need the name to find classes by name, or am I missing something?

Do we? can we use
http://msdn.microsoft.com/en-us/library/windows/desktop/ms633582(v=vs.85).aspx 
?

[Bernhard Bauer, 2012-11-18 19:15:26]

On 2012/11/16 21:35:32, cpu wrote:
> On 2012/11/15 23:26:29, Bernhard Bauer wrote:
> > Thanks for the reviews! I'll do the fixes tomorrow.
> > 
> >
>
http://codereview.chromium.org/11417003/diff/1/chrome/browser/printing/print_...
> > File chrome/browser/printing/print_preview_tab_controller_unittest.cc
(right):
> > 
> >
>
http://codereview.chromium.org/11417003/diff/1/chrome/browser/printing/print_...
> > chrome/browser/printing/print_preview_tab_controller_unittest.cc:53:
> > webkit::npapi::MockPluginList plugin_list_;
> > On 2012/11/15 18:53:50, Peter Kasting wrote:
> > > Nit: DISALLOW_COPY_AND_ASSIGN too
> > 
> > Gahh, I always forget these. Note to self: write a clang plug-in that checks
> > this.
> > 
> > http://codereview.chromium.org/11417003/diff/1/ui/base/win/window_impl.cc
> > File ui/base/win/window_impl.cc (left):
> > 
> >
>
http://codereview.chromium.org/11417003/diff/1/ui/base/win/window_impl.cc#old...
> > ui/base/win/window_impl.cc:56: << ". Error = " << GetLastError();
> > On 2012/11/15 19:08:36, cpu wrote:
> > > So we are leaking the classes, I would be ok with this if you can assert
we
> > > create a bounded number of them. For example, tooltips and other transient
> > > windows we make.
> > 
> > Hm, if I read http://crbug.com/113008 correct, we already fail to unregister
> > them either way, as there is still a window open when we try to.
> > 
> > In any case, I can run this with some logging tomorrow to get a feeling for
> the
> > number of classes.
> > 
> > > In regular windows programs you would only have a small set of window
> classes,
> > > say 3 or 4, but our code is generic and complicated.
> > 
> >
>
http://codereview.chromium.org/11417003/diff/1/ui/base/win/window_impl.cc#old...
> > ui/base/win/window_impl.cc:105: // The ATOM returned from registering the
> window
> > class.
> > On 2012/11/15 19:08:36, cpu wrote:
> > > I would rather keep the atom and ditch the name.
> > 
> > But we need the name to find classes by name, or am I missing something?
> 
> Do we? can we use
>
http://msdn.microsoft.com/en-us/library/windows/desktop/ms633582%28v=vs.85%29...

> ?

Hm, GetClassName() requires an HWND, and we need the class name before creating
the window. OTOH, I can modify the signature of RetrieveClassName so that it
returns an ATOM, and then use that at the call sites instead. Does that SGTY?

http://codereview.chromium.org/11417003/diff/1/chrome/browser/printing/print_...
File chrome/browser/printing/print_preview_tab_controller.cc (right):

http://codereview.chromium.org/11417003/diff/1/chrome/browser/printing/print_...
chrome/browser/printing/print_preview_tab_controller.cc:66: pdf_plugin_path,
&pdf_plugin)) {
On 2012/11/15 18:53:50, Peter Kasting wrote:
> Nit: Indent 4, not 8.
> 
> {} is optional.

Done.

http://codereview.chromium.org/11417003/diff/1/chrome/browser/printing/print_...
File chrome/browser/printing/print_preview_tab_controller_unittest.cc (right):

http://codereview.chromium.org/11417003/diff/1/chrome/browser/printing/print_...
chrome/browser/printing/print_preview_tab_controller_unittest.cc:37:
PrintPreviewTabControllerUnitTest() {}
On 2012/11/15 18:53:50, Peter Kasting wrote:
> Nit: While here, can you de-inline these member functions?  This particular
> class is tiny so it's not too hard to read as-is, but we try to move people
away
> from having class members inlined just because they're in a .cc file
(especially
> for things like virtual functions).

Moved it into a base class.

http://codereview.chromium.org/11417003/diff/1/chrome/browser/ui/webui/print_...
File chrome/browser/ui/webui/print_preview/print_preview_ui_unittest.cc (right):

http://codereview.chromium.org/11417003/diff/1/chrome/browser/ui/webui/print_...
chrome/browser/ui/webui/print_preview/print_preview_ui_unittest.cc:47:
PrintPreviewUIUnitTest() {}
On 2012/11/15 18:53:50, Peter Kasting wrote:
> Hmm, any place we could put a combined version of this class and the one in
the
> previous file?  Since they're nearly identical...

Done.

http://codereview.chromium.org/11417003/diff/1/ui/base/win/window_impl.cc
File ui/base/win/window_impl.cc (left):

http://codereview.chromium.org/11417003/diff/1/ui/base/win/window_impl.cc#old...
ui/base/win/window_impl.cc:56: << ". Error = " << GetLastError();
On 2012/11/15 23:26:30, Bernhard Bauer wrote:
> On 2012/11/15 19:08:36, cpu wrote:
> > So we are leaking the classes, I would be ok with this if you can assert we
> > create a bounded number of them. For example, tooltips and other transient
> > windows we make.
> 
> Hm, if I read http://crbug.com/113008 correct, we already fail to unregister
> them either way, as there is still a window open when we try to.
> 
> In any case, I can run this with some logging tomorrow to get a feeling for
the
> number of classes.

Ok, I ran unit_tests and I got only one registration, for Chrome_WidgetWin_0.
During a regular browser run we might get more, but those weren't unregistered
before the end of the process anyway.

> > In regular windows programs you would only have a small set of window
classes,
> > say 3 or 4, but our code is generic and complicated.
>

http://codereview.chromium.org/11417003/diff/1/ui/base/win/window_impl.cc
File ui/base/win/window_impl.cc (right):

http://codereview.chromium.org/11417003/diff/1/ui/base/win/window_impl.cc#new...
ui/base/win/window_impl.cc:47: static ClassRegistrar* GetInstance() {
On 2012/11/15 18:53:50, Peter Kasting wrote:
> Nit: Would be good to de-inline this as well.
> 
> Also, Google style guide says static member functions go after
> constructor/destructor.

Done.

[cpu_(ooo_6.6-7.5), 2012-11-20 20:19:57]

maybe use GetClassInfo instead? I believe we only have classes in the chrome.dll
so the module is fixed therefore GetClassInfo(x, makeintatom(atom), &name)

[Peter Kasting, 2012-11-20 21:05:07]

LGTM

https://codereview.chromium.org/11417003/diff/10003/chrome/browser/printing/p...
File chrome/browser/printing/print_preview_test.h (right):

https://codereview.chromium.org/11417003/diff/10003/chrome/browser/printing/p...
chrome/browser/printing/print_preview_test.h:17: virtual void SetUp() OVERRIDE;
Nit: Extra space

https://codereview.chromium.org/11417003/diff/10003/ui/base/win/window_impl.cc
File ui/base/win/window_impl.cc (right):

https://codereview.chromium.org/11417003/diff/10003/ui/base/win/window_impl.c...
ui/base/win/window_impl.cc:96: base::IntToString16(registered_count_++);
Nit: Indent 4, not even

I get nervous when I see people intermixing string16 and wstring like this. 
Last time I talked with Darin he suggested we just nuke wstring entirely, even
in Windows code.  So maybe |name| should be declared as a string16.

[Bernhard Bauer, 2012-11-21 16:48:44]

https://codereview.chromium.org/11417003/diff/10003/chrome/browser/printing/p...
File chrome/browser/printing/print_preview_test.h (right):

https://codereview.chromium.org/11417003/diff/10003/chrome/browser/printing/p...
chrome/browser/printing/print_preview_test.h:17: virtual void SetUp() OVERRIDE;
On 2012/11/20 21:05:07, Peter Kasting wrote:
> Nit: Extra space

Done.

https://codereview.chromium.org/11417003/diff/10003/ui/base/win/window_impl.cc
File ui/base/win/window_impl.cc (right):

https://codereview.chromium.org/11417003/diff/10003/ui/base/win/window_impl.c...
ui/base/win/window_impl.cc:96: base::IntToString16(registered_count_++);
On 2012/11/20 21:05:07, Peter Kasting wrote:
> Nit: Indent 4, not even
> 
> I get nervous when I see people intermixing string16 and wstring like this. 
> Last time I talked with Darin he suggested we just nuke wstring entirely, even
> in Windows code.  So maybe |name| should be declared as a string16.

Done.

[Bernhard Bauer, 2012-11-26 20:28:00]

On 2012/11/20 20:19:57, cpu wrote:
> maybe use GetClassInfo instead? I believe we only have classes in the
chrome.dll
> so the module is fixed therefore GetClassInfo(x, makeintatom(atom), &name)

Hmph, looks like Rietveld decided to indulge in Thanksgiving gluttony by eating
my comment. Reposting:

What's the problem with the current version of the CL? We only really need the
class name to retrieve information about the class name later, which we can do
with the atom as well. Other places in Chrome code that deal with window class
names seem to use atoms as well.

Also, would it be okay to land _some_ version of this and then iterate? I
originally wanted to make a tiny fix in the print preview code (for a stable
release blocker, to boot), and somehow it turned into a complete refactoring of
window class registration :-/

[cpu_(ooo_6.6-7.5), 2012-11-27 23:44:53]

lgtm

[commit-bot: I haz the power, 2012-11-28 00:20:59]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/bauerb@chromium.org/11417003/10

[commit-bot: I haz the power, 2012-11-28 13:25:24]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/bauerb@chromium.org/11417003/10

[commit-bot: I haz the power, 2012-11-28 14:45:49]

Change committed as 169938