Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
207553008:
Surface button for loading stale cache copy on net error page. (Closed)
|
Created:
6 years, 9 months ago by Randy Smith (Not in Mondays) Modified:
6 years, 8 months ago Reviewers:
sky, Elly Fong-Jones, Deprecated (see juliatuttle), Alexei Svitkine (slow), Nico, mmenke, jar (doing other things) CC:
chromium-reviews, davidben Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionSurface button for loading stale cache copy on net error page.
In cases in which a network error occurs, and we have a stale copy of the
top level resource that was attempting to be loaded, show a button
on the network error page that the user can use to load that stale copy.
BUG=329620
R=mmenke@chromium.org
R=ellyjones@chromium.org
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=264696
Patch Set 1 #Patch Set 2 : Shift text to match behavior doc. #
Total comments: 10
Patch Set 3 : Incorporated Matt's comments. #Patch Set 4 : Sync'd to r261035 #Patch Set 5 : Add var to variables in swapButtonOrder function. #Patch Set 6 : Incorporated Matt's comments from c#5. #Patch Set 7 : Added hover text. #Patch Set 8 : Added UMA (big patch). #Patch Set 9 : Minor tweaks from self-review. #
Total comments: 37
Patch Set 10 : Incorporated comments. #
Total comments: 43
Patch Set 11 : Incoporated all comments. #Patch Set 12 : Make new functionality work transparently on IOS, which doesn't have gin bindings. #
Total comments: 24
Patch Set 13 : Incorporated Matt's comments on PS12. #Patch Set 14 : Sync'd to r264192. #
Total comments: 8
Patch Set 15 : Incorporated comments. #Patch Set 16 : Include unsaved emacs buffers :-J. #Patch Set 17 : Fix compiler error. #
Total comments: 2
Patch Set 18 : Fix incorrect spelling of iOS. #
Total comments: 22
Messages
Total messages: 47 (0 generated)
https://codereview.chromium.org/207553008/diff/20001/chrome/common/localized_... File chrome/common/localized_error.cc (right): https://codereview.chromium.org/207553008/diff/20001/chrome/common/localized_... chrome/common/localized_error.cc:672: if (stale_copy_in_cache) { I suggest putting this below use_default_suggestions. Suggesting loading from Google's cache and a separate suggestion to load from the local cache just seems very weird to me. https://codereview.chromium.org/207553008/diff/20001/chrome/common/localized_... chrome/common/localized_error.cc:675: "msg", l10n_util::GetStringUTF16(IDS_ERRORPAGES_BUTTON_STALE)); This subdirectory isn't needed. Instead you can just set staleCopyInCache(Or whatever) to be this string directly, and use the attibutes on the button: jscontent="staleCopyInCache" jsdisplay="staleCopyInCache" https://codereview.chromium.org/207553008/diff/20001/chrome/common/localized_... chrome/common/localized_error.cc:676: error_strings->Set("staleCopyInCache", stale_load_button); Getting the "IDS_ERRORPAGES_BUTTON_STALE" string, storing it in the "stale_load_button" value, and then putting it in the "staleCopyInCache" Javascript variable seems a little weird (Admittedly, this is the case with the reload button as well). I suggest renaming them to be more consistent. Just to throw out one suggestion: IDS_ERRORPAGES_BUTTON_LOAD_STALE / stale_load_button / staleLoadButton
Oops...sorry if I commented before this was ready. I knew I had a review to do for you, and just saw this one in my queue.
On 2014/03/26 15:18:12, mmenke wrote: > Oops...sorry if I commented before this was ready. I knew I had a review to do > for you, and just saw this one in my queue. No worries--I think the comments are useful anyway. I was going to specifically ask about the positioning with regard to use_default_suggestions, but you beat me too it :-}. There may not be activity on this CL for a bit, though, as I hash through the UI issues (review, finch, etc.)
Comments incorporated. Not currently requesting a re-review, as this CL is likely to change as part of the UI review process. https://codereview.chromium.org/207553008/diff/20001/chrome/common/localized_... File chrome/common/localized_error.cc (right): https://codereview.chromium.org/207553008/diff/20001/chrome/common/localized_... chrome/common/localized_error.cc:672: if (stale_copy_in_cache) { On 2014/03/26 15:06:14, mmenke wrote: > I suggest putting this below use_default_suggestions. Suggesting loading from > Google's cache and a separate suggestion to load from the local cache just seems > very weird to me. This currently happens only in the link doctor case? (That's what it looks like to me from reading the code, but I want to check.) I'm ok but a little uncomfortable with this--the problem is that there's no reason in this particular section of code why we'd make loading a stale copy dependent on whether or not we're doing non-default suggestions; there isn't any reason why the non-default suggestions would have anything to do with a cache. But it's a pretty minor point, and I agree with the goal. https://codereview.chromium.org/207553008/diff/20001/chrome/common/localized_... chrome/common/localized_error.cc:675: "msg", l10n_util::GetStringUTF16(IDS_ERRORPAGES_BUTTON_STALE)); On 2014/03/26 15:06:14, mmenke wrote: > This subdirectory isn't needed. Instead you can just set staleCopyInCache(Or > whatever) to be this string directly, and use the attibutes on the button: > > jscontent="staleCopyInCache" jsdisplay="staleCopyInCache" Done (now that I understand what's going on :-}). https://codereview.chromium.org/207553008/diff/20001/chrome/common/localized_... chrome/common/localized_error.cc:676: error_strings->Set("staleCopyInCache", stale_load_button); On 2014/03/26 15:06:14, mmenke wrote: > Getting the "IDS_ERRORPAGES_BUTTON_STALE" string, storing it in the > "stale_load_button" value, and then putting it in the "staleCopyInCache" > Javascript variable seems a little weird (Admittedly, this is the case with the > reload button as well). > > I suggest renaming them to be more consistent. Just to throw out one > suggestion: > > IDS_ERRORPAGES_BUTTON_LOAD_STALE / stale_load_button / staleLoadButton Done. Let me know if you'd like me to regularize the reload button.
Quick responses to your responses. https://codereview.chromium.org/207553008/diff/20001/chrome/common/localized_... File chrome/common/localized_error.cc (right): https://codereview.chromium.org/207553008/diff/20001/chrome/common/localized_... chrome/common/localized_error.cc:672: if (stale_copy_in_cache) { On 2014/03/26 20:40:12, rdsmith wrote: > On 2014/03/26 15:06:14, mmenke wrote: > > I suggest putting this below use_default_suggestions. Suggesting loading from > > Google's cache and a separate suggestion to load from the local cache just > seems > > very weird to me. > > This currently happens only in the link doctor case? (That's what it looks like > to me from reading the code, but I want to check.) I'm ok but a little > uncomfortable with this--the problem is that there's no reason in this > particular section of code why we'd make loading a stale copy dependent on > whether or not we're doing non-default suggestions; there isn't any reason why > the non-default suggestions would have anything to do with a cache. But it's a > pretty minor point, and I agree with the goal. Yes, this is only in the Link Doctor case. This is actually a "default suggestion", as I see it (Though renaming the bool to suggest_load_from_cache would change that opinion), but my real concern is just the two cache links, not what is and is not "default". If you want to just hide the button when Google suggests we check it's cache, though that gets a bit hairy (And I've seen Link Doctor lie about what's in Google's cache). https://codereview.chromium.org/207553008/diff/20001/chrome/common/localized_... chrome/common/localized_error.cc:676: error_strings->Set("staleCopyInCache", stale_load_button); On 2014/03/26 20:40:12, rdsmith wrote: > On 2014/03/26 15:06:14, mmenke wrote: > > Getting the "IDS_ERRORPAGES_BUTTON_STALE" string, storing it in the > > "stale_load_button" value, and then putting it in the "staleCopyInCache" > > Javascript variable seems a little weird (Admittedly, this is the case with > the > > reload button as well). > > > > I suggest renaming them to be more consistent. Just to throw out one > > suggestion: > > > > IDS_ERRORPAGES_BUTTON_LOAD_STALE / stale_load_button / staleLoadButton > > Done. Let me know if you'd like me to regularize the reload button. I think it'd be great if you went ahead and cleaned that up while you're here.
Is there any sort of in-UI explanation of what a 'stale local copy' is? Maybe some kind of descriptive text on the page? I'm worried people won't know what the button does.
On 2014/03/27 15:15:01, Elly Jones wrote: > Is there any sort of in-UI explanation of what a 'stale local copy' is? Maybe > some kind of descriptive text on the page? I'm worried people won't know what > the button does. Sorry; I somehow missed that you had posted this. Yes, I'm concerned about the same issue; I was going to raise it in the UI review, but from what Sid has said, we're expected to have a working UI to critique. Given that, what I think I'd like to do is create a hover text that describes what's going on; I'm already worried about the excess text on the button. I'll look into doing that.
Matt, Elly: Willing to give this a real review? I think I'm ready to actually push this through. I'll figure out the stampers after you guys give the OK. https://codereview.chromium.org/207553008/diff/20001/chrome/common/localized_... File chrome/common/localized_error.cc (right): https://codereview.chromium.org/207553008/diff/20001/chrome/common/localized_... chrome/common/localized_error.cc:672: if (stale_copy_in_cache) { On 2014/03/27 14:28:28, mmenke wrote: > On 2014/03/26 20:40:12, rdsmith wrote: > > On 2014/03/26 15:06:14, mmenke wrote: > > > I suggest putting this below use_default_suggestions. Suggesting loading > from > > > Google's cache and a separate suggestion to load from the local cache just > > seems > > > very weird to me. > > > > This currently happens only in the link doctor case? (That's what it looks > like > > to me from reading the code, but I want to check.) I'm ok but a little > > uncomfortable with this--the problem is that there's no reason in this > > particular section of code why we'd make loading a stale copy dependent on > > whether or not we're doing non-default suggestions; there isn't any reason why > > the non-default suggestions would have anything to do with a cache. But it's > a > > pretty minor point, and I agree with the goal. > > Yes, this is only in the Link Doctor case. This is actually a "default > suggestion", as I see it (Though renaming the bool to suggest_load_from_cache > would change that opinion), but my real concern is just the two cache links, not > what is and is not "default". If you want to just hide the button when Google > suggests we check it's cache, though that gets a bit hairy (And I've seen Link > Doctor lie about what's in Google's cache). Yeah, I thought about what it would take to do it the "right" way, and decided against. So yes, load stale cache is a default suggestion; I'm good with that :-}. https://codereview.chromium.org/207553008/diff/20001/chrome/common/localized_... chrome/common/localized_error.cc:676: error_strings->Set("staleCopyInCache", stale_load_button); On 2014/03/27 14:28:28, mmenke wrote: > On 2014/03/26 20:40:12, rdsmith wrote: > > On 2014/03/26 15:06:14, mmenke wrote: > > > Getting the "IDS_ERRORPAGES_BUTTON_STALE" string, storing it in the > > > "stale_load_button" value, and then putting it in the "staleCopyInCache" > > > Javascript variable seems a little weird (Admittedly, this is the case with > > the > > > reload button as well). > > > > > > I suggest renaming them to be more consistent. Just to throw out one > > > suggestion: > > > > > > IDS_ERRORPAGES_BUTTON_LOAD_STALE / stale_load_button / staleLoadButton > > > > Done. Let me know if you'd like me to regularize the reload button. > > I think it'd be great if you went ahead and cleaned that up while you're here. I believe I've done this (just changing the templateData name from "reload" to "reloadButton"); let me know if you think I've missed something.
On 2014/04/02 19:20:58, rdsmith wrote: > Matt, Elly: Willing to give this a real review? I think I'm ready to actually > push this through. I'll figure out the stampers after you guys give the OK. Actually, no, I still need to do UMA. Reviews welcome but not solicited; sorry for the false alarm. > > https://codereview.chromium.org/207553008/diff/20001/chrome/common/localized_... > File chrome/common/localized_error.cc (right): > > https://codereview.chromium.org/207553008/diff/20001/chrome/common/localized_... > chrome/common/localized_error.cc:672: if (stale_copy_in_cache) { > On 2014/03/27 14:28:28, mmenke wrote: > > On 2014/03/26 20:40:12, rdsmith wrote: > > > On 2014/03/26 15:06:14, mmenke wrote: > > > > I suggest putting this below use_default_suggestions. Suggesting loading > > from > > > > Google's cache and a separate suggestion to load from the local cache just > > > seems > > > > very weird to me. > > > > > > This currently happens only in the link doctor case? (That's what it looks > > like > > > to me from reading the code, but I want to check.) I'm ok but a little > > > uncomfortable with this--the problem is that there's no reason in this > > > particular section of code why we'd make loading a stale copy dependent on > > > whether or not we're doing non-default suggestions; there isn't any reason > why > > > the non-default suggestions would have anything to do with a cache. But > it's > > a > > > pretty minor point, and I agree with the goal. > > > > Yes, this is only in the Link Doctor case. This is actually a "default > > suggestion", as I see it (Though renaming the bool to suggest_load_from_cache > > would change that opinion), but my real concern is just the two cache links, > not > > what is and is not "default". If you want to just hide the button when Google > > suggests we check it's cache, though that gets a bit hairy (And I've seen Link > > Doctor lie about what's in Google's cache). > > Yeah, I thought about what it would take to do it the "right" way, and decided > against. So yes, load stale cache is a default suggestion; I'm good with that > :-}. > > https://codereview.chromium.org/207553008/diff/20001/chrome/common/localized_... > chrome/common/localized_error.cc:676: error_strings->Set("staleCopyInCache", > stale_load_button); > On 2014/03/27 14:28:28, mmenke wrote: > > On 2014/03/26 20:40:12, rdsmith wrote: > > > On 2014/03/26 15:06:14, mmenke wrote: > > > > Getting the "IDS_ERRORPAGES_BUTTON_STALE" string, storing it in the > > > > "stale_load_button" value, and then putting it in the "staleCopyInCache" > > > > Javascript variable seems a little weird (Admittedly, this is the case > with > > > the > > > > reload button as well). > > > > > > > > I suggest renaming them to be more consistent. Just to throw out one > > > > suggestion: > > > > > > > > IDS_ERRORPAGES_BUTTON_LOAD_STALE / stale_load_button / staleLoadButton > > > > > > Done. Let me know if you'd like me to regularize the reload button. > > > > I think it'd be great if you went ahead and cleaned that up while you're here. > > I believe I've done this (just changing the templateData name from "reload" to > "reloadButton"); let me know if you think I've missed something.
Ok, I think I'm ready for review of this CL. Adding ttuttle@ at his request for error page changes. Elly, Matt: If you're willing, I'd like you both to look at the whole thing; Elly for high level, Matt for details (though I'm interested in feedback on either of those levels from either of you), Thomas for error page architecture hacking (specifically NetErrorTabHelper changes, but anything else that catches your eye). Specific things I'm interested in comments on: * I re-plumbed the functionality of the reload button to be in NetErrorHelperCore, since I needed to do that for the load stale functionality and I wanted to keep them compatible. * The amount of hassle to get the button presses into NetErrorHelperCore is annoying to me, but I thought it was better to do that than to implement the functionality elsewhere and have UMA scattered through the code. But I'm not completely happy, so interested in feedback. * I co-opted the NetErrorTabHelper to track browser level reload events because it already tracks whether we've got an error page or not. (This is the area Thomas was interested in reviewing.) * I thought about trying to create the NetErrorHelper interface for the buttons in the same way as the NetErrorHelperCore interface, i.e. one function with an enum argument. But I wasn't sure where to put them enum such that they both could see it, so I did it this way. * I'm thinking about merging ErrorPageHelperFunctions and NetErrorHelper (i.e. doing the gin bindings directly on NetErrorHelper). If I do, that won't be in this CL, but if any of you have reactions to that idea, I'd like to hear them.
This looks pretty good...As an added bonus, I'll need to track clicks on the Link Doctor page, and can just reuse this mechanism, so the requests can be held on by Javascript objects and won't go away after the navigation commits. One thing to be aware of: The Link Doctor page currently is not tracking clicks properly, and we're looking into how to fix that for M35. The revert is not clean, and fixing it in M35 without modifying strings in all cases, including opening links in new tabs, is complicated. So we'll see what happens there. https://codereview.chromium.org/207553008/diff/200001/chrome/browser/ui/brows... File chrome/browser/ui/browser_commands.cc (right): https://codereview.chromium.org/207553008/diff/200001/chrome/browser/ui/brows... chrome/browser/ui/browser_commands.cc:249: net_error_tab_helper->NotifyBrowserReloading(); We can't deduce this from the information passed to WebContentsObserver::DidStartNavigationToPendingEntry? https://codereview.chromium.org/207553008/diff/200001/chrome/renderer/net/err... File chrome/renderer/net/error_page_helper_functions.cc (right): https://codereview.chromium.org/207553008/diff/200001/chrome/renderer/net/err... chrome/renderer/net/error_page_helper_functions.cc:5: #include "error_page_helper_functions.h" Use full path. https://codereview.chromium.org/207553008/diff/200001/chrome/renderer/net/err... chrome/renderer/net/error_page_helper_functions.cc:7: #include "base/strings/string_util.h" Is this needed? Or just base/strings/string_piece.h? https://codereview.chromium.org/207553008/diff/200001/chrome/renderer/net/err... chrome/renderer/net/error_page_helper_functions.cc:13: #include "third_party/WebKit/public/web/WebDataSource.h" Are either of these two needed? https://codereview.chromium.org/207553008/diff/200001/chrome/renderer/net/err... chrome/renderer/net/error_page_helper_functions.cc:27: if (context.IsEmpty()) Can this happen? https://codereview.chromium.org/207553008/diff/200001/chrome/renderer/net/err... File chrome/renderer/net/error_page_helper_functions.h (right): https://codereview.chromium.org/207553008/diff/200001/chrome/renderer/net/err... chrome/renderer/net/error_page_helper_functions.h:25: class ErrorPageHelperFunctions Think NetErrorHelperFunctions to be more consistent or [Net]ErrorPageController - a couple classes like this use controller, and NetErrorPageFunctionsCalledFromJavascript seems a bit too long. :) https://codereview.chromium.org/207553008/diff/200001/chrome/renderer/net/err... chrome/renderer/net/error_page_helper_functions.h:25: class ErrorPageHelperFunctions Think this is worth a comment (Error pages functions called from Javascript) https://codereview.chromium.org/207553008/diff/200001/chrome/renderer/net/err... chrome/renderer/net/error_page_helper_functions.h:58: content::RenderFrame* render_frame_; Don't need our own render_frame_: render_frame() is NULLed out before destruct. https://codereview.chromium.org/207553008/diff/200001/chrome/renderer/net/err... chrome/renderer/net/error_page_helper_functions.h:60: const GURL page_url_; Doesn't seem to be needed. https://codereview.chromium.org/207553008/diff/200001/chrome/renderer/net/err... chrome/renderer/net/error_page_helper_functions.h:62: bool render_frame_destroyed_; Not used. https://codereview.chromium.org/207553008/diff/200001/chrome/renderer/net/net... File chrome/renderer/net/net_error_helper_core.cc (right): https://codereview.chromium.org/207553008/diff/200001/chrome/renderer/net/net... chrome/renderer/net/net_error_helper_core.cc:376: navigation_from_button_ = NO_BUTTON; Problem: Navigate to foo.com/, get an error. Click reload. Reload takes a while. Click back button. We end up at another error page, but with the same RenderView, resulting in recording the wrong thing (Some omnibox loads may also reuse the RenderView). https://codereview.chromium.org/207553008/diff/200001/chrome/renderer/net/net... chrome/renderer/net/net_error_helper_core.cc:412: chrome_common_net::LOAD_STALE_BUTTON_SHOWN_EVENT); nit: Use braces on multi-line if's (x2). https://codereview.chromium.org/207553008/diff/200001/chrome/renderer/net/net... chrome/renderer/net/net_error_helper_core.cc:506: if (pending_error_page_info_.get()) { nit: .get() not needed. https://codereview.chromium.org/207553008/diff/200001/chrome/renderer/net/net... chrome/renderer/net/net_error_helper_core.cc:557: // changed by a DNS error update. nit: Don't use we in comments. https://codereview.chromium.org/207553008/diff/200001/chrome/renderer/resourc... File chrome/renderer/resources/neterror.html (right): https://codereview.chromium.org/207553008/diff/200001/chrome/renderer/resourc... chrome/renderer/resources/neterror.html:25: jsselect="reloadButton" jsvalues=".url:reloadUrl" jsvalues=".url:reloadUrl" is no longer needed (Can get rid of the field entirely, actually). Wonder if we show the cache reload confirmation button if we reload this way...Probably not, but worth checking.
On 2014/04/08 22:25:45, rdsmith wrote: > Ok, I think I'm ready for review of this CL. Adding ttuttle@ at his request for > error page changes. Elly, Matt: If you're willing, I'd like you both to look at > the whole thing; Elly for high level, Matt for details (though I'm interested in > feedback on either of those levels from either of you), Thomas for error page > architecture hacking (specifically NetErrorTabHelper changes, but anything else > that catches your eye). > > Specific things I'm interested in comments on: > * I re-plumbed the functionality of the reload button to be in > NetErrorHelperCore, since I needed to do that for the load stale functionality > and I wanted to keep them compatible. > > * The amount of hassle to get the button presses into NetErrorHelperCore is > annoying to me, but I thought it was better to do that than to implement the > functionality elsewhere and have UMA scattered through the code. But I'm not > completely happy, so interested in feedback. > > * I co-opted the NetErrorTabHelper to track browser level reload events because > it already tracks whether we've got an error page or not. (This is the area > Thomas was interested in reviewing.) > > * I thought about trying to create the NetErrorHelper interface for the buttons > in the same way as the NetErrorHelperCore interface, i.e. one function with an > enum argument. But I wasn't sure where to put them enum such that they both > could see it, so I did it this way. > > * I'm thinking about merging ErrorPageHelperFunctions and NetErrorHelper (i.e. > doing the gin bindings directly on NetErrorHelper). If I do, that won't be in > this CL, but if any of you have reactions to that idea, I'd like to hear them. One other general question, for reviewers to comment on if they have an opinion: Currently, if the user clicks "reload" (browser or page) and we come back to the error page, we get a bump in count for pages and buttons shown. I could also piggy-back on Elly's change to suppress error page loads if that load would occur over an old error page, which would keep the statistics a bit more in line with what I think the user is experiencing (if they hit reload and get an error page, I think they experience it as the same error page). However, if the reload would change the error failure, that doesn't get updated on the page, which is a functional differences, albeit a minor one. So I think my inclination is to accept the stats being off, but I'm interested in other perspectives. Long-term, the right thing is probably to inject the new error information into the JS in both reload and auto-reload cases, but I suspect that's more complex a change than we want to take on now.
On 2014/04/09 16:32:49, rdsmith wrote: > On 2014/04/08 22:25:45, rdsmith wrote: > > Ok, I think I'm ready for review of this CL. Adding ttuttle@ at his request > for > > error page changes. Elly, Matt: If you're willing, I'd like you both to look > at > > the whole thing; Elly for high level, Matt for details (though I'm interested > in > > feedback on either of those levels from either of you), Thomas for error page > > architecture hacking (specifically NetErrorTabHelper changes, but anything > else > > that catches your eye). > > > > Specific things I'm interested in comments on: > > * I re-plumbed the functionality of the reload button to be in > > NetErrorHelperCore, since I needed to do that for the load stale functionality > > and I wanted to keep them compatible. > > > > * The amount of hassle to get the button presses into NetErrorHelperCore is > > annoying to me, but I thought it was better to do that than to implement the > > functionality elsewhere and have UMA scattered through the code. But I'm not > > completely happy, so interested in feedback. > > > > * I co-opted the NetErrorTabHelper to track browser level reload events > because > > it already tracks whether we've got an error page or not. (This is the area > > Thomas was interested in reviewing.) > > > > * I thought about trying to create the NetErrorHelper interface for the > buttons > > in the same way as the NetErrorHelperCore interface, i.e. one function with an > > enum argument. But I wasn't sure where to put them enum such that they both > > could see it, so I did it this way. > > > > * I'm thinking about merging ErrorPageHelperFunctions and NetErrorHelper (i.e. > > doing the gin bindings directly on NetErrorHelper). If I do, that won't be in > > this CL, but if any of you have reactions to that idea, I'd like to hear them. > > One other general question, for reviewers to comment on if they have an opinion: > Currently, if the user clicks "reload" (browser or page) and we come back to the > error page, we get a bump in count for pages and buttons shown. I could also > piggy-back on Elly's change to suppress error page loads if that load would > occur over an old error page, which would keep the statistics a bit more in line > with what I think the user is experiencing (if they hit reload and get an error > page, I think they experience it as the same error page). However, if the > reload would change the error failure, that doesn't get updated on the page, > which is a functional differences, albeit a minor one. So I think my > inclination is to accept the stats being off, but I'm interested in other > perspectives. > > Long-term, the right thing is probably to inject the new error information into > the JS in both reload and auto-reload cases, but I suspect that's more complex a > change than we want to take on now. I agree - on user initiated loads, I think keeping an incorrect error code is a bad idea (Don't like it on automatic reloads either, but I can live with it)
design lgtm. https://codereview.chromium.org/207553008/diff/200001/chrome/renderer/net/err... File chrome/renderer/net/error_page_helper_functions.cc (right): https://codereview.chromium.org/207553008/diff/200001/chrome/renderer/net/err... chrome/renderer/net/error_page_helper_functions.cc:39: if (!render_frame_) can this object outlive the render_frame_?
On 2014/04/09 17:49:45, mmenke wrote: > On 2014/04/09 16:32:49, rdsmith wrote: > > On 2014/04/08 22:25:45, rdsmith wrote: > > > Ok, I think I'm ready for review of this CL. Adding ttuttle@ at his request > > for > > > error page changes. Elly, Matt: If you're willing, I'd like you both to > look > > at > > > the whole thing; Elly for high level, Matt for details (though I'm > interested > > in > > > feedback on either of those levels from either of you), Thomas for error > page > > > architecture hacking (specifically NetErrorTabHelper changes, but anything > > else > > > that catches your eye). > > > > > > Specific things I'm interested in comments on: > > > * I re-plumbed the functionality of the reload button to be in > > > NetErrorHelperCore, since I needed to do that for the load stale > functionality > > > and I wanted to keep them compatible. > > > > > > * The amount of hassle to get the button presses into NetErrorHelperCore is > > > annoying to me, but I thought it was better to do that than to implement the > > > functionality elsewhere and have UMA scattered through the code. But I'm > not > > > completely happy, so interested in feedback. > > > > > > * I co-opted the NetErrorTabHelper to track browser level reload events > > because > > > it already tracks whether we've got an error page or not. (This is the area > > > Thomas was interested in reviewing.) > > > > > > * I thought about trying to create the NetErrorHelper interface for the > > buttons > > > in the same way as the NetErrorHelperCore interface, i.e. one function with > an > > > enum argument. But I wasn't sure where to put them enum such that they both > > > could see it, so I did it this way. > > > > > > * I'm thinking about merging ErrorPageHelperFunctions and NetErrorHelper > (i.e. > > > doing the gin bindings directly on NetErrorHelper). If I do, that won't be > in > > > this CL, but if any of you have reactions to that idea, I'd like to hear > them. > > > > One other general question, for reviewers to comment on if they have an > opinion: > > Currently, if the user clicks "reload" (browser or page) and we come back to > the > > error page, we get a bump in count for pages and buttons shown. I could also > > piggy-back on Elly's change to suppress error page loads if that load would > > occur over an old error page, which would keep the statistics a bit more in > line > > with what I think the user is experiencing (if they hit reload and get an > error > > page, I think they experience it as the same error page). However, if the > > reload would change the error failure, that doesn't get updated on the page, > > which is a functional differences, albeit a minor one. So I think my > > inclination is to accept the stats being off, but I'm interested in other > > perspectives. > > > > Long-term, the right thing is probably to inject the new error information > into > > the JS in both reload and auto-reload cases, but I suspect that's more complex > a > > change than we want to take on now. > > I agree - on user initiated loads, I think keeping an incorrect error code is a > bad idea (Don't like it on automatic reloads either, but I can live with it) Should we have a separate effort to do the error page javascript injection on new errors? Seems like it would solve both these problems.
All comments incorporated; ready for a new round of review. Jochen: I'm adding you as a reviewer because I've tweaked NetErrorPageController (originally ErrorCacheLoad); I don't think I've done anything gin related that's different than what you've already reviewed, but you might want to glance at it anyway. Also, could you response to Matt's question in https://codereview.chromium.org/207553008/diff/200001/chrome/renderer/net/err...; it's cut&pasted code, and I'm not sure of the answer. Thanks! https://codereview.chromium.org/207553008/diff/200001/chrome/browser/ui/brows... File chrome/browser/ui/browser_commands.cc (right): https://codereview.chromium.org/207553008/diff/200001/chrome/browser/ui/brows... chrome/browser/ui/browser_commands.cc:249: net_error_tab_helper->NotifyBrowserReloading(); On 2014/04/09 15:59:26, mmenke wrote: > We can't deduce this from the information passed to > WebContentsObserver::DidStartNavigationToPendingEntry? So I stopped here going down the stack because this was the last place where I could be sure that we really were coming from the browser reload button. Codesearch says that NavigationController::Reload* is called all over the place. So I didn't think I could with certainty tell that the browser reload button was being hit if I went below this point in the stack. https://codereview.chromium.org/207553008/diff/200001/chrome/renderer/net/err... File chrome/renderer/net/error_page_helper_functions.cc (right): https://codereview.chromium.org/207553008/diff/200001/chrome/renderer/net/err... chrome/renderer/net/error_page_helper_functions.cc:5: #include "error_page_helper_functions.h" On 2014/04/09 15:59:26, mmenke wrote: > Use full path. Ooops; done. https://codereview.chromium.org/207553008/diff/200001/chrome/renderer/net/err... chrome/renderer/net/error_page_helper_functions.cc:7: #include "base/strings/string_util.h" On 2014/04/09 15:59:26, mmenke wrote: > Is this needed? Or just base/strings/string_piece.h? Done. https://codereview.chromium.org/207553008/diff/200001/chrome/renderer/net/err... chrome/renderer/net/error_page_helper_functions.cc:13: #include "third_party/WebKit/public/web/WebDataSource.h" On 2014/04/09 15:59:26, mmenke wrote: > Are either of these two needed? Nope; the refactor into NEHC removed these references. Thanks for the catch. Done. https://codereview.chromium.org/207553008/diff/200001/chrome/renderer/net/err... chrome/renderer/net/error_page_helper_functions.cc:27: if (context.IsEmpty()) On 2014/04/09 15:59:26, mmenke wrote: > Can this happen? So this code was cut&pasted from DomAutomationController, so I don't know :-}. I don't personally see how it could be, and would be happy to move it to a DCHECK for that reason. Jochen? https://codereview.chromium.org/207553008/diff/200001/chrome/renderer/net/err... chrome/renderer/net/error_page_helper_functions.cc:39: if (!render_frame_) On 2014/04/10 17:15:37, Elly Jones wrote: > can this object outlive the render_frame_? I'm going based on Jochen's comment in https://codereview.chromium.org/140083004/diff/40001/chrome/renderer/net/erro..., which says "Yes". I can't tell you why, though; maybe it's just independent lifetimes. https://codereview.chromium.org/207553008/diff/200001/chrome/renderer/net/err... File chrome/renderer/net/error_page_helper_functions.h (right): https://codereview.chromium.org/207553008/diff/200001/chrome/renderer/net/err... chrome/renderer/net/error_page_helper_functions.h:25: class ErrorPageHelperFunctions On 2014/04/09 15:59:26, mmenke wrote: > Think this is worth a comment (Error pages functions called from Javascript) Done. https://codereview.chromium.org/207553008/diff/200001/chrome/renderer/net/err... chrome/renderer/net/error_page_helper_functions.h:25: class ErrorPageHelperFunctions On 2014/04/09 15:59:26, mmenke wrote: > Think NetErrorHelperFunctions to be more consistent or [Net]ErrorPageController > - a couple classes like this use controller, and > NetErrorPageFunctionsCalledFromJavascript seems a bit too long. :) Done. https://codereview.chromium.org/207553008/diff/200001/chrome/renderer/net/err... chrome/renderer/net/error_page_helper_functions.h:58: content::RenderFrame* render_frame_; On 2014/04/09 15:59:26, mmenke wrote: > Don't need our own render_frame_: render_frame() is NULLed out before destruct. Done. https://codereview.chromium.org/207553008/diff/200001/chrome/renderer/net/err... chrome/renderer/net/error_page_helper_functions.h:60: const GURL page_url_; On 2014/04/09 15:59:26, mmenke wrote: > Doesn't seem to be needed. Good catch. Done. https://codereview.chromium.org/207553008/diff/200001/chrome/renderer/net/err... chrome/renderer/net/error_page_helper_functions.h:62: bool render_frame_destroyed_; On 2014/04/09 15:59:26, mmenke wrote: > Not used. Done. https://codereview.chromium.org/207553008/diff/200001/chrome/renderer/net/net... File chrome/renderer/net/net_error_helper_core.cc (right): https://codereview.chromium.org/207553008/diff/200001/chrome/renderer/net/net... chrome/renderer/net/net_error_helper_core.cc:376: navigation_from_button_ = NO_BUTTON; On 2014/04/09 15:59:26, mmenke wrote: > Problem: Navigate to foo.com/, get an error. Click reload. Reload takes a > while. Click back button. We end up at another error page, but with the same > RenderView, resulting in recording the wrong thing (Some omnibox loads may also > reuse the RenderView). Good point. I added in a test to make sure it was the same URL; do you think that solves the problem? (I tested manually by hijacking Reload -> www.google.com, and that didn't increment the reload->error count.) https://codereview.chromium.org/207553008/diff/200001/chrome/renderer/net/net... chrome/renderer/net/net_error_helper_core.cc:412: chrome_common_net::LOAD_STALE_BUTTON_SHOWN_EVENT); On 2014/04/09 15:59:26, mmenke wrote: > nit: Use braces on multi-line if's (x2). Done. https://codereview.chromium.org/207553008/diff/200001/chrome/renderer/net/net... chrome/renderer/net/net_error_helper_core.cc:506: if (pending_error_page_info_.get()) { On 2014/04/09 15:59:26, mmenke wrote: > nit: .get() not needed. Done. https://codereview.chromium.org/207553008/diff/200001/chrome/renderer/net/net... chrome/renderer/net/net_error_helper_core.cc:557: // changed by a DNS error update. On 2014/04/09 15:59:26, mmenke wrote: > nit: Don't use we in comments. Done. https://codereview.chromium.org/207553008/diff/200001/chrome/renderer/resourc... File chrome/renderer/resources/neterror.html (right): https://codereview.chromium.org/207553008/diff/200001/chrome/renderer/resourc... chrome/renderer/resources/neterror.html:25: jsselect="reloadButton" jsvalues=".url:reloadUrl" On 2014/04/09 15:59:26, mmenke wrote: > jsvalues=".url:reloadUrl" is no longer needed (Can get rid of the field > entirely, actually). Done. > Wonder if we show the cache reload confirmation button if we reload this > way...Probably not, but worth checking. Experimental results: * Form post -> web page, Browser reload: Prompt the user to make sure they're ok with it. * Form post -> error page, browser reload: Prompt. * Form post -> error page, reload button: No prompt; error page is just reloaded. * Form post -> error page, reload button on page -> error page, browser reload: No prompt. * Form post -> error page, reload button on page -> error page, re-enable network, browser reload: No prompt (and request to server is a GET). I.e. not only does the reload button on the error page not result in a prompt, it also turns the internal recording of the page into a GET from a POST. I dug into this a little bit, and the call into the reload pathway that shows the warning comes from NavigationControllerImpl::ReloadInternal(), which as noted earlier is where the browser reload button goes. The reload button on the error page goes to blink:WebFrame::reload. If there's a renderer side function already existing that hooks into the chromium reload path, we could hook in very easily, but I'm not aware of one. If we want the prompt behavior, we're probably going to need to create an IPC.
https://codereview.chromium.org/207553008/diff/200001/chrome/renderer/net/net... File chrome/renderer/net/net_error_helper_core.cc (right): https://codereview.chromium.org/207553008/diff/200001/chrome/renderer/net/net... chrome/renderer/net/net_error_helper_core.cc:376: navigation_from_button_ = NO_BUTTON; On 2014/04/10 21:51:01, rdsmith wrote: > On 2014/04/09 15:59:26, mmenke wrote: > > Problem: Navigate to foo.com/, get an error. Click reload. Reload takes a > > while. Click back button. We end up at another error page, but with the same > > RenderView, resulting in recording the wrong thing (Some omnibox loads may > also > > reuse the RenderView). > > Good point. I added in a test to make sure it was the same URL; do you think > that solves the problem? (I tested manually by hijacking Reload -> > http://www.google.com, and that didn't increment the reload->error count.) This still isn't perfect, but it's probably good enough, if we can't do better without adding significant complexity. One simple case where this doesn't work is user presses the reload button button on the page, it hangs, and then the user presses the browser's reload button, or F5, and the second load succeeds. I do think this funkiness is worth a comment. https://codereview.chromium.org/207553008/diff/210001/chrome/browser/errorpag... File chrome/browser/errorpage_browsertest.cc (right): https://codereview.chromium.org/207553008/diff/210001/chrome/browser/errorpag... chrome/browser/errorpage_browsertest.cc:275: testing::AssertionResult ReloadStaleCopyFromCache() { Now that we have a button, should "click" it with a script instead. https://codereview.chromium.org/207553008/diff/210001/chrome/browser/errorpag... chrome/browser/errorpage_browsertest.cc:683: EXPECT_TRUE(ProbeStaleCopyValue(true)); Suggest also checking the button's visibility. Should also check that it's not visible (Same for the other test) https://codereview.chromium.org/207553008/diff/210001/chrome/browser/errorpag... chrome/browser/errorpage_browsertest.cc:883: ProbeStaleCopyValue(false); Should also check that the button's hidden. https://codereview.chromium.org/207553008/diff/210001/chrome/browser/net/net_... File chrome/browser/net/net_error_tab_helper.cc (right): https://codereview.chromium.org/207553008/diff/210001/chrome/browser/net/net_... chrome/browser/net/net_error_tab_helper.cc:83: chrome_common_net::BROWSER_RELOAD_BUTTON_CLICKED_EVENT); +braces. https://codereview.chromium.org/207553008/diff/210001/chrome/browser/net/net_... chrome/browser/net/net_error_tab_helper.cc:83: chrome_common_net::BROWSER_RELOAD_BUTTON_CLICKED_EVENT); Does this record F5s? What about right clicking and selecting reload? Do we care? Not really sure what the purpose is here. https://codereview.chromium.org/207553008/diff/210001/chrome/common/localized... File chrome/common/localized_error.cc (right): https://codereview.chromium.org/207553008/diff/210001/chrome/common/localized... chrome/common/localized_error.cc:502: bool stale_load_button, nit: Maybe show_stale_load_button? https://codereview.chromium.org/207553008/diff/210001/chrome/common/localized... chrome/common/localized_error.cc:652: reload_button->SetString("reloadUrl", failed_url.spec()); This string is no longer used. https://codereview.chromium.org/207553008/diff/210001/chrome/common/net/net_e... File chrome/common/net/net_error_info.h (right): https://codereview.chromium.org/207553008/diff/210001/chrome/common/net/net_e... chrome/common/net/net_error_info.h:12: PAGE_SHOWN_EVENT, // Error pages shown. enums generally share prefixes, not suffixes. I also think these are a bit too ambiguous...Suggest: NET_ERROR_PAGE_SHOWN, NET_ERROR_GOAT_STRIKE, etc. https://codereview.chromium.org/207553008/diff/210001/chrome/renderer/net/net... File chrome/renderer/net/net_error_helper_core.cc (right): https://codereview.chromium.org/207553008/diff/210001/chrome/renderer/net/net... chrome/renderer/net/net_error_helper_core.cc:370: pending_error_page_info_->error.unreachableURL) { suggest indenting the last line 4 more, for easier reading. https://codereview.chromium.org/207553008/diff/210001/chrome/renderer/net/net... chrome/renderer/net/net_error_helper_core.cc:372: navigation_from_button_ == LOAD_STALE_BUTTON); include logging.h https://codereview.chromium.org/207553008/diff/210001/chrome/renderer/net/net... chrome/renderer/net/net_error_helper_core.cc:376: chrome_common_net::LOAD_STALE_BUTTON_ERROR_EVENT); think this is easier to read of the last two lines are indented 4 more https://codereview.chromium.org/207553008/diff/210001/chrome/renderer/net/net... File chrome/renderer/net/net_error_helper_core.h (right): https://codereview.chromium.org/207553008/diff/210001/chrome/renderer/net/net... chrome/renderer/net/net_error_helper_core.h:162: // care of in javascript. nit: capitalize Javascript. https://codereview.chromium.org/207553008/diff/210001/chrome/renderer/net/net... File chrome/renderer/net/net_error_page_controller.h (right): https://codereview.chromium.org/207553008/diff/210001/chrome/renderer/net/net... chrome/renderer/net/net_error_page_controller.h:8: #include "base/basictypes.h" Use macros.h instead (basictypes includes DISALLOW_COPY_AND_ASSIGN but not OVERRIDE, I believe) https://codereview.chromium.org/207553008/diff/210001/chrome/renderer/net/net... chrome/renderer/net/net_error_page_controller.h:11: #include "url/gurl.h" Not used. https://codereview.chromium.org/207553008/diff/210001/chrome/renderer/net/net... chrome/renderer/net/net_error_page_controller.h:13: // NetErrorPageController class: optional: Think this is a little redundant. Suggest replacing "this class" with the class name, or just getting rid of this extra line. https://codereview.chromium.org/207553008/diff/210001/chrome/renderer/net/net... chrome/renderer/net/net_error_page_controller.h:16: // error page loaded by NetErrorHelper. It is bound to the javascript nit: Javascript. https://codereview.chromium.org/207553008/diff/210001/chrome/renderer/net/net... chrome/renderer/net/net_error_page_controller.h:17: // window.errorCacheLoad object. These generally go right above the class, not the forward declarations https://codereview.chromium.org/207553008/diff/210001/chrome/renderer/net/net... chrome/renderer/net/net_error_page_controller.h:17: // window.errorCacheLoad object. The window.helperFunctions object, now. window.errorPageController may be a better name. https://codereview.chromium.org/207553008/diff/210001/chrome/renderer/net/net... chrome/renderer/net/net_error_page_controller.h:19: class GURL; Not used.
https://codereview.chromium.org/207553008/diff/210001/chrome/renderer/net/net... File chrome/renderer/net/net_error_page_controller.h (right): https://codereview.chromium.org/207553008/diff/210001/chrome/renderer/net/net... chrome/renderer/net/net_error_page_controller.h:28: public content::RenderFrameObserver { why do you derive from RFO?
Jochen: I'll take a look at this and get back to you (current guess: historical, can be removed), but I don't think the question I asked you is dependent on this; could you take a look at https://codereview.chromium.org/207553008/diff/200001/chrome/renderer/net/err... comment? -- Randy On Monday, April 14, 2014, <jochen@chromium.org> wrote: > > https://codereview.chromium.org/207553008/diff/210001/ > chrome/renderer/net/net_error_page_controller.h > File chrome/renderer/net/net_error_page_controller.h (right): > > https://codereview.chromium.org/207553008/diff/210001/ > chrome/renderer/net/net_error_page_controller.h#newcode28 > chrome/renderer/net/net_error_page_controller.h:28: public > content::RenderFrameObserver { > why do you derive from RFO? > > https://codereview.chromium.org/207553008/ > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
https://codereview.chromium.org/207553008/diff/200001/chrome/renderer/net/err... File chrome/renderer/net/error_page_helper_functions.cc (right): https://codereview.chromium.org/207553008/diff/200001/chrome/renderer/net/err... chrome/renderer/net/error_page_helper_functions.cc:27: if (context.IsEmpty()) On 2014/04/10 21:51:01, rdsmith wrote: > On 2014/04/09 15:59:26, mmenke wrote: > > Can this happen? > > So this code was cut&pasted from DomAutomationController, so I don't know :-}. > I don't personally see how it could be, and would be happy to move it to a > DCHECK for that reason. Jochen? If JavaScript is disabled by content settings, this can happen. https://codereview.chromium.org/207553008/diff/200001/chrome/renderer/net/err... chrome/renderer/net/error_page_helper_functions.cc:39: if (!render_frame_) On 2014/04/10 21:51:01, rdsmith wrote: > On 2014/04/10 17:15:37, Elly Jones wrote: > > can this object outlive the render_frame_? > > I'm going based on Jochen's comment in > https://codereview.chromium.org/140083004/diff/40001/chrome/renderer/net/erro..., > which says "Yes". I can't tell you why, though; maybe it's just independent > lifetimes. Right, the javascript wrapper is destroyed via garbage collection which can be delayed beyond the lifetime of the render frame https://codereview.chromium.org/207553008/diff/210001/chrome/renderer/net/net... File chrome/renderer/net/net_error_page_controller.h (right): https://codereview.chromium.org/207553008/diff/210001/chrome/renderer/net/net... chrome/renderer/net/net_error_page_controller.h:28: public content::RenderFrameObserver { On 2014/04/14 07:32:35, jochen wrote: > why do you derive from RFO? ah, I see that you're using RenderFrameObserver::render_frame()
net_error_{,tab_}helper.* look fine to me.
https://codereview.chromium.org/207553008/diff/210001/chrome/renderer/net/net...
File chrome/renderer/net/net_error_helper_core.h (right):
https://codereview.chromium.org/207553008/diff/210001/chrome/renderer/net/net...
chrome/renderer/net/net_error_helper_core.h:162: // care of in javascript.
On 2014/04/11 19:42:32, mmenke wrote:
> nit: capitalize Javascript.
Is it Javascript or JavaScript?
https://codereview.chromium.org/207553008/diff/210001/chrome/renderer/net/net... File chrome/renderer/net/net_error_page_controller.h (right): https://codereview.chromium.org/207553008/diff/210001/chrome/renderer/net/net... chrome/renderer/net/net_error_page_controller.h:28: public content::RenderFrameObserver { On 2014/04/14 13:11:53, jochen wrote: > On 2014/04/14 07:32:35, jochen wrote: > > why do you derive from RFO? > > ah, I see that you're using RenderFrameObserver::render_frame() Yea, we're using the OnDestruct method, so we don't call into the render frame after it's destroyed, since apparently this can both outlive render frame, and some of its Click functions may be called after the render frame is destroyed (I assume).
Ready for next round of review. David: I added you to the cc list in case you had any comments on https://codereview.chromium.org/207553008/diff/200001/chrome/renderer/net/net...; you're welcome to ignore this CL except for that question. Matt pointed out to me offline that this CL was going to break IOS, because they weren't going to have the new C++ bindings that we're now routing the various error page button clicks through. PS12 attempts to address that issue; it makes the javascript work properly even if the new C++ bindings aren't available. Matt: I've used a sledgehammer here in checking for the bindings directly in the window namespace; if you'd prefer me to use the <if> syntax in the file to conditionalize on IOS, I'm happy to do that too. https://codereview.chromium.org/207553008/diff/200001/chrome/renderer/net/net... File chrome/renderer/net/net_error_helper_core.cc (right): https://codereview.chromium.org/207553008/diff/200001/chrome/renderer/net/net... chrome/renderer/net/net_error_helper_core.cc:376: navigation_from_button_ = NO_BUTTON; On 2014/04/11 19:42:32, mmenke wrote: > On 2014/04/10 21:51:01, rdsmith wrote: > > On 2014/04/09 15:59:26, mmenke wrote: > > > Problem: Navigate to foo.com/, get an error. Click reload. Reload takes a > > > while. Click back button. We end up at another error page, but with the > same > > > RenderView, resulting in recording the wrong thing (Some omnibox loads may > > also > > > reuse the RenderView). > > > > Good point. I added in a test to make sure it was the same URL; do you think > > that solves the problem? (I tested manually by hijacking Reload -> > > http://www.google.com, and that didn't increment the reload->error count.) > > This still isn't perfect, but it's probably good enough, if we can't do better > without adding significant complexity. One simple case where this doesn't work > is user presses the reload button button on the page, it hangs, and then the > user presses the browser's reload button, or F5, and the second load succeeds. > > I do think this funkiness is worth a comment. I've added a comment, but I'd like to get a design in my head for doing the right thing before deciding that we can't do better without adding significant complexity. While this isn't currently true (:-{), to my mind the page reload and the browser reload button should be functionally doing the same thing. If they (eventually) do, we're not going to be able to distinguish between them unless we get an "about to load" signal on the renderer. It looks to me as if DidStartProvisionalLoad() is such a signal. If we null out navigation_from_button_ on the second call to DidStartProvisionalLoad() after we do either Reload or Load Stale (and turned the url check into a DCHECK), do you think that would do the job? I think the only way that could fail is if we had a race between a browser initiated navigation and our renderer initiated one, and (in the current implementation) I'm not sure that can happen since the renderer initiated one is (I think) synchronous to DidStartProvisionalLoad(). WDYT? (I'm also going to cc David in, since I think he knows this code pretty well.) https://codereview.chromium.org/207553008/diff/210001/chrome/browser/errorpag... File chrome/browser/errorpage_browsertest.cc (right): https://codereview.chromium.org/207553008/diff/210001/chrome/browser/errorpag... chrome/browser/errorpage_browsertest.cc:275: testing::AssertionResult ReloadStaleCopyFromCache() { On 2014/04/11 19:42:32, mmenke wrote: > Now that we have a button, should "click" it with a script instead. Done. https://codereview.chromium.org/207553008/diff/210001/chrome/browser/errorpag... chrome/browser/errorpage_browsertest.cc:683: EXPECT_TRUE(ProbeStaleCopyValue(true)); On 2014/04/11 19:42:32, mmenke wrote: > Suggest also checking the button's visibility. Should also check that it's not > visible (Same for the other test) Done. https://codereview.chromium.org/207553008/diff/210001/chrome/browser/errorpag... chrome/browser/errorpage_browsertest.cc:883: ProbeStaleCopyValue(false); On 2014/04/11 19:42:32, mmenke wrote: > Should also check that the button's hidden. Done. https://codereview.chromium.org/207553008/diff/210001/chrome/browser/net/net_... File chrome/browser/net/net_error_tab_helper.cc (right): https://codereview.chromium.org/207553008/diff/210001/chrome/browser/net/net_... chrome/browser/net/net_error_tab_helper.cc:83: chrome_common_net::BROWSER_RELOAD_BUTTON_CLICKED_EVENT); On 2014/04/11 19:42:32, mmenke wrote: > +braces. Whoops :-{. Done. https://codereview.chromium.org/207553008/diff/210001/chrome/browser/net/net_... chrome/browser/net/net_error_tab_helper.cc:83: chrome_common_net::BROWSER_RELOAD_BUTTON_CLICKED_EVENT); On 2014/04/11 19:42:32, mmenke wrote: > Does this record F5s? What about right clicking and selecting reload? Do we > care? Not really sure what the purpose is here. First, the facts: It captures F5, but not right click->selecting reload, I presume because F5 goes through the browser and right click through the renderer. Then, the intent: My purpose is to figure out how often people use the reload button on the error page versus "other" ways of reloading (loosely, gathering data to see if we might want to nuke the reload button at some point. Ideally, I'd like to break out the different other ways to reload so that we could understand what different people's "reload" instincts are. But I don't want to do more browser plumbing. I consider it important to catch the right click case, so I went digging, and it looks like the simplest way to get what I want is to go with your suggestion of a PS or two back to hook DidStartNavigationToPendingEntry(), and just dump all browser initiated navigations in the same bin. Done :-}. https://codereview.chromium.org/207553008/diff/210001/chrome/common/localized... File chrome/common/localized_error.cc (right): https://codereview.chromium.org/207553008/diff/210001/chrome/common/localized... chrome/common/localized_error.cc:502: bool stale_load_button, On 2014/04/11 19:42:32, mmenke wrote: > nit: Maybe show_stale_load_button? Done. https://codereview.chromium.org/207553008/diff/210001/chrome/common/localized... chrome/common/localized_error.cc:652: reload_button->SetString("reloadUrl", failed_url.spec()); On 2014/04/11 19:42:32, mmenke wrote: > This string is no longer used. Right. Got rid of the subdirectory and just assigned the message to reloadButton directly. (Note: I may revert this as part of handling the potential IOS breakage. If I do, I'll do it in a separate PS, so you can review just what I've done to handle that case separately.) https://codereview.chromium.org/207553008/diff/210001/chrome/common/net/net_e... File chrome/common/net/net_error_info.h (right): https://codereview.chromium.org/207553008/diff/210001/chrome/common/net/net_e... chrome/common/net/net_error_info.h:12: PAGE_SHOWN_EVENT, // Error pages shown. On 2014/04/11 19:42:32, mmenke wrote: > enums generally share prefixes, not suffixes. I also think these are a bit too > ambiguous...Suggest: > > NET_ERROR_PAGE_SHOWN, > NET_ERROR_GOAT_STRIKE, > etc. Yeah, they used to be under NetErrorHelperCore, which gave more context. Done. https://codereview.chromium.org/207553008/diff/210001/chrome/renderer/net/net... File chrome/renderer/net/net_error_helper_core.cc (right): https://codereview.chromium.org/207553008/diff/210001/chrome/renderer/net/net... chrome/renderer/net/net_error_helper_core.cc:370: pending_error_page_info_->error.unreachableURL) { On 2014/04/11 19:42:32, mmenke wrote: > suggest indenting the last line 4 more, for easier reading. Done. https://codereview.chromium.org/207553008/diff/210001/chrome/renderer/net/net... chrome/renderer/net/net_error_helper_core.cc:372: navigation_from_button_ == LOAD_STALE_BUTTON); On 2014/04/11 19:42:32, mmenke wrote: > include logging.h Done. https://codereview.chromium.org/207553008/diff/210001/chrome/renderer/net/net... chrome/renderer/net/net_error_helper_core.cc:376: chrome_common_net::LOAD_STALE_BUTTON_ERROR_EVENT); On 2014/04/11 19:42:32, mmenke wrote: > think this is easier to read of the last two lines are indented 4 more Done. https://codereview.chromium.org/207553008/diff/210001/chrome/renderer/net/net... File chrome/renderer/net/net_error_helper_core.h (right): https://codereview.chromium.org/207553008/diff/210001/chrome/renderer/net/net... chrome/renderer/net/net_error_helper_core.h:162: // care of in javascript. On 2014/04/14 13:56:28, ttuttle wrote: > On 2014/04/11 19:42:32, mmenke wrote: > > nit: capitalize Javascript. > > Is it Javascript or JavaScript? A bit of web surfing suggests JavaScript. https://codereview.chromium.org/207553008/diff/210001/chrome/renderer/net/net... chrome/renderer/net/net_error_helper_core.h:162: // care of in javascript. On 2014/04/11 19:42:32, mmenke wrote: > nit: capitalize Javascript. The bit of web surfing I did in response to Thomas' nit suggests that there's debate about that issue :-}. Having said that, done. https://codereview.chromium.org/207553008/diff/210001/chrome/renderer/net/net... File chrome/renderer/net/net_error_page_controller.h (right): https://codereview.chromium.org/207553008/diff/210001/chrome/renderer/net/net... chrome/renderer/net/net_error_page_controller.h:8: #include "base/basictypes.h" On 2014/04/11 19:42:32, mmenke wrote: > Use macros.h instead (basictypes includes DISALLOW_COPY_AND_ASSIGN but not > OVERRIDE, I believe) Done. https://codereview.chromium.org/207553008/diff/210001/chrome/renderer/net/net... chrome/renderer/net/net_error_page_controller.h:11: #include "url/gurl.h" On 2014/04/11 19:42:32, mmenke wrote: > Not used. Done. https://codereview.chromium.org/207553008/diff/210001/chrome/renderer/net/net... chrome/renderer/net/net_error_page_controller.h:13: // NetErrorPageController class: On 2014/04/11 19:42:32, mmenke wrote: > optional: Think this is a little redundant. Suggest replacing "this class" > with the class name, or just getting rid of this extra line. Done. https://codereview.chromium.org/207553008/diff/210001/chrome/renderer/net/net... chrome/renderer/net/net_error_page_controller.h:16: // error page loaded by NetErrorHelper. It is bound to the javascript On 2014/04/11 19:42:32, mmenke wrote: > nit: Javascript. Done (JavaScript). https://codereview.chromium.org/207553008/diff/210001/chrome/renderer/net/net... chrome/renderer/net/net_error_page_controller.h:17: // window.errorCacheLoad object. On 2014/04/11 19:42:32, mmenke wrote: > These generally go right above the class, not the forward declarations Done. https://codereview.chromium.org/207553008/diff/210001/chrome/renderer/net/net... chrome/renderer/net/net_error_page_controller.h:17: // window.errorCacheLoad object. On 2014/04/11 19:42:32, mmenke wrote: > The window.helperFunctions object, now. window.errorPageController may be a > better name. Done. https://codereview.chromium.org/207553008/diff/210001/chrome/renderer/net/net... chrome/renderer/net/net_error_page_controller.h:19: class GURL; On 2014/04/11 19:42:32, mmenke wrote: > Not used. Done.
Just nits. https://codereview.chromium.org/207553008/diff/240001/chrome/browser/errorpag... File chrome/browser/errorpage_browsertest.cc (right): https://codereview.chromium.org/207553008/diff/240001/chrome/browser/errorpag... chrome/browser/errorpage_browsertest.cc:279: return testing::AssertionFailure() << "Result is " << result; nit: "Cache probe result is..." maybe? https://codereview.chromium.org/207553008/diff/240001/chrome/browser/errorpag... chrome/browser/errorpage_browsertest.cc:303: std::string LoadStaleButtonLabel() { Suggest either making this static, or putting in an anonymous namespace. https://codereview.chromium.org/207553008/diff/240001/chrome/browser/net/net_... File chrome/browser/net/net_error_tab_helper.cc (right): https://codereview.chromium.org/207553008/diff/240001/chrome/browser/net/net_... chrome/browser/net/net_error_tab_helper.cc:89: if (is_error_page_) { Think it's a little weird to use early return in the NO_RELOAD check, but not for the is_error_page_ check. I'm actually fine with any other combination (early returning on both, including both in the if, or early returning in the is_error_page_ case first, but then checking the reload_type in the if - this is the NetErrorTabHelper, afterall). https://codereview.chromium.org/207553008/diff/240001/chrome/browser/ui/brows... File chrome/browser/ui/browser_commands.cc (right): https://codereview.chromium.org/207553008/diff/240001/chrome/browser/ui/brows... chrome/browser/ui/browser_commands.cc:243: For simplicity, should just remove this change from this CL. Unless you really, really feel strongly about this blank line. https://codereview.chromium.org/207553008/diff/240001/chrome/common/net/net_e... File chrome/common/net/net_error_info.h (right): https://codereview.chromium.org/207553008/diff/240001/chrome/common/net/net_e... chrome/common/net/net_error_info.h:12: NET_ERROR_PAGE_SHOWN, // Error pages shown. nit: SHould probably line up all the comments. https://codereview.chromium.org/207553008/diff/240001/chrome/common/net/net_e... chrome/common/net/net_error_info.h:16: NET_ERROR_PAGE_RELOAD_BUTTON_ERROR, // Reload button clicks -> error. For all of these comments, shouldn't "clicks" be "clicked"? https://codereview.chromium.org/207553008/diff/240001/chrome/common/net/net_e... chrome/common/net/net_error_info.h:19: NET_ERROR_PAGE_LOAD_STALE_BUTTON_CLICKED, // Load stale button clicks. Don't suppose there's space to indent these all two more? Technically, should be two spaces between code and comments. https://codereview.chromium.org/207553008/diff/240001/chrome/common/net/net_e... chrome/common/net/net_error_info.h:24: NET_ERROR_PAGE_BROWSER_INITIATED_RELOAD, // Reload initiated from browser. nit: +space before comment. https://codereview.chromium.org/207553008/diff/240001/chrome/renderer/resourc... File chrome/renderer/resources/neterror.html (right): https://codereview.chromium.org/207553008/diff/240001/chrome/renderer/resourc... chrome/renderer/resources/neterror.html:33: jsdisplay="more" jsvalues=".moreText:more; .lessText:less;" nit: Don't use tabs https://codereview.chromium.org/207553008/diff/240001/chrome/renderer/resourc... File chrome/renderer/resources/neterror.js (right): https://codereview.chromium.org/207553008/diff/240001/chrome/renderer/resourc... chrome/renderer/resources/neterror.js:83: } nit: These all should be using two space indent. https://codereview.chromium.org/207553008/diff/240001/chrome/renderer/resourc... chrome/renderer/resources/neterror.js:83: } Can't you just use: if (errorPageController) { errorPageController.blah(); }
Incorporated Matt's comments on PS12. https://codereview.chromium.org/207553008/diff/240001/chrome/browser/errorpag... File chrome/browser/errorpage_browsertest.cc (right): https://codereview.chromium.org/207553008/diff/240001/chrome/browser/errorpag... chrome/browser/errorpage_browsertest.cc:279: return testing::AssertionFailure() << "Result is " << result; On 2014/04/15 16:02:25, mmenke wrote: > nit: "Cache probe result is..." maybe? Done. https://codereview.chromium.org/207553008/diff/240001/chrome/browser/errorpag... chrome/browser/errorpage_browsertest.cc:303: std::string LoadStaleButtonLabel() { On 2014/04/15 16:02:25, mmenke wrote: > Suggest either making this static, or putting in an anonymous namespace. Done. https://codereview.chromium.org/207553008/diff/240001/chrome/browser/net/net_... File chrome/browser/net/net_error_tab_helper.cc (right): https://codereview.chromium.org/207553008/diff/240001/chrome/browser/net/net_... chrome/browser/net/net_error_tab_helper.cc:89: if (is_error_page_) { On 2014/04/15 16:02:25, mmenke wrote: > Think it's a little weird to use early return in the NO_RELOAD check, but not > for the is_error_page_ check. > > I'm actually fine with any other combination (early returning on both, including > both in the if, or early returning in the is_error_page_ case first, but then > checking the reload_type in the if - this is the NetErrorTabHelper, afterall). Completely fair. I opted for the early return if not an error page, then a commented if conditional. https://codereview.chromium.org/207553008/diff/240001/chrome/browser/ui/brows... File chrome/browser/ui/browser_commands.cc (right): https://codereview.chromium.org/207553008/diff/240001/chrome/browser/ui/brows... chrome/browser/ui/browser_commands.cc:243: On 2014/04/15 16:02:25, mmenke wrote: > For simplicity, should just remove this change from this CL. Unless you really, > really feel strongly about this blank line. Don't you understand how much this line improves the file? The passion and simplicity it brings??? Ah, well, great works of art are often not understood when they're first created. I'll remove it now and wait for a more enlightened audience. (I.e. "Ooops" :-}) https://codereview.chromium.org/207553008/diff/240001/chrome/common/net/net_e... File chrome/common/net/net_error_info.h (right): https://codereview.chromium.org/207553008/diff/240001/chrome/common/net/net_e... chrome/common/net/net_error_info.h:12: NET_ERROR_PAGE_SHOWN, // Error pages shown. On 2014/04/15 16:02:25, mmenke wrote: > nit: SHould probably line up all the comments. Done. https://codereview.chromium.org/207553008/diff/240001/chrome/common/net/net_e... chrome/common/net/net_error_info.h:16: NET_ERROR_PAGE_RELOAD_BUTTON_ERROR, // Reload button clicks -> error. On 2014/04/15 16:02:25, mmenke wrote: > For all of these comments, shouldn't "clicks" be "clicked"? Done. https://codereview.chromium.org/207553008/diff/240001/chrome/common/net/net_e... chrome/common/net/net_error_info.h:19: NET_ERROR_PAGE_LOAD_STALE_BUTTON_CLICKED, // Load stale button clicks. On 2014/04/15 16:02:25, mmenke wrote: > Don't suppose there's space to indent these all two more? Technically, should > be two spaces between code and comments. I think I only need to indent one more to get two spaces, which I've done. https://codereview.chromium.org/207553008/diff/240001/chrome/common/net/net_e... chrome/common/net/net_error_info.h:24: NET_ERROR_PAGE_BROWSER_INITIATED_RELOAD, // Reload initiated from browser. On 2014/04/15 16:02:25, mmenke wrote: > nit: +space before comment. I'm assuming this is redundant with your other comments? I don't see the need once everything's been lined up. https://codereview.chromium.org/207553008/diff/240001/chrome/renderer/resourc... File chrome/renderer/resources/neterror.html (right): https://codereview.chromium.org/207553008/diff/240001/chrome/renderer/resourc... chrome/renderer/resources/neterror.html:33: jsdisplay="more" jsvalues=".moreText:more; .lessText:less;" On 2014/04/15 16:02:25, mmenke wrote: > nit: Don't use tabs Done. https://codereview.chromium.org/207553008/diff/240001/chrome/renderer/resourc... File chrome/renderer/resources/neterror.js (right): https://codereview.chromium.org/207553008/diff/240001/chrome/renderer/resourc... chrome/renderer/resources/neterror.js:83: } On 2014/04/15 16:02:25, mmenke wrote: > nit: These all should be using two space indent. Done. https://codereview.chromium.org/207553008/diff/240001/chrome/renderer/resourc... chrome/renderer/resources/neterror.js:83: } On 2014/04/15 16:02:25, mmenke wrote: > Can't you just use: > > if (errorPageController) { > errorPageController.blah(); > } When I disable the Gin bindings (to mimc IOS) and try that, I get: [14801:14801:0415/142343:INFO:CONSOLE(386)] "Uncaught ReferenceError: errorPageController is not defined", source: data:text/html,chromewebdata (386)
I want to do a careful review tomorrow before signing off on this, but I'm happy with it. The revert of the new Link Doctor API revert is now in the CQ, so hopefully that won't delay landing this CL.
https://codereview.chromium.org/207553008/diff/200001/chrome/renderer/net/net... File chrome/renderer/net/net_error_helper_core.cc (right): https://codereview.chromium.org/207553008/diff/200001/chrome/renderer/net/net... chrome/renderer/net/net_error_helper_core.cc:376: navigation_from_button_ = NO_BUTTON; On 2014/04/14 22:02:13, rdsmith wrote: > On 2014/04/11 19:42:32, mmenke wrote: > > On 2014/04/10 21:51:01, rdsmith wrote: > > > On 2014/04/09 15:59:26, mmenke wrote: > > > > Problem: Navigate to foo.com/, get an error. Click reload. Reload takes > a > > > > while. Click back button. We end up at another error page, but with the > > same > > > > RenderView, resulting in recording the wrong thing (Some omnibox loads may > > > also > > > > reuse the RenderView). > > > > > > Good point. I added in a test to make sure it was the same URL; do you > think > > > that solves the problem? (I tested manually by hijacking Reload -> > > > http://www.google.com, and that didn't increment the reload->error count.) > > > > This still isn't perfect, but it's probably good enough, if we can't do better > > without adding significant complexity. One simple case where this doesn't > work > > is user presses the reload button button on the page, it hangs, and then the > > user presses the browser's reload button, or F5, and the second load succeeds. > > > > I do think this funkiness is worth a comment. > > I've added a comment, but I'd like to get a design in my head for doing the > right thing before deciding that we can't do better without adding significant > complexity. While this isn't currently true (:-{), to my mind the page reload > and the browser reload button should be functionally doing the same thing. If > they (eventually) do, we're not going to be able to distinguish between them > unless we get an "about to load" signal on the renderer. It looks to me as if > DidStartProvisionalLoad() is such a signal. If we null out > navigation_from_button_ on the second call to DidStartProvisionalLoad() after we > do either Reload or Load Stale (and turned the url check into a DCHECK), do you > think that would do the job? I think the only way that could fail is if we had > a race between a browser initiated navigation and our renderer initiated one, > and (in the current implementation) I'm not sure that can happen since the > renderer initiated one is (I think) synchronous to DidStartProvisionalLoad(). > WDYT? (I'm also going to cc David in, since I think he knows this code pretty > well.) Hrm. So I don't entirely follow what's going on here, but pending_error_page_info_ is the error info corresponding to the error page that's busy loading, right? And the problem is that sometimes a load commits but it's a different load because the error page load got canceled first? So, in that case, I think you want to listen for didFailProvisionalLoad. That should reliably get emitted if your load gets aborted for whatever reason? There's another complexity in that pending_error_page_info_ might not actually correspond to a load it looks like? RenderFrameImpl::didFailProvisionalLoad calls GetNavigationErrorStrings on the renderer client which seems to eventually call GetErrorHTML and set pending_error_page_info_. (Is this what the comment means by "pre-provisional"?) Looks like there are a number of codepaths in RFI::didFailProvisionalLoad which abort before LoadNavigationErrorPage is called, so you're not guaranteed your pre-provisional load actually happens. I'm not sure how you'd blow that one away if your "pre-provisional" error page never actually materializes. Maybe separate pre-provisional from provisional error info and only upgrade pre-provisional to provisional when you see a didStartProvisionalLoad for kUnreachableWebDataURL, which is what error pages use. Or reliably signal on all the early returns? Or am I misunderstanding what's going on?
On 2014/04/15 22:04:38, David Benjamin wrote: > https://codereview.chromium.org/207553008/diff/200001/chrome/renderer/net/net... > File chrome/renderer/net/net_error_helper_core.cc (right): > > https://codereview.chromium.org/207553008/diff/200001/chrome/renderer/net/net... > chrome/renderer/net/net_error_helper_core.cc:376: navigation_from_button_ = > NO_BUTTON; > On 2014/04/14 22:02:13, rdsmith wrote: > > On 2014/04/11 19:42:32, mmenke wrote: > > > On 2014/04/10 21:51:01, rdsmith wrote: > > > > On 2014/04/09 15:59:26, mmenke wrote: > > > > > Problem: Navigate to foo.com/, get an error. Click reload. Reload > takes > > a > > > > > while. Click back button. We end up at another error page, but with > the > > > same > > > > > RenderView, resulting in recording the wrong thing (Some omnibox loads > may > > > > also > > > > > reuse the RenderView). > > > > > > > > Good point. I added in a test to make sure it was the same URL; do you > > think > > > > that solves the problem? (I tested manually by hijacking Reload -> > > > > http://www.google.com, and that didn't increment the reload->error count.) > > > > > > This still isn't perfect, but it's probably good enough, if we can't do > better > > > without adding significant complexity. One simple case where this doesn't > > work > > > is user presses the reload button button on the page, it hangs, and then the > > > user presses the browser's reload button, or F5, and the second load > succeeds. > > > > > > I do think this funkiness is worth a comment. > > > > I've added a comment, but I'd like to get a design in my head for doing the > > right thing before deciding that we can't do better without adding significant > > complexity. While this isn't currently true (:-{), to my mind the page reload > > and the browser reload button should be functionally doing the same thing. If > > they (eventually) do, we're not going to be able to distinguish between them > > unless we get an "about to load" signal on the renderer. It looks to me as if > > DidStartProvisionalLoad() is such a signal. If we null out > > navigation_from_button_ on the second call to DidStartProvisionalLoad() after > we > > do either Reload or Load Stale (and turned the url check into a DCHECK), do > you > > think that would do the job? I think the only way that could fail is if we > had > > a race between a browser initiated navigation and our renderer initiated one, > > and (in the current implementation) I'm not sure that can happen since the > > renderer initiated one is (I think) synchronous to DidStartProvisionalLoad(). > > WDYT? (I'm also going to cc David in, since I think he knows this code pretty > > well.) > > Hrm. So I don't entirely follow what's going on here, but > pending_error_page_info_ is the error info corresponding to the error page > that's busy loading, right? And the problem is that sometimes a load commits but > it's a different load because the error page load got canceled first? > > So, in that case, I think you want to listen for didFailProvisionalLoad. That > should reliably get emitted if your load gets aborted for whatever reason? Are we sure it's reliably emitted? There's no real documentation, and a ton of cases where a load may be aborted, when process swaps are taken into account. > There's another complexity in that pending_error_page_info_ might not actually > correspond to a load it looks like? RenderFrameImpl::didFailProvisionalLoad > calls GetNavigationErrorStrings on the renderer client which seems to eventually > call GetErrorHTML and set pending_error_page_info_. (Is this what the comment > means by "pre-provisional"?) Looks like there are a number of codepaths in > RFI::didFailProvisionalLoad which abort before LoadNavigationErrorPage is > called, so you're not guaranteed your pre-provisional load actually happens. I'm > not sure how you'd blow that one away if your "pre-provisional" error page never > actually materializes. In those cases, GetErrorPageStrings is also not called, so we don't populate provisional_error_page_info_ in the first place. There may be other paths to worry about, however. > Maybe separate pre-provisional from provisional error info and only upgrade > pre-provisional to provisional when you see a didStartProvisionalLoad for > kUnreachableWebDataURL, which is what error pages use. Or reliably signal on all > the early returns? If there are cases where this happens, this sounds reasonable to me...Though the idea of *3* different sets of live error page information gives me nightmares. Either way, I don't think this would belong in this CL. > Or am I misunderstanding what's going on? You do indeed understand the concerns.
LGTM https://codereview.chromium.org/207553008/diff/240001/chrome/renderer/resourc... File chrome/renderer/resources/neterror.js (right): https://codereview.chromium.org/207553008/diff/240001/chrome/renderer/resourc... chrome/renderer/resources/neterror.js:83: } On 2014/04/15 18:27:29, rdsmith wrote: > On 2014/04/15 16:02:25, mmenke wrote: > > Can't you just use: > > > > if (errorPageController) { > > errorPageController.blah(); > > } > > When I disable the Gin bindings (to mimc IOS) and try that, I get: > > [14801:14801:0415/142343:INFO:CONSOLE(386)] "Uncaught ReferenceError: > errorPageController is not defined", source: data:text/html,chromewebdata (386) Ah, right. "window.errorPageController" would work, though - I prefer it for being more C-like, and I don't think we care about the case where it has a value of null, false, 0, or the empty string. But I don't feel strongly about it. https://codereview.chromium.org/207553008/diff/280001/chrome/renderer/net/net... File chrome/renderer/net/net_error_helper_core.cc (right): https://codereview.chromium.org/207553008/diff/280001/chrome/renderer/net/net... chrome/renderer/net/net_error_helper_core.cc:514: if (pending_error_page_info_) { This should have an "&& frame_type == MAIN_FRAME". I believe my CL will rework this a bit by splitting up the subframe and main frame cases, so it will end up being marginally less ugly...in some respects, at least. https://codereview.chromium.org/207553008/diff/280001/chrome/renderer/resourc... File chrome/renderer/resources/neterror.html (right): https://codereview.chromium.org/207553008/diff/280001/chrome/renderer/resourc... chrome/renderer/resources/neterror.html:60: </button> nit: This should line up with the corresponding open tag.
Scott: Would you be willing to stamp this based on mmenke's approval? This is specifically for the following files: * errorpage_browsertest.cc * chrome/common/localized_error.* * chrome/common/net/net_error_info.* * chrome/renderer/resources/neterror.* I (and I think he) trusts his judgement on everything but neterror.*, and functionality wise on neterror.*. The UI change to neterror.* is small (adding a button to an error page in certain specific circumstances) and is going to be run by UI review as soon as this CL lands. WDYT?
On 2014/04/17 15:06:53, rdsmith wrote: > Scott: Would you be willing to stamp this based on mmenke's approval? This is > specifically for the following files: > * errorpage_browsertest.cc > * chrome/common/localized_error.* > * chrome/common/net/net_error_info.* > * chrome/renderer/resources/neterror.* > > I (and I think he) trusts his judgement on everything but neterror.*, and > functionality wise on neterror.*. The UI change to neterror.* is small (adding > a button to an error page in certain specific circumstances) and is going to be > run by UI review as soon as this CL lands. > > WDYT? To be clear: I will be addressing Matt's most recent comments, just wanting to get the top level stamp in motion now that I have his LGTM.
errorpage_browsertest.cc chrome/common/localized_error.* chrome/common/net/net_error_info.* LGTM For chrome/renderer/resources/neterror.* please get a better owner. https://codereview.chromium.org/207553008/diff/280001/chrome/browser/errorpag... File chrome/browser/errorpage_browsertest.cc (right): https://codereview.chromium.org/207553008/diff/280001/chrome/browser/errorpag... chrome/browser/errorpage_browsertest.cc:141: std::string LoadStaleButtonLabel() { I was confused by this name is it some what implies it's doing loading. How prefixing with Get? https://codereview.chromium.org/207553008/diff/280001/chrome/common/net/net_e... File chrome/common/net/net_error_info.h (right): https://codereview.chromium.org/207553008/diff/280001/chrome/common/net/net_e... chrome/common/net/net_error_info.h:12: NET_ERROR_PAGE_SHOWN, // Error pages shown. I would prefer you followed the convention in content and didn't shorten these, eg NETWORK_ERROR_...
Thanks; Scott; will incorporate your comments. NIco, willing to take a look at neterror.*? The last stamp I need ... :-}
Comments from Matt & Scott incorporated. https://codereview.chromium.org/207553008/diff/240001/chrome/renderer/resourc... File chrome/renderer/resources/neterror.js (right): https://codereview.chromium.org/207553008/diff/240001/chrome/renderer/resourc... chrome/renderer/resources/neterror.js:83: } On 2014/04/17 14:39:25, mmenke wrote: > On 2014/04/15 18:27:29, rdsmith wrote: > > On 2014/04/15 16:02:25, mmenke wrote: > > > Can't you just use: > > > > > > if (errorPageController) { > > > errorPageController.blah(); > > > } > > > > When I disable the Gin bindings (to mimc IOS) and try that, I get: > > > > [14801:14801:0415/142343:INFO:CONSOLE(386)] "Uncaught ReferenceError: > > errorPageController is not defined", source: data:text/html,chromewebdata > (386) > > Ah, right. "window.errorPageController" would work, though - I prefer it for > being more C-like, and I don't think we care about the case where it has a value > of null, false, 0, or the empty string. But I don't feel strongly about it. Huh. Learn something new every day; thank you. Done. https://codereview.chromium.org/207553008/diff/280001/chrome/browser/errorpag... File chrome/browser/errorpage_browsertest.cc (right): https://codereview.chromium.org/207553008/diff/280001/chrome/browser/errorpag... chrome/browser/errorpage_browsertest.cc:141: std::string LoadStaleButtonLabel() { On 2014/04/17 15:26:54, sky wrote: > I was confused by this name is it some what implies it's doing loading. How > prefixing with Get? Done. https://codereview.chromium.org/207553008/diff/280001/chrome/common/net/net_e... File chrome/common/net/net_error_info.h (right): https://codereview.chromium.org/207553008/diff/280001/chrome/common/net/net_e... chrome/common/net/net_error_info.h:12: NET_ERROR_PAGE_SHOWN, // Error pages shown. On 2014/04/17 15:26:54, sky wrote: > I would prefer you followed the convention in content and didn't shorten these, > eg NETWORK_ERROR_... My 80 columns!! :-} Done. https://codereview.chromium.org/207553008/diff/280001/chrome/renderer/net/net... File chrome/renderer/net/net_error_helper_core.cc (right): https://codereview.chromium.org/207553008/diff/280001/chrome/renderer/net/net... chrome/renderer/net/net_error_helper_core.cc:514: if (pending_error_page_info_) { On 2014/04/17 14:39:25, mmenke wrote: > This should have an "&& frame_type == MAIN_FRAME". I believe my CL will rework > this a bit by splitting up the subframe and main frame cases, so it will end up > being marginally less ugly...in some respects, at least. Done. https://codereview.chromium.org/207553008/diff/280001/chrome/renderer/resourc... File chrome/renderer/resources/neterror.html (right): https://codereview.chromium.org/207553008/diff/280001/chrome/renderer/resourc... chrome/renderer/resources/neterror.html:60: </button> On 2014/04/17 14:39:25, mmenke wrote: > nit: This should line up with the corresponding open tag. Done.
neterror lgtm stamp based on mmenke's review https://codereview.chromium.org/207553008/diff/340001/chrome/renderer/resourc... File chrome/renderer/resources/neterror.js (right): https://codereview.chromium.org/207553008/diff/340001/chrome/renderer/resourc... chrome/renderer/resources/neterror.js:65: // IOS. iOS, not IOS. iOS is Apple's mobile operating system, IOS is Cisco's router OS (http://en.wikipedia.org/wiki/Cisco_IOS)
Thanks! https://codereview.chromium.org/207553008/diff/340001/chrome/renderer/resourc... File chrome/renderer/resources/neterror.js (right): https://codereview.chromium.org/207553008/diff/340001/chrome/renderer/resourc... chrome/renderer/resources/neterror.js:65: // IOS. On 2014/04/17 16:51:47, Nico wrote: > iOS, not IOS. iOS is Apple's mobile operating system, IOS is Cisco's router OS > (http://en.wikipedia.org/wiki/Cisco_IOS) Done.
The CQ bit was checked by rdsmith@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/rdsmith@chromium.org/207553008/360001
On 2014/04/17 18:49:52, I haz the power (commit-bot) wrote: > CQ is trying da patch. Follow status at > https://chromium-status.appspot.com/cq/rdsmith@chromium.org/207553008/360001 Looks like your last tryjob died after completing successfully.
Message was sent while issue was closed.
Change committed as 264696
Message was sent while issue was closed.
https://codereview.chromium.org/207553008/diff/360001/chrome/common/net/net_e... File chrome/common/net/net_error_info.cc (right): https://codereview.chromium.org/207553008/diff/360001/chrome/common/net/net_e... chrome/common/net/net_error_info.cc:41: UMA_HISTOGRAM_ENUMERATION("Net.ErrorPageCounts", event, Please add an entry in tools/metrics/histograms/histograms.xml for this (as part of this CL). https://codereview.chromium.org/207553008/diff/360001/chrome/renderer/net/net... File chrome/renderer/net/net_error_helper_core.cc (right): https://codereview.chromium.org/207553008/diff/360001/chrome/renderer/net/net... chrome/renderer/net/net_error_helper_core.cc:379: navigation_from_button_ == RELOAD_BUTTON ? nit: This will fit on the previous line...and then you can avoid the questionable double-indent you applied on the next two line (standard 4 space indent is probably more style compliant). https://codereview.chromium.org/207553008/diff/360001/chrome/renderer/net/net... File chrome/renderer/net/net_error_helper_core.h (right): https://codereview.chromium.org/207553008/diff/360001/chrome/renderer/net/net... chrome/renderer/net/net_error_helper_core.h:216: // Non-NO_BUTTON only when a navigation has been initiated from the error nit: I couldn't parse the comment without working hard (the double negative hurt me a tad). Perhaps better would be: |navigation_from_button_| is true when RELOAD, LOAD_STALE, or MORE _BUTTON was pressed. It is used to detect when such navigations result in errors. https://codereview.chromium.org/207553008/diff/360001/chrome/renderer/net/net... File chrome/renderer/net/net_error_helper_core_unittest.cc (right): https://codereview.chromium.org/207553008/diff/360001/chrome/renderer/net/net... chrome/renderer/net/net_error_helper_core_unittest.cc:117: NetErrorHelperCoreTest() : timer_(new base::MockTimer(false, false)), nit: When we have to put initializers on multiple lines, put them all on following lines, with the ":" indented 4 spaces, and then align initializers under the first one. https://codereview.chromium.org/207553008/diff/360001/chrome/renderer/net/net... File chrome/renderer/net/net_error_page_controller.cc (right): https://codereview.chromium.org/207553008/diff/360001/chrome/renderer/net/net... chrome/renderer/net/net_error_page_controller.cc:80: isolate) nit: This should only be indented 4 from the start of the line. If you really like this style, you can use an open paren before "gin" to establish a parenthesized group. https://codereview.chromium.org/207553008/diff/360001/chrome/renderer/net/net... File chrome/renderer/net/net_error_page_controller.h (right): https://codereview.chromium.org/207553008/diff/360001/chrome/renderer/net/net... chrome/renderer/net/net_error_page_controller.h:20: class NetErrorPageController nit: Should this be in the net namespace? https://codereview.chromium.org/207553008/diff/360001/chrome/renderer/resourc... File chrome/renderer/resources/neterror.js (right): https://codereview.chromium.org/207553008/diff/360001/chrome/renderer/resourc... chrome/renderer/resources/neterror.js:77: } nit: No need for curlies (per style used in this file).
Message was sent while issue was closed.
https://codereview.chromium.org/207553008/diff/360001/chrome/renderer/net/net... File chrome/renderer/net/net_error_helper_core.cc (right): https://codereview.chromium.org/207553008/diff/360001/chrome/renderer/net/net... chrome/renderer/net/net_error_helper_core.cc:379: navigation_from_button_ == RELOAD_BUTTON ? On 2014/04/21 23:48:19, jar wrote: > nit: This will fit on the previous line...and then you can avoid the > questionable double-indent you applied on the next two line (standard 4 space > indent is probably more style compliant). While the Google style guide doesn't explicitly discuss single arguments that take up multiple-lines, this does seem to fit more closely what it does say than what you suggest. The rules it gives: "If the arguments do not all fit on one line, they should be broken up onto multiple lines, with each subsequent line aligned with the first argument. Do not add spaces after the open paren or before the close paren" "Arguments may optionally all be placed on subsequent lines, with one line per argument" In particular, your suggestion seems to violate the "with each subsequent line aligned with the first argument". (This follows it, with the addendum that the subsequent lines of each argument are indented 4 spaces)
Message was sent while issue was closed.
https://codereview.chromium.org/207553008/diff/360001/chrome/renderer/net/net... File chrome/renderer/net/net_error_helper_core.cc (right): https://codereview.chromium.org/207553008/diff/360001/chrome/renderer/net/net... chrome/renderer/net/net_error_helper_core.cc:379: navigation_from_button_ == RELOAD_BUTTON ? On 2014/04/22 00:24:59, mmenke wrote: > On 2014/04/21 23:48:19, jar wrote: > > nit: This will fit on the previous line...and then you can avoid the > > questionable double-indent you applied on the next two line (standard 4 space > > indent is probably more style compliant). > > While the Google style guide doesn't explicitly discuss single arguments that > take up multiple-lines, this does seem to fit more closely what it does say than > what you suggest. > > The rules it gives: > > "If the arguments do not all fit on one line, they should be broken up onto > multiple lines, with each subsequent line aligned with the first argument. Do > not add spaces after the open paren or before the close paren" > > "Arguments may optionally all be placed on subsequent lines, with one line per > argument" > > In particular, your suggestion seems to violate the "with each subsequent line > aligned with the first argument". (This follows it, with the addendum that the > subsequent lines of each argument are indented 4 spaces) If you really prefer to start a new line... that's fine... nit: ... just don't indent the next two lines "extra" because there is a "?" on the previous line. This is all one monster line continuation, and there is no reason to indent further on additional lines
Message was sent while issue was closed.
I've actually incorporated the comments I respond to below in https://codereview.chromium.org/248123002/, also adding the histograms (in the proper place) in that CL. Please review and respond on that CL. https://codereview.chromium.org/207553008/diff/360001/chrome/common/net/net_e... File chrome/common/net/net_error_info.cc (right): https://codereview.chromium.org/207553008/diff/360001/chrome/common/net/net_e... chrome/common/net/net_error_info.cc:41: UMA_HISTOGRAM_ENUMERATION("Net.ErrorPageCounts", event, On 2014/04/21 23:48:19, jar wrote: > Please add an entry in tools/metrics/histograms/histograms.xml for this (as part > of this CL). Apologies; as noted elsewhere, I was confused about the fact that we still had a public histograms repository. https://codereview.chromium.org/207553008/diff/360001/chrome/renderer/net/net... File chrome/renderer/net/net_error_helper_core.cc (right): https://codereview.chromium.org/207553008/diff/360001/chrome/renderer/net/net... chrome/renderer/net/net_error_helper_core.cc:379: navigation_from_button_ == RELOAD_BUTTON ? On 2014/04/22 17:53:43, jar wrote: > On 2014/04/22 00:24:59, mmenke wrote: > > On 2014/04/21 23:48:19, jar wrote: > > > nit: This will fit on the previous line...and then you can avoid the > > > questionable double-indent you applied on the next two line (standard 4 > space > > > indent is probably more style compliant). > > > > While the Google style guide doesn't explicitly discuss single arguments that > > take up multiple-lines, this does seem to fit more closely what it does say > than > > what you suggest. > > > > The rules it gives: > > > > "If the arguments do not all fit on one line, they should be broken up onto > > multiple lines, with each subsequent line aligned with the first argument. Do > > not add spaces after the open paren or before the close paren" > > > > "Arguments may optionally all be placed on subsequent lines, with one line per > > argument" > > > > In particular, your suggestion seems to violate the "with each subsequent line > > aligned with the first argument". (This follows it, with the addendum that > the > > subsequent lines of each argument are indented 4 spaces) > > If you really prefer to start a new line... that's fine... > > nit: ... just don't indent the next two lines "extra" because there is a "?" on > the previous line. This is all one monster line continuation, and there is no > reason to indent further on additional lines I formatted it this way in response to Matt's comment in https://codereview.chromium.org/207553008/diff/210001/chrome/renderer/net/net... . I don't particularly care, so I'm happy to do whatever you and Matt agree on, but in the absence of an agreement, I'll leave it the way I've already landed it. https://codereview.chromium.org/207553008/diff/360001/chrome/renderer/net/net... File chrome/renderer/net/net_error_helper_core.h (right): https://codereview.chromium.org/207553008/diff/360001/chrome/renderer/net/net... chrome/renderer/net/net_error_helper_core.h:216: // Non-NO_BUTTON only when a navigation has been initiated from the error On 2014/04/21 23:48:19, jar wrote: > nit: I couldn't parse the comment without working hard (the double negative hurt > me a tad). Perhaps better would be: > > |navigation_from_button_| is true when RELOAD, LOAD_STALE, or MORE _BUTTON was > pressed. It is used to detect when such navigations result in errors. That doesn't quite capture what I'm going for, but I gave something else a try. Let me know what you think. https://codereview.chromium.org/207553008/diff/360001/chrome/renderer/net/net... File chrome/renderer/net/net_error_helper_core_unittest.cc (right): https://codereview.chromium.org/207553008/diff/360001/chrome/renderer/net/net... chrome/renderer/net/net_error_helper_core_unittest.cc:117: NetErrorHelperCoreTest() : timer_(new base::MockTimer(false, false)), On 2014/04/21 23:48:19, jar wrote: > nit: When we have to put initializers on multiple lines, put them all on > following lines, with the ":" indented 4 spaces, and then align initializers > under the first one. I'm a little reluctant, since this wasn't code I originally wrote or that I changed in this CL. Matt, do you have an opinion? I think of this as your code. https://codereview.chromium.org/207553008/diff/360001/chrome/renderer/net/net... File chrome/renderer/net/net_error_page_controller.cc (right): https://codereview.chromium.org/207553008/diff/360001/chrome/renderer/net/net... chrome/renderer/net/net_error_page_controller.cc:80: isolate) On 2014/04/21 23:48:19, jar wrote: > nit: This should only be indented 4 from the start of the line. If you really > like this style, you can use an open paren before "gin" to establish a > parenthesized group. Done. https://codereview.chromium.org/207553008/diff/360001/chrome/renderer/net/net... File chrome/renderer/net/net_error_page_controller.h (right): https://codereview.chromium.org/207553008/diff/360001/chrome/renderer/net/net... chrome/renderer/net/net_error_page_controller.h:20: class NetErrorPageController On 2014/04/21 23:48:19, jar wrote: > nit: Should this be in the net namespace? I followed the model of NetErrorHelper, which isn't. That makes sense to me, as we're nowhere near src/net, which is what I think of the net namespace as meaning. https://codereview.chromium.org/207553008/diff/360001/chrome/renderer/resourc... File chrome/renderer/resources/neterror.js (right): https://codereview.chromium.org/207553008/diff/360001/chrome/renderer/resourc... chrome/renderer/resources/neterror.js:77: } On 2014/04/21 23:48:19, jar wrote: > nit: No need for curlies (per style used in this file). Done.
Message was sent while issue was closed.
https://codereview.chromium.org/207553008/diff/360001/chrome/renderer/net/net... File chrome/renderer/net/net_error_helper_core.cc (right): https://codereview.chromium.org/207553008/diff/360001/chrome/renderer/net/net... chrome/renderer/net/net_error_helper_core.cc:379: navigation_from_button_ == RELOAD_BUTTON ? On 2014/04/22 20:40:02, rdsmith wrote: > On 2014/04/22 17:53:43, jar wrote: > > On 2014/04/22 00:24:59, mmenke wrote: > > > On 2014/04/21 23:48:19, jar wrote: > > > > nit: This will fit on the previous line...and then you can avoid the > > > > questionable double-indent you applied on the next two line (standard 4 > > space > > > > indent is probably more style compliant). > > > > > > While the Google style guide doesn't explicitly discuss single arguments > that > > > take up multiple-lines, this does seem to fit more closely what it does say > > than > > > what you suggest. > > > > > > The rules it gives: > > > > > > "If the arguments do not all fit on one line, they should be broken up onto > > > multiple lines, with each subsequent line aligned with the first argument. > Do > > > not add spaces after the open paren or before the close paren" > > > > > > "Arguments may optionally all be placed on subsequent lines, with one line > per > > > argument" > > > > > > In particular, your suggestion seems to violate the "with each subsequent > line > > > aligned with the first argument". (This follows it, with the addendum that > > the > > > subsequent lines of each argument are indented 4 spaces) > > > > If you really prefer to start a new line... that's fine... > > > > nit: ... just don't indent the next two lines "extra" because there is a "?" > on > > the previous line. This is all one monster line continuation, and there is no > > reason to indent further on additional lines > > I formatted it this way in response to Matt's comment in > https://codereview.chromium.org/207553008/diff/210001/chrome/renderer/net/net... > . I don't particularly care, so I'm happy to do whatever you and Matt agree on, > but in the absence of an agreement, I'll leave it the way I've already landed > it. The style guide says nothing about this case, so we can't really rely on it. I find this a lot clearer because it makes it clear these are continuation lines, rather than separate parameters. I don't think jar and I are going to reach consensus on this, other than perhaps by searching the codebase and finding out what the most common style is. One option is just to write out a whole else/if statement, I guess. https://codereview.chromium.org/207553008/diff/360001/chrome/renderer/net/net... File chrome/renderer/net/net_error_helper_core_unittest.cc (right): https://codereview.chromium.org/207553008/diff/360001/chrome/renderer/net/net... chrome/renderer/net/net_error_helper_core_unittest.cc:117: NetErrorHelperCoreTest() : timer_(new base::MockTimer(false, false)), On 2014/04/22 20:40:02, rdsmith wrote: > On 2014/04/21 23:48:19, jar wrote: > > nit: When we have to put initializers on multiple lines, put them all on > > following lines, with the ":" indented 4 spaces, and then align initializers > > under the first one. > > I'm a little reluctant, since this wasn't code I originally wrote or that I > changed in this CL. Matt, do you have an opinion? I think of this as your > code. I'm fine with either style. Looking at the C++ style guide, it looks like jar's unambiguously right on this one. https://codereview.chromium.org/207553008/diff/360001/chrome/renderer/net/net... File chrome/renderer/net/net_error_page_controller.h (right): https://codereview.chromium.org/207553008/diff/360001/chrome/renderer/net/net... chrome/renderer/net/net_error_page_controller.h:20: class NetErrorPageController On 2014/04/22 20:40:02, rdsmith wrote: > On 2014/04/21 23:48:19, jar wrote: > > nit: Should this be in the net namespace? > > I followed the model of NetErrorHelper, which isn't. That makes sense to me, as > we're nowhere near src/net, which is what I think of the net namespace as > meaning. > You're correct - neither browser/net nor renderer/net should be in the net namespace.
Message was sent while issue was closed.
(In https://codereview.chromium.org/248123002/). https://codereview.chromium.org/207553008/diff/360001/chrome/renderer/net/net... File chrome/renderer/net/net_error_helper_core_unittest.cc (right): https://codereview.chromium.org/207553008/diff/360001/chrome/renderer/net/net... chrome/renderer/net/net_error_helper_core_unittest.cc:117: NetErrorHelperCoreTest() : timer_(new base::MockTimer(false, false)), On 2014/04/22 21:03:27, mmenke wrote: > On 2014/04/22 20:40:02, rdsmith wrote: > > On 2014/04/21 23:48:19, jar wrote: > > > nit: When we have to put initializers on multiple lines, put them all on > > > following lines, with the ":" indented 4 spaces, and then align initializers > > > under the first one. > > > > I'm a little reluctant, since this wasn't code I originally wrote or that I > > changed in this CL. Matt, do you have an opinion? I think of this as your > > code. > > I'm fine with either style. Looking at the C++ style guide, it looks like jar's > unambiguously right on this one. Done.
Message was sent while issue was closed.
Thanks for the nit responses! LGTM % discussion. (whether in this, or in another CL). https://codereview.chromium.org/207553008/diff/360001/chrome/renderer/net/net... File chrome/renderer/net/net_error_helper_core.cc (right): https://codereview.chromium.org/207553008/diff/360001/chrome/renderer/net/net... chrome/renderer/net/net_error_helper_core.cc:379: navigation_from_button_ == RELOAD_BUTTON ? On 2014/04/22 21:03:27, mmenke wrote: > On 2014/04/22 20:40:02, rdsmith wrote: > > On 2014/04/22 17:53:43, jar wrote: > > > On 2014/04/22 00:24:59, mmenke wrote: > > > > On 2014/04/21 23:48:19, jar wrote: > > > > > nit: This will fit on the previous line...and then you can avoid the > > > > > questionable double-indent you applied on the next two line (standard 4 > > > space > > > > > indent is probably more style compliant). > > > > > > > > While the Google style guide doesn't explicitly discuss single arguments > > that > > > > take up multiple-lines, this does seem to fit more closely what it does > say > > > than > > > > what you suggest. > > > > > > > > The rules it gives: > > > > > > > > "If the arguments do not all fit on one line, they should be broken up > onto > > > > multiple lines, with each subsequent line aligned with the first argument. > > Do > > > > not add spaces after the open paren or before the close paren" > > > > > > > > "Arguments may optionally all be placed on subsequent lines, with one line > > per > > > > argument" > > > > > > > > In particular, your suggestion seems to violate the "with each subsequent > > line > > > > aligned with the first argument". (This follows it, with the addendum > that > > > the > > > > subsequent lines of each argument are indented 4 spaces) > > > > > > If you really prefer to start a new line... that's fine... > > > > > > nit: ... just don't indent the next two lines "extra" because there is a > "?" > > on > > > the previous line. This is all one monster line continuation, and there is > no > > > reason to indent further on additional lines > > > > I formatted it this way in response to Matt's comment in > > > https://codereview.chromium.org/207553008/diff/210001/chrome/renderer/net/net... > > . I don't particularly care, so I'm happy to do whatever you and Matt agree > on, > > but in the absence of an agreement, I'll leave it the way I've already landed > > it. > > The style guide says nothing about this case, so we can't really rely on it. > > I find this a lot clearer because it makes it clear these are continuation > lines, rather than separate parameters. I don't think jar and I are going to > reach consensus on this, other than perhaps by searching the codebase and > finding out what the most common style is. > > One option is just to write out a whole else/if statement, I guess. As noted, this is a nit. I couldn't find any reason for adding additional indentation. To be honest, I think your indentation makes some sense... I just couldn't find any justification for doing so... and could see that IF you did this... then you could keep on indenting more and more "whenever it feels right" ... and I didn't like where that was going. When I see extra indents, I try to figure out why, as "I must have missed something." Bottom line: Leave it as is if you feel strongly that it helps more than it hurts (folks like me that are surprised by it). https://codereview.chromium.org/207553008/diff/360001/chrome/renderer/net/net... File chrome/renderer/net/net_error_page_controller.h (right): https://codereview.chromium.org/207553008/diff/360001/chrome/renderer/net/net... chrome/renderer/net/net_error_page_controller.h:20: class NetErrorPageController On 2014/04/22 21:03:27, mmenke wrote: > On 2014/04/22 20:40:02, rdsmith wrote: > > On 2014/04/21 23:48:19, jar wrote: > > > nit: Should this be in the net namespace? > > > > I followed the model of NetErrorHelper, which isn't. That makes sense to me, > as > > we're nowhere near src/net, which is what I think of the net namespace as > > meaning. > > > > You're correct - neither browser/net nor renderer/net should be in the net > namespace. You're correct. This is not in the src/net/... path.... I didn't read the path name well. Sorry. I researched namespaces standards in Chromium (reading threads about it)... and couldn't find a justification to land it anywhere specific... but the name, starting with "NetError" sure seemed to confuse me. Most other classes in this directory (other than NetErrorHelper*) have names that don't resemble well know libraries... I'll get over it... and get used to it. Sorry for the diversion. |
