Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
422933002:
Shift the error page "More" button over to text. (Closed)
|
Created:
6 years, 4 months ago by Randy Smith (Not in Mondays) Modified:
6 years, 4 months ago CC:
arv+watch_chromium.org, chromium-reviews, nkostylev+watch_chromium.org, oshima+watch_chromium.org, sky, stevenjb+watch_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Project:
chromium Visibility:
Public. |
DescriptionShift the error page "More" button over to text and float to side.
Note that this also involved copying neterror.css/js over to
chrome/browser/resources/ssl so as to break the dependency between
blocking.html in that directory and the css/js files in
chrome/renderer/resources. That copy means that, while I'm going
through presubmit errors before uploading, I"m not fixing them all--
there are pre-existing errors in neterror.css that I don't have the
expertise to fix.
BUG=None
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=287538
Patch Set 1 #Patch Set 2 : Sync'd to r286338 #Patch Set 3 : Intermediate patch. #Patch Set 4 : Incorporated UI feedback. #
Total comments: 19
Patch Set 5 : Incorporated comments. #
Total comments: 14
Patch Set 6 : Next round of comments incorporated. #
Total comments: 16
Patch Set 7 : Most recent set of comments. #
Total comments: 17
Patch Set 8 : Incorporated comments. #
Total comments: 2
Patch Set 9 : Incorporated comments. #
Total comments: 27
Patch Set 10 : Incorporeated comments from Matt. #
Total comments: 4
Patch Set 11 : Incorporated dbeam's comments. #Patch Set 12 : Fix various problems. #Patch Set 13 : Sync'd to r287354. #
Total comments: 8
Patch Set 14 : Incorporated Matt & Adrienne's comments. #Patch Set 15 : Sync'c to r287451 to get past removal of chrome/browser/resources/ssl/blocking.html and thus the pr… #
Messages
Total messages: 49 (0 generated)
Matt: PTAL? Adrienne: Your comments are welcome if you'd like to make them, but the only changes here that I'd think of you as being needed for reviewing are for the files in chrome/browser/resources/ssl, and they are pretty much direct copies of what used to be in chrome/renderer/resources with some (not all :-J) of the presubmit errors fixed. So this is primarily FYI.
On 2014/08/01 14:30:54, rdsmith wrote: > Matt: PTAL? > > Adrienne: Your comments are welcome if you'd like to make them, but the only > changes here that I'd think of you as being needed for reviewing are for the > files in chrome/browser/resources/ssl, and they are pretty much direct copies of > what used to be in chrome/renderer/resources with some (not all :-J) of the > presubmit errors fixed. So this is primarily FYI. Eee...Need some linebreaks in that description.
On 2014/08/01 14:31:34, mmenke wrote: > On 2014/08/01 14:30:54, rdsmith wrote: > > Matt: PTAL? > > > > Adrienne: Your comments are welcome if you'd like to make them, but the only > > changes here that I'd think of you as being needed for reviewing are for the > > files in chrome/browser/resources/ssl, and they are pretty much direct copies > of > > what used to be in chrome/renderer/resources with some (not all :-J) of the > > presubmit errors fixed. So this is primarily FYI. > > Eee...Need some linebreaks in that description. Done.
On 2014/08/01 14:31:34, mmenke wrote: > On 2014/08/01 14:30:54, rdsmith wrote: > > Matt: PTAL? > > > > Adrienne: Your comments are welcome if you'd like to make them, but the only > > changes here that I'd think of you as being needed for reviewing are for the > > files in chrome/browser/resources/ssl, and they are pretty much direct copies > of > > what used to be in chrome/renderer/resources with some (not all :-J) of the > > presubmit errors fixed. So this is primarily FYI. > > Eee...Need some linebreaks in that description. Going to apply and play with this locally this morning - I find CSS pretty much impossible to review without seeing it used, and inspecting the DOM.
On 2014/08/01 14:34:37, mmenke wrote: > On 2014/08/01 14:31:34, mmenke wrote: > > On 2014/08/01 14:30:54, rdsmith wrote: > > > Matt: PTAL? > > > > > > Adrienne: Your comments are welcome if you'd like to make them, but the only > > > changes here that I'd think of you as being needed for reviewing are for the > > > files in chrome/browser/resources/ssl, and they are pretty much direct > copies > > of > > > what used to be in chrome/renderer/resources with some (not all :-J) of the > > > presubmit errors fixed. So this is primarily FYI. > > > > Eee...Need some linebreaks in that description. > > Going to apply and play with this locally this morning - I find CSS pretty much > impossible to review without seeing it used, and inspecting the DOM. Sounds good. Let me know if you want the extra patch I use for testing (for generating net errors dynamically in a running chrome). One extra question: In neterror.css I added -webkit-small-control to get the Details link to be the same font size as the buttons. But I'm not completely happy with that because <dogmeme>I have no idea what I'm doing</dogmeme>. Could you take an extra look at that, and if you understand what's going on, explain it to me? :-} :-|
On 2014/08/01 14:40:11, rdsmith wrote: > On 2014/08/01 14:34:37, mmenke wrote: > > On 2014/08/01 14:31:34, mmenke wrote: > > > On 2014/08/01 14:30:54, rdsmith wrote: > > > > Matt: PTAL? > > > > > > > > Adrienne: Your comments are welcome if you'd like to make them, but the > only > > > > changes here that I'd think of you as being needed for reviewing are for > the > > > > files in chrome/browser/resources/ssl, and they are pretty much direct > > copies > > > of > > > > what used to be in chrome/renderer/resources with some (not all :-J) of > the > > > > presubmit errors fixed. So this is primarily FYI. > > > > > > Eee...Need some linebreaks in that description. > > > > Going to apply and play with this locally this morning - I find CSS pretty > much > > impossible to review without seeing it used, and inspecting the DOM. > > Sounds good. Let me know if you want the extra patch I use for testing (for > generating net errors dynamically in a running chrome). > > One extra question: In neterror.css I added -webkit-small-control to get the > Details link to be the same font size as the buttons. But I'm not completely > happy with that because <dogmeme>I have no idea what I'm doing</dogmeme>. Could > you take an extra look at that, and if you understand what's going on, explain > it to me? :-} :-| That's the thing that decided me on needing to see the page in action, actually, so yes, definitely going to take an extra look at that. I don't have a build of chrome quite as ready to apply and go as I thought I did, so may be a while before I can get back to you, unfortunately.
On 2014/08/01 14:43:54, mmenke wrote: > On 2014/08/01 14:40:11, rdsmith wrote: > > On 2014/08/01 14:34:37, mmenke wrote: > > > On 2014/08/01 14:31:34, mmenke wrote: > > > > On 2014/08/01 14:30:54, rdsmith wrote: > > > > > Matt: PTAL? > > > > > > > > > > Adrienne: Your comments are welcome if you'd like to make them, but the > > only > > > > > changes here that I'd think of you as being needed for reviewing are for > > the > > > > > files in chrome/browser/resources/ssl, and they are pretty much direct > > > copies > > > > of > > > > > what used to be in chrome/renderer/resources with some (not all :-J) of > > the > > > > > presubmit errors fixed. So this is primarily FYI. > > > > > > > > Eee...Need some linebreaks in that description. > > > > > > Going to apply and play with this locally this morning - I find CSS pretty > > much > > > impossible to review without seeing it used, and inspecting the DOM. > > > > Sounds good. Let me know if you want the extra patch I use for testing (for > > generating net errors dynamically in a running chrome). > > > > One extra question: In neterror.css I added -webkit-small-control to get the > > Details link to be the same font size as the buttons. But I'm not completely > > happy with that because <dogmeme>I have no idea what I'm doing</dogmeme>. > Could > > you take an extra look at that, and if you understand what's going on, explain > > it to me? :-} :-| > > That's the thing that decided me on needing to see the page in action, actually, > so yes, definitely going to take an extra look at that. I don't have a build of > chrome quite as ready to apply and go as I thought I did, so may be a while > before I can get back to you, unfortunately. I'll just give the rational for the change then, for context: In my initial screenshots to chrome-ui-review I hadn't noticed that the Reload button and Details link didn't have the same font size. When I went back and inspected with devtools, they showed up as having the same font size; they were both inheriting from #buttons. I went through and did semi-exhaustive search, turning off properties in the CSS inheritance hierarchy for the button until the button jumped to the same size as the Details link, and the property I had just disabled was -webkit-small-control. So I added -webkit-small-control to the Details link :-J. No worries on the response time; I'll keep an eye on email through the day.
https://codereview.chromium.org/422933002/diff/60001/chrome/app/generated_res... File chrome/app/generated_resources.grd (left): https://codereview.chromium.org/422933002/diff/60001/chrome/app/generated_res... chrome/app/generated_resources.grd:8537: <message name="IDS_ERRORPAGES_BUTTON_MORE" desc="Label for the button that expands the details on an error page"> Should these strings be renamed? https://codereview.chromium.org/422933002/diff/60001/chrome/renderer/resource... File chrome/renderer/resources/neterror.css (right): https://codereview.chromium.org/422933002/diff/60001/chrome/renderer/resource... chrome/renderer/resources/neterror.css:197: font: -webkit-small-control; I don't like using css properties with webkit prefixes - unclear how long they'll be alive, and even when we get rid of them, there's still the issue of Safari (Most of the other webkit properties are now supported unprefixed on Chrome, but weren't yet on iOS, last check). Maybe just set the size explicitly? Using font-size: 13px results in nearly the same size font on windows, at least. https://codereview.chromium.org/422933002/diff/60001/chrome/renderer/resource... chrome/renderer/resources/neterror.css:377: .salient-right.salient-button { I've never seen salient used in this context. Default? Suggested? https://codereview.chromium.org/422933002/diff/60001/chrome/renderer/resource... chrome/renderer/resources/neterror.css:377: .salient-right.salient-button { Neither salient-right nor salient-left seems to be used in the HTML. I'm seeing two centered side-by-side buttons. Is this what I should be seeing? https://codereview.chromium.org/422933002/diff/60001/chrome/renderer/resource... chrome/renderer/resources/neterror.css:378: float:right; nit: These should all have spaces after the colon. https://codereview.chromium.org/422933002/diff/60001/chrome/renderer/resource... chrome/renderer/resources/neterror.css:399: content: ''; What does this do? https://codereview.chromium.org/422933002/diff/60001/chrome/renderer/resource... chrome/renderer/resources/neterror.css:400: display: table; I don't think you need to use both float and a table layout.
https://codereview.chromium.org/422933002/diff/60001/chrome/renderer/resource... File chrome/renderer/resources/neterror.css (right): https://codereview.chromium.org/422933002/diff/60001/chrome/renderer/resource... chrome/renderer/resources/neterror.css:377: .salient-right.salient-button { On 2014/08/01 15:50:42, mmenke wrote: > Neither salient-right nor salient-left seems to be used in the HTML. I'm seeing > two centered side-by-side buttons. Is this what I should be seeing? Oops...That should be neither "salient-right not salient-button". See why salient-right is not (See comment in other file), but what's salient-button for? https://codereview.chromium.org/422933002/diff/60001/chrome/renderer/resource... File chrome/renderer/resources/neterror.js (right): https://codereview.chromium.org/422933002/diff/60001/chrome/renderer/resource... chrome/renderer/resources/neterror.js:121: buttonDiv.className = "salient-left"; Oh...missed this file. You need to use salient-right in the HTML as well...
Matt: PTAL? https://codereview.chromium.org/422933002/diff/60001/chrome/app/generated_res... File chrome/app/generated_resources.grd (left): https://codereview.chromium.org/422933002/diff/60001/chrome/app/generated_res... chrome/app/generated_resources.grd:8537: <message name="IDS_ERRORPAGES_BUTTON_MORE" desc="Label for the button that expands the details on an error page"> On 2014/08/01 15:50:42, mmenke wrote: > Should these strings be renamed? Done. https://codereview.chromium.org/422933002/diff/60001/chrome/renderer/resource... File chrome/renderer/resources/neterror.css (right): https://codereview.chromium.org/422933002/diff/60001/chrome/renderer/resource... chrome/renderer/resources/neterror.css:197: font: -webkit-small-control; On 2014/08/01 15:50:42, mmenke wrote: > I don't like using css properties with webkit prefixes - unclear how long > they'll be alive, and even when we get rid of them, there's still the issue of > Safari (Most of the other webkit properties are now supported unprefixed on > Chrome, but weren't yet on iOS, last check). > > Maybe just set the size explicitly? Using font-size: 13px results in nearly the > same size font on windows, at least. So this makes me nervous specifically because I don't grok the webkit-small-control property, and I'm trying to match it with a specific font size. Two forms of nervousness: a) I feel like if I do this I'd sorta have to check every single platform we have to make sure it matches (in case webkit-small-control is setup differently on different platforms), and b) if at some point in the future the definition of webkit-small-control changes, we'll silently break here. I hear you about IOS, though. How would you feel about my overriding the font for both Details and Reload to force them to be the same (and neither be webkit-small-control)? https://codereview.chromium.org/422933002/diff/60001/chrome/renderer/resource... chrome/renderer/resources/neterror.css:377: .salient-right.salient-button { On 2014/08/01 15:50:42, mmenke wrote: > Neither salient-right nor salient-left seems to be used in the HTML. I'm seeing > two centered side-by-side buttons. Is this what I should be seeing? So you should *not* be seeing two centered side-by-side buttons. I'm looking for two buttons floated to the opposite ends of the box. That's very interesting. I suspect it's because you're on windows and I'm not; I'll see if I can force testing of that one before uploading the next PS. The styles in question should be set in the JS. Having said that, I don't seem to be setting salient button; I'll look into that. https://codereview.chromium.org/422933002/diff/60001/chrome/renderer/resource... chrome/renderer/resources/neterror.css:377: .salient-right.salient-button { On 2014/08/01 15:52:51, mmenke wrote: > On 2014/08/01 15:50:42, mmenke wrote: > > Neither salient-right nor salient-left seems to be used in the HTML. I'm > seeing > > two centered side-by-side buttons. Is this what I should be seeing? > > Oops...That should be neither "salient-right not salient-button". See why > salient-right is not (See comment in other file), but what's salient-button for? So I played around with this and gave up on being clever; I'm now just setting styles directly on the elements from JS. I wanted to do something with inheritance & classes, but a) classes themselves don't appear to be inherited in the DOM, only the styles they pull in for the node they apply to, and b) if I set any kind of "float" style on the #buttons div, it pops the buttons out of the div. I'm happy to solve this general problem (remapping float: left/right back and forth between the two sets of buttons) any way you'd like, as long as it works :-}. Specifically, I'm happy to set classes directly rather than styles, but given the level of detail I was already on, I didn't see much point in the indirection. But let me know what you'd like. https://codereview.chromium.org/422933002/diff/60001/chrome/renderer/resource... chrome/renderer/resources/neterror.css:377: .salient-right.salient-button { On 2014/08/01 15:50:42, mmenke wrote: > I've never seen salient used in this context. Default? Suggested? Moot given rewrite. https://codereview.chromium.org/422933002/diff/60001/chrome/renderer/resource... chrome/renderer/resources/neterror.css:378: float:right; On 2014/08/01 15:50:42, mmenke wrote: > nit: These should all have spaces after the colon. Done. https://codereview.chromium.org/422933002/diff/60001/chrome/renderer/resource... chrome/renderer/resources/neterror.css:399: content: ''; On 2014/08/01 15:50:42, mmenke wrote: > What does this do? No clue; copied from https://developer.mozilla.org/en-US/docs/Web/CSS/clear. My guess would be that it sets the content of the newly created pseudo-node to nothing. It's necessary, though; without it the buttons float outside the box. https://codereview.chromium.org/422933002/diff/60001/chrome/renderer/resource... chrome/renderer/resources/neterror.css:400: display: table; On 2014/08/01 15:50:42, mmenke wrote: > I don't think you need to use both float and a table layout. You're right--done. https://codereview.chromium.org/422933002/diff/60001/chrome/renderer/resource... File chrome/renderer/resources/neterror.js (right): https://codereview.chromium.org/422933002/diff/60001/chrome/renderer/resource... chrome/renderer/resources/neterror.js:121: buttonDiv.className = "salient-left"; On 2014/08/01 15:52:51, mmenke wrote: > Oh...missed this file. You need to use salient-right in the HTML as well... Moot given rewrite, but my intent was that the JS would be the only thing that set salient-right, and the css file would pick it up.
https://codereview.chromium.org/422933002/diff/60001/chrome/renderer/resource... File chrome/renderer/resources/neterror.css (right): https://codereview.chromium.org/422933002/diff/60001/chrome/renderer/resource... chrome/renderer/resources/neterror.css:197: font: -webkit-small-control; On 2014/08/01 18:14:15, rdsmith wrote: > On 2014/08/01 15:50:42, mmenke wrote: > > I don't like using css properties with webkit prefixes - unclear how long > > they'll be alive, and even when we get rid of them, there's still the issue of > > Safari (Most of the other webkit properties are now supported unprefixed on > > Chrome, but weren't yet on iOS, last check). > > > > Maybe just set the size explicitly? Using font-size: 13px results in nearly > the > > same size font on windows, at least. > > So this makes me nervous specifically because I don't grok the > webkit-small-control property, and I'm trying to match it with a specific font > size. Two forms of nervousness: a) I feel like if I do this I'd sorta have to > check every single platform we have to make sure it matches (in case > webkit-small-control is setup differently on different platforms), and b) if at > some point in the future the definition of webkit-small-control changes, we'll > silently break here. I hear you about IOS, though. > > How would you feel about my overriding the font for both Details and Reload to > force them to be the same (and neither be webkit-small-control)? Isn't that just what I suggested? Either way, sounds good to me. https://codereview.chromium.org/422933002/diff/80001/chrome/renderer/resource... File chrome/renderer/resources/neterror.html (right): https://codereview.chromium.org/422933002/diff/80001/chrome/renderer/resource... chrome/renderer/resources/neterror.html:24: class="blue-button text-button" Don't use tabs. https://codereview.chromium.org/422933002/diff/80001/chrome/renderer/resource... File chrome/renderer/resources/neterror.js (right): https://codereview.chromium.org/422933002/diff/80001/chrome/renderer/resource... chrome/renderer/resources/neterror.js:104: // on the right. This comment is no longer accurate. It's used everywhere. Also doesn't just reorder the buttons anymore. https://codereview.chromium.org/422933002/diff/80001/chrome/renderer/resource... chrome/renderer/resources/neterror.js:113: var primary_leftmost = true; nit: primaryLeftmost... Or maybe primaryControlOnLeft? https://codereview.chromium.org/422933002/diff/80001/chrome/renderer/resource... chrome/renderer/resources/neterror.js:120: buttonDiv.insertBefore(secondary, detailsButton); Suggestion: Put the buttons in their own div, and just set the float style of that instead of the two buttons independently. Also, then you just need to flip the order of the buttons, you don't need to move them relative to the details link, I believe, as the float left/right should take care of that. https://codereview.chromium.org/422933002/diff/80001/chrome/renderer/resource... chrome/renderer/resources/neterror.js:121: primary.style['float'] = "left"; Use single quotes for JS strings. https://codereview.chromium.org/422933002/diff/80001/chrome/renderer/resource... chrome/renderer/resources/neterror.js:129: detailsButton.style['float'] = "left"; I could be wrong, but I suspect you're going to get some pushback from setting styles directly, rather than using CSS. https://codereview.chromium.org/422933002/diff/80001/chrome/renderer/resource... chrome/renderer/resources/neterror.js:134: detailsButton.style['float'] = "center"; I don't believe there is a such thing as float: center.
Matt: PTAL? (Thank you, by the way, for the prompt turnaround on this--I don't assume that's easy, and I'm grateful.) https://codereview.chromium.org/422933002/diff/80001/chrome/renderer/resource... File chrome/renderer/resources/neterror.html (right): https://codereview.chromium.org/422933002/diff/80001/chrome/renderer/resource... chrome/renderer/resources/neterror.html:24: class="blue-button text-button" On 2014/08/01 18:29:33, mmenke wrote: > Don't use tabs. Done. https://codereview.chromium.org/422933002/diff/80001/chrome/renderer/resource... File chrome/renderer/resources/neterror.js (right): https://codereview.chromium.org/422933002/diff/80001/chrome/renderer/resource... chrome/renderer/resources/neterror.js:104: // on the right. On 2014/08/01 18:29:33, mmenke wrote: > This comment is no longer accurate. It's used everywhere. Also doesn't just > reorder the buttons anymore. Done. https://codereview.chromium.org/422933002/diff/80001/chrome/renderer/resource... chrome/renderer/resources/neterror.js:113: var primary_leftmost = true; On 2014/08/01 18:29:33, mmenke wrote: > nit: primaryLeftmost... Or maybe primaryControlOnLeft? Done. https://codereview.chromium.org/422933002/diff/80001/chrome/renderer/resource... chrome/renderer/resources/neterror.js:120: buttonDiv.insertBefore(secondary, detailsButton); On 2014/08/01 18:29:33, mmenke wrote: > Suggestion: Put the buttons in their own div, and just set the float style of > that instead of the two buttons independently. > > Also, then you just need to flip the order of the buttons, you don't need to > move them relative to the details link, I believe, as the float left/right > should take care of that. Thank you for nudging me again on this suggestions; it simplifies the code. https://codereview.chromium.org/422933002/diff/80001/chrome/renderer/resource... chrome/renderer/resources/neterror.js:121: primary.style['float'] = "left"; On 2014/08/01 18:29:33, mmenke wrote: > Use single quotes for JS strings. Done. https://codereview.chromium.org/422933002/diff/80001/chrome/renderer/resource... chrome/renderer/resources/neterror.js:129: detailsButton.style['float'] = "left"; On 2014/08/01 18:29:33, mmenke wrote: > I could be wrong, but I suspect you're going to get some pushback from setting > styles directly, rather than using CSS. I switched over to CSS, and combined with your suggestion above, it may be a *little* cleaner :-}. But that's just one person's opinion, and I'm happy to do the change just based on likely pushback (which I have no problem believing in). https://codereview.chromium.org/422933002/diff/80001/chrome/renderer/resource... chrome/renderer/resources/neterror.js:134: detailsButton.style['float'] = "center"; On 2014/08/01 18:29:33, mmenke wrote: > I don't believe there is a such thing as float: center. Done.
LGTM. Have you tested it with both layouts? If not, should do so, just to be safe. https://codereview.chromium.org/422933002/diff/100001/chrome/renderer/resourc... File chrome/renderer/resources/neterror.css (right): https://codereview.chromium.org/422933002/diff/100001/chrome/renderer/resourc... chrome/renderer/resources/neterror.css:183: /* Override the user agent specification of webkit-small-control. */ nit: Looks like you have an extra space before the */ https://codereview.chromium.org/422933002/diff/100001/chrome/renderer/resourc... chrome/renderer/resources/neterror.css:398: width: 100%; Does adding "visibility: hidden; " make a difference to spacing or not? (Just Googled around randomly about clear + after + floats, and saw it used as well) https://codereview.chromium.org/422933002/diff/100001/chrome/renderer/resourc... File chrome/renderer/resources/neterror.html (right): https://codereview.chromium.org/422933002/diff/100001/chrome/renderer/resourc... chrome/renderer/resources/neterror.html:23: <div id="control-buttons"> optional: Maybe rename the outer div controls, and the inner one buttons? Up to you. https://codereview.chromium.org/422933002/diff/100001/chrome/renderer/resourc... chrome/renderer/resources/neterror.html:36: </div> Replace tabs with spaces. https://codereview.chromium.org/422933002/diff/100001/chrome/renderer/resourc... File chrome/renderer/resources/neterror.js (right): https://codereview.chromium.org/422933002/diff/100001/chrome/renderer/resourc... chrome/renderer/resources/neterror.js:103: // Sets up the proper button layout on all platforms. nit: Maybe "Sets up the proper button layout for the current platform." https://codereview.chromium.org/422933002/diff/100001/chrome/renderer/resourc... chrome/renderer/resources/neterror.js:111: var secondary = staleLoadButton; optional: Suggest primaryButton / secondaryButton. https://codereview.chromium.org/422933002/diff/100001/chrome/renderer/resourc... chrome/renderer/resources/neterror.js:126: controlButtonDiv.className += additionalClass; Can this be moved into the body of the if as well? Hrm...If so, could actually just set the property of the parent, and switch your selectors to .suggested-left>#control-buttons and the like...that's probably just making things too fancy and complicated, though. https://codereview.chromium.org/422933002/diff/100001/chrome/renderer/resourc... chrome/renderer/resources/neterror.js:129: detailsButton.className += additionalClass; Use "detailsButton.classList.add(additionalClass);" Can then get rid of the leading spaces on your class names, too.
Thanks very much! I did rewrite the float style stuff one more time based on your suggestion, so you might want to give it another scan, but I'll move on to stamps on the strength of your LGTM. > Have you tested it with both layouts? If not, should do so, just to be safe. If you mean, did I force primaryControlOnLeft = true as well as false, yes, I've been testing with that since I realized you were seeing different behavior on Windows in terms of floating than I was on linux. I haven't been testing on anything but Linux, though, just setting that value to true in a debug branch. https://codereview.chromium.org/422933002/diff/100001/chrome/renderer/resourc... File chrome/renderer/resources/neterror.css (right): https://codereview.chromium.org/422933002/diff/100001/chrome/renderer/resourc... chrome/renderer/resources/neterror.css:183: /* Override the user agent specification of webkit-small-control. */ On 2014/08/01 20:56:51, mmenke wrote: > nit: Looks like you have an extra space before the */ Done. https://codereview.chromium.org/422933002/diff/100001/chrome/renderer/resourc... chrome/renderer/resources/neterror.css:398: width: 100%; On 2014/08/01 20:56:51, mmenke wrote: > Does adding "visibility: hidden; " make a difference to spacing or not? (Just > Googled around randomly about clear + after + floats, and saw it used as well) I don't see any difference. https://codereview.chromium.org/422933002/diff/100001/chrome/renderer/resourc... File chrome/renderer/resources/neterror.html (right): https://codereview.chromium.org/422933002/diff/100001/chrome/renderer/resourc... chrome/renderer/resources/neterror.html:23: <div id="control-buttons"> On 2014/08/01 20:56:51, mmenke wrote: > optional: Maybe rename the outer div controls, and the inner one buttons? Up > to you. So I'm still calling the Details link a details button (following stuff I've seen in other such implementations, which may have been historical) so I'm inclined against. https://codereview.chromium.org/422933002/diff/100001/chrome/renderer/resourc... chrome/renderer/resources/neterror.html:36: </div> On 2014/08/01 20:56:51, mmenke wrote: > Replace tabs with spaces. Arggh--gotta do some emacs hacking (emacs does this automatically for me for .cpp files). Done. https://codereview.chromium.org/422933002/diff/100001/chrome/renderer/resourc... File chrome/renderer/resources/neterror.js (right): https://codereview.chromium.org/422933002/diff/100001/chrome/renderer/resourc... chrome/renderer/resources/neterror.js:103: // Sets up the proper button layout on all platforms. On 2014/08/01 20:56:51, mmenke wrote: > nit: Maybe "Sets up the proper button layout for the current platform." Done. https://codereview.chromium.org/422933002/diff/100001/chrome/renderer/resourc... chrome/renderer/resources/neterror.js:111: var secondary = staleLoadButton; On 2014/08/01 20:56:51, mmenke wrote: > optional: Suggest primaryButton / secondaryButton. Done. https://codereview.chromium.org/422933002/diff/100001/chrome/renderer/resourc... chrome/renderer/resources/neterror.js:126: controlButtonDiv.className += additionalClass; On 2014/08/01 20:56:51, mmenke wrote: > Can this be moved into the body of the if as well? (Other than the paragraph below) Why? I still have to set additionalClass for use in the if conditional below, and I'd rather not pull that in because duplicating it across the else would be pretty verbose. > Hrm...If so, could actually just set the property of the parent, and switch your > selectors to .suggested-left>#control-buttons and the like...that's probably > just making things too fancy and complicated, though. Actually, that might have been the syntax I was looking for; I'd rather do that, if it works. <later> Hmm. The CSS is just as wordy, but we save a bit of JS, so done. https://codereview.chromium.org/422933002/diff/100001/chrome/renderer/resourc... chrome/renderer/resources/neterror.js:129: detailsButton.className += additionalClass; On 2014/08/01 20:56:51, mmenke wrote: > Use "detailsButton.classList.add(additionalClass);" > > Can then get rid of the leading spaces on your class names, too. Thanks; I didn't know about that syntax. Done.
Scott: Willing to stamp (or review) now that I've got Matt's ok? The stuff in chrome/browser is just copies + a partial set of fixes based on the presubmit checks; the real meat of the CL is in chrome/renderer. Honesty compels me to call your attention to the CL description--I am pushing forward on this CL despite presubmit errors from CSS checks. I did not create any of those errors (they existed previously and presubmit's picking up on stuff because I created a copy of a file), and I'm reluctant to fix them because I don't understand the sections of CSS that they're referring to (they want me to use flex/linear-gradient instead of webkit-flex/webkit-linear-gradient, and I don't think it's just a straight text substitution). blocking.html and dependents is also slated for deletion by Adrienne Felt at some point in the near/medium future, so I'm reluctant to put a lot of effort into cleanup. I'm happy to put effort into cleanup of neterror.js if you'd like, but I'd rather do that after feature freeze.
Jochen: Willing to take a look at this? I'm going to retarget to you from sky@ since he was OOO late last week and may have stuff to catch up on, and is caught up in Moveaggedon in MTV early next week. See previous comment for context and warnings. Matt's primary reviewer, but you'll want to make a judgement call as to how detailed your review should be. Stamping is fine by me.
https://codereview.chromium.org/422933002/diff/120001/chrome/browser/resource... File chrome/browser/resources/ssl/neterror.css (right): https://codereview.chromium.org/422933002/diff/120001/chrome/browser/resource... chrome/browser/resources/ssl/neterror.css:7: * Note: This is also used by a file in chrome/browser/resources/ssl. When This comment makes less sense in a file that is in c/b/r/ssl. https://codereview.chromium.org/422933002/diff/120001/chrome/renderer/resourc... File chrome/renderer/resources/neterror.css (right): https://codereview.chromium.org/422933002/diff/120001/chrome/renderer/resourc... chrome/renderer/resources/neterror.css:382: .suggested-left > #control-buttons { You could combine these selectors where the properties match. https://codereview.chromium.org/422933002/diff/120001/chrome/renderer/resourc... File chrome/renderer/resources/neterror.html (right): https://codereview.chromium.org/422933002/diff/120001/chrome/renderer/resourc... chrome/renderer/resources/neterror.html:37: <a href="#" id="details-button" class="active-text" Any reason why this is a link instead of a button styled like a link? https://codereview.chromium.org/422933002/diff/120001/chrome/renderer/resourc... File chrome/renderer/resources/neterror.js (right): https://codereview.chromium.org/422933002/diff/120001/chrome/renderer/resourc... chrome/renderer/resources/neterror.js:115: primaryControlOnLeft = false; Could you do this check purely in JS? Having grit expressions in the middle of Javascript makes me queasy. https://codereview.chromium.org/422933002/diff/120001/chrome/renderer/resourc... chrome/renderer/resources/neterror.js:128: detailsButton.classList.add('singular'); This needs to be indented two spaces less.
https://codereview.chromium.org/422933002/diff/120001/chrome/browser/resource... File chrome/browser/resources/ssl/neterror.css (right): https://codereview.chromium.org/422933002/diff/120001/chrome/browser/resource... chrome/browser/resources/ssl/neterror.css:7: * Note: This is also used by a file in chrome/browser/resources/ssl. When On 2014/08/04 11:49:18, Bernhard Bauer wrote: > This comment makes less sense in a file that is in c/b/r/ssl. And is no longer accurate, even in c/r/r. Removed in both places. https://codereview.chromium.org/422933002/diff/120001/chrome/renderer/resourc... File chrome/renderer/resources/neterror.css (right): https://codereview.chromium.org/422933002/diff/120001/chrome/renderer/resourc... chrome/renderer/resources/neterror.css:382: .suggested-left > #control-buttons { On 2014/08/04 11:49:18, Bernhard Bauer wrote: > You could combine these selectors where the properties match. Done, but please do give it another look as I'm not confident in my selector-foo. https://codereview.chromium.org/422933002/diff/120001/chrome/renderer/resourc... File chrome/renderer/resources/neterror.html (right): https://codereview.chromium.org/422933002/diff/120001/chrome/renderer/resourc... chrome/renderer/resources/neterror.html:37: <a href="#" id="details-button" class="active-text" On 2014/08/04 11:49:18, Bernhard Bauer wrote: > Any reason why this is a link instead of a button styled like a link? I was modeling after chrome/browser/resources/interstitial_v2.html. Given that I have this working now and that it took some fiddling to get things to match link vs. button and I'm hoping to land today, I'm reluctant to try and change it in this CL. But if you think that's a better way to do this, I'm happy to go back and fix it after I land. https://codereview.chromium.org/422933002/diff/120001/chrome/renderer/resourc... File chrome/renderer/resources/neterror.js (right): https://codereview.chromium.org/422933002/diff/120001/chrome/renderer/resourc... chrome/renderer/resources/neterror.js:115: primaryControlOnLeft = false; On 2014/08/04 11:49:18, Bernhard Bauer wrote: > Could you do this check purely in JS? Having grit expressions in the middle of > Javascript makes me queasy. Would it be better if I hoisted it up outside the function and put it around the declaration of a file level variable? (Sorta parallel to what used to be here.) There might be a way to do this in JS, but if I shift over to using JS to detect the platform, that sorta requires me to test it on every single platform to make sure that the probe behaves properly, whereas if I keep the old mechanism I can presume the old behavior hasn't changed. https://codereview.chromium.org/422933002/diff/120001/chrome/renderer/resourc... chrome/renderer/resources/neterror.js:128: detailsButton.classList.add('singular'); On 2014/08/04 11:49:18, Bernhard Bauer wrote: > This needs to be indented two spaces less. Whoops, sorry. Done.
https://codereview.chromium.org/422933002/diff/120001/chrome/renderer/resourc... File chrome/renderer/resources/neterror.css (right): https://codereview.chromium.org/422933002/diff/120001/chrome/renderer/resourc... chrome/renderer/resources/neterror.css:382: .suggested-left > #control-buttons { On 2014/08/04 12:21:27, rdsmith wrote: > On 2014/08/04 11:49:18, Bernhard Bauer wrote: > > You could combine these selectors where the properties match. > > Done, but please do give it another look as I'm not confident in my > selector-foo. Yup, this is fine. I personally would break after the comma, so it's a bit clearer how the two selectors correspond. https://codereview.chromium.org/422933002/diff/120001/chrome/renderer/resourc... File chrome/renderer/resources/neterror.html (right): https://codereview.chromium.org/422933002/diff/120001/chrome/renderer/resourc... chrome/renderer/resources/neterror.html:37: <a href="#" id="details-button" class="active-text" On 2014/08/04 12:21:27, rdsmith wrote: > On 2014/08/04 11:49:18, Bernhard Bauer wrote: > > Any reason why this is a link instead of a button styled like a link? > > I was modeling after chrome/browser/resources/interstitial_v2.html. > > Given that I have this working now and that it took some fiddling to get things > to match link vs. button and I'm hoping to land today, I'm reluctant to try and > change it in this CL. But if you think that's a better way to do this, I'm > happy to go back and fix it after I land. > There are some potential issues with using links: They will trigger a navigation that shows up in the back/forward history, the # will show up in the URL, they can be opened in a new tab which will open a second copy of the page, etc. Not all of these apply here though (this is just an error page that does not show a URL or traditional navigation controls, right?) and some of them can be worked around, but generally it's easier not to have to think about this and use a styled button instead. But coming back to your original point, I'm fine with keeping the link in this CL if you have confirmed that it will do what you want. https://codereview.chromium.org/422933002/diff/120001/chrome/renderer/resourc... File chrome/renderer/resources/neterror.js (right): https://codereview.chromium.org/422933002/diff/120001/chrome/renderer/resourc... chrome/renderer/resources/neterror.js:115: primaryControlOnLeft = false; On 2014/08/04 12:21:27, rdsmith wrote: > On 2014/08/04 11:49:18, Bernhard Bauer wrote: > > Could you do this check purely in JS? Having grit expressions in the middle of > > Javascript makes me queasy. > > Would it be better if I hoisted it up outside the function and put it around the > declaration of a file level variable? (Sorta parallel to what used to be here.) > There might be a way to do this in JS, but if I shift over to using JS to > detect the platform, that sorta requires me to test it on every single platform > to make sure that the probe behaves properly, whereas if I keep the old > mechanism I can presume the old behavior hasn't changed. Hm, fair point. Yeah, I think hoisting it outside of the function would make things a bit easier to parse. Then longer-term, I feel we should move these sort of platform-dependent UI decisions to some shared file, but that's not necessarily something that needs to be done by you, or in this CL. https://codereview.chromium.org/422933002/diff/140001/chrome/renderer/resourc... File chrome/renderer/resources/neterror.js (right): https://codereview.chromium.org/422933002/diff/140001/chrome/renderer/resourc... chrome/renderer/resources/neterror.js:127: staleLoadButton.style.display == 'none') { No, this one is still part of the condition, so it needs to be indented four spaces. The next line is body, so two spaces.
https://codereview.chromium.org/422933002/diff/120001/chrome/renderer/resourc... File chrome/renderer/resources/neterror.css (right): https://codereview.chromium.org/422933002/diff/120001/chrome/renderer/resourc... chrome/renderer/resources/neterror.css:382: .suggested-left > #control-buttons { On 2014/08/04 13:00:12, Bernhard Bauer wrote: > On 2014/08/04 12:21:27, rdsmith wrote: > > On 2014/08/04 11:49:18, Bernhard Bauer wrote: > > > You could combine these selectors where the properties match. > > > > Done, but please do give it another look as I'm not confident in my > > selector-foo. > > Yup, this is fine. I personally would break after the comma, so it's a bit > clearer how the two selectors correspond. Done. https://codereview.chromium.org/422933002/diff/120001/chrome/renderer/resourc... File chrome/renderer/resources/neterror.html (right): https://codereview.chromium.org/422933002/diff/120001/chrome/renderer/resourc... chrome/renderer/resources/neterror.html:37: <a href="#" id="details-button" class="active-text" On 2014/08/04 13:00:12, Bernhard Bauer wrote: > On 2014/08/04 12:21:27, rdsmith wrote: > > On 2014/08/04 11:49:18, Bernhard Bauer wrote: > > > Any reason why this is a link instead of a button styled like a link? > > > > I was modeling after chrome/browser/resources/interstitial_v2.html. > > > > Given that I have this working now and that it took some fiddling to get > things > > to match link vs. button and I'm hoping to land today, I'm reluctant to try > and > > change it in this CL. But if you think that's a better way to do this, I'm > > happy to go back and fix it after I land. > > > > There are some potential issues with using links: They will trigger a navigation > that shows up in the back/forward history, the # will show up in the URL, they > can be opened in a new tab which will open a second copy of the page, etc. Not > all of these apply here though (this is just an error page that does not show a > URL or traditional navigation controls, right?) and some of them can be worked > around, but generally it's easier not to have to think about this and use a > styled button instead. > > But coming back to your original point, I'm fine with keeping the link in this > CL if you have confirmed that it will do what you want. Hmmm. I tested the history behavior, and you're quite right, an entry does get added to the history :-{. Ok, I'm on board with shifting over to a styled button. Ok for me to do it in a follow-up CL, or is that cheating? If cheating, what does your schedule today look like/are you ok with my switching to a different reviewer when you sign off for the day? https://codereview.chromium.org/422933002/diff/120001/chrome/renderer/resourc... File chrome/renderer/resources/neterror.js (right): https://codereview.chromium.org/422933002/diff/120001/chrome/renderer/resourc... chrome/renderer/resources/neterror.js:115: primaryControlOnLeft = false; On 2014/08/04 13:00:12, Bernhard Bauer wrote: > On 2014/08/04 12:21:27, rdsmith wrote: > > On 2014/08/04 11:49:18, Bernhard Bauer wrote: > > > Could you do this check purely in JS? Having grit expressions in the middle > of > > > Javascript makes me queasy. > > > > Would it be better if I hoisted it up outside the function and put it around > the > > declaration of a file level variable? (Sorta parallel to what used to be > here.) > > There might be a way to do this in JS, but if I shift over to using JS to > > detect the platform, that sorta requires me to test it on every single > platform > > to make sure that the probe behaves properly, whereas if I keep the old > > mechanism I can presume the old behavior hasn't changed. > > Hm, fair point. Yeah, I think hoisting it outside of the function would make > things a bit easier to parse. > > Then longer-term, I feel we should move these sort of platform-dependent UI > decisions to some shared file, but that's not necessarily something that needs > to be done by you, or in this CL. Done. I thought about just wrapping a declaration in grit, but I didn't want to be checking for existence of a particular key on the document hash, so I just hoisted what's currently there. https://codereview.chromium.org/422933002/diff/140001/chrome/renderer/resourc... File chrome/renderer/resources/neterror.js (right): https://codereview.chromium.org/422933002/diff/140001/chrome/renderer/resourc... chrome/renderer/resources/neterror.js:127: staleLoadButton.style.display == 'none') { On 2014/08/04 13:00:12, Bernhard Bauer wrote: > No, this one is still part of the condition, so it needs to be indented four > spaces. The next line is body, so two spaces. This is a combination of getting up too early this morning (for me :-}), moving too fast, and not having my emacs configured properly for JS :-{. Done.
LGTM if you want to fix the navigation issue in a followup CL, otherwise see below. https://codereview.chromium.org/422933002/diff/120001/chrome/renderer/resourc... File chrome/renderer/resources/neterror.html (right): https://codereview.chromium.org/422933002/diff/120001/chrome/renderer/resourc... chrome/renderer/resources/neterror.html:37: <a href="#" id="details-button" class="active-text" On 2014/08/04 13:14:24, rdsmith wrote: > On 2014/08/04 13:00:12, Bernhard Bauer wrote: > > On 2014/08/04 12:21:27, rdsmith wrote: > > > On 2014/08/04 11:49:18, Bernhard Bauer wrote: > > > > Any reason why this is a link instead of a button styled like a link? > > > > > > I was modeling after chrome/browser/resources/interstitial_v2.html. > > > > > > Given that I have this working now and that it took some fiddling to get > > things > > > to match link vs. button and I'm hoping to land today, I'm reluctant to try > > and > > > change it in this CL. But if you think that's a better way to do this, I'm > > > happy to go back and fix it after I land. > > > > > > > There are some potential issues with using links: They will trigger a > navigation > > that shows up in the back/forward history, the # will show up in the URL, they > > can be opened in a new tab which will open a second copy of the page, etc. Not > > all of these apply here though (this is just an error page that does not show > a > > URL or traditional navigation controls, right?) and some of them can be worked > > around, but generally it's easier not to have to think about this and use a > > styled button instead. > > > > But coming back to your original point, I'm fine with keeping the link in this > > CL if you have confirmed that it will do what you want. > > Hmmm. I tested the history behavior, and you're quite right, an entry does get > added to the history :-{. Ok, I'm on board with shifting over to a styled > button. Ok for me to do it in a follow-up CL, or is that cheating? If > cheating, what does your schedule today look like/are you ok with my switching > to a different reviewer when you sign off for the day? > Either is fine with me. If you want to keep it as a link, you can work around this with preventDefaultOnPoundLinkClicks() in ui/webui/resources/js/util.js, or you make it a link-button (CSS in ui/webui/resource/css/widgets.cc). And if you want to do that in a followup CL, that's okay with me too (maybe leave a TODO in here), or you do it in this one, and ping me when you want me to review stuff. If it's just small deltas, I can look at them any time during the evening (my time, i.e. GMT+1).
rubberstamp lgtm
No need for another signoff from me - It's clear Bernhard knows what he's doing (Which I certainly do not). https://codereview.chromium.org/422933002/diff/160001/chrome/renderer/resourc... File chrome/renderer/resources/neterror.css (right): https://codereview.chromium.org/422933002/diff/160001/chrome/renderer/resourc... chrome/renderer/resources/neterror.css:374: button, it should be centered. */ I don't think I've seen this style used in Chrome. It's generally: /** * Really, REALLY important stuff * that takes up more than one line. */ (Sometimes with an extra asterisk before the end comment tag, sometimes without the extra asterisk on the first line, sometimes without the top line). https://codereview.chromium.org/422933002/diff/160001/chrome/renderer/resourc... File chrome/renderer/resources/neterror.js (right): https://codereview.chromium.org/422933002/diff/160001/chrome/renderer/resourc... chrome/renderer/resources/neterror.js:103: // Sets up the proper button layout for the current platform. This should be just above setButtonLayout.
Sounds good. There turn out to be try job failures (I was too focussed on the review) so I'll need to nail those; I may need another round from you for whatever changes are involved with that. https://codereview.chromium.org/422933002/diff/160001/chrome/renderer/resourc... File chrome/renderer/resources/neterror.css (right): https://codereview.chromium.org/422933002/diff/160001/chrome/renderer/resourc... chrome/renderer/resources/neterror.css:374: button, it should be centered. */ On 2014/08/04 15:57:39, mmenke wrote: > I don't think I've seen this style used in Chrome. It's generally: > > /** > * Really, REALLY important stuff > * that takes up more than one line. > */ > > (Sometimes with an extra asterisk before the end comment tag, sometimes without > the extra asterisk on the first line, sometimes without the top line). Ok. I don't consider that comment really really important (anymore), so I've shortened it to a single line. https://codereview.chromium.org/422933002/diff/160001/chrome/renderer/resourc... File chrome/renderer/resources/neterror.js (right): https://codereview.chromium.org/422933002/diff/160001/chrome/renderer/resourc... chrome/renderer/resources/neterror.js:103: // Sets up the proper button layout for the current platform. On 2014/08/04 15:57:39, mmenke wrote: > This should be just above setButtonLayout. Done.
https://codereview.chromium.org/422933002/diff/160001/chrome/renderer/resourc... File chrome/renderer/resources/neterror.css (right): https://codereview.chromium.org/422933002/diff/160001/chrome/renderer/resourc... chrome/renderer/resources/neterror.css:374: button, it should be centered. */ On 2014/08/04 16:14:41, rdsmith wrote: > On 2014/08/04 15:57:39, mmenke wrote: > > I don't think I've seen this style used in Chrome. It's generally: > > > > /** > > * Really, REALLY important stuff > > * that takes up more than one line. > > */ > > > > (Sometimes with an extra asterisk before the end comment tag, sometimes > without > > the extra asterisk on the first line, sometimes without the top line). > > Ok. I don't consider that comment really really important (anymore), so I've > shortened it to a single line. That wasn't meant to be a dig at the importance of the comment, for the record. I was trying to be silly.
On 2014/08/04 16:18:03, mmenke wrote: > https://codereview.chromium.org/422933002/diff/160001/chrome/renderer/resourc... > File chrome/renderer/resources/neterror.css (right): > > https://codereview.chromium.org/422933002/diff/160001/chrome/renderer/resourc... > chrome/renderer/resources/neterror.css:374: button, it should be centered. */ > On 2014/08/04 16:14:41, rdsmith wrote: > > On 2014/08/04 15:57:39, mmenke wrote: > > > I don't think I've seen this style used in Chrome. It's generally: > > > > > > /** > > > * Really, REALLY important stuff > > > * that takes up more than one line. > > > */ > > > > > > (Sometimes with an extra asterisk before the end comment tag, sometimes > > without > > > the extra asterisk on the first line, sometimes without the top line). > > > > Ok. I don't consider that comment really really important (anymore), so I've > > shortened it to a single line. > > That wasn't meant to be a dig at the importance of the comment, for the record. > I was trying to be silly. Ah, ok. I'm just in "incorporate comments as fast as possible unless they're going to cause me hurt" mode :-}. I may go back and replace the comment; when I looked at it, I didn't think the style needed a lot of explanation, but I've been pretty immersed in the style issues the last couple of days.
https://codereview.chromium.org/422933002/diff/160001/chrome/renderer/resourc... File chrome/renderer/resources/neterror.css (right): https://codereview.chromium.org/422933002/diff/160001/chrome/renderer/resourc... chrome/renderer/resources/neterror.css:374: button, it should be centered. */ On 2014/08/04 15:57:39, mmenke wrote: > I don't think I've seen this style used in Chrome. It's generally: > > /** > * Really, REALLY important stuff > * that takes up more than one line. > */ > > (Sometimes with an extra asterisk before the end comment tag, sometimes without > the extra asterisk on the first line, sometimes without the top line). I don't have a strong opinion on which exact style you should use, but I would rather not use the double-asterisk one, as that is generally used for semantic comments (Javadoc, JSDoc). I don't think there is an equivalent for CSS, but I wouldn't want to create the impression that it is supposed to be processed.
re: your css comment question (and drive-by nits) https://codereview.chromium.org/422933002/diff/160001/chrome/renderer/resourc... File chrome/renderer/resources/neterror.css (right): https://codereview.chromium.org/422933002/diff/160001/chrome/renderer/resourc... chrome/renderer/resources/neterror.css:52: margin: 10px 0 30px 0; 10px 0 30px; https://codereview.chromium.org/422933002/diff/160001/chrome/renderer/resourc... chrome/renderer/resources/neterror.css:259: border-bottom-left-radius: 0; this is probably wrong in RTL https://codereview.chromium.org/422933002/diff/160001/chrome/renderer/resourc... chrome/renderer/resources/neterror.css:312: margin: 10px 0 15px 0; margin: 10px 0 15px; https://codereview.chromium.org/422933002/diff/160001/chrome/renderer/resourc... chrome/renderer/resources/neterror.css:373: if they should float left or right. If there's only a details opt nit: 1\s between comments https://codereview.chromium.org/422933002/diff/160001/chrome/renderer/resourc... chrome/renderer/resources/neterror.css:374: button, it should be centered. */ On 2014/08/04 16:32:17, Bernhard Bauer wrote: > On 2014/08/04 15:57:39, mmenke wrote: > > I don't think I've seen this style used in Chrome. It's generally: > > > > /** > > * Really, REALLY important stuff > > * that takes up more than one line. > > */ > > > > (Sometimes with an extra asterisk before the end comment tag, sometimes > without > > the extra asterisk on the first line, sometimes without the top line). > > I don't have a strong opinion on which exact style you should use, but I would > rather not use the double-asterisk one, as that is generally used for semantic > comments (Javadoc, JSDoc). I don't think there is an equivalent for CSS, but I > wouldn't want to create the impression that it is supposed to be processed. cssdoc and closure stylesheets do parse /** * A doc comment. * @require {some.namespace} */ comments. if you don't want this to be picked up by a doc tool, change to /* Some long comment * that spans lines. */ like our copyright headers. the \s\* at the beginning of each line is solely conventional, but one that's pretty consistent throughout webui. https://codereview.chromium.org/422933002/diff/160001/chrome/renderer/resourc... chrome/renderer/resources/neterror.css:394: } why are you using float and a clearfix instead of flexbox + flexbox ordering (if necessary)? https://codereview.chromium.org/422933002/diff/160001/chrome/renderer/resourc... File chrome/renderer/resources/neterror.html (right): https://codereview.chromium.org/422933002/diff/160001/chrome/renderer/resourc... chrome/renderer/resources/neterror.html:38: onclick="detailsButtonClick(); toggleHelpBox()" onclick= :'( https://codereview.chromium.org/422933002/diff/160001/chrome/renderer/resourc... File chrome/renderer/resources/neterror.js (right): https://codereview.chromium.org/422933002/diff/160001/chrome/renderer/resourc... chrome/renderer/resources/neterror.js:13: } nit: no curlies https://codereview.chromium.org/422933002/diff/160001/chrome/renderer/resourc... chrome/renderer/resources/neterror.js:118: var primaryControlOnLeft = true; probably meant to remove this? https://codereview.chromium.org/422933002/diff/160001/chrome/renderer/resourc... chrome/renderer/resources/neterror.js:120: if (primaryControlOnLeft) { nit: <if expr="is_win> buttons.classList.add('suggested-right'); controlButtonDiv.insertBefore(secondaryButton, primaryButton); </if> <if expr="not is_win"> buttons.classList.add('suggested-left'); controlButtonDiv.insertBefore(primaryButton, secondaryButton); </if> https://codereview.chromium.org/422933002/diff/180001/chrome/renderer/resourc... File chrome/renderer/resources/neterror.js (right): https://codereview.chromium.org/422933002/diff/180001/chrome/renderer/resourc... chrome/renderer/resources/neterror.js:13: } nit: no curlies https://codereview.chromium.org/422933002/diff/180001/chrome/renderer/resourc... chrome/renderer/resources/neterror.js:98: if (window.errorPageController) { nit: no curlies
https://codereview.chromium.org/422933002/diff/160001/chrome/renderer/resourc... File chrome/renderer/resources/neterror.css (right): https://codereview.chromium.org/422933002/diff/160001/chrome/renderer/resourc... chrome/renderer/resources/neterror.css:52: margin: 10px 0 30px 0; On 2014/08/04 17:19:02, Dan Beam wrote: > 10px 0 30px; Done. https://codereview.chromium.org/422933002/diff/160001/chrome/renderer/resourc... chrome/renderer/resources/neterror.css:259: border-bottom-left-radius: 0; On 2014/08/04 17:19:02, Dan Beam wrote: > this is probably wrong in RTL Meaning my re-ordering of the CSS properties as per the presubmit check is wrong, or that border-bottom-left-radius: 0; is wrong? If the latter, I'd like to claim that since I didn't change it, it doesn't need to be fixed in this CL. https://codereview.chromium.org/422933002/diff/160001/chrome/renderer/resourc... chrome/renderer/resources/neterror.css:312: margin: 10px 0 15px 0; On 2014/08/04 17:19:02, Dan Beam wrote: > margin: 10px 0 15px; Done. https://codereview.chromium.org/422933002/diff/160001/chrome/renderer/resourc... chrome/renderer/resources/neterror.css:373: if they should float left or right. If there's only a details On 2014/08/04 17:19:02, Dan Beam wrote: > opt nit: 1\s between comments Sorry, I'm not understanding. One space between the two sentences in the comment? Or don't indent by three spaces at the beginning of the line to align with the line above? If your comment refers to the rewritten version of this comment on the next patchset about the two spaces between the period and the */, done. https://codereview.chromium.org/422933002/diff/160001/chrome/renderer/resourc... chrome/renderer/resources/neterror.css:374: button, it should be centered. */ On 2014/08/04 17:19:02, Dan Beam wrote: > On 2014/08/04 16:32:17, Bernhard Bauer wrote: > > On 2014/08/04 15:57:39, mmenke wrote: > > > I don't think I've seen this style used in Chrome. It's generally: > > > > > > /** > > > * Really, REALLY important stuff > > > * that takes up more than one line. > > > */ > > > > > > (Sometimes with an extra asterisk before the end comment tag, sometimes > > without > > > the extra asterisk on the first line, sometimes without the top line). > > > > I don't have a strong opinion on which exact style you should use, but I would > > rather not use the double-asterisk one, as that is generally used for semantic > > comments (Javadoc, JSDoc). I don't think there is an equivalent for CSS, but I > > wouldn't want to create the impression that it is supposed to be processed. > > cssdoc and closure stylesheets do parse > > /** > * A doc comment. > * @require {some.namespace} > */ > > comments. if you don't want this to be picked up by a doc tool, change to > > /* Some long comment > * that spans lines. */ > > like our copyright headers. the \s\* at the beginning of each line is solely > conventional, but one that's pretty consistent throughout webui. Moot given rewrite; I'll take this into account if I rewrite the comment again. https://codereview.chromium.org/422933002/diff/160001/chrome/renderer/resourc... chrome/renderer/resources/neterror.css:394: } On 2014/08/04 17:19:02, Dan Beam wrote: > why are you using float and a clearfix instead of flexbox + flexbox ordering (if > necessary)? Because I don't really know CSS (which you probably figured out by now :-}) and didn't realize that there were preferred and non-preferred ways to do this. This parallels chrome/browser/resources/ssl/interstitial_v2.css, several things I found on the net, and works. Is it important enough to use flexbox & flexbox ordering that I should climb the learning curve and switch to it? https://codereview.chromium.org/422933002/diff/160001/chrome/renderer/resourc... File chrome/renderer/resources/neterror.html (right): https://codereview.chromium.org/422933002/diff/160001/chrome/renderer/resourc... chrome/renderer/resources/neterror.html:38: onclick="detailsButtonClick(); toggleHelpBox()" On 2014/08/04 17:19:03, Dan Beam wrote: > onclick= :'( Was this a request for a change or an expression of sadness? :-} :-| https://codereview.chromium.org/422933002/diff/160001/chrome/renderer/resourc... File chrome/renderer/resources/neterror.js (right): https://codereview.chromium.org/422933002/diff/160001/chrome/renderer/resourc... chrome/renderer/resources/neterror.js:13: } On 2014/08/04 17:19:03, Dan Beam wrote: > nit: no curlies Done. https://codereview.chromium.org/422933002/diff/160001/chrome/renderer/resourc... chrome/renderer/resources/neterror.js:118: var primaryControlOnLeft = true; On 2014/08/04 17:19:03, Dan Beam wrote: > probably meant to remove this? Yeah, not sure how my (first round of :-}) testing didn't catch that. Done. https://codereview.chromium.org/422933002/diff/160001/chrome/renderer/resourc... chrome/renderer/resources/neterror.js:120: if (primaryControlOnLeft) { On 2014/08/04 17:19:03, Dan Beam wrote: > nit: > > <if expr="is_win> > buttons.classList.add('suggested-right'); > controlButtonDiv.insertBefore(secondaryButton, primaryButton); > </if> > <if expr="not is_win"> > buttons.classList.add('suggested-left'); > controlButtonDiv.insertBefore(primaryButton, secondaryButton); > </if> You're directly contradicting bauerb@'s request that I move the grit out of the JS. I don't suppose you'd be willing not to push on the point? If I have to wait for you guys to work out who's authoritative, I really don't think I'm landing this today, as I suspect he's home and possibly asleep. https://codereview.chromium.org/422933002/diff/180001/chrome/renderer/resourc... File chrome/renderer/resources/neterror.js (right): https://codereview.chromium.org/422933002/diff/180001/chrome/renderer/resourc... chrome/renderer/resources/neterror.js:13: } On 2014/08/04 17:19:03, Dan Beam wrote: > nit: no curlies Done. https://codereview.chromium.org/422933002/diff/180001/chrome/renderer/resourc... chrome/renderer/resources/neterror.js:98: if (window.errorPageController) { On 2014/08/04 17:19:03, Dan Beam wrote: > nit: no curlies Done.
https://codereview.chromium.org/422933002/diff/160001/chrome/renderer/resourc... File chrome/renderer/resources/neterror.js (right): https://codereview.chromium.org/422933002/diff/160001/chrome/renderer/resourc... chrome/renderer/resources/neterror.js:120: if (primaryControlOnLeft) { On 2014/08/04 18:05:27, rdsmith wrote: > On 2014/08/04 17:19:03, Dan Beam wrote: > > nit: > > > > <if expr="is_win> > > buttons.classList.add('suggested-right'); > > controlButtonDiv.insertBefore(secondaryButton, primaryButton); > > </if> > > <if expr="not is_win"> > > buttons.classList.add('suggested-left'); > > controlButtonDiv.insertBefore(primaryButton, secondaryButton); > > </if> > > You're directly contradicting bauerb@'s request that I move the grit out of the > JS. I don't suppose you'd be willing not to push on the point? If I have to > wait for you guys to work out who's authoritative, I really don't think I'm > landing this today, as I suspect he's home and possibly asleep. Not asleep, just having dinner :) To briefly reiterate my argument for Dan, I would rather do these decisions in Javascript (similar to how we define and check cr.isMac, cr.isWin etc. in Javascript) rather than <if>-ing them. Later we could replace the <if> definition with one at runtime and move it to a shared file. In the interest of not blocking this CL, I'm okay with landing either version though. I just wanted to say my piece :)
Would Matt or Adrienne (or anyone else, really, I just want a knowledgeable pair of eyes) like to review PS11->PS12? The basic problem was the Bernhard was right :-}; using an <a> element was badness. In this particular case, it messed up the navigation the NetErrorHelper was seeing, which resulted in reloading not working. Shifting that required shifting some styles around as well. I'm running try jobs after merging to TOT just to maximize the chance that using the CQ will actually work.
This LGTM, but I really don't know CSS well. https://codereview.chromium.org/422933002/diff/240001/chrome/renderer/resourc... File chrome/renderer/resources/neterror.css (right): https://codereview.chromium.org/422933002/diff/240001/chrome/renderer/resourc... chrome/renderer/resources/neterror.css:379: text-decoration: underline; 2-space indent. https://codereview.chromium.org/422933002/diff/240001/chrome/renderer/resourc... chrome/renderer/resources/neterror.css:384: padding-right: 20px; 2-space indent.
a few more minor comments https://codereview.chromium.org/422933002/diff/240001/chrome/renderer/resourc... File chrome/renderer/resources/neterror.css (right): https://codereview.chromium.org/422933002/diff/240001/chrome/renderer/resourc... chrome/renderer/resources/neterror.css:185: .active-text { It looks like active-text is no longer being used https://codereview.chromium.org/422933002/diff/240001/chrome/renderer/resourc... chrome/renderer/resources/neterror.css:377: min-width: 0; do you need to specify min-width: 0, or can you just leave it off?
Comments incorporated. https://codereview.chromium.org/422933002/diff/240001/chrome/renderer/resourc... File chrome/renderer/resources/neterror.css (right): https://codereview.chromium.org/422933002/diff/240001/chrome/renderer/resourc... chrome/renderer/resources/neterror.css:185: .active-text { On 2014/08/05 00:51:13, felt wrote: > It looks like active-text is no longer being used Done. https://codereview.chromium.org/422933002/diff/240001/chrome/renderer/resourc... chrome/renderer/resources/neterror.css:377: min-width: 0; On 2014/08/05 00:51:13, felt wrote: > do you need to specify min-width: 0, or can you just leave it off? I need to override the default min-width (specified in text-button; 65px) somehow. https://codereview.chromium.org/422933002/diff/240001/chrome/renderer/resourc... chrome/renderer/resources/neterror.css:379: text-decoration: underline; On 2014/08/05 00:20:13, mmenke wrote: > 2-space indent. Done. https://codereview.chromium.org/422933002/diff/240001/chrome/renderer/resourc... chrome/renderer/resources/neterror.css:384: padding-right: 20px; On 2014/08/05 00:20:13, mmenke wrote: > 2-space indent. Done.
On 2014/08/05 01:00:39, rdsmith wrote: > Comments incorporated. > > https://codereview.chromium.org/422933002/diff/240001/chrome/renderer/resourc... > File chrome/renderer/resources/neterror.css (right): > > https://codereview.chromium.org/422933002/diff/240001/chrome/renderer/resourc... > chrome/renderer/resources/neterror.css:185: .active-text { > On 2014/08/05 00:51:13, felt wrote: > > It looks like active-text is no longer being used > > Done. > > https://codereview.chromium.org/422933002/diff/240001/chrome/renderer/resourc... > chrome/renderer/resources/neterror.css:377: min-width: 0; > On 2014/08/05 00:51:13, felt wrote: > > do you need to specify min-width: 0, or can you just leave it off? > > I need to override the default min-width (specified in text-button; 65px) > somehow. > > https://codereview.chromium.org/422933002/diff/240001/chrome/renderer/resourc... > chrome/renderer/resources/neterror.css:379: text-decoration: underline; > On 2014/08/05 00:20:13, mmenke wrote: > > 2-space indent. > > Done. > > https://codereview.chromium.org/422933002/diff/240001/chrome/renderer/resourc... > chrome/renderer/resources/neterror.css:384: padding-right: 20px; > On 2014/08/05 00:20:13, mmenke wrote: > > 2-space indent. > > Done. changes from 11->14 lgtm
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/422933002/260001
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: win_gpu on tryserver.chromium.gpu (http://build.chromium.org/p/tryserver.chromium.gpu/builders/win_gpu/builds/42333) chromium_presubmit on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...) linux_chromium_chromeos_rel_swarming on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_rel_swarming on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
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/422933002/280001
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: linux_chromium_chromeos_rel_swarming on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_rel_swarming on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) mac_chromium_compile_dbg on tryserver.chromium.mac (http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) win_chromium_x64_rel on tryserver.chromium.win (http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_chromeos_rel_swarming on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
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/422933002/280001
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: linux_chromium_chromeos_rel_swarming on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_rel_swarming on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
Message was sent while issue was closed.
Change committed as 287538 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
