Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1375023002:
Adds support for POST request with bodies on WKWebView. (Closed)
|
Created:
5 years, 2 months ago by stkhapugin Modified:
5 years ago CC:
chromium-reviews, bzanotti Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdds support for POST request with bodies on WKWebView.
These requests are now handled through a javascript hack to work around
this webkit bug: https://bugs.webkit.org/show_bug.cgi?id=145410.
BUG=489692
Committed: https://crrev.com/8d13d52f53c03f6b78ab8fbf7148155780063244
Cr-Commit-Position: refs/heads/master@{#363256}
Patch Set 1 #Patch Set 2 : Missing javascript #
Total comments: 58
Patch Set 3 : comments-1 #Patch Set 4 : formatting #Patch Set 5 : formatting #Patch Set 6 : other comments addressed #Patch Set 7 : Escaping characters for JavaScript #Patch Set 8 : Formatting #
Total comments: 8
Patch Set 9 : Some comments addressed #Patch Set 10 : fix + formatting #Patch Set 11 : styling #Patch Set 12 : formatting and renaming #Patch Set 13 : missing files #Patch Set 14 : rest of the commetns #
Total comments: 38
Patch Set 15 : sync commit #Patch Set 16 : Added unit test and error handling; Synced. #Patch Set 17 : missing file #
Total comments: 62
Patch Set 18 : All comments addressed #
Total comments: 40
Patch Set 19 : Another round of comments #
Total comments: 6
Patch Set 20 : Another round of comments #Patch Set 21 : more comments #Patch Set 22 : Xcode has reverted my changes and I'm reapplying #
Total comments: 17
Patch Set 23 : s/BOOL/bool #Patch Set 24 : Fixed leaking WKWV #Patch Set 25 : Styling #Patch Set 26 : Addressed Noyau's comments #Messages
Total messages: 47 (15 generated)
stkhapugin@chromium.org changed reviewers: + eugenebut@chromium.org, stuartmorgan@chromium.org
This is not perfect, but it works. The fix is inspired by https://code.google.com/p/chromium/codesearch#chromium/src/ios/chrome/browser... and the js code is somewhat similar. I think the similarity is small enough and there's no need in factoring this out to a separate class, but I'll be happy to hear your input. +eugenebut +stuartmorgan as owners +bzanotti as author of gaia fix. PTAL.
Nice :) https://codereview.chromium.org/1375023002/diff/20001/ios/web/web_state/ui/cr... File ios/web/web_state/ui/crw_wk_web_view_web_controller.mm (right): https://codereview.chromium.org/1375023002/diff/20001/ios/web/web_state/ui/cr... ios/web/web_state/ui/crw_wk_web_view_web_controller.mm:61: // load the resource because of 302s on POST request (the cookies of the first "the cookies of the first response are correctly set" seems to be a comment from Mirror code. I don't see any retrying here. https://codereview.chromium.org/1375023002/diff/20001/ios/web/web_state/ui/cr... ios/web/web_state/ui/crw_wk_web_view_web_controller.mm:71: NSString* const kPostRequestTemplate = This is a large piece of JS code. Maybe it will be better to store in in a separate .js file as a function, compile with closure and load using web::GetPageScript function? https://codereview.chromium.org/1375023002/diff/20001/ios/web/web_state/ui/cr... ios/web/web_state/ui/crw_wk_web_view_web_controller.mm:73: "function b64toBlob(b64Data, contentType) {" NIT: s/b64toBlob/b64ToBlob (camelCase according to JS Style Guide) https://codereview.chromium.org/1375023002/diff/20001/ios/web/web_state/ui/cr... ios/web/web_state/ui/crw_wk_web_view_web_controller.mm:75: " sliceSize = 512;" var sliceSize; Otherwise this code creates a global (window.sliceSize) variable. https://codereview.chromium.org/1375023002/diff/20001/ios/web/web_state/ui/cr... ios/web/web_state/ui/crw_wk_web_view_web_controller.mm:125: [NSError errorWithDomain:error.domain code:error.code userInfo:userInfo]; NIT: I understand that this was done by git cl format, but please revert unrelated formatting changes, since the pollute git blame. https://codereview.chromium.org/1375023002/diff/20001/ios/web/web_state/ui/cr... ios/web/web_state/ui/crw_wk_web_view_web_controller.mm:501: // Constructs an HTML page that executes the request through javascript and NIT: s/javascript/JavaScript https://codereview.chromium.org/1375023002/diff/20001/ios/web/web_state/ui/cr... ios/web/web_state/ui/crw_wk_web_view_web_controller.mm:503: - (NSString*)htmlForPOSTRequest:(NSURLRequest*)request { s/htmlForPOSTRequest:/HTMLForPOSTRequest: https://codereview.chromium.org/1375023002/diff/20001/ios/web/web_state/ui/cr... ios/web/web_state/ui/crw_wk_web_view_web_controller.mm:503: - (NSString*)htmlForPOSTRequest:(NSURLRequest*)request { Every method needs a predeclaration, and method comments should go before that predeclaration. https://codereview.chromium.org/1375023002/diff/20001/ios/web/web_state/ui/cr... ios/web/web_state/ui/crw_wk_web_view_web_controller.mm:507: NSMutableString* headerString = [[[NSMutableString alloc] init] autorelease]; [NSMutableString string] https://codereview.chromium.org/1375023002/diff/20001/ios/web/web_state/ui/cr... ios/web/web_state/ui/crw_wk_web_view_web_controller.mm:511: [[[request allHTTPHeaderFields][headerField] copy] autorelease]; Is it really necessary to copy this string? https://codereview.chromium.org/1375023002/diff/20001/ios/web/web_state/ui/cr... ios/web/web_state/ui/crw_wk_web_view_web_controller.mm:527: // expected to be low. How does it perform with huge images? :) https://codereview.chromium.org/1375023002/diff/20001/ios/web/web_state/ui/cr... ios/web/web_state/ui/crw_wk_web_view_web_controller.mm:531: NSString* htmlString = [self htmlForPOSTRequest:request]; NIT: s/htmlString/HTML acronyms should be all caps, and String suffix is really superfluous here. https://codereview.chromium.org/1375023002/diff/20001/ios/web/web_state/ui/cr... ios/web/web_state/ui/crw_wk_web_view_web_controller.mm:585: ProceduralBlock POSTBlock = ^{ Is there a specific reason why you want to hold this code in the block? Why not inline it instead? https://codereview.chromium.org/1375023002/diff/20001/ios/web/web_state/ui/cr... ios/web/web_state/ui/crw_wk_web_view_web_controller.mm:595: ProceduralBlock webViewNavigationBlock = ^{ Please move this block closer to the place where it's actually used. https://codereview.chromium.org/1375023002/diff/20001/ios/web/web_state/ui/cr... ios/web/web_state/ui/crw_wk_web_view_web_controller.mm:616: if (!POSTData.length) { If POSTData exist, navigation is postResubmission but WWBackForwardNavigationItem does not exist then Form Resubmission dialog should not be shown (because there is no way to actually resubmit the form). Because of that "(!POSTData.length)" check does not look correct. https://codereview.chromium.org/1375023002/diff/20001/ios/web/web_state/ui/cr... ios/web/web_state/ui/crw_wk_web_view_web_controller.mm:622: if (POSTData.length && !isFormResubmission) { Should this "if" go before previous top level if (before line 610)? https://codereview.chromium.org/1375023002/diff/20001/ios/web/web_state/ui/cr... ios/web/web_state/ui/crw_wk_web_view_web_controller.mm:736: stringWithFormat:@"WKWebViewWebController.NetworkActivityIndicatorKey.%@", Please revert this (and subsequent) changes to avoid git blame pollution.
https://codereview.chromium.org/1375023002/diff/20001/ios/web/web_state/ui/cr... File ios/web/web_state/ui/crw_wk_web_view_web_controller.mm (right): https://codereview.chromium.org/1375023002/diff/20001/ios/web/web_state/ui/cr... ios/web/web_state/ui/crw_wk_web_view_web_controller.mm:125: [NSError errorWithDomain:error.domain code:error.code userInfo:userInfo]; On 2015/09/29 17:46:46, eugenebut wrote: > NIT: I understand that this was done by git cl format, but please revert > unrelated formatting changes, since the pollute git blame. git cl format shouldn't have touched all these otherwise untouched files; this looks like a file-level format.
bzanotti@chromium.org changed reviewers: + bzanotti@chromium.org
https://codereview.chromium.org/1375023002/diff/20001/ios/web/web_state/ui/cr... File ios/web/web_state/ui/crw_wk_web_view_web_controller.mm (right): https://codereview.chromium.org/1375023002/diff/20001/ios/web/web_state/ui/cr... ios/web/web_state/ui/crw_wk_web_view_web_controller.mm:513: [headerString appendFormat:@"req.setRequestHeader(\"%@\", \"%@\");", There might be some escaping required here. https://codereview.chromium.org/1375023002/diff/20001/ios/web/web_state/ui/cr... ios/web/web_state/ui/crw_wk_web_view_web_controller.mm:518: return [NSString stringWithFormat:kPostRequestTemplate, originURL, Same here for |originURL|, it might require some escaping to be handled correctly in JavaScript.
Thanks for comments, I addressed them. PTAL https://codereview.chromium.org/1375023002/diff/20001/ios/web/web_state/ui/cr... File ios/web/web_state/ui/crw_wk_web_view_web_controller.mm (right): https://codereview.chromium.org/1375023002/diff/20001/ios/web/web_state/ui/cr... ios/web/web_state/ui/crw_wk_web_view_web_controller.mm:61: // load the resource because of 302s on POST request (the cookies of the first On 2015/09/29 17:46:45, eugenebut wrote: > "the cookies of the first response are correctly set" seems to be a comment from > Mirror code. I don't see any retrying here. Done. https://codereview.chromium.org/1375023002/diff/20001/ios/web/web_state/ui/cr... ios/web/web_state/ui/crw_wk_web_view_web_controller.mm:71: NSString* const kPostRequestTemplate = On 2015/09/29 17:46:46, eugenebut wrote: > This is a large piece of JS code. Maybe it will be better to store in in a > separate .js file as a function, compile with closure and load using > web::GetPageScript function? This is actually a piece of html. How do you think, will it work if I load a blank page and load&execute the script in it? Also note that I'm loading the page with a baseURL, so I still need to load html string to specify it. https://codereview.chromium.org/1375023002/diff/20001/ios/web/web_state/ui/cr... ios/web/web_state/ui/crw_wk_web_view_web_controller.mm:73: "function b64toBlob(b64Data, contentType) {" On 2015/09/29 17:46:46, eugenebut wrote: > NIT: s/b64toBlob/b64ToBlob (camelCase according to JS Style Guide) Done. https://codereview.chromium.org/1375023002/diff/20001/ios/web/web_state/ui/cr... ios/web/web_state/ui/crw_wk_web_view_web_controller.mm:75: " sliceSize = 512;" On 2015/09/29 17:46:45, eugenebut wrote: > var sliceSize; Otherwise this code creates a global (window.sliceSize) variable. Done. https://codereview.chromium.org/1375023002/diff/20001/ios/web/web_state/ui/cr... ios/web/web_state/ui/crw_wk_web_view_web_controller.mm:125: [NSError errorWithDomain:error.domain code:error.code userInfo:userInfo]; On 2015/09/29 21:24:44, stuartmorgan wrote: > On 2015/09/29 17:46:46, eugenebut wrote: > > NIT: I understand that this was done by git cl format, but please revert > > unrelated formatting changes, since the pollute git blame. > > git cl format shouldn't have touched all these otherwise untouched files; this > looks like a file-level format. Done. Thanks for explaining the reason, because I didn't know what am I supposed to do here. It was caused by the fact that I was debugging this in downstream checkout and copy-pasted the entire file once it was done, hence git cl format checked the entire file. https://codereview.chromium.org/1375023002/diff/20001/ios/web/web_state/ui/cr... ios/web/web_state/ui/crw_wk_web_view_web_controller.mm:125: [NSError errorWithDomain:error.domain code:error.code userInfo:userInfo]; On 2015/09/29 17:46:46, eugenebut wrote: > NIT: I understand that this was done by git cl format, but please revert > unrelated formatting changes, since the pollute git blame. Done. https://codereview.chromium.org/1375023002/diff/20001/ios/web/web_state/ui/cr... ios/web/web_state/ui/crw_wk_web_view_web_controller.mm:501: // Constructs an HTML page that executes the request through javascript and On 2015/09/29 17:46:46, eugenebut wrote: > NIT: s/javascript/JavaScript Done. https://codereview.chromium.org/1375023002/diff/20001/ios/web/web_state/ui/cr... ios/web/web_state/ui/crw_wk_web_view_web_controller.mm:503: - (NSString*)htmlForPOSTRequest:(NSURLRequest*)request { On 2015/09/29 17:46:46, eugenebut wrote: > s/htmlForPOSTRequest:/HTMLForPOSTRequest: Done. https://codereview.chromium.org/1375023002/diff/20001/ios/web/web_state/ui/cr... ios/web/web_state/ui/crw_wk_web_view_web_controller.mm:503: - (NSString*)htmlForPOSTRequest:(NSURLRequest*)request { On 2015/09/29 17:46:46, eugenebut wrote: > Every method needs a predeclaration, and method comments should go before that > predeclaration. Done. https://codereview.chromium.org/1375023002/diff/20001/ios/web/web_state/ui/cr... ios/web/web_state/ui/crw_wk_web_view_web_controller.mm:507: NSMutableString* headerString = [[[NSMutableString alloc] init] autorelease]; On 2015/09/29 17:46:45, eugenebut wrote: > [NSMutableString string] Done. https://codereview.chromium.org/1375023002/diff/20001/ios/web/web_state/ui/cr... ios/web/web_state/ui/crw_wk_web_view_web_controller.mm:511: [[[request allHTTPHeaderFields][headerField] copy] autorelease]; On 2015/09/29 17:46:46, eugenebut wrote: > Is it really necessary to copy this string? Done. https://codereview.chromium.org/1375023002/diff/20001/ios/web/web_state/ui/cr... ios/web/web_state/ui/crw_wk_web_view_web_controller.mm:513: [headerString appendFormat:@"req.setRequestHeader(\"%@\", \"%@\");", On 2015/09/30 09:06:54, bzanotti wrote: > There might be some escaping required here. Done. https://codereview.chromium.org/1375023002/diff/20001/ios/web/web_state/ui/cr... ios/web/web_state/ui/crw_wk_web_view_web_controller.mm:518: return [NSString stringWithFormat:kPostRequestTemplate, originURL, On 2015/09/30 09:06:54, bzanotti wrote: > Same here for |originURL|, it might require some escaping to be handled > correctly in JavaScript. Done. https://codereview.chromium.org/1375023002/diff/20001/ios/web/web_state/ui/cr... ios/web/web_state/ui/crw_wk_web_view_web_controller.mm:527: // expected to be low. On 2015/09/29 17:46:46, eugenebut wrote: > How does it perform with huge images? :) I tested with iPad Air 1 and a 3k*2k image, and it was surprisingly good. This is mainly because we already have the logic to downscale large images. The bottleneck is still the load of the image (we need to load it for the second time). This can be improved by doing all work in JavaScript, including image resizing and opening a new tab. However, I decided to go with this solution, because POST may be useful for other things. :) https://codereview.chromium.org/1375023002/diff/20001/ios/web/web_state/ui/cr... ios/web/web_state/ui/crw_wk_web_view_web_controller.mm:531: NSString* htmlString = [self htmlForPOSTRequest:request]; On 2015/09/29 17:46:45, eugenebut wrote: > NIT: s/htmlString/HTML > > acronyms should be all caps, and String suffix is really superfluous here. Done. https://codereview.chromium.org/1375023002/diff/20001/ios/web/web_state/ui/cr... ios/web/web_state/ui/crw_wk_web_view_web_controller.mm:585: ProceduralBlock POSTBlock = ^{ On 2015/09/29 17:46:46, eugenebut wrote: > Is there a specific reason why you want to hold this code in the block? Why not > inline it instead? It's a question of style, really. Two blocks were already here, so when I added a 3rd way to navigate to URL, I decided to keep the style. I moved the blocks together, so that all options are explained in one place and then all the option selection is nice and straightforward. I understand that the third block is not actually passed as a block, so if you insist, I will restructure this. But I want to have some logic behind the way things are structured here. https://codereview.chromium.org/1375023002/diff/20001/ios/web/web_state/ui/cr... ios/web/web_state/ui/crw_wk_web_view_web_controller.mm:595: ProceduralBlock webViewNavigationBlock = ^{ On 2015/09/29 17:46:45, eugenebut wrote: > Please move this block closer to the place where it's actually used. Please see above. https://codereview.chromium.org/1375023002/diff/20001/ios/web/web_state/ui/cr... ios/web/web_state/ui/crw_wk_web_view_web_controller.mm:616: if (!POSTData.length) { On 2015/09/29 17:46:46, eugenebut wrote: > If POSTData exist, navigation is postResubmission but > WWBackForwardNavigationItem does not exist then Form Resubmission dialog should > not be shown (because there is no way to actually resubmit the form). > > Because of that "(!POSTData.length)" check does not look correct. Done. https://codereview.chromium.org/1375023002/diff/20001/ios/web/web_state/ui/cr... ios/web/web_state/ui/crw_wk_web_view_web_controller.mm:622: if (POSTData.length && !isFormResubmission) { On 2015/09/29 17:46:45, eugenebut wrote: > Should this "if" go before previous top level if (before line 610)? Done. https://codereview.chromium.org/1375023002/diff/20001/ios/web/web_state/ui/cr... ios/web/web_state/ui/crw_wk_web_view_web_controller.mm:736: stringWithFormat:@"WKWebViewWebController.NetworkActivityIndicatorKey.%@", On 2015/09/29 17:46:46, eugenebut wrote: > Please revert this (and subsequent) changes to avoid git blame pollution. Done.
lgtm with changes below. Stuart will finish the review. https://codereview.chromium.org/1375023002/diff/20001/ios/web/web_state/ui/cr... File ios/web/web_state/ui/crw_wk_web_view_web_controller.mm (right): https://codereview.chromium.org/1375023002/diff/20001/ios/web/web_state/ui/cr... ios/web/web_state/ui/crw_wk_web_view_web_controller.mm:71: NSString* const kPostRequestTemplate = On 2015/09/30 15:05:14, stkhapugin wrote: > On 2015/09/29 17:46:46, eugenebut wrote: > > This is a large piece of JS code. Maybe it will be better to store in in a > > separate .js file as a function, compile with closure and load using > > web::GetPageScript function? > > This is actually a piece of html. You can load your script from file and then wrap inside "<html><script>" tags. In general I don't have strong opinion on this. You can leave the code as it is as long as other reviewers are fine with having long js function here. > How do you think, will it work if I load a > blank page and load&execute the script in it? Also note that I'm loading the > page with a baseURL, so I still need to load html string to specify it. Loading the page and executing js inside will be tricky, because load is asynchronous operation. https://codereview.chromium.org/1375023002/diff/20001/ios/web/web_state/ui/cr... ios/web/web_state/ui/crw_wk_web_view_web_controller.mm:527: // expected to be low. On 2015/09/30 15:05:14, stkhapugin wrote: > On 2015/09/29 17:46:46, eugenebut wrote: > > How does it perform with huge images? :) > > I tested with iPad Air 1 and a 3k*2k image, and it was surprisingly good. This > is mainly because we already have the logic to downscale large images. The > bottleneck is still the load of the image (we need to load it for the second > time). This can be improved by doing all work in JavaScript, including image > resizing and opening a new tab. > > However, I decided to go with this solution, because POST may be useful for > other things. :) Thank you. Then I guess you can remove "hence the performance is expected to be low" comment. https://codereview.chromium.org/1375023002/diff/20001/ios/web/web_state/ui/cr... ios/web/web_state/ui/crw_wk_web_view_web_controller.mm:585: ProceduralBlock POSTBlock = ^{ On 2015/09/30 15:05:14, stkhapugin wrote: > On 2015/09/29 17:46:46, eugenebut wrote: > > Is there a specific reason why you want to hold this code in the block? Why > not > > inline it instead? > > It's a question of style, really. Two blocks were already here, so when I added > a 3rd way to navigate to URL, I decided to keep the style. I moved the blocks > together, so that all options are explained in one place and then all the option > selection is nice and straightforward. > > I understand that the third block is not actually passed as a block, so if you > insist, I will restructure this. But I want to have some logic behind the way > things are structured here. Block creation has some overhead, so it's not only about style. I would encourage you to flatten the code and get rid of this block. https://codereview.chromium.org/1375023002/diff/20001/ios/web/web_state/ui/cr... ios/web/web_state/ui/crw_wk_web_view_web_controller.mm:595: ProceduralBlock webViewNavigationBlock = ^{ On 2015/09/30 15:05:13, stkhapugin wrote: > On 2015/09/29 17:46:45, eugenebut wrote: > > Please move this block closer to the place where it's actually used. > > Please see above. From C++ Style Guide: "Place a function's variables in the narrowest scope possible" https://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Local_Variables Please reorder variables inside this method to follow style guide. https://codereview.chromium.org/1375023002/diff/140001/ios/web/web_state/ui/c... File ios/web/web_state/ui/crw_wk_web_view_web_controller.mm (right): https://codereview.chromium.org/1375023002/diff/140001/ios/web/web_state/ui/c... ios/web/web_state/ui/crw_wk_web_view_web_controller.mm:519: - (NSString*)HTMLForPOSTRequest:(NSURLRequest*)request { NIT: Please move these methods before "webViewConfigurationProvider" to follow predeclarations order. https://codereview.chromium.org/1375023002/diff/140001/ios/web/web_state/ui/c... ios/web/web_state/ui/crw_wk_web_view_web_controller.mm:594: // If the request has POST data and is not a form resubmission, configure and This comment should be at line 621, not here.
https://codereview.chromium.org/1375023002/diff/140001/ios/web/web_state/ui/c... File ios/web/web_state/ui/crw_wk_web_view_web_controller.mm (right): https://codereview.chromium.org/1375023002/diff/140001/ios/web/web_state/ui/c... ios/web/web_state/ui/crw_wk_web_view_web_controller.mm:60: return base::SysUTF8ToNSString(base::GetQuotedJSONString(string)); The round-tripping of what's going to be a really big string makes me sad. I don't suppose GTM or google3 iOS code has a method we could use that does this directly on NSString? https://codereview.chromium.org/1375023002/diff/140001/ios/web/web_state/ui/c... ios/web/web_state/ui/crw_wk_web_view_web_controller.mm:75: @"<html><script>" I'm with Eugene; this is sizable chunk of JS, and it's pretty hard to read like this. We're also missing out on the compiler giving us warnings, and I think it says something about what this structure encourages that none of the methods here are commented. I'd like to see this done via a helper object that looks more like one of the JSInjectionManager classes in than it loads the file, caches it, and adds whatever templating. That can also hold the fiddly logic of HTMLForPOSTRequest: instead of that being in WebController itself. Note that you'd want to replace the inline templating with something where the JS is written to take params, and then the final building step would append JS that calls those functions, with arguments that are filled in by template. That makes the templating easier to understand (vs scattered %@ in a giant string), and let the JS stand alone. (And at that point, couldn't we share with Mirror pretty easily after all? It would just need a binary vs string body option. The request header building+escaping seems like a fair amount of code code that's important to get right, so having two copies concerns me.)
Please take another look. https://codereview.chromium.org/1375023002/diff/140001/ios/web/web_state/ui/c... File ios/web/web_state/ui/crw_wk_web_view_web_controller.mm (right): https://codereview.chromium.org/1375023002/diff/140001/ios/web/web_state/ui/c... ios/web/web_state/ui/crw_wk_web_view_web_controller.mm:60: return base::SysUTF8ToNSString(base::GetQuotedJSONString(string)); On 2015/10/01 at 23:36:27, stuartmorgan wrote: > The round-tripping of what's going to be a really big string makes me sad. I don't suppose GTM or google3 iOS code has a method we could use that does this directly on NSString? I can't find a way to do it directly on NSString. I also don't think that it is a big issue. The strings IRL seem to be quite small, especially compared to binary data encoded to base64 and then to binary and carried over on main thread. https://codereview.chromium.org/1375023002/diff/140001/ios/web/web_state/ui/c... ios/web/web_state/ui/crw_wk_web_view_web_controller.mm:75: @"<html><script>" On 2015/10/01 23:36:27, stuartmorgan wrote: > I'm with Eugene; this is sizable chunk of JS, and it's pretty hard to read like > this. We're also missing out on the compiler giving us warnings, and I think it > says something about what this structure encourages that none of the methods > here are commented. > > I'd like to see this done via a helper object that looks more like one of the > JSInjectionManager classes in than it loads the file, caches it, and adds > whatever templating. That can also hold the fiddly logic of HTMLForPOSTRequest: > instead of that being in WebController itself. > > Note that you'd want to replace the inline templating with something where the > JS is written to take params, and then the final building step would append JS > that calls those functions, with arguments that are filled in by template. That > makes the templating easier to understand (vs scattered %@ in a giant string), > and let the JS stand alone. > > (And at that point, couldn't we share with Mirror pretty easily after all? It > would just need a binary vs string body option. The request header > building+escaping seems like a fair amount of code code that's important to get > right, so having two copies concerns me.) Done. https://codereview.chromium.org/1375023002/diff/140001/ios/web/web_state/ui/c... ios/web/web_state/ui/crw_wk_web_view_web_controller.mm:519: - (NSString*)HTMLForPOSTRequest:(NSURLRequest*)request { On 2015/09/30 15:45:16, eugenebut wrote: > NIT: Please move these methods before "webViewConfigurationProvider" to follow > predeclarations order. Done. https://codereview.chromium.org/1375023002/diff/140001/ios/web/web_state/ui/c... ios/web/web_state/ui/crw_wk_web_view_web_controller.mm:594: // If the request has POST data and is not a form resubmission, configure and On 2015/09/30 15:45:16, eugenebut wrote: > This comment should be at line 621, not here. Done.
https://codereview.chromium.org/1375023002/diff/20001/ios/web/web_state/ui/cr... File ios/web/web_state/ui/crw_wk_web_view_web_controller.mm (right): https://codereview.chromium.org/1375023002/diff/20001/ios/web/web_state/ui/cr... ios/web/web_state/ui/crw_wk_web_view_web_controller.mm:585: ProceduralBlock POSTBlock = ^{ On 2015/09/30 15:45:16, eugenebut wrote: > On 2015/09/30 15:05:14, stkhapugin wrote: > > On 2015/09/29 17:46:46, eugenebut wrote: > > > Is there a specific reason why you want to hold this code in the block? Why > > not > > > inline it instead? > > > > It's a question of style, really. Two blocks were already here, so when I > added > > a 3rd way to navigate to URL, I decided to keep the style. I moved the blocks > > together, so that all options are explained in one place and then all the > option > > selection is nice and straightforward. > > > > I understand that the third block is not actually passed as a block, so if you > > insist, I will restructure this. But I want to have some logic behind the way > > things are structured here. > Block creation has some overhead, so it's not only about style. I would > encourage you to flatten the code and get rid of this block. Please address this comment. https://codereview.chromium.org/1375023002/diff/20001/ios/web/web_state/ui/cr... ios/web/web_state/ui/crw_wk_web_view_web_controller.mm:595: ProceduralBlock webViewNavigationBlock = ^{ On 2015/09/30 15:45:16, eugenebut wrote: > On 2015/09/30 15:05:13, stkhapugin wrote: > > On 2015/09/29 17:46:45, eugenebut wrote: > > > Please move this block closer to the place where it's actually used. > > > > Please see above. > From C++ Style Guide: "Place a function's variables in the narrowest scope > possible" > https://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Local_Variables > > Please reorder variables inside this method to follow style guide. > Please address this comment. https://codereview.chromium.org/1375023002/diff/260001/ios/web/web_state/js/c... File ios/web/web_state/js/crw_js_post_request_loader.h (right): https://codereview.chromium.org/1375023002/diff/260001/ios/web/web_state/js/c... ios/web/web_state/js/crw_js_post_request_loader.h:5: #ifndef IOS_WEB_WEB_STATE_JS_CRW_JS_HTML_LOADER_H_ This does not match filename https://codereview.chromium.org/1375023002/diff/260001/ios/web/web_state/js/c... ios/web/web_state/js/crw_js_post_request_loader.h:11: @interface CRWJSPOSTRequestLoader : NSObject Do we need a class here? Can we have a function instead? https://codereview.chromium.org/1375023002/diff/260001/ios/web/web_state/js/c... ios/web/web_state/js/crw_js_post_request_loader.h:14: - (void)loadPOSTRequest:(NSURLRequest*)request inWebView:(WKWebView*)webView; This needs a unit test https://codereview.chromium.org/1375023002/diff/260001/ios/web/web_state/js/c... File ios/web/web_state/js/crw_js_post_request_loader.mm (right): https://codereview.chromium.org/1375023002/diff/260001/ios/web/web_state/js/c... ios/web/web_state/js/crw_js_post_request_loader.mm:34: NSString* urlString = [[request URL] absoluteString]; s/urlString/URLString https://codereview.chromium.org/1375023002/diff/260001/ios/web/web_state/js/c... ios/web/web_state/js/crw_js_post_request_loader.mm:63: // Constructs an HTML page that executes the request through JavaScript and Remove this? https://codereview.chromium.org/1375023002/diff/260001/ios/web/web_state/js/r... File ios/web/web_state/js/resources/post_request.js (right): https://codereview.chromium.org/1375023002/diff/260001/ios/web/web_state/js/r... ios/web/web_state/js/resources/post_request.js:10: goog.provide('__crPOSTRequestHack'); s/__crPOSTRequestHack/__crWeb.postRequestHack https://codereview.chromium.org/1375023002/diff/260001/ios/web/web_state/js/r... ios/web/web_state/js/resources/post_request.js:15: __crPOSTRequestHack = {}; __crPostRequestHack According to JS Style Guide acronyms should use camelCase. Just search for "Url" here: https://engdoc.corp.google.com/eng/doc/javascriptguide.xml?cl=head#Naming to find examples https://codereview.chromium.org/1375023002/diff/260001/ios/web/web_state/js/r... ios/web/web_state/js/resources/post_request.js:26: __crPOSTRequestHack.runPOSTRequest = function(url, headers, base64Data, NIT: s/base64Data/body this way we can keep consistent terminology https://codereview.chromium.org/1375023002/diff/260001/ios/web/web_state/js/r... ios/web/web_state/js/resources/post_request.js:60: var createAndSendPOSTRequest = function(url, headers, b64Data, contentType){ createAndSendPostRequest https://codereview.chromium.org/1375023002/diff/260001/ios/web/web_state/js/r... ios/web/web_state/js/resources/post_request.js:60: var createAndSendPOSTRequest = function(url, headers, b64Data, contentType){ NIT: s/b64Data/body https://codereview.chromium.org/1375023002/diff/260001/ios/web/web_state/js/r... ios/web/web_state/js/resources/post_request.js:64: request.setRequestHeader(key, headers[key]); You want to make hasOwnProperty check, otherwise your |key| may come from |headers| superclass. https://codereview.chromium.org/1375023002/diff/260001/ios/web/web_state/js/r... ios/web/web_state/js/resources/post_request.js:75: document.write(createAndSendPOSTRequest(url, headers, base64Data, document.write reinjects our scripts w/o giving us any callbacks (sorry I missed this during the last codereview). Could you please check if Find In Page works after Search By Image. https://codereview.chromium.org/1375023002/diff/260001/ios/web/web_state/ui/c... File ios/web/web_state/ui/crw_wk_web_view_web_controller.mm (right): https://codereview.chromium.org/1375023002/diff/260001/ios/web/web_state/ui/c... ios/web/web_state/ui/crw_wk_web_view_web_controller.mm:90: base::mac::ObjCPropertyReleaser _propertyReleaser_CRWWKWebViewWebController; I would prefer to avoid mixing ObjCPropertyReleaser with scoped_nsobject (which is already used here). So please consider using scoped_nsobject for CRWJSPOSTRequestLoader. https://codereview.chromium.org/1375023002/diff/260001/ios/web/web_state/ui/c... ios/web/web_state/ui/crw_wk_web_view_web_controller.mm:180: // that executes the request through javascript and replaces document with the NIT: s/javascript/JavaScript https://codereview.chromium.org/1375023002/diff/260001/ios/web/web_state/ui/c... ios/web/web_state/ui/crw_wk_web_view_web_controller.mm:187: - (void)loadPOSTRequestWithBody:(NSMutableURLRequest*)request; Word before argument should describe argument. This name suggests that argument is "Body", which is not true. s/loadPOSTRequestWithBody:/loadPOSTRequest:
https://codereview.chromium.org/1375023002/diff/260001/ios/web/web_state/js/c... File ios/web/web_state/js/crw_js_post_request_loader.mm (right): https://codereview.chromium.org/1375023002/diff/260001/ios/web/web_state/js/c... ios/web/web_state/js/crw_js_post_request_loader.mm:24: NSString* js = web::GetPageScript(@"post_request"); How long does this take on an older device? I suspect we may want to cache this. https://codereview.chromium.org/1375023002/diff/260001/ios/web/web_state/js/c... ios/web/web_state/js/crw_js_post_request_loader.mm:31: - (NSString*)stringToExecutePOSTRequest:(NSURLRequest*)request { None of your private methods are declared or documented. https://codereview.chromium.org/1375023002/diff/260001/ios/web/web_state/js/c... ios/web/web_state/js/crw_js_post_request_loader.mm:56: encoding:NSUTF8StringEncoding] autorelease]; This doesn't need any quoting or escaping? If not, please add a comment here explaining why, since it's non-obvious here and above why only this isn't escaped. https://codereview.chromium.org/1375023002/diff/260001/ios/web/web_state/js/r... File ios/web/web_state/js/resources/post_request.js (right): https://codereview.chromium.org/1375023002/diff/260001/ios/web/web_state/js/r... ios/web/web_state/js/resources/post_request.js:69: throw request.status; Why are we throwing a JS exception for non-200 status? More generally, what exactly does happen if, e.g., a user tries to re-post and there's an error.
On 2015/10/08 19:11:29, stuartmorgan wrote: > More generally, what exactly does happen if, e.g., a user tries to re-post and > there's an error. I need to use [WKWebView loadHTMLString:baseURL:] instead of evaluateJavaScript because I'll get cross-origin errors otherwise. Do you think it would be appropriate to post a message from javascript and add a message handler to CRWWKScriptMessageRouter when the request fails? What should such a handler do - is it enough to do [id<WKNavigationDelegate> webView:didFailNavigation:withError:] or even [CRWWKWebViewWebController handleLoadError:inMainFrame:]?
A JS message with a handler sounds reasonable, yes. It should *definitely* not call a WKWebView delegate method directly, as that would be incredibly confusing. It should call whatever internals make sense to handle the error (factoring out common pieces if necessary).
Please take another look.
Please reply to comments fro previous round. https://codereview.chromium.org/1375023002/diff/20001/ios/web/web_state/ui/cr... File ios/web/web_state/ui/crw_wk_web_view_web_controller.mm (right): https://codereview.chromium.org/1375023002/diff/20001/ios/web/web_state/ui/cr... ios/web/web_state/ui/crw_wk_web_view_web_controller.mm:527: // expected to be low. On 2015/09/30 15:45:16, eugenebut wrote: > On 2015/09/30 15:05:14, stkhapugin wrote: > > On 2015/09/29 17:46:46, eugenebut wrote: > > > How does it perform with huge images? :) > > > > I tested with iPad Air 1 and a 3k*2k image, and it was surprisingly good. This > > is mainly because we already have the logic to downscale large images. The > > bottleneck is still the load of the image (we need to load it for the second > > time). This can be improved by doing all work in JavaScript, including image > > resizing and opening a new tab. > > > > However, I decided to go with this solution, because POST may be useful for > > other things. :) > Thank you. Then I guess you can remove "hence the performance is expected to be > low" comment. Please address this comment. https://codereview.chromium.org/1375023002/diff/20001/ios/web/web_state/ui/cr... ios/web/web_state/ui/crw_wk_web_view_web_controller.mm:585: ProceduralBlock POSTBlock = ^{ On 2015/10/08 17:46:32, eugenebut wrote: > On 2015/09/30 15:45:16, eugenebut wrote: > > On 2015/09/30 15:05:14, stkhapugin wrote: > > > On 2015/09/29 17:46:46, eugenebut wrote: > > > > Is there a specific reason why you want to hold this code in the block? > Why > > > not > > > > inline it instead? > > > > > > It's a question of style, really. Two blocks were already here, so when I > > added > > > a 3rd way to navigate to URL, I decided to keep the style. I moved the > blocks > > > together, so that all options are explained in one place and then all the > > option > > > selection is nice and straightforward. > > > > > > I understand that the third block is not actually passed as a block, so if > you > > > insist, I will restructure this. But I want to have some logic behind the > way > > > things are structured here. > > Block creation has some overhead, so it's not only about style. I would > > encourage you to flatten the code and get rid of this block. > Please address this comment. Please address this comment. https://codereview.chromium.org/1375023002/diff/20001/ios/web/web_state/ui/cr... ios/web/web_state/ui/crw_wk_web_view_web_controller.mm:595: ProceduralBlock webViewNavigationBlock = ^{ On 2015/10/08 17:46:32, eugenebut wrote: > On 2015/09/30 15:45:16, eugenebut wrote: > > On 2015/09/30 15:05:13, stkhapugin wrote: > > > On 2015/09/29 17:46:45, eugenebut wrote: > > > > Please move this block closer to the place where it's actually used. > > > > > > Please see above. > > From C++ Style Guide: "Place a function's variables in the narrowest scope > > possible" > > > https://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Local_Variables > > > > Please reorder variables inside this method to follow style guide. > > > Please address this comment. Please address this comment. https://codereview.chromium.org/1375023002/diff/320001/ios/web/ios_web.gyp File ios/web/ios_web.gyp (right): https://codereview.chromium.org/1375023002/diff/320001/ios/web/ios_web.gyp#ne... ios/web/ios_web.gyp:240: 'web_state/js/crw_js_post_request_loader.h', Please update GN file as well. https://codereview.chromium.org/1375023002/diff/320001/ios/web/ios_web_unitte... File ios/web/ios_web_unittests.gyp (right): https://codereview.chromium.org/1375023002/diff/320001/ios/web/ios_web_unitte... ios/web/ios_web_unittests.gyp:65: 'web_state/js/crw_js_post_request_loader_unittest.mm', Please update GN file as well. https://codereview.chromium.org/1375023002/diff/320001/ios/web/web_state/js/c... File ios/web/web_state/js/crw_js_post_request_loader.h (right): https://codereview.chromium.org/1375023002/diff/320001/ios/web/web_state/js/c... ios/web/web_state/js/crw_js_post_request_loader.h:8: #import <WebKit/WKWebView.h> s/<WebKit/WKWebView.h>/<WebKit/WebKit.h> https://google.github.io/styleguide/objcguide.xml?showone=Use_Root_Frameworks... https://codereview.chromium.org/1375023002/diff/320001/ios/web/web_state/js/c... ios/web/web_state/js/crw_js_post_request_loader.h:22: errorBlock:(void (^)(NSError*))errorBlock There should be only one completion handler for both success and failure cases. For success case NSError is nil. - (void)loadPOSTRequest:(NSURLRequest*)request .... completionHandler:(void (^)(NSError*))completionHandler Having 2 blocks is error prone, lead to code duplication and makes formatting uglier. https://codereview.chromium.org/1375023002/diff/320001/ios/web/web_state/js/c... File ios/web/web_state/js/crw_js_post_request_loader.mm (right): https://codereview.chromium.org/1375023002/diff/320001/ios/web/web_state/js/c... ios/web/web_state/js/crw_js_post_request_loader.mm:8: #include "base/mac/objc_property_releaser.h" s/include/import https://codereview.chromium.org/1375023002/diff/320001/ios/web/web_state/js/c... ios/web/web_state/js/crw_js_post_request_loader.mm:21: NSString* const kErrorHandlerName = @"POSTErrorHandler"; These need comments https://codereview.chromium.org/1375023002/diff/320001/ios/web/web_state/js/c... ios/web/web_state/js/crw_js_post_request_loader.mm:38: - (NSString*)stringToExecutePOSTRequest:(NSURLRequest*)request; s/stringToExecutePOSTRequest:/scriptToExecutePOSTRequest: https://codereview.chromium.org/1375023002/diff/320001/ios/web/web_state/js/c... ios/web/web_state/js/crw_js_post_request_loader.mm:41: - (NSString*)stringForJavaScriptFromRequestHeaders:(NSDictionary*)headers; s/stringForJavaScriptFromRequestHeaders:/JSONForJavaScriptFromRequestHeaders: https://codereview.chromium.org/1375023002/diff/320001/ios/web/web_state/js/c... ios/web/web_state/js/crw_js_post_request_loader.mm:69: _requestScript = [web::GetPageScript(@"post_request") retain]; s/retain/copy Strings should be copied. https://google.github.io/styleguide/objcguide.xml?showone=Use_Root_Frameworks... https://codereview.chromium.org/1375023002/diff/320001/ios/web/web_state/js/c... ios/web/web_state/js/crw_js_post_request_loader.mm:88: // during its execution otherwise. Which block is deallocated? one which is passed to |setScriptMessageHandler:| or |successBlock|? https://codereview.chromium.org/1375023002/diff/320001/ios/web/web_state/js/c... ios/web/web_state/js/crw_js_post_request_loader.mm:104: NSError* error = [NSError errorWithDomain:@"http" Please add a constant for error domain and put it to the header. Also it should be something less generic that @http" https://codereview.chromium.org/1375023002/diff/320001/ios/web/web_state/js/c... ios/web/web_state/js/crw_js_post_request_loader.mm:105: code:[statusCode integerValue] Optional NIT: s/[statusCode integerValue]/statusCode.integerValue https://codereview.chromium.org/1375023002/diff/320001/ios/web/web_state/js/c... ios/web/web_state/js/crw_js_post_request_loader.mm:121: NSString* js = [self requestScript]; Optional NIT: s/[self requestScript]/self.requestScript https://codereview.chromium.org/1375023002/diff/320001/ios/web/web_state/js/c... ios/web/web_state/js/crw_js_post_request_loader.mm:121: NSString* js = [self requestScript]; NIT: Please drop this variable and just inline the call. If you don't want to do that then s/js/JS https://codereview.chromium.org/1375023002/diff/320001/ios/web/web_state/js/c... ios/web/web_state/js/crw_js_post_request_loader.mm:143: stringWithFormat:@"__crPostRequestHack.runPostRequest(%@, %@, %@, %@)", s/Hack/Workaround :) https://codereview.chromium.org/1375023002/diff/320001/ios/web/web_state/js/c... ios/web/web_state/js/crw_js_post_request_loader.mm:152: if (headers) { Optional NIT: I would restructure this: if (headers) { NSData* headerData = [NSJSONSerialization dataWithJSONObject:headers options:0 error:nil]; if (headerData) { return [[[NSString alloc] initWithData:headerData encoding:NSUTF8StringEncoding] autorelease]; } } return @"{}"; https://codereview.chromium.org/1375023002/diff/320001/ios/web/web_state/js/c... File ios/web/web_state/js/crw_js_post_request_loader_unittest.mm (right): https://codereview.chromium.org/1375023002/diff/320001/ios/web/web_state/js/c... ios/web/web_state/js/crw_js_post_request_loader_unittest.mm:9: #import "base/strings/sys_string_conversions.h" s/import/include https://codereview.chromium.org/1375023002/diff/320001/ios/web/web_state/js/c... ios/web/web_state/js/crw_js_post_request_loader_unittest.mm:22: std::string expectedHTML = FYI: Variables of class type with static storage duration are forbidden: https://engdoc.corp.google.com/eng/doc/devguide/cpp/styleguide.shtml?cl=head#... https://codereview.chromium.org/1375023002/diff/320001/ios/web/web_state/js/c... ios/web/web_state/js/crw_js_post_request_loader_unittest.mm:23: R"delimeter(<html><script>var __crPostRequestHack={runPostRequest:function(k,l,m,n){var p=function(b,d,c,g){var e=new XMLHttpRequest;e.open("POST",b,!1);for(var a in d)d.hasOwnProperty(a)&&e.setRequestHeader(a,d[a]);b=atob(c);g=g||"";d=[];for(c=0;c<b.length;c+=512){a=b.slice(c,c+512);for(var h=Array(a.length),f=0;f<a.length;f++)h[f]=a.charCodeAt(f);a=new Uint8Array(h);d.push(a)}b=new Blob(d,{type:g});e.send(b);if(200!=e.status)throw e.status;return e.responseText};document.open();try{document.write(p(k,l,m,n)),window.webkit.messageHandlers.POSTSuccess.postMessage("")}catch(b){window.webkit.messageHandlers.POSTErrorHandler.postMessage(b)}document.close()}}; This is too fragile, and will be breaking very often. We need some other approach for unittesting. How about overriding XHR.send() ? https://codereview.chromium.org/1375023002/diff/320001/ios/web/web_state/js/c... ios/web/web_state/js/crw_js_post_request_loader_unittest.mm:52: TEST(CRWJSPOSTRequestLoaderTest, installsCallbacks) { s/installsCallbacks/InstallsCallbacks https://codereview.chromium.org/1375023002/diff/320001/ios/web/web_state/js/c... ios/web/web_state/js/crw_js_post_request_loader_unittest.mm:52: TEST(CRWJSPOSTRequestLoaderTest, installsCallbacks) { Installing and removing script message router is testing interactions and implementation detail of this class, hence I don't think it worth testing. However it up up to you. https://codereview.chromium.org/1375023002/diff/320001/ios/web/web_state/js/r... File ios/web/web_state/js/resources/post_request.js (right): https://codereview.chromium.org/1375023002/diff/320001/ios/web/web_state/js/r... ios/web/web_state/js/resources/post_request.js:39: for (var offset = 0; offset < byteCharacters.length; offset += sliceSize){ Space before |{|, here and everywhere. https://codereview.chromium.org/1375023002/diff/320001/ios/web/web_state/js/r... ios/web/web_state/js/resources/post_request.js:48: var blob = new Blob(byteArrays, {type: contentType}); return new Blob(byteArrays, {type: contentType}); https://codereview.chromium.org/1375023002/diff/320001/ios/web/web_state/js/r... ios/web/web_state/js/resources/post_request.js:79: contentType)); This does not look like correct formatting https://codereview.chromium.org/1375023002/diff/320001/ios/web/web_state/ui/c... File ios/web/web_state/ui/crw_wk_web_view_web_controller.mm (right): https://codereview.chromium.org/1375023002/diff/320001/ios/web/web_state/ui/c... ios/web/web_state/ui/crw_wk_web_view_web_controller.mm:156: base::mac::ObjCPropertyReleaser _propertyReleaser_CRWWKWebViewWebController; Please don't use property releaser here. All memory is managed with scoped_pointers and new code should be consistent with that. https://codereview.chromium.org/1375023002/diff/320001/ios/web/web_state/ui/c... ios/web/web_state/ui/crw_wk_web_view_web_controller.mm:250: - (void)loadPOSTRequestWithBody:(NSMutableURLRequest*)request; Word before argument should describe argument. How about just loadPOSTRequest:? https://codereview.chromium.org/1375023002/diff/320001/ios/web/web_state/ui/c... ios/web/web_state/ui/crw_wk_web_view_web_controller.mm:762: if (!self.POSTRequestLoader) { s/self.POSTRequestLoader/_POSTRequestLoader https://codereview.chromium.org/1375023002/diff/320001/ios/web/web_state/ui/c... ios/web/web_state/ui/crw_wk_web_view_web_controller.mm:770: inWebView:_wkWebView This is not correct formatting. Di you run git cl format?
PTAL. I believe I addressed all the comments except "http" error domain where I need advice. https://codereview.chromium.org/1375023002/diff/20001/ios/web/web_state/ui/cr... File ios/web/web_state/ui/crw_wk_web_view_web_controller.mm (right): https://codereview.chromium.org/1375023002/diff/20001/ios/web/web_state/ui/cr... ios/web/web_state/ui/crw_wk_web_view_web_controller.mm:71: NSString* const kPostRequestTemplate = On 2015/09/30 15:45:16, eugenebut wrote: > On 2015/09/30 15:05:14, stkhapugin wrote: > > On 2015/09/29 17:46:46, eugenebut wrote: > > > This is a large piece of JS code. Maybe it will be better to store in in a > > > separate .js file as a function, compile with closure and load using > > > web::GetPageScript function? > > > > This is actually a piece of html. > You can load your script from file and then wrap inside "<html><script>" tags. > In general I don't have strong opinion on this. You can leave the code as it is > as long as other reviewers are fine with having long js function here. > > > How do you think, will it work if I load a > > blank page and load&execute the script in it? Also note that I'm loading the > > page with a baseURL, so I still need to load html string to specify it. > Loading the page and executing js inside will be tricky, because load is > asynchronous operation. Acknowledged. https://codereview.chromium.org/1375023002/diff/20001/ios/web/web_state/ui/cr... ios/web/web_state/ui/crw_wk_web_view_web_controller.mm:527: // expected to be low. On 2015/09/30 15:45:16, eugenebut wrote: > On 2015/09/30 15:05:14, stkhapugin wrote: > > On 2015/09/29 17:46:46, eugenebut wrote: > > > How does it perform with huge images? :) > > > > I tested with iPad Air 1 and a 3k*2k image, and it was surprisingly good. This > > is mainly because we already have the logic to downscale large images. The > > bottleneck is still the load of the image (we need to load it for the second > > time). This can be improved by doing all work in JavaScript, including image > > resizing and opening a new tab. > > > > However, I decided to go with this solution, because POST may be useful for > > other things. :) > Thank you. Then I guess you can remove "hence the performance is expected to be > low" comment. Done. https://codereview.chromium.org/1375023002/diff/20001/ios/web/web_state/ui/cr... ios/web/web_state/ui/crw_wk_web_view_web_controller.mm:527: // expected to be low. On 2015/12/02 17:06:14, eugenebut wrote: > On 2015/09/30 15:45:16, eugenebut wrote: > > On 2015/09/30 15:05:14, stkhapugin wrote: > > > On 2015/09/29 17:46:46, eugenebut wrote: > > > > How does it perform with huge images? :) > > > > > > I tested with iPad Air 1 and a 3k*2k image, and it was surprisingly good. > This > > > is mainly because we already have the logic to downscale large images. The > > > bottleneck is still the load of the image (we need to load it for the second > > > time). This can be improved by doing all work in JavaScript, including image > > > resizing and opening a new tab. > > > > > > However, I decided to go with this solution, because POST may be useful for > > > other things. :) > > Thank you. Then I guess you can remove "hence the performance is expected to > be > > low" comment. > Please address this comment. Done. https://codereview.chromium.org/1375023002/diff/20001/ios/web/web_state/ui/cr... ios/web/web_state/ui/crw_wk_web_view_web_controller.mm:585: ProceduralBlock POSTBlock = ^{ On 2015/09/30 15:45:16, eugenebut wrote: > On 2015/09/30 15:05:14, stkhapugin wrote: > > On 2015/09/29 17:46:46, eugenebut wrote: > > > Is there a specific reason why you want to hold this code in the block? Why > > not > > > inline it instead? > > > > It's a question of style, really. Two blocks were already here, so when I > added > > a 3rd way to navigate to URL, I decided to keep the style. I moved the blocks > > together, so that all options are explained in one place and then all the > option > > selection is nice and straightforward. > > > > I understand that the third block is not actually passed as a block, so if you > > insist, I will restructure this. But I want to have some logic behind the way > > things are structured here. > Block creation has some overhead, so it's not only about style. I would > encourage you to flatten the code and get rid of this block. Done. https://codereview.chromium.org/1375023002/diff/20001/ios/web/web_state/ui/cr... ios/web/web_state/ui/crw_wk_web_view_web_controller.mm:585: ProceduralBlock POSTBlock = ^{ On 2015/10/08 17:46:32, eugenebut wrote: > On 2015/09/30 15:45:16, eugenebut wrote: > > On 2015/09/30 15:05:14, stkhapugin wrote: > > > On 2015/09/29 17:46:46, eugenebut wrote: > > > > Is there a specific reason why you want to hold this code in the block? > Why > > > not > > > > inline it instead? > > > > > > It's a question of style, really. Two blocks were already here, so when I > > added > > > a 3rd way to navigate to URL, I decided to keep the style. I moved the > blocks > > > together, so that all options are explained in one place and then all the > > option > > > selection is nice and straightforward. > > > > > > I understand that the third block is not actually passed as a block, so if > you > > > insist, I will restructure this. But I want to have some logic behind the > way > > > things are structured here. > > Block creation has some overhead, so it's not only about style. I would > > encourage you to flatten the code and get rid of this block. > Please address this comment. Done. https://codereview.chromium.org/1375023002/diff/20001/ios/web/web_state/ui/cr... ios/web/web_state/ui/crw_wk_web_view_web_controller.mm:585: ProceduralBlock POSTBlock = ^{ On 2015/12/02 17:06:14, eugenebut wrote: > On 2015/10/08 17:46:32, eugenebut wrote: > > On 2015/09/30 15:45:16, eugenebut wrote: > > > On 2015/09/30 15:05:14, stkhapugin wrote: > > > > On 2015/09/29 17:46:46, eugenebut wrote: > > > > > Is there a specific reason why you want to hold this code in the block? > > Why > > > > not > > > > > inline it instead? > > > > > > > > It's a question of style, really. Two blocks were already here, so when I > > > added > > > > a 3rd way to navigate to URL, I decided to keep the style. I moved the > > blocks > > > > together, so that all options are explained in one place and then all the > > > option > > > > selection is nice and straightforward. > > > > > > > > I understand that the third block is not actually passed as a block, so if > > you > > > > insist, I will restructure this. But I want to have some logic behind the > > way > > > > things are structured here. > > > Block creation has some overhead, so it's not only about style. I would > > > encourage you to flatten the code and get rid of this block. > > Please address this comment. > Please address this comment. Done. https://codereview.chromium.org/1375023002/diff/20001/ios/web/web_state/ui/cr... ios/web/web_state/ui/crw_wk_web_view_web_controller.mm:595: ProceduralBlock webViewNavigationBlock = ^{ On 2015/10/08 17:46:32, eugenebut wrote: > On 2015/09/30 15:45:16, eugenebut wrote: > > On 2015/09/30 15:05:13, stkhapugin wrote: > > > On 2015/09/29 17:46:45, eugenebut wrote: > > > > Please move this block closer to the place where it's actually used. > > > > > > Please see above. > > From C++ Style Guide: "Place a function's variables in the narrowest scope > > possible" > > > https://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Local_Variables > > > > Please reorder variables inside this method to follow style guide. > > > Please address this comment. Done. https://codereview.chromium.org/1375023002/diff/20001/ios/web/web_state/ui/cr... ios/web/web_state/ui/crw_wk_web_view_web_controller.mm:595: ProceduralBlock webViewNavigationBlock = ^{ On 2015/12/02 17:06:14, eugenebut wrote: > On 2015/10/08 17:46:32, eugenebut wrote: > > On 2015/09/30 15:45:16, eugenebut wrote: > > > On 2015/09/30 15:05:13, stkhapugin wrote: > > > > On 2015/09/29 17:46:45, eugenebut wrote: > > > > > Please move this block closer to the place where it's actually used. > > > > > > > > Please see above. > > > From C++ Style Guide: "Place a function's variables in the narrowest scope > > > possible" > > > > > > https://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Local_Variables > > > > > > Please reorder variables inside this method to follow style guide. > > > > > Please address this comment. > Please address this comment. Done. https://codereview.chromium.org/1375023002/diff/20001/ios/web/web_state/ui/cr... ios/web/web_state/ui/crw_wk_web_view_web_controller.mm:595: ProceduralBlock webViewNavigationBlock = ^{ On 2015/09/30 15:45:16, eugenebut wrote: > On 2015/09/30 15:05:13, stkhapugin wrote: > > On 2015/09/29 17:46:45, eugenebut wrote: > > > Please move this block closer to the place where it's actually used. > > > > Please see above. > From C++ Style Guide: "Place a function's variables in the narrowest scope > possible" > https://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Local_Variables > > Please reorder variables inside this method to follow style guide. > Done. https://codereview.chromium.org/1375023002/diff/260001/ios/web/web_state/js/c... File ios/web/web_state/js/crw_js_post_request_loader.h (right): https://codereview.chromium.org/1375023002/diff/260001/ios/web/web_state/js/c... ios/web/web_state/js/crw_js_post_request_loader.h:5: #ifndef IOS_WEB_WEB_STATE_JS_CRW_JS_HTML_LOADER_H_ On 2015/10/08 17:46:32, eugenebut wrote: > This does not match filename Done. https://codereview.chromium.org/1375023002/diff/260001/ios/web/web_state/js/c... ios/web/web_state/js/crw_js_post_request_loader.h:11: @interface CRWJSPOSTRequestLoader : NSObject On 2015/10/08 17:46:32, eugenebut wrote: > Do we need a class here? Can we have a function instead? We need a class here because we want to cache the javascript from the file. https://codereview.chromium.org/1375023002/diff/260001/ios/web/web_state/js/c... ios/web/web_state/js/crw_js_post_request_loader.h:14: - (void)loadPOSTRequest:(NSURLRequest*)request inWebView:(WKWebView*)webView; On 2015/10/08 17:46:32, eugenebut wrote: > This needs a unit test Done. https://codereview.chromium.org/1375023002/diff/260001/ios/web/web_state/js/c... File ios/web/web_state/js/crw_js_post_request_loader.mm (right): https://codereview.chromium.org/1375023002/diff/260001/ios/web/web_state/js/c... ios/web/web_state/js/crw_js_post_request_loader.mm:24: NSString* js = web::GetPageScript(@"post_request"); On 2015/10/08 19:11:29, stuartmorgan wrote: > How long does this take on an older device? I suspect we may want to cache this. Done. https://codereview.chromium.org/1375023002/diff/260001/ios/web/web_state/js/c... ios/web/web_state/js/crw_js_post_request_loader.mm:31: - (NSString*)stringToExecutePOSTRequest:(NSURLRequest*)request { On 2015/10/08 19:11:29, stuartmorgan wrote: > None of your private methods are declared or documented. Done. https://codereview.chromium.org/1375023002/diff/260001/ios/web/web_state/js/c... ios/web/web_state/js/crw_js_post_request_loader.mm:34: NSString* urlString = [[request URL] absoluteString]; On 2015/10/08 17:46:32, eugenebut wrote: > s/urlString/URLString Done. https://codereview.chromium.org/1375023002/diff/260001/ios/web/web_state/js/c... ios/web/web_state/js/crw_js_post_request_loader.mm:56: encoding:NSUTF8StringEncoding] autorelease]; On 2015/10/08 19:11:29, stuartmorgan wrote: > This doesn't need any quoting or escaping? If not, please add a comment here > explaining why, since it's non-obvious here and above why only this isn't > escaped. Done. https://codereview.chromium.org/1375023002/diff/260001/ios/web/web_state/js/c... ios/web/web_state/js/crw_js_post_request_loader.mm:63: // Constructs an HTML page that executes the request through JavaScript and On 2015/10/08 17:46:32, eugenebut wrote: > Remove this? Done. https://codereview.chromium.org/1375023002/diff/260001/ios/web/web_state/js/r... File ios/web/web_state/js/resources/post_request.js (right): https://codereview.chromium.org/1375023002/diff/260001/ios/web/web_state/js/r... ios/web/web_state/js/resources/post_request.js:10: goog.provide('__crPOSTRequestHack'); On 2015/10/08 17:46:32, eugenebut wrote: > s/__crPOSTRequestHack/__crWeb.postRequestHack Done. https://codereview.chromium.org/1375023002/diff/260001/ios/web/web_state/js/r... ios/web/web_state/js/resources/post_request.js:15: __crPOSTRequestHack = {}; On 2015/10/08 17:46:32, eugenebut wrote: > __crPostRequestHack > > According to JS Style Guide acronyms should use camelCase. > Just search for "Url" here: > https://engdoc.corp.google.com/eng/doc/javascriptguide.xml?cl=head#Naming to > find examples Done. https://codereview.chromium.org/1375023002/diff/260001/ios/web/web_state/js/r... ios/web/web_state/js/resources/post_request.js:26: __crPOSTRequestHack.runPOSTRequest = function(url, headers, base64Data, On 2015/10/08 17:46:32, eugenebut wrote: > NIT: s/base64Data/body this way we can keep consistent terminology Done. https://codereview.chromium.org/1375023002/diff/260001/ios/web/web_state/js/r... ios/web/web_state/js/resources/post_request.js:60: var createAndSendPOSTRequest = function(url, headers, b64Data, contentType){ On 2015/10/08 17:46:32, eugenebut wrote: > createAndSendPostRequest Done. https://codereview.chromium.org/1375023002/diff/260001/ios/web/web_state/js/r... ios/web/web_state/js/resources/post_request.js:60: var createAndSendPOSTRequest = function(url, headers, b64Data, contentType){ On 2015/10/08 17:46:32, eugenebut wrote: > NIT: s/b64Data/body Done. https://codereview.chromium.org/1375023002/diff/260001/ios/web/web_state/js/r... ios/web/web_state/js/resources/post_request.js:64: request.setRequestHeader(key, headers[key]); On 2015/10/08 17:46:32, eugenebut wrote: > You want to make hasOwnProperty check, otherwise your |key| may come from > |headers| superclass. Done. https://codereview.chromium.org/1375023002/diff/260001/ios/web/web_state/js/r... ios/web/web_state/js/resources/post_request.js:69: throw request.status; On 2015/10/08 19:11:29, stuartmorgan wrote: > Why are we throwing a JS exception for non-200 status? > > More generally, what exactly does happen if, e.g., a user tries to re-post and > there's an error. I added an error handler that catches any non-200 HTTP status and sends it over to Objective-C code via message handlers. Then the standard logic for displaying errors is executed in CRWWKWebViewWebController. The only question I have is how to wrap this into an NSError cleanly, i.e. which error domain I should use and is there any userInfo that should be provided (localized description etc)? https://codereview.chromium.org/1375023002/diff/260001/ios/web/web_state/js/r... ios/web/web_state/js/resources/post_request.js:75: document.write(createAndSendPOSTRequest(url, headers, base64Data, On 2015/10/08 17:46:32, eugenebut wrote: > document.write reinjects our scripts w/o giving us any callbacks (sorry I missed > this during the last codereview). > Could you please check if Find In Page works after Search By Image. Done. https://codereview.chromium.org/1375023002/diff/260001/ios/web/web_state/ui/c... File ios/web/web_state/ui/crw_wk_web_view_web_controller.mm (right): https://codereview.chromium.org/1375023002/diff/260001/ios/web/web_state/ui/c... ios/web/web_state/ui/crw_wk_web_view_web_controller.mm:90: base::mac::ObjCPropertyReleaser _propertyReleaser_CRWWKWebViewWebController; On 2015/10/08 17:46:33, eugenebut wrote: > I would prefer to avoid mixing ObjCPropertyReleaser with scoped_nsobject (which > is already used here). > So please consider using scoped_nsobject for CRWJSPOSTRequestLoader. Done. https://codereview.chromium.org/1375023002/diff/260001/ios/web/web_state/ui/c... ios/web/web_state/ui/crw_wk_web_view_web_controller.mm:180: // that executes the request through javascript and replaces document with the On 2015/10/08 17:46:33, eugenebut wrote: > NIT: s/javascript/JavaScript Done. https://codereview.chromium.org/1375023002/diff/260001/ios/web/web_state/ui/c... ios/web/web_state/ui/crw_wk_web_view_web_controller.mm:187: - (void)loadPOSTRequestWithBody:(NSMutableURLRequest*)request; On 2015/10/08 17:46:33, eugenebut wrote: > Word before argument should describe argument. This name suggests that argument > is "Body", which is not true. > > s/loadPOSTRequestWithBody:/loadPOSTRequest: Done. https://codereview.chromium.org/1375023002/diff/320001/ios/web/ios_web.gyp File ios/web/ios_web.gyp (right): https://codereview.chromium.org/1375023002/diff/320001/ios/web/ios_web.gyp#ne... ios/web/ios_web.gyp:240: 'web_state/js/crw_js_post_request_loader.h', On 2015/12/02 17:06:14, eugenebut wrote: > Please update GN file as well. Done. https://codereview.chromium.org/1375023002/diff/320001/ios/web/ios_web_unitte... File ios/web/ios_web_unittests.gyp (right): https://codereview.chromium.org/1375023002/diff/320001/ios/web/ios_web_unitte... ios/web/ios_web_unittests.gyp:65: 'web_state/js/crw_js_post_request_loader_unittest.mm', Done https://codereview.chromium.org/1375023002/diff/320001/ios/web/web_state/js/c... File ios/web/web_state/js/crw_js_post_request_loader.h (right): https://codereview.chromium.org/1375023002/diff/320001/ios/web/web_state/js/c... ios/web/web_state/js/crw_js_post_request_loader.h:8: #import <WebKit/WKWebView.h> On 2015/12/02 17:06:15, eugenebut wrote: > s/<WebKit/WKWebView.h>/<WebKit/WebKit.h> > > https://google.github.io/styleguide/objcguide.xml?showone=Use_Root_Frameworks... Done. https://codereview.chromium.org/1375023002/diff/320001/ios/web/web_state/js/c... ios/web/web_state/js/crw_js_post_request_loader.h:22: errorBlock:(void (^)(NSError*))errorBlock On 2015/12/02 17:06:15, eugenebut wrote: > There should be only one completion handler for both success and failure cases. > For success case NSError is nil. > > - (void)loadPOSTRequest:(NSURLRequest*)request > .... > completionHandler:(void (^)(NSError*))completionHandler > > Having 2 blocks is error prone, lead to code duplication and makes formatting > uglier. Done. https://codereview.chromium.org/1375023002/diff/320001/ios/web/web_state/js/c... File ios/web/web_state/js/crw_js_post_request_loader.mm (right): https://codereview.chromium.org/1375023002/diff/320001/ios/web/web_state/js/c... ios/web/web_state/js/crw_js_post_request_loader.mm:8: #include "base/mac/objc_property_releaser.h" On 2015/12/02 17:06:15, eugenebut wrote: > s/include/import Done. https://codereview.chromium.org/1375023002/diff/320001/ios/web/web_state/js/c... ios/web/web_state/js/crw_js_post_request_loader.mm:21: NSString* const kErrorHandlerName = @"POSTErrorHandler"; On 2015/12/02 17:06:15, eugenebut wrote: > These need comments Done. https://codereview.chromium.org/1375023002/diff/320001/ios/web/web_state/js/c... ios/web/web_state/js/crw_js_post_request_loader.mm:38: - (NSString*)stringToExecutePOSTRequest:(NSURLRequest*)request; On 2015/12/02 17:06:15, eugenebut wrote: > s/stringToExecutePOSTRequest:/scriptToExecutePOSTRequest: Done. https://codereview.chromium.org/1375023002/diff/320001/ios/web/web_state/js/c... ios/web/web_state/js/crw_js_post_request_loader.mm:41: - (NSString*)stringForJavaScriptFromRequestHeaders:(NSDictionary*)headers; On 2015/12/02 17:06:15, eugenebut wrote: > s/stringForJavaScriptFromRequestHeaders:/JSONForJavaScriptFromRequestHeaders: Done. https://codereview.chromium.org/1375023002/diff/320001/ios/web/web_state/js/c... ios/web/web_state/js/crw_js_post_request_loader.mm:69: _requestScript = [web::GetPageScript(@"post_request") retain]; On 2015/12/02 17:06:15, eugenebut wrote: > s/retain/copy > > Strings should be copied. > https://google.github.io/styleguide/objcguide.xml?showone=Use_Root_Frameworks... Done. https://codereview.chromium.org/1375023002/diff/320001/ios/web/web_state/js/c... ios/web/web_state/js/crw_js_post_request_loader.mm:88: // during its execution otherwise. On 2015/12/02 17:06:15, eugenebut wrote: > Which block is deallocated? one which is passed to |setScriptMessageHandler:| or > |successBlock|? The message handler block (the one where this comment is) is only retained by the dictionary in message router. Once you removeScriptHandlerForName:webView:, this block gets deallocated while being executed. https://codereview.chromium.org/1375023002/diff/320001/ios/web/web_state/js/c... ios/web/web_state/js/crw_js_post_request_loader.mm:104: NSError* error = [NSError errorWithDomain:@"http" On 2015/12/02 17:06:15, eugenebut wrote: > Please add a constant for error domain and put it to the header. Also it should > be something less generic that @http" Since the only error that can happen here is HTTP, should I use NSURLErrorDomain or something like that to get the default handling for HTTP errors? https://codereview.chromium.org/1375023002/diff/320001/ios/web/web_state/js/c... ios/web/web_state/js/crw_js_post_request_loader.mm:105: code:[statusCode integerValue] On 2015/12/02 17:06:15, eugenebut wrote: > Optional NIT: s/[statusCode integerValue]/statusCode.integerValue Done. https://codereview.chromium.org/1375023002/diff/320001/ios/web/web_state/js/c... ios/web/web_state/js/crw_js_post_request_loader.mm:121: NSString* js = [self requestScript]; On 2015/12/02 17:06:15, eugenebut wrote: > Optional NIT: s/[self requestScript]/self.requestScript Done. https://codereview.chromium.org/1375023002/diff/320001/ios/web/web_state/js/c... ios/web/web_state/js/crw_js_post_request_loader.mm:121: NSString* js = [self requestScript]; On 2015/12/02 17:06:15, eugenebut wrote: > NIT: Please drop this variable and just inline the call. If you don't want to do > that then s/js/JS Done. https://codereview.chromium.org/1375023002/diff/320001/ios/web/web_state/js/c... ios/web/web_state/js/crw_js_post_request_loader.mm:143: stringWithFormat:@"__crPostRequestHack.runPostRequest(%@, %@, %@, %@)", On 2015/12/02 17:06:15, eugenebut wrote: > s/Hack/Workaround :) Done. https://codereview.chromium.org/1375023002/diff/320001/ios/web/web_state/js/c... ios/web/web_state/js/crw_js_post_request_loader.mm:152: if (headers) { On 2015/12/02 17:06:15, eugenebut wrote: > Optional NIT: I would restructure this: > > if (headers) { > NSData* headerData = > [NSJSONSerialization dataWithJSONObject:headers options:0 error:nil]; > if (headerData) { > return [[[NSString alloc] initWithData:headerData > encoding:NSUTF8StringEncoding] autorelease]; > } > } > return @"{}"; Done. https://codereview.chromium.org/1375023002/diff/320001/ios/web/web_state/js/c... File ios/web/web_state/js/crw_js_post_request_loader_unittest.mm (right): https://codereview.chromium.org/1375023002/diff/320001/ios/web/web_state/js/c... ios/web/web_state/js/crw_js_post_request_loader_unittest.mm:9: #import "base/strings/sys_string_conversions.h" On 2015/12/02 17:06:15, eugenebut wrote: > s/import/include Done. https://codereview.chromium.org/1375023002/diff/320001/ios/web/web_state/js/c... ios/web/web_state/js/crw_js_post_request_loader_unittest.mm:22: std::string expectedHTML = On 2015/12/02 17:06:15, eugenebut wrote: > FYI: Variables of class type with static storage duration are forbidden: > > https://engdoc.corp.google.com/eng/doc/devguide/cpp/styleguide.shtml?cl=head#... Done. https://codereview.chromium.org/1375023002/diff/320001/ios/web/web_state/js/c... ios/web/web_state/js/crw_js_post_request_loader_unittest.mm:23: R"delimeter(<html><script>var __crPostRequestHack={runPostRequest:function(k,l,m,n){var p=function(b,d,c,g){var e=new XMLHttpRequest;e.open("POST",b,!1);for(var a in d)d.hasOwnProperty(a)&&e.setRequestHeader(a,d[a]);b=atob(c);g=g||"";d=[];for(c=0;c<b.length;c+=512){a=b.slice(c,c+512);for(var h=Array(a.length),f=0;f<a.length;f++)h[f]=a.charCodeAt(f);a=new Uint8Array(h);d.push(a)}b=new Blob(d,{type:g});e.send(b);if(200!=e.status)throw e.status;return e.responseText};document.open();try{document.write(p(k,l,m,n)),window.webkit.messageHandlers.POSTSuccess.postMessage("")}catch(b){window.webkit.messageHandlers.POSTErrorHandler.postMessage(b)}document.close()}}; On 2015/12/02 17:06:15, eugenebut wrote: > This is too fragile, and will be breaking very often. We need some other > approach for unittesting. How about overriding XHR.send() ? Done. https://codereview.chromium.org/1375023002/diff/320001/ios/web/web_state/js/c... ios/web/web_state/js/crw_js_post_request_loader_unittest.mm:52: TEST(CRWJSPOSTRequestLoaderTest, installsCallbacks) { On 2015/12/02 17:06:16, eugenebut wrote: > Installing and removing script message router is testing interactions and > implementation detail of this class, hence I don't think it worth testing. > However it up up to you. OK, I'll remove this test for simplicity. https://codereview.chromium.org/1375023002/diff/320001/ios/web/web_state/js/c... ios/web/web_state/js/crw_js_post_request_loader_unittest.mm:52: TEST(CRWJSPOSTRequestLoaderTest, installsCallbacks) { On 2015/12/02 17:06:16, eugenebut wrote: > s/installsCallbacks/InstallsCallbacks Acknowledged. https://codereview.chromium.org/1375023002/diff/320001/ios/web/web_state/js/r... File ios/web/web_state/js/resources/post_request.js (right): https://codereview.chromium.org/1375023002/diff/320001/ios/web/web_state/js/r... ios/web/web_state/js/resources/post_request.js:39: for (var offset = 0; offset < byteCharacters.length; offset += sliceSize){ On 2015/12/02 17:06:16, eugenebut wrote: > Space before |{|, here and everywhere. Done. https://codereview.chromium.org/1375023002/diff/320001/ios/web/web_state/js/r... ios/web/web_state/js/resources/post_request.js:48: var blob = new Blob(byteArrays, {type: contentType}); On 2015/12/02 17:06:16, eugenebut wrote: > return new Blob(byteArrays, {type: contentType}); Done. https://codereview.chromium.org/1375023002/diff/320001/ios/web/web_state/js/r... ios/web/web_state/js/resources/post_request.js:79: contentType)); On 2015/12/02 17:06:16, eugenebut wrote: > This does not look like correct formatting Done. https://codereview.chromium.org/1375023002/diff/320001/ios/web/web_state/ui/c... File ios/web/web_state/ui/crw_wk_web_view_web_controller.mm (right): https://codereview.chromium.org/1375023002/diff/320001/ios/web/web_state/ui/c... ios/web/web_state/ui/crw_wk_web_view_web_controller.mm:156: base::mac::ObjCPropertyReleaser _propertyReleaser_CRWWKWebViewWebController; On 2015/12/02 17:06:16, eugenebut wrote: > Please don't use property releaser here. All memory is managed with > scoped_pointers and new code should be consistent with that. Done. https://codereview.chromium.org/1375023002/diff/320001/ios/web/web_state/ui/c... ios/web/web_state/ui/crw_wk_web_view_web_controller.mm:250: - (void)loadPOSTRequestWithBody:(NSMutableURLRequest*)request; On 2015/12/02 17:06:16, eugenebut wrote: > Word before argument should describe argument. How about just loadPOSTRequest:? Done. https://codereview.chromium.org/1375023002/diff/320001/ios/web/web_state/ui/c... ios/web/web_state/ui/crw_wk_web_view_web_controller.mm:762: if (!self.POSTRequestLoader) { On 2015/12/02 17:06:16, eugenebut wrote: > s/self.POSTRequestLoader/_POSTRequestLoader Done. https://codereview.chromium.org/1375023002/diff/320001/ios/web/web_state/ui/c... ios/web/web_state/ui/crw_wk_web_view_web_controller.mm:770: inWebView:_wkWebView On 2015/12/02 17:06:16, eugenebut wrote: > This is not correct formatting. Di you run git cl format? Done. git cl format doesn't format some things for me, presumably because I am copying changes over between downstream and upstream trees. Sorry for that and thanks for pointing out places where it failed.
https://codereview.chromium.org/1375023002/diff/320001/ios/web/web_state/js/c... File ios/web/web_state/js/crw_js_post_request_loader.mm (right): https://codereview.chromium.org/1375023002/diff/320001/ios/web/web_state/js/c... ios/web/web_state/js/crw_js_post_request_loader.mm:88: // during its execution otherwise. On 2015/12/03 15:43:02, stkhapugin wrote: > On 2015/12/02 17:06:15, eugenebut wrote: > > Which block is deallocated? one which is passed to |setScriptMessageHandler:| > or > > |successBlock|? > > The message handler block (the one where this comment is) is only retained by > the dictionary in message router. Once you removeScriptHandlerForName:webView:, > this block gets deallocated while being executed. Thank you for explanation. I think it will be cleaner if you change ScriptMessageRouter as follows: - (void)tryRemoveScriptMessageHandlerForName:(NSString*)messageName webView:(WKWebView*)webView { NSMapTable* webViewToHandlerMap = [_handlers objectForKey:messageName]; id handler = [webViewToHandlerMap objectForKey:webView]; if (!handler) return; // Extend the lifetime of |handler| so removeScriptMessageHandlerForName: can be called from // inside of |handler|. [[handler retain] autorelease]; if (webViewToHandlerMap.count == 1) { [_handlers removeObjectForKey:messageName]; [_userContentController removeScriptMessageHandlerForName:messageName]; } else { [webViewToHandlerMap removeObjectForKey:webView]; } } https://codereview.chromium.org/1375023002/diff/320001/ios/web/web_state/js/c... ios/web/web_state/js/crw_js_post_request_loader.mm:104: NSError* error = [NSError errorWithDomain:@"http" On 2015/12/03 15:43:02, stkhapugin wrote: > On 2015/12/02 17:06:15, eugenebut wrote: > > Please add a constant for error domain and put it to the header. Also it > should > > be something less generic that @http" > > Since the only error that can happen here is HTTP, should I use NSURLErrorDomain > or something like that to get the default handling for HTTP errors? NSURLErrorDomain is used by System libraries, so we should not use it. Since WebController does not actually use that NSError, I think the easiest way would be replacing NSError with |BOOL success|. What do you think? https://codereview.chromium.org/1375023002/diff/340001/ios/web/web_state/js/c... File ios/web/web_state/js/crw_js_post_request_loader.h (right): https://codereview.chromium.org/1375023002/diff/340001/ios/web/web_state/js/c... ios/web/web_state/js/crw_js_post_request_loader.h:18: // executed. In case of successful request, the passed error is nil. Add: |completionBlock| can not be null. https://codereview.chromium.org/1375023002/diff/340001/ios/web/web_state/js/c... ios/web/web_state/js/crw_js_post_request_loader.h:22: completionBlock:(void (^)(NSError*))completionBlock; Optional NIT s/completionBlock/completionHandler sounds more modern. https://codereview.chromium.org/1375023002/diff/340001/ios/web/web_state/js/c... File ios/web/web_state/js/crw_js_post_request_loader.mm (right): https://codereview.chromium.org/1375023002/diff/340001/ios/web/web_state/js/c... ios/web/web_state/js/crw_js_post_request_loader.mm:7: #import "base/json/string_escape.h" s/import/include Sorry for missing this previously :( https://codereview.chromium.org/1375023002/diff/340001/ios/web/web_state/js/c... ios/web/web_state/js/crw_js_post_request_loader.mm:26: NSString* const kSuccessHandlerName = @"POSTSuccess"; NIT: s/POSTSuccess/POSTSuccessHandler https://codereview.chromium.org/1375023002/diff/340001/ios/web/web_state/js/c... ios/web/web_state/js/crw_js_post_request_loader.mm:52: [_requestScript release]; We don't do manual memory management in Chromium. Please either use scoped_nsobject, so you can reset this ivar in handleMemoryWarning. https://codereview.chromium.org/1375023002/diff/340001/ios/web/web_state/js/c... ios/web/web_state/js/crw_js_post_request_loader.mm:152: NSData* headerData = nil; NIT: Move variable declaration to line 154 and combine with initialization. https://codereview.chromium.org/1375023002/diff/340001/ios/web/web_state/js/c... File ios/web/web_state/js/crw_js_post_request_loader_unittest.mm (right): https://codereview.chromium.org/1375023002/diff/340001/ios/web/web_state/js/c... ios/web/web_state/js/crw_js_post_request_loader_unittest.mm:13: #include "ios/web/test/web_test.h" s/include/import https://codereview.chromium.org/1375023002/diff/340001/ios/web/web_state/js/c... ios/web/web_state/js/crw_js_post_request_loader_unittest.mm:20: class CRWJSPOSTRequestLoaderTest : public web::WebTest {}; typedef web::WebTest CRWJSPOSTRequestLoaderTest; https://codereview.chromium.org/1375023002/diff/340001/ios/web/web_state/js/c... ios/web/web_state/js/crw_js_post_request_loader_unittest.mm:34: /// Tests that the POST request is correctly executed through XMLHttpRequest. NIT: One extra "/" https://codereview.chromium.org/1375023002/diff/340001/ios/web/web_state/js/c... ios/web/web_state/js/crw_js_post_request_loader_unittest.mm:37: CRWJSPOSTRequestLoader* loader = Please use scoped_nsobject instead of autorelease. This is not a rule from Style Guide but it's a strong Chromium convention. https://codereview.chromium.org/1375023002/diff/340001/ios/web/web_state/js/c... ios/web/web_state/js/crw_js_post_request_loader_unittest.mm:39: WKWebView* webView = web::CreateWKWebView(CGRectZero, GetBrowserState()); This is a memory leak, please wrap this variable into scoped_nsobject https://codereview.chromium.org/1375023002/diff/340001/ios/web/web_state/js/c... ios/web/web_state/js/crw_js_post_request_loader_unittest.mm:39: WKWebView* webView = web::CreateWKWebView(CGRectZero, GetBrowserState()); s/webView/web_view This is C++ method and C++ naming should be used. https://codereview.chromium.org/1375023002/diff/340001/ios/web/web_state/js/c... ios/web/web_state/js/crw_js_post_request_loader_unittest.mm:42: CRWWKScriptMessageRouter* messageRouter = [[[CRWWKScriptMessageRouter alloc] Please use scoped_nsobject instead of autorelease https://codereview.chromium.org/1375023002/diff/340001/ios/web/web_state/js/c... ios/web/web_state/js/crw_js_post_request_loader_unittest.mm:47: NSString* JS = [NSString stringWithFormat:@"%@; XMLHttpRequest.prototype.send\ s/JS/js C++ naming Style https://codereview.chromium.org/1375023002/diff/340001/ios/web/web_state/js/c... ios/web/web_state/js/crw_js_post_request_loader_unittest.mm:47: NSString* JS = [NSString stringWithFormat:@"%@; XMLHttpRequest.prototype.send\ Optional NIT: Not sure if this can give you better formatting, but you may try: NSString* js = [blobToBase64StringScript string byAppendingString@"; XMLHttpRequest.prototype.send = function(x) { blobToBase64(x); };"]; https://codereview.chromium.org/1375023002/diff/340001/ios/web/web_state/js/c... ios/web/web_state/js/crw_js_post_request_loader_unittest.mm:58: NSString* POSTBody = @"123"; s/POSTBody/post_body C++ naming style https://codereview.chromium.org/1375023002/diff/340001/ios/web/web_state/ui/c... File ios/web/web_state/ui/crw_wk_web_view_web_controller.mm (left): https://codereview.chromium.org/1375023002/diff/340001/ios/web/web_state/ui/c... ios/web/web_state/ui/crw_wk_web_view_web_controller.mm:1717: if ([self currentBackForwardListItemHolder]->navigation_type() == ditto https://codereview.chromium.org/1375023002/diff/340001/ios/web/web_state/ui/c... File ios/web/web_state/ui/crw_wk_web_view_web_controller.mm (right): https://codereview.chromium.org/1375023002/diff/340001/ios/web/web_state/ui/c... ios/web/web_state/ui/crw_wk_web_view_web_controller.mm:1589: // Make sure that the URL is as expected, and re-check Please revert https://codereview.chromium.org/1375023002/diff/340001/ios/web/web_state/ui/c... ios/web/web_state/ui/crw_wk_web_view_web_controller.mm:1592: _documentURL.GetOrigin() == url.GetOrigin()) { ditto
PTAL. Eugene, thanks for your patience and thorough comments! https://codereview.chromium.org/1375023002/diff/320001/ios/web/web_state/js/c... File ios/web/web_state/js/crw_js_post_request_loader.mm (right): https://codereview.chromium.org/1375023002/diff/320001/ios/web/web_state/js/c... ios/web/web_state/js/crw_js_post_request_loader.mm:88: // during its execution otherwise. On 2015/12/03 16:51:42, eugenebut wrote: > On 2015/12/03 15:43:02, stkhapugin wrote: > > On 2015/12/02 17:06:15, eugenebut wrote: > > > Which block is deallocated? one which is passed to > |setScriptMessageHandler:| > > or > > > |successBlock|? > > > > The message handler block (the one where this comment is) is only retained by > > the dictionary in message router. Once you > removeScriptHandlerForName:webView:, > > this block gets deallocated while being executed. > Thank you for explanation. I think it will be cleaner if you change > ScriptMessageRouter as follows: > > - (void)tryRemoveScriptMessageHandlerForName:(NSString*)messageName > webView:(WKWebView*)webView { > NSMapTable* webViewToHandlerMap = [_handlers objectForKey:messageName]; > id handler = [webViewToHandlerMap objectForKey:webView]; > if (!handler) > return; > // Extend the lifetime of |handler| so removeScriptMessageHandlerForName: can > be called from > // inside of |handler|. > [[handler retain] autorelease]; > if (webViewToHandlerMap.count == 1) { > [_handlers removeObjectForKey:messageName]; > [_userContentController removeScriptMessageHandlerForName:messageName]; > } else { > [webViewToHandlerMap removeObjectForKey:webView]; > } > } Done. https://codereview.chromium.org/1375023002/diff/320001/ios/web/web_state/js/c... ios/web/web_state/js/crw_js_post_request_loader.mm:104: NSError* error = [NSError errorWithDomain:@"http" On 2015/12/03 16:51:42, eugenebut wrote: > On 2015/12/03 15:43:02, stkhapugin wrote: > > On 2015/12/02 17:06:15, eugenebut wrote: > > > Please add a constant for error domain and put it to the header. Also it > > should > > > be something less generic that @http" > > > > Since the only error that can happen here is HTTP, should I use > NSURLErrorDomain > > or something like that to get the default handling for HTTP errors? > NSURLErrorDomain is used by System libraries, so we should not use it. Since > WebController does not actually use that NSError, I think the easiest way would > be replacing NSError with |BOOL success|. What do you think? I agree that it's not necessary to emit an NSError here, but how should a non-200 status be handled then? https://codereview.chromium.org/1375023002/diff/340001/ios/web/web_state/js/c... File ios/web/web_state/js/crw_js_post_request_loader.h (right): https://codereview.chromium.org/1375023002/diff/340001/ios/web/web_state/js/c... ios/web/web_state/js/crw_js_post_request_loader.h:18: // executed. In case of successful request, the passed error is nil. On 2015/12/03 16:51:42, eugenebut wrote: > Add: |completionBlock| can not be null. Done. https://codereview.chromium.org/1375023002/diff/340001/ios/web/web_state/js/c... ios/web/web_state/js/crw_js_post_request_loader.h:22: completionBlock:(void (^)(NSError*))completionBlock; On 2015/12/03 16:51:42, eugenebut wrote: > Optional NIT s/completionBlock/completionHandler sounds more modern. Done. https://codereview.chromium.org/1375023002/diff/340001/ios/web/web_state/js/c... File ios/web/web_state/js/crw_js_post_request_loader.mm (right): https://codereview.chromium.org/1375023002/diff/340001/ios/web/web_state/js/c... ios/web/web_state/js/crw_js_post_request_loader.mm:7: #import "base/json/string_escape.h" On 2015/12/03 16:51:42, eugenebut wrote: > s/import/include > > Sorry for missing this previously :( Done. https://codereview.chromium.org/1375023002/diff/340001/ios/web/web_state/js/c... ios/web/web_state/js/crw_js_post_request_loader.mm:26: NSString* const kSuccessHandlerName = @"POSTSuccess"; On 2015/12/03 16:51:42, eugenebut wrote: > NIT: s/POSTSuccess/POSTSuccessHandler Done. https://codereview.chromium.org/1375023002/diff/340001/ios/web/web_state/js/c... ios/web/web_state/js/crw_js_post_request_loader.mm:52: [_requestScript release]; On 2015/12/03 16:51:42, eugenebut wrote: > We don't do manual memory management in Chromium. Please either use > scoped_nsobject, so you can reset this ivar in handleMemoryWarning. Done. https://codereview.chromium.org/1375023002/diff/340001/ios/web/web_state/js/c... ios/web/web_state/js/crw_js_post_request_loader.mm:152: NSData* headerData = nil; On 2015/12/03 16:51:42, eugenebut wrote: > NIT: Move variable declaration to line 154 and combine with initialization. Done. https://codereview.chromium.org/1375023002/diff/340001/ios/web/web_state/js/c... File ios/web/web_state/js/crw_js_post_request_loader_unittest.mm (right): https://codereview.chromium.org/1375023002/diff/340001/ios/web/web_state/js/c... ios/web/web_state/js/crw_js_post_request_loader_unittest.mm:13: #include "ios/web/test/web_test.h" On 2015/12/03 16:51:42, eugenebut wrote: > s/include/import Done. https://codereview.chromium.org/1375023002/diff/340001/ios/web/web_state/js/c... ios/web/web_state/js/crw_js_post_request_loader_unittest.mm:20: class CRWJSPOSTRequestLoaderTest : public web::WebTest {}; On 2015/12/03 16:51:42, eugenebut wrote: > typedef web::WebTest CRWJSPOSTRequestLoaderTest; Done. https://codereview.chromium.org/1375023002/diff/340001/ios/web/web_state/js/c... ios/web/web_state/js/crw_js_post_request_loader_unittest.mm:34: /// Tests that the POST request is correctly executed through XMLHttpRequest. On 2015/12/03 16:51:42, eugenebut wrote: > NIT: One extra "/" Done. https://codereview.chromium.org/1375023002/diff/340001/ios/web/web_state/js/c... ios/web/web_state/js/crw_js_post_request_loader_unittest.mm:37: CRWJSPOSTRequestLoader* loader = On 2015/12/03 16:51:42, eugenebut wrote: > Please use scoped_nsobject instead of autorelease. This is not a rule from Style > Guide but it's a strong Chromium convention. Done. Should this become a part of style guide? Together with comment in property releaser telling people not to use it in new code? https://codereview.chromium.org/1375023002/diff/340001/ios/web/web_state/js/c... ios/web/web_state/js/crw_js_post_request_loader_unittest.mm:39: WKWebView* webView = web::CreateWKWebView(CGRectZero, GetBrowserState()); On 2015/12/03 16:51:42, eugenebut wrote: > This is a memory leak, please wrap this variable into scoped_nsobject Done. https://codereview.chromium.org/1375023002/diff/340001/ios/web/web_state/js/c... ios/web/web_state/js/crw_js_post_request_loader_unittest.mm:39: WKWebView* webView = web::CreateWKWebView(CGRectZero, GetBrowserState()); On 2015/12/03 16:51:42, eugenebut wrote: > This is a memory leak, please wrap this variable into scoped_nsobject Done. https://codereview.chromium.org/1375023002/diff/340001/ios/web/web_state/js/c... ios/web/web_state/js/crw_js_post_request_loader_unittest.mm:42: CRWWKScriptMessageRouter* messageRouter = [[[CRWWKScriptMessageRouter alloc] On 2015/12/03 16:51:42, eugenebut wrote: > Please use scoped_nsobject instead of autorelease Done. https://codereview.chromium.org/1375023002/diff/340001/ios/web/web_state/js/c... ios/web/web_state/js/crw_js_post_request_loader_unittest.mm:47: NSString* JS = [NSString stringWithFormat:@"%@; XMLHttpRequest.prototype.send\ On 2015/12/03 16:51:42, eugenebut wrote: > s/JS/js > > C++ naming Style Done. https://codereview.chromium.org/1375023002/diff/340001/ios/web/web_state/js/c... ios/web/web_state/js/crw_js_post_request_loader_unittest.mm:47: NSString* JS = [NSString stringWithFormat:@"%@; XMLHttpRequest.prototype.send\ On 2015/12/03 16:51:42, eugenebut wrote: > Optional NIT: Not sure if this can give you better formatting, but you may try: > > NSString* js = [blobToBase64StringScript string byAppendingString@"; > XMLHttpRequest.prototype.send = function(x) { blobToBase64(x); };"]; Done. https://codereview.chromium.org/1375023002/diff/340001/ios/web/web_state/js/c... ios/web/web_state/js/crw_js_post_request_loader_unittest.mm:58: NSString* POSTBody = @"123"; On 2015/12/03 16:51:42, eugenebut wrote: > s/POSTBody/post_body > > C++ naming style Done. https://codereview.chromium.org/1375023002/diff/340001/ios/web/web_state/ui/c... File ios/web/web_state/ui/crw_wk_web_view_web_controller.mm (left): https://codereview.chromium.org/1375023002/diff/340001/ios/web/web_state/ui/c... ios/web/web_state/ui/crw_wk_web_view_web_controller.mm:1717: if ([self currentBackForwardListItemHolder]->navigation_type() == On 2015/12/03 16:51:42, eugenebut wrote: > ditto Done. https://codereview.chromium.org/1375023002/diff/340001/ios/web/web_state/ui/c... File ios/web/web_state/ui/crw_wk_web_view_web_controller.mm (right): https://codereview.chromium.org/1375023002/diff/340001/ios/web/web_state/ui/c... ios/web/web_state/ui/crw_wk_web_view_web_controller.mm:1589: // Make sure that the URL is as expected, and re-check On 2015/12/03 16:51:43, eugenebut wrote: > Please revert Done. https://codereview.chromium.org/1375023002/diff/340001/ios/web/web_state/ui/c... ios/web/web_state/ui/crw_wk_web_view_web_controller.mm:1592: _documentURL.GetOrigin() == url.GetOrigin()) { On 2015/12/03 16:51:42, eugenebut wrote: > ditto Done.
Thank you for patience and making changes. lgtm modulo comments below. https://codereview.chromium.org/1375023002/diff/320001/ios/web/web_state/js/c... File ios/web/web_state/js/crw_js_post_request_loader.mm (right): https://codereview.chromium.org/1375023002/diff/320001/ios/web/web_state/js/c... ios/web/web_state/js/crw_js_post_request_loader.mm:104: NSError* error = [NSError errorWithDomain:@"http" On 2015/12/03 17:51:02, stkhapugin wrote: > On 2015/12/03 16:51:42, eugenebut wrote: > > On 2015/12/03 15:43:02, stkhapugin wrote: > > > On 2015/12/02 17:06:15, eugenebut wrote: > > > > Please add a constant for error domain and put it to the header. Also it > > > should > > > > be something less generic that @http" > > > > > > Since the only error that can happen here is HTTP, should I use > > NSURLErrorDomain > > > or something like that to get the default handling for HTTP errors? > > NSURLErrorDomain is used by System libraries, so we should not use it. Since > > WebController does not actually use that NSError, I think the easiest way > would > > be replacing NSError with |BOOL success|. What do you think? > > I agree that it's not necessary to emit an NSError here, but how should a > non-200 status be handled then? Oh, I'm sorry, I'm blind. WC actually uses that NSError. I guess using NSURLErrorDomain is fine (not ideal, but fine). https://codereview.chromium.org/1375023002/diff/340001/ios/web/web_state/js/c... File ios/web/web_state/js/crw_js_post_request_loader_unittest.mm (right): https://codereview.chromium.org/1375023002/diff/340001/ios/web/web_state/js/c... ios/web/web_state/js/crw_js_post_request_loader_unittest.mm:37: CRWJSPOSTRequestLoader* loader = On 2015/12/03 17:51:03, stkhapugin wrote: > On 2015/12/03 16:51:42, eugenebut wrote: > > Please use scoped_nsobject instead of autorelease. This is not a rule from > Style > > Guide but it's a strong Chromium convention. > > Done. Should this become a part of style guide? Together with comment in > property releaser telling people not to use it in new code? I think this falls into Consistency category. Chromium Style Guide does not cover Objective-C, which is probably fine. https://codereview.chromium.org/1375023002/diff/360001/ios/web/web_state/js/c... File ios/web/web_state/js/crw_js_post_request_loader.mm (right): https://codereview.chromium.org/1375023002/diff/360001/ios/web/web_state/js/c... ios/web/web_state/js/crw_js_post_request_loader.mm:92: extra linebreak https://codereview.chromium.org/1375023002/diff/360001/ios/web/web_state/js/c... ios/web/web_state/js/crw_js_post_request_loader.mm:123: [_requestScript release]; Please do not call release directly, this code will crash. https://codereview.chromium.org/1375023002/diff/360001/ios/web/web_state/js/c... File ios/web/web_state/js/crw_js_post_request_loader_unittest.mm (right): https://codereview.chromium.org/1375023002/diff/360001/ios/web/web_state/js/c... ios/web/web_state/js/crw_js_post_request_loader_unittest.mm:26: NSString* const blobToBase64StringScript = s/blobToBase64StringScript/kBlobToBase64StringScript
The CQ bit was checked by stkhapugin@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1375023002/420001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1375023002/420001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios_rel_device_ninja on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...)
The CQ bit was checked by stkhapugin@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1375023002/440001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1375023002/440001
noyau@chromium.org changed reviewers: + noyau@chromium.org
https://codereview.chromium.org/1375023002/diff/420001/ios/web/web_state/js/c... File ios/web/web_state/js/crw_js_post_request_loader.h (right): https://codereview.chromium.org/1375023002/diff/420001/ios/web/web_state/js/c... ios/web/web_state/js/crw_js_post_request_loader.h:19: // The |completionHandler| must not be null. Neither is the request nor the web view or the messageRouter in the current implementation. https://codereview.chromium.org/1375023002/diff/420001/ios/web/web_state/js/c... File ios/web/web_state/js/crw_js_post_request_loader.mm (right): https://codereview.chromium.org/1375023002/diff/420001/ios/web/web_state/js/c... ios/web/web_state/js/crw_js_post_request_loader.mm:66: } Please move init before dealloc. https://codereview.chromium.org/1375023002/diff/420001/ios/web/web_state/js/c... ios/web/web_state/js/crw_js_post_request_loader.mm:72: return _requestScript.get(); Do you need the get(), I thought C++ was doing it for you there? https://codereview.chromium.org/1375023002/diff/420001/ios/web/web_state/js/c... ios/web/web_state/js/crw_js_post_request_loader.mm:148: // no quotes since JavaScripts takes this parameter as an join lines. https://codereview.chromium.org/1375023002/diff/420001/ios/web/web_state/js/c... File ios/web/web_state/js/crw_js_post_request_loader_unittest.mm (right): https://codereview.chromium.org/1375023002/diff/420001/ios/web/web_state/js/c... ios/web/web_state/js/crw_js_post_request_loader_unittest.mm:74: } This indent is weird, and hard to read. Can you split the block out in a variable instead? https://codereview.chromium.org/1375023002/diff/420001/ios/web/web_state/js/r... File ios/web/web_state/js/resources/post_request.js (right): https://codereview.chromium.org/1375023002/diff/420001/ios/web/web_state/js/r... ios/web/web_state/js/resources/post_request.js:27: contentType) { Formatting is weird here. https://codereview.chromium.org/1375023002/diff/420001/ios/web/web_state/js/r... ios/web/web_state/js/resources/post_request.js:29: /** Indent should be 2 here, not 4 https://codereview.chromium.org/1375023002/diff/420001/ios/web/web_state/js/r... ios/web/web_state/js/resources/post_request.js:83: document.close(); Indent is wrong here.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios_dbg_simulator_ninja on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...)
The CQ bit was checked by stkhapugin@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1375023002/460001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1375023002/460001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios_dbg_simulator_ninja on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...)
The CQ bit was checked by stkhapugin@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1375023002/500001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1375023002/500001
PTAL. Eric, I've done things you requested. Eugene, I addressed your comments. Non-stylistic changes since your last approval are in the unittest, where I now remove the XHRSendHandler so that the web_view is not leaked, plus test now requires WKWV. https://codereview.chromium.org/1375023002/diff/320001/ios/web/web_state/js/c... File ios/web/web_state/js/crw_js_post_request_loader.mm (right): https://codereview.chromium.org/1375023002/diff/320001/ios/web/web_state/js/c... ios/web/web_state/js/crw_js_post_request_loader.mm:104: NSError* error = [NSError errorWithDomain:@"http" On 2015/12/03 18:13:21, eugenebut wrote: > On 2015/12/03 17:51:02, stkhapugin wrote: > > On 2015/12/03 16:51:42, eugenebut wrote: > > > On 2015/12/03 15:43:02, stkhapugin wrote: > > > > On 2015/12/02 17:06:15, eugenebut wrote: > > > > > Please add a constant for error domain and put it to the header. Also it > > > > should > > > > > be something less generic that @http" > > > > > > > > Since the only error that can happen here is HTTP, should I use > > > NSURLErrorDomain > > > > or something like that to get the default handling for HTTP errors? > > > NSURLErrorDomain is used by System libraries, so we should not use it. Since > > > WebController does not actually use that NSError, I think the easiest way > > would > > > be replacing NSError with |BOOL success|. What do you think? > > > > I agree that it's not necessary to emit an NSError here, but how should a > > non-200 status be handled then? > Oh, I'm sorry, I'm blind. WC actually uses that NSError. I guess using > NSURLErrorDomain is fine (not ideal, but fine). Done. https://codereview.chromium.org/1375023002/diff/340001/ios/web/web_state/js/c... File ios/web/web_state/js/crw_js_post_request_loader_unittest.mm (right): https://codereview.chromium.org/1375023002/diff/340001/ios/web/web_state/js/c... ios/web/web_state/js/crw_js_post_request_loader_unittest.mm:37: CRWJSPOSTRequestLoader* loader = On 2015/12/03 18:13:21, eugenebut wrote: > On 2015/12/03 17:51:03, stkhapugin wrote: > > On 2015/12/03 16:51:42, eugenebut wrote: > > > Please use scoped_nsobject instead of autorelease. This is not a rule from > > Style > > > Guide but it's a strong Chromium convention. > > > > Done. Should this become a part of style guide? Together with comment in > > property releaser telling people not to use it in new code? > I think this falls into Consistency category. Chromium Style Guide does not > cover Objective-C, which is probably fine. Acknowledged. https://codereview.chromium.org/1375023002/diff/360001/ios/web/web_state/js/c... File ios/web/web_state/js/crw_js_post_request_loader.mm (right): https://codereview.chromium.org/1375023002/diff/360001/ios/web/web_state/js/c... ios/web/web_state/js/crw_js_post_request_loader.mm:92: On 2015/12/03 18:13:21, eugenebut wrote: > extra linebreak Done. https://codereview.chromium.org/1375023002/diff/360001/ios/web/web_state/js/c... ios/web/web_state/js/crw_js_post_request_loader.mm:123: [_requestScript release]; On 2015/12/03 18:13:21, eugenebut wrote: > Please do not call release directly, this code will crash. Done. https://codereview.chromium.org/1375023002/diff/360001/ios/web/web_state/js/c... File ios/web/web_state/js/crw_js_post_request_loader_unittest.mm (right): https://codereview.chromium.org/1375023002/diff/360001/ios/web/web_state/js/c... ios/web/web_state/js/crw_js_post_request_loader_unittest.mm:26: NSString* const blobToBase64StringScript = On 2015/12/03 18:13:21, eugenebut wrote: > s/blobToBase64StringScript/kBlobToBase64StringScript Done. https://codereview.chromium.org/1375023002/diff/420001/ios/web/web_state/js/c... File ios/web/web_state/js/crw_js_post_request_loader.h (right): https://codereview.chromium.org/1375023002/diff/420001/ios/web/web_state/js/c... ios/web/web_state/js/crw_js_post_request_loader.h:19: // The |completionHandler| must not be null. On 2015/12/04 12:19:18, noyau wrote: > Neither is the request nor the web view or the messageRouter in the current > implementation. Done. https://codereview.chromium.org/1375023002/diff/420001/ios/web/web_state/js/c... File ios/web/web_state/js/crw_js_post_request_loader.mm (right): https://codereview.chromium.org/1375023002/diff/420001/ios/web/web_state/js/c... ios/web/web_state/js/crw_js_post_request_loader.mm:66: } On 2015/12/04 12:19:18, noyau wrote: > Please move init before dealloc. Done. Is it because "init... methods should be grouped together, followed by other NSObject methods."? https://codereview.chromium.org/1375023002/diff/420001/ios/web/web_state/js/c... ios/web/web_state/js/crw_js_post_request_loader.mm:72: return _requestScript.get(); On 2015/12/04 12:19:18, noyau wrote: > Do you need the get(), I thought C++ was doing it for you there? Done. https://codereview.chromium.org/1375023002/diff/420001/ios/web/web_state/js/c... ios/web/web_state/js/crw_js_post_request_loader.mm:148: // no quotes since JavaScripts takes this parameter as an On 2015/12/04 12:19:18, noyau wrote: > join lines. Done. https://codereview.chromium.org/1375023002/diff/420001/ios/web/web_state/js/c... File ios/web/web_state/js/crw_js_post_request_loader_unittest.mm (right): https://codereview.chromium.org/1375023002/diff/420001/ios/web/web_state/js/c... ios/web/web_state/js/crw_js_post_request_loader_unittest.mm:74: } On 2015/12/04 12:19:18, noyau wrote: > This indent is weird, and hard to read. Can you split the block out in a > variable instead? Done. This is why block should be the last parameter of the method that takes a block. That way the indentation like that is perfectly readable. https://codereview.chromium.org/1375023002/diff/420001/ios/web/web_state/js/r... File ios/web/web_state/js/resources/post_request.js (right): https://codereview.chromium.org/1375023002/diff/420001/ios/web/web_state/js/r... ios/web/web_state/js/resources/post_request.js:27: contentType) { On 2015/12/04 12:19:18, noyau wrote: > Formatting is weird here. Reformatted as per JS style guide: // Arguments start on a new line, indented four spaces. Preferred when the // arguments don't fit on the same line with the function name (or the keyword // "function") but fit entirely on the second line. Works with very long // function names, survives renaming without reindenting, low on space. https://codereview.chromium.org/1375023002/diff/420001/ios/web/web_state/js/r... ios/web/web_state/js/resources/post_request.js:29: /** On 2015/12/04 12:19:18, noyau wrote: > Indent should be 2 here, not 4 Done. https://codereview.chromium.org/1375023002/diff/420001/ios/web/web_state/js/r... ios/web/web_state/js/resources/post_request.js:83: document.close(); On 2015/12/04 12:19:18, noyau wrote: > Indent is wrong here. Done.
lgtm https://codereview.chromium.org/1375023002/diff/420001/ios/web/web_state/js/c... File ios/web/web_state/js/crw_js_post_request_loader.mm (right): https://codereview.chromium.org/1375023002/diff/420001/ios/web/web_state/js/c... ios/web/web_state/js/crw_js_post_request_loader.mm:66: } On 2015/12/04 16:28:10, stkhapugin wrote: > On 2015/12/04 12:19:18, noyau wrote: > > Please move init before dealloc. > > Done. Is it because "init... methods should be grouped together, followed by > other NSObject methods."? yup
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by stkhapugin@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from eugenebut@chromium.org Link to the patchset: https://codereview.chromium.org/1375023002/#ps500001 (title: "Addressed Noyau's comments")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1375023002/500001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1375023002/500001
Message was sent while issue was closed.
Description was changed from ========== Adds support for POST request with bodies on WKWebView. These requests are now handled through a javascript hack to work around this webkit bug: https://bugs.webkit.org/show_bug.cgi?id=145410. BUG=489692 ========== to ========== Adds support for POST request with bodies on WKWebView. These requests are now handled through a javascript hack to work around this webkit bug: https://bugs.webkit.org/show_bug.cgi?id=145410. BUG=489692 ==========
Message was sent while issue was closed.
Committed patchset #26 (id:500001)
Message was sent while issue was closed.
Description was changed from ========== Adds support for POST request with bodies on WKWebView. These requests are now handled through a javascript hack to work around this webkit bug: https://bugs.webkit.org/show_bug.cgi?id=145410. BUG=489692 ========== to ========== Adds support for POST request with bodies on WKWebView. These requests are now handled through a javascript hack to work around this webkit bug: https://bugs.webkit.org/show_bug.cgi?id=145410. BUG=489692 Committed: https://crrev.com/8d13d52f53c03f6b78ab8fbf7148155780063244 Cr-Commit-Position: refs/heads/master@{#363256} ==========
Message was sent while issue was closed.
Patchset 26 (id:??) landed as https://crrev.com/8d13d52f53c03f6b78ab8fbf7148155780063244 Cr-Commit-Position: refs/heads/master@{#363256} |
