Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
340039:
Add "Report Bug" dialog to Mac OSX.... (Closed)
|
Created:
11 years, 1 month ago by Miranda Callahan Modified:
9 years, 7 months ago CC:
chromium-reviews_googlegroups.com, John Grabowski, pam+watch_chromium.org, ben+cc_chromium.org, Glenn Wilson Base URL:
svn://chrome-svn/chrome/trunk/src/ Visibility:
Public. |
DescriptionAdd "Report Bug" dialog to Mac OSX.
BUG= http://crbug.com/19282
TEST= Use report bug dialog on Mac OSX.
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=30815
Patch Set 1 #Patch Set 2 : '' #
Total comments: 44
Patch Set 3 : '' #
Total comments: 30
Patch Set 4 : '' #Patch Set 5 : '' #Patch Set 6 : '' #Patch Set 7 : '' #Patch Set 8 : '' #Patch Set 9 : '' #Patch Set 10 : '' #
Total comments: 6
Patch Set 11 : '' #
Total comments: 7
Patch Set 12 : '' #
Total comments: 22
Patch Set 13 : '' #Patch Set 14 : '' #
Total comments: 6
Patch Set 15 : '' #Patch Set 16 : '' #Messages
Total messages: 23 (0 generated)
http://codereview.chromium.org/340039/diff/2001/3002 File chrome/browser/cocoa/bug_report_window_controller.h (right): http://codereview.chromium.org/340039/diff/2001/3002#newcode20 Line 20: IBOutlet NSButton* sendReportButton_; make private as well? http://codereview.chromium.org/340039/diff/2001/3002#newcode56 Line 56: @property (retain, nonatomic) NSButton* sendReportButton; Not sure why you need this property. Is part of a tvltastic l10n thing? http://codereview.chromium.org/340039/diff/2001/3003 File chrome/browser/cocoa/bug_report_window_controller.mm (right): http://codereview.chromium.org/340039/diff/2001/3003#newcode27 Line 27: - (id)initWithTabContents:(TabContents*)currentTab you nit test you nit test one of us one of us http://codereview.chromium.org/340039/diff/2001/3003#newcode67 Line 67: I think you need to implement windowWillClose: to call [self autorelease] and (if not already) make this controller a delegate of the window so the controller is closed when the window is closed. Else you leak the controller.
This is a first pass at some comments -- I haven't actually read the logic very carefully. +1 on the unit test (and run it through valgrind, please). http://codereview.chromium.org/340039/diff/2001/3002 File chrome/browser/cocoa/bug_report_window_controller.h (right): http://codereview.chromium.org/340039/diff/2001/3002#newcode20 Line 20: IBOutlet NSButton* sendReportButton_; On 2009/10/29 22:03:45, John Grabowski wrote: > make private as well? Probably should be, though, if privacy were enforced at all, this would the IBOutlet (see below). You might want to mark this as "weak". http://codereview.chromium.org/340039/diff/2001/3002#newcode31 Line 31: NSString* pageURL_; Please label all these as "strong". http://codereview.chromium.org/340039/diff/2001/3002#newcode38 Line 38: NSArray* bugTypeList_; Ditto. http://codereview.chromium.org/340039/diff/2001/3002#newcode56 Line 56: @property (retain, nonatomic) NSButton* sendReportButton; This shouldn't be "retain" for an IBOutlet. Probably the *correct* thing to do is to make *this* the IBOutlet. http://codereview.chromium.org/340039/diff/2001/3002#newcode58 Line 58: @property (copy) NSString* pageURL; You need to release "copy" members in -dealloc. You can probably also mark all these nonatomic. http://codereview.chromium.org/340039/diff/2001/3003 File chrome/browser/cocoa/bug_report_window_controller.mm (right): http://codereview.chromium.org/340039/diff/2001/3003#newcode14 Line 14: #include "grit/chromium_strings.h" Alphabetical order please. http://codereview.chromium.org/340039/diff/2001/3003#newcode29 Line 29: NSString *nibpath = [mac_util::MainAppBundle() Please pick one of "type *var" or "type* var" and stick to it. Also, I'm not thrilled with your alignment of things here; there's room to put |pathForResource:...| on the preceding line, and line up |ofType:...| on the next line. http://codereview.chromium.org/340039/diff/2001/3003#newcode53 Line 53: std::vector<unsigned char> *screenshot_png = Is there a reason you don't just png_data_(new ...) immediately here? http://codereview.chromium.org/340039/diff/2001/3003#newcode69 Line 69: [bugTypeList_ release]; See my comments in the header file about things you need to release. http://codereview.chromium.org/340039/diff/2001/3003#newcode88 Line 88: sendScreenshot_ && png_data_.get() ? Using a temporary variable might make this clearer. http://codereview.chromium.org/340039/diff/2001/3003#newcode109 Line 109: if (buttonTitle != [sendReportButton_ title]) { Is this really a valid comparison? (This compares pointers....)
Addressed all comments; unit test coming! http://codereview.chromium.org/340039/diff/2001/3002 File chrome/browser/cocoa/bug_report_window_controller.h (right): http://codereview.chromium.org/340039/diff/2001/3002#newcode20 Line 20: IBOutlet NSButton* sendReportButton_; On 2009/10/29 22:03:45, John Grabowski wrote: > make private as well? Done. http://codereview.chromium.org/340039/diff/2001/3002#newcode20 Line 20: IBOutlet NSButton* sendReportButton_; On 2009/10/29 22:58:08, viettrungluu wrote: > On 2009/10/29 22:03:45, John Grabowski wrote: > > make private as well? > > Probably should be, though, if privacy were enforced at all, this would the > IBOutlet (see below). You might want to mark this as "weak". Done. http://codereview.chromium.org/340039/diff/2001/3002#newcode31 Line 31: NSString* pageURL_; On 2009/10/29 22:58:08, viettrungluu wrote: > Please label all these as "strong". Done. http://codereview.chromium.org/340039/diff/2001/3002#newcode38 Line 38: NSArray* bugTypeList_; On 2009/10/29 22:58:08, viettrungluu wrote: > Ditto. Done. http://codereview.chromium.org/340039/diff/2001/3002#newcode56 Line 56: @property (retain, nonatomic) NSButton* sendReportButton; On 2009/10/29 22:03:45, John Grabowski wrote: > Not sure why you need this property. Is part of a tvltastic l10n thing? Kind of -- we have to change the title based on whether or not we have a "phishing page" bug or not... http://codereview.chromium.org/340039/diff/2001/3002#newcode56 Line 56: @property (retain, nonatomic) NSButton* sendReportButton; On 2009/10/29 22:58:08, viettrungluu wrote: > This shouldn't be "retain" for an IBOutlet. Probably the *correct* thing to do > is to make *this* the IBOutlet. Done! http://codereview.chromium.org/340039/diff/2001/3002#newcode58 Line 58: @property (copy) NSString* pageURL; On 2009/10/29 22:58:08, viettrungluu wrote: > You need to release "copy" members in -dealloc. You can probably also mark all > these nonatomic. Done. http://codereview.chromium.org/340039/diff/2001/3003 File chrome/browser/cocoa/bug_report_window_controller.mm (right): http://codereview.chromium.org/340039/diff/2001/3003#newcode14 Line 14: #include "grit/chromium_strings.h" On 2009/10/29 22:58:08, viettrungluu wrote: > Alphabetical order please. Done. http://codereview.chromium.org/340039/diff/2001/3003#newcode27 Line 27: - (id)initWithTabContents:(TabContents*)currentTab On 2009/10/29 22:03:45, John Grabowski wrote: > you nit test > you nit test > one of us > one of us > Done. http://codereview.chromium.org/340039/diff/2001/3003#newcode29 Line 29: NSString *nibpath = [mac_util::MainAppBundle() On 2009/10/29 22:58:08, viettrungluu wrote: > Please pick one of "type *var" or "type* var" and stick to it. Also, I'm not > thrilled with your alignment of things here; there's room to put > |pathForResource:...| on the preceding line, and line up |ofType:...| on the > next line. Done. http://codereview.chromium.org/340039/diff/2001/3003#newcode53 Line 53: std::vector<unsigned char> *screenshot_png = On 2009/10/29 22:58:08, viettrungluu wrote: > Is there a reason you don't just png_data_(new ...) immediately here? Done. http://codereview.chromium.org/340039/diff/2001/3003#newcode67 Line 67: On 2009/10/29 22:03:45, John Grabowski wrote: > I think you need to implement windowWillClose: to call [self autorelease] and > (if not already) make this controller a delegate of the window so the controller > is closed when the window is closed. Else you leak the controller. Done. http://codereview.chromium.org/340039/diff/2001/3003#newcode69 Line 69: [bugTypeList_ release]; On 2009/10/29 22:58:08, viettrungluu wrote: > See my comments in the header file about things you need to release. Done. http://codereview.chromium.org/340039/diff/2001/3003#newcode88 Line 88: sendScreenshot_ && png_data_.get() ? On 2009/10/29 22:58:08, viettrungluu wrote: > Using a temporary variable might make this clearer. Done. http://codereview.chromium.org/340039/diff/2001/3003#newcode109 Line 109: if (buttonTitle != [sendReportButton_ title]) { On 2009/10/29 22:58:08, viettrungluu wrote: > Is this really a valid comparison? (This compares pointers....) Well, it's working fine, but I agree that it doesn't seem kosher. Changed.
http://codereview.chromium.org/340039/diff/2001/3003 File chrome/browser/cocoa/bug_report_window_controller.mm (right): http://codereview.chromium.org/340039/diff/2001/3003#newcode27 Line 27: - (id)initWithTabContents:(TabContents*)currentTab On 2009/10/30 00:19:16, Miranda Callahan wrote: > On 2009/10/29 22:03:45, John Grabowski wrote: > > you nit test > > you nit test > > one of us > > one of us > > > > Done. u r a liar
http://codereview.chromium.org/340039/diff/2001/3003 File chrome/browser/cocoa/bug_report_window_controller.mm (right): http://codereview.chromium.org/340039/diff/2001/3003#newcode27 Line 27: - (id)initWithTabContents:(TabContents*)currentTab On 2009/10/30 00:32:31, John Grabowski wrote: > On 2009/10/30 00:19:16, Miranda Callahan wrote: > > On 2009/10/29 22:03:45, John Grabowski wrote: > > > you nit test > > > you nit test > > > one of us > > > one of us > > > > > > > Done. > > u r a liar > Oops, I meant "done in the future imperfect tense" -- properly spelled "done-ish". Coming soon to theaters near you!
originally i filed one bug for this in the tool menu and one for this in the menu bar. It would be really nice for the menubar one to work even if there was no browser window open. it doesn't look like that is doable at the moment. http://codereview.chromium.org/340039/diff/2001/3002 File chrome/browser/cocoa/bug_report_window_controller.h (right): http://codereview.chromium.org/340039/diff/2001/3002#newcode27 Line 27: scoped_ptr< std::vector<unsigned char> > png_data_; i thought style guide said all objc style in objc code? why use a middle underscore here? http://codereview.chromium.org/340039/diff/2001/3002#newcode45 Line 45: // send the report of the bug or broken web site. app model? should it be a sheet/tab modal? http://codereview.chromium.org/340039/diff/2001/3003 File chrome/browser/cocoa/bug_report_window_controller.mm (right): http://codereview.chromium.org/340039/diff/2001/3003#newcode48 Line 48: nil]]; this looks like it should leak, an alloc/init usually needs a release/autorelease, the setting should then do it's own retain or copy. what is this list for? it looks a lot like a menu list of some kind, why not do this all in the nib directly? http://codereview.chromium.org/340039/diff/2001/3003#newcode99 Line 99: - (BOOL)isPhishingReport { general comment - i don't like this. the name makes it appear to be an accessor, but it actually is changing a button title and moving a button? it also seems to get called when we go to send the report? shouldn't it be changing as the user toggle the report type in the ui? http://codereview.chromium.org/340039/diff/2001/3003#newcode112 Line 112: [GTMUILocalizerAndLayoutTweaker sizeToFitView:sendReportButton_]; this returns an NSSize, just catch that width and you have the amount you need to move the button. http://codereview.chromium.org/340039/diff/2001/3003#newcode117 Line 117: [[sendReportButton_ superview] setNeedsDisplay:YES]; is this really needed? why inval the whole parent, just inval before and after the resize/move. no point it making the dirty area bigger then need be.
http://codereview.chromium.org/340039/diff/8/1011 File chrome/browser/cocoa/bug_report_window_controller.mm (right): http://codereview.chromium.org/340039/diff/8/1011#newcode89 Line 89: unsigned char png_data_chars = (*png_data_.get())[0]; um, this is gonna copy out the first byte of data, so when you pass this off below that call is gonna read crap off the stack hoping it is a png...
Drive-by. http://codereview.chromium.org/340039/diff/8/1010 File chrome/browser/cocoa/bug_report_window_controller.h (right): http://codereview.chromium.org/340039/diff/8/1010#newcode30 Line 30: NSUInteger bugType_; // Strong. Having "Strong" on non-pointers looks weird to me. Is this something about bindings? What does "Strong" mean in this context? http://codereview.chromium.org/340039/diff/8/1010#newcode34 Line 34: BOOL sendScreenshot_; // Strong. Likewise. http://codereview.chromium.org/340039/diff/8/1011 File chrome/browser/cocoa/bug_report_window_controller.mm (right): http://codereview.chromium.org/340039/diff/8/1011#newcode117 Line 117: // if (buttonTitle != [sendReportButton_ title]) { Remove comment
http://codereview.chromium.org/340039/diff/8/1012 File chrome/browser/cocoa/browser_window_cocoa.mm (right): http://codereview.chromium.org/340039/diff/8/1012#newcode249 Line 249: if ((current_tab) and (current_tab->controller().GetActiveEntry())) { no need for extra parens here. This should just read: if (current_tab && current_tab->controller()....) { I don't think we use |and| and |or| anywhere in the chrome code. You might want to check with brettw/darin on their preferences. http://codereview.chromium.org/340039/diff/8/1012#newcode250 Line 250: scoped_nsobject<BugReportWindowController> controller( don't wrap window controllers with scoped objects. This will double-release the object when you autorelease it in the windowWillClose. http://codereview.chromium.org/340039/diff/8/1010 File chrome/browser/cocoa/bug_report_window_controller.h (right): http://codereview.chromium.org/340039/diff/8/1010#newcode18 Line 18: @interface BugReportWindowController : NSWindowController { needs a class-level comment on what it does, how it's used, etc etc. http://codereview.chromium.org/340039/diff/8/1010#newcode27 Line 27: scoped_ptr< std::vector<unsigned char> > png_data_; pngData, per obj-c style guide. http://codereview.chromium.org/340039/diff/8/1010#newcode36 Line 36: // Menu for the bug type popup button. We create it here instead of in the omit "the". It should read "We create it here instead of in IB so that...". http://codereview.chromium.org/340039/diff/8/1010#newcode41 Line 41: // Initialize with the contents of the tab to be reported as buggy / wrong. Can either be null? What happens? http://codereview.chromium.org/340039/diff/8/1010#newcode52 Line 52: // True if the user has selected the phishing report option. BOOLs are YES or NO, not true/false. http://codereview.chromium.org/340039/diff/8/1010#newcode55 Line 55: // Properties for bindings period at end of sentence. http://codereview.chromium.org/340039/diff/8/1011 File chrome/browser/cocoa/bug_report_window_controller.mm (right): http://codereview.chromium.org/340039/diff/8/1011#newcode52 Line 52: png_data_.reset(new std::vector<unsigned char>); why lazily create the vector on the heap? Why not just make it a member variable? the vector should heap-allocate the storage space anyway, and then you don't have to worry about the scoped_ptr. http://codereview.chromium.org/340039/diff/8/1011#newcode118 Line 118: if (![buttonTitle isEqualToString: [sendReportButton_ title]]) { no space after colon http://codereview.chromium.org/340039/diff/8/1011#newcode125 Line 125: [[sendReportButton_ superview] setNeedsDisplay:YES]; is this needed?
All right; I think I addressed everyone's comments here. I also added a unit test, and hooked the dialog box into the main menu. After discussion with Trung and reading TVL's comment, I think it makes sense to have this dialog be a tab modal sheet when it is called from a specific misbehaving page, but remain a modal dialog when called from the main menu with no browser window open. In the interest of not having this CL go on for yet another week, however, I would like to finish this up with the app modal dialog as is, and file a bug against myself to fix in the future, if that sounds ok. http://codereview.chromium.org/340039/diff/2001/3002 File chrome/browser/cocoa/bug_report_window_controller.h (right): http://codereview.chromium.org/340039/diff/2001/3002#newcode27 Line 27: scoped_ptr< std::vector<unsigned char> > png_data_; On 2009/10/30 01:37:47, TVL wrote: > i thought style guide said all objc style in objc code? why use a middle > underscore here? Fixed. http://codereview.chromium.org/340039/diff/2001/3002#newcode45 Line 45: // send the report of the bug or broken web site. On 2009/10/30 01:37:47, TVL wrote: > app model? should it be a sheet/tab modal? It should probably be a modal dialog when called from the main menu with no browser open, and a tab modal sheet when called from a specific page. For now, I changed the initialization method to give different menu options when called with an open window as opposed to with no windows at all open. I'd like to check this in, and file a bug against myself to change the call from a specific page to tab modal, if that's ok. http://codereview.chromium.org/340039/diff/2001/3003 File chrome/browser/cocoa/bug_report_window_controller.mm (right): http://codereview.chromium.org/340039/diff/2001/3003#newcode48 Line 48: nil]]; On 2009/10/30 01:37:47, TVL wrote: > this looks like it should leak, an alloc/init usually needs a > release/autorelease, the setting should then do it's own retain or copy. > > what is this list for? it looks a lot like a menu list of some kind, why not do > this all in the nib directly? > I release the bugTypeList in dealloc; do I need to do something more? I am creating this here because I need to check the value of the selected bugtype list item to see if it's the "phishing page;" I'd rather not test against text values from the nib. Also I now create the menu differently based on whether or not a browser window is open. http://codereview.chromium.org/340039/diff/2001/3003#newcode99 Line 99: - (BOOL)isPhishingReport { On 2009/10/30 01:37:47, TVL wrote: > general comment - i don't like this. the name makes it appear to be an > accessor, but it actually is changing a button title and moving a button? it > also seems to get called when we go to send the report? shouldn't it be > changing as the user toggle the report type in the ui? Good point; I split into two methods. It is getting toggled as user changes report type; see keyPathsForValuesAffectingValueForKey. http://codereview.chromium.org/340039/diff/2001/3003#newcode112 Line 112: [GTMUILocalizerAndLayoutTweaker sizeToFitView:sendReportButton_]; On 2009/10/30 01:37:47, TVL wrote: > this returns an NSSize, just catch that width and you have the amount you need > to move the button. Done. http://codereview.chromium.org/340039/diff/2001/3003#newcode117 Line 117: [[sendReportButton_ superview] setNeedsDisplay:YES]; On 2009/10/30 01:37:47, TVL wrote: > is this really needed? why inval the whole parent, just inval before and after > the resize/move. no point it making the dirty area bigger then need be. Done. http://codereview.chromium.org/340039/diff/8/1012 File chrome/browser/cocoa/browser_window_cocoa.mm (right): http://codereview.chromium.org/340039/diff/8/1012#newcode249 Line 249: if ((current_tab) and (current_tab->controller().GetActiveEntry())) { On 2009/10/30 14:45:30, pink wrote: > no need for extra parens here. This should just read: > > if (current_tab && current_tab->controller()....) { > > I don't think we use |and| and |or| anywhere in the chrome code. You might want > to check with brettw/darin on their preferences. Done. http://codereview.chromium.org/340039/diff/8/1012#newcode250 Line 250: scoped_nsobject<BugReportWindowController> controller( On 2009/10/30 14:45:30, pink wrote: > don't wrap window controllers with scoped objects. This will double-release the > object when you autorelease it in the windowWillClose. Done. http://codereview.chromium.org/340039/diff/8/1010 File chrome/browser/cocoa/bug_report_window_controller.h (right): http://codereview.chromium.org/340039/diff/8/1010#newcode18 Line 18: @interface BugReportWindowController : NSWindowController { On 2009/10/30 14:45:30, pink wrote: > needs a class-level comment on what it does, how it's used, etc etc. Done. http://codereview.chromium.org/340039/diff/8/1010#newcode27 Line 27: scoped_ptr< std::vector<unsigned char> > png_data_; On 2009/10/30 14:45:30, pink wrote: > pngData, per obj-c style guide. Done. http://codereview.chromium.org/340039/diff/8/1010#newcode30 Line 30: NSUInteger bugType_; // Strong. On 2009/10/30 01:47:03, Nico wrote: > Having "Strong" on non-pointers looks weird to me. Is this something about > bindings? What does "Strong" mean in this context? No, I was trying to fix things quickly, which I shouldn't have. Removed. http://codereview.chromium.org/340039/diff/8/1010#newcode34 Line 34: BOOL sendScreenshot_; // Strong. On 2009/10/30 01:47:03, Nico wrote: > Likewise. Done. http://codereview.chromium.org/340039/diff/8/1010#newcode36 Line 36: // Menu for the bug type popup button. We create it here instead of in the On 2009/10/30 14:45:30, pink wrote: > omit "the". It should read "We create it here instead of in IB so that...". Done. http://codereview.chromium.org/340039/diff/8/1010#newcode41 Line 41: // Initialize with the contents of the tab to be reported as buggy / wrong. On 2009/10/30 14:45:30, pink wrote: > Can either be null? What happens? Added comment. http://codereview.chromium.org/340039/diff/8/1010#newcode52 Line 52: // True if the user has selected the phishing report option. On 2009/10/30 14:45:30, pink wrote: > BOOLs are YES or NO, not true/false. Done. http://codereview.chromium.org/340039/diff/8/1010#newcode55 Line 55: // Properties for bindings On 2009/10/30 14:45:30, pink wrote: > period at end of sentence. Done. http://codereview.chromium.org/340039/diff/8/1011 File chrome/browser/cocoa/bug_report_window_controller.mm (right): http://codereview.chromium.org/340039/diff/8/1011#newcode52 Line 52: png_data_.reset(new std::vector<unsigned char>); On 2009/10/30 14:45:30, pink wrote: > why lazily create the vector on the heap? Why not just make it a member > variable? the vector should heap-allocate the storage space anyway, and then you > don't have to worry about the scoped_ptr. Done. http://codereview.chromium.org/340039/diff/8/1011#newcode89 Line 89: unsigned char png_data_chars = (*png_data_.get())[0]; On 2009/10/30 01:45:49, TVL wrote: > um, this is gonna copy out the first byte of data, so when you pass this off > below that call is gonna read crap off the stack hoping it is a png... ack, yes -- changed back to version 1. http://codereview.chromium.org/340039/diff/8/1011#newcode117 Line 117: // if (buttonTitle != [sendReportButton_ title]) { On 2009/10/30 01:47:03, Nico wrote: > Remove comment Done. http://codereview.chromium.org/340039/diff/8/1011#newcode118 Line 118: if (![buttonTitle isEqualToString: [sendReportButton_ title]]) { On 2009/10/30 14:45:30, pink wrote: > no space after colon Done. http://codereview.chromium.org/340039/diff/8/1011#newcode125 Line 125: [[sendReportButton_ superview] setNeedsDisplay:YES]; On 2009/10/30 14:45:30, pink wrote: > is this needed? No; removed.
LGTM if URL and Title filled in for user. http://codereview.chromium.org/340039/diff/5003/5007 File chrome/browser/cocoa/bug_report_window_controller.mm (right): http://codereview.chromium.org/340039/diff/5003/5007#newcode70 Line 70: [self setPageURL:@""]; On Windows this is filled in for the user. http://codereview.chromium.org/340039/diff/5003/5007#newcode71 Line 71: [self setPageTitle:@""]; On Windows this is filled in for the user. http://codereview.chromium.org/340039/diff/5003/5007#newcode77 Line 77: // Delegate callback; closing the window deletes the controller. Delegate callback SO THAT closing the window deletes the controller. Be clear your comment implies behavior you are adding and not something implicit. http://codereview.chromium.org/340039/diff/1022/22#newcode142 Line 142: + (NSSet*)keyPathsForValuesAffectingValueForKey:(NSString*)key { Docstring why you need to override this. http://codereview.chromium.org/340039/diff/1022/26 File chrome/browser/cocoa/bug_report_window_controller_unittest.mm (right): http://codereview.chromium.org/340039/diff/1022/26#newcode42 Line 42: [controller setBugType: 2]; [obj selector:<space>arg] seems unusual. Can we keep [obj selector:arg]? http://codereview.chromium.org/340039/diff/1022/26#newcode47 Line 47: EXPECT_FALSE([controller checkForPhishAndUpdateUI]); Change something in the UI to confirm something, anything is hooked up?
http://codereview.chromium.org/340039/diff/5003/5007 File chrome/browser/cocoa/bug_report_window_controller.mm (right): http://codereview.chromium.org/340039/diff/5003/5007#newcode70 Line 70: [self setPageURL:@""]; On 2009/11/01 21:54:15, John Grabowski wrote: > On Windows this is filled in for the user. Yes, it is filled in for Mac as well when there is a page open; this is specifically for the case that there is no open browser, and "Report Bug" is being called from the main menu. Will add comment to make this clear. http://codereview.chromium.org/340039/diff/5003/5007#newcode71 Line 71: [self setPageTitle:@""]; On 2009/11/01 21:54:15, John Grabowski wrote: > On Windows this is filled in for the user. See above. http://codereview.chromium.org/340039/diff/5003/5007#newcode77 Line 77: // Delegate callback; closing the window deletes the controller. On 2009/11/01 21:54:15, John Grabowski wrote: > Delegate callback SO THAT closing the window deletes the controller. > Be clear your comment implies behavior you are adding and not something > implicit. Done. http://codereview.chromium.org/340039/diff/1022/22#newcode142 Line 142: + (NSSet*)keyPathsForValuesAffectingValueForKey:(NSString*)key { On 2009/11/01 21:54:15, John Grabowski wrote: > Docstring why you need to override this. Done. http://codereview.chromium.org/340039/diff/1022/26 File chrome/browser/cocoa/bug_report_window_controller_unittest.mm (right): http://codereview.chromium.org/340039/diff/1022/26#newcode42 Line 42: [controller setBugType: 2]; On 2009/11/01 21:54:15, John Grabowski wrote: > [obj selector:<space>arg] seems unusual. Can we keep [obj selector:arg]? > Done. http://codereview.chromium.org/340039/diff/1022/26#newcode47 Line 47: EXPECT_FALSE([controller checkForPhishAndUpdateUI]); On 2009/11/01 21:54:15, John Grabowski wrote: > Change something in the UI to confirm something, anything is hooked up? Hmm. The tests below check to make sure that the URL and page title have been correctly recorded. As far as testing user changes to the textfields, I'm not sure how to do that except through the controller -- so I would be doing a setPageURL and then a getter on pageURL and testing for equality -- but that doesn't seem worthwhile. Is there a smarter way to test these changes that I'm not seeing?
Still LGTM, but... http://codereview.chromium.org/340039/diff/1022/26 File chrome/browser/cocoa/bug_report_window_controller_unittest.mm (right): http://codereview.chromium.org/340039/diff/1022/26#newcode47 Line 47: EXPECT_FALSE([controller checkForPhishAndUpdateUI]); On 2009/11/01 22:42:55, Miranda Callahan wrote: > On 2009/11/01 21:54:15, John Grabowski wrote: > > Change something in the UI to confirm something, anything is hooked up? > > Hmm. The tests below check to make sure that the URL and page title have been > correctly recorded. As far as testing user changes to the textfields, I'm not > sure how to do that except through the controller -- so I would be doing a > setPageURL and then a getter on pageURL and testing for equality -- but that > doesn't seem worthwhile. Is there a smarter way to test these changes that I'm > not seeing? > Maybe not. http://codereview.chromium.org/340039/diff/2024/9007 File chrome/browser/cocoa/bug_report_window_controller.mm (right): http://codereview.chromium.org/340039/diff/2024/9007#newcode148 Line 148: + (NSSet*)keyPathsForValuesAffectingValueForKey:(NSString*)key { I don't understand. In the nib, the "Send Report" button (title ^IDS_BUGREPORT_SEND_REPORT) has no Cocoa binding. Neither does it's cell. The big text box next to the ^IDS_BUGREPORT_DESCRIPTION_LABEL name does have "enabled" bound to checkForPhishAndUpdateUI. Is that what you mean?
As a sidenote, the current "Report Bug" dialog for Windows is up for a very much needed revamp. Gwilson has more details.
As a sidenote, the current "Report Bug" dialog for Windows is up for a very much needed revamp. Gwilson has more details.
http://codereview.chromium.org/340039/diff/2024/9010 File chrome/browser/app_controller_mac.mm (right): http://codereview.chromium.org/340039/diff/2024/9010#newcode484 Line 484: } else if (action == @selector(reportBug:)) { why did you have to use a direct wired selector? couldn't you just use commandDispatch like we do about 10 lines up for enabling and then catch it directly below? Then you'll only catch it when there is no front browser. http://codereview.chromium.org/340039/diff/2024/9007 File chrome/browser/cocoa/bug_report_window_controller.mm (right): http://codereview.chromium.org/340039/diff/2024/9007#newcode129 Line 129: - (BOOL)checkForPhishAndUpdateUI { I'm still not really a fan of this because it's a getter with a major side effect, someone could change the code and not call this at the same time and the button would get out of sync. It seems like the right patters is to make this class the delegate for the menu and have it listen for changes to the selection or provided a custom impl of SetButType (just synthesize the getter). That way any time the bug type changes, you can directly get the button in sync again. http://codereview.chromium.org/340039/diff/2024/9011 File chrome/browser/cocoa/bug_report_window_controller_unittest.mm (right): http://codereview.chromium.org/340039/diff/2024/9011#newcode23 Line 23: RenderViewHostTestHarness::TearDown(); why do you have to override and call the parent methods? http://codereview.chromium.org/340039/diff/2024/9011#newcode55 Line 55: EXPECT_TRUE([[controller bugTypeList] count] == 8); expect_eq? http://codereview.chromium.org/340039/diff/2024/9011#newcode74 Line 74: EXPECT_TRUE([[controller bugTypeList] count] == 4); expect_eq?
http://codereview.chromium.org/340039/diff/2024/9010 File chrome/browser/app_controller_mac.mm (right): http://codereview.chromium.org/340039/diff/2024/9010#newcode683 Line 683: BugReportWindowController* controller = [[BugReportWindowController alloc] i'd put everything after the = on a new line, indented 4 spaces from the previous line. That'll allow you to better align the rest of the call. http://codereview.chromium.org/340039/diff/2024/9008 File chrome/browser/cocoa/browser_window_cocoa.mm (right): http://codereview.chromium.org/340039/diff/2024/9008#newcode250 Line 250: BugReportWindowController* controller = [[BugReportWindowController alloc] put everything after = on a new line http://codereview.chromium.org/340039/diff/2024/9006 File chrome/browser/cocoa/bug_report_window_controller.h (right): http://codereview.chromium.org/340039/diff/2024/9006#newcode29 Line 29: // Values bound to data in the dialog box. Maybe mention that they can't be scoped_nsobjects because we used them for bindings. http://codereview.chromium.org/340039/diff/2024/9006#newcode58 Line 58: - (void)closeDialog; does anyone need to call this publicly? http://codereview.chromium.org/340039/diff/2024/9007 File chrome/browser/cocoa/bug_report_window_controller.mm (right): http://codereview.chromium.org/340039/diff/2024/9007#newcode90 Line 90: - (void)dealloc { usually dealloc goes right after the init in the file, just to keep them together.
I think I addressed all the concerns; please take another look -- esp. TVL, as I made more substantial changes in response to your comments. Thanks again for your patience and help. http://codereview.chromium.org/340039/diff/2024/9010 File chrome/browser/app_controller_mac.mm (right): http://codereview.chromium.org/340039/diff/2024/9010#newcode484 Line 484: } else if (action == @selector(reportBug:)) { On 2009/11/02 12:41:09, TVL wrote: > why did you have to use a direct wired selector? couldn't you just use > commandDispatch like we do about 10 lines up for enabling and then catch it > directly below? Then you'll only catch it when there is no front browser. Done. http://codereview.chromium.org/340039/diff/2024/9010#newcode683 Line 683: BugReportWindowController* controller = [[BugReportWindowController alloc] On 2009/11/02 16:31:31, pink wrote: > i'd put everything after the = on a new line, indented 4 spaces from the > previous line. That'll allow you to better align the rest of the call. Done. http://codereview.chromium.org/340039/diff/2024/9008 File chrome/browser/cocoa/browser_window_cocoa.mm (right): http://codereview.chromium.org/340039/diff/2024/9008#newcode250 Line 250: BugReportWindowController* controller = [[BugReportWindowController alloc] On 2009/11/02 16:31:31, pink wrote: > put everything after = on a new line Done. http://codereview.chromium.org/340039/diff/2024/9006 File chrome/browser/cocoa/bug_report_window_controller.h (right): http://codereview.chromium.org/340039/diff/2024/9006#newcode29 Line 29: // Values bound to data in the dialog box. On 2009/11/02 16:31:31, pink wrote: > Maybe mention that they can't be scoped_nsobjects because we used them for > bindings. Done. http://codereview.chromium.org/340039/diff/2024/9006#newcode58 Line 58: - (void)closeDialog; On 2009/11/02 16:31:31, pink wrote: > does anyone need to call this publicly? No; removed. http://codereview.chromium.org/340039/diff/2024/9007 File chrome/browser/cocoa/bug_report_window_controller.mm (right): http://codereview.chromium.org/340039/diff/2024/9007#newcode90 Line 90: - (void)dealloc { On 2009/11/02 16:31:31, pink wrote: > usually dealloc goes right after the init in the file, just to keep them > together. Oh, of course -- like a dtor. Fixed. http://codereview.chromium.org/340039/diff/2024/9007#newcode129 Line 129: - (BOOL)checkForPhishAndUpdateUI { On 2009/11/02 12:41:09, TVL wrote: > I'm still not really a fan of this because it's a getter with a major side > effect, someone could change the code and not call this at the same time and the > button would get out of sync. It seems like the right patters is to make this > class the delegate for the menu and have it listen for changes to the selection > or provided a custom impl of SetButType (just synthesize the getter). That way > any time the bug type changes, you can directly get the button in sync again. Implemented! http://codereview.chromium.org/340039/diff/2024/9007#newcode148 Line 148: + (NSSet*)keyPathsForValuesAffectingValueForKey:(NSString*)key { On 2009/11/01 23:53:18, John Grabowski wrote: > I don't understand. > In the nib, the "Send Report" button (title ^IDS_BUGREPORT_SEND_REPORT) has no > Cocoa binding. Neither does it's cell. > > The big text box next to the ^IDS_BUGREPORT_DESCRIPTION_LABEL name does have > "enabled" bound to checkForPhishAndUpdateUI. Is that what you mean? > > This was my roundabout way of forcing the button to resize when the bugType changed; I've redone this according to tvl's suggestion above, and it's more elegant. http://codereview.chromium.org/340039/diff/2024/9011 File chrome/browser/cocoa/bug_report_window_controller_unittest.mm (right): http://codereview.chromium.org/340039/diff/2024/9011#newcode23 Line 23: RenderViewHostTestHarness::TearDown(); On 2009/11/02 12:41:09, TVL wrote: > why do you have to override and call the parent methods? Good question. Fixed. http://codereview.chromium.org/340039/diff/2024/9011#newcode55 Line 55: EXPECT_TRUE([[controller bugTypeList] count] == 8); On 2009/11/02 12:41:09, TVL wrote: > expect_eq? Done. http://codereview.chromium.org/340039/diff/2024/9011#newcode74 Line 74: EXPECT_TRUE([[controller bugTypeList] count] == 4); On 2009/11/02 12:41:09, TVL wrote: > expect_eq? Done.
On 2009/11/02 05:50:15, jeremy wrote: > As a sidenote, the current "Report Bug" dialog for Windows is up for a very much > needed revamp. Gwilson has more details. Glenn, could you let me know the status of this -- and assign a bug to me if need be?
might have some extra code left around. otherwise, lg, tx. http://codereview.chromium.org/340039/diff/8004/8011 File chrome/browser/app_controller_mac.mm (right): http://codereview.chromium.org/340039/diff/8004/8011#newcode470 Line 470: case IDC_REPORT_BUG: the comment above seems to imply you don't need this here, it can always return true. http://codereview.chromium.org/340039/diff/8004/8011#newcode569 Line 569: menuState_->UpdateCommandEnabled(IDC_REPORT_BUG, false); why false? http://codereview.chromium.org/340039/diff/8004/8011#newcode689 Line 689: - (IBAction)reportBug:(id)sender { i believe this can go away if you are using the command now.
http://codereview.chromium.org/340039/diff/8004/8011 File chrome/browser/app_controller_mac.mm (right): http://codereview.chromium.org/340039/diff/8004/8011#newcode470 Line 470: case IDC_REPORT_BUG: On 2009/11/02 22:28:36, TVL wrote: > the comment above seems to imply you don't need this here, it can always return > true. Done. http://codereview.chromium.org/340039/diff/8004/8011#newcode569 Line 569: menuState_->UpdateCommandEnabled(IDC_REPORT_BUG, false); On 2009/11/02 22:28:36, TVL wrote: > why false? left over from an experiment; removed. http://codereview.chromium.org/340039/diff/8004/8011#newcode689 Line 689: - (IBAction)reportBug:(id)sender { On 2009/11/02 22:28:36, TVL wrote: > i believe this can go away if you are using the command now. indeed, removed.
lgtm |
