Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
4766003:
Preview CL for adding an Infobar facility to Google Chrome Frame.... (Closed)
|
Created:
10 years, 1 month ago by erikwright (departed) Modified:
9 years, 7 months ago CC:
ananta Base URL:
svn://svn.chromium.org/chrome/trunk/src/ Visibility:
Public. |
DescriptionAdd an infobar facility to Chrome Frame. Allows clients to display and hide arbitrary content at the top or botom of an IE tab.
BUG=None
TEST=chrome_frame_unittests --gtest_filter=Infobars*
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=68176
Patch Set 1 #
Total comments: 31
Patch Set 2 : '' #Patch Set 3 : '' #
Total comments: 74
Patch Set 4 : '' #Patch Set 5 : '' #
Total comments: 27
Patch Set 6 : '' #Patch Set 7 : '' #Messages
Total messages: 11 (0 generated)
Here are some style comments. I have nothing to say about ATL and the like. http://codereview.chromium.org/4766003/diff/1/chrome_frame/bho.cc File chrome_frame/bho.cc (right): http://codereview.chromium.org/4766003/diff/1/chrome_frame/bho.cc#newcode17 chrome_frame/bho.cc:17: #include "chrome_frame/ready_prompt.h" Sort. http://codereview.chromium.org/4766003/diff/1/chrome_frame/bho.cc#newcode170 chrome_frame/bho.cc:170: delete content; Making callers remember to delete the instance if Show fails seems tricky. How about making Show take a scoped_ptr<InfobarContent>* that it release()es on success? http://codereview.chromium.org/4766003/diff/1/chrome_frame/bho.cc#newcode172 chrome_frame/bho.cc:172: } I see lots of checking for unexpected return codes here, but no logging. How will failures be diagnosed? http://codereview.chromium.org/4766003/diff/1/chrome_frame/infobar_manager.cc File chrome_frame/infobar_manager.cc (right): http://codereview.chromium.org/4766003/diff/1/chrome_frame/infobar_manager.cc... chrome_frame/infobar_manager.cc:171: if (type < FIRST_INFOBAR_TYPE || type >= END_OF_INFOBAR_TYPE || This looks like programmer error. Change to DCHECK if so. http://codereview.chromium.org/4766003/diff/1/chrome_frame/infobar_manager.cc... chrome_frame/infobar_manager.cc:172: infobars_[type] == NULL) { How about this one, programmer error or something that could really happen? If the former, consider getting rid of return values for all functions that only fail on programmer error (in which case use DCHECK). If the latter, consider adding VLOG or somesuch to make the condition(s) diagnosable. http://codereview.chromium.org/4766003/diff/1/chrome_frame/infobar_manager.cc... chrome_frame/infobar_manager.cc:209: if (NULL == window_handle) { I prefer this style of comparison. For consistency, consider making a sweep for (foo == NULL), (!foo) and other equivalent expressions and change them to this style. Likewise for the inverse (NULL != foo). http://codereview.chromium.org/4766003/diff/1/chrome_frame/infobar_window.cc File chrome_frame/infobar_window.cc (right): http://codereview.chromium.org/4766003/diff/1/chrome_frame/infobar_window.cc#... chrome_frame/infobar_window.cc:16: const UINT kInfobarSlidingTimerInterval = 50U; Consider putting "ms" in the constant name so that it's obvious everywhere that it's in ms? http://codereview.chromium.org/4766003/diff/1/chrome_frame/infobar_window.cc#... chrome_frame/infobar_window.cc:25: : type_(type), Although I like spreading the initializer list out like this, I think the style guide prefers to not have so much vertical space taken up. http://codereview.chromium.org/4766003/diff/1/chrome_frame/infobar_window.h File chrome_frame/infobar_window.h (right): http://codereview.chromium.org/4766003/diff/1/chrome_frame/infobar_window.h#n... chrome_frame/infobar_window.h:27: END_OF_INFOBAR_TYPE = 2 The way this value is used, I would find it more natural for it to be called a COUNT. http://codereview.chromium.org/4766003/diff/1/chrome_frame/infobar_window.h#n... chrome_frame/infobar_window.h:77: FrameImpl(InfobarWindow* infobar_window) explicit http://codereview.chromium.org/4766003/diff/1/chrome_frame/infobar_window.h#n... chrome_frame/infobar_window.h:85: InfobarWindow* infobar_window_; DISALLOW_COPY_AND_ASSIGN? http://codereview.chromium.org/4766003/diff/1/chrome_frame/ready_prompt.cc File chrome_frame/ready_prompt.cc (right): http://codereview.chromium.org/4766003/diff/1/chrome_frame/ready_prompt.cc#ne... chrome_frame/ready_prompt.cc:5: } newline after this http://codereview.chromium.org/4766003/diff/1/chrome_frame/ready_prompt.cc#ne... chrome_frame/ready_prompt.cc:7: if (frame_ && frame_ != frame) { Some single-statement ifs in the CL have braces, some don't. Consider picking one style and using everywhere. It seems that the codebase as a whole isn't consistent, but I think an individual CL introducing new code should be. http://codereview.chromium.org/4766003/diff/1/chrome_frame/ready_prompt.cc#ne... chrome_frame/ready_prompt.cc:14: return E_UNEXPECTED; Is expected that frame_ == frame upon E_UNEXPECTED (no pun intended)?
Still looking at this CL. While discussing this with Amit, an interesting point came up. We already add the chromeframe string to the user agent in the outgoing requests issued by IE. A site which sends in content with the meta tag could return chrome specific content which would be unusable in IE anyway. We could probably render this in Chrome directly and display an infobar in chrome using tested code. We could turn off chrome frame for the user if the user ignored the info bar which would work on similar lines as the proposal for IE. -Ananta
We don't need a generic info bar implementation in this case. So it'll be better to have a very basic info bar implementation (for e.g. no slide in) with much less code. http://codereview.chromium.org/4766003/diff/1/chrome_frame/bho.cc File chrome_frame/bho.cc (right): http://codereview.chromium.org/4766003/diff/1/chrome_frame/bho.cc#newcode156 chrome_frame/bho.cc:156: ScopedComPtr<IServiceProvider> service_provider; Check for whether or not to show infobar is still in works, right? Also, check out DoQueryService. http://codereview.chromium.org/4766003/diff/1/chrome_frame/infobar_manager.cc File chrome_frame/infobar_manager.cc (right): http://codereview.chromium.org/4766003/diff/1/chrome_frame/infobar_manager.cc... chrome_frame/infobar_manager.cc:161: for (int index = 0; index < END_OF_INFOBAR_TYPE; ++index) { Feels like an overkill for our rudimentary infobar requirements. We can probably do away with the manager class and fold necessary 'manager' functionality into a INFOBAR_TOP style infobar. http://codereview.chromium.org/4766003/diff/1/chrome_frame/ready_prompt.cc File chrome_frame/ready_prompt.cc (right): http://codereview.chromium.org/4766003/diff/1/chrome_frame/ready_prompt.cc#ne... chrome_frame/ready_prompt.cc:42: SetWindowPos(HWND_TOP, &dimensions, SWP_NOMOVE | SWP_NOSIZE | SWP_SHOWWINDOW); nit: I think you can combine move here if you remove SWP_NOMOVE
FYI, I don't know why it is not showing EOLs at the end of files... they appear locally. TODO is mainly to write some tests. Also, I would prefer to simplify the InfobarContent::Reset thing to just be deletion. If an InfobarContent contains something that can't be deleted, it should orphan that thing upon deletion and provide some other means for recovering the resource (i.e., a CWindow cleared in its OnFinalMessage). http://codereview.chromium.org/4766003/diff/1/chrome_frame/bho.cc File chrome_frame/bho.cc (right): http://codereview.chromium.org/4766003/diff/1/chrome_frame/bho.cc#newcode156 chrome_frame/bho.cc:156: ScopedComPtr<IServiceProvider> service_provider; On 2010/11/12 00:04:49, amit wrote: > Check for whether or not to show infobar is still in works, right? Also, check > out DoQueryService. Yes. Now factored out into a separate CL. http://codereview.chromium.org/4766003/diff/1/chrome_frame/bho.cc#newcode170 chrome_frame/bho.cc:170: delete content; On 2010/11/10 17:59:29, grt wrote: > Making callers remember to delete the instance if Show fails seems tricky. How > about making Show take a scoped_ptr<InfobarContent>* that it release()es on > success? It now unequivocally takes ownership upon Show. http://codereview.chromium.org/4766003/diff/1/chrome_frame/bho.cc#newcode172 chrome_frame/bho.cc:172: } On 2010/11/10 17:59:29, grt wrote: > I see lots of checking for unexpected return codes here, but no logging. How > will failures be diagnosed? Yes. This is just a sample integration for now. http://codereview.chromium.org/4766003/diff/1/chrome_frame/infobar_manager.cc File chrome_frame/infobar_manager.cc (right): http://codereview.chromium.org/4766003/diff/1/chrome_frame/infobar_manager.cc... chrome_frame/infobar_manager.cc:161: for (int index = 0; index < END_OF_INFOBAR_TYPE; ++index) { On 2010/11/12 00:04:49, amit wrote: > Feels like an overkill for our rudimentary infobar requirements. We can > probably do away with the manager class and fold necessary 'manager' > functionality into a INFOBAR_TOP style infobar. The TOP vs BOTTOM and sliding is unnecessary, but also not the tricky bit. I prefer to keep it so that my implementation can easily replace the CEEE one. The hard bit is the window lifecycle. For that, I don't think there is a working, testable, and comprehensible solution that can be wrapped into a single-file / single-class implementation. http://codereview.chromium.org/4766003/diff/1/chrome_frame/infobar_manager.cc... chrome_frame/infobar_manager.cc:171: if (type < FIRST_INFOBAR_TYPE || type >= END_OF_INFOBAR_TYPE || On 2010/11/10 17:59:29, grt wrote: > This looks like programmer error. Change to DCHECK if so. Yes. A relic of the original code. Ideally, I'd like to get rid of this passing of a type enum altogether in the final CL. http://codereview.chromium.org/4766003/diff/1/chrome_frame/infobar_manager.cc... chrome_frame/infobar_manager.cc:172: infobars_[type] == NULL) { On 2010/11/10 17:59:29, grt wrote: > How about this one, programmer error or something that could really happen? If > the former, consider getting rid of return values for all functions that only > fail on programmer error (in which case use DCHECK). If the latter, consider > adding VLOG or somesuch to make the condition(s) diagnosable. Yes. I'm updating it to only return success or failure in cases where it can unexpectedly fail (i.e., Windows returns a failure). http://codereview.chromium.org/4766003/diff/1/chrome_frame/infobar_manager.cc... chrome_frame/infobar_manager.cc:209: if (NULL == window_handle) { On 2010/11/10 17:59:29, grt wrote: > I prefer this style of comparison. For consistency, consider making a sweep for > (foo == NULL), (!foo) and other equivalent expressions and change them to this > style. Likewise for the inverse (NULL != foo). Agreed. Not the least because scoped_ptr does not allow evaluation as a boolean. Although it will be the reverse (to meet coding style). http://codereview.chromium.org/4766003/diff/1/chrome_frame/infobar_window.cc File chrome_frame/infobar_window.cc (right): http://codereview.chromium.org/4766003/diff/1/chrome_frame/infobar_window.cc#... chrome_frame/infobar_window.cc:16: const UINT kInfobarSlidingTimerInterval = 50U; On 2010/11/10 17:59:29, grt wrote: > Consider putting "ms" in the constant name so that it's obvious everywhere that > it's in ms? Agreed. http://codereview.chromium.org/4766003/diff/1/chrome_frame/infobar_window.cc#... chrome_frame/infobar_window.cc:25: : type_(type), On 2010/11/10 17:59:29, grt wrote: > Although I like spreading the initializer list out like this, I think the style > guide prefers to not have so much vertical space taken up. It does prefer to avoid unnecessary whitespace, but it also specifically mandates that, when a list of parameters or initializers must be split on more than one line, each element appears on its own line (with an exception if sets of elements are clearly related, i.e., width and height). http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Constr... http://codereview.chromium.org/4766003/diff/1/chrome_frame/infobar_window.h File chrome_frame/infobar_window.h (right): http://codereview.chromium.org/4766003/diff/1/chrome_frame/infobar_window.h#n... chrome_frame/infobar_window.h:27: END_OF_INFOBAR_TYPE = 2 On 2010/11/10 17:59:29, grt wrote: > The way this value is used, I would find it more natural for it to be called a > COUNT. Hope to get rid of it altogether. http://codereview.chromium.org/4766003/diff/1/chrome_frame/infobar_window.h#n... chrome_frame/infobar_window.h:77: FrameImpl(InfobarWindow* infobar_window) On 2010/11/10 17:59:29, grt wrote: > explicit Thanks. http://codereview.chromium.org/4766003/diff/1/chrome_frame/infobar_window.h#n... chrome_frame/infobar_window.h:85: InfobarWindow* infobar_window_; On 2010/11/10 17:59:29, grt wrote: > DISALLOW_COPY_AND_ASSIGN? Thanks. http://codereview.chromium.org/4766003/diff/1/chrome_frame/ready_prompt.cc File chrome_frame/ready_prompt.cc (right): http://codereview.chromium.org/4766003/diff/1/chrome_frame/ready_prompt.cc#ne... chrome_frame/ready_prompt.cc:7: if (frame_ && frame_ != frame) { On 2010/11/10 17:59:29, grt wrote: > Some single-statement ifs in the CL have braces, some don't. Consider picking > one style and using everywhere. It seems that the codebase as a whole isn't > consistent, but I think an individual CL introducing new code should be. Agreed. Planning a sweep at the end. http://codereview.chromium.org/4766003/diff/1/chrome_frame/ready_prompt.cc#ne... chrome_frame/ready_prompt.cc:14: return E_UNEXPECTED; On 2010/11/10 17:59:29, grt wrote: > Is expected that frame_ == frame upon E_UNEXPECTED (no pun intended)? No. already fixed in working copy. http://codereview.chromium.org/4766003/diff/25001/chrome_frame/chrome_frame.gyp File chrome_frame/chrome_frame.gyp (right): http://codereview.chromium.org/4766003/diff/25001/chrome_frame/chrome_frame.g... chrome_frame/chrome_frame.gyp:762: 'ready_prompt.cc', Ignore these (will go in a separate CL). http://codereview.chromium.org/4766003/diff/25001/chrome_frame/infobars/inter... File chrome_frame/infobars/internal/displaced_window.h (right): http://codereview.chromium.org/4766003/diff/25001/chrome_frame/infobars/inter... chrome_frame/infobars/internal/displaced_window.h:62: // Schedules a deferred task on the window message loop of this instance's To verify whether or not this really needs to be deferred. If not (my suspicion), I'll get rid of the task queue altogether.
I am having a little difficulty in following the design. I think here's what
needs to be done for the info bar. Please let me know if some steps are
missing.
Assumptions: Info bar can be contained by a BHO instance since it's per tab.
Also, it is not going to outlive BHO instance.
- When it is time to show info bar, Get to the mshtml container hwnd (preferably
using IWebBrowser2::get_HWND?)
- InfobarContainer is the manager that subclasses the tab (mshtml container)
window. Infobar is the instance of info bar widow.
- Attach InfobarContainer to the container hwnd:
- Create Infobar with initial size of 0
- Subclass container
- handle WM_NCCALCSIZE to make space and resize Infobar accordingly
- SetWindowPos of SWP_FRAMECHANGED to force layout now
- Dismiss Infobar
- BeforeNavigate in BHO
- User dismissed
- Close the Infobar, notify container.
- Keep the InfobarContainer around, it's not safe to remove subclass.
- Show again:
- Subclass is in place, create Infobar again and force layout.
- Tab closed
- Cleanup Infobar in WM_DESTROY of subclass (InfobarContainer)
- Cleanup InfobarContainer in BHO SetSite(NULL)
I can see parts of this in DisplacedWindowManager and HostWindowManager but it's
not clear why they are separate. InfobarWindow maps nicely to InfoBar.
http://codereview.chromium.org/4766003/diff/25001/chrome_frame/infobars/inter...
File chrome_frame/infobars/internal/displaced_window.cc (right):
http://codereview.chromium.org/4766003/diff/25001/chrome_frame/infobars/inter...
chrome_frame/infobars/internal/displaced_window.cc:66: // Whether
calc_valid_rects is true or false, we could treat beginning of
This is somewhat unlikely but if calc_valid_rects is TRUE and the original
window proc returns valid rectangles overlapping with infobar it could lead to
painting issues during sizing. Please double check the sizing, moving etc. with
info bar displayed. Alternatively, you can assert that IntersectRect of infobar
rect and the other two rects should be empty.
nice job. the code is easily readable and neat. http://codereview.chromium.org/4766003/diff/25001/chrome_frame/infobars/conte... File chrome_frame/infobars/content.h (right): http://codereview.chromium.org/4766003/diff/25001/chrome_frame/infobars/conte... chrome_frame/infobars/content.h:10: class InfobarContent { add a few comments that explain the purpose of these interfaces http://codereview.chromium.org/4766003/diff/25001/chrome_frame/infobars/inter... File chrome_frame/infobars/internal/displaced_window.cc (right): http://codereview.chromium.org/4766003/diff/25001/chrome_frame/infobars/inter... chrome_frame/infobars/internal/displaced_window.cc:17: DCHECK(delegate != NULL); fyi - when you run gcl lint on this change list, you'll probably get recommendations to use DCHECK_NE and DCHECK_EQ for these. http://codereview.chromium.org/4766003/diff/25001/chrome_frame/infobars/inter... chrome_frame/infobars/internal/displaced_window.cc:21: PinModule(); add a comment for why this class requires pinning http://codereview.chromium.org/4766003/diff/25001/chrome_frame/infobars/inter... chrome_frame/infobars/internal/displaced_window.cc:37: task_queue_.Push(task); since there's no locking, can we DCHECK that this call is being made on the correct thread? E.g. you can add a utility method that exists only when DCHECK does that would look something like this: bool CurrentThreadIsWindowThread(HWND hwnd) { return ::GetCurrentThreadId() == GetWindowThreadProcessId(hwnd, NULL); } and then use it in methods like these: DCHECK(CurrentThreadIsWindowThread(m_hWnd)); http://codereview.chromium.org/4766003/diff/25001/chrome_frame/infobars/inter... chrome_frame/infobars/internal/displaced_window.cc:38: if (IsWindow()) Is there a chance that the window doesn't exist at this point? if not, maybe you could add else NOTREACHED(); http://codereview.chromium.org/4766003/diff/25001/chrome_frame/infobars/inter... chrome_frame/infobars/internal/displaced_window.cc:43: WPARAM wparam, indent either 4 spaces or to the same indent as the first parameter http://codereview.chromium.org/4766003/diff/25001/chrome_frame/infobars/inter... chrome_frame/infobars/internal/displaced_window.cc:54: if (IsWindow()) I think the coding guidelines call for {} when the body of a condition spans multiple lines (even if there's only one semicolon). maybe I'm wrong http://codereview.chromium.org/4766003/diff/25001/chrome_frame/infobars/inter... File chrome_frame/infobars/internal/displaced_window.h (right): http://codereview.chromium.org/4766003/diff/25001/chrome_frame/infobars/inter... chrome_frame/infobars/internal/displaced_window.h:59: TM_RUN_TASK_QUEUE = WM_USER + 600, use WM_APP instead of WM_USER. http://codereview.chromium.org/4766003/diff/25001/chrome_frame/infobars/inter... chrome_frame/infobars/internal/displaced_window.h:74: // The size of the displaced window is being calculated. Allow add empty lines between functions http://codereview.chromium.org/4766003/diff/25001/chrome_frame/infobars/inter... chrome_frame/infobars/internal/displaced_window.h:83: void OnFinalMessage(HWND); virtual? btw, even though you declare it as private, the function is really public since the interface it is declared in, declares it as such. http://codereview.chromium.org/4766003/diff/25001/chrome_frame/infobars/inter... File chrome_frame/infobars/internal/host_window.cc (right): http://codereview.chromium.org/4766003/diff/25001/chrome_frame/infobars/inter... chrome_frame/infobars/internal/host_window.cc:67: CHECK(displaced_window_manager_ == NULL); to be clear - CHECK() will kill IE for the user if this ever hits - you sure that's intended? http://codereview.chromium.org/4766003/diff/25001/chrome_frame/infobars/inter... chrome_frame/infobars/internal/host_window.cc:84: void HostWindowManager::OnFinalMessage(HWND /*hWnd*/) { remove /* */ http://codereview.chromium.org/4766003/diff/25001/chrome_frame/infobars/inter... chrome_frame/infobars/internal/host_window.cc:89: WPARAM wparam, fix indentation http://codereview.chromium.org/4766003/diff/25001/chrome_frame/infobars/inter... File chrome_frame/infobars/internal/host_window.h (right): http://codereview.chromium.org/4766003/diff/25001/chrome_frame/infobars/inter... chrome_frame/infobars/internal/host_window.h:9: #include <atlapp.h> // Must be included AFTER base. what functionality from atlapp.h do you need? looks like atlwin.h could be sufficient http://codereview.chromium.org/4766003/diff/25001/chrome_frame/infobars/inter... chrome_frame/infobars/internal/host_window.h:10: #include <atlcrack.h> ... and I think atlwin depends on atlcrack... or rather "is addicted to" http://codereview.chromium.org/4766003/diff/25001/chrome_frame/infobars/inter... chrome_frame/infobars/internal/host_window.h:61: virtual void OnFinalMessage(HWND /*hWnd*/); remove /* */ http://codereview.chromium.org/4766003/diff/25001/chrome_frame/infobars/inter... chrome_frame/infobars/internal/host_window.h:69: WM_GET_DELEGATE = WM_APP + 1982 hah, that's the first time an enum made me age conscious! :) http://codereview.chromium.org/4766003/diff/25001/chrome_frame/infobars/inter... chrome_frame/infobars/internal/host_window.h:76: // Finds the window to be displaced and instantiate a DisplacedWindowManager add empty line http://codereview.chromium.org/4766003/diff/25001/chrome_frame/infobars/inter... File chrome_frame/infobars/internal/infobar_window.h (right): http://codereview.chromium.org/4766003/diff/25001/chrome_frame/infobars/inter... chrome_frame/infobars/internal/infobar_window.h:9: #include <atlapp.h> // Must be included AFTER base. same comment for atlapp as before (and I believe for all atlapp includes) http://codereview.chromium.org/4766003/diff/25001/chrome_frame/infobars/inter... chrome_frame/infobars/internal/infobar_window.h:54: // Hides the infobar. I prefer spaces between functions for readability - especially those that have comments. http://codereview.chromium.org/4766003/diff/25001/chrome_frame/infobars/inter... chrome_frame/infobars/internal/infobar_window.h:67: : infobar_window_(infobar_window) { } indent http://codereview.chromium.org/4766003/diff/25001/chrome_frame/infobars/manag... File chrome_frame/infobars/manager.cc (right): http://codereview.chromium.org/4766003/diff/25001/chrome_frame/infobars/manag... chrome_frame/infobars/manager.cc:20: explicit InfobarManagerImpl(HostWindowManager *manager) : manager_(manager) { ...Manager* manager
Looks good; mostly nits below. http://codereview.chromium.org/4766003/diff/25001/chrome_frame/infobars/conte... File chrome_frame/infobars/content.h (right): http://codereview.chromium.org/4766003/diff/25001/chrome_frame/infobars/conte... chrome_frame/infobars/content.h:10: class InfobarContent { Consider a name change for this and other interface classes: Chrome style guide says "The Google style guide encourages (and in some case requires) the use of the "Interface" suffix for pure virtual interfaces. In many cases, we use "Delegate" instead." http://codereview.chromium.org/4766003/diff/25001/chrome_frame/infobars/conte... chrome_frame/infobars/content.h:41: }; I see classes and namespaces terminated with comments like this: }; // class InfobarContent in the codebase (note the two spaces). I find them handy. If you don't object, please add. http://codereview.chromium.org/4766003/diff/25001/chrome_frame/infobars/inter... File chrome_frame/infobars/internal/displaced_window.cc (right): http://codereview.chromium.org/4766003/diff/25001/chrome_frame/infobars/inter... chrome_frame/infobars/internal/displaced_window.cc:43: WPARAM wparam, indentation http://codereview.chromium.org/4766003/diff/25001/chrome_frame/infobars/inter... File chrome_frame/infobars/internal/host_window.cc (right): http://codereview.chromium.org/4766003/diff/25001/chrome_frame/infobars/inter... chrome_frame/infobars/internal/host_window.cc:41: // Variable to hold the class name. The size does not matter as long as it indent http://codereview.chromium.org/4766003/diff/25001/chrome_frame/infobars/inter... chrome_frame/infobars/internal/host_window.cc:75: PinModule(); Are both calls to PinModule (here and in DisplacedWindowManager) needed? Should PinModule only happen if the subclass succeeds? http://codereview.chromium.org/4766003/diff/25001/chrome_frame/infobars/inter... chrome_frame/infobars/internal/host_window.cc:134: if (rect == NULL) why not simply: if (rect != NULL) delegate_->AdjustDisplacedWindowDimensions(rect); ? http://codereview.chromium.org/4766003/diff/25001/chrome_frame/infobars/inter... File chrome_frame/infobars/internal/infobar_window.cc (right): http://codereview.chromium.org/4766003/diff/25001/chrome_frame/infobars/inter... chrome_frame/infobars/internal/infobar_window.cc:8: Include <algorithm> to explicitly pull in std::min/max http://codereview.chromium.org/4766003/diff/25001/chrome_frame/infobars/inter... chrome_frame/infobars/internal/infobar_window.cc:188: ResetContent(); Multiple exits that require the same DoStuffBeforeExiting() calls can cause trouble when others add new early exits but forget the magic cleanup. Could you refactor all occurrences of these in the CL so that there's (ideally) one failure exit that does the proper cleanup? http://codereview.chromium.org/4766003/diff/25001/chrome_frame/infobars/inter... File chrome_frame/infobars/internal/infobar_window.h (right): http://codereview.chromium.org/4766003/diff/25001/chrome_frame/infobars/inter... chrome_frame/infobars/internal/infobar_window.h:18: // InfobarWindow is the window created on the top or bottom of the browser tab Technically speaking, is the InfobarWindow _in_ the browser tab window (i.e., a child of it)? From this comment, I'm not sure if the InfobarWindow obscures part of the tab window ("on top of") or not. http://codereview.chromium.org/4766003/diff/25001/chrome_frame/infobars/inter... chrome_frame/infobars/internal/infobar_window.h:70: virtual HWND GetFrameWindow() { return *infobar_window_; } Just a note: Chrome style guide says not to inline virtual function definitions ("virtual functions should never be declared this way"), although the Google C++ guide allows it. Personally, I'm fine with it as-is. http://codereview.chromium.org/4766003/diff/25001/chrome_frame/infobars/manag... File chrome_frame/infobars/manager.cc (right): http://codereview.chromium.org/4766003/diff/25001/chrome_frame/infobars/manag... chrome_frame/infobars/manager.cc:28: virtual bool Show(InfobarContent *content, InfobarType type) { InfobarContent* http://codereview.chromium.org/4766003/diff/25001/chrome_frame/infobars/manag... chrome_frame/infobars/manager.cc:62: HostWindowManager *manager_; HostWindowManager* http://codereview.chromium.org/4766003/diff/25001/chrome_frame/infobars/manag... File chrome_frame/infobars/manager.h (right): http://codereview.chromium.org/4766003/diff/25001/chrome_frame/infobars/manag... chrome_frame/infobars/manager.h:23: static InfobarManager* Get(HWND tab_window); Please document ownership: does caller own the manager after the call? Can this return NULL?
Further refactored this, and added some unit tests. This CL is now proposed for commit and will only be changed in response to codereview feedback. Thanks! http://codereview.chromium.org/4766003/diff/25001/chrome_frame/infobars/conte... File chrome_frame/infobars/content.h (right): http://codereview.chromium.org/4766003/diff/25001/chrome_frame/infobars/conte... chrome_frame/infobars/content.h:10: class InfobarContent { On 2010/11/25 18:00:13, grt wrote: > Consider a name change for this and other interface classes: Chrome style guide > says "The Google style guide encourages (and in some case requires) the use of > the "Interface" suffix for pure virtual interfaces. In many cases, we use > "Delegate" instead." Thanks. I looked at the Google C++ guide. It doesn't seem to express an opinion as to whether Interface is required (unless there are pure-virtual subclasses). I find it a bit awkward to say "this member is an InfobarContentInterface" - to me, the member is "an InfobarContent". I did make some other changes to the names (mainly, in my opinion, reducing redundancy). Some of them are simply called "Delegate" (nested within the class to which they are a delegate), which I find natural. http://codereview.chromium.org/4766003/diff/25001/chrome_frame/infobars/conte... chrome_frame/infobars/content.h:41: }; On 2010/11/25 18:00:13, grt wrote: > I see classes and namespaces terminated with comments like this: > }; // class InfobarContent > in the codebase (note the two spaces). I find them handy. If you don't object, > please add. Done. http://codereview.chromium.org/4766003/diff/25001/chrome_frame/infobars/inter... File chrome_frame/infobars/internal/displaced_window.cc (right): http://codereview.chromium.org/4766003/diff/25001/chrome_frame/infobars/inter... chrome_frame/infobars/internal/displaced_window.cc:17: DCHECK(delegate != NULL); On 2010/11/24 16:08:24, tommi wrote: > fyi - when you run gcl lint on this change list, you'll probably get > recommendations to use DCHECK_NE and DCHECK_EQ for these. I did run lint. It didn't report this. Furthermore, using DCHECK_NE(delegate, NULL) reports the following: s:\src\chromium\src\base/logging.h(495) : error C2446: '!=' : no conversion from 'const int' to 'DisplacedWindowManager::Delegate *const ' The only option would be to write DCHECK_NULL and DCHECK_NOT_NULL... maybe a good idea for a later date? http://codereview.chromium.org/4766003/diff/25001/chrome_frame/infobars/inter... chrome_frame/infobars/internal/displaced_window.cc:21: PinModule(); On 2010/11/24 16:08:24, tommi wrote: > add a comment for why this class requires pinning (NB this code moved to subclassing_window.h - I added a comment there). I inherited this from the CEEE implementation. I think it might be required, depending on how IE unloads BHO modules, but I don't know. I think BHOs are more or less guaranteed to stay loaded until near the end of the browser process' life, and I think that the tab window probably lives that long as well. In that case, it can't hurt to pin, and depending on the order of tab window destruction vs. BHO unloading, might be essential. On the other hand, if: 1) a tab window could be destroyed in the middle of the process life 2) it is guaranteed to be destroyed before BHOs are unloaded 3) there are typical use cases where a BHO might be unloaded mid-process-life Then it might be a mistake to pin the module. Ultimately, I think it's safest to pin. Let me know what you think? http://codereview.chromium.org/4766003/diff/25001/chrome_frame/infobars/inter... chrome_frame/infobars/internal/displaced_window.cc:37: task_queue_.Push(task); On 2010/11/24 16:08:24, tommi wrote: > since there's no locking, can we DCHECK that this call is being made on the > correct thread? > E.g. you can add a utility method that exists only when DCHECK does that would > look something like this: > > bool CurrentThreadIsWindowThread(HWND hwnd) { > return ::GetCurrentThreadId() == GetWindowThreadProcessId(hwnd, NULL); > } > > and then use it in methods like these: > > DCHECK(CurrentThreadIsWindowThread(m_hWnd)); This code no longer exists. But the comment is relevant: Nothing in the infobars implementation is threadsafe, so these checks would have to go everywhere. If you think it's important, I'd suggest putting the checks at the external entry points, which are: InfobarManagerImpl (Show, Hide, HideAll) InfobarWindow::FrameImpl (GetFrameWindow, CloseInfobar) Everything else is internal only (and clearly indicated as such by directory structure) and has no threading, so I think that thread-safety checks within the module would be unnecessary. What do you think? http://codereview.chromium.org/4766003/diff/25001/chrome_frame/infobars/inter... chrome_frame/infobars/internal/displaced_window.cc:38: if (IsWindow()) On 2010/11/24 16:08:24, tommi wrote: > Is there a chance that the window doesn't exist at this point? > if not, maybe you could add > > else > NOTREACHED(); I'm uncertain about what happens when a window is destroyed. Is WM_DESTROY guaranteed to be the last message sent to the window? I assume that IsWindow is true during processing of WM_DESTROY and false afterwards? If my assumptions are correct, then this check is not required because the client of this class is notified when it is destroyed and should no longer invoke on it. I've removed any IsWindow checks that I thought were superfluous, and the basic suite of testcases has not yielded any problems. http://codereview.chromium.org/4766003/diff/25001/chrome_frame/infobars/inter... chrome_frame/infobars/internal/displaced_window.cc:43: WPARAM wparam, On 2010/11/24 16:08:24, tommi wrote: > indent either 4 spaces or to the same indent as the first parameter Done. http://codereview.chromium.org/4766003/diff/25001/chrome_frame/infobars/inter... chrome_frame/infobars/internal/displaced_window.cc:43: WPARAM wparam, On 2010/11/25 18:00:13, grt wrote: > indentation Done. http://codereview.chromium.org/4766003/diff/25001/chrome_frame/infobars/inter... chrome_frame/infobars/internal/displaced_window.cc:54: if (IsWindow()) On 2010/11/24 16:08:24, tommi wrote: > I think the coding guidelines call for {} when the body of a condition spans > multiple lines (even if there's only one semicolon). maybe I'm wrong Done. http://codereview.chromium.org/4766003/diff/25001/chrome_frame/infobars/inter... chrome_frame/infobars/internal/displaced_window.cc:66: // Whether calc_valid_rects is true or false, we could treat beginning of On 2010/11/24 14:21:35, amit wrote: > This is somewhat unlikely but if calc_valid_rects is TRUE and the original > window proc returns valid rectangles overlapping with infobar it could lead to > painting issues during sizing. Please double check the sizing, moving etc. with > info bar displayed. Alternatively, you can assert that IntersectRect of infobar > rect and the other two rects should be empty. Great point. To be honest I didn't fully understand this event before reading you comment. I looked into it further, and here's what I think: 1) If the DefWindowProc returns anything other than WVR_VALIDRECTS, I can essentially do as I do now, which is to say ignore the unused 2nd and 3rd rects and leave the window's align/redraw request intact. 2) In the case of WVR_VALIDRECTS, if we end up adjusting the rect, there is no surefire way to "do the right thing" to respect the original window's intentions. The only solution is to return WVR_REDRAW. Does that seem right to you? http://codereview.chromium.org/4766003/diff/25001/chrome_frame/infobars/inter... File chrome_frame/infobars/internal/displaced_window.h (right): http://codereview.chromium.org/4766003/diff/25001/chrome_frame/infobars/inter... chrome_frame/infobars/internal/displaced_window.h:59: TM_RUN_TASK_QUEUE = WM_USER + 600, On 2010/11/24 16:08:24, tommi wrote: > use WM_APP instead of WM_USER. Now uses RegisterWindowEvent (see subclassing_window.h). http://codereview.chromium.org/4766003/diff/25001/chrome_frame/infobars/inter... chrome_frame/infobars/internal/displaced_window.h:62: // Schedules a deferred task on the window message loop of this instance's On 2010/11/24 06:24:56, erikwright wrote: > To verify whether or not this really needs to be deferred. If not (my > suspicion), I'll get rid of the task queue altogether. Verified that it does not need to be deferred. Yay. http://codereview.chromium.org/4766003/diff/25001/chrome_frame/infobars/inter... chrome_frame/infobars/internal/displaced_window.h:74: // The size of the displaced window is being calculated. Allow On 2010/11/24 16:08:24, tommi wrote: > add empty lines between functions Done. http://codereview.chromium.org/4766003/diff/25001/chrome_frame/infobars/inter... chrome_frame/infobars/internal/displaced_window.h:83: void OnFinalMessage(HWND); On 2010/11/24 16:08:24, tommi wrote: > virtual? > > btw, even though you declare it as private, the function is really public since > the interface it is declared in, declares it as such. Thanks. Simply a mistake that it was in this section. I didn't know about the style-guide rule to include 'virtual' when overriding virtual methods. http://codereview.chromium.org/4766003/diff/25001/chrome_frame/infobars/inter... File chrome_frame/infobars/internal/host_window.cc (right): http://codereview.chromium.org/4766003/diff/25001/chrome_frame/infobars/inter... chrome_frame/infobars/internal/host_window.cc:41: // Variable to hold the class name. The size does not matter as long as it On 2010/11/25 18:00:13, grt wrote: > indent Done. http://codereview.chromium.org/4766003/diff/25001/chrome_frame/infobars/inter... chrome_frame/infobars/internal/host_window.cc:67: CHECK(displaced_window_manager_ == NULL); On 2010/11/24 16:08:24, tommi wrote: > to be clear - CHECK() will kill IE for the user if this ever hits - you sure > that's intended? I think we have to. The alternative is to accept a call into an already destroyed object, which is a crash in the best case and an exploit in the worst. http://codereview.chromium.org/4766003/diff/25001/chrome_frame/infobars/inter... chrome_frame/infobars/internal/host_window.cc:75: PinModule(); On 2010/11/25 18:00:13, grt wrote: > Are both calls to PinModule (here and in DisplacedWindowManager) needed? Should > PinModule only happen if the subclass succeeds? Only one is required, but for the purposes of decoupling, I prefer for the DisplacedWindowManager to not know that the HostWindowManager has also done this. Agreed, moving it after SubclassWindow success makes sense. Relevant code moved to subclassing_window.h http://codereview.chromium.org/4766003/diff/25001/chrome_frame/infobars/inter... chrome_frame/infobars/internal/host_window.cc:84: void HostWindowManager::OnFinalMessage(HWND /*hWnd*/) { On 2010/11/24 16:08:24, tommi wrote: > remove /* */ According to coding style we comment parameter names if unused: http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Functi... http://codereview.chromium.org/4766003/diff/25001/chrome_frame/infobars/inter... chrome_frame/infobars/internal/host_window.cc:89: WPARAM wparam, On 2010/11/24 16:08:24, tommi wrote: > fix indentation Done. http://codereview.chromium.org/4766003/diff/25001/chrome_frame/infobars/inter... chrome_frame/infobars/internal/host_window.cc:134: if (rect == NULL) On 2010/11/25 18:00:13, grt wrote: > why not simply: > if (rect != NULL) > delegate_->AdjustDisplacedWindowDimensions(rect); > ? In fact, I think it's clear from the contract that the pointer should not be null. I added a check on the surface of this call-path (DisplacedWindowManager::OnNcCalcSize) to make sure it's not NULL there. There is also a DCHECK down in InfobarWindow::ReserveSpace (where the pointer is actually de-referenced). Moved up to the top of this file, FYI. http://codereview.chromium.org/4766003/diff/25001/chrome_frame/infobars/inter... File chrome_frame/infobars/internal/host_window.h (right): http://codereview.chromium.org/4766003/diff/25001/chrome_frame/infobars/inter... chrome_frame/infobars/internal/host_window.h:9: #include <atlapp.h> // Must be included AFTER base. On 2010/11/24 16:08:24, tommi wrote: > what functionality from atlapp.h do you need? > looks like atlwin.h could be sufficient CWindowImpl in atlwin.h, which was transiently included. Modified to a direct include. Thanks! (Moved to subclassing_window.h) http://codereview.chromium.org/4766003/diff/25001/chrome_frame/infobars/inter... chrome_frame/infobars/internal/host_window.h:10: #include <atlcrack.h> On 2010/11/24 16:08:24, tommi wrote: > ... and I think atlwin depends on atlcrack... or rather "is addicted to" Yep. Sadly, I need both for CWindowImpl. http://codereview.chromium.org/4766003/diff/25001/chrome_frame/infobars/inter... chrome_frame/infobars/internal/host_window.h:61: virtual void OnFinalMessage(HWND /*hWnd*/); On 2010/11/24 16:08:24, tommi wrote: > remove /* */ Thanks. It's quite confusing that we comment them in the definition but not in the declaration. http://codereview.chromium.org/4766003/diff/25001/chrome_frame/infobars/inter... chrome_frame/infobars/internal/host_window.h:69: WM_GET_DELEGATE = WM_APP + 1982 On 2010/11/24 16:08:24, tommi wrote: > hah, that's the first time an enum made me age conscious! :) Done ;) http://codereview.chromium.org/4766003/diff/25001/chrome_frame/infobars/inter... chrome_frame/infobars/internal/host_window.h:76: // Finds the window to be displaced and instantiate a DisplacedWindowManager On 2010/11/24 16:08:24, tommi wrote: > add empty line Done. http://codereview.chromium.org/4766003/diff/25001/chrome_frame/infobars/inter... File chrome_frame/infobars/internal/infobar_window.cc (right): http://codereview.chromium.org/4766003/diff/25001/chrome_frame/infobars/inter... chrome_frame/infobars/internal/infobar_window.cc:8: On 2010/11/25 18:00:13, grt wrote: > Include <algorithm> to explicitly pull in std::min/max Awesome catch! http://codereview.chromium.org/4766003/diff/25001/chrome_frame/infobars/inter... chrome_frame/infobars/internal/infobar_window.cc:188: ResetContent(); On 2010/11/25 18:00:13, grt wrote: > Multiple exits that require the same DoStuffBeforeExiting() calls can cause > trouble when others add new early exits but forget the magic cleanup. Could you > refactor all occurrences of these in the CL so that there's (ideally) one > failure exit that does the proper cleanup? Yeah, I didn't like it either. Moved to Show(), BTW. http://codereview.chromium.org/4766003/diff/25001/chrome_frame/infobars/inter... File chrome_frame/infobars/internal/infobar_window.h (right): http://codereview.chromium.org/4766003/diff/25001/chrome_frame/infobars/inter... chrome_frame/infobars/internal/infobar_window.h:9: #include <atlapp.h> // Must be included AFTER base. On 2010/11/24 16:08:24, tommi wrote: > same comment for atlapp as before (and I believe for all atlapp includes) Done. (Moved to subclassing_window.h) http://codereview.chromium.org/4766003/diff/25001/chrome_frame/infobars/inter... chrome_frame/infobars/internal/infobar_window.h:18: // InfobarWindow is the window created on the top or bottom of the browser tab On 2010/11/25 18:00:13, grt wrote: > Technically speaking, is the InfobarWindow _in_ the browser tab window (i.e., a > child of it)? From this comment, I'm not sure if the InfobarWindow obscures > part of the tab window ("on top of") or not. Comment revised. Please let me know if it does not answer the question. http://codereview.chromium.org/4766003/diff/25001/chrome_frame/infobars/inter... chrome_frame/infobars/internal/infobar_window.h:54: // Hides the infobar. On 2010/11/24 16:08:24, tommi wrote: > I prefer spaces between functions for readability - especially those that have > comments. Done. http://codereview.chromium.org/4766003/diff/25001/chrome_frame/infobars/inter... chrome_frame/infobars/internal/infobar_window.h:67: : infobar_window_(infobar_window) { } On 2010/11/24 16:08:24, tommi wrote: > indent Done. http://codereview.chromium.org/4766003/diff/25001/chrome_frame/infobars/inter... chrome_frame/infobars/internal/infobar_window.h:70: virtual HWND GetFrameWindow() { return *infobar_window_; } On 2010/11/25 18:00:13, grt wrote: > Just a note: Chrome style guide says not to inline virtual function definitions > ("virtual functions should never be declared this way"), although the Google C++ > guide allows it. Personally, I'm fine with it as-is. Thanks! Given that they can't normally be inlined, and are definitely part of the implementation, I decided to move them to the .cc http://codereview.chromium.org/4766003/diff/25001/chrome_frame/infobars/manag... File chrome_frame/infobars/manager.cc (right): http://codereview.chromium.org/4766003/diff/25001/chrome_frame/infobars/manag... chrome_frame/infobars/manager.cc:20: explicit InfobarManagerImpl(HostWindowManager *manager) : manager_(manager) { On 2010/11/24 16:08:24, tommi wrote: > ...Manager* manager Done. http://codereview.chromium.org/4766003/diff/25001/chrome_frame/infobars/manag... chrome_frame/infobars/manager.cc:28: virtual bool Show(InfobarContent *content, InfobarType type) { On 2010/11/25 18:00:13, grt wrote: > InfobarContent* Done. http://codereview.chromium.org/4766003/diff/25001/chrome_frame/infobars/manag... chrome_frame/infobars/manager.cc:62: HostWindowManager *manager_; On 2010/11/25 18:00:13, grt wrote: > HostWindowManager* Done. http://codereview.chromium.org/4766003/diff/25001/chrome_frame/infobars/manag... File chrome_frame/infobars/manager.h (right): http://codereview.chromium.org/4766003/diff/25001/chrome_frame/infobars/manag... chrome_frame/infobars/manager.h:23: static InfobarManager* Get(HWND tab_window); On 2010/11/25 18:00:13, grt wrote: > Please document ownership: does caller own the manager after the call? Can this > return NULL? Thanks.
lgtm. very nice work Erik. Just nits below. http://codereview.chromium.org/4766003/diff/54001/chrome_frame/infobars/inter... File chrome_frame/infobars/internal/infobar_window.cc (right): http://codereview.chromium.org/4766003/diff/54001/chrome_frame/infobars/inter... chrome_frame/infobars/internal/infobar_window.cc:45: FunctionStub::Destroy(timer_stub_); check for NULL? http://codereview.chromium.org/4766003/diff/54001/chrome_frame/infobars/inter... chrome_frame/infobars/internal/infobar_window.cc:52: timer_stub_ = FunctionStub::Create(reinterpret_cast<uintptr_t>(host), maybe first DCHECK(timer_stub_ == NULL); http://codereview.chromium.org/4766003/diff/54001/chrome_frame/infobars/inter... chrome_frame/infobars/internal/infobar_window.cc:102: infobar_rect.bottom = rect->top + current_height_); this is a bit confusing because the assignment to infobar_rect.bottom is done on the 2nd line but happens before the 1st line. You're also using the current value of rect->top on the second line which is newer than the value on the first line. Furthermore a reader might think that you meant to do == and not = :) So, I'd prefer: infobar_rect.bottom = rect->top + current_height_; rect->top = std::min(rect->bottom, infobar_rect.bottom); http://codereview.chromium.org/4766003/diff/54001/chrome_frame/infobars/inter... chrome_frame/infobars/internal/infobar_window.cc:105: rect->bottom = std::max( same here http://codereview.chromium.org/4766003/diff/54001/chrome_frame/test/infobar_u... File chrome_frame/test/infobar_unittests.cc (right): http://codereview.chromium.org/4766003/diff/54001/chrome_frame/test/infobar_u... chrome_frame/test/infobar_unittests.cc:1: // Copyright (c) 2010 The Chromium Authors. All rights reserved. Excellent tests! http://codereview.chromium.org/4766003/diff/54001/chrome_frame/test/infobar_u... chrome_frame/test/infobar_unittests.cc:127: new testing::StrictMock<MockDelegate<MockWindowSubclass> >(); indent http://codereview.chromium.org/4766003/diff/54001/chrome_frame/test/infobar_u... chrome_frame/test/infobar_unittests.cc:442: remove extra empty line
Killer tests! LGTM with nits. http://codereview.chromium.org/4766003/diff/54001/chrome_frame/infobars/conte... File chrome_frame/infobars/content.h (right): http://codereview.chromium.org/4766003/diff/54001/chrome_frame/infobars/conte... chrome_frame/infobars/content.h:15: // The instance will be deleted by the infobar facility when it is no longer remove extra space in "facility when" http://codereview.chromium.org/4766003/diff/54001/chrome_frame/infobars/inter... File chrome_frame/infobars/internal/displaced_window.cc (right): http://codereview.chromium.org/4766003/diff/54001/chrome_frame/infobars/inter... chrome_frame/infobars/internal/displaced_window.cc:36: if (calc_valid_rects && ret == WVR_VALIDRECTS && !EqualRect(&natural_rect, nit: I find it more readable if the line wraps after && so that the EqualRect() call isn't split. http://codereview.chromium.org/4766003/diff/54001/chrome_frame/infobars/inter... File chrome_frame/infobars/internal/host_window.cc (right): http://codereview.chromium.org/4766003/diff/54001/chrome_frame/infobars/inter... chrome_frame/infobars/internal/host_window.cc:24: virtual void AdjustDisplacedWindowDimensions(RECT *rect); RECT* rect http://codereview.chromium.org/4766003/diff/54001/chrome_frame/infobars/inter... chrome_frame/infobars/internal/host_window.cc:32: DisplacedWindowDelegate(HostWindowManager* manager) I think this should wrap after the open paren rather than after the :: http://codereview.chromium.org/4766003/diff/54001/chrome_frame/infobars/inter... chrome_frame/infobars/internal/host_window.cc:50: AdjustDisplacedWindowDimensions(RECT *rect) { RECT* rect and same comment about wrapping http://codereview.chromium.org/4766003/diff/54001/chrome_frame/infobars/inter... File chrome_frame/infobars/internal/infobar_window.cc (right): http://codereview.chromium.org/4766003/diff/54001/chrome_frame/infobars/inter... chrome_frame/infobars/internal/infobar_window.cc:56: bool InfobarWindow::Show(InfobarContent *content) { InfobarContent* content http://codereview.chromium.org/4766003/diff/54001/chrome_frame/infobars/inter... chrome_frame/infobars/internal/infobar_window.cc:57: DCHECK(host_ != NULL); maybe instead do: if (host_ == NULL) { NOTREACHED() << "some explanation"; return false; } http://codereview.chromium.org/4766003/diff/54001/chrome_frame/infobars/inter... chrome_frame/infobars/internal/infobar_window.cc:81: DCHECK(host_ != NULL); similar proposal as above http://codereview.chromium.org/4766003/diff/54001/chrome_frame/infobars/inter... chrome_frame/infobars/internal/infobar_window.cc:144: DPLOG(ERROR) << "Failure in SetTimer."; DPLOG_IF(ERROR, timer_id_ == 0) << ...; http://codereview.chromium.org/4766003/diff/54001/chrome_frame/infobars/inter... File chrome_frame/infobars/internal/infobar_window.h (right): http://codereview.chromium.org/4766003/diff/54001/chrome_frame/infobars/inter... chrome_frame/infobars/internal/infobar_window.h:50: bool Show(InfobarContent *content); InfobarContent* content http://codereview.chromium.org/4766003/diff/54001/chrome_frame/infobars/inter... File chrome_frame/infobars/internal/subclassing_window.h (right): http://codereview.chromium.org/4766003/diff/54001/chrome_frame/infobars/inter... chrome_frame/infobars/internal/subclassing_window.h:65: LRESULT result = ::SendMessage(hwnd, nit: Can these lines be collapsed to reduce vertical space without hampering readability? If you wrap after '=', does all of the SendMessage call fit on the next line? http://codereview.chromium.org/4766003/diff/54001/chrome_frame/infobars/inter... chrome_frame/infobars/internal/subclassing_window.h:88: static UINT message_id( This will register the same message id for all instantiations of the SubclassingWindowWithDelegate template. Is this the right thing? http://codereview.chromium.org/4766003/diff/54001/chrome_frame/infobars/manag... File chrome_frame/infobars/manager.cc (right): http://codereview.chromium.org/4766003/diff/54001/chrome_frame/infobars/manag... chrome_frame/infobars/manager.cc:14: // InfobarManager nit: end the sentence w/ '.' http://codereview.chromium.org/4766003/diff/54001/chrome_frame/infobars/manag... chrome_frame/infobars/manager.cc:93: // Transferred to host_manager in call to Initialize nit: end the sentence w/ '.' http://codereview.chromium.org/4766003/diff/54001/chrome_frame/infobars/manag... File chrome_frame/infobars/manager.h (right): http://codereview.chromium.org/4766003/diff/54001/chrome_frame/infobars/manag... chrome_frame/infobars/manager.h:41: virtual bool Show(InfobarContent *content, InfobarType type) = 0; InfobarContent* content http://codereview.chromium.org/4766003/diff/54001/chrome_frame/test/infobar_u... File chrome_frame/test/infobar_unittests.cc (right): http://codereview.chromium.org/4766003/diff/54001/chrome_frame/test/infobar_u... chrome_frame/test/infobar_unittests.cc:163: new testing::StrictMock<MockDelegate<MockWindowSubclass> >(); indent http://codereview.chromium.org/4766003/diff/54001/chrome_frame/test/infobar_u... chrome_frame/test/infobar_unittests.cc:232: new testing::StrictMock<MockDelegate<DisplacedWindowManager> >(); indent http://codereview.chromium.org/4766003/diff/54001/chrome_frame/test/infobar_u... chrome_frame/test/infobar_unittests.cc:251: new testing::StrictMock<MockDelegate<HostWindowManager> >(); indent http://codereview.chromium.org/4766003/diff/54001/chrome_frame/test/infobar_u... chrome_frame/test/infobar_unittests.cc:553: InfobarManager *manager = InfobarManager::Get(parent_hwnd); InfobarManager* manager
lgtm++ http://codereview.chromium.org/4766003/diff/54001/chrome_frame/infobars/inter... File chrome_frame/infobars/internal/subclassing_window.h (right): http://codereview.chromium.org/4766003/diff/54001/chrome_frame/infobars/inter... chrome_frame/infobars/internal/subclassing_window.h:50: PLOG(ERROR) << "Failed to subclass an HWND"; nit: it'll be nice if the error code is available in the message. |
