Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
13737002:
New network error pages: Only show an icon when in iframe (Closed)
|
Created:
7 years, 8 months ago by mmenke Modified:
7 years, 8 months ago CC:
chromium-reviews, cbentzel+watch_chromium.org, oshima+watch_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src/ Visibility:
Public. |
DescriptionNew network error pages: Only show an icon (And description
on mouse over) when a network error happens in an iframe.
Also add the same icon to the main frame error page.
BUG=174194
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=194509
Patch Set 1 : #Patch Set 2 : Slightly more consistent markup #Patch Set 3 : Moved icons to a separate CL #Patch Set 4 : sync #Patch Set 5 : Show details instead of error code #
Total comments: 4
Patch Set 6 : Response to comments, increase minimum size requited to show error text #Patch Set 7 : Add comment #
Total comments: 1
Patch Set 8 : sync #Patch Set 9 : sync #Patch Set 10 : Add todo #
Total comments: 8
Patch Set 11 : Response to comments (embed images, move subframe CSS) #
Total comments: 4
Patch Set 12 : Switch to flexbox #Messages
Total messages: 22 (0 generated)
Is this a reasonable way to add an icon to a page generated by the renderer process? The old solution embedded all the images in the HTML resource, which seemed to waste space and didn't allow changing the icon based on error code (Which this CL doesn't do, anyways, but seems like it could be useful).
tony: Ping!
[newt]: Mind reviewing another of these? Not sure about using a single HTML document for both main frame and subframes. The main advantage is that it's easier to plug in - this works on iOS without code changes, for instance. Would have to make iOS-specific changes to choose between two document.
this looks OK to me. I agree that putting two separate pages in the same html file is a bit odd, but it does guarantee this'll work on iOS. https://codereview.chromium.org/13737002/diff/42001/chrome/renderer/resources... File chrome/renderer/resources/neterror.html (right): https://codereview.chromium.org/13737002/diff/42001/chrome/renderer/resources... chrome/renderer/resources/neterror.html:17: #mainFrameError { this page is inconsistent about id/class naming style, but dash-form is preferred over camelCase: http://dev.chromium.org/developers/web-development-style-guide http://google-styleguide.googlecode.com/svn/trunk/htmlcssguide.xml?showone=ID... https://codereview.chromium.org/13737002/diff/42001/chrome/renderer/resources... chrome/renderer/resources/neterror.html:207: background-color: #EEE; is this needed? subFrameError already has background-color #EEE in the hover state
Thanks so much for the feedback! https://codereview.chromium.org/13737002/diff/42001/chrome/renderer/resources... File chrome/renderer/resources/neterror.html (right): https://codereview.chromium.org/13737002/diff/42001/chrome/renderer/resources... chrome/renderer/resources/neterror.html:17: #mainFrameError { On 2013/04/10 21:10:20, newt wrote: > this page is inconsistent about id/class naming style, but dash-form is > preferred over camelCase: > http://dev.chromium.org/developers/web-development-style-guide > http://google-styleguide.googlecode.com/svn/trunk/htmlcssguide.xml?showone=ID... The inconsistencies are my fault - it started out with camel case form, then I added dash form because it was preferred. I've switched to dash form everywhere except for failedUrl (Which is used in a number of strings). https://codereview.chromium.org/13737002/diff/42001/chrome/renderer/resources... chrome/renderer/resources/neterror.html:207: background-color: #EEE; On 2013/04/10 21:10:20, newt wrote: > is this needed? subFrameError already has background-color #EEE in the hover > state No it's not - thanks for catching that.
[+thakis]: Please review both files. I'm using a single file for both the subframe and mainframe case because it works better on iOS. If you think there should actually be two different html files for each case, and a separate followup iOS patch, I'll switch.
[newt]: Could you sign off on this? [thakis]: Ping! Looks like you may have a bit of a backlog. If I don't hear back from you today (Either a review, or saying you'll get to this tomorrow), I'll look for another reviewer. This has to get in for UI review this week.
lgtm with one nit https://codereview.chromium.org/13737002/diff/53001/chrome/renderer/resources... File chrome/renderer/resources/neterror.html (right): https://codereview.chromium.org/13737002/diff/53001/chrome/renderer/resources... chrome/renderer/resources/neterror.html:89: .failedUrl { remove this. looks like failedUrl isn't used in the html
On 2013/04/16 17:14:09, newt wrote: > lgtm with one nit > > https://codereview.chromium.org/13737002/diff/53001/chrome/renderer/resources... > File chrome/renderer/resources/neterror.html (right): > > https://codereview.chromium.org/13737002/diff/53001/chrome/renderer/resources... > chrome/renderer/resources/neterror.html:89: .failedUrl { > remove this. looks like failedUrl isn't used in the html failedUrl is used inside strings for specific error messages. It's both a template string name used as a class name - they have different naming conventions.
ah, lgtm then
Sorry for the delay, I was mostly afk for the last 4 days and I'm catching up on reviews. https://codereview.chromium.org/13737002/diff/67001/chrome/common/localized_e... File chrome/common/localized_error.cc (right): https://codereview.chromium.org/13737002/diff/67001/chrome/common/localized_e... chrome/common/localized_error.cc:661: IDR_ERROR_NETWORK_GENERIC, ui::GetMaxScaleFactor()), This is not correct. This needs to be a -webkit-image-set() with one image per scale factor. Can you just use a chrome://theme/RESOURCE_ID img src instead of lining the asset? Then you can use the @2x suffix too. If that's not an option for some reason, I think our html processor automatically inlines images into data urls if you put a relative path to a png into the html file, so there's no need to write code for this anyways. (Example: with a resource url: https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/res... (the resource packer will insert -webkit-image-set() automatically, and attach "@2x" for the hidpi version) Example with a relative path: https://code.google.com/p/chromium/codesearch#chromium/src/ui/webui/resources... I think here too the processor will include a -webkit-image-set() thing automatically if there's a png with the same name in a "2x" subfolder) https://codereview.chromium.org/13737002/diff/67001/chrome/renderer/resources... File chrome/renderer/resources/neterror.html (right): https://codereview.chromium.org/13737002/diff/67001/chrome/renderer/resources... chrome/renderer/resources/neterror.html:184: } Can you point me to the mock for error pages on subframes? I couldn't find it.
https://codereview.chromium.org/13737002/diff/67001/chrome/common/localized_e... File chrome/common/localized_error.cc (right): https://codereview.chromium.org/13737002/diff/67001/chrome/common/localized_e... chrome/common/localized_error.cc:661: IDR_ERROR_NETWORK_GENERIC, ui::GetMaxScaleFactor()), On 2013/04/16 23:15:11, Nico wrote: > This is not correct. This needs to be a -webkit-image-set() with one image per > scale factor. > > Can you just use a chrome://theme/RESOURCE_ID img src instead of lining the > asset? Then you can use the @2x suffix too. > > If that's not an option for some reason, I think our html processor > automatically inlines images into data urls if you put a relative path to a png > into the html file, so there's no need to write code for this anyways. > > (Example: with a resource url: > https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/res... > (the resource packer will insert -webkit-image-set() automatically, and attach > "@2x" for the hidpi version) > > Example with a relative path: > https://code.google.com/p/chromium/codesearch#chromium/src/ui/webui/resources... > I think here too the processor will include a -webkit-image-set() thing > automatically if there's a png with the same name in a "2x" subfolder) This is run in a generic renderer process, and does not have access to chrome:// URLs. Everything it uses has to be embedded in the page when it's loaded. We could embed resources of different sizes in the HTML, though I really don't like that solution. I'm open to other ideas. https://codereview.chromium.org/13737002/diff/67001/chrome/renderer/resources... File chrome/renderer/resources/neterror.html (right): https://codereview.chromium.org/13737002/diff/67001/chrome/renderer/resources... chrome/renderer/resources/neterror.html:184: } On 2013/04/16 23:15:11, Nico wrote: > Can you point me to the mock for error pages on subframes? I couldn't find it. The mocks are here: https://www.corp.google.com/~kenmoore/mocks/chrome/ErrorPages/Pass4/chrome-er... (There's a checkbox to switch to an iframe). I switched from the error code to the error description because the error code doesn't internationalize.
https://codereview.chromium.org/13737002/diff/67001/chrome/common/localized_e... File chrome/common/localized_error.cc (right): https://codereview.chromium.org/13737002/diff/67001/chrome/common/localized_e... chrome/common/localized_error.cc:661: IDR_ERROR_NETWORK_GENERIC, ui::GetMaxScaleFactor()), On 2013/04/16 23:23:02, mmenke wrote: > On 2013/04/16 23:15:11, Nico wrote: > > This is not correct. This needs to be a -webkit-image-set() with one image per > > scale factor. > > > > Can you just use a chrome://theme/RESOURCE_ID img src instead of lining the > > asset? Then you can use the @2x suffix too. > > > > If that's not an option for some reason, I think our html processor > > automatically inlines images into data urls if you put a relative path to a > png > > into the html file, so there's no need to write code for this anyways. > > > > (Example: with a resource url: > > > https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/res... > > (the resource packer will insert -webkit-image-set() automatically, and attach > > "@2x" for the hidpi version) > > > > Example with a relative path: > > > https://code.google.com/p/chromium/codesearch#chromium/src/ui/webui/resources... > > I think here too the processor will include a -webkit-image-set() thing > > automatically if there's a png with the same name in a "2x" subfolder) > > This is run in a generic renderer process, and does not have access to chrome:// > URLs. > > Everything it uses has to be embedded in the page when it's loaded. > > We could embed resources of different sizes in the HTML, though I really don't > like that solution. > > I'm open to other ideas. Sorry...Just skimmed your response. I'll go ahead and embed the images. They're small, It just seems wrong.
Since you don't have access to chrome:// urls, have you tried just putting a relative path to the png into the html file? That should include a data url for the png, for all scale factors the platform cares about. https://codereview.chromium.org/13737002/diff/67001/chrome/renderer/resources... File chrome/renderer/resources/neterror.html (right): https://codereview.chromium.org/13737002/diff/67001/chrome/renderer/resources... chrome/renderer/resources/neterror.html:184: } On 2013/04/16 23:23:02, mmenke wrote: > On 2013/04/16 23:15:11, Nico wrote: > > Can you point me to the mock for error pages on subframes? I couldn't find it. > > The mocks are here: > https://www.corp.google.com/%7Ekenmoore/mocks/chrome/ErrorPages/Pass4/chrome-... > (There's a checkbox to switch to an iframe). I switched from the error code to > the error description because the error code doesn't internationalize. Thanks. Does it make sense to use the same html file for mainframe and framed error pages? They look pretty different. If you want to keep it like you have, I'd probably move this css block next to the one at the top that hides the main frame UI.
On Tue, Apr 16, 2013 at 4:27 PM, <mmenke@chromium.org> wrote: > > https://codereview.chromium.**org/13737002/diff/67001/** > chrome/common/localized_error.**cc<https://codereview.chromium.org/13737002/diff/67001/chrome/common/localized_error.cc> > File chrome/common/localized_error.**cc (right): > > https://codereview.chromium.**org/13737002/diff/67001/** > chrome/common/localized_error.**cc#newcode661<https://codereview.chromium.org/13737002/diff/67001/chrome/common/localized_error.cc#newcode661> > chrome/common/localized_error.**cc:661: IDR_ERROR_NETWORK_GENERIC, > ui::GetMaxScaleFactor()), > On 2013/04/16 23:23:02, mmenke wrote: > >> On 2013/04/16 23:15:11, Nico wrote: >> > This is not correct. This needs to be a -webkit-image-set() with one >> > image per > >> > scale factor. >> > >> > Can you just use a chrome://theme/RESOURCE_ID img src instead of >> > lining the > >> > asset? Then you can use the @2x suffix too. >> > >> > If that's not an option for some reason, I think our html processor >> > automatically inlines images into data urls if you put a relative >> > path to a > >> png >> > into the html file, so there's no need to write code for this >> > anyways. > >> > >> > (Example: with a resource url: >> > >> > > https://code.google.com/p/**chromium/codesearch#chromium/** > src/chrome/browser/resources/**help/help.html&q=chrome:%** > 25255C/%25255C/theme%25255C/%**252520file:html&sq=package:** > chromium&type=cs&l=21<https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/resources/help/help.html&q=chrome:%25255C/%25255C/theme%25255C/%252520file:html&sq=package:chromium&type=cs&l=21> > > > (the resource packer will insert -webkit-image-set() automatically, >> > and attach > >> > "@2x" for the hidpi version) >> > >> > Example with a relative path: >> > >> > > https://code.google.com/p/**chromium/codesearch#chromium/** > src/ui/webui/resources/css/**widgets.css&q=url%25255C%2528%** > 252520file:webui&sq=package:**chromium&type=cs&l=56<https://code.google.com/p/chromium/codesearch#chromium/src/ui/webui/resources/css/widgets.css&q=url%25255C%2528%252520file:webui&sq=package:chromium&type=cs&l=56> > > > I think here too the processor will include a -webkit-image-set() >> > thing > >> > automatically if there's a png with the same name in a "2x" >> > subfolder) > > This is run in a generic renderer process, and does not have access to >> > chrome:// > >> URLs. >> > > Everything it uses has to be embedded in the page when it's loaded. >> > > We could embed resources of different sizes in the HTML, though I >> > really don't > >> like that solution. >> > > I'm open to other ideas. >> > > Sorry...Just skimmed your response. I'll go ahead and embed the images. > They're small, It just seems wrong. > The reasoning is that you can move a chromium window from a hidpi screen to a lodpi screen and back, and you want pixel-perfect assets on both displays. > > https://codereview.chromium.**org/13737002/<https://codereview.chromium.org/1... >
https://codereview.chromium.org/13737002/diff/67001/chrome/common/localized_e... File chrome/common/localized_error.cc (right): https://codereview.chromium.org/13737002/diff/67001/chrome/common/localized_e... chrome/common/localized_error.cc:661: IDR_ERROR_NETWORK_GENERIC, ui::GetMaxScaleFactor()), On 2013/04/16 23:27:18, mmenke wrote: > On 2013/04/16 23:23:02, mmenke wrote: > > On 2013/04/16 23:15:11, Nico wrote: > > > This is not correct. This needs to be a -webkit-image-set() with one image > per > > > scale factor. > > > > > > Can you just use a chrome://theme/RESOURCE_ID img src instead of lining the > > > asset? Then you can use the @2x suffix too. > > > > > > If that's not an option for some reason, I think our html processor > > > automatically inlines images into data urls if you put a relative path to a > > png > > > into the html file, so there's no need to write code for this anyways. > > > > > > (Example: with a resource url: > > > > > > https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/res... > > > (the resource packer will insert -webkit-image-set() automatically, and > attach > > > "@2x" for the hidpi version) > > > > > > Example with a relative path: > > > > > > https://code.google.com/p/chromium/codesearch#chromium/src/ui/webui/resources... > > > I think here too the processor will include a -webkit-image-set() thing > > > automatically if there's a png with the same name in a "2x" subfolder) > > > > This is run in a generic renderer process, and does not have access to > chrome:// > > URLs. > > > > Everything it uses has to be embedded in the page when it's loaded. > > > > We could embed resources of different sizes in the HTML, though I really don't > > like that solution. > > > > I'm open to other ideas. > > Sorry...Just skimmed your response. I'll go ahead and embed the images. > They're small, It just seems wrong. I've embedded the images. https://codereview.chromium.org/13737002/diff/67001/chrome/renderer/resources... File chrome/renderer/resources/neterror.html (right): https://codereview.chromium.org/13737002/diff/67001/chrome/renderer/resources... chrome/renderer/resources/neterror.html:184: } On 2013/04/16 23:27:20, Nico wrote: > On 2013/04/16 23:23:02, mmenke wrote: > > On 2013/04/16 23:15:11, Nico wrote: > > > Can you point me to the mock for error pages on subframes? I couldn't find > it. > > > > The mocks are here: > > > https://www.corp.google.com/%257Ekenmoore/mocks/chrome/ErrorPages/Pass4/chrom... > > (There's a checkbox to switch to an iframe). I switched from the error code > to > > the error description because the error code doesn't internationalize. > > Thanks. Does it make sense to use the same html file for mainframe and framed > error pages? They look pretty different. > > If you want to keep it like you have, I'd probably move this css block next to > the one at the top that hides the main frame UI. The main reason I was thinking a single file is the way to go is because iOS shares localized_error with everything else, but it's used from a different file that separately references this one. I'm a bit worried about regressions/iOS breakages due to this fact, and thought a single file would make problems less likely. If you disagree, I'm happy to split it into two files and land a separate iOS change once they roll Chrome to a recent enough version. Right now, I've just moved the CSS block up to the top.
Oh, and no worries about your review speed. Was just concerned you may be OOO.
If you think having a single file is better, then let's do that. lgtm. https://codereview.chromium.org/13737002/diff/74001/chrome/renderer/resources... File chrome/renderer/resources/neterror.html (right): https://codereview.chromium.org/13737002/diff/74001/chrome/renderer/resources... chrome/renderer/resources/neterror.html:190: overflow: hidden; Do iframes really leak their content without this? https://codereview.chromium.org/13737002/diff/74001/chrome/renderer/resources... chrome/renderer/resources/neterror.html:200: vertical-align: middle; Does this do anything? I thought having it on the cell is enough. (nit: Using flexbox instead of display: table might be a somewhat nicer way to do this UI.)
https://codereview.chromium.org/13737002/diff/74001/chrome/renderer/resources... File chrome/renderer/resources/neterror.html (right): https://codereview.chromium.org/13737002/diff/74001/chrome/renderer/resources... chrome/renderer/resources/neterror.html:190: overflow: hidden; On 2013/04/17 00:03:08, Nico wrote: > Do iframes really leak their content without this? No. They can show scroll bars, though, if things unexpectedly don't quite fit. https://codereview.chromium.org/13737002/diff/74001/chrome/renderer/resources... chrome/renderer/resources/neterror.html:200: vertical-align: middle; On 2013/04/17 00:03:08, Nico wrote: > Does this do anything? I thought having it on the cell is enough. > > (nit: Using flexbox instead of display: table might be a somewhat nicer way to > do this UI.) You're right that it's not needed. I've gone ahead and switched to a flexbox.
lgtm!
On 2013/04/17 00:37:11, Nico wrote: > lgtm! Thanks so much for all the reviews (And the useful feedback)!
Message was sent while issue was closed.
Committed patchset #12 manually as r194509 (presubmit successful). |
