Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
4338001:
Implement print preview tab controller. (Closed)
|
Created:
10 years, 1 month ago by kmadhusu Modified:
9 years, 7 months ago CC:
chromium-reviews, Paweł Hajdan Jr., ben+cc_chromium.org, brettw-cc_chromium.org, darin-cc_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionImplement print preview tab controller.
For print preview, a PP tab should be linked with the tab that initiated the printing operation. If the tab initiates a second printing operation while the first print preview tab is still open, that PP tab should be focused/activated. There may be more than one PP tab open.
Hook ctrl+p to show print preview tab.
BUG=59653, 57893
TEST=new unittest created.
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=66319
Patch Set 1 #
Total comments: 6
Patch Set 2 : Hook up ctrl+p to show print preview tab. #
Total comments: 43
Patch Set 3 : Made changes as per comments. #Patch Set 4 : '' #Patch Set 5 : Fixed a style issue. #
Total comments: 22
Patch Set 6 : Made code changes as per comments. #
Total comments: 22
Patch Set 7 : Fixed style issues. #Patch Set 8 : Made code changes as per comments. #Patch Set 9 : Uploading my changes again. #
Total comments: 15
Patch Set 10 : '' #
Total comments: 2
Patch Set 11 : '' #
Total comments: 39
Patch Set 12 : '' #Patch Set 13 : '' #
Total comments: 12
Patch Set 14 : '' #Messages
Total messages: 26 (0 generated)
Can you also just hook up IDC_PRINT to this code in this CL? It should be pretty simple. http://codereview.chromium.org/4338001/diff/1/5 File chrome/browser/printing/print_preview_tab_controller.cc (right): http://codereview.chromium.org/4338001/diff/1/5#newcode30 chrome/browser/printing/print_preview_tab_controller.cc:30: TabContents* current_tab = GetCurrentTabContents(); It might be good to pass in a TabContents rather than always getting the current TabContents here. i.e. If we have tab AA and tab BB open. If tab BB has a javascript function that does: sleep(30); // seconds window.print(); and the window.print() fires when we're on tab AA, we don't want to accidentally set tab AA and the initiator for tab BB's print preview tab. http://codereview.chromium.org/4338001/diff/1/5#newcode69 chrome/browser/printing/print_preview_tab_controller.cc:69: TabContents* PrintPreviewTabController::GetPreviewTab() { If you pass in a TabContents* here as well, you can simplify IsPrintPreviewTab() to just: return (GetPreviewTab(tab) != NULL); http://codereview.chromium.org/4338001/diff/1/6 File chrome/browser/printing/print_preview_tab_controller.h (right): http://codereview.chromium.org/4338001/diff/1/6#newcode61 chrome/browser/printing/print_preview_tab_controller.h:61: typedef std::map<TabContents*, TabContents* > PreviewTabMap; nit: extra space before >.
Lei, Thanks for your comments. Made code changes as per your suggestions and hooked up ctrl+p to show print preview tab. http://codereview.chromium.org/4338001/diff/1/5 File chrome/browser/printing/print_preview_tab_controller.cc (right): http://codereview.chromium.org/4338001/diff/1/5#newcode30 chrome/browser/printing/print_preview_tab_controller.cc:30: TabContents* current_tab = GetCurrentTabContents(); On 2010/11/02 22:37:40, Lei Zhang wrote: > It might be good to pass in a TabContents rather than always getting the current > TabContents here. > > i.e. If we have tab AA and tab BB open. If tab BB has a javascript function that > does: > > sleep(30); // seconds > window.print(); > > and the window.print() fires when we're on tab AA, we don't want to accidentally > set tab AA and the initiator for tab BB's print preview tab. Done. http://codereview.chromium.org/4338001/diff/1/5#newcode69 chrome/browser/printing/print_preview_tab_controller.cc:69: TabContents* PrintPreviewTabController::GetPreviewTab() { On 2010/11/02 22:37:40, Lei Zhang wrote: > If you pass in a TabContents* here as well, you can simplify IsPrintPreviewTab() > to just: > > return (GetPreviewTab(tab) != NULL); Done. http://codereview.chromium.org/4338001/diff/1/6 File chrome/browser/printing/print_preview_tab_controller.h (right): http://codereview.chromium.org/4338001/diff/1/6#newcode61 chrome/browser/printing/print_preview_tab_controller.h:61: typedef std::map<TabContents*, TabContents* > PreviewTabMap; On 2010/11/02 22:37:40, Lei Zhang wrote: > nit: extra space before >. Done.
http://codereview.chromium.org/4338001/diff/6001/7003 File chrome/browser/browser_process_impl.h (right): http://codereview.chromium.org/4338001/diff/6001/7003#newcode163 chrome/browser/browser_process_impl.h:163: bool created_print_preview_tab_controller_; I realize that everyone else has used this pattern, but I really don't see the purpose of using a |create_blah_| variable when print_preview_tab_controller.get() will tell you the same answer. Can you change this? http://codereview.chromium.org/4338001/diff/6001/7004 File chrome/browser/printing/print_preview_tab_controller.cc (right): http://codereview.chromium.org/4338001/diff/6001/7004#newcode5 chrome/browser/printing/print_preview_tab_controller.cc:5: #include "chrome/browser/printing/print_preview_tab_controller.h" Alphabetize the include order. http://codereview.chromium.org/4338001/diff/6001/7005 File chrome/browser/printing/print_preview_tab_controller.h (right): http://codereview.chromium.org/4338001/diff/6001/7005#newcode12 chrome/browser/printing/print_preview_tab_controller.h:12: #include "chrome/browser/browser.h" Forward declare 'class Browser' and remove this include. http://codereview.chromium.org/4338001/diff/6001/7005#newcode13 chrome/browser/printing/print_preview_tab_controller.h:13: #include "chrome/browser/tab_contents/tab_contents.h" Forward declare 'class TabContents' and remove this include. http://codereview.chromium.org/4338001/diff/6001/7005#newcode19 chrome/browser/printing/print_preview_tab_controller.h:19: // This class is a singleton that manages print preview tabs and You call this class a Singleton, but the constructor is public and you haven't friended |DefaultSingletonStraits|. Please read base/singleton.h comments. http://codereview.chromium.org/4338001/diff/6001/7005#newcode24 chrome/browser/printing/print_preview_tab_controller.h:24: public: One-space indent for access level specifiers. http://codereview.chromium.org/4338001/diff/6001/7005#newcode33 chrome/browser/printing/print_preview_tab_controller.h:33: TabContents* initiator_tab, int id); Document |id|. http://codereview.chromium.org/4338001/diff/6001/7005#newcode35 chrome/browser/printing/print_preview_tab_controller.h:35: private: One-space indent for access level specifiers. http://codereview.chromium.org/4338001/diff/6001/7005#newcode38 chrome/browser/printing/print_preview_tab_controller.h:38: // Returns true if the current tab is a preview tab. "current tab" is misleading. I'm assuming you mean "Returns true if |tab| is a print preview tab." http://codereview.chromium.org/4338001/diff/6001/7005#newcode41 chrome/browser/printing/print_preview_tab_controller.h:41: // Return the browser window with browser_window_id. |browser_window_id|. http://codereview.chromium.org/4338001/diff/6001/7005#newcode60 chrome/browser/printing/print_preview_tab_controller.h:60: typedef std::map<TabContents*, TabContents*> PreviewTabMap; Should probably be called |PrintPreviewTabMap|. http://codereview.chromium.org/4338001/diff/6001/7006 File chrome/browser/printing/print_preview_tab_controller_unittest.cc (right): http://codereview.chromium.org/4338001/diff/6001/7006#newcode5 chrome/browser/printing/print_preview_tab_controller_unittest.cc:5: #include "chrome/browser/printing/print_preview_tab_controller.h" Alphabetize include order. http://codereview.chromium.org/4338001/diff/6001/7006#newcode10 chrome/browser/printing/print_preview_tab_controller_unittest.cc:10: class PrintPreviewTabControllerTest : public BrowserWithTestWindowTest { You can get rid of all these lines with: typedef BrowserWithTestWindowTest PrintPreviewControllerTest; http://codereview.chromium.org/4338001/diff/6001/7006#newcode18 chrome/browser/printing/print_preview_tab_controller_unittest.cc:18: // Create/Get a preview tab for initiator tab. How about a test with multiple windows?
http://codereview.chromium.org/4338001/diff/6001/7003 File chrome/browser/browser_process_impl.h (right): http://codereview.chromium.org/4338001/diff/6001/7003#newcode163 chrome/browser/browser_process_impl.h:163: bool created_print_preview_tab_controller_; On 2010/11/05 20:51:11, James Hawkins wrote: > I realize that everyone else has used this pattern, but I really don't see the > purpose of using a |create_blah_| variable when > print_preview_tab_controller.get() will tell you the same answer. Can you change > this? I think this is because you can be in a situation where you have destroyed your FooManager, but someone calls BrowserProcessImpl::foo_manager(), in which case you don't want to create a new one.
http://codereview.chromium.org/4338001/diff/6001/7002 File chrome/browser/browser_process_impl.cc (right): http://codereview.chromium.org/4338001/diff/6001/7002#newcode719 chrome/browser/browser_process_impl.cc:719: DCHECK(print_preview_tab_controller_.get() == NULL); DCHECK(!created_print_preview_tab_controller_ && print_preview_tab_controller_.get() == NULL); http://codereview.chromium.org/4338001/diff/6001/7008 File chrome/browser/renderer_host/render_view_host.cc (right): http://codereview.chromium.org/4338001/diff/6001/7008#newcode59 chrome/browser/renderer_host/render_view_host.cc:59: #include "chrome/browser/printing/print_preview_tab_controller.h" nit: put this in alphabetical order. http://codereview.chromium.org/4338001/diff/6001/7008#newcode2242 chrome/browser/renderer_host/render_view_host.cc:2242: TabContents* tab = delegate_? delegate_->GetAsTabContents():NULL; nit: (I can't find it in the style guide, but...) put spaces around the ':'. http://codereview.chromium.org/4338001/diff/6001/7008#newcode2258 chrome/browser/renderer_host/render_view_host.cc:2258: // Initiator tab This is the exact same code as above. Put it in a function? http://codereview.chromium.org/4338001/diff/6001/7009 File chrome/browser/tab_contents/tab_contents.cc (right): http://codereview.chromium.org/4338001/diff/6001/7009#newcode1357 chrome/browser/tab_contents/tab_contents.cc:1357: if (showing_interstitial_page()) This makes the same check in TabContents::PrintNow() redundant. http://codereview.chromium.org/4338001/diff/6001/7009#newcode1360 chrome/browser/tab_contents/tab_contents.cc:1360: switches::kEnablePrintPreview)) nit: when the if statement is on multiple lines, wrap the blocks in {}. see: [Formatting -> Conditionals] http://codereview.chromium.org/4338001/diff/6001/7010 File chrome/chrome_browser.gypi (right): http://codereview.chromium.org/4338001/diff/6001/7010#newcode2475 chrome/chrome_browser.gypi:2475: 'browser/printing/print_preview_tab_controller.cc', nit: Please put these in order -> 4 slots up.
http://codereview.chromium.org/4338001/diff/6001/7002 File chrome/browser/browser_process_impl.cc (right): http://codereview.chromium.org/4338001/diff/6001/7002#newcode719 chrome/browser/browser_process_impl.cc:719: DCHECK(print_preview_tab_controller_.get() == NULL); On 2010/11/05 21:42:56, Lei Zhang wrote: > DCHECK(!created_print_preview_tab_controller_ && > print_preview_tab_controller_.get() == NULL); Done. http://codereview.chromium.org/4338001/diff/6001/7003 File chrome/browser/browser_process_impl.h (right): http://codereview.chromium.org/4338001/diff/6001/7003#newcode163 chrome/browser/browser_process_impl.h:163: bool created_print_preview_tab_controller_; On 2010/11/05 20:51:11, James Hawkins wrote: > I realize that everyone else has used this pattern, but I really don't see the > purpose of using a |create_blah_| variable when > print_preview_tab_controller.get() will tell you the same answer. Can you change > this? Done. http://codereview.chromium.org/4338001/diff/6001/7004 File chrome/browser/printing/print_preview_tab_controller.cc (right): http://codereview.chromium.org/4338001/diff/6001/7004#newcode5 chrome/browser/printing/print_preview_tab_controller.cc:5: #include "chrome/browser/printing/print_preview_tab_controller.h" On 2010/11/05 20:51:11, James Hawkins wrote: > Alphabetize the include order. Done. http://codereview.chromium.org/4338001/diff/6001/7005 File chrome/browser/printing/print_preview_tab_controller.h (right): http://codereview.chromium.org/4338001/diff/6001/7005#newcode12 chrome/browser/printing/print_preview_tab_controller.h:12: #include "chrome/browser/browser.h" On 2010/11/05 20:51:11, James Hawkins wrote: > Forward declare 'class Browser' and remove this include. Done. http://codereview.chromium.org/4338001/diff/6001/7005#newcode13 chrome/browser/printing/print_preview_tab_controller.h:13: #include "chrome/browser/tab_contents/tab_contents.h" On 2010/11/05 20:51:11, James Hawkins wrote: > Forward declare 'class TabContents' and remove this include. Done. http://codereview.chromium.org/4338001/diff/6001/7005#newcode19 chrome/browser/printing/print_preview_tab_controller.h:19: // This class is a singleton that manages print preview tabs and On 2010/11/05 20:51:11, James Hawkins wrote: > You call this class a Singleton, but the constructor is public and you haven't > friended |DefaultSingletonStraits|. Please read base/singleton.h comments. I read base/singleton.h comments. I don't think this class falls under that category. I modified the comments correspondingly. http://codereview.chromium.org/4338001/diff/6001/7005#newcode24 chrome/browser/printing/print_preview_tab_controller.h:24: public: On 2010/11/05 20:51:11, James Hawkins wrote: > One-space indent for access level specifiers. Done. http://codereview.chromium.org/4338001/diff/6001/7005#newcode33 chrome/browser/printing/print_preview_tab_controller.h:33: TabContents* initiator_tab, int id); On 2010/11/05 20:51:11, James Hawkins wrote: > Document |id|. Done. http://codereview.chromium.org/4338001/diff/6001/7005#newcode35 chrome/browser/printing/print_preview_tab_controller.h:35: private: On 2010/11/05 20:51:11, James Hawkins wrote: > One-space indent for access level specifiers. Done. http://codereview.chromium.org/4338001/diff/6001/7005#newcode38 chrome/browser/printing/print_preview_tab_controller.h:38: // Returns true if the current tab is a preview tab. On 2010/11/05 20:51:11, James Hawkins wrote: > "current tab" is misleading. I'm assuming you mean "Returns true if |tab| is a > print preview tab." Done. http://codereview.chromium.org/4338001/diff/6001/7005#newcode41 chrome/browser/printing/print_preview_tab_controller.h:41: // Return the browser window with browser_window_id. On 2010/11/05 20:51:11, James Hawkins wrote: > |browser_window_id|. Done. http://codereview.chromium.org/4338001/diff/6001/7005#newcode60 chrome/browser/printing/print_preview_tab_controller.h:60: typedef std::map<TabContents*, TabContents*> PreviewTabMap; On 2010/11/05 20:51:11, James Hawkins wrote: > Should probably be called |PrintPreviewTabMap|. Done. http://codereview.chromium.org/4338001/diff/6001/7006 File chrome/browser/printing/print_preview_tab_controller_unittest.cc (right): http://codereview.chromium.org/4338001/diff/6001/7006#newcode5 chrome/browser/printing/print_preview_tab_controller_unittest.cc:5: #include "chrome/browser/printing/print_preview_tab_controller.h" On 2010/11/05 20:51:11, James Hawkins wrote: > Alphabetize include order. Done. http://codereview.chromium.org/4338001/diff/6001/7006#newcode10 chrome/browser/printing/print_preview_tab_controller_unittest.cc:10: class PrintPreviewTabControllerTest : public BrowserWithTestWindowTest { On 2010/11/05 20:51:11, James Hawkins wrote: > You can get rid of all these lines with: > > typedef BrowserWithTestWindowTest PrintPreviewControllerTest; Done. http://codereview.chromium.org/4338001/diff/6001/7006#newcode18 chrome/browser/printing/print_preview_tab_controller_unittest.cc:18: // Create/Get a preview tab for initiator tab. On 2010/11/05 20:51:11, James Hawkins wrote: > How about a test with multiple windows? Created a new test case for these scenarios: (1) Create multiple print preview tabs in the same browser for different initiator tabs. (2) If the print preview tab already exists, it gets activated/focused when we get the preview tab for the initiator. http://codereview.chromium.org/4338001/diff/6001/7008 File chrome/browser/renderer_host/render_view_host.cc (right): http://codereview.chromium.org/4338001/diff/6001/7008#newcode59 chrome/browser/renderer_host/render_view_host.cc:59: #include "chrome/browser/printing/print_preview_tab_controller.h" On 2010/11/05 21:42:56, Lei Zhang wrote: > nit: put this in alphabetical order. Done. http://codereview.chromium.org/4338001/diff/6001/7008#newcode2242 chrome/browser/renderer_host/render_view_host.cc:2242: TabContents* tab = delegate_? delegate_->GetAsTabContents():NULL; On 2010/11/05 21:42:56, Lei Zhang wrote: > nit: (I can't find it in the style guide, but...) put spaces around the ':'. Done. http://codereview.chromium.org/4338001/diff/6001/7008#newcode2258 chrome/browser/renderer_host/render_view_host.cc:2258: // Initiator tab On 2010/11/05 21:42:56, Lei Zhang wrote: > This is the exact same code as above. Put it in a function? Done. http://codereview.chromium.org/4338001/diff/6001/7009 File chrome/browser/tab_contents/tab_contents.cc (right): http://codereview.chromium.org/4338001/diff/6001/7009#newcode1357 chrome/browser/tab_contents/tab_contents.cc:1357: if (showing_interstitial_page()) On 2010/11/05 21:42:56, Lei Zhang wrote: > This makes the same check in TabContents::PrintNow() redundant. Since PrintNow() function is called from other places, I am not removing the check from PrintNow() function but I am moving this check within the if-condition. http://codereview.chromium.org/4338001/diff/6001/7009#newcode1360 chrome/browser/tab_contents/tab_contents.cc:1360: switches::kEnablePrintPreview)) On 2010/11/05 21:42:56, Lei Zhang wrote: > nit: when the if statement is on multiple lines, wrap the blocks in {}. see: > [Formatting -> Conditionals] Done. http://codereview.chromium.org/4338001/diff/6001/7010 File chrome/chrome_browser.gypi (right): http://codereview.chromium.org/4338001/diff/6001/7010#newcode2475 chrome/chrome_browser.gypi:2475: 'browser/printing/print_preview_tab_controller.cc', On 2010/11/05 21:42:56, Lei Zhang wrote: > nit: Please put these in order -> 4 slots up. Done.
I found a test case that doesn't work: 1. Open google.com in tab A. 2. Press ctrl + p, this opens print preview in tab A_p. 3. Visit yahoo.com in tab A_p. 4. Go back to tab A, press ctrl + p again. It focuses tab A_p instead of bringing up print preview.
Lei, Thanks for the test case. Made code changes to handle these situations. Please retest and let me know your comments.
I'll finish reviewing this later. Not feeling well. http://codereview.chromium.org/4338001/diff/44001/chrome/browser/printing/pri... File chrome/browser/printing/print_preview_tab_controller.cc (right): http://codereview.chromium.org/4338001/diff/44001/chrome/browser/printing/pri... chrome/browser/printing/print_preview_tab_controller.cc:5: #include "chrome/browser/browser.h" Plese use: chrome/browser/ui/browser.h chrome/browser/ui/browser_list.h chrome/browser/ui/browser_navigator.h http://codereview.chromium.org/4338001/diff/44001/chrome/browser/printing/pri... chrome/browser/printing/print_preview_tab_controller.cc:9: #include "chrome/browser/printing/print_preview_tab_controller.h" nit: this goes on the top of the include list. http://codereview.chromium.org/4338001/diff/44001/chrome/browser/printing/pri... chrome/browser/printing/print_preview_tab_controller.cc:28: registrar_.RemoveAll(); ~NotificationRegistrar() does this already. http://codereview.chromium.org/4338001/diff/44001/chrome/browser/printing/pri... chrome/browser/printing/print_preview_tab_controller.cc:55: return ((preview_tab != NULL)? (preview_tab == tab) : false); You don't need the != NULL part here. Same goes for most if statements. http://codereview.chromium.org/4338001/diff/44001/chrome/browser/printing/pri... File chrome/browser/printing/print_preview_tab_controller.h (right): http://codereview.chromium.org/4338001/diff/44001/chrome/browser/printing/pri... chrome/browser/printing/print_preview_tab_controller.h:20: // This class manages print preview tabs and initiator tabs. You should definite what the initiator tab is and describe the relationship between initiator tabs and print preview tabs. http://codereview.chromium.org/4338001/diff/44001/chrome/browser/printing/pri... chrome/browser/printing/print_preview_tab_controller.h:33: TabContents* get_print_preview_tab( nit: Method names should be LikeThis() not like_this(). http://codereview.chromium.org/4338001/diff/44001/chrome/browser/printing/pri... chrome/browser/printing/print_preview_tab_controller.h:85: } // namespace printing nit: 2 spaces after } http://codereview.chromium.org/4338001/diff/44001/chrome/browser/printing/pri... File chrome/browser/printing/print_preview_tab_controller_unittest.cc (right): http://codereview.chromium.org/4338001/diff/44001/chrome/browser/printing/pri... chrome/browser/printing/print_preview_tab_controller_unittest.cc:5: #include "chrome/browser/browser_list.h" -> chrome/browser/ui/browser_list.h http://codereview.chromium.org/4338001/diff/44001/chrome/browser/renderer_hos... File chrome/browser/renderer_host/browser_render_process_host.cc (right): http://codereview.chromium.org/4338001/diff/44001/chrome/browser/renderer_hos... chrome/browser/renderer_host/browser_render_process_host.cc:628: switches::kEnablePrintPreview You can put a trailing comma after the last item. http://codereview.chromium.org/4338001/diff/44001/chrome/browser/renderer_hos... File chrome/browser/renderer_host/render_view_host.cc (right): http://codereview.chromium.org/4338001/diff/44001/chrome/browser/renderer_hos... chrome/browser/renderer_host/render_view_host.cc:2244: printing::PrintPreviewTabController* tab_controller = What happens when GetInstance() returns NULL? http://codereview.chromium.org/4338001/diff/44001/chrome/browser/renderer_hos... File chrome/browser/renderer_host/render_view_host.h (right): http://codereview.chromium.org/4338001/diff/44001/chrome/browser/renderer_hos... chrome/browser/renderer_host/render_view_host.h:727: TabContents* GetPrintPreviewTab(); Should this be private?
http://codereview.chromium.org/4338001/diff/44001/chrome/browser/printing/pri... File chrome/browser/printing/print_preview_tab_controller.cc (right): http://codereview.chromium.org/4338001/diff/44001/chrome/browser/printing/pri... chrome/browser/printing/print_preview_tab_controller.cc:5: #include "chrome/browser/browser.h" On 2010/11/12 02:10:55, Lei Zhang wrote: > Plese use: > > chrome/browser/ui/browser.h > chrome/browser/ui/browser_list.h > chrome/browser/ui/browser_navigator.h Done. http://codereview.chromium.org/4338001/diff/44001/chrome/browser/printing/pri... chrome/browser/printing/print_preview_tab_controller.cc:9: #include "chrome/browser/printing/print_preview_tab_controller.h" On 2010/11/12 02:10:55, Lei Zhang wrote: > nit: this goes on the top of the include list. Done. http://codereview.chromium.org/4338001/diff/44001/chrome/browser/printing/pri... chrome/browser/printing/print_preview_tab_controller.cc:28: registrar_.RemoveAll(); On 2010/11/12 02:10:55, Lei Zhang wrote: > ~NotificationRegistrar() does this already. Done. http://codereview.chromium.org/4338001/diff/44001/chrome/browser/printing/pri... chrome/browser/printing/print_preview_tab_controller.cc:55: return ((preview_tab != NULL)? (preview_tab == tab) : false); On 2010/11/12 02:10:55, Lei Zhang wrote: > You don't need the != NULL part here. Same goes for most if statements. Done. http://codereview.chromium.org/4338001/diff/44001/chrome/browser/printing/pri... File chrome/browser/printing/print_preview_tab_controller.h (right): http://codereview.chromium.org/4338001/diff/44001/chrome/browser/printing/pri... chrome/browser/printing/print_preview_tab_controller.h:20: // This class manages print preview tabs and initiator tabs. On 2010/11/12 02:10:55, Lei Zhang wrote: > You should definite what the initiator tab is and describe the relationship > between initiator tabs and print preview tabs. Done. http://codereview.chromium.org/4338001/diff/44001/chrome/browser/printing/pri... chrome/browser/printing/print_preview_tab_controller.h:33: TabContents* get_print_preview_tab( On 2010/11/12 02:10:55, Lei Zhang wrote: > nit: Method names should be LikeThis() not like_this(). Done. http://codereview.chromium.org/4338001/diff/44001/chrome/browser/printing/pri... chrome/browser/printing/print_preview_tab_controller.h:85: } // namespace printing On 2010/11/12 02:10:55, Lei Zhang wrote: > nit: 2 spaces after } Done. http://codereview.chromium.org/4338001/diff/44001/chrome/browser/printing/pri... File chrome/browser/printing/print_preview_tab_controller_unittest.cc (right): http://codereview.chromium.org/4338001/diff/44001/chrome/browser/printing/pri... chrome/browser/printing/print_preview_tab_controller_unittest.cc:5: #include "chrome/browser/browser_list.h" On 2010/11/12 02:10:55, Lei Zhang wrote: > -> chrome/browser/ui/browser_list.h Done. http://codereview.chromium.org/4338001/diff/44001/chrome/browser/renderer_hos... File chrome/browser/renderer_host/browser_render_process_host.cc (right): http://codereview.chromium.org/4338001/diff/44001/chrome/browser/renderer_hos... chrome/browser/renderer_host/browser_render_process_host.cc:628: switches::kEnablePrintPreview On 2010/11/12 02:10:55, Lei Zhang wrote: > You can put a trailing comma after the last item. Done. http://codereview.chromium.org/4338001/diff/44001/chrome/browser/renderer_hos... File chrome/browser/renderer_host/render_view_host.cc (right): http://codereview.chromium.org/4338001/diff/44001/chrome/browser/renderer_hos... chrome/browser/renderer_host/render_view_host.cc:2244: printing::PrintPreviewTabController* tab_controller = On 2010/11/12 02:10:55, Lei Zhang wrote: > What happens when GetInstance() returns NULL? Made code change to check the return value. http://codereview.chromium.org/4338001/diff/44001/chrome/browser/renderer_hos... File chrome/browser/renderer_host/render_view_host.h (right): http://codereview.chromium.org/4338001/diff/44001/chrome/browser/renderer_hos... chrome/browser/renderer_host/render_view_host.h:727: TabContents* GetPrintPreviewTab(); On 2010/11/12 02:10:55, Lei Zhang wrote: > Should this be private? Done.
http://codereview.chromium.org/4338001/diff/55001/chrome/browser/printing/pri... File chrome/browser/printing/print_preview_tab_controller.cc (right): http://codereview.chromium.org/4338001/diff/55001/chrome/browser/printing/pri... chrome/browser/printing/print_preview_tab_controller.cc:34: TabContents* current_tab, int browser_window_id ) { nit: |current_tab| should be named "initiator_tab" to match the prototype. http://codereview.chromium.org/4338001/diff/55001/chrome/browser/printing/pri... chrome/browser/printing/print_preview_tab_controller.cc:110: GetIndexOfTabContents(initiator_tab)+1; nit: foo + 1; http://codereview.chromium.org/4338001/diff/55001/chrome/browser/printing/pri... File chrome/browser/printing/print_preview_tab_controller.h (right): http://codereview.chromium.org/4338001/diff/55001/chrome/browser/printing/pri... chrome/browser/printing/print_preview_tab_controller.h:48: Browser* GetBrowserWindow(int browser_window_id); This is a one line private method that's only called in one place. Remove? http://codereview.chromium.org/4338001/diff/55001/chrome/browser/printing/pri... chrome/browser/printing/print_preview_tab_controller.h:76: // 1:1 relationship between initiator tab and print preview tab. You should state which type of tabs is the key and which is the value. It sounds like key = initiator here, but it's actually PP. http://codereview.chromium.org/4338001/diff/55001/chrome/browser/renderer_hos... File chrome/browser/renderer_host/render_view_host.cc (right): http://codereview.chromium.org/4338001/diff/55001/chrome/browser/renderer_hos... chrome/browser/renderer_host/render_view_host.cc:2240: // current_tab is initiator_tab nit: |current_tab| is initiator tab http://codereview.chromium.org/4338001/diff/55001/chrome/browser/renderer_hos... chrome/browser/renderer_host/render_view_host.cc:2241: TabContents* current_tab = delegate_? delegate_->GetAsTabContents() : NULL; nit: space before '?' http://codereview.chromium.org/4338001/diff/55001/chrome/browser/tab_contents... File chrome/browser/tab_contents/tab_contents.cc (right): http://codereview.chromium.org/4338001/diff/55001/chrome/browser/tab_contents... chrome/browser/tab_contents/tab_contents.cc:64: #include "chrome/browser/printing/print_preview_tab_controller.h" nit: (again) alphabetical order
http://codereview.chromium.org/4338001/diff/55001/chrome/browser/printing/pri... File chrome/browser/printing/print_preview_tab_controller.cc (right): http://codereview.chromium.org/4338001/diff/55001/chrome/browser/printing/pri... chrome/browser/printing/print_preview_tab_controller.cc:82: // Add TAB_CONTENTS_DESTROYED notification. nit: the comments in {Add,Remove}Observers are not useful. http://codereview.chromium.org/4338001/diff/55001/chrome/browser/printing/pri... File chrome/browser/printing/print_preview_tab_controller.h (right): http://codereview.chromium.org/4338001/diff/55001/chrome/browser/printing/pri... chrome/browser/printing/print_preview_tab_controller.h:60: // Add |tab| notification(TAB_CONTENTS_DESTROYED & NAV_ENTRY_COMMITTED) nit: Just combine the comments for Add and RemoveObservers: // Add/Remove observers for notifications from |tab|. http://codereview.chromium.org/4338001/diff/55001/chrome/browser/printing/pri... chrome/browser/printing/print_preview_tab_controller.h:69: virtual void Observe(NotificationType type, this needs to be public. http://codereview.chromium.org/4338001/diff/55001/chrome/browser/printing/pri... chrome/browser/printing/print_preview_tab_controller.h:74: void EraseMapEntry(TabContents* initiator_tab); again, why have a private, one line method that's only called once?
http://codereview.chromium.org/4338001/diff/55001/chrome/browser/printing/pri... File chrome/browser/printing/print_preview_tab_controller.cc (right): http://codereview.chromium.org/4338001/diff/55001/chrome/browser/printing/pri... chrome/browser/printing/print_preview_tab_controller.cc:34: TabContents* current_tab, int browser_window_id ) { On 2010/11/12 23:17:18, Lei Zhang wrote: > nit: |current_tab| should be named "initiator_tab" to match the prototype. Done. http://codereview.chromium.org/4338001/diff/55001/chrome/browser/printing/pri... chrome/browser/printing/print_preview_tab_controller.cc:82: // Add TAB_CONTENTS_DESTROYED notification. On 2010/11/12 23:48:40, Lei Zhang wrote: > nit: the comments in {Add,Remove}Observers are not useful. Done. http://codereview.chromium.org/4338001/diff/55001/chrome/browser/printing/pri... chrome/browser/printing/print_preview_tab_controller.cc:110: GetIndexOfTabContents(initiator_tab)+1; On 2010/11/12 23:17:18, Lei Zhang wrote: > nit: foo + 1; Done. http://codereview.chromium.org/4338001/diff/55001/chrome/browser/printing/pri... File chrome/browser/printing/print_preview_tab_controller.h (right): http://codereview.chromium.org/4338001/diff/55001/chrome/browser/printing/pri... chrome/browser/printing/print_preview_tab_controller.h:48: Browser* GetBrowserWindow(int browser_window_id); On 2010/11/12 23:17:18, Lei Zhang wrote: > This is a one line private method that's only called in one place. Remove? Removed. http://codereview.chromium.org/4338001/diff/55001/chrome/browser/printing/pri... chrome/browser/printing/print_preview_tab_controller.h:60: // Add |tab| notification(TAB_CONTENTS_DESTROYED & NAV_ENTRY_COMMITTED) On 2010/11/12 23:48:40, Lei Zhang wrote: > nit: Just combine the comments for Add and RemoveObservers: > > // Add/Remove observers for notifications from |tab|. Done. http://codereview.chromium.org/4338001/diff/55001/chrome/browser/printing/pri... chrome/browser/printing/print_preview_tab_controller.h:69: virtual void Observe(NotificationType type, On 2010/11/12 23:48:40, Lei Zhang wrote: > this needs to be public. Done. http://codereview.chromium.org/4338001/diff/55001/chrome/browser/printing/pri... chrome/browser/printing/print_preview_tab_controller.h:74: void EraseMapEntry(TabContents* initiator_tab); On 2010/11/12 23:48:40, Lei Zhang wrote: > again, why have a private, one line method that's only called once? Removed. http://codereview.chromium.org/4338001/diff/55001/chrome/browser/printing/pri... chrome/browser/printing/print_preview_tab_controller.h:76: // 1:1 relationship between initiator tab and print preview tab. On 2010/11/12 23:17:18, Lei Zhang wrote: > You should state which type of tabs is the key and which is the value. It sounds > like key = initiator here, but it's actually PP. Done. http://codereview.chromium.org/4338001/diff/55001/chrome/browser/renderer_hos... File chrome/browser/renderer_host/render_view_host.cc (right): http://codereview.chromium.org/4338001/diff/55001/chrome/browser/renderer_hos... chrome/browser/renderer_host/render_view_host.cc:2240: // current_tab is initiator_tab On 2010/11/12 23:17:18, Lei Zhang wrote: > nit: |current_tab| is initiator tab Done. http://codereview.chromium.org/4338001/diff/55001/chrome/browser/renderer_hos... chrome/browser/renderer_host/render_view_host.cc:2241: TabContents* current_tab = delegate_? delegate_->GetAsTabContents() : NULL; On 2010/11/12 23:17:18, Lei Zhang wrote: > nit: space before '?' Done. http://codereview.chromium.org/4338001/diff/55001/chrome/browser/tab_contents... File chrome/browser/tab_contents/tab_contents.cc (right): http://codereview.chromium.org/4338001/diff/55001/chrome/browser/tab_contents... chrome/browser/tab_contents/tab_contents.cc:64: #include "chrome/browser/printing/print_preview_tab_controller.h" On 2010/11/12 23:17:18, Lei Zhang wrote: > nit: (again) alphabetical order Done.
http://codereview.chromium.org/4338001/diff/78001/chrome/browser/printing/pri... File chrome/browser/printing/print_preview_tab_controller.cc (right): http://codereview.chromium.org/4338001/diff/78001/chrome/browser/printing/pri... chrome/browser/printing/print_preview_tab_controller.cc:34: TabContents* initiator_tab, int browser_window_id ) { The current implementation of IsPrintPreviewTab() below assumes the tab you pass in here, |initiator_tab|, is not NULL. Add a DCHECK? http://codereview.chromium.org/4338001/diff/78001/chrome/browser/printing/pri... chrome/browser/printing/print_preview_tab_controller.cc:50: const GURL& url = tab->GetURL(); If you already have a mapping of print preview tabs to initiator tabs, why do you also have this check? http://codereview.chromium.org/4338001/diff/78001/chrome/browser/printing/pri... chrome/browser/printing/print_preview_tab_controller.cc:56: return ((preview_tab)? (preview_tab == tab) : false); nit: you don't need the outer most parenthesis and the set around |preview_tab| on the left. Add a space before '?'. http://codereview.chromium.org/4338001/diff/78001/chrome/browser/printing/pri... chrome/browser/printing/print_preview_tab_controller.cc:71: if ((it->second == current_tab) || (it->first == current_tab)) if you first do preview_tab_map_.find(current_tab) and handle that case, you don't need to check it->first here. http://codereview.chromium.org/4338001/diff/78001/chrome/browser/printing/pri... chrome/browser/printing/print_preview_tab_controller.cc:157: bool reload = (transition_type == PageTransition::RELOAD || nit: no need for |reload| and other variables below since they are only used once, immediately after they get assigned. http://codereview.chromium.org/4338001/diff/78001/chrome/browser/printing/pri... File chrome/browser/printing/print_preview_tab_controller.h (right): http://codereview.chromium.org/4338001/diff/78001/chrome/browser/printing/pri... chrome/browser/printing/print_preview_tab_controller.h:50: bool IsPrintPreviewTab(TabContents* tab); nit: Be consistent. Either name this IsPreviewTab() or change GetPreviewTab() to GetPrintPreviewTab(). http://codereview.chromium.org/4338001/diff/78001/chrome/browser/printing/pri... chrome/browser/printing/print_preview_tab_controller.h:56: TabContents* GetPreviewTab(TabContents* initiator_tab); nit: |initiator_tab| is called "current_tab" in the implementation. This should be renamed because the tabs passed to this method are not always initiator tabs. You should mention what happens when you pass in a preview tab.
http://codereview.chromium.org/4338001/diff/78001/chrome/browser/printing/pri... File chrome/browser/printing/print_preview_tab_controller.cc (right): http://codereview.chromium.org/4338001/diff/78001/chrome/browser/printing/pri... chrome/browser/printing/print_preview_tab_controller.cc:34: TabContents* initiator_tab, int browser_window_id ) { On 2010/11/13 05:04:56, Lei Zhang wrote: > The current implementation of IsPrintPreviewTab() below assumes the tab you pass > in here, |initiator_tab|, is not NULL. Add a DCHECK? Done. http://codereview.chromium.org/4338001/diff/78001/chrome/browser/printing/pri... chrome/browser/printing/print_preview_tab_controller.cc:50: const GURL& url = tab->GetURL(); On 2010/11/13 05:04:56, Lei Zhang wrote: > If you already have a mapping of print preview tabs to initiator tabs, why do > you also have this check? I need this check because we can navigate to a preview tab by forward/back button. At that time, we dont have an entry in the preview_tab_map_ to check for. http://codereview.chromium.org/4338001/diff/78001/chrome/browser/printing/pri... chrome/browser/printing/print_preview_tab_controller.cc:56: return ((preview_tab)? (preview_tab == tab) : false); On 2010/11/13 05:04:56, Lei Zhang wrote: > nit: you don't need the outer most parenthesis and the set around |preview_tab| > on the left. Add a space before '?'. Previous check is sufficient to identify a preview tab. I am removing this code. http://codereview.chromium.org/4338001/diff/78001/chrome/browser/printing/pri... chrome/browser/printing/print_preview_tab_controller.cc:71: if ((it->second == current_tab) || (it->first == current_tab)) On 2010/11/13 05:04:56, Lei Zhang wrote: > if you first do preview_tab_map_.find(current_tab) and handle that case, you > don't need to check it->first here. To my understanding, "find(key)" function is used to find whether an entry exist in the map with the specified key. Here I am trying to check whether the |current_tab| is either key or value of a map entry. So, I am using for loop to check for both cases. http://codereview.chromium.org/4338001/diff/78001/chrome/browser/printing/pri... chrome/browser/printing/print_preview_tab_controller.cc:157: bool reload = (transition_type == PageTransition::RELOAD || On 2010/11/13 05:04:56, Lei Zhang wrote: > nit: no need for |reload| and other variables below since they are only used > once, immediately after they get assigned. Done. http://codereview.chromium.org/4338001/diff/78001/chrome/browser/printing/pri... File chrome/browser/printing/print_preview_tab_controller.h (right): http://codereview.chromium.org/4338001/diff/78001/chrome/browser/printing/pri... chrome/browser/printing/print_preview_tab_controller.h:50: bool IsPrintPreviewTab(TabContents* tab); On 2010/11/13 05:04:56, Lei Zhang wrote: > nit: Be consistent. Either name this IsPreviewTab() or change GetPreviewTab() to > GetPrintPreviewTab(). Renamed IsPrintPreviewTab() to IsPreviewTab(). http://codereview.chromium.org/4338001/diff/78001/chrome/browser/printing/pri... chrome/browser/printing/print_preview_tab_controller.h:56: TabContents* GetPreviewTab(TabContents* initiator_tab); On 2010/11/13 05:04:56, Lei Zhang wrote: > nit: |initiator_tab| is called "current_tab" in the implementation. This should > be renamed because the tabs passed to this method are not always initiator tabs. > You should mention what happens when you pass in a preview tab. Done.
http://codereview.chromium.org/4338001/diff/78001/chrome/browser/printing/pri... File chrome/browser/printing/print_preview_tab_controller.cc (right): http://codereview.chromium.org/4338001/diff/78001/chrome/browser/printing/pri... chrome/browser/printing/print_preview_tab_controller.cc:71: if ((it->second == current_tab) || (it->first == current_tab)) On 2010/11/15 21:22:27, kmadhusu wrote: > On 2010/11/13 05:04:56, Lei Zhang wrote: > > if you first do preview_tab_map_.find(current_tab) and handle that case, you > > don't need to check it->first here. > > To my understanding, "find(key)" function is used to find whether an entry exist > in the map with the specified key. Here I am trying to check whether the > |current_tab| is either key or value of a map entry. So, I am using for loop to > check for both cases. If preview_tab_map_.find(current_tab) returns preview_tab_map_.end(), then you know |current_tab| does not exist as a key, so you can just check to see if it's a value in the map. http://codereview.chromium.org/4338001/diff/85001/chrome/browser/printing/pri... chrome/browser/printing/print_preview_tab_controller.cc:53: return true; nit: drop the if statement and just return (foo && bar);
On 2010/11/16 00:47:39, Lei Zhang wrote: > http://codereview.chromium.org/4338001/diff/78001/chrome/browser/printing/pri... > File chrome/browser/printing/print_preview_tab_controller.cc (right): > > http://codereview.chromium.org/4338001/diff/78001/chrome/browser/printing/pri... > chrome/browser/printing/print_preview_tab_controller.cc:71: if ((it->second == > current_tab) || (it->first == current_tab)) > On 2010/11/15 21:22:27, kmadhusu wrote: > > On 2010/11/13 05:04:56, Lei Zhang wrote: > > > if you first do preview_tab_map_.find(current_tab) and handle that case, you > > > don't need to check it->first here. > > > > To my understanding, "find(key)" function is used to find whether an entry > exist > > in the map with the specified key. Here I am trying to check whether the > > |current_tab| is either key or value of a map entry. So, I am using for loop > to > > check for both cases. > > If preview_tab_map_.find(current_tab) returns preview_tab_map_.end(), then you > know |current_tab| does not exist as a key, so you can just check to see if it's > a value in the map. > > Done. http://codereview.chromium.org/4338001/diff/85001/chrome/browser/printing/pri... > chrome/browser/printing/print_preview_tab_controller.cc:53: return true; > nit: drop the if statement and just return (foo && bar); Done.
http://codereview.chromium.org/4338001/diff/85001/chrome/browser/printing/pri... File chrome/browser/printing/print_preview_tab_controller.cc (right): http://codereview.chromium.org/4338001/diff/85001/chrome/browser/printing/pri... chrome/browser/printing/print_preview_tab_controller.cc:53: if (url.SchemeIs(chrome::kChromeUIScheme) && On 2010/11/16 00:47:39, Lei Zhang wrote: > nit: drop the if statement and just return (foo && bar); Done.
LGTM
http://codereview.chromium.org/4338001/diff/94001/chrome/browser/printing/pri... File chrome/browser/printing/print_preview_tab_controller.cc (right): http://codereview.chromium.org/4338001/diff/94001/chrome/browser/printing/pri... chrome/browser/printing/print_preview_tab_controller.cc:37: // |initiator_tab| is preview tab. is a print preview tab. http://codereview.chromium.org/4338001/diff/94001/chrome/browser/printing/pri... chrome/browser/printing/print_preview_tab_controller.cc:41: // Get the preview tab for |initiator_tab|. print preview http://codereview.chromium.org/4338001/diff/94001/chrome/browser/printing/pri... File chrome/browser/printing/print_preview_tab_controller.h (right): http://codereview.chromium.org/4338001/diff/94001/chrome/browser/printing/pri... chrome/browser/printing/print_preview_tab_controller.h:20: // For print preview, a print preview (PP) tab should be linked with the s/should be/is/ http://codereview.chromium.org/4338001/diff/94001/chrome/browser/printing/pri... chrome/browser/printing/print_preview_tab_controller.h:24: // open. There is 1:1 relationship between PP tabs and initiating tabs. This s/is 1:1/is a 1:1/ http://codereview.chromium.org/4338001/diff/94001/chrome/browser/printing/pri... chrome/browser/printing/print_preview_tab_controller.h:37: // |browser_window_id| is used to identify |initiator_tab| browser window. "|browser_window_id| is the browser window containing |initiator_tab|." http://codereview.chromium.org/4338001/diff/94001/chrome/browser/printing/pri... chrome/browser/printing/print_preview_tab_controller.h:50: bool IsPreviewTab(TabContents* tab); IsPrintPreviewTab http://codereview.chromium.org/4338001/diff/94001/chrome/browser/printing/pri... chrome/browser/printing/print_preview_tab_controller.h:52: // Return the initiator tab for |preview_tab| or return NULL. Returns http://codereview.chromium.org/4338001/diff/94001/chrome/browser/printing/pri... chrome/browser/printing/print_preview_tab_controller.h:55: // Return preview tab for |current_tab| or return NULL. Returns http://codereview.chromium.org/4338001/diff/94001/chrome/browser/printing/pri... chrome/browser/printing/print_preview_tab_controller.h:56: // If |current_tab| is preview tab return |current_tab|. returns http://codereview.chromium.org/4338001/diff/94001/chrome/browser/printing/pri... chrome/browser/printing/print_preview_tab_controller.h:57: TabContents* GetPreviewTab(TabContents* current_tab); s/current/initiator/ http://codereview.chromium.org/4338001/diff/94001/chrome/browser/printing/pri... chrome/browser/printing/print_preview_tab_controller.h:59: // Create a new print preview tab. Creates http://codereview.chromium.org/4338001/diff/94001/chrome/browser/printing/pri... chrome/browser/printing/print_preview_tab_controller.h:63: // Add/Remove observers for notifications from |tab|. Adds/Removes http://codereview.chromium.org/4338001/diff/94001/chrome/browser/printing/pri... chrome/browser/printing/print_preview_tab_controller.h:76: // Are we waiting for a new preview tab with NavigationType::NEW_PAGE? If so, It's best not to describe a member variable w/ a question. This is typically done as: // True if the controller is waiting for a new preview tab via NavigationType::NEW_PAGE Another note here: don't use "we" in comments, describe what "we" is actually referring to. http://codereview.chromium.org/4338001/diff/94001/chrome/browser/printing/pri... chrome/browser/printing/print_preview_tab_controller.h:83: } // namespace printing Blank line after this. http://codereview.chromium.org/4338001/diff/94001/chrome/browser/printing/pri... File chrome/browser/printing/print_preview_tab_controller_unittest.cc (right): http://codereview.chromium.org/4338001/diff/94001/chrome/browser/printing/pri... chrome/browser/printing/print_preview_tab_controller_unittest.cc:14: // Test: Create/Get a preview tab for initiator tab. Remove all "Test: ". http://codereview.chromium.org/4338001/diff/94001/chrome/browser/renderer_hos... File chrome/browser/renderer_host/render_view_host.cc (right): http://codereview.chromium.org/4338001/diff/94001/chrome/browser/renderer_hos... chrome/browser/renderer_host/render_view_host.cc:2218: // |current_tab| is initiator tab. s/current/initiator/ http://codereview.chromium.org/4338001/diff/94001/chrome/browser/renderer_hos... chrome/browser/renderer_host/render_view_host.cc:2235: TabContents* preview_tab = GetPrintPreviewTab(); print_preview_tab http://codereview.chromium.org/4338001/diff/94001/chrome/browser/renderer_hos... chrome/browser/renderer_host/render_view_host.cc:2235: TabContents* preview_tab = GetPrintPreviewTab(); Does GetPrintPreviewTab() open/activate a tab?
http://codereview.chromium.org/4338001/diff/94001/chrome/browser/printing/pri... File chrome/browser/printing/print_preview_tab_controller.h (right): http://codereview.chromium.org/4338001/diff/94001/chrome/browser/printing/pri... chrome/browser/printing/print_preview_tab_controller.h:57: TabContents* GetPreviewTab(TabContents* current_tab); On 2010/11/16 02:15:42, James Hawkins wrote: > s/current/initiator/ The tab being passed in is arbitrary. It is not always an initiator tab.
http://codereview.chromium.org/4338001/diff/94001/chrome/browser/printing/pri... File chrome/browser/printing/print_preview_tab_controller.h (right): http://codereview.chromium.org/4338001/diff/94001/chrome/browser/printing/pri... chrome/browser/printing/print_preview_tab_controller.h:57: TabContents* GetPreviewTab(TabContents* current_tab); On 2010/11/16 02:24:23, Lei Zhang wrote: > On 2010/11/16 02:15:42, James Hawkins wrote: > > s/current/initiator/ > > The tab being passed in is arbitrary. It is not always an initiator tab. Then "current" is also wrong and should be changed. http://codereview.chromium.org/4338001/diff/94001/chrome/browser/printing/pri... chrome/browser/printing/print_preview_tab_controller.h:57: TabContents* GetPreviewTab(TabContents* current_tab); Taking Lei's comment into account, I believe this should be changed to: TabContents* GetPrintPreviewForTab(TabContents* tab);
http://codereview.chromium.org/4338001/diff/94001/chrome/browser/printing/pri... File chrome/browser/printing/print_preview_tab_controller.cc (right): http://codereview.chromium.org/4338001/diff/94001/chrome/browser/printing/pri... chrome/browser/printing/print_preview_tab_controller.cc:37: // |initiator_tab| is preview tab. On 2010/11/16 02:15:42, James Hawkins wrote: > is a print preview tab. Done. http://codereview.chromium.org/4338001/diff/94001/chrome/browser/printing/pri... chrome/browser/printing/print_preview_tab_controller.cc:41: // Get the preview tab for |initiator_tab|. On 2010/11/16 02:15:42, James Hawkins wrote: > print preview Done. http://codereview.chromium.org/4338001/diff/94001/chrome/browser/printing/pri... File chrome/browser/printing/print_preview_tab_controller.h (right): http://codereview.chromium.org/4338001/diff/94001/chrome/browser/printing/pri... chrome/browser/printing/print_preview_tab_controller.h:20: // For print preview, a print preview (PP) tab should be linked with the On 2010/11/16 02:15:42, James Hawkins wrote: > s/should be/is/ Done. http://codereview.chromium.org/4338001/diff/94001/chrome/browser/printing/pri... chrome/browser/printing/print_preview_tab_controller.h:24: // open. There is 1:1 relationship between PP tabs and initiating tabs. This On 2010/11/16 02:15:42, James Hawkins wrote: > s/is 1:1/is a 1:1/ Done. http://codereview.chromium.org/4338001/diff/94001/chrome/browser/printing/pri... chrome/browser/printing/print_preview_tab_controller.h:37: // |browser_window_id| is used to identify |initiator_tab| browser window. On 2010/11/16 02:15:42, James Hawkins wrote: > "|browser_window_id| is the browser window containing |initiator_tab|." Done. http://codereview.chromium.org/4338001/diff/94001/chrome/browser/printing/pri... chrome/browser/printing/print_preview_tab_controller.h:50: bool IsPreviewTab(TabContents* tab); On 2010/11/16 02:15:42, James Hawkins wrote: > IsPrintPreviewTab Done. http://codereview.chromium.org/4338001/diff/94001/chrome/browser/printing/pri... chrome/browser/printing/print_preview_tab_controller.h:52: // Return the initiator tab for |preview_tab| or return NULL. On 2010/11/16 02:15:42, James Hawkins wrote: > Returns Done. http://codereview.chromium.org/4338001/diff/94001/chrome/browser/printing/pri... chrome/browser/printing/print_preview_tab_controller.h:55: // Return preview tab for |current_tab| or return NULL. On 2010/11/16 02:15:42, James Hawkins wrote: > Returns Done. http://codereview.chromium.org/4338001/diff/94001/chrome/browser/printing/pri... chrome/browser/printing/print_preview_tab_controller.h:56: // If |current_tab| is preview tab return |current_tab|. On 2010/11/16 02:15:42, James Hawkins wrote: > returns Done. http://codereview.chromium.org/4338001/diff/94001/chrome/browser/printing/pri... chrome/browser/printing/print_preview_tab_controller.h:57: TabContents* GetPreviewTab(TabContents* current_tab); On 2010/11/16 02:27:15, James Hawkins wrote: > Taking Lei's comment into account, I believe this should be changed to: > > TabContents* GetPrintPreviewForTab(TabContents* tab); Done. http://codereview.chromium.org/4338001/diff/94001/chrome/browser/printing/pri... chrome/browser/printing/print_preview_tab_controller.h:59: // Create a new print preview tab. On 2010/11/16 02:15:42, James Hawkins wrote: > Creates Done. http://codereview.chromium.org/4338001/diff/94001/chrome/browser/printing/pri... chrome/browser/printing/print_preview_tab_controller.h:63: // Add/Remove observers for notifications from |tab|. On 2010/11/16 02:15:42, James Hawkins wrote: > Adds/Removes Done. http://codereview.chromium.org/4338001/diff/94001/chrome/browser/printing/pri... chrome/browser/printing/print_preview_tab_controller.h:76: // Are we waiting for a new preview tab with NavigationType::NEW_PAGE? If so, On 2010/11/16 02:15:42, James Hawkins wrote: > It's best not to describe a member variable w/ a question. This is typically > done as: > > // True if the controller is waiting for a new preview tab via > NavigationType::NEW_PAGE > > Another note here: don't use "we" in comments, describe what "we" is actually > referring to. Done. http://codereview.chromium.org/4338001/diff/94001/chrome/browser/printing/pri... chrome/browser/printing/print_preview_tab_controller.h:83: } // namespace printing On 2010/11/16 02:15:42, James Hawkins wrote: > Blank line after this. Done. http://codereview.chromium.org/4338001/diff/94001/chrome/browser/printing/pri... File chrome/browser/printing/print_preview_tab_controller_unittest.cc (right): http://codereview.chromium.org/4338001/diff/94001/chrome/browser/printing/pri... chrome/browser/printing/print_preview_tab_controller_unittest.cc:14: // Test: Create/Get a preview tab for initiator tab. On 2010/11/16 02:15:42, James Hawkins wrote: > Remove all "Test: ". Done. http://codereview.chromium.org/4338001/diff/94001/chrome/browser/renderer_hos... File chrome/browser/renderer_host/render_view_host.cc (right): http://codereview.chromium.org/4338001/diff/94001/chrome/browser/renderer_hos... chrome/browser/renderer_host/render_view_host.cc:2218: // |current_tab| is initiator tab. On 2010/11/16 02:15:42, James Hawkins wrote: > s/current/initiator/ Done. http://codereview.chromium.org/4338001/diff/94001/chrome/browser/renderer_hos... chrome/browser/renderer_host/render_view_host.cc:2235: TabContents* preview_tab = GetPrintPreviewTab(); On 2010/11/16 02:15:42, James Hawkins wrote: > print_preview_tab Done. http://codereview.chromium.org/4338001/diff/94001/chrome/browser/renderer_hos... chrome/browser/renderer_host/render_view_host.cc:2235: TabContents* preview_tab = GetPrintPreviewTab(); On 2010/11/16 02:15:42, James Hawkins wrote: > Does GetPrintPreviewTab() open/activate a tab? Yes. I renamed the function to "GetOrCreatePrintPreviewTab".
LGTM with comment nits fixed. http://codereview.chromium.org/4338001/diff/114001/chrome/browser/printing/pr... File chrome/browser/printing/print_preview_tab_controller.cc (right): http://codereview.chromium.org/4338001/diff/114001/chrome/browser/printing/pr... chrome/browser/printing/print_preview_tab_controller.cc:37: // |initiator_tab| is a print preview tab. This comment is stating the obvious, please remove it. http://codereview.chromium.org/4338001/diff/114001/chrome/browser/printing/pr... chrome/browser/printing/print_preview_tab_controller.cc:141: } Blank line here. http://codereview.chromium.org/4338001/diff/114001/chrome/browser/printing/pr... File chrome/browser/printing/print_preview_tab_controller.h (right): http://codereview.chromium.org/4338001/diff/114001/chrome/browser/printing/pr... chrome/browser/printing/print_preview_tab_controller.h:36: // Get/Create the print preview tab for initiator_tab. |initiator_tab| http://codereview.chromium.org/4338001/diff/114001/chrome/browser/printing/pr... chrome/browser/printing/print_preview_tab_controller.h:52: // Returns initiator tab for |preview_tab| or NULL. or NULL when...? http://codereview.chromium.org/4338001/diff/114001/chrome/browser/printing/pr... chrome/browser/printing/print_preview_tab_controller.h:55: // Returns preview tab for |tab| or NULL. or NULL when...? http://codereview.chromium.org/4338001/diff/114001/chrome/browser/printing/pr... chrome/browser/printing/print_preview_tab_controller.h:73: // A registrar for listening for TAB_CONTENTS_DESTROYED notification. s/TAB_CONTENTS_DESTROYED // "A registrar for listening for notifications." ^ Because it's easy to get out of date (it's already out of date).
http://codereview.chromium.org/4338001/diff/114001/chrome/browser/printing/pr... File chrome/browser/printing/print_preview_tab_controller.cc (right): http://codereview.chromium.org/4338001/diff/114001/chrome/browser/printing/pr... chrome/browser/printing/print_preview_tab_controller.cc:37: // |initiator_tab| is a print preview tab. On 2010/11/16 18:24:30, James Hawkins wrote: > This comment is stating the obvious, please remove it. Done. http://codereview.chromium.org/4338001/diff/114001/chrome/browser/printing/pr... chrome/browser/printing/print_preview_tab_controller.cc:141: } On 2010/11/16 18:24:30, James Hawkins wrote: > Blank line here. Done. http://codereview.chromium.org/4338001/diff/114001/chrome/browser/printing/pr... File chrome/browser/printing/print_preview_tab_controller.h (right): http://codereview.chromium.org/4338001/diff/114001/chrome/browser/printing/pr... chrome/browser/printing/print_preview_tab_controller.h:36: // Get/Create the print preview tab for initiator_tab. On 2010/11/16 18:24:30, James Hawkins wrote: > |initiator_tab| Done. http://codereview.chromium.org/4338001/diff/114001/chrome/browser/printing/pr... chrome/browser/printing/print_preview_tab_controller.h:52: // Returns initiator tab for |preview_tab| or NULL. On 2010/11/16 18:24:30, James Hawkins wrote: > or NULL when...? Done. http://codereview.chromium.org/4338001/diff/114001/chrome/browser/printing/pr... chrome/browser/printing/print_preview_tab_controller.h:55: // Returns preview tab for |tab| or NULL. On 2010/11/16 18:24:30, James Hawkins wrote: > or NULL when...? Done. http://codereview.chromium.org/4338001/diff/114001/chrome/browser/printing/pr... chrome/browser/printing/print_preview_tab_controller.h:73: // A registrar for listening for TAB_CONTENTS_DESTROYED notification. On 2010/11/16 18:24:30, James Hawkins wrote: > s/TAB_CONTENTS_DESTROYED // > > "A registrar for listening for notifications." > > ^ Because it's easy to get out of date (it's already out of date). Done. |
