Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2094583004:
Initial commit for Chrome metainstaller on Mac. (Closed)
DescriptionInitial commit for Chrome metainstaller on Mac.
NOTRY=true
Committed: https://crrev.com/73447cf7a9a9c28d59013993f14eff3c84840013
Cr-Commit-Position: refs/heads/master@{#405140}
Patch Set 1 #
Total comments: 23
Patch Set 2 : Addressing roughly half of the comments from the first CL, added some comments as suggested. #Patch Set 3 : First step to transitioning functional request.* into separate classes. #Patch Set 4 : Additional progress addressing downloader and parser, moved BUILD dependencies away from root. #
Total comments: 26
Patch Set 5 : Many adjustments made to improve code health and style. Replaced request.* with NetworkCommunicatio… #Patch Set 6 : This is -- really -- patch 5.5. No need to look at request.*! #Patch Set 7 : Fully addressed making the .dmg download asynchronously #
Total comments: 35
Patch Set 8 : Fixed majority of comments excluding refactoring for blocks in main. #
Total comments: 9
Patch Set 9 : Addressed all comments except main block comment from elly #
Total comments: 23
Patch Set 10 : Made small changes according to LGTM comments #Messages
Total messages: 61 (16 generated)
Description was changed from ========== Initial commit for Chrome metainstaller on Mac. BUG= ========== to ========== Initial commit for Chrome metainstaller on Mac. BUG= ==========
zengster@google.com changed reviewers: + ellyjones@chromium.org, mark@chromium.org
zengster@google.com changed reviewers: + ivanhernandez@google.com
Not bad! I think the design might need a little bit of reworking but the style is mostly spot-on and the code is quite clean and readable. Good work :) https://codereview.chromium.org/2094583004/diff/1/chrome/installer/mac/app/OW... File chrome/installer/mac/app/OWNERS (right): https://codereview.chromium.org/2094583004/diff/1/chrome/installer/mac/app/OW... chrome/installer/mac/app/OWNERS:1: mmentovai@google.com OWNERS entries should be @chromium.org, since those are the accounts we mostly use for code review :) https://codereview.chromium.org/2094583004/diff/1/chrome/installer/mac/app/do... File chrome/installer/mac/app/downloader.h (right): https://codereview.chromium.org/2094583004/diff/1/chrome/installer/mac/app/do... chrome/installer/mac/app/downloader.h:10: NSURL* getChromeImageURL(NSData*); These functions should have comments describing what they do and names for their parameters. Also, it might be best if these were all part of a Downloader class, which could handle all the steps here together. https://codereview.chromium.org/2094583004/diff/1/chrome/installer/mac/app/do... chrome/installer/mac/app/downloader.h:13: void writeDataToDisk(NSData*); This name needs to be longer and more descriptive, specifically about what the data is and where it gets written. Perhaps something like 'writeChromeImageToDownloadsDirectory'? Although that kind of function sort of suggests that a class might be better here. https://codereview.chromium.org/2094583004/diff/1/chrome/installer/mac/app/do... File chrome/installer/mac/app/downloader.m (right): https://codereview.chromium.org/2094583004/diff/1/chrome/installer/mac/app/do... chrome/installer/mac/app/downloader.m:14: NSArray *downloadLinks = [NSArray arrayWithArray: parser.chromeDownloadURLs]; no space after colon https://codereview.chromium.org/2094583004/diff/1/chrome/installer/mac/app/do... chrome/installer/mac/app/downloader.m:15: NSString *chromeURLString = [downloadLinks firstObject]; can downloadLinks ever be empty? What if parser fails? https://codereview.chromium.org/2094583004/diff/1/chrome/installer/mac/app/do... chrome/installer/mac/app/downloader.m:21: return [NSData dataWithContentsOfURL:chromeURL]; [NSData dataWithContentsOfURL:] will do the entire download synchronously on the UI thread, so the entire app will be non-responsive while this call is running, which could be quite some time. There's a note in the documentation for that method about this problem. I think we need to do an asynchronous download using [NSURLSession downloadTaskWithURL:] instead. https://codereview.chromium.org/2094583004/diff/1/chrome/installer/mac/app/do... chrome/installer/mac/app/downloader.m:29: downloadDirectory, @"GoogleChrome.dmg"]; I think it would be nicer to use [NSString pathWithComponents:] here instead. https://codereview.chromium.org/2094583004/diff/1/chrome/installer/mac/app/do... chrome/installer/mac/app/downloader.m:33: void writeDataToDisk(NSData* topSecretData) { is it really top secret? :) https://codereview.chromium.org/2094583004/diff/1/chrome/installer/mac/app/ma... File chrome/installer/mac/app/main.m (right): https://codereview.chromium.org/2094583004/diff/1/chrome/installer/mac/app/ma... chrome/installer/mac/app/main.m:16: NSData *topSecretData = downloadChromeAsData(chromeURL); chromeImageData? https://codereview.chromium.org/2094583004/diff/1/chrome/installer/mac/app/pa... File chrome/installer/mac/app/parser.h (right): https://codereview.chromium.org/2094583004/diff/1/chrome/installer/mac/app/pa... chrome/installer/mac/app/parser.h:14: - (id)init; These need method comments. https://codereview.chromium.org/2094583004/diff/1/chrome/installer/mac/app/pa... chrome/installer/mac/app/parser.h:15: - (void)parseXML:(NSData*)someData; someXml? https://codereview.chromium.org/2094583004/diff/1/chrome/installer/mac/app/pa... File chrome/installer/mac/app/parser.m (right): https://codereview.chromium.org/2094583004/diff/1/chrome/installer/mac/app/pa... chrome/installer/mac/app/parser.m:10: if( self = [super init] ) { proper style is "if ((self = [super init])) {" for this https://codereview.chromium.org/2094583004/diff/1/chrome/installer/mac/app/pa... chrome/installer/mac/app/parser.m:16: - (void)parseXML:(NSData*)someData { This probably needs more robust error handling than just NSLog https://codereview.chromium.org/2094583004/diff/1/chrome/installer/mac/app/pa... chrome/installer/mac/app/parser.m:27: - (void)appendFilenameToURL { This definitely needs an explanatory comment. https://codereview.chromium.org/2094583004/diff/1/chrome/installer/mac/app/pa... chrome/installer/mac/app/parser.m:36: - (void)parser:(NSXMLParser *)parser This should have a comment remarking that it's part of XMLParserDelegate https://codereview.chromium.org/2094583004/diff/1/chrome/installer/mac/app/re... File chrome/installer/mac/app/request.h (right): https://codereview.chromium.org/2094583004/diff/1/chrome/installer/mac/app/re... chrome/installer/mac/app/request.h:6: #error "What are you doing?" This error should be more descriptive (and probably inside the include guards) https://codereview.chromium.org/2094583004/diff/1/chrome/installer/mac/app/re... chrome/installer/mac/app/request.h:13: #include <mach-o/arch.h> The constants defined in that header aren't used in this header, so we should include it in the implementation file instead of the header. https://codereview.chromium.org/2094583004/diff/1/chrome/installer/mac/app/re... chrome/installer/mac/app/request.h:16: NSString* getArchitecture(void); These belong on a class probably (maybe as static methods) instead of as free functions. https://codereview.chromium.org/2094583004/diff/1/chrome/installer/mac/app/re... chrome/installer/mac/app/request.h:16: NSString* getArchitecture(void); These functions need explanatory comments too. https://codereview.chromium.org/2094583004/diff/1/chrome/installer/mac/app/re... File chrome/installer/mac/app/request.m (right): https://codereview.chromium.org/2094583004/diff/1/chrome/installer/mac/app/re... chrome/installer/mac/app/request.m:12: const NSString *omahaEndpoint = @"https://tools.google.com/service/update2"; this can be local to the function where it's used https://codereview.chromium.org/2094583004/diff/1/chrome/installer/mac/app/re... chrome/installer/mac/app/request.m:13: static NSData *payload; having a global for this value (which is only set or used in one function) isn't a good design pattern. This should just be an ordinary variable in sendRequestAndReceiveResponse. https://codereview.chromium.org/2094583004/diff/1/chrome/installer/mac/app/re... chrome/installer/mac/app/request.m:38: NSString *appid = @"com.google.Chrome"; //important important how? are the other variables unimportant? https://codereview.chromium.org/2094583004/diff/1/chrome/installer/mac/app/re... chrome/installer/mac/app/request.m:125: sleep(1); This will hang the UI for the entire app (inside the sleep) until the download finishes, which will be a really janky user experience. I think the download needs to be genuinely asynchronous, using a delegate for callbacks when the NSURLSessionDataTask receives data, so that the UI can still react during the download.
These are for the first class, just to get things rolling. https://codereview.chromium.org/2094583004/diff/60001/BUILD.gn File BUILD.gn (right): https://codereview.chromium.org/2094583004/diff/60001/BUILD.gn#newcode1044 BUILD.gn:1044: group("root") { This doesn’t look right. https://codereview.chromium.org/2094583004/diff/60001/chrome/installer/mac/ap... File chrome/installer/mac/app/BUILD.gn (right): https://codereview.chromium.org/2094583004/diff/60001/chrome/installer/mac/ap... chrome/installer/mac/app/BUILD.gn:1: executable("downloader") { Put the copyright/license boilerplate up here. https://codereview.chromium.org/2094583004/diff/60001/chrome/installer/mac/ap... File chrome/installer/mac/app/downloader.h (right): https://codereview.chromium.org/2094583004/diff/60001/chrome/installer/mac/ap... chrome/installer/mac/app/downloader.h:1: // Copyright (c) 2016 The Chromium Authors. All rights reserved. Minor nit: we don’t put the “(c)” in anymore. https://codereview.chromium.org/2094583004/diff/60001/chrome/installer/mac/ap... chrome/installer/mac/app/downloader.h:11: @property(nonatomic, copy) NSMutableArray *chromeDownloadURLs; Nit: Chrome style is to put the * on the type name, not the variable. This is actually what you’ve done elsewhere, like in “(void)parseXML:(NSData*)omahaResponseXML” below. https://codereview.chromium.org/2094583004/diff/60001/chrome/installer/mac/ap... chrome/installer/mac/app/downloader.h:14: // Prepares the array for use in the following methods. A comment isn’t normally needed for -init methods, especially when they don’t take any arguments. It’s an Objective-C convention that an object isn’t usable until an -init method has been called. https://codereview.chromium.org/2094583004/diff/60001/chrome/installer/mac/ap... chrome/installer/mac/app/downloader.h:17: // Will parse the XML passed into it and extract all the url's it finds as well Comments should be written more directly. Instead of “will x, then will y”, you can just say “X and Y.” It’s easier and faster to read if it says “Parses omahaResponseXML and extracts …, then adds …” https://codereview.chromium.org/2094583004/diff/60001/chrome/installer/mac/ap... chrome/installer/mac/app/downloader.h:17: // Will parse the XML passed into it and extract all the url's it finds as well URLs, not url's. Elsewhere, too. https://codereview.chromium.org/2094583004/diff/60001/chrome/installer/mac/ap... chrome/installer/mac/app/downloader.h:18: // as the filename. Will then add each url into the array of url's and report How does it report errors? It has a void return value, so this isn’t obvious. https://codereview.chromium.org/2094583004/diff/60001/chrome/installer/mac/ap... chrome/installer/mac/app/downloader.h:22: // The URL's we got from the previous method are incomplete and need the go/avoidwe https://codereview.chromium.org/2094583004/diff/60001/chrome/installer/mac/ap... chrome/installer/mac/app/downloader.h:25: - (void)appendFilenameToURL; Not sure this makes sense to expose from this class. Usually, when you see that a method is only used in the .mm file but isn’t called by anyone else, it makes sense to take it out of the header. One option is to stick it into a private category in the .mm file to hide it from the rest of the world. https://codereview.chromium.org/2094583004/diff/60001/chrome/installer/mac/ap... chrome/installer/mac/app/downloader.h:27: // Expects the NSData object passed in to be an Omaha XML response file Start with a description of what this method does, and then deal with parameters and expectations. Like: “Calls the XML parser, extracts URLs, and returns the first one. chromeImageAsData is the XML response from Omaha.” You can just call it “XML” or an “XML response.” Saying “file” makes it sound like it’s been written to disk somewhere, but we’ve just got the XML in memory. https://codereview.chromium.org/2094583004/diff/60001/chrome/installer/mac/ap... chrome/installer/mac/app/downloader.h:28: // Will then call the XML parser and extract the four URL's to the current Why four? It might not always be four. https://codereview.chromium.org/2094583004/diff/60001/chrome/installer/mac/ap... chrome/installer/mac/app/downloader.h:32: - (NSURL*)getChromeImageURL: (NSData*)chromeImageAsData; Style nit: no space after the colon. Do it like you did with -parseXML. https://codereview.chromium.org/2094583004/diff/60001/chrome/installer/mac/ap... chrome/installer/mac/app/downloader.h:32: - (NSURL*)getChromeImageURL: (NSData*)chromeImageAsData; but I think that chromeImageAsData is a misleading name here. https://codereview.chromium.org/2094583004/diff/60001/chrome/installer/mac/ap... chrome/installer/mac/app/downloader.h:34: // Takes the URL from getChromeImageURL and packages it into a NSData for easier Not sure what this does. https://codereview.chromium.org/2094583004/diff/60001/chrome/installer/mac/ap... chrome/installer/mac/app/downloader.h:38: // Returns a path to an individuals home download folder. an individuals→the user’s https://codereview.chromium.org/2094583004/diff/60001/chrome/installer/mac/ap... chrome/installer/mac/app/downloader.h:41: // Uses the packaged data from before as well as the filepath from before to “before” = when? https://codereview.chromium.org/2094583004/diff/60001/chrome/installer/mac/ap... chrome/installer/mac/app/downloader.h:45: - (void)downloadChromeImageToDownloadsDirectory: (NSData*)responseXMLData; What’s this do? https://codereview.chromium.org/2094583004/diff/60001/chrome/installer/mac/ap... File chrome/installer/mac/app/downloader.m (right): https://codereview.chromium.org/2094583004/diff/60001/chrome/installer/mac/ap... chrome/installer/mac/app/downloader.m:10: @synthesize chromeDownloadURLs; We synthesize things into instance variables with trailing underscores, so that it’s clear in the code when you’re referring to an instance variable. You’d do: @synthesize chromeDownloadURLs = chromeDownloadURLs_; But actually, now that I see what you’re doing with these, they don’t look very property-like at all. You might be better off with pure instance variables instead of properties. https://codereview.chromium.org/2094583004/diff/60001/chrome/installer/mac/ap... chrome/installer/mac/app/downloader.m:32: int count = [chromeDownloadURLs count]; -[NSArray count] returns type NSUInteger, not int. https://codereview.chromium.org/2094583004/diff/60001/chrome/installer/mac/ap... chrome/installer/mac/app/downloader.m:33: for (int i = 0; i < count; i++) { But to just walk over a NSArray in Objective-C, you can write for (id downloadURL in chromeDownloadURLs) { } https://codereview.chromium.org/2094583004/diff/60001/chrome/installer/mac/ap... chrome/installer/mac/app/downloader.m:41: // Will search the XML data for the tag "url" and the subsequent "codebase" attribute Watch the 80-column limit https://codereview.chromium.org/2094583004/diff/60001/chrome/installer/mac/ap... chrome/installer/mac/app/downloader.m:60: [self appendFilenameToURL]; If the parse failed, chromeImageFilename might not be set at all, and then this will try to use it. That can’t be good! https://codereview.chromium.org/2094583004/diff/60001/chrome/installer/mac/ap... chrome/installer/mac/app/downloader.m:72: //TODO(ivan): make this not suck using [NSURLSession downloadTaskWithURL:] instead 👍. I like the emoji! https://codereview.chromium.org/2094583004/diff/60001/chrome/installer/mac/ap... chrome/installer/mac/app/downloader.m:89: [chromeImageAsData writeToFile:filePath atomically:YES]; What if it already exists, you overwrite it? We’ll need to cope with this. Not necessarily now, but you should call it out in a TODO. https://codereview.chromium.org/2094583004/diff/60001/tools/gn/tutorial/BUILD.gn File tools/gn/tutorial/BUILD.gn (right): https://codereview.chromium.org/2094583004/diff/60001/tools/gn/tutorial/BUILD... tools/gn/tutorial/BUILD.gn:1: executable("hello_world") { You shouldn’t check this file in. The gn tutorial asks its students to create it, so it shouldn’t exist in the tree.
Some of these comments could apply to other occurrences in the same file, or even in other files. I only mentioned the first instance of something the first time I spotted it, but you should consider whether any of these comments might apply elsewhere as you integrate this feedback.
On 2016/06/28 15:44:04, Mark Mentovai wrote: > Some of these comments could apply to other occurrences in the same file, or > even in other files. I only mentioned the first instance of something the first > time I spotted it, but you should consider whether any of these comments might > apply elsewhere as you integrate this feedback. Hi Elly & Mark! We're ready for another look-through on this CL. This is a pretty substantiative review, so we would be happy to talk about it after the fourth of july weekend. All comments welcome! We think we've addressed all of the comments left so far with patches #5-7, so it'd be helpful to look at the delta between patch #4 and patch #7.
This is shaping up pretty well :) some comments below; most of them are small things. https://codereview.chromium.org/2094583004/diff/120001/chrome/installer/mac/a... File chrome/installer/mac/app/NetworkCommunication.h (right): https://codereview.chromium.org/2094583004/diff/120001/chrome/installer/mac/a... chrome/installer/mac/app/NetworkCommunication.h:18: // things that don't really matter, or does it have a positive / I think it's fine to just have one NSURLSession, but you could make one per purpose instead if you want to. https://codereview.chromium.org/2094583004/diff/120001/chrome/installer/mac/a... chrome/installer/mac/app/NetworkCommunication.h:37: - (NSURLSessionDataTask*)sendDataRequest; Hm... sendDataRequestWithCompletionHandler:? Will anyone ever want to do requests with no way to tell if they completed or failed? Also, returning the underlying NSURLSessionDataTasks seems like a bit of an abstraction violation to me - there's no reason the caller here should care that NSURLSession is in use, right? https://codereview.chromium.org/2094583004/diff/120001/chrome/installer/mac/a... File chrome/installer/mac/app/NetworkCommunication.m (right): https://codereview.chromium.org/2094583004/diff/120001/chrome/installer/mac/a... chrome/installer/mac/app/NetworkCommunication.m:34: if(request) { "if (request", i.e., space after if and before ( https://codereview.chromium.org/2094583004/diff/120001/chrome/installer/mac/a... chrome/installer/mac/app/NetworkCommunication.m:68: no blank line needed here https://codereview.chromium.org/2094583004/diff/120001/chrome/installer/mac/a... File chrome/installer/mac/app/OmahaCommunication.h (right): https://codereview.chromium.org/2094583004/diff/120001/chrome/installer/mac/a... chrome/installer/mac/app/OmahaCommunication.h:11: typedef void (^AfterBlock)(NSData*); OmahaRequestCompletionHandler? https://codereview.chromium.org/2094583004/diff/120001/chrome/installer/mac/a... chrome/installer/mac/app/OmahaCommunication.h:22: // and adjusts the URLRequest by changing the HTTP Method to "POST", which is This is an implementation detail. Also, is there a reason to expose the NSURLRequest at all? Does the caller need to modify/etc it? https://codereview.chromium.org/2094583004/diff/120001/chrome/installer/mac/a... chrome/installer/mac/app/OmahaCommunication.h:26: // the next block of code to run after the asynchronous jump. what asynchronous jump? This is setting the completion callback, right? https://codereview.chromium.org/2094583004/diff/120001/chrome/installer/mac/a... chrome/installer/mac/app/OmahaCommunication.h:29: - (void)sendRequest; I would do sendRequestWithCompletionHandler: or something and avoid having setResponseHandlingWithBlock: https://codereview.chromium.org/2094583004/diff/120001/chrome/installer/mac/a... File chrome/installer/mac/app/OmahaCommunication.m (right): https://codereview.chromium.org/2094583004/diff/120001/chrome/installer/mac/a... chrome/installer/mac/app/OmahaCommunication.m:25: NSMutableURLRequest* request = [sessionHelper Hm, is there a reason for this function to be separate from the initializer? ie, will anyone ever want to call the initializer and not call this function once immediately afterward? https://codereview.chromium.org/2094583004/diff/120001/chrome/installer/mac/a... chrome/installer/mac/app/OmahaCommunication.m:26: createRequestWithURLasString:@"https://tools.google.com/service/update2" this should probably be a constant somewhere, and maybe override-able with a commandline argument for testing https://codereview.chromium.org/2094583004/diff/120001/chrome/installer/mac/a... chrome/installer/mac/app/OmahaCommunication.m:36: if(error) { I think we need to notify the user of this class that an error happened somehow other than NSLog. Ideally the user-supplied callback would take an optional NSError* argument so they can get the error and pass it back up. https://codereview.chromium.org/2094583004/diff/120001/chrome/installer/mac/a... chrome/installer/mac/app/OmahaCommunication.m:44: return; same comment - we need a better way to indicate this error. https://codereview.chromium.org/2094583004/diff/120001/chrome/installer/mac/a... chrome/installer/mac/app/OmahaCommunication.m:47: // run block here this comment doesn't seem like it adds much to understanding this function. https://codereview.chromium.org/2094583004/diff/120001/chrome/installer/mac/a... chrome/installer/mac/app/OmahaCommunication.m:56: [sessionHelper sendDataRequest]; I could collapse this and setResponseHandlingWithBlock: into the same function. https://codereview.chromium.org/2094583004/diff/120001/chrome/installer/mac/a... File chrome/installer/mac/app/OmahaXMLRequest.h (right): https://codereview.chromium.org/2094583004/diff/120001/chrome/installer/mac/a... chrome/installer/mac/app/OmahaXMLRequest.h:13: // functions provided by SystemInfo. Maybe this function should call SystemInfo itself and not take these parameters? Would the caller ever want to supply anything other than the values from SystemInfo? https://codereview.chromium.org/2094583004/diff/120001/chrome/installer/mac/a... File chrome/installer/mac/app/OmahaXMLRequest.m (right): https://codereview.chromium.org/2094583004/diff/120001/chrome/installer/mac/a... chrome/installer/mac/app/OmahaXMLRequest.m:10: // Boris (borisv) indicated that the OS version, platform, appid, and version I'd just write "borisv@" here, no need for the first name. https://codereview.chromium.org/2094583004/diff/120001/chrome/installer/mac/a... File chrome/installer/mac/app/delegate.h (right): https://codereview.chromium.org/2094583004/diff/120001/chrome/installer/mac/a... chrome/installer/mac/app/delegate.h:5: #ifndef CHROME_INSTALLER_MAC_APP_DELEGATE_H_ I would use a more detailed filename here (DownloadDelegate.h?) https://codereview.chromium.org/2094583004/diff/120001/chrome/installer/mac/a... chrome/installer/mac/app/delegate.h:12: - (NSString*)getDownloadsFilePath; This seems like a sort of odd function to have on a delegate? https://codereview.chromium.org/2094583004/diff/120001/chrome/installer/mac/a... File chrome/installer/mac/app/delegate.m (right): https://codereview.chromium.org/2094583004/diff/120001/chrome/installer/mac/a... chrome/installer/mac/app/delegate.m:9: - (NSString*)getDownloadsFilePath { This should probably be override-able somehow (command-line argument?) for integration testing https://codereview.chromium.org/2094583004/diff/120001/chrome/installer/mac/a... chrome/installer/mac/app/delegate.m:41: [manager copyItemAtURL:location toURL:downloadsDirectory error:nil]; What do we do if it's not? https://codereview.chromium.org/2094583004/diff/120001/chrome/installer/mac/a... File chrome/installer/mac/app/downloader.h (right): https://codereview.chromium.org/2094583004/diff/120001/chrome/installer/mac/a... chrome/installer/mac/app/downloader.h:12: // Calls parseXML and returns the first URL. Then calls appendFilename to form Comments in headers should be about the interface of the function - ie, what it takes as inputs, what it returns as outputs, any side-effects it has. They probably shouldn't describe the actual helper functions the function uses, since then when the function body changes the comment will get stale. https://codereview.chromium.org/2094583004/diff/120001/chrome/installer/mac/a... chrome/installer/mac/app/downloader.h:14: // TODO(ivan): check the URL's actually work ie. response is a 200/202 and TODOs should have your username in them, so this would be TODO(ivanhernandez) https://codereview.chromium.org/2094583004/diff/120001/chrome/installer/mac/a... chrome/installer/mac/app/downloader.h:23: - (void)downloadChromeImageToDownloadsDirectory:(NSData*)responseXMLData; I think this is the only function that should really be publicly exposed here. The others can be private helper functions that are only visible in the corresponding .m file. also, the argument needs to be documented/named more descriptively; what is it a response from? maybe something like '(NSData*)omahaResponseXml'. https://codereview.chromium.org/2094583004/diff/120001/chrome/installer/mac/a... File chrome/installer/mac/app/downloader.m (right): https://codereview.chromium.org/2094583004/diff/120001/chrome/installer/mac/a... chrome/installer/mac/app/downloader.m:21: [completeURLs addObject: [NSString stringWithFormat:@"%@%@",URL,filename]]; no space after : (in the rest of this file, too) https://codereview.chromium.org/2094583004/diff/120001/chrome/installer/mac/a... chrome/installer/mac/app/downloader.m:26: // Calls parseXML to extract URLs and the filename. Then calls appenFilename to It's better for the comment to describe what the function does, not how it does it. You can tell from looking at the code that it calls parseXML and then appendFilename. https://codereview.chromium.org/2094583004/diff/120001/chrome/installer/mac/a... chrome/installer/mac/app/downloader.m:28: - (NSURL*)parseOhamaResponseForChromeImageURLAndFilename:(NSData*)omahaResponseXML { getChromeImageURLFromOmahaResponse:? https://codereview.chromium.org/2094583004/diff/120001/chrome/installer/mac/a... File chrome/installer/mac/app/main.m (right): https://codereview.chromium.org/2094583004/diff/120001/chrome/installer/mac/a... chrome/installer/mac/app/main.m:23: talkToOmahaThenExecuteBlock(^(NSData* data) { This is taking the kind of shape we were talking about, but I think we'll want separate functions for each of the "steps", since otherwise this will start to look like this: talkToOmahaThenExecuteBlock(..., downloadChromeImageToDownloadsDirectoryThenExecuteBlock(..., unpackChromeImageThenExecuteBlock(..., ...))) and it will probably get quite hard to read. I think we should make a class (just internal to this file) that has, like, onOmahaRequestCompleted:, onImageDownloadCompleted:, and so on, then have each handler function kick off the "next" step (and it can also update the UI), so for example onOmahaRequestCompleted: would be like: [download downloadChromeImageToDownloadsDirectory:data withCallback:@selector(onImageDownloadCompleted:) delegate:self] or something, to arrange for the downloader to run the "next step" when it completes. https://codereview.chromium.org/2094583004/diff/120001/chrome/installer/mac/a... chrome/installer/mac/app/main.m:29: [[NSRunLoop mainRunLoop] runUntilDate:[NSDate dateWithTimeIntervalSinceNow:3]]; why 3? https://codereview.chromium.org/2094583004/diff/120001/chrome/installer/mac/a... File chrome/installer/mac/app/parser.h (right): https://codereview.chromium.org/2094583004/diff/120001/chrome/installer/mac/a... chrome/installer/mac/app/parser.h:24: - (void)parseXML; a) this probably needs a way to signal errors b) maybe parseXML should take an NSData* directly and return the array of incomplete download URLs? If so this could be a static function on this class if you like: - (NSArray*)parseXML:(NSData*)omahaResponseXML error:(NSError**)error; https://codereview.chromium.org/2094583004/diff/120001/chrome/installer/mac/a... File chrome/installer/mac/app/systemInfo.h (right): https://codereview.chromium.org/2094583004/diff/120001/chrome/installer/mac/a... chrome/installer/mac/app/systemInfo.h:21: // Gets the operating system version of the client. After querying NSProcess, It isn't usually useful to document how the function works, but rather what it does from an outside perspective, so you can probably delete the "After querying NSProcess," sentence. https://codereview.chromium.org/2094583004/diff/120001/chrome/installer/mac/a... File chrome/installer/mac/app/systemInfo.m (right): https://codereview.chromium.org/2094583004/diff/120001/chrome/installer/mac/a... chrome/installer/mac/app/systemInfo.m:18: operatingSystemVersionString]; The docs for [NSProcessInfo operatingSystemVersionString] say: "The operating system version string is human readable, localized, and is appropriate for displaying to the user. This string is /not/ appropriate for parsing." It probably isn't appropriate for taking apart like this and sending to Omaha; we should use [NSProcessInfo operatingSystemVersion] instead and build the string ourselves.
Thanks ellyjones for your suggestions! We have submitted another CL for review; Mark, feel free to look at this version of the code from now on. The only comment we have not addressed yet is relevant to creating a class to circumvent having a chain of blocks.
Thanks ellyjones for your suggestions! We have submitted another CL for review; Mark, feel free to look at this version of the code from now on. The only comment we have not addressed yet is relevant to creating a class to circumvent having a chain of blocks.
Thanks ellyjones for your suggestions! We have submitted another CL for review; Mark, feel free to look at this version of the code from now on. The only comment we have not addressed yet is relevant to creating a class to circumvent having a chain of blocks.
Thanks ellyjones for your suggestions! We have submitted another CL for review; Mark, feel free to look at this version of the code from now on. The only comment we have not addressed yet is relevant to creating a class to circumvent having a chain of blocks.
Thanks ellyjones for your suggestions! We have submitted another CL for review; Mark, feel free to look at this version of the code from now on. The only comment we have not addressed yet is relevant to creating a class to circumvent having a chain of blocks.
Thanks ellyjones for your suggestions! We have submitted another CL for review; Mark, feel free to look at this version of the code from now on. The only comment we have not addressed yet is relevant to creating a class to circumvent having a chain of blocks.
Thanks ellyjones for your suggestions! We have submitted another CL for review; Mark, feel free to look at this version of the code from now on. The only comment we have not addressed yet is relevant to creating a class to circumvent having a chain of blocks.
Thanks ellyjones for your suggestions! We have submitted another CL for review; Mark, feel free to look at this version of the code from now on. The only comment we have not addressed yet is relevant to creating a class to circumvent having a chain of blocks.
Thanks ellyjones for your suggestions! We have submitted another CL for review; Mark, feel free to look at this version of the code from now on. The only comment we have not addressed yet is relevant to creating a class to circumvent having a chain of blocks.
Thanks ellyjones for your suggestions! We have submitted another CL for review; Mark, feel free to look at this version of the code from now on. The only comment we have not addressed yet is relevant to creating a class to circumvent having a chain of blocks.
Thanks ellyjones for your suggestions! We have submitted another CL for review; Mark, feel free to look at this version of the code from now on. The only comment we have not addressed yet is relevant to creating a class to circumvent having a chain of blocks.
Thanks ellyjones for your suggestions! We have submitted another CL for review; Mark, feel free to look at this version of the code from now on. The only comment we have not addressed yet is relevant to creating a class to circumvent having a chain of blocks.
Thanks ellyjones for your suggestions! We have submitted another CL for review; Mark, feel free to look at this version of the code from now on. The only comment we have not addressed yet is relevant to creating a class to circumvent having a chain of blocks.
Thanks ellyjones for your suggestions! We have submitted another CL for review; Mark, feel free to look at this version of the code from now on. The only comment we have not addressed yet is relevant to creating a class to circumvent having a chain of blocks.
Thanks ellyjones for your suggestions! We have submitted another CL for review; Mark, feel free to look at this version of the code from now on. The only comment we have not addressed yet is relevant to creating a class to circumvent having a chain of blocks.
Thanks ellyjones for your suggestions! We have submitted another CL for review; Mark, feel free to look at this version of the code from now on. The only comment we have not addressed yet is relevant to creating a class to circumvent having a chain of blocks.
rsesek asked if you guys knew about clang-format yet. I thought “maybe not!” clang-format is an automated formatter that usually gets most mechanical things right. It’s integrated into git cl, so you can just run “git cl format” to get clang-format to make a pass over your change and fix things up. Give it a try and see whether you like it.
Remaining comments. https://codereview.chromium.org/2094583004/diff/120001/chrome/installer/mac/a... File chrome/installer/mac/app/NetworkCommunication.h (right): https://codereview.chromium.org/2094583004/diff/120001/chrome/installer/mac/a... chrome/installer/mac/app/NetworkCommunication.h:37: - (NSURLSessionDataTask*)sendDataRequest; On 2016/07/06 15:22:01, Elly Jones wrote: > Hm... sendDataRequestWithCompletionHandler:? Will anyone ever want to do > requests with no way to tell if they completed or failed? > > Also, returning the underlying NSURLSessionDataTasks seems like a bit of an > abstraction violation to me - there's no reason the caller here should care that > NSURLSession is in use, right? including the completion handler means that delegate functions relevant to that data task would not trigger. https://codereview.chromium.org/2094583004/diff/120001/chrome/installer/mac/a... File chrome/installer/mac/app/OmahaCommunication.h (right): https://codereview.chromium.org/2094583004/diff/120001/chrome/installer/mac/a... chrome/installer/mac/app/OmahaCommunication.h:22: // and adjusts the URLRequest by changing the HTTP Method to "POST", which is On 2016/07/06 15:22:01, Elly Jones wrote: > This is an implementation detail. Also, is there a reason to expose the > NSURLRequest at all? Does the caller need to modify/etc it? no, but it's a good idea to be able to verify if the request is valid... i was thinking this would be useful for testing. https://codereview.chromium.org/2094583004/diff/120001/chrome/installer/mac/a... File chrome/installer/mac/app/OmahaCommunication.m (right): https://codereview.chromium.org/2094583004/diff/120001/chrome/installer/mac/a... chrome/installer/mac/app/OmahaCommunication.m:26: createRequestWithURLasString:@"https://tools.google.com/service/update2" On 2016/07/06 15:22:02, Elly Jones wrote: > this should probably be a constant somewhere, and maybe override-able with a > commandline argument for testing how do we accomplish this? https://codereview.chromium.org/2094583004/diff/120001/chrome/installer/mac/a... File chrome/installer/mac/app/main.m (right): https://codereview.chromium.org/2094583004/diff/120001/chrome/installer/mac/a... chrome/installer/mac/app/main.m:29: [[NSRunLoop mainRunLoop] runUntilDate:[NSDate dateWithTimeIntervalSinceNow:3]]; On 2016/07/06 15:22:02, Elly Jones wrote: > why 3? it's 3 for random testing purposes; we usually use 1 though. later we intend on simply using run, but for now we are saving ourselves from typing control-c too often haha
https://codereview.chromium.org/2094583004/diff/140001/chrome/installer/mac/a... File chrome/installer/mac/app/DownloadDelegate.h (right): https://codereview.chromium.org/2094583004/diff/140001/chrome/installer/mac/a... chrome/installer/mac/app/DownloadDelegate.h:13: Definitely run clang-format on this via git cl format as recommended. This will help with things like the 80-column limit and colon alignment. A couple of other style nits that I see: There are two blank lines in a row here, that’s uncommon. Keep it to one. There also are two blank lines at the end of this file, there shouldn’t be any. You should be consistent with the way that your files are named. I see DownloadDelegate starting with a capital D, but I also see systemInfo with a lowercase s. Starting everything with a capital letter is a more Objective-C way to go, but the Google/Chrome style is to use all lowercase with underscores, like download_delegate and system_info. Since this is an isolated Objective-C project, it’s fine to go with the Objective-C convention, like DownloadDelegate. These are .m files, which is Objective-C with no C++. We may want to use C++ in some or all of them, which would be a change to .mm. We can do this as needed, I’m just mentioning it so that you’re aware. https://codereview.chromium.org/2094583004/diff/140001/chrome/installer/mac/a... File chrome/installer/mac/app/DownloadDelegate.m (right): https://codereview.chromium.org/2094583004/diff/140001/chrome/installer/mac/a... chrome/installer/mac/app/DownloadDelegate.m:25: NSFileManager* manager = [[NSFileManager alloc] init]; Background reading on scoped_nsobject<>, although you guys can use ARC: http://an.enduringcolumn.com/2011/05/bilingual-memory-management.html. https://codereview.chromium.org/2094583004/diff/140001/chrome/installer/mac/a... chrome/installer/mac/app/DownloadDelegate.m:26: NSString* temporaryDownloadDirectory = [NSString stringWithFormat:@"%@", location.path should give you an NSString*, you don’t need to use -stringWithFormat:. Is this really a directory? It’s named as a directory, but that seems weird. Should we make sure that the NSURL is a file URL (isFileURL) before doing this? https://codereview.chromium.org/2094583004/diff/140001/chrome/installer/mac/a... chrome/installer/mac/app/DownloadDelegate.m:31: [manager copyItemAtURL:location toURL:downloadsDirectory error:nil]; What happens to the original temporary copy? https://codereview.chromium.org/2094583004/diff/140001/chrome/installer/mac/a... chrome/installer/mac/app/DownloadDelegate.m:32: NSLog(@"File was downloaded to Downloads."); We shouldn’t check in logging like this. https://codereview.chromium.org/2094583004/diff/140001/chrome/installer/mac/a... File chrome/installer/mac/app/NetworkCommunication.h (right): https://codereview.chromium.org/2094583004/diff/140001/chrome/installer/mac/a... chrome/installer/mac/app/NetworkCommunication.h:28: - (NSMutableURLRequest*)createRequestWithURLasString:(NSString*) urlString URLasString looks weird, like “UR Las String”. Consider changing the capitalization to UrlAsString to make this easier to read. https://codereview.chromium.org/2094583004/diff/140001/chrome/installer/mac/a... File chrome/installer/mac/app/NetworkCommunication.m (right): https://codereview.chromium.org/2094583004/diff/140001/chrome/installer/mac/a... chrome/installer/mac/app/NetworkCommunication.m:12: @synthesize session; Please name the variables that back your properties with trailing underscores, like @synthesize session = session_; That helps us see elsewhere in the code when we’re working with an instance variable. https://codereview.chromium.org/2094583004/diff/140001/chrome/installer/mac/a... chrome/installer/mac/app/NetworkCommunication.m:23: NSURLSessionConfiguration* sessionConfig = [NSURLSessionConfiguration Both sessionConfig and session are autoreleased, so you can’t really stash them in instance variables. https://codereview.chromium.org/2094583004/diff/140001/chrome/installer/mac/a... chrome/installer/mac/app/NetworkCommunication.m:34: if (request) { When would this ever happen?
Looking for more feedback! We have covered all comments except fixing main to be based on a class as opposed to blocks.
Nice work! This is just about ready; all my remaining comments are little cleanups. LGTM, but let mark@ sign off on it too. https://codereview.chromium.org/2094583004/diff/160001/chrome/installer/mac/a... File chrome/installer/mac/app/NetworkCommunication.h (right): https://codereview.chromium.org/2094583004/diff/160001/chrome/installer/mac/a... chrome/installer/mac/app/NetworkCommunication.h:26: // Contrary to the assumption in Objective-C that all return values are returned That's because we're returning a pointer to it, not the value itself (so conceptually we are returning by reference here). This comment is probably superfluous since that's what the star indicates. https://codereview.chromium.org/2094583004/diff/160001/chrome/installer/mac/a... File chrome/installer/mac/app/NetworkCommunication.m (right): https://codereview.chromium.org/2094583004/diff/160001/chrome/installer/mac/a... chrome/installer/mac/app/NetworkCommunication.m:51: return; nit: superfluous return https://codereview.chromium.org/2094583004/diff/160001/chrome/installer/mac/a... chrome/installer/mac/app/NetworkCommunication.m:64: return; nit: this is superfluous https://codereview.chromium.org/2094583004/diff/160001/chrome/installer/mac/a... File chrome/installer/mac/app/OmahaCommunication.m (right): https://codereview.chromium.org/2094583004/diff/160001/chrome/installer/mac/a... chrome/installer/mac/app/OmahaCommunication.m:28: createRequestWithUrlAsString:@"https://tools.google.com/service/update2" We will want this to be a command-line flag later, but let's leave it for just now. https://codereview.chromium.org/2094583004/diff/160001/chrome/installer/mac/a... File chrome/installer/mac/app/downloader.m (right): https://codereview.chromium.org/2094583004/diff/160001/chrome/installer/mac/a... chrome/installer/mac/app/downloader.m:18: NSArray* filenameComposition = @[ filePathToDownloads, @"GoogleChrome.dmg" ]; nit: no spaces around [] https://codereview.chromium.org/2094583004/diff/160001/chrome/installer/mac/a... chrome/installer/mac/app/downloader.m:32: [completeURLs addObject:[NSString stringWithFormat:@"%@%@", URL, filename]]; [NSURL URLwithString:relativeToURL:] I think https://codereview.chromium.org/2094583004/diff/160001/chrome/installer/mac/a... chrome/installer/mac/app/downloader.m:84: if (!writeWasSuccessful) { just return writeWasSuccessful :) https://codereview.chromium.org/2094583004/diff/160001/chrome/installer/mac/a... File chrome/installer/mac/app/parser.h (right): https://codereview.chromium.org/2094583004/diff/160001/chrome/installer/mac/a... chrome/installer/mac/app/parser.h:19: // Parses an XML and extracts all the URL's it finds as well as the filename. an XML? https://codereview.chromium.org/2094583004/diff/160001/chrome/installer/mac/a... File chrome/installer/mac/app/parser.m (right): https://codereview.chromium.org/2094583004/diff/160001/chrome/installer/mac/a... chrome/installer/mac/app/parser.m:9: //- (id)initWithXML:(NSData*)omahaResponseXML{ you can remove the dead code :) https://codereview.chromium.org/2094583004/diff/160001/chrome/installer/mac/a... File chrome/installer/mac/app/systemInfo.m (right): https://codereview.chromium.org/2094583004/diff/160001/chrome/installer/mac/a... chrome/installer/mac/app/systemInfo.m:17: // NSOperatingSystemVersion osVersion = [[NSProcessInfo processInfo] This commented-out code doesn't need to be here.
LGTM! OK to check in after these comments and Elly’s are addressed. Then we’ll really get moving quickly! https://codereview.chromium.org/2094583004/diff/160001/chrome/installer/mac/a... File chrome/installer/mac/app/BUILD.gn (right): https://codereview.chromium.org/2094583004/diff/160001/chrome/installer/mac/a... chrome/installer/mac/app/BUILD.gn:1: # Copyright 2014 The Chromium Authors. All rights reserved. 2016. Make sure all of your new files say 2016. https://codereview.chromium.org/2094583004/diff/160001/chrome/installer/mac/a... chrome/installer/mac/app/BUILD.gn:12: "downloader.m", Downloader.m and Parser.m, capital letters, and rename the files and their headers to match. Also, rename systemInfo.m to SystemInfo.m (it’s listed with a capital here but the file’s name has it lowercased.) https://codereview.chromium.org/2094583004/diff/160001/chrome/installer/mac/a... File chrome/installer/mac/app/DownloadDelegate.m (right): https://codereview.chromium.org/2094583004/diff/160001/chrome/installer/mac/a... chrome/installer/mac/app/DownloadDelegate.m:5: #import "DownloadDelegate.h" DownloadDelegate.m should #import/#include like this: #import "DownloadDelegate.h" (the header that corresponds to this file) (blank line) #include <assert.h> (#include, don’t #import, non-ObjC headers) (blank line) #include "downloader.h" (other #imports/#includes go here) See https://google.github.io/styleguide/cppguide.html#Names_and_Order_of_Includes https://codereview.chromium.org/2094583004/diff/160001/chrome/installer/mac/a... File chrome/installer/mac/app/NetworkCommunication.h (right): https://codereview.chromium.org/2094583004/diff/160001/chrome/installer/mac/a... chrome/installer/mac/app/NetworkCommunication.h:26: // Contrary to the assumption in Objective-C that all return values are returned Elly Jones wrote: > That's because we're returning a pointer to it, not the value itself (so > conceptually we are returning by reference here). This comment is probably > superfluous since that's what the star indicates. I’m coming over to talk about this. We may want to draw something. https://codereview.chromium.org/2094583004/diff/160001/chrome/installer/mac/a... File chrome/installer/mac/app/NetworkCommunication.m (right): https://codereview.chromium.org/2094583004/diff/160001/chrome/installer/mac/a... chrome/installer/mac/app/NetworkCommunication.m:6: #include "NetworkCommunication.h" #import "NetworkCommunication.h" (blank line) #import <Foundation/Foundation.h> https://codereview.chromium.org/2094583004/diff/160001/chrome/installer/mac/a... File chrome/installer/mac/app/OmahaCommunication.h (right): https://codereview.chromium.org/2094583004/diff/160001/chrome/installer/mac/a... chrome/installer/mac/app/OmahaCommunication.h:8: #import <Foundation/Foundation.h> Blank line between #includes of <system headers> and "our headers" https://codereview.chromium.org/2094583004/diff/160001/chrome/installer/mac/a... File chrome/installer/mac/app/OmahaCommunication.m (right): https://codereview.chromium.org/2094583004/diff/160001/chrome/installer/mac/a... chrome/installer/mac/app/OmahaCommunication.m:5: #import <Foundation/Foundation.h> #import "OmahaCommunication.h" (blank line) #import <Foundation/Foundation.h> https://codereview.chromium.org/2094583004/diff/160001/chrome/installer/mac/a... File chrome/installer/mac/app/OmahaXMLRequest.m (right): https://codereview.chromium.org/2094583004/diff/160001/chrome/installer/mac/a... chrome/installer/mac/app/OmahaXMLRequest.m:5: #import <Foundation/Foundation.h> #import "OmahaXMLRequest.h> #import <Foundation/Foundation.h> #import "SystemInfo.h" https://codereview.chromium.org/2094583004/diff/160001/chrome/installer/mac/a... chrome/installer/mac/app/OmahaXMLRequest.m:16: NSString* keystoneVersion = @"KeystoneAgent-1.99.4.0"; I don’t think we should masquerade as Keystone here, it’ll make it harder to distinguish on the server. https://codereview.chromium.org/2094583004/diff/160001/chrome/installer/mac/a... File chrome/installer/mac/app/downloader.m (right): https://codereview.chromium.org/2094583004/diff/160001/chrome/installer/mac/a... chrome/installer/mac/app/downloader.m:5: #import <Foundation/Foundation.h> #import "downloader.h" (blank line) #import <Foundation/Foundation.h> (blank line) #import "DownloadDelegate.h" (keep things sorted within each section) #import "downloader.h" #import "NetworkCommunication.h" #import "parser.h" https://codereview.chromium.org/2094583004/diff/160001/chrome/installer/mac/a... File chrome/installer/mac/app/main.m (right): https://codereview.chromium.org/2094583004/diff/160001/chrome/installer/mac/a... chrome/installer/mac/app/main.m:5: #import <Foundation/Foundation.h> Blank line after this one. https://codereview.chromium.org/2094583004/diff/160001/chrome/installer/mac/a... File chrome/installer/mac/app/parser.h (right): https://codereview.chromium.org/2094583004/diff/160001/chrome/installer/mac/a... chrome/installer/mac/app/parser.h:10: @interface Parser : NSObject<NSXMLParserDelegate> { “Parser” is too generic. How about OmahaXMLParser, which matches OmahaXMLRequest nicely? Rename the files accordingly (and update the #if guards above). https://codereview.chromium.org/2094583004/diff/160001/chrome/installer/mac/a... File chrome/installer/mac/app/systemInfo.m (right): https://codereview.chromium.org/2094583004/diff/160001/chrome/installer/mac/a... chrome/installer/mac/app/systemInfo.m:5: #import "SystemInfo.h" Blank after me too.
The CQ bit was checked by zengster@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from mark@chromium.org, ellyjones@chromium.org Link to the patchset: https://codereview.chromium.org/2094583004/#ps180001 (title: "Made small changes according to LGTM comments")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by zengster@google.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
Really sorry about that, guys. There’s nothing wrong with your patch, it’s just a sick bot. Try ticking the commit box one more time to give it another chance. If that fails, we can intervene manually.
On 2016/07/12 20:00:13, Mark Mentovai wrote: > Really sorry about that, guys. There’s nothing wrong with your patch, it’s just > a sick bot. Try ticking the commit box one more time to give it another chance. > If that fails, we can intervene manually. Thanks Mark. Will try a third time -- it seems that this linux_chromium_asan_rel_ng builder is the only one failing. Should I submit a CQ issue?
The CQ bit was checked by zengster@google.com
That bot’s been a problem for a few weeks. I’d be surprised if they didn’t already know about it, but sure, you can file an infra ticket about it.
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The next step is to edit the CL description to have this tag at the bottom: NOTRY=true (you can replace BUG= since an empty BUG line serves no useful purpose.) You can edit this message with “git cl description” or just use the “Edit Issue” link on the codereview site. This tag tells the commit-queue to skip trybot validation. It’s a coarse hammer, but since we have one problem child bot and we know that we’re not causing its problems, I think it’s all right to use here. With that tag set, try the commit checkbox (hopefully) one last time.
The CQ bit was checked by ellyjones@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
On 2016/07/13 14:06:12, commit-bot: I haz the power wrote: > CQ is trying da patch. Follow status at > > https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or... Huh. Wonder who just clicked the CQ box. I'll try the NOTRY thing and attempt this again.
Description was changed from ========== Initial commit for Chrome metainstaller on Mac. BUG= ========== to ========== Initial commit for Chrome metainstaller on Mac. NOTRY=true ==========
The CQ bit was unchecked by zengster@google.com
The CQ bit was checked by zengster@google.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
On 2016/07/13 14:10:54, Anna Zeng wrote: > On 2016/07/13 14:06:12, commit-bot: I haz the power wrote: > > CQ is trying da patch. Follow status at > > > > > https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or... > > Huh. Wonder who just clicked the CQ box. I'll try the NOTRY thing and attempt > this again. It was me :)
Message was sent while issue was closed.
Description was changed from ========== Initial commit for Chrome metainstaller on Mac. NOTRY=true ========== to ========== Initial commit for Chrome metainstaller on Mac. NOTRY=true ==========
Message was sent while issue was closed.
Committed patchset #10 (id:180001)
Message was sent while issue was closed.
CQ bit was unchecked.
Message was sent while issue was closed.
Description was changed from ========== Initial commit for Chrome metainstaller on Mac. NOTRY=true ========== to ========== Initial commit for Chrome metainstaller on Mac. NOTRY=true Committed: https://crrev.com/73447cf7a9a9c28d59013993f14eff3c84840013 Cr-Commit-Position: refs/heads/master@{#405140} ==========
Message was sent while issue was closed.
Patchset 10 (id:??) landed as https://crrev.com/73447cf7a9a9c28d59013993f14eff3c84840013 Cr-Commit-Position: refs/heads/master@{#405140}
Message was sent while issue was closed.
There you go! |
