Update the Ready Mode UI in response to feedback from UI leads, Chrome Frame ...

Update the Ready Mode UI in response to feedback from UI leads, Chrome Frame team.

BUG=None
TEST=chrome_frame_unittests --gtest_filter=Ready*

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=72366

[erikwright (departed), 2011-01-24 06:58:38]

http://codereview.chromium.org/6314016/diff/1/chrome_frame/ready_mode/interna...
File chrome_frame/ready_mode/internal/ready_prompt_content.cc (right):

http://codereview.chromium.org/6314016/diff/1/chrome_frame/ready_mode/interna...
chrome_frame/ready_mode/internal/ready_prompt_content.cc:6: 
Oops. This is missing atlbase.h, atlwin.h, and Exdisp.h.

http://codereview.chromium.org/6314016/diff/1/chrome_frame/ready_mode/interna...
File chrome_frame/ready_mode/internal/ready_prompt_window.cc (right):

http://codereview.chromium.org/6314016/diff/1/chrome_frame/ready_mode/interna...
chrome_frame/ready_mode/internal/ready_prompt_window.cc:10: // These seem to be
required by atlctrlx?
Anyone know why atlctrlx.h is requiring min and max in the global namespace and
where I'm expected to get them from?

[robertshield, 2011-01-24 16:10:21]

http://codereview.chromium.org/6314016/diff/1/chrome/app/google_chrome_string...
File chrome/app/google_chrome_strings.grd (right):

http://codereview.chromium.org/6314016/diff/1/chrome/app/google_chrome_string...
chrome/app/google_chrome_strings.grd:212: This site recommends Google Chrome
Frame (already installed).
This string doesn't match the one in the .rc file. Should it?

http://codereview.chromium.org/6314016/diff/1/chrome_frame/ready_mode/interna...
File chrome_frame/ready_mode/internal/ready_prompt_content.cc (right):

http://codereview.chromium.org/6314016/diff/1/chrome_frame/ready_mode/interna...
chrome_frame/ready_mode/internal/ready_prompt_content.cc:6: 
On 2011/01/24 06:58:39, erikwright wrote:
> Oops. This is missing atlbase.h, atlwin.h, and Exdisp.h.

Add them? ;-)

http://codereview.chromium.org/6314016/diff/1/chrome_frame/ready_mode/interna...
chrome_frame/ready_mode/internal/ready_prompt_content.cc:31: ReadyPromptWindow*
new_window_ = new ReadyPromptWindow();
new_window_ is not a member so shouldn't have the trailing underscore.

http://codereview.chromium.org/6314016/diff/1/chrome_frame/ready_mode/interna...
File chrome_frame/ready_mode/internal/ready_prompt_window.cc (right):

http://codereview.chromium.org/6314016/diff/1/chrome_frame/ready_mode/interna...
chrome_frame/ready_mode/internal/ready_prompt_window.cc:8: #include <atlctrls.h>
I think you want to include atlapp.h and atlwin.h first. (If you look at the top
of the headers, they tell you which files they need including).

http://codereview.chromium.org/6314016/diff/1/chrome_frame/ready_mode/interna...
chrome_frame/ready_mode/internal/ready_prompt_window.cc:10: // These seem to be
required by atlctrlx?
On 2011/01/24 06:58:39, erikwright wrote:
> Anyone know why atlctrlx.h is requiring min and max in the global namespace
and
> where I'm expected to get them from?

Follow the above comment, and probably won't need this.

http://codereview.chromium.org/6314016/diff/1/chrome_frame/ready_mode/interna...
chrome_frame/ready_mode/internal/ready_prompt_window.cc:42: DCHECK(web_browser_
== NULL);
if Initialize can't be called twice, consider having an intialized_ var that
gets set and DCHECKing on that instead of having all these doubled checks.

http://codereview.chromium.org/6314016/diff/1/chrome_frame/ready_mode/interna...
File chrome_frame/ready_mode/internal/ready_prompt_window.h (right):

http://codereview.chromium.org/6314016/diff/1/chrome_frame/ready_mode/interna...
chrome_frame/ready_mode/internal/ready_prompt_window.h:39: virtual
~ReadyPromptWindow();
It looks like this object shouldn't be created on the stack (given the delete
call in Initialize). As such, the destructor can be marked private to prevent
accidental stack creation. (More Effective C++, item 27).

http://codereview.chromium.org/6314016/diff/1/chrome_frame/ready_mode/interna...
chrome_frame/ready_mode/internal/ready_prompt_window.h:54: IWebBrowser2*
web_browser);
I should have made this comment on an earlier review, but since I'm here, I'll
nag now :-)

It looks like this object manages its own life-cycle. As such, having the caller
new() it up and then hand over ownership (or not) back to the instance in
Initialize feels awkward. 

Why not simply have a static factory method along the lines of

base::WeakPtr<ReadyPromptWindow> CreateReadyPromptWindow(...)

[erikwright (departed), 2011-01-24 17:14:01]

OK. A lot more polished, still will do a bit of manual testing, but hoping to
commit before 13h30 if possible.

http://codereview.chromium.org/6314016/diff/1/chrome/app/google_chrome_string...
File chrome/app/google_chrome_strings.grd (right):

http://codereview.chromium.org/6314016/diff/1/chrome/app/google_chrome_string...
chrome/app/google_chrome_strings.grd:212: This site recommends Google Chrome
Frame (already installed).
On 2011/01/24 16:10:21, robertshield wrote:
> This string doesn't match the one in the .rc file. Should it?

No. Amit asked to have two strings defined, so we could defer the decision of
which to use. This is the backup.

http://codereview.chromium.org/6314016/diff/1/chrome_frame/ready_mode/interna...
File chrome_frame/ready_mode/internal/ready_prompt_content.cc (right):

http://codereview.chromium.org/6314016/diff/1/chrome_frame/ready_mode/interna...
chrome_frame/ready_mode/internal/ready_prompt_content.cc:6: 
On 2011/01/24 16:10:21, robertshield wrote:
> On 2011/01/24 06:58:39, erikwright wrote:
> > Oops. This is missing atlbase.h, atlwin.h, and Exdisp.h.
> 
> Add them? ;-)

Yeah. Wanted to get this out for initial feedback ASAP. My workstation crashed
before I discovered this.

Anyway, not needed now.

http://codereview.chromium.org/6314016/diff/1/chrome_frame/ready_mode/interna...
File chrome_frame/ready_mode/internal/ready_prompt_window.cc (right):

http://codereview.chromium.org/6314016/diff/1/chrome_frame/ready_mode/interna...
chrome_frame/ready_mode/internal/ready_prompt_window.cc:8: #include <atlctrls.h>
On 2011/01/24 16:10:21, robertshield wrote:
> I think you want to include atlapp.h and atlwin.h first. (If you look at the
top
> of the headers, they tell you which files they need including).

They are already in ready_prompt_window.h (the declaration for this class, so I
assume it's not standard practice to repeat them here).

http://codereview.chromium.org/6314016/diff/1/chrome_frame/ready_mode/interna...
chrome_frame/ready_mode/internal/ready_prompt_window.cc:10: // These seem to be
required by atlctrlx?
On 2011/01/24 16:10:21, robertshield wrote:
> On 2011/01/24 06:58:39, erikwright wrote:
> > Anyone know why atlctrlx.h is requiring min and max in the global namespace
> and
> > where I'm expected to get them from?
> 
> Follow the above comment, and probably won't need this.

Sadly, I still get the following:


1>s:\src\chromium\src\third_party\wtl\include\atlctrlx.h(4190) : error C3861:
'min': identifier not found
1>        s:\src\chromium\src\third_party\wtl\include\atlctrlx.h(4137) : while
compiling class template member function 'void
WTL::CTabViewImpl<T>::BuildWindowMenu(HMENU,int,bool,bool,bool,bool)'
1>        with
1>        [
1>            T=WTL::CTabView
1>        ]
1>        s:\src\chromium\src\third_party\wtl\include\atlctrlx.h(4817) : see
reference to class template instantiation 'WTL::CTabViewImpl<T>' being compiled
1>        with
1>        [
1>            T=WTL::CTabView
1>        ]
1>s:\src\chromium\src\third_party\wtl\include\atlctrlx.h(4190) : error C3861:
'min': identifier not found

http://codereview.chromium.org/6314016/diff/1/chrome_frame/ready_mode/interna...
chrome_frame/ready_mode/internal/ready_prompt_window.cc:42: DCHECK(web_browser_
== NULL);
On 2011/01/24 16:10:21, robertshield wrote:
> if Initialize can't be called twice, consider having an intialized_ var that
> gets set and DCHECKing on that instead of having all these doubled checks.

I think this is resolved with the change to factory method.

http://codereview.chromium.org/6314016/diff/1/chrome_frame/ready_mode/interna...
File chrome_frame/ready_mode/internal/ready_prompt_window.h (right):

http://codereview.chromium.org/6314016/diff/1/chrome_frame/ready_mode/interna...
chrome_frame/ready_mode/internal/ready_prompt_window.h:39: virtual
~ReadyPromptWindow();
On 2011/01/24 16:10:21, robertshield wrote:
> It looks like this object shouldn't be created on the stack (given the delete
> call in Initialize). As such, the destructor can be marked private to prevent
> accidental stack creation. (More Effective C++, item 27).

Done.

http://codereview.chromium.org/6314016/diff/1/chrome_frame/ready_mode/interna...
chrome_frame/ready_mode/internal/ready_prompt_window.h:54: IWebBrowser2*
web_browser);
On 2011/01/24 16:10:21, robertshield wrote:
> I should have made this comment on an earlier review, but since I'm here, I'll
> nag now :-)
> 
> It looks like this object manages its own life-cycle. As such, having the
caller
> new() it up and then hand over ownership (or not) back to the instance in
> Initialize feels awkward. 
> 
> Why not simply have a static factory method along the lines of
> 
> base::WeakPtr<ReadyPromptWindow> CreateReadyPromptWindow(...)

Done.

[erikwright (departed), 2011-01-24 17:54:18]

Did a gclient sync due to try failure. No changes, though, other than the
updated base revision.

[tommi (sloooow) - chröme, 2011-01-24 17:54:22]

looks pretty good to me.  I'll give it another go over.  Here's what I got so
far.

http://codereview.chromium.org/6314016/diff/10001/chrome_frame/ready_mode/int...
File chrome_frame/ready_mode/internal/ready_prompt_window.cc (right):

http://codereview.chromium.org/6314016/diff/10001/chrome_frame/ready_mode/int...
chrome_frame/ready_mode/internal/ready_prompt_window.cc:10: // These seem to be
required by atlctrlx?
include atlbase first?

http://codereview.chromium.org/6314016/diff/10001/chrome_frame/ready_mode/int...
chrome_frame/ready_mode/internal/ready_prompt_window.cc:53: return
base::WeakPtr<ReadyPromptWindow>();
will instance get deleted?

http://codereview.chromium.org/6314016/diff/10001/chrome_frame/ready_mode/int...
chrome_frame/ready_mode/internal/ready_prompt_window.cc:58: ATL::CWindow rte =
instance->GetDlgItem(IDC_PROMPT_LINK);
I don't think you need the ATL:: prefix

http://codereview.chromium.org/6314016/diff/10001/chrome_frame/ready_mode/int...
chrome_frame/ready_mode/internal/ready_prompt_window.cc:103: LRESULT
ReadyPromptWindow::OnLearnMore(WORD /*wParam*/,
uncomment arguments and remove hungarian (for other window handlers too)

http://codereview.chromium.org/6314016/diff/10001/chrome_frame/ready_mode/int...
File chrome_frame/ready_mode/internal/ready_prompt_window.h (right):

http://codereview.chromium.org/6314016/diff/10001/chrome_frame/ready_mode/int...
chrome_frame/ready_mode/internal/ready_prompt_window.h:91: LRESULT
OnLearnMore(WORD wParam, LPNMHDR lParam, BOOL& bHandled);  // NOLINT
remove all hungarian

http://codereview.chromium.org/6314016/diff/10001/chrome_frame/ready_mode/rea...
File chrome_frame/ready_mode/ready_mode.cc (right):

http://codereview.chromium.org/6314016/diff/10001/chrome_frame/ready_mode/rea...
chrome_frame/ready_mode/ready_mode.cc:108: web_browser->AddRef();
use the = operator?  (it comes from the scoped_refptr class)

[tommi (sloooow) - chröme, 2011-01-24 18:06:25]

OK.

[erikwright (departed), 2011-01-24 18:55:19]

http://codereview.chromium.org/6314016/diff/10001/chrome_frame/ready_mode/int...
File chrome_frame/ready_mode/internal/ready_prompt_window.cc (right):

http://codereview.chromium.org/6314016/diff/10001/chrome_frame/ready_mode/int...
chrome_frame/ready_mode/internal/ready_prompt_window.cc:10: // These seem to be
required by atlctrlx?
On 2011/01/24 17:54:23, tommi wrote:
> include atlbase first?

It's done transitively in ready_prompt_window.h

http://codereview.chromium.org/6314016/diff/10001/chrome_frame/ready_mode/int...
chrome_frame/ready_mode/internal/ready_prompt_window.cc:53: return
base::WeakPtr<ReadyPromptWindow>();
On 2011/01/24 17:54:23, tommi wrote:
> will instance get deleted?

Yes - it's a weak ptr with no other references.

http://codereview.chromium.org/6314016/diff/10001/chrome_frame/ready_mode/int...
chrome_frame/ready_mode/internal/ready_prompt_window.cc:58: ATL::CWindow rte =
instance->GetDlgItem(IDC_PROMPT_LINK);
On 2011/01/24 17:54:23, tommi wrote:
> I don't think you need the ATL:: prefix

Done.

http://codereview.chromium.org/6314016/diff/10001/chrome_frame/ready_mode/int...
chrome_frame/ready_mode/internal/ready_prompt_window.cc:103: LRESULT
ReadyPromptWindow::OnLearnMore(WORD /*wParam*/,
On 2011/01/24 17:54:23, tommi wrote:
> uncomment arguments and remove hungarian (for other window handlers too)

The names correspond to the function 'prototypes' as defined in ATL crack. There
is a 1->1 relationship, and I think it makes sense to keep the names to make the
relationship obvious.
Commenting unused arguments is in accordance with style guide:

http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Functi...

http://codereview.chromium.org/6314016/diff/10001/chrome_frame/ready_mode/int...
File chrome_frame/ready_mode/internal/ready_prompt_window.h (right):

http://codereview.chromium.org/6314016/diff/10001/chrome_frame/ready_mode/int...
chrome_frame/ready_mode/internal/ready_prompt_window.h:91: LRESULT
OnLearnMore(WORD wParam, LPNMHDR lParam, BOOL& bHandled);  // NOLINT
On 2011/01/24 17:54:23, tommi wrote:
> remove all hungarian

See comment in the implementation file.

http://codereview.chromium.org/6314016/diff/10001/chrome_frame/ready_mode/rea...
File chrome_frame/ready_mode/ready_mode.cc (right):

http://codereview.chromium.org/6314016/diff/10001/chrome_frame/ready_mode/rea...
chrome_frame/ready_mode/ready_mode.cc:108: web_browser->AddRef();
On 2011/01/24 17:54:23, tommi wrote:
> use the = operator?  (it comes from the scoped_refptr class)

Done.

[robertshield, 2011-01-24 19:26:40]

LGTM